diff --git a/CHANGELOG.md b/CHANGELOG.md index 9de1bf906e..b7d968ddee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- [#2150: Prevent CSURF deprecated warning](https://github.com/alphagov/govuk-prototype-kit/pull/2150) + ## 13.6.2 ### Fixes diff --git a/cypress/e2e/plugins/1-available-plugins-tests/install-available-plugin.cypress.js b/cypress/e2e/plugins/1-available-plugins-tests/install-available-plugin.cypress.js index b7e9dd3b2c..24568286c8 100644 --- a/cypress/e2e/plugins/1-available-plugins-tests/install-available-plugin.cypress.js +++ b/cypress/e2e/plugins/1-available-plugins-tests/install-available-plugin.cypress.js @@ -63,7 +63,7 @@ describe('Management plugins: ', () => { body: { package: plugin } }).then(response => { expect(response.status).to.eq(403) - expect(response.body).to.eq('invalid csrf token') + expect(response.body).to.have.property('error', 'invalid csrf token') }) }) diff --git a/lib/assets/javascripts/manage-prototype/manage-plugins.js b/lib/assets/javascripts/manage-prototype/manage-plugins.js index 26bc718b19..f12b81a3c5 100644 --- a/lib/assets/javascripts/manage-prototype/manage-plugins.js +++ b/lib/assets/javascripts/manage-prototype/manage-plugins.js @@ -38,14 +38,13 @@ show('panel-error') } - const postRequest = (url) => { - const token = document.querySelector('meta[name="csrf-token"]').getAttribute('content') + const postRequest = (url, token) => { return fetch(url, { method: 'POST', headers: { Accept: 'application/json', 'Content-Type': 'application/json', - 'CSRF-Token': token + 'X-CSRF-TOKEN': token }, body: JSON.stringify({ package: packageName, @@ -102,6 +101,12 @@ }) } + const getToken = () => { + return fetch('/manage-prototype/csrf-token') + .then(response => response.json()) + .then(({ token }) => token) + } + const performAction = (event) => { if (!actionTimeoutId) { actionTimeoutId = setTimeout(() => { @@ -116,7 +121,8 @@ show('panel-processing') - return postRequest(`/manage-prototype/plugins/${mode}`) + return getToken() + .then((token) => postRequest(`/manage-prototype/plugins/${mode}`, token)) .then(data => { switch (data.status) { case 'completed': diff --git a/lib/assets/javascripts/manage-prototype/manage-plugins.test.js b/lib/assets/javascripts/manage-prototype/manage-plugins.test.js index 4b34e57c2e..3a1a61185f 100644 --- a/lib/assets/javascripts/manage-prototype/manage-plugins.test.js +++ b/lib/assets/javascripts/manage-prototype/manage-plugins.test.js @@ -48,9 +48,12 @@ const errorHTML = ` ` +const token = 'csrf-test-token' + describe('manage-plugins', () => { global.fetch = jest.fn().mockResolvedValue(null) + const getTokenUrl = '/manage-prototype/csrf-token' const performActionUrl = '/manage-prototype/plugins/' const pollStatusUrl = '/manage-prototype/plugins//status' @@ -92,7 +95,6 @@ describe('manage-plugins', () => { } return 'action-timeout-id' }) - document.head.innerHTML = '' document.body.innerHTML = loadedHTML fetchList = [] }) @@ -119,33 +121,57 @@ describe('manage-plugins', () => { }) it('completed', async () => { - mockFetch(['completed']) + mockFetch([ + { token }, + 'completed' + ]) await managePlugins.performAction() expect(document.body.innerHTML).toEqual(completedHTML) - expect(global.fetch).toHaveBeenCalledTimes(1) - expect(fetchList).toEqual([performActionUrl]) + expect(global.fetch).toHaveBeenCalledTimes(fetchList.length) + expect(fetchList).toEqual([ + getTokenUrl, + performActionUrl + ]) }) it('error', async () => { - mockFetch(['error']) + mockFetch([ + { token }, + 'error' + ]) await managePlugins.performAction() expect(document.body.innerHTML).toEqual(errorHTML) - expect(global.fetch).toHaveBeenCalledTimes(1) - expect(fetchList).toEqual([performActionUrl]) + expect(global.fetch).toHaveBeenCalledTimes(fetchList.length) + expect(fetchList).toEqual([ + getTokenUrl, + performActionUrl + ]) }) it('will restart', async () => { - mockFetch(['throws', 'processing', 'completed']) + mockFetch([ + { token }, + 'throws', + { token }, + 'processing', + 'completed'] + ) await managePlugins.performAction() expect(document.body.innerHTML).toEqual(completedHTML) - expect(global.fetch).toHaveBeenCalledTimes(3) - expect(fetchList).toEqual([performActionUrl, performActionUrl, pollStatusUrl]) + expect(global.fetch).toHaveBeenCalledTimes(fetchList.length) + expect(fetchList).toEqual([ + getTokenUrl, + performActionUrl, + getTokenUrl, + performActionUrl, + pollStatusUrl + ]) }) }) @@ -164,8 +190,12 @@ describe('manage-plugins', () => { await managePlugins.pollStatus() expect(document.body.innerHTML).toEqual(completedHTML) - expect(global.fetch).toHaveBeenCalledTimes(3) - expect(fetchList).toEqual([pollStatusUrl, pollStatusUrl, pollStatusUrl]) + expect(global.fetch).toHaveBeenCalledTimes(fetchList.length) + expect(fetchList).toEqual([ + pollStatusUrl, + pollStatusUrl, + pollStatusUrl + ]) }) it('error', async () => { @@ -178,8 +208,12 @@ describe('manage-plugins', () => { await managePlugins.pollStatus() expect(document.body.innerHTML).toEqual(errorHTML) - expect(global.fetch).toHaveBeenCalledTimes(3) - expect(fetchList).toEqual([pollStatusUrl, pollStatusUrl, pollStatusUrl]) + expect(global.fetch).toHaveBeenCalledTimes(fetchList.length) + expect(fetchList).toEqual([ + pollStatusUrl, + pollStatusUrl, + pollStatusUrl + ]) }) }) @@ -190,6 +224,7 @@ describe('manage-plugins', () => { it('completed', async () => { mockFetch([ + { token }, 'processing', 'processing', 'throws', @@ -199,12 +234,19 @@ describe('manage-plugins', () => { await managePlugins.init() expect(document.body.innerHTML).toEqual(completedHTML) - expect(global.fetch).toHaveBeenCalledTimes(4) - expect(fetchList).toEqual([performActionUrl, pollStatusUrl, pollStatusUrl, pollStatusUrl]) + expect(global.fetch).toHaveBeenCalledTimes(fetchList.length) + expect(fetchList).toEqual([ + getTokenUrl, + performActionUrl, + pollStatusUrl, + pollStatusUrl, + pollStatusUrl + ]) }) it('error', async () => { mockFetch([ + { token }, 'processing', 'processing', 'throws', @@ -214,8 +256,14 @@ describe('manage-plugins', () => { await managePlugins.init() expect(document.body.innerHTML).toEqual(errorHTML) - expect(global.fetch).toHaveBeenCalledTimes(4) - expect(fetchList).toEqual([performActionUrl, pollStatusUrl, pollStatusUrl, pollStatusUrl]) + expect(global.fetch).toHaveBeenCalledTimes(fetchList.length) + expect(fetchList).toEqual([ + getTokenUrl, + performActionUrl, + pollStatusUrl, + pollStatusUrl, + pollStatusUrl + ]) }) }) }) diff --git a/lib/manage-prototype-handlers.js b/lib/manage-prototype-handlers.js index 335c4901c9..39d22069ae 100644 --- a/lib/manage-prototype-handlers.js +++ b/lib/manage-prototype-handlers.js @@ -5,6 +5,7 @@ const path = require('path') // npm dependencies const fse = require('fs-extra') const querystring = require('querystring') +const { doubleCsrf } = require('csrf-csrf') // local dependencies const config = require('./config') @@ -79,6 +80,27 @@ function getManagementView (filename) { // Local dependencies const encryptPassword = require('./utils').encryptPassword +const { invalidCsrfTokenError, generateToken, doubleCsrfProtection } = doubleCsrf({ + getSecret: (req) => 'Secret', + cookieName: 'x-csrf-token' +}) + +// Error handling, validation error interception +const csrfErrorHandler = (error, req, res, next) => { + if (error === invalidCsrfTokenError) { + res.status(403).json({ + error: 'invalid csrf token' + }) + } else { + next() + } +} + +function getCsrfTokenHandler (req, res) { + const token = generateToken(res, req) + return res.json({ token }) +} + // Clear all data in session function getClearDataHandler (req, res) { res.render(getManagementView('clear-data.njk')) @@ -562,7 +584,6 @@ async function getPluginsModeHandler (req, res) { command: getCommand(mode, chosenPlugin), verb, isSameOrigin, - csrfToken: req.csrfToken(), returnLink }) } @@ -658,6 +679,8 @@ async function postPluginsModeHandler (req, res) { module.exports = { contextPath, setKitRestarted, + csrfProtection: [doubleCsrfProtection, csrfErrorHandler], + getCsrfTokenHandler, getClearDataHandler, postClearDataHandler, getPasswordHandler, diff --git a/lib/manage-prototype-handlers.test.js b/lib/manage-prototype-handlers.test.js index 63b008c190..795d5a21e4 100644 --- a/lib/manage-prototype-handlers.test.js +++ b/lib/manage-prototype-handlers.test.js @@ -355,7 +355,7 @@ describe('manage-prototype-handlers', () => { }) describe('plugins handlers', () => { - const csrfToken = 'CSRF-TOKEN' + const csrfToken = 'x-csrf-token' const packageName = 'test-package' const latestVersion = '2.0.0' const previousVersion = '1.0.0' @@ -412,7 +412,6 @@ describe('manage-prototype-handlers', () => { expect.objectContaining({ chosenPlugin: availablePlugin, command: `npm install ${packageName} --save-exact`, - csrfToken, currentSection: 'Plugins', pageName: `Install ${pluginDisplayName.name}`, currentUrl: req.originalUrl, diff --git a/lib/manage-prototype-routes.js b/lib/manage-prototype-routes.js index 3d03b5dfdb..913fbd868e 100644 --- a/lib/manage-prototype-routes.js +++ b/lib/manage-prototype-routes.js @@ -1,12 +1,11 @@ // npm dependencies -const csrf = require('csurf') const express = require('express') -const csrfProtection = csrf({ cookie: false }) - const { contextPath, setKitRestarted, + csrfProtection, + getCsrfTokenHandler, getClearDataHandler, postClearDataHandler, getPasswordHandler, @@ -34,6 +33,8 @@ redirectingRouter.use((req, res) => { res.redirect(req.originalUrl.replace('/manage-prototype', contextPath)) }) +router.get('/csrf-token', getCsrfTokenHandler) + // Clear all data in session router.get('/clear-data', getClearDataHandler) diff --git a/lib/nunjucks/views/manage-prototype/plugin-install-or-uninstall.njk b/lib/nunjucks/views/manage-prototype/plugin-install-or-uninstall.njk index 00e33a0c19..b7523eaf5a 100644 --- a/lib/nunjucks/views/manage-prototype/plugin-install-or-uninstall.njk +++ b/lib/nunjucks/views/manage-prototype/plugin-install-or-uninstall.njk @@ -1,10 +1,6 @@ {% extends "views/manage-prototype/layout.njk" %} {% from "govuk/components/button/macro.njk" import govukButton %} -{% block meta %} - -{% endblock %} - {% block content %}