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 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..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 || []) - .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/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/lib/manage-prototype-handlers.js b/lib/manage-prototype-handlers.js index 8c5b50c3be..0e8286c74e 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'), req.app.locals)) } 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'), { ...req.app.locals, 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'), req.app.locals)) } } @@ -173,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, @@ -189,7 +198,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 +251,13 @@ async function getTemplatesHandler (req, res) { } } - res.render(getManagementView('templates.njk'), { + res.send(nunjucksManagementEnv.render(getManagementView('templates.njk'), { + ...req.app.locals, currentSection: pageName, links: managementLinks, availableTemplates, commonTemplatesDetails - }) + })) } function locateTemplateConfig (req) { @@ -279,6 +289,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 +304,8 @@ function getTemplatesInstallHandler (req, res) { const templateConfig = locateTemplateConfig(req) if (templateConfig) { - res.render(getManagementView('template-install.njk'), { + res.send(nunjucksManagementEnv.render(getManagementView('template-install.njk'), { + ...req.app.locals, currentSection: 'Templates', pageName: `Create new ${templateConfig.name}`, currentUrl: req.originalUrl, @@ -307,7 +320,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 +394,14 @@ 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'), { + ...req.app.locals, currentSection: 'Templates', pageName, links: managementLinks, url: chosenUrl, filePath: path.join('app', 'views', `${chosenUrl}.${getFileExtensionForNunjucksFiles()}`) - }) + })) } function buildPluginData (pluginData) { @@ -510,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, @@ -520,7 +535,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 +630,8 @@ 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'), { + ...req.app.locals, currentSection: 'Plugins', pageName, currentUrl: req.originalUrl, @@ -627,7 +643,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 b2b3b010b6..a719a834ef 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') @@ -128,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: {}, @@ -138,7 +146,8 @@ describe('manage-prototype-handlers', () => { } res = { render: jest.fn(), - redirect: jest.fn() + redirect: jest.fn(), + send: jest.fn() } next = jest.fn() }) @@ -149,8 +158,9 @@ describe('manage-prototype-handlers', () => { it('getClearDataHandler', () => { getClearDataHandler(req, res) - expect(res.render).toHaveBeenCalledWith( - 'views/manage-prototype/clear-data.njk' + expect(mockNunjucksRender).toHaveBeenCalledWith( + 'views/manage-prototype/clear-data.njk', + req.app.locals ) }) @@ -160,7 +170,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' ) }) @@ -168,9 +178,13 @@ 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: '/' } + expect.objectContaining({ + ...req.app.locals, + error: undefined, + returnURL: '/' + }) ) }) @@ -201,17 +215,22 @@ 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' }) + expect.objectContaining({ + ...req.app.locals, + currentSection: 'Home', + latestAvailableKit: '1.0.0' + }) ) }) describe('developmentOnlyMiddleware', () => { it('in production', () => { developmentOnlyMiddleware(req, res, next) - expect(res.render).toHaveBeenCalledWith( - 'views/manage-prototype/manage-prototype-not-available.njk' + expect(mockNunjucksRender).toHaveBeenCalledWith( + 'views/manage-prototype/manage-prototype-not-available.njk', + req.app.locals ) }) @@ -228,16 +247,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(res) req.query.package = packageName req.query.template = templatePath - res.send = mockSend plugins.getByType.mockReturnValue([{ packageName, item: { @@ -246,16 +261,14 @@ describe('manage-prototype-handlers', () => { path: templatePath } }]) - nunjucksConfiguration.getNunjucksAppEnv.mockImplementation(() => ({ - render: () => view - })) }) it('getTemplatesHandler', async () => { await getTemplatesHandler(req, res) - expect(res.render).toHaveBeenCalledWith( + expect(mockNunjucksRender).toHaveBeenCalledWith( 'views/manage-prototype/templates.njk', expect.objectContaining({ + ...req.app.locals, currentSection: 'Templates', availableTemplates: [{ packageName, @@ -275,14 +288,20 @@ 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({ + ...req.app.locals, + 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.') }) }) @@ -295,9 +314,10 @@ 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({ + ...req.app.locals, currentSection: 'Templates', pageName: 'Create new A page with everything', chosenUrl, @@ -331,7 +351,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', () => { @@ -388,9 +408,10 @@ 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({ + ...req.app.locals, currentSection: 'Templates', pageName: 'Page created', filePath: path.join(`app/views${chosenUrl}.html`) @@ -440,9 +461,10 @@ 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({ + ...req.app.locals, currentSection: 'Plugins', isSearchPage: false, isInstalledPage: true, @@ -455,9 +477,10 @@ 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({ + ...req.app.locals, currentSection: 'Plugins', isSearchPage: true, isInstalledPage: false, @@ -483,9 +506,10 @@ 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({ + ...req.app.locals, chosenPlugin: availablePlugin, command: `npm install ${packageName} --save-exact`, currentSection: 'Plugins', 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 71fd7db979..b801d1a0c5 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 || []) - .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,12 @@ if (config.isDevelopment) { nunjucksConfig.express = app -const nunjucksAppEnv = getNunjucksAppEnv(appViews) +// 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]), + govukFrontendInternal +) expressNunjucks(nunjucksAppEnv, app)