From 1acf5b3a14b38f0f5a43535898328f7ceeb4e293 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Thu, 5 Oct 2023 14:30:31 +0100 Subject: [PATCH 1/8] Ensure GOV.UK Frontend config fields are type safe GOV.UK Frontend `nunjucksPaths` and `sass` config fields may potentially be `string | string[]` values one day --- lib/build.js | 2 +- lib/errorServer.js | 2 +- lib/govukFrontendPaths.js | 19 +++++++++++++++---- server.js | 2 +- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/lib/build.js b/lib/build.js index c98f2cd501..283b811e48 100644 --- a/lib/build.js +++ b/lib/build.js @@ -96,7 +96,7 @@ function sassKitFrontendDependency () { const govukFrontendInternal = govukFrontendPaths([packageDir, projectDir]) // Get GOV.UK Frontend (internal) stylesheets - const govukFrontendSass = (govukFrontendInternal.config?.sass || []) + const govukFrontendSass = [govukFrontendInternal.config.sass].flat() .map(sassPath => path.join(govukFrontendInternal.baseDir, sassPath)) const fileContents = sassVariables('/manage-prototype/dependencies') + diff --git a/lib/errorServer.js b/lib/errorServer.js index 1c152b6167..1016113365 100644 --- a/lib/errorServer.js +++ b/lib/errorServer.js @@ -103,7 +103,7 @@ function runErrorServer (error) { res.writeHead(500) // Get GOV.UK Frontend (internal) views - const govukFrontendNunjucksPaths = (govukFrontendInternal.config?.nunjucksPaths || []) + const govukFrontendNunjucksPaths = [govukFrontendInternal.config.nunjucksPaths].flat() .map(nunjucksPath => path.join(govukFrontendInternal.baseDir, nunjucksPath)) const fileContentsParts = [] diff --git a/lib/govukFrontendPaths.js b/lib/govukFrontendPaths.js index a35f96ea11..000ba2f51e 100644 --- a/lib/govukFrontendPaths.js +++ b/lib/govukFrontendPaths.js @@ -8,7 +8,7 @@ const fse = require('fs-extra') * Find GOV.UK Frontend via search paths * * @param {string[]} searchPaths - Search paths for `require.resolve()` - * @returns {{ baseDir: string, includePath: string, assetPath: string, config: { [key: string]: unknown } }} + * @returns {GOVUKFrontendPaths} */ function govukFrontendPaths (searchPaths = []) { /** @@ -29,12 +29,23 @@ function govukFrontendPaths (searchPaths = []) { assetPath: `/${path.relative(baseDir, path.join(includeDir, 'assets'))}`, // GOV.UK Frontend plugin config - config: fse.readJsonSync(path.join(baseDir, 'govuk-prototype-kit.config.json'), { - throws: false - }) + config: fse.readJsonSync(path.join(baseDir, 'govuk-prototype-kit.config.json'), { throws: false }) ?? { + nunjucksPaths: [], + sass: [] + } } } module.exports = { govukFrontendPaths } + +/** + * GOV.UK Frontend paths object + * + * @typedef {object} GOVUKFrontendPaths + * @property {string} baseDir - GOV.UK Frontend directory path + * @property {URL["pathname"]} includePath - URL path to GOV.UK Frontend includes + * @property {URL["pathname"]} assetPath - URL path to GOV.UK Frontend assets + * @property {{ [key: string]: unknown }} config - GOV.UK Frontend plugin config + */ diff --git a/server.js b/server.js index 71fd7db979..108b66afd5 100644 --- a/server.js +++ b/server.js @@ -81,7 +81,7 @@ app.use(cookieParser()) app.use(require('./lib/authentication.js')()) // Get GOV.UK Frontend (internal) views -const govukFrontendNunjucksPaths = (govukFrontendInternal.config?.nunjucksPaths || []) +const govukFrontendNunjucksPaths = [govukFrontendInternal.config.nunjucksPaths].flat() .map(nunjucksPath => path.join(govukFrontendInternal.baseDir, nunjucksPath)) // Set up App From 9a0f669f16b3db1802e2834f386ca4c7ea785a68 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Thu, 5 Oct 2023 18:36:28 +0100 Subject: [PATCH 2/8] =?UTF-8?q?Remove=20GOV.UK=20Frontend=20=E2=80=9Cinter?= =?UTF-8?q?nal=E2=80=9D=20views=20from=20server?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prototype pages should use the “plugin” version by default although we’ll add an improved way to provide a backup version should the plugin be uninstalled --- server.js | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/server.js b/server.js index 108b66afd5..efe1851ab5 100644 --- a/server.js +++ b/server.js @@ -16,7 +16,7 @@ const { expressNunjucks, getNunjucksAppEnv, stopWatchingNunjucks } = require('./ dotenv.config() // Local dependencies -const { projectDir, packageDir, finalBackupNunjucksDir } = require('./lib/utils/paths') +const { projectDir, packageDir, appViewsDir, finalBackupNunjucksDir } = require('./lib/utils/paths') const config = require('./lib/config.js').getConfig() const packageJson = require('./package.json') const { govukFrontendPaths } = require('./lib/govukFrontendPaths') @@ -80,18 +80,6 @@ app.use(cookieParser()) // static assets to prevent unauthorised access app.use(require('./lib/authentication.js')()) -// Get GOV.UK Frontend (internal) views -const govukFrontendNunjucksPaths = [govukFrontendInternal.config.nunjucksPaths].flat() - .map(nunjucksPath => path.join(govukFrontendInternal.baseDir, nunjucksPath)) - -// Set up App -const appViews = [ - path.join(projectDir, '/app/views/') -].concat(plugins.getAppViews([ - ...govukFrontendNunjucksPaths, - finalBackupNunjucksDir -])) - const nunjucksConfig = { autoescape: true, noCache: true, @@ -104,7 +92,9 @@ if (config.isDevelopment) { nunjucksConfig.express = app -const nunjucksAppEnv = getNunjucksAppEnv(appViews) +const nunjucksAppEnv = getNunjucksAppEnv( + plugins.getAppViews([appViewsDir, finalBackupNunjucksDir]) +) expressNunjucks(nunjucksAppEnv, app) From bb1cebbd5c689942da15d5bc658487e828d97ce8 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 6 Oct 2023 08:58:51 +0100 Subject: [PATCH 3/8] Support backup GOV.UK Frontend if plugin is uninstalled Adds an option param to `getNunjucksAppEnv()` to append backup view directories --- lib/errorServer.js | 12 ++++------- lib/nunjucks/nunjucksConfiguration.js | 31 ++++++++++++++++++++++++--- server.js | 5 ++++- 3 files changed, 36 insertions(+), 12 deletions(-) diff --git a/lib/errorServer.js b/lib/errorServer.js index 1016113365..c5e48e3c83 100644 --- a/lib/errorServer.js +++ b/lib/errorServer.js @@ -102,17 +102,13 @@ function runErrorServer (error) { res.setHeader('Content-Type', 'text/html') res.writeHead(500) - // Get GOV.UK Frontend (internal) views - const govukFrontendNunjucksPaths = [govukFrontendInternal.config.nunjucksPaths].flat() - .map(nunjucksPath => path.join(govukFrontendInternal.baseDir, nunjucksPath)) - const fileContentsParts = [] try { - const nunjucksAppEnv = getNunjucksAppEnv([ - path.join(__dirname, 'nunjucks'), - ...govukFrontendNunjucksPaths - ]) + const nunjucksAppEnv = getNunjucksAppEnv( + [path.join(__dirname, 'nunjucks')], + govukFrontendInternal // Add GOV.UK Frontend paths to Nunjucks views + ) res.end(nunjucksAppEnv.render('views/error-handling/server-error', { govukFrontendInternal, // Add GOV.UK Frontend paths to Nunjucks context ...getErrorModel(error) diff --git a/lib/nunjucks/nunjucksConfiguration.js b/lib/nunjucks/nunjucksConfiguration.js index 7dda1fbc06..4541b1eeae 100644 --- a/lib/nunjucks/nunjucksConfiguration.js +++ b/lib/nunjucks/nunjucksConfiguration.js @@ -1,11 +1,32 @@ const path = require('path') -const nunjucks = require('nunjucks') +const { Environment } = require('nunjucks') const NunjucksLoader = require('./nunjucksLoader') const { stopWatchingNunjucks } = NunjucksLoader const { startPerformanceTimer, endPerformanceTimer } = require('../utils/performance') -function getNunjucksAppEnv (appViews) { - return new nunjucks.Environment(new NunjucksLoader(appViews)) +/** + * Create Nunjucks environment + * + * Provide an optional GOV.UK Frontend paths object to append + * backup view directories if the plugin version is uninstalled + * + * @param {string[]} appViews + * @param {GOVUKFrontendPaths} [govukFrontend] + * @returns {import('nunjucks').Environment} + */ +function getNunjucksAppEnv (appViews, govukFrontend) { + const nunjucksViews = [...appViews] + + if (govukFrontend) { + const { baseDir, config } = govukFrontend + + // Combine with backup GOV.UK Frontend views + nunjucksViews.push(...[config.nunjucksPaths].flat() + .map(nunjucksPath => path.join(baseDir, nunjucksPath)) + ) + } + + return new Environment(new NunjucksLoader(nunjucksViews)) } function expressNunjucks (env, app) { @@ -33,3 +54,7 @@ function expressNunjucks (env, app) { } module.exports = { NunjucksLoader, getNunjucksAppEnv, expressNunjucks, stopWatchingNunjucks } + +/** + * @typedef {import('../govukFrontendPaths').GOVUKFrontendPaths} GOVUKFrontendPaths + */ diff --git a/server.js b/server.js index efe1851ab5..b801d1a0c5 100644 --- a/server.js +++ b/server.js @@ -92,8 +92,11 @@ if (config.isDevelopment) { nunjucksConfig.express = app +// Finds GOV.UK Frontend via `getAppViews()` only if installed +// but uses the internal package as a backup if uninstalled const nunjucksAppEnv = getNunjucksAppEnv( - plugins.getAppViews([appViewsDir, finalBackupNunjucksDir]) + plugins.getAppViews([appViewsDir, finalBackupNunjucksDir]), + govukFrontendInternal ) expressNunjucks(nunjucksAppEnv, app) From 045496f54da9b52714ba1082462efd5bd1f8182e Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 6 Oct 2023 17:14:30 +0100 Subject: [PATCH 4/8] Ensure Nunjucks render is mocked before `require()` Jest uses Babel to hoist `jest.mock()` calls above `require()` already But this change avoids needing a second call to update the mock implementation in a `beforeEach()` when it would be too late to do so --- lib/manage-prototype-handlers.test.js | 42 ++++++++++++++------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/lib/manage-prototype-handlers.test.js b/lib/manage-prototype-handlers.test.js index b2b3b010b6..4b1ea81f30 100644 --- a/lib/manage-prototype-handlers.test.js +++ b/lib/manage-prototype-handlers.test.js @@ -5,7 +5,6 @@ const path = require('path') // npm dependencies const fse = require('fs-extra') -const nunjucksConfiguration = require('./nunjucks/nunjucksConfiguration') // local dependencies const config = require('./config') @@ -16,6 +15,17 @@ const packages = require('./plugins/packages') const projectPackage = require('../package.json') const knownPlugins = require('../known-plugins.json') +const mockNunjucksRender = jest.fn() +const mockNunjucksAppEnv = jest.fn(() => ({ + render: mockNunjucksRender +})) + +// Avoid hoisting with `jest.doMock()` to ensure +// Nunjucks render + environment mocks stay in scope +jest.doMock('./nunjucks/nunjucksConfiguration', () => ({ + getNunjucksAppEnv: mockNunjucksAppEnv +})) + const { setKitRestarted, getPasswordHandler, @@ -61,13 +71,6 @@ jest.mock('fs-extra', () => { readJsonSync: jest.fn().mockReturnValue({}) } }) -jest.mock('./nunjucks/nunjucksConfiguration', () => { - return { - getNunjucksAppEnv: jest.fn().mockImplementation(() => ({ - render: jest.fn() - })) - } -}) jest.mock('./utils', () => { return { encryptPassword: jest.fn().mockReturnValue('encrypted password') @@ -138,7 +141,8 @@ describe('manage-prototype-handlers', () => { } res = { render: jest.fn(), - redirect: jest.fn() + redirect: jest.fn(), + send: jest.fn() } next = jest.fn() }) @@ -228,16 +232,12 @@ describe('manage-prototype-handlers', () => { const pluginDisplayName = { name: 'Test Package' } const templatePath = '/template' const encodedTemplatePath = encodeURIComponent(templatePath) - const view = 'Test View' const chosenUrl = '/chosen-url' - let mockSend beforeEach(() => { - mockSend = jest.fn() - res.status = jest.fn().mockReturnValue({ send: mockSend }) + res.status = jest.fn().mockReturnValue({ send: res.send }) req.query.package = packageName req.query.template = templatePath - res.send = mockSend plugins.getByType.mockReturnValue([{ packageName, item: { @@ -246,9 +246,6 @@ describe('manage-prototype-handlers', () => { path: templatePath } }]) - nunjucksConfiguration.getNunjucksAppEnv.mockImplementation(() => ({ - render: () => view - })) }) it('getTemplatesHandler', async () => { @@ -275,14 +272,19 @@ describe('manage-prototype-handlers', () => { it('template found', async () => { await getTemplatesViewHandler(req, res) expect(res.status).not.toHaveBeenCalled() - expect(mockSend).toHaveBeenCalledWith(view) + expect(mockNunjucksRender).toHaveBeenCalledWith( + path.join(packageName, templatePath), + expect.objectContaining({ + serviceName: 'Service name goes here' + }) + ) }) it('template not found', async () => { plugins.getByType.mockReturnValue([]) await getTemplatesViewHandler(req, res) expect(res.status).toHaveBeenCalledWith(404) - expect(mockSend).toHaveBeenCalledWith('Template not found.') + expect(res.send).toHaveBeenCalledWith('Template not found.') }) }) @@ -331,7 +333,7 @@ describe('manage-prototype-handlers', () => { plugins.getByType.mockReturnValue([]) await getTemplatesInstallHandler(req, res) expect(res.status).toHaveBeenCalledWith(404) - expect(mockSend).toHaveBeenCalledWith('Template not found.') + expect(res.send).toHaveBeenCalledWith('Template not found.') }) describe('postTemplatesInstallHandler', () => { From 12c107c6203fd12fcca7870982f6953180bcd194 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 6 Oct 2023 17:02:34 +0100 Subject: [PATCH 5/8] Configure Manage prototype Nunjucks environment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This prevents Manage prototype pages using “plugin” GOV.UK Frontend Unlike other management pages, the handler `getTemplatesViewHandler()` will still pick up the GOV.UK Frontend “plugin” version --- lib/manage-prototype-handlers.js | 38 +++++++++++++++++---------- lib/manage-prototype-handlers.test.js | 22 ++++++++-------- 2 files changed, 35 insertions(+), 25 deletions(-) diff --git a/lib/manage-prototype-handlers.js b/lib/manage-prototype-handlers.js index 8c5b50c3be..967490d6aa 100644 --- a/lib/manage-prototype-handlers.js +++ b/lib/manage-prototype-handlers.js @@ -10,6 +10,7 @@ const { doubleCsrf } = require('csrf-csrf') const config = require('./config') const plugins = require('./plugins/plugins') const { exec } = require('./exec') +const { govukFrontendPaths } = require('./govukFrontendPaths') const { prototypeAppScripts } = require('./utils') const { projectDir, packageDir, appViewsDir } = require('./utils/paths') const nunjucksConfiguration = require('./nunjucks/nunjucksConfiguration') @@ -31,6 +32,13 @@ const appViews = plugins.getAppViews([ path.join(packageDir, 'lib/final-backup-nunjucks') ]) +// Nunjucks environment for management pages skips `getAppViews()` to +// avoid plugins but adds GOV.UK Frontend views via internal package +const nunjucksManagementEnv = nunjucksConfiguration.getNunjucksAppEnv( + [path.join(__dirname, 'nunjucks')], + govukFrontendPaths([packageDir, projectDir]) +) + let kitRestarted = false const { @@ -83,19 +91,19 @@ function getCsrfTokenHandler (req, res) { // Clear all data in session function getClearDataHandler (req, res) { - res.render(getManagementView('clear-data.njk')) + res.send(nunjucksManagementEnv.render(getManagementView('clear-data.njk'))) } function postClearDataHandler (req, res) { req.session.data = {} - res.render(getManagementView('clear-data-success.njk')) + res.send(nunjucksManagementEnv.render(getManagementView('clear-data-success.njk'))) } // Render password page with a returnURL to redirect people to where they came from function getPasswordHandler (req, res) { const returnURL = req.query.returnURL || '/' const error = req.query.error - res.render(getManagementView('password.njk'), { returnURL, error }) + res.send(nunjucksManagementEnv.render(getManagementView('password.njk'), { returnURL, error })) } // Check authentication password @@ -125,7 +133,7 @@ function developmentOnlyMiddleware (req, res, next) { if (config.getConfig().isDevelopment || req.url.startsWith('/dependencies/govuk-frontend')) { next() } else { - res.render(getManagementView('manage-prototype-not-available.njk')) + res.send(nunjucksManagementEnv.render(getManagementView('manage-prototype-not-available.njk'))) } } @@ -189,7 +197,7 @@ async function getHomeHandler (req, res) { ] } - res.render(getManagementView('index.njk'), viewData) + res.send(nunjucksManagementEnv.render(getManagementView('index.njk'), viewData)) } function exampleTemplateConfig (packageName, { name, path }) { @@ -242,12 +250,12 @@ async function getTemplatesHandler (req, res) { } } - res.render(getManagementView('templates.njk'), { + res.send(nunjucksManagementEnv.render(getManagementView('templates.njk'), { currentSection: pageName, links: managementLinks, availableTemplates, commonTemplatesDetails - }) + })) } function locateTemplateConfig (req) { @@ -279,6 +287,8 @@ function getTemplatesViewHandler (req, res) { } const templateConfig = locateTemplateConfig(req) + // Nunjucks environment for template previews uses `getAppViews()` to + // add plugins including GOV.UK Frontend views via project package const nunjucksAppEnv = nunjucksConfiguration.getNunjucksAppEnv(appViews) if (templateConfig) { @@ -292,7 +302,7 @@ function getTemplatesInstallHandler (req, res) { const templateConfig = locateTemplateConfig(req) if (templateConfig) { - res.render(getManagementView('template-install.njk'), { + res.send(nunjucksManagementEnv.render(getManagementView('template-install.njk'), { currentSection: 'Templates', pageName: `Create new ${templateConfig.name}`, currentUrl: req.originalUrl, @@ -307,7 +317,7 @@ function getTemplatesInstallHandler (req, res) { invalid: 'Path must not include !$&\'()*+,;=:?#[]@.% or space' })[req.query.errorType], chosenUrl: req.query['chosen-url'] - }) + })) } else { res.status(404).send('Template not found.') } @@ -381,13 +391,13 @@ function getTemplatesPostInstallHandler (req, res) { const pageName = 'Page created' const chosenUrl = req.query['chosen-url'] - res.render(getManagementView('template-post-install.njk'), { + res.send(nunjucksManagementEnv.render(getManagementView('template-post-install.njk'), { currentSection: 'Templates', pageName, links: managementLinks, url: chosenUrl, filePath: path.join('app', 'views', `${chosenUrl}.${getFileExtensionForNunjucksFiles()}`) - }) + })) } function buildPluginData (pluginData) { @@ -520,7 +530,7 @@ async function getPluginsHandler (req, res) { foundMessage, status } - res.render(getManagementView('plugins.njk'), model) + res.send(nunjucksManagementEnv.render(getManagementView('plugins.njk'), model)) } async function postPluginsHandler (req, res) { @@ -615,7 +625,7 @@ async function getPluginsModeHandler (req, res) { dependencyHeading = `${fullPluginName} needs other plugins` } - res.render(getManagementView('plugin-install-or-uninstall.njk'), { + res.send(nunjucksManagementEnv.render(getManagementView('plugin-install-or-uninstall.njk'), { currentSection: 'Plugins', pageName, currentUrl: req.originalUrl, @@ -627,7 +637,7 @@ async function getPluginsModeHandler (req, res) { verb, isSameOrigin, returnLink - }) + })) } function setKitRestarted (state) { diff --git a/lib/manage-prototype-handlers.test.js b/lib/manage-prototype-handlers.test.js index 4b1ea81f30..affaea9646 100644 --- a/lib/manage-prototype-handlers.test.js +++ b/lib/manage-prototype-handlers.test.js @@ -153,7 +153,7 @@ describe('manage-prototype-handlers', () => { it('getClearDataHandler', () => { getClearDataHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/clear-data.njk' ) }) @@ -164,7 +164,7 @@ describe('manage-prototype-handlers', () => { } postClearDataHandler(req, res) expect(req.session.data).toEqual({}) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/clear-data-success.njk' ) }) @@ -172,7 +172,7 @@ describe('manage-prototype-handlers', () => { it('getPasswordHandler', () => { req.query.returnUrl = '/' getPasswordHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/password.njk', { error: undefined, returnURL: '/' } ) @@ -205,7 +205,7 @@ describe('manage-prototype-handlers', () => { it('getHomeHandler', async () => { packages.lookupPackageInfo.mockResolvedValue({ packageName: 'govuk-prototype-kit', latestVersion: '1.0.0' }) await getHomeHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/index.njk', expect.objectContaining({ currentSection: 'Home', latestAvailableKit: '1.0.0' }) ) @@ -214,7 +214,7 @@ describe('manage-prototype-handlers', () => { describe('developmentOnlyMiddleware', () => { it('in production', () => { developmentOnlyMiddleware(req, res, next) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/manage-prototype-not-available.njk' ) }) @@ -250,7 +250,7 @@ describe('manage-prototype-handlers', () => { it('getTemplatesHandler', async () => { await getTemplatesHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/templates.njk', expect.objectContaining({ currentSection: 'Templates', @@ -297,7 +297,7 @@ describe('manage-prototype-handlers', () => { async function testGetTemplatesInstallHandler (error) { await getTemplatesInstallHandler(req, res) expect(res.status).not.toHaveBeenCalled() - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/template-install.njk', expect.objectContaining({ currentSection: 'Templates', @@ -390,7 +390,7 @@ describe('manage-prototype-handlers', () => { it('getTemplatesPostInstallHandler', async () => { req.query['chosen-url'] = chosenUrl await getTemplatesPostInstallHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/template-post-install.njk', expect.objectContaining({ currentSection: 'Templates', @@ -442,7 +442,7 @@ describe('manage-prototype-handlers', () => { fse.readJsonSync.mockReturnValue(undefined) req.route.path = 'plugins-installed' await getPluginsHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/plugins.njk', expect.objectContaining({ currentSection: 'Plugins', @@ -457,7 +457,7 @@ describe('manage-prototype-handlers', () => { fse.readJsonSync.mockReturnValue(undefined) req.route.path = 'plugins' await getPluginsHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/plugins.njk', expect.objectContaining({ currentSection: 'Plugins', @@ -485,7 +485,7 @@ describe('manage-prototype-handlers', () => { req.query.package = packageName req.csrfToken = jest.fn().mockReturnValue(csrfToken) await getPluginsModeHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/plugin-install-or-uninstall.njk', expect.objectContaining({ chosenPlugin: availablePlugin, From 293015b97015e1db43ba010c1addbe22df1d0d2c Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Fri, 6 Oct 2023 17:11:00 +0100 Subject: [PATCH 6/8] Add Express.js locals to Manage prototype Nunjucks context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Management pages use locals like `serviceName` which custom Nunjucks environments don’t provide Here we’re restoring the same `app.locals` that the route’s `res.render()` included by default previously --- lib/manage-prototype-handlers.js | 12 ++++++++--- lib/manage-prototype-handlers.test.js | 30 +++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/lib/manage-prototype-handlers.js b/lib/manage-prototype-handlers.js index 967490d6aa..0e8286c74e 100644 --- a/lib/manage-prototype-handlers.js +++ b/lib/manage-prototype-handlers.js @@ -91,7 +91,7 @@ function getCsrfTokenHandler (req, res) { // Clear all data in session function getClearDataHandler (req, res) { - res.send(nunjucksManagementEnv.render(getManagementView('clear-data.njk'))) + res.send(nunjucksManagementEnv.render(getManagementView('clear-data.njk'), req.app.locals)) } function postClearDataHandler (req, res) { @@ -103,7 +103,7 @@ function postClearDataHandler (req, res) { function getPasswordHandler (req, res) { const returnURL = req.query.returnURL || '/' const error = req.query.error - res.send(nunjucksManagementEnv.render(getManagementView('password.njk'), { returnURL, error })) + res.send(nunjucksManagementEnv.render(getManagementView('password.njk'), { ...req.app.locals, returnURL, error })) } // Check authentication password @@ -133,7 +133,7 @@ function developmentOnlyMiddleware (req, res, next) { if (config.getConfig().isDevelopment || req.url.startsWith('/dependencies/govuk-frontend')) { next() } else { - res.send(nunjucksManagementEnv.render(getManagementView('manage-prototype-not-available.njk'))) + res.send(nunjucksManagementEnv.render(getManagementView('manage-prototype-not-available.njk'), req.app.locals)) } } @@ -181,6 +181,7 @@ async function getHomeHandler (req, res) { const kitPackage = await lookupPackageInfo('govuk-prototype-kit') const viewData = { + ...req.app.locals, currentUrl: req.originalUrl, currentSection: pageName, links: managementLinks, @@ -251,6 +252,7 @@ async function getTemplatesHandler (req, res) { } res.send(nunjucksManagementEnv.render(getManagementView('templates.njk'), { + ...req.app.locals, currentSection: pageName, links: managementLinks, availableTemplates, @@ -303,6 +305,7 @@ function getTemplatesInstallHandler (req, res) { if (templateConfig) { res.send(nunjucksManagementEnv.render(getManagementView('template-install.njk'), { + ...req.app.locals, currentSection: 'Templates', pageName: `Create new ${templateConfig.name}`, currentUrl: req.originalUrl, @@ -392,6 +395,7 @@ function getTemplatesPostInstallHandler (req, res) { const chosenUrl = req.query['chosen-url'] res.send(nunjucksManagementEnv.render(getManagementView('template-post-install.njk'), { + ...req.app.locals, currentSection: 'Templates', pageName, links: managementLinks, @@ -520,6 +524,7 @@ async function getPluginsHandler (req, res) { const foundMessage = found === 1 ? found + ' Plugin found' : found + ' Plugins found' const updatesMessage = updates ? updates === 1 ? updates + ' UPDATE AVAILABLE' : updates + ' UPDATES AVAILABLE' : '' const model = { + ...req.app.locals, currentSection: pageName, links: managementLinks, isInstalledPage, @@ -626,6 +631,7 @@ async function getPluginsModeHandler (req, res) { } res.send(nunjucksManagementEnv.render(getManagementView('plugin-install-or-uninstall.njk'), { + ...req.app.locals, currentSection: 'Plugins', pageName, currentUrl: req.originalUrl, diff --git a/lib/manage-prototype-handlers.test.js b/lib/manage-prototype-handlers.test.js index affaea9646..83325caaa1 100644 --- a/lib/manage-prototype-handlers.test.js +++ b/lib/manage-prototype-handlers.test.js @@ -131,6 +131,11 @@ describe('manage-prototype-handlers', () => { fse.exists.mockResolvedValue(true) fse.readJsonSync.mockReturnValue({}) req = { + app: { + locals: { + serviceName: 'Service name goes here' + } + }, headers: {}, body: {}, query: {}, @@ -154,7 +159,8 @@ describe('manage-prototype-handlers', () => { it('getClearDataHandler', () => { getClearDataHandler(req, res) expect(mockNunjucksRender).toHaveBeenCalledWith( - 'views/manage-prototype/clear-data.njk' + 'views/manage-prototype/clear-data.njk', + req.app.locals ) }) @@ -174,7 +180,11 @@ describe('manage-prototype-handlers', () => { getPasswordHandler(req, res) expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/password.njk', - { error: undefined, returnURL: '/' } + expect.objectContaining({ + ...req.app.locals, + error: undefined, + returnURL: '/' + }) ) }) @@ -207,7 +217,11 @@ describe('manage-prototype-handlers', () => { await getHomeHandler(req, res) expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/index.njk', - expect.objectContaining({ currentSection: 'Home', latestAvailableKit: '1.0.0' }) + expect.objectContaining({ + ...req.app.locals, + currentSection: 'Home', + latestAvailableKit: '1.0.0' + }) ) }) @@ -215,7 +229,8 @@ describe('manage-prototype-handlers', () => { it('in production', () => { developmentOnlyMiddleware(req, res, next) expect(mockNunjucksRender).toHaveBeenCalledWith( - 'views/manage-prototype/manage-prototype-not-available.njk' + 'views/manage-prototype/manage-prototype-not-available.njk', + req.app.locals ) }) @@ -253,6 +268,7 @@ describe('manage-prototype-handlers', () => { expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/templates.njk', expect.objectContaining({ + ...req.app.locals, currentSection: 'Templates', availableTemplates: [{ packageName, @@ -275,6 +291,7 @@ describe('manage-prototype-handlers', () => { expect(mockNunjucksRender).toHaveBeenCalledWith( path.join(packageName, templatePath), expect.objectContaining({ + ...req.app.locals, serviceName: 'Service name goes here' }) ) @@ -300,6 +317,7 @@ describe('manage-prototype-handlers', () => { expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/template-install.njk', expect.objectContaining({ + ...req.app.locals, currentSection: 'Templates', pageName: 'Create new A page with everything', chosenUrl, @@ -393,6 +411,7 @@ describe('manage-prototype-handlers', () => { expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/template-post-install.njk', expect.objectContaining({ + ...req.app.locals, currentSection: 'Templates', pageName: 'Page created', filePath: path.join(`app/views${chosenUrl}.html`) @@ -445,6 +464,7 @@ describe('manage-prototype-handlers', () => { expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/plugins.njk', expect.objectContaining({ + ...req.app.locals, currentSection: 'Plugins', isSearchPage: false, isInstalledPage: true, @@ -460,6 +480,7 @@ describe('manage-prototype-handlers', () => { expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/plugins.njk', expect.objectContaining({ + ...req.app.locals, currentSection: 'Plugins', isSearchPage: true, isInstalledPage: false, @@ -488,6 +509,7 @@ describe('manage-prototype-handlers', () => { expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/plugin-install-or-uninstall.njk', expect.objectContaining({ + ...req.app.locals, chosenPlugin: availablePlugin, command: `npm install ${packageName} --save-exact`, currentSection: 'Plugins', From 4781c10abcff15f7a49f6d88c6bc381f8a9b63c8 Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Tue, 10 Oct 2023 15:15:06 +0100 Subject: [PATCH 7/8] Add changelog entry --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce3424b95b..9648277399 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Fixes + +- [#2355: Prevent management pages using "plugin" GOV.UK Frontend views](https://github.com/alphagov/govuk-prototype-kit/pull/2355) + ## 13.13.4 ### Fixes From a31e6be569df9391cfbac607fac298857441eefd Mon Sep 17 00:00:00 2001 From: Colin Rotherham Date: Wed, 11 Oct 2023 09:51:03 +0100 Subject: [PATCH 8/8] Code review suggestion --- lib/manage-prototype-handlers.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/manage-prototype-handlers.test.js b/lib/manage-prototype-handlers.test.js index 83325caaa1..a719a834ef 100644 --- a/lib/manage-prototype-handlers.test.js +++ b/lib/manage-prototype-handlers.test.js @@ -250,7 +250,7 @@ describe('manage-prototype-handlers', () => { const chosenUrl = '/chosen-url' beforeEach(() => { - res.status = jest.fn().mockReturnValue({ send: res.send }) + res.status = jest.fn().mockReturnValue(res) req.query.package = packageName req.query.template = templatePath plugins.getByType.mockReturnValue([{