From 1c1111f1b30fcaaab4892c2e3cd9eddf5b25ccc6 Mon Sep 17 00:00:00 2001 From: Nathaniel Steers Date: Wed, 31 Jul 2024 17:36:56 +0100 Subject: [PATCH] PP-12546 remove csrf secret from charge objects this change is part of a wider series that are being merged in sequence to prevent the need for a downtime deployment - do not set charge level csrf secret - do not validate tokens against charge level csrf secret - update tests to reflect new behaviour --- app/controllers/secure.controller.js | 3 --- app/middleware/csrf.js | 5 ++-- test/middleware/csrf.test.js | 34 ++-------------------------- test/test-helpers/session.js | 1 - test/test.env | 1 - 5 files changed, 4 insertions(+), 40 deletions(-) diff --git a/app/controllers/secure.controller.js b/app/controllers/secure.controller.js index 17452d3f8..7fc300a12 100644 --- a/app/controllers/secure.controller.js +++ b/app/controllers/secure.controller.js @@ -1,7 +1,5 @@ 'use strict' -const csrf = require('csrf') - const { PAYMENT_EXTERNAL_ID, GATEWAY_ACCOUNT_ID, @@ -54,7 +52,6 @@ exports.new = async function (req, res) { cookies.deleteSessionVariable(req, chargeToDelete) } cookies.setSessionChargeState(req, createChargeIdSessionKey(chargeId), new ChargeState()) - cookies.setSessionVariable(req, createChargeIdSessionKey(chargeId), { csrfSecret: csrf().secretSync() }) // TODO: remove after PP-12546 has been merged res.redirect(303, generateRoute(resolveActionName(chargeStatus, 'get'), { chargeId })) } } catch (err) { diff --git a/app/middleware/csrf.js b/app/middleware/csrf.js index 71dd31ab6..077e6993d 100644 --- a/app/middleware/csrf.js +++ b/app/middleware/csrf.js @@ -31,11 +31,10 @@ exports.csrfCheck = (req, res, next) => { return responseRouter.response(req, res, 'UNAUTHORISED') } - const chargeSession = session.retrieve(req, chargeId) || {} // TODO: remove after PP-12546 has been merged const sessionCsrfSecret = cookies.getSessionCsrfSecret(req) const csrfToken = req.body.csrfToken - if (!chargeSession.csrfSecret && !sessionCsrfSecret) { + if (!sessionCsrfSecret) { responseRouter.response(req, res, 'UNAUTHORISED') logger.warn('CSRF secret is not defined', { ...getLoggingFields(req), @@ -43,7 +42,7 @@ exports.csrfCheck = (req, res, next) => { url: req.originalUrl, method: req.method }) - } else if (!csrfValid(csrfToken, chargeSession.csrfSecret, req) && !csrfValid(csrfToken, sessionCsrfSecret, req)) { + } else if (!csrfValid(csrfToken, sessionCsrfSecret, req)) { responseRouter.systemErrorResponse(req, res, 'CSRF is invalid') } else { next() diff --git a/test/middleware/csrf.test.js b/test/middleware/csrf.test.js index 2c21afba8..52a0938c7 100644 --- a/test/middleware/csrf.test.js +++ b/test/middleware/csrf.test.js @@ -4,7 +4,6 @@ const _ = require('lodash') const expect = require('chai').expect const nock = require('nock') const helper = require('../test-helpers/test-helpers.js') - const { csrfCheck, csrfTokenGeneration } = require('../../app/middleware/csrf.js') describe('retrieve param test', function () { @@ -22,9 +21,9 @@ describe('retrieve param test', function () { body: {}, route: { methods: { get: true } }, frontend_state: { - csrfSecret: process.env.CSRF_USER_SECRET_TWO, + csrfSecret: process.env.CSRF_USER_SECRET, ch_foo: { - csrfSecret: process.env.CSRF_USER_SECRET + data: 'blah' } }, get: () => null @@ -34,7 +33,6 @@ describe('retrieve param test', function () { delete noCharge.frontend_state.ch_foo const noSecret = _.cloneDeep(validGetRequest) - delete noSecret.frontend_state.ch_foo.csrfSecret delete noSecret.frontend_state.csrfSecret const invalidPost = _.cloneDeep(validGetRequest) @@ -47,16 +45,6 @@ describe('retrieve param test', function () { const validPost = _.cloneDeep(invalidPost) validPost.body.csrfToken = helper.csrfToken() - const validPostWithSessionSecretAndChargeSecretAndChargeSecretGeneratedToken = _.cloneDeep(invalidPost) - validPostWithSessionSecretAndChargeSecretAndChargeSecretGeneratedToken.body.csrfToken = helper.csrfToken(process.env.CSRF_USER_SECRET) - - const validPostWithSessionSecretAndChargeSecretAndSessionSecretGeneratedToken = _.cloneDeep(invalidPost) - validPostWithSessionSecretAndChargeSecretAndSessionSecretGeneratedToken.body.csrfToken = helper.csrfToken(process.env.CSRF_USER_SECRET_TWO) - - const validPostWithSessionSecretAndSessionSecretGeneratedToken = _.cloneDeep(invalidPost) - delete validPostWithSessionSecretAndSessionSecretGeneratedToken.frontend_state.ch_foo.csrfSecret - validPostWithSessionSecretAndSessionSecretGeneratedToken.body.csrfToken = helper.csrfToken(process.env.CSRF_USER_SECRET_TWO) - const validPut = _.cloneDeep(invalidPut) validPut.body.csrfToken = helper.csrfToken() @@ -122,24 +110,6 @@ describe('retrieve param test', function () { assertValidRequest(next, resp, status, render) }) - it('should use charge secret for csrf check when session secret, charge secret and charge secret generated token are present', function () { - const resp = _.cloneDeep(response) - csrfCheck(validPostWithSessionSecretAndChargeSecretAndChargeSecretGeneratedToken, resp, next) - expect(next.called).to.be.true // eslint-disable-line - }) - - it('should use session secret for csrf check when session secret, charge secret and session secret generated token are present', function () { - const resp = _.cloneDeep(response) - csrfCheck(validPostWithSessionSecretAndChargeSecretAndSessionSecretGeneratedToken, resp, next) - expect(next.called).to.be.true // eslint-disable-line - }) - - it('should use session secret for csrf check when only session secret is set', function () { - const resp = _.cloneDeep(response) - csrfCheck(validPostWithSessionSecretAndSessionSecretGeneratedToken, resp, next) - expect(next.called).to.be.true // eslint-disable-line - }) - it('should error if no csrfToken in put request', function () { const resp = _.cloneDeep(response) csrfCheck(invalidPut, resp, next) diff --git a/test/test-helpers/session.js b/test/test-helpers/session.js index 67e29e48d..c361b324a 100644 --- a/test/test-helpers/session.js +++ b/test/test-helpers/session.js @@ -14,7 +14,6 @@ function createReturnUrlKey (chargeId) { function createSessionWithReturnUrl (chargeId, chargeSession, returnUrl) { chargeSession = chargeSession || { data: new ChargeState().toString() } - chargeSession.csrfSecret = process.env.CSRF_USER_SECRET const session = {} if (arguments.length > 0) { session.csrfSecret = process.env.CSRF_USER_SECRET diff --git a/test/test.env b/test/test.env index a44468199..a43c288e8 100644 --- a/test/test.env +++ b/test/test.env @@ -4,7 +4,6 @@ SECURE_COOKIE_OFF=false COOKIE_MAX_AGE=5400000 SESSION_ENCRYPTION_KEY=naskjwefvwei72rjkwfmjwfi72rfkjwefmjwefiuwefjkbwfiu24fmjbwfk CSRF_USER_SECRET=123456789012345678 -CSRF_USER_SECRET_TWO=987654321012345678 CARDID_HOST=http://cardid.pymnt.localdomain:65530 WORLDPAY_APPLE_PAY_MERCHANT_ID=merchant.uk.gov.service.payments.test STRIPE_APPLE_PAY_MERCHANT_ID=merchant.uk.gov.service.payments.stripe.test