From 74ecebbe32b5a12310e41b9ce8742aa70463e335 Mon Sep 17 00:00:00 2001 From: Nathaniel Steers Date: Mon, 16 Dec 2024 16:39:10 +0000 Subject: [PATCH 1/2] PP-13360 csrf express middleware --- .secrets.baseline | 11 +- karma.conf.js | 1 + package-lock.json | 135 +++++++++++++++++ package.json | 4 +- src/utils/middleware/csrf.middleware.js | 79 ++++++++++ src/utils/middleware/csrf.middleware.test.js | 147 +++++++++++++++++++ 6 files changed, 375 insertions(+), 2 deletions(-) create mode 100644 src/utils/middleware/csrf.middleware.js create mode 100644 src/utils/middleware/csrf.middleware.test.js diff --git a/.secrets.baseline b/.secrets.baseline index 0145c73f..f6a1793d 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -116,7 +116,16 @@ "is_verified": false, "line_number": 20 } + ], + "src/utils/middleware/csrf.middleware.js": [ + { + "type": "Secret Keyword", + "filename": "src/utils/middleware/csrf.middleware.js", + "hashed_secret": "fb23222a82d3fc0120ff287e546fee1be335c81a", + "is_verified": false, + "line_number": 9 + } ] }, - "generated_at": "2024-07-22T14:03:26Z" + "generated_at": "2024-12-16T16:47:12Z" } diff --git a/karma.conf.js b/karma.conf.js index 43bf0648..e2f96e17 100644 --- a/karma.conf.js +++ b/karma.conf.js @@ -14,6 +14,7 @@ module.exports = karma => karma.set({ exclude: [ '**/axios-base-client.test.js', 'analytics/**/*.js', + 'utils/middleware/**/*.js', '**/errors.test.js' ], plugins: [ diff --git a/package-lock.json b/package-lock.json index 76153876..54fcea9b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,6 +10,7 @@ "license": "MIT", "dependencies": { "axios": "^1.6.5", + "csrf": "^3.1.0", "lodash": "4.17.21", "moment-timezone": "0.5.43", "rfc822-validate": "1.0.0", @@ -20,6 +21,7 @@ "@babel/cli": "^7.4.4", "@babel/core": "^7.4.5", "@babel/preset-env": "^7.4.5", + "@types/express": "^5.0.0", "babelify": "^10.0.0", "brfs": "^2.0.2", "browser-env": "^3.2.6", @@ -2944,6 +2946,25 @@ "@babel/types": "^7.20.7" } }, + "node_modules/@types/body-parser": { + "version": "1.19.5", + "resolved": "https://registry.npmjs.org/@types/body-parser/-/body-parser-1.19.5.tgz", + "integrity": "sha512-fB3Zu92ucau0iQ0JMCFQE7b/dv8Ot07NI3KaZIkIUNXq82k4eBAqUaneXfleGY9JWskeS9y+u0nXMyspcuQrCg==", + "dev": true, + "dependencies": { + "@types/connect": "*", + "@types/node": "*" + } + }, + "node_modules/@types/connect": { + "version": "3.4.38", + "resolved": "https://registry.npmjs.org/@types/connect/-/connect-3.4.38.tgz", + "integrity": "sha512-K6uROf1LD88uDQqJCktA4yzL1YYAK6NgfsI0v/mTgyPKWsX1CnJ0XPSDhViejru1GcRkLWb8RlzFYJRqGUbaug==", + "dev": true, + "dependencies": { + "@types/node": "*" + } + }, "node_modules/@types/cookie": { "version": "0.4.1", "resolved": "https://registry.npmjs.org/@types/cookie/-/cookie-0.4.1.tgz", @@ -2975,6 +2996,30 @@ "integrity": "sha512-/kYRxGDLWzHOB7q+wtSUQlFrtcdUccpfy+X+9iMBpHK8QLLhx2wIPYuS5DYtR9Wa/YlZAbIovy7qVdB1Aq6Lyw==", "dev": true }, + "node_modules/@types/express": { + "version": "5.0.0", + "resolved": "https://registry.npmjs.org/@types/express/-/express-5.0.0.tgz", + "integrity": "sha512-DvZriSMehGHL1ZNLzi6MidnsDhUZM/x2pRdDIKdwbUNqqwHxMlRdkxtn6/EPKyqKpHqTl/4nRZsRNLpZxZRpPQ==", + "dev": true, + "dependencies": { + "@types/body-parser": "*", + "@types/express-serve-static-core": "^5.0.0", + "@types/qs": "*", + "@types/serve-static": "*" + } + }, + "node_modules/@types/express-serve-static-core": { + "version": "5.0.2", + "resolved": "https://registry.npmjs.org/@types/express-serve-static-core/-/express-serve-static-core-5.0.2.tgz", + "integrity": "sha512-vluaspfvWEtE4vcSDlKRNer52DvOGrB2xv6diXy6UKyKW0lqZiWHGNApSyxOv+8DE5Z27IzVvE7hNkxg7EXIcg==", + "dev": true, + "dependencies": { + "@types/node": "*", + "@types/qs": "*", + "@types/range-parser": "*", + "@types/send": "*" + } + }, "node_modules/@types/graceful-fs": { "version": "4.1.9", "resolved": "https://registry.npmjs.org/@types/graceful-fs/-/graceful-fs-4.1.9.tgz", @@ -2984,6 +3029,12 @@ "@types/node": "*" } }, + "node_modules/@types/http-errors": { + "version": "2.0.4", + "resolved": "https://registry.npmjs.org/@types/http-errors/-/http-errors-2.0.4.tgz", + "integrity": "sha512-D0CFMMtydbJAegzOyHjtiKPLlvnm3iTZyZRSZoLq2mRhDdmLfIWOCYPfQJ4cu2erKghU++QvjcUjp/5h7hESpA==", + "dev": true + }, "node_modules/@types/istanbul-lib-coverage": { "version": "2.0.6", "resolved": "https://registry.npmjs.org/@types/istanbul-lib-coverage/-/istanbul-lib-coverage-2.0.6.tgz", @@ -3031,6 +3082,12 @@ "integrity": "sha512-dRLjCWHYg4oaA77cxO64oO+7JwCwnIzkZPdrrC71jQmQtlhM556pwKo5bUzqvZndkVbeFLIIi+9TC40JNF5hNQ==", "dev": true }, + "node_modules/@types/mime": { + "version": "1.3.5", + "resolved": "https://registry.npmjs.org/@types/mime/-/mime-1.3.5.tgz", + "integrity": "sha512-/pyBZWSLD2n0dcHE3hq8s8ZvcETHtEuF+3E7XVt0Ig2nvsVQXdghHVcEkIWjy9A0wKfTn97a/PSDYohKIlnP/w==", + "dev": true + }, "node_modules/@types/minimist": { "version": "1.2.5", "resolved": "https://registry.npmjs.org/@types/minimist/-/minimist-1.2.5.tgz", @@ -3052,6 +3109,39 @@ "integrity": "sha512-37i+OaWTh9qeK4LSHPsyRC7NahnGotNuZvjLSgcPzblpHB3rrCJxAOgI5gCdKm7coonsaX1Of0ILiTcnZjbfxA==", "dev": true }, + "node_modules/@types/qs": { + "version": "6.9.17", + "resolved": "https://registry.npmjs.org/@types/qs/-/qs-6.9.17.tgz", + "integrity": "sha512-rX4/bPcfmvxHDv0XjfJELTTr+iB+tn032nPILqHm5wbthUUUuVtNGGqzhya9XUxjTP8Fpr0qYgSZZKxGY++svQ==", + "dev": true + }, + "node_modules/@types/range-parser": { + "version": "1.2.7", + "resolved": "https://registry.npmjs.org/@types/range-parser/-/range-parser-1.2.7.tgz", + "integrity": "sha512-hKormJbkJqzQGhziax5PItDUTMAM9uE2XXQmM37dyd4hVM+5aVl7oVxMVUiVQn2oCQFN/LKCZdvSM0pFRqbSmQ==", + "dev": true + }, + "node_modules/@types/send": { + "version": "0.17.4", + "resolved": "https://registry.npmjs.org/@types/send/-/send-0.17.4.tgz", + "integrity": "sha512-x2EM6TJOybec7c52BX0ZspPodMsQUd5L6PRwOunVyVUhXiBSKf3AezDL8Dgvgt5o0UfKNfuA0eMLr2wLT4AiBA==", + "dev": true, + "dependencies": { + "@types/mime": "^1", + "@types/node": "*" + } + }, + "node_modules/@types/serve-static": { + "version": "1.15.7", + "resolved": "https://registry.npmjs.org/@types/serve-static/-/serve-static-1.15.7.tgz", + "integrity": "sha512-W8Ym+h8nhuRwaKPaDw34QUkwsGi6Rc4yYqvKFo5rm2FUEhCFbzVWrxXUxuKK8TASjWsysJY0nsmNCGhCOIsrOw==", + "dev": true, + "dependencies": { + "@types/http-errors": "*", + "@types/node": "*", + "@types/send": "*" + } + }, "node_modules/@types/stack-utils": { "version": "2.0.3", "resolved": "https://registry.npmjs.org/@types/stack-utils/-/stack-utils-2.0.3.tgz", @@ -5292,6 +5382,19 @@ "node": "*" } }, + "node_modules/csrf": { + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/csrf/-/csrf-3.1.0.tgz", + "integrity": "sha512-uTqEnCvWRk042asU6JtapDTcJeeailFy4ydOQS28bj1hcLnYRiqi8SsD2jS412AY1I/4qdOwWZun774iqywf9w==", + "dependencies": { + "rndm": "1.2.0", + "tsscmp": "1.0.6", + "uid-safe": "2.1.5" + }, + "engines": { + "node": ">= 0.8" + } + }, "node_modules/cssom": { "version": "0.5.0", "resolved": "https://registry.npmjs.org/cssom/-/cssom-0.5.0.tgz", @@ -14069,6 +14172,14 @@ "quote-stream": "bin/cmd.js" } }, + "node_modules/random-bytes": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/random-bytes/-/random-bytes-1.0.0.tgz", + "integrity": "sha512-iv7LhNVO047HzYR3InF6pUcUsPQiHTM1Qal51DcGSuZFBil1aBBWG5eHPNek7bvILMaYJ/8RU1e8w1AMdHmLQQ==", + "engines": { + "node": ">= 0.8" + } + }, "node_modules/randombytes": { "version": "2.1.0", "resolved": "https://registry.npmjs.org/randombytes/-/randombytes-2.1.0.tgz", @@ -14750,6 +14861,11 @@ "inherits": "^2.0.1" } }, + "node_modules/rndm": { + "version": "1.2.0", + "resolved": "https://registry.npmjs.org/rndm/-/rndm-1.2.0.tgz", + "integrity": "sha512-fJhQQI5tLrQvYIYFpOnFinzv9dwmR7hRnUz1XqP3OJ1jIweTNOd6aTO4jwQSgcBSFUB+/KHJxuGneime+FdzOw==" + }, "node_modules/run-parallel": { "version": "1.2.0", "resolved": "https://registry.npmjs.org/run-parallel/-/run-parallel-1.2.0.tgz", @@ -16283,6 +16399,14 @@ "node": ">=4" } }, + "node_modules/tsscmp": { + "version": "1.0.6", + "resolved": "https://registry.npmjs.org/tsscmp/-/tsscmp-1.0.6.tgz", + "integrity": "sha512-LxhtAkPDTkVCMQjt2h6eBVY28KCjikZqZfMcC15YBeNjkgUpdCfBu5HoiOTDu86v6smE8yOjyEktJ8hlbANHQA==", + "engines": { + "node": ">=0.6.x" + } + }, "node_modules/tty-browserify": { "version": "0.0.1", "resolved": "https://registry.npmjs.org/tty-browserify/-/tty-browserify-0.0.1.tgz", @@ -16486,6 +16610,17 @@ "node": ">=0.8.0" } }, + "node_modules/uid-safe": { + "version": "2.1.5", + "resolved": "https://registry.npmjs.org/uid-safe/-/uid-safe-2.1.5.tgz", + "integrity": "sha512-KPHm4VL5dDXKz01UuEd88Df+KzynaohSL9fBh096KWAxSKZQDI2uBrVqtvRM4rwrIrRRKsdLNML/lnaaVSRioA==", + "dependencies": { + "random-bytes": "~1.0.0" + }, + "engines": { + "node": ">= 0.8" + } + }, "node_modules/umd": { "version": "3.0.3", "resolved": "https://registry.npmjs.org/umd/-/umd-3.0.3.tgz", diff --git a/package.json b/package.json index a3eef578..b010cada 100644 --- a/package.json +++ b/package.json @@ -20,7 +20,7 @@ "prepare": "npm run transpile && npm run browserify-analytics", "test": "npm run jest-tests npm && npm run karma-tests", "karma-tests": "karma start", - "jest-tests": "jest src/analytics src/utils/axios-base-client/axios-base-client.test.js", + "jest-tests": "jest src/analytics src/utils/axios-base-client/axios-base-client.test.js src/utils/middleware/*.test.js", "lint": "standard --fix" }, "repository": { @@ -112,6 +112,7 @@ "@babel/cli": "^7.4.4", "@babel/core": "^7.4.5", "@babel/preset-env": "^7.4.5", + "@types/express": "^5.0.0", "babelify": "^10.0.0", "brfs": "^2.0.2", "browser-env": "^3.2.6", @@ -144,6 +145,7 @@ }, "dependencies": { "axios": "^1.6.5", + "csrf": "^3.1.0", "lodash": "4.17.21", "moment-timezone": "0.5.43", "rfc822-validate": "1.0.0", diff --git a/src/utils/middleware/csrf.middleware.js b/src/utils/middleware/csrf.middleware.js new file mode 100644 index 00000000..a9b1671e --- /dev/null +++ b/src/utils/middleware/csrf.middleware.js @@ -0,0 +1,79 @@ +const csrf = require('csrf') + +/** + * @param logger {Logger} + * @param sessionKey {string} + * @param secretKey {string} + * @param tokenKey {string} + */ +const configureCsrfMiddleware = (logger, sessionKey, secretKey = 'csrfSecret', tokenKey = 'csrfToken') => { + logger.debug('--- CSRF middleware configuration ---') + logger.debug(`Secret is set at req.[${sessionKey}][${secretKey}]`) + logger.debug(`Token is checked at req.body|query[${tokenKey}]`) + logger.debug('-------------------------------------') + /** + * @param csrfToken {string} + * @param csrfSecret {string} + * @param req {e.Request} + */ + const csrfValid = (csrfToken, csrfSecret, req) => { + if (!csrfSecret) { + logger.debug('CSRF secret not found when validating token') + return false + } + if (!['put', 'post'].includes(req.method.toLowerCase())) { + return true + } + return csrf().verify(csrfSecret, csrfToken) + } + + /** + * @param req {e.Request} + * @param res {e.Response} + * @param next {e.NextFunction} + */ + const setSecret = (req, res, next) => { + const csrfSecret = req[sessionKey][secretKey] + if (!csrfSecret) { + logger.debug('Synchronising CSRF secret') + req[sessionKey][secretKey] = csrf().secretSync() + } + next() + } + + /** + * @param req {e.Request} + * @param res {e.Response} + * @param next {e.NextFunction} + */ + const checkToken = (req, res, next) => { + const csrfSecret = req[sessionKey][secretKey] + const csrfToken = (req.body && req.body[tokenKey]) || (req.query && req.query[tokenKey]) + if (!csrfValid(csrfToken, csrfSecret, req)) { + next(new Error('Invalid CSRF token')) + } else { + next() + } + } + + /** + * @param req {e.Request} + * @param res {e.Response} + * @param next {e.NextFunction} + */ + const generateToken = (req, res, next) => { + const csrfSecret = req[sessionKey][secretKey] + res.locals.csrf = csrf().create(csrfSecret) + next() + } + + return { + setSecret, + checkToken, + generateToken + } +} + +module.exports = { + configureCsrfMiddleware +} diff --git a/src/utils/middleware/csrf.middleware.test.js b/src/utils/middleware/csrf.middleware.test.js new file mode 100644 index 00000000..21cf1773 --- /dev/null +++ b/src/utils/middleware/csrf.middleware.test.js @@ -0,0 +1,147 @@ +const { expect } = require('chai') +const { configureCsrfMiddleware } = require('./csrf.middleware') +const csrf = require('csrf') +const sinon = require('sinon') + +describe('CSRF Middleware', () => { + let mockLogger + let middleware + let mockReq + let mockRes + let mockNext + + beforeEach(() => { + mockLogger = { + debug: sinon.spy() + } + + middleware = configureCsrfMiddleware(mockLogger, 'session', 'testSecret', 'testToken') + + mockReq = { + session: {}, + method: 'GET', + body: {}, + query: {} + } + + mockRes = { + locals: {} + } + + mockNext = sinon.spy() + }) + + describe('setSecret', () => { + it('should set CSRF secret if none exists', () => { + middleware.setSecret(mockReq, mockRes, mockNext) + + expect(mockReq.session.testSecret).to.exist // eslint-disable-line + expect(mockNext.calledOnce).to.be.true // eslint-disable-line + }) + + it('should not override existing CSRF secret', () => { + const existingSecret = 'existing-secret' // pragma: allowlist secret + mockReq.session.testSecret = existingSecret + + middleware.setSecret(mockReq, mockRes, mockNext) + + expect(mockReq.session.testSecret).to.equal(existingSecret) + expect(mockNext.calledOnce).to.be.true // eslint-disable-line + }) + }) + + describe('checkToken', () => { + beforeEach(() => { + mockReq.session.testSecret = csrf().secretSync() + }) + + it('should allow GET requests without token', () => { + mockReq.method = 'GET' + + middleware.checkToken(mockReq, mockRes, mockNext) + + expect(mockNext.calledOnce).to.be.true // eslint-disable-line + expect(mockNext.args[0][0]).to.not.exist // eslint-disable-line + }) + + it('should allow HEAD requests without token', () => { + mockReq.method = 'HEAD' + + middleware.checkToken(mockReq, mockRes, mockNext) + + expect(mockNext.calledOnce).to.be.true // eslint-disable-line + expect(mockNext.args[0][0]).to.not.exist // eslint-disable-line + }) + + it('should reject POST requests without token', () => { + mockReq.method = 'POST' + + middleware.checkToken(mockReq, mockRes, mockNext) + + expect(mockNext.calledWith(sinon.match.instanceOf(Error))).to.be.true // eslint-disable-line + expect(mockNext.args[0][0].message).to.equal('Invalid CSRF token') + }) + + it('should reject PUT requests without token', () => { + mockReq.method = 'PUT' + + middleware.checkToken(mockReq, mockRes, mockNext) + + expect(mockNext.calledWith(sinon.match.instanceOf(Error))).to.be.true // eslint-disable-line + expect(mockNext.args[0][0].message).to.equal('Invalid CSRF token') + }) + + it('should accept valid token in request body', () => { + mockReq.method = 'POST' + mockReq.body.testToken = csrf().create(mockReq.session.testSecret) + + middleware.checkToken(mockReq, mockRes, mockNext) + + expect(mockNext.calledOnce).to.be.true // eslint-disable-line + expect(mockNext.args[0][0]).to.not.exist // eslint-disable-line + }) + + it('should accept valid token in query string', () => { + mockReq.method = 'POST' + mockReq.query.testToken = csrf().create(mockReq.session.testSecret) + + middleware.checkToken(mockReq, mockRes, mockNext) + + expect(mockNext.calledOnce).to.be.true // eslint-disable-line + expect(mockNext.args[0][0]).to.not.exist // eslint-disable-line + }) + + it('should reject invalid token', () => { + mockReq.method = 'POST' + mockReq.body.testToken = 'invalid-token' + + middleware.checkToken(mockReq, mockRes, mockNext) + + expect(mockNext.calledWith(sinon.match.instanceOf(Error))).to.be.true // eslint-disable-line + expect(mockNext.args[0][0].message).to.equal('Invalid CSRF token') + }) + }) + + describe('generateToken', () => { + it('should generate token and set in res.locals', () => { + mockReq.session.testSecret = csrf().secretSync() + + middleware.generateToken(mockReq, mockRes, mockNext) + + expect(mockRes.locals.csrf).to.exist // eslint-disable-line + expect(typeof mockRes.locals.csrf).to.equal('string') + expect(mockNext.calledOnce).to.be.true // eslint-disable-line + }) + + it('should generate valid token that passes verification', () => { + mockReq.session.testSecret = csrf().secretSync() + + middleware.generateToken(mockReq, mockRes, mockNext) + + const generatedToken = mockRes.locals.csrf + const isValid = csrf().verify(mockReq.session.testSecret, generatedToken) + + expect(isValid).to.be.true // eslint-disable-line + }) + }) +}) From 9a153c6aa4ca67c30af661d84510db06a03b7434 Mon Sep 17 00:00:00 2001 From: Nathaniel Steers Date: Mon, 16 Dec 2024 18:58:30 +0000 Subject: [PATCH 2/2] PP-13360 csrf error class --- .secrets.baseline | 8 ++- src/utils/middleware/csrf.middleware.js | 64 ++++++++++---------- src/utils/middleware/csrf.middleware.test.js | 12 ++-- 3 files changed, 45 insertions(+), 39 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index f6a1793d..f30ab5c8 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -75,6 +75,10 @@ { "path": "detect_secrets.filters.allowlist.is_line_allowlisted" }, + { + "path": "detect_secrets.filters.common.is_baseline_file", + "filename": ".secrets.baseline" + }, { "path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies", "min_level": 2 @@ -123,9 +127,9 @@ "filename": "src/utils/middleware/csrf.middleware.js", "hashed_secret": "fb23222a82d3fc0120ff287e546fee1be335c81a", "is_verified": false, - "line_number": 9 + "line_number": 16 } ] }, - "generated_at": "2024-12-16T16:47:12Z" + "generated_at": "2024-12-16T18:58:18Z" } diff --git a/src/utils/middleware/csrf.middleware.js b/src/utils/middleware/csrf.middleware.js index a9b1671e..ca35ff2f 100644 --- a/src/utils/middleware/csrf.middleware.js +++ b/src/utils/middleware/csrf.middleware.js @@ -1,31 +1,23 @@ const csrf = require('csrf') +class CsrfError extends Error { + constructor (message) { + super(message) + this.name = 'CsrfError' + } +} + /** * @param logger {Logger} - * @param sessionKey {string} - * @param secretKey {string} - * @param tokenKey {string} + * @param sessionName {string} the name of the object on the request where the secret key will be stored + * @param secretName {string} the name of the key in session object that will hold the secret value + * @param tokenName {string} the name of the key on the request body/query that will hold the token value */ -const configureCsrfMiddleware = (logger, sessionKey, secretKey = 'csrfSecret', tokenKey = 'csrfToken') => { +const configureCsrfMiddleware = (logger, sessionName, secretName = 'csrfSecret', tokenName = 'csrfToken') => { logger.debug('--- CSRF middleware configuration ---') - logger.debug(`Secret is set at req.[${sessionKey}][${secretKey}]`) - logger.debug(`Token is checked at req.body|query[${tokenKey}]`) + logger.debug(`Secret is set at req['${sessionName}']['${secretName}']`) + logger.debug(`Token is checked at req.body|query['${tokenName}']`) logger.debug('-------------------------------------') - /** - * @param csrfToken {string} - * @param csrfSecret {string} - * @param req {e.Request} - */ - const csrfValid = (csrfToken, csrfSecret, req) => { - if (!csrfSecret) { - logger.debug('CSRF secret not found when validating token') - return false - } - if (!['put', 'post'].includes(req.method.toLowerCase())) { - return true - } - return csrf().verify(csrfSecret, csrfToken) - } /** * @param req {e.Request} @@ -33,10 +25,10 @@ const configureCsrfMiddleware = (logger, sessionKey, secretKey = 'csrfSecret', t * @param next {e.NextFunction} */ const setSecret = (req, res, next) => { - const csrfSecret = req[sessionKey][secretKey] + const csrfSecret = req[sessionName]?.[secretName] if (!csrfSecret) { logger.debug('Synchronising CSRF secret') - req[sessionKey][secretKey] = csrf().secretSync() + req[sessionName][secretName] = csrf().secretSync() } next() } @@ -47,13 +39,22 @@ const configureCsrfMiddleware = (logger, sessionKey, secretKey = 'csrfSecret', t * @param next {e.NextFunction} */ const checkToken = (req, res, next) => { - const csrfSecret = req[sessionKey][secretKey] - const csrfToken = (req.body && req.body[tokenKey]) || (req.query && req.query[tokenKey]) - if (!csrfValid(csrfToken, csrfSecret, req)) { - next(new Error('Invalid CSRF token')) - } else { - next() + // short circuit the check if method is not PUT/POST + if (!['PUT', 'POST'].includes(req.method.toUpperCase())) { + return next() + } + const csrfSecret = req[sessionName]?.[secretName] + const csrfToken = req.body?.[tokenName] || req.query?.[tokenName] + if (!csrfSecret) { + return next(new CsrfError(`CSRF secret was not found on ${sessionName} when validating token`)) } + if (!csrfToken) { + return next(new CsrfError('CSRF token was not found in body or query for PUT/POST request')) + } + if (!csrf().verify(csrfSecret, csrfToken)) { + return next(new CsrfError('Invalid CSRF token')) + } + next() } /** @@ -62,7 +63,7 @@ const configureCsrfMiddleware = (logger, sessionKey, secretKey = 'csrfSecret', t * @param next {e.NextFunction} */ const generateToken = (req, res, next) => { - const csrfSecret = req[sessionKey][secretKey] + const csrfSecret = req[sessionName][secretName] res.locals.csrf = csrf().create(csrfSecret) next() } @@ -75,5 +76,6 @@ const configureCsrfMiddleware = (logger, sessionKey, secretKey = 'csrfSecret', t } module.exports = { - configureCsrfMiddleware + configureCsrfMiddleware, + CsrfError } diff --git a/src/utils/middleware/csrf.middleware.test.js b/src/utils/middleware/csrf.middleware.test.js index 21cf1773..f4f372bb 100644 --- a/src/utils/middleware/csrf.middleware.test.js +++ b/src/utils/middleware/csrf.middleware.test.js @@ -1,5 +1,5 @@ const { expect } = require('chai') -const { configureCsrfMiddleware } = require('./csrf.middleware') +const { configureCsrfMiddleware, CsrfError } = require('./csrf.middleware') const csrf = require('csrf') const sinon = require('sinon') @@ -78,8 +78,8 @@ describe('CSRF Middleware', () => { middleware.checkToken(mockReq, mockRes, mockNext) - expect(mockNext.calledWith(sinon.match.instanceOf(Error))).to.be.true // eslint-disable-line - expect(mockNext.args[0][0].message).to.equal('Invalid CSRF token') + expect(mockNext.calledWith(sinon.match.instanceOf(CsrfError))).to.be.true // eslint-disable-line + expect(mockNext.args[0][0].message).to.equal('CSRF token was not found in body or query for PUT/POST request') }) it('should reject PUT requests without token', () => { @@ -87,8 +87,8 @@ describe('CSRF Middleware', () => { middleware.checkToken(mockReq, mockRes, mockNext) - expect(mockNext.calledWith(sinon.match.instanceOf(Error))).to.be.true // eslint-disable-line - expect(mockNext.args[0][0].message).to.equal('Invalid CSRF token') + expect(mockNext.calledWith(sinon.match.instanceOf(CsrfError))).to.be.true // eslint-disable-line + expect(mockNext.args[0][0].message).to.equal('CSRF token was not found in body or query for PUT/POST request') }) it('should accept valid token in request body', () => { @@ -117,7 +117,7 @@ describe('CSRF Middleware', () => { middleware.checkToken(mockReq, mockRes, mockNext) - expect(mockNext.calledWith(sinon.match.instanceOf(Error))).to.be.true // eslint-disable-line + expect(mockNext.calledWith(sinon.match.instanceOf(CsrfError))).to.be.true // eslint-disable-line expect(mockNext.args[0][0].message).to.equal('Invalid CSRF token') }) })