From bec77a2c669ee4957f5a5d0740129e7a7d30e9e8 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 9 Sep 2021 14:33:30 +0100 Subject: [PATCH 1/7] Bump gulp-sass Intended to deal with this security vulnerability: https://github.blog/2021-09-08-github-security-update-vulnerabilities-tar-npmcli-arborist/ Bumping gulp-sass to version 5 removes its dependency on the tar package mentioned in that article. Version 5 requires you to specify a compiler directly in the gulpfile so that code is changed in line with this guidance: https://github.com/dlmanning/gulp-sass/tree/master#migrating-to-version-5 Note: node-sass is now deprecated so this also changes the sass compiler gulp-sass uses to dart-sass (aka 'sass'), the compiler now recommended by the Sass project: https://sass-lang.com/dart-sass This also bumps gulp and all its plugin modules to their latest versions, for parity. --- gulpfile.js | 5 ++--- package.json | 11 ++++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/gulpfile.js b/gulpfile.js index 35b51964e..eb178018f 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -20,7 +20,7 @@ plugins.cssUrlAdjuster = require('gulp-css-url-adjuster'); plugins.jshint = require('gulp-jshint'); plugins.prettyerror = require('gulp-prettyerror'); plugins.rollup = require('gulp-better-rollup') -plugins.sass = require('gulp-sass'); +plugins.sass = require('gulp-sass')(require('sass')); plugins.sassLint = require('gulp-sass-lint'); plugins.uglify = require('gulp-uglify'); @@ -207,8 +207,7 @@ const sass = () => { paths.src + '/stylesheets/print.scss' ]) .pipe(plugins.prettyerror()) - .pipe(plugins.sass({ - outputStyle: 'nested', + .pipe(plugins.sass.sync({ includePaths: [ paths.npm + 'govuk-elements-sass/public/sass/', paths.toolkit + 'stylesheets/', diff --git a/package.json b/package.json index e062a4ee1..8a935626a 100644 --- a/package.json +++ b/package.json @@ -27,20 +27,21 @@ "govuk_frontend_toolkit": "8.1.0", "govuk-elements-sass": "3.1.2", "govuk-frontend": "2.13.0", - "gulp": "4.0.0", + "gulp": "4.0.2", "gulp-add-src": "1.0.0", "gulp-babel": "8.0.0", "gulp-better-rollup": "4.0.1", - "gulp-clean-css": "4.2.0", + "gulp-clean-css": "4.3.0", "gulp-concat": "2.6.1", - "gulp-include": "2.3.1", - "gulp-sass": "4.0.2", + "gulp-include": "2.4.1", + "gulp-sass": "5.0.0", "gulp-uglify": "3.0.2", "hogan": "1.0.2", "jquery": "3.5.0", "leaflet": "1.6.0", "query-command-supported": "1.0.0", "rollup": "1.23.1", + "sass": "1.32.7", "streamqueue": "1.1.2", "textarea-caret": "3.1.0", "timeago": "1.6.5" @@ -48,7 +49,7 @@ "devDependencies": { "gulp-css-url-adjuster": "0.2.3", "gulp-jshint": "2.1.0", - "gulp-prettyerror": "1.2.1", + "gulp-prettyerror": "2.0.0", "gulp-sass-lint": "1.4.0", "jest": "24.7.1", "jest-date-mock": "^1.0.8", From 87f54d1e886f5c45ef7f12d88ef988c3f9343641 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 14 Sep 2021 15:01:07 +0100 Subject: [PATCH 2/7] Replace diffDOM library with domdiff A while ago diffDOM moved its code to use ES6 modules and started using various language features specific to ES6. These two things happened independently btw. The result of this is that the version of diffDOM suitable for our build pipeline, structured as an immediately invoked function evocation (IIFE), now requires polyfills of some ES6 features to work in the older browsers we support, like IE11. It's also worth noting that in the move to ES6 the maintainers of diffDOM have adopted a process whereby users who need to support older browsers now have to add polyfill code for any ES6 features they choose to use. This commmit proposes a move to the domdiff library instead because: - it runs on all javascript runtimes with no polyfills - it is 2KB instead of diffDOM's 25KB Domdiff takes a different approach to diffDOM, in that it compares existing nodes and new nodes and replaces the existing ones with the new ones if there are differences. By contrast, diffDOM will make in-place changes to nodes if there are enough similarities. In other words, in most situations, diffDOM won't change the node in $component whereas domdiff will. Because of this, I've had to change the updateContent.js code to cache the data-key attribute's value so we don't lose access to it by overwrite the $component variable with a different jQuery selection. --- app/assets/javascripts/updateContent.js | 17 ++++++++++++----- gulpfile.js | 2 +- package.json | 2 +- tests/javascripts/updateContent.test.js | 8 +++++--- 4 files changed, 19 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/updateContent.js b/app/assets/javascripts/updateContent.js index b607e77df..85cfa61e5 100644 --- a/app/assets/javascripts/updateContent.js +++ b/app/assets/javascripts/updateContent.js @@ -2,7 +2,6 @@ "use strict"; var queues = {}; - var dd = new global.diffDOM(); var defaultInterval = 2000; var interval = 0; @@ -11,10 +10,18 @@ 1000 )); - var getRenderer = $component => response => dd.apply( - $component.get(0), - dd.diff($component.get(0), $(response[$component.data('key')]).get(0)) - ); + var getRenderer = $component => { + var key = $component.data('key'); // use closure to retain key when component is replaced + return response => { + $component = $( + global.domdiff( + $component.parent().get(0), + [$component.get(0)], + [$(response[key]).get(0)] + )[0] + ); + }; + }; var getQueue = resource => ( queues[resource] = queues[resource] || [] diff --git a/gulpfile.js b/gulpfile.js index eb178018f..68964ea9d 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -146,7 +146,7 @@ const javascripts = () => { paths.npm + 'hogan.js/dist/hogan-3.0.2.js', paths.npm + 'jquery/dist/jquery.min.js', paths.npm + 'query-command-supported/dist/queryCommandSupported.min.js', - paths.npm + 'diff-dom/diffDOM.js', + paths.npm + 'domdiff/min.js', paths.npm + 'timeago/jquery.timeago.js', paths.npm + 'textarea-caret/index.js', paths.npm + 'cbor-js/cbor.js' diff --git a/package.json b/package.json index 8a935626a..93b86a45d 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,7 @@ "@babel/preset-env": "7.4.2", "cbor-js": "0.1.0", "del": "5.1.0", - "diff-dom": "2.5.1", + "domdiff": "2.2.2", "govuk_frontend_toolkit": "8.1.0", "govuk-elements-sass": "3.1.2", "govuk-frontend": "2.13.0", diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index 2d5a36c66..48f77a60e 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -31,9 +31,11 @@ beforeAll(() => { $.ajax.mockImplementation(() => jqueryAJAXReturnObj); - // because we're running in node, diffDOM executes as a module - // in the normal browser environment it will attach to window so we replicate that here - window.diffDOM = require('../../node_modules/diff-dom/diffDOM.js'); + // because we're running in node, we can't execute the normal domdiff code like a browser because + // its variables (actually only one: domdiff) stay local to it + // in the normal browser environment it will attach to window so we replicate that here by using + // the CommonJS module version and assigning it to window + window.domdiff = require('domdiff/cjs').default; require('../../app/assets/javascripts/updateContent.js'); }); From 55287e944d9e0f224db6aaeb060da667c36b9451 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 14 Sep 2021 16:57:52 +0100 Subject: [PATCH 3/7] Update updateContent tests to reflect its use The way we're using the updateContent.js code is slightly different to expected and to the scenarios in our tests. This changes the tests to match that use. The expected behaviour was for updates to a module's HTML to happen to the HTML inside of the div[data-module=update-content] element. So with initial HTML of:
Existing content
...should be updated to be:
New content
Instead the HTML returned by the AJAX requests replaced the div[data-module=update-content] element. So with initial HTML of:
Existing content
...will be updated to be:
New content
This doesn't seem to create any noticable changes to the visual interface so, I think, went unnoticed. The assumption I am making, of this being unintended, is based on the fact that the div[data-module=update-content] element has an aria-live attribute, which authors would normally want to stay in the page when updates happen. Note: This commit doesn't try and fix the problem, as the behaviour still largely works and the lack of aria-live actually seems to be a positive thing, meaning non-visual users aren't told of every update but can discover it themselves if needed. --- tests/javascripts/updateContent.test.js | 82 +++++++++++++------------ 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index 48f77a60e..0214b64b1 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -46,52 +46,58 @@ afterAll(() => { describe('Update content', () => { + let HTMLString; + let initialHTMLString; + beforeEach(() => { // store HTML in string to allow use in AJAX responses HTMLString = ` -
-
-
`; - document.body.innerHTML = HTMLString; - // default the response to match the existing content + initialHTMLString = `
+ ${HTMLString} +
`; + + document.body.innerHTML = initialHTMLString; + + // default the response to match the content inside div[data-module] responseObj[updateKey] = HTMLString; }); From aeaa96124cd9c93501dc0b2d4baa58dcc57a95cc Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 14 Sep 2021 20:52:02 +0100 Subject: [PATCH 4/7] Fix node version & lock down npm version The intention behind the version of node in the engines property was for that version to be the minimum required so it was always missing the `>=` prefix. This adds that prefix and also adds a setting for npm, to prevent use of insecure versions. See this article for details: https://github.blog/2021-09-08-github-security-update-vulnerabilities-tar-npmcli-arborist/ --- package.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/package.json b/package.json index 93b86a45d..c1efc35c1 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,8 @@ "version": "0.0.1", "description": "Admin front end for GOV.UK Notify", "engines": { - "node": "10.15.3" + "node": ">=10.15.3", + "npm": "6.14.15 || >= 7.21.0" }, "scripts": { "test": "gulp lint && jest --config tests/javascripts/jest.config.js tests/javascripts", From c61698753f23f58ab893e5a13d4428ad8471fe06 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 15 Sep 2021 16:24:01 +0100 Subject: [PATCH 5/7] Remove version restrictions for NPM We do need NPM to be run above those versions but I'd rather enforce that here after I'm sure this app will run on images that have a valid version. --- package.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/package.json b/package.json index c1efc35c1..726b41cfd 100644 --- a/package.json +++ b/package.json @@ -3,8 +3,7 @@ "version": "0.0.1", "description": "Admin front end for GOV.UK Notify", "engines": { - "node": ">=10.15.3", - "npm": "6.14.15 || >= 7.21.0" + "node": ">=10.15.3" }, "scripts": { "test": "gulp lint && jest --config tests/javascripts/jest.config.js tests/javascripts", From 5115344927af30f06dcd91f3a0e65abeb399fdbf Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Fri, 17 Sep 2021 15:55:05 +0100 Subject: [PATCH 6/7] Remove rebuild of node-sass from bootstrap We now use `sass`, a JavaScript version of Sass, compiled from dart-sass*, so shouldn't need to rebuild it to solve issues with C libraries. *https://github.com/sass/dart-sass --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 2ff1b1ed6..484f78867 100644 --- a/Makefile +++ b/Makefile @@ -25,7 +25,7 @@ VIRTUALENV_ROOT := $(shell [ -z $$VIRTUAL_ENV ] && echo $$(pwd)/venv || echo $$V .PHONY: bootstrap bootstrap: generate-version-file ## Set up everything to run the app pip3 install -r requirements_for_test.txt - npm install && npm rebuild node-sass && npm run build + npm install && npm run build .PHONY: run-flask run-flask: ## Run flask From 37ae04f67eac2b27587b832a357cea4936621dab Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Tue, 21 Sep 2021 20:18:24 +0100 Subject: [PATCH 7/7] Make test comments explain use of require better Based on comments on the pull request associated with this work: https://github.com/alphagov/notifications-admin/pull/4023#discussion_r711156154 --- tests/javascripts/updateContent.test.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/javascripts/updateContent.test.js b/tests/javascripts/updateContent.test.js index 0214b64b1..c1496b4b7 100644 --- a/tests/javascripts/updateContent.test.js +++ b/tests/javascripts/updateContent.test.js @@ -31,10 +31,12 @@ beforeAll(() => { $.ajax.mockImplementation(() => jqueryAJAXReturnObj); - // because we're running in node, we can't execute the normal domdiff code like a browser because - // its variables (actually only one: domdiff) stay local to it - // in the normal browser environment it will attach to window so we replicate that here by using - // the CommonJS module version and assigning it to window + // using require to execute the version we use in our our frontend build here can't add + // the domdiff variable to this scope like it does when executed in browsers because + // that version doesn't export it + // we use CommonJS version instead because it does (as the default property) + // see https://nodejs.org/en/knowledge/getting-started/what-is-require/ for more info + // also, we're not a browser so we need to manually attach domdiff to window window.domdiff = require('domdiff/cjs').default; require('../../app/assets/javascripts/updateContent.js');