Merge pull request #2150 from alphagov/2129-fix-our-csrf-vulnerability

Prevent CSURF deprecated warning
alphagov · May 5, 2023 · 21fd1c8 · 21fd1c8
2 parents 4df5036 + 02201e7
commit 21fd1c8
Show file tree

Hide file tree

Showing 11 changed files with 123 additions and 179 deletions.
diff --git 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
@@ -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
@@ -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
@@ -48,9 +48,12 @@ const errorHTML = `
       </div>
     `
 
+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 = '<meta content="authenticity_token" name="csrf-token" />'
     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
@@ -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
@@ -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
@@ -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
@@ -1,10 +1,6 @@
 {% extends "views/manage-prototype/layout.njk" %}
 {% from "govuk/components/button/macro.njk" import govukButton %}
 
-{% block meta %}
-  <meta name="csrf-token" content="{{ csrfToken }}">
-{% endblock %}
-
 {% block content %}
   <div class="govuk-grid-row govuk-prototype-kit-manage-prototype-plugin-processing">
     <div class="govuk-grid-column-two-thirds">