More security

app.js

@@ -17,6 +17,7 @@ const session = require('express-session')
 const RedisStore = require('connect-redis')(session)
 const helmet = require('helmet')
 const nunjucks = require('nunjucks')
+const { xss } = require('express-xss-sanitizer')
 
 const { APP_PORT, ENV, REDIS_URL, SESSION_SECRET } = require('./config')
 const addComponentRoutes = require('./routes/add-component')
@@ -90,6 +91,7 @@ expressNunjucks(app, {
 app.use(express.urlencoded({ extended: true }))
 app.use(express.static(path.join(__dirname, 'public')))
 app.use(express.json())
+app.use(xss())
 
 // Routes
 app.use('/contribute/add-new-component', addComponentRoutes)

helpers/previous-page.js

@@ -21,7 +21,7 @@ const previousPage = (url, session) => {
 
   // Check if there are subpages in the session for the current page
   if (subpage && Number.isInteger(Number(subpage))) {
-    if(subpage > 1) {
+    if (subpage > 1) {
       return `${urlRoot}/${currentPage}/${subpage - 1}`
     }
 

middleware/component-session.js

@@ -1,5 +1,7 @@
 const crypto = require('crypto')
 
+const sanitize = require('sanitize-filename')
+
 const {
   MAX_ADD_ANOTHER: maxAddAnother,
   ADD_NEW_COMPONENT_ROUTE,
@@ -21,15 +23,15 @@ const getHashedUrl = (url) => {
 }
 
 const getTemplate = (req) => {
-  let template = `${req.params.page || req.url.replace('/', '')}`
+  let template = sanitize(`${req.params.page || req.url.replace('/', '')}`)
   if (!Object.keys(COMPONENT_FORM_PAGES).includes(template)) {
     template = 'error'
   }
   return template
 }
 
 const getPageData = (req) => {
-  let pageData = `${req.params.page || req.url.replace('/', '')}`
+  let pageData = sanitize(`${req.params.page || req.url.replace('/', '')}`)
   if (!Object.keys(COMPONENT_FORM_PAGES).includes(pageData)) {
     pageData = {}
   }

package.json

@@ -71,6 +71,7 @@
     "express-nunjucks": "^3.1.2",
     "express-rate-limit": "^7.5.0",
     "express-session": "^1.18.1",
+    "express-xss-sanitizer": "^2.0.0",
     "govuk-frontend": "^5.9.0",
     "helmet": "^8.1.0",
     "ioredis": "^5.6.0",
@@ -80,6 +81,7 @@
     "node-fetch": "2.7.0",
     "notifications-node-client": "^8.2.1",
     "nunjucks": "^3.2.4",
+    "sanitize-filename": "^1.6.3",
     "sanitize-html": "^2.15.0"
   },
   "devDependencies": {

routes/add-component.js

@@ -1,6 +1,7 @@
 const crypto = require('crypto')
 
 const express = require('express')
+const { xss } = require('express-xss-sanitizer')
 const multer = require('multer')
 
 const { COMPONENT_FORM_PAGES, ADD_NEW_COMPONENT_ROUTE } = require('../config')
@@ -77,9 +78,6 @@ const setCsrfToken = (req, res, next) => {
 
 router.all('*', setCsrfToken)
 
-// TODO: Should we not have a post * route that verfies the CSRF for *all* posts
-// rather than relying on the chack being added to every post route declared?
-
 // TODO:  Why is this a get * ? Can it not just be set in the get /checkYourAnswersPath route below?
 router.get('*', (req, res, next) => {
   if (req?.session) {
@@ -182,6 +180,7 @@ router.get(
 
 router.post(
   ['/remove/:page', '/remove/:page/:subpage'],
+  xss(),
   verifyCsrf,
   validatePageParams,
   removeFromSession,
@@ -263,12 +262,13 @@ router.post(
   ['/component-image', '/component-image/:subpage'],
   validatePageParams,
   upload.single('componentImage'),
+  xss(),
+  verifyCsrf,
   saveFileToRedis,
   canAddAnother,
   validateFormDataFileUpload,
   setNextPage,
   getBackLink,
-  verifyCsrf,
   validateFormData,
   (req, res, next) => {
     if (req.file) {
@@ -298,22 +298,15 @@ router.post(
   }
 )
 
-// Accessibility file upload
-router.post(
-  ['/add-internal-audit', '/add-external-audit', '/add-assistive-tech'],
-  upload.single('accessibilityReport'),
-  saveFileToRedis,
-  validateFormDataFileUpload
-)
-
 // Form submissions for pages
 router.post(
   ['/:page', '/:page/:subpage'],
+  xss(),
+  verifyCsrf,
   validatePageParams,
   setNextPage,
   canSkipQuestion,
   getBackLink,
-  verifyCsrf,
   validateFormData,
   saveSession,
   (req, res, next) => {

views/community/pages/add-assistive-tech.njk

@@ -23,7 +23,7 @@
         errorList: errorList
       }) }}
     {% endif %}
-    <form method="POST" action="{{ submitUrl }}" enctype="multipart/form-data">
+    <form method="POST" action="{{ submitUrl }}">
       <span class="govuk-caption-l">Submit a new component</span>
       <h1 class="govuk-heading-xl">Testing with assistive technology</h1>
 

views/community/pages/add-external-audit.njk

@@ -23,7 +23,7 @@
         errorList: errorList
       }) }}
     {% endif %}
-    <form method="POST" action="{{ submitUrl }}" enctype="multipart/form-data">
+    <form method="POST" action="{{ submitUrl }}">
       <span class="govuk-caption-l">Submit a new component</span>
       <h1 class="govuk-heading-xl">External accessibility audit</h1>
 

views/community/pages/add-internal-audit.njk

@@ -23,7 +23,7 @@
         errorList: errorList
       }) }}
     {% endif %}
-    <form method="POST" action="{{ submitUrl }}" enctype="multipart/form-data">
+    <form method="POST" action="{{ submitUrl }}">
       <span class="govuk-caption-l">Submit a new component</span>
       <h1 class="govuk-heading-xl">Internal accessibility review</h1>