diff --git a/.secrets.baseline b/.secrets.baseline index 4fba8e27e..9363d40f3 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -372,7 +372,7 @@ "filename": "test/unit/cookies.test.js", "hashed_secret": "ef5c25da3f68da43b33a9bc96de633756beb2d88", "is_verified": false, - "line_number": 35 + "line_number": 36 } ], "test/utils/normalise.test.js": [ @@ -385,5 +385,5 @@ } ] }, - "generated_at": "2024-07-29T10:56:11Z" + "generated_at": "2024-07-30T15:02:23Z" } diff --git a/app/controllers/return.controller.js b/app/controllers/return.controller.js index c61b46f88..06d821ebf 100644 --- a/app/controllers/return.controller.js +++ b/app/controllers/return.controller.js @@ -6,15 +6,24 @@ const Charge = require('../models/charge') const responseRouter = require('../utils/response-router') const { CORRELATION_HEADER } = require('../../config/correlation-header') const { withAnalyticsError } = require('../utils/analytics') -const cookies = require('../utils/cookies') const { createChargeIdSessionKey } = require('../utils/session') +const cookies = require('../utils/cookies') exports.return = (req, res) => { const correlationId = req.headers[CORRELATION_HEADER] || '' const charge = Charge(correlationId) - // Remove the charge data from the cookie - const cookieKey = createChargeIdSessionKey(req.chargeId) - cookies.deleteSessionVariable(req, cookieKey) + const chargeKey = createChargeIdSessionKey(req.chargeId) + const chargeState = cookies.getSessionChargeState(req, chargeKey) + if (chargeState) { + chargeState.markTerminal() + cookies.setSessionChargeState(req, chargeKey, chargeState) + } else { + /* TODO: remove after PP-12546 has been merged + / we no longer need to delete the session variable in the return controller as the cookie is cleaned up in the + / secure controller, we're just cleaning up any existing payment journeys prior to the merge of PP-12546 + */ + cookies.deleteSessionVariable(req, chargeKey) + } if (charge.isCancellableCharge(req.chargeData.status)) { return charge.cancel(req.chargeId, getLoggingFields(req)) .then(() => logger.warn('Return controller cancelled payment', getLoggingFields(req))) diff --git a/app/controllers/secure.controller.js b/app/controllers/secure.controller.js index 170c334ce..17452d3f8 100644 --- a/app/controllers/secure.controller.js +++ b/app/controllers/secure.controller.js @@ -1,7 +1,7 @@ 'use strict' -// NPM dependencies const csrf = require('csrf') + const { PAYMENT_EXTERNAL_ID, GATEWAY_ACCOUNT_ID, @@ -16,11 +16,12 @@ const Token = require('../models/token') const Charge = require('../models/charge') const responseRouter = require('../utils/response-router') const { createChargeIdSessionKey } = require('../utils/session') -const { setSessionVariable, getSessionVariable } = require('../utils/cookies') +const cookies = require('../utils/cookies') const CORRELATION_HEADER = require('../../config/correlation-header').CORRELATION_HEADER const withAnalyticsError = require('../utils/analytics').withAnalyticsError const { resolveActionName } = require('../services/state.service') const paths = require('../paths') +const { ChargeState } = require('../models/ChargeState') exports.new = async function (req, res) { const chargeTokenId = req.params.chargeTokenId || req.body.chargeTokenId @@ -36,7 +37,7 @@ exports.new = async function (req, res) { setLoggingField(req, GATEWAY_ACCOUNT_TYPE, tokenResponse.charge.gateway_account.type) if (tokenResponse.used === true) { - if (!getSessionVariable(req, createChargeIdSessionKey(chargeId))) { + if (!cookies.getSessionVariable(req, createChargeIdSessionKey(chargeId))) { throw new Error('UNAUTHORISED') } logger.info('Payment token being reused', getLoggingFields(req)) @@ -48,7 +49,12 @@ exports.new = async function (req, res) { } else { logger.info('Payment token used for the first time', getLoggingFields(req)) await Token.markTokenAsUsed(chargeTokenId, correlationId, getLoggingFields(req)) - setSessionVariable(req, createChargeIdSessionKey(chargeId), { csrfSecret: csrf().secretSync() }) + if (cookies.getChargesOnSession(req).length >= 10) { + const chargeToDelete = cookies.findSessionChargeToDelete(req) + 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 029118b58..7b4133d47 100644 --- a/app/middleware/csrf.js +++ b/app/middleware/csrf.js @@ -8,11 +8,21 @@ const logger = require('../utils/logger')(__filename) const { getLoggingFields } = require('../utils/logging-fields-helper') const session = require('../utils/session') const responseRouter = require('../utils/response-router') +const cookies = require('../utils/cookies') + +exports.csrfSetSecret = (req, _, next) => { + const csrfSecret = cookies.getSessionCsrfSecret(req) + if (!csrfSecret) { + logger.info('Setting CSRF secret for session') + cookies.setSessionVariable(req, 'csrfSecret', csrf().secretSync()) + } + next() +} exports.csrfTokenGeneration = (req, res, next) => { const chargeId = fetchAndValidateChargeId(req) const chargeSession = session.retrieve(req, chargeId) - res.locals.csrf = csrf().create(chargeSession.csrfSecret) + res.locals.csrf = csrf().create(chargeSession.csrfSecret) // TODO: generate tokens using the session secret next() } @@ -23,11 +33,11 @@ exports.csrfCheck = (req, res, next) => { return responseRouter.response(req, res, 'UNAUTHORISED') } - const chargeSession = session.retrieve(req, chargeId) || {} + const chargeSession = session.retrieve(req, chargeId) || {} // TODO: remove after PP-12546 has been merged + const sessionCsrfSecret = cookies.getSessionCsrfSecret(req) const csrfToken = req.body.csrfToken - chargeSession.csrfTokens = chargeSession.csrfTokens || [] - if (!chargeSession.csrfSecret) { + if (!chargeSession.csrfSecret && !sessionCsrfSecret) { responseRouter.response(req, res, 'UNAUTHORISED') logger.warn('CSRF secret is not defined', { ...getLoggingFields(req), @@ -35,10 +45,9 @@ exports.csrfCheck = (req, res, next) => { url: req.originalUrl, method: req.method }) - } else if (!csrfValid(csrfToken, chargeSession, req)) { + } else if (!csrfValid(csrfToken, chargeSession.csrfSecret, req) && !csrfValid(csrfToken, sessionCsrfSecret, req)) { responseRouter.systemErrorResponse(req, res, 'CSRF is invalid') } else { - chargeSession.csrfTokens.push(csrfToken) next() } } @@ -50,13 +59,13 @@ function fetchAndValidateChargeId (req) { return false } -function csrfValid (csrfToken, chargeSession, req) { +function csrfValid (csrfToken, secret, req) { + if (!secret) { + return false + } if (!['put', 'post'].includes(req.method.toLowerCase())) { return true - } else if (chargeSession.csrfTokens.includes(csrfToken)) { - logger.warn('CSRF token was already used', getLoggingFields(req)) - return false } else { - return csrf().verify(chargeSession.csrfSecret, csrfToken) + return csrf().verify(secret, csrfToken) } } diff --git a/app/models/ChargeState.js b/app/models/ChargeState.js new file mode 100644 index 000000000..aace8e649 --- /dev/null +++ b/app/models/ChargeState.js @@ -0,0 +1,52 @@ +const logger = require('../utils/logger')(__filename) + +class ChargeState { + /** + * @param {Number} [createdAt] + * @param {Number} [accessedAt] + * @param {boolean} [isTerminal] + */ + constructor (createdAt, accessedAt, isTerminal) { + this.createdAt = createdAt || epochSecondsNow() + this.accessedAt = accessedAt || epochSecondsNow() + this.isTerminal = isTerminal || false + } + + updateAccessedAt () { + this.accessedAt = epochSecondsNow() + } + + markTerminal () { + this.updateAccessedAt() + this.isTerminal = true + } + + toString () { + return `${this.createdAt},${this.accessedAt},${this.isTerminal ? 'T' : 'F'}` + } +} + +const chargeStateFromString = (data) => { + try { + const dataParts = data.split(',') + if (dataParts.length === 3) { + return new ChargeState(Number(dataParts[0]), Number(dataParts[1]), dataParts[2] === 'T') + } else { + logger.error('argument is not a valid ChargeState') + return null + } + } catch (e) { + logger.warn(`Error de-serialising ChargeState from string: ${e.message}`) + return null + } +} + +const epochSecondsNow = () => { + return Math.floor(Date.now() / 1000) +} + +module.exports = { + ChargeState, + chargeStateFromString, + epochSecondsNow +} diff --git a/app/routes.js b/app/routes.js index 67c40c3ac..43ab0b2ed 100644 --- a/app/routes.js +++ b/app/routes.js @@ -14,7 +14,7 @@ const { log } = require('./controllers/client-side-logging.controller') const paths = require('./paths.js') // Express middleware -const { csrfCheck, csrfTokenGeneration } = require('./middleware/csrf.js') +const { csrfSetSecret, csrfCheck, csrfTokenGeneration } = require('./middleware/csrf.js') const actionName = require('./middleware/action-name.js') const stateEnforcer = require('./middleware/state-enforcer.js') const retrieveCharge = require('./middleware/retrieve-charge.js') @@ -43,6 +43,7 @@ exports.bind = function (app) { const card = paths.card const standardMiddlewareStack = [ + csrfSetSecret, csrfCheck, csrfTokenGeneration, actionName, diff --git a/app/services/clients/connector.client.js b/app/services/clients/connector.client.js index 3f6addbe8..fcea78b95 100644 --- a/app/services/clients/connector.client.js +++ b/app/services/clients/connector.client.js @@ -74,7 +74,7 @@ async function _putConnector (url, payload, description, loggingFields = {}, cal requestLogger.logRequestStart(context, loggingFields) try { configureClient(client, url) - const response = await client.put(url, payload, description ) + const response = await client.put(url, payload, description) logger.info('PUT to %s ended - total time %dms', url, new Date() - startTime, loggingFields) incrementStatusCodeCounter(callingFunctionName, response.status) if (response.status > 499 && response.status < 600) { diff --git a/app/utils/cookies.js b/app/utils/cookies.js index 16faee363..1721979c4 100644 --- a/app/utils/cookies.js +++ b/app/utils/cookies.js @@ -1,6 +1,7 @@ 'use strict' const clientSessions = require('client-sessions') +const { chargeStateFromString, epochSecondsNow } = require('../models/ChargeState') const logger = require('../utils/logger')(__filename) // Constants @@ -17,7 +18,11 @@ module.exports = { namedCookie, deleteSessionVariable, isSessionPresent, - getSessionVariableNames + getChargesOnSession, + getSessionCsrfSecret, + getSessionChargeState, + setSessionChargeState, + findSessionChargeToDelete } /** @@ -133,6 +138,49 @@ function deleteSessionVariable (req, key) { } } +/** + * @param {Request} req + * @returns {string} + */ +function findSessionChargeToDelete (req) { + const now = epochSecondsNow() + const yesterday = now - 86400 + const ninetyMinutesAgo = now - 5400 + + const chargesSortedByAccessedDate = getChargesOnSession(req) + .map(chargeId => { + return { + chargeId, + state: getSessionChargeState(req, chargeId) + } + }) + .sort(({ state: a }, { state: b }) => a.accessedAt - b.accessedAt) + + logger.info(`User session contains ${chargesSortedByAccessedDate.length} charges`) + + const leastRecentlyAccessedChargeCreatedMoreThanTwentyFourHoursAgo = chargesSortedByAccessedDate.find(charge => charge.state.createdAt < yesterday) + + if (leastRecentlyAccessedChargeCreatedMoreThanTwentyFourHoursAgo) { + return leastRecentlyAccessedChargeCreatedMoreThanTwentyFourHoursAgo.chargeId + } + + const leastRecentlyAccessedTerminalCharge = chargesSortedByAccessedDate.find(charge => charge.state.isTerminal) + + if (leastRecentlyAccessedTerminalCharge) { + return leastRecentlyAccessedTerminalCharge.chargeId + } + + const leastRecentlyAccessedChargeCreatedMoreThanNinetyMinutesAgo = chargesSortedByAccessedDate.find(charge => charge.state.createdAt < ninetyMinutesAgo) + + if (leastRecentlyAccessedChargeCreatedMoreThanNinetyMinutesAgo) { + return leastRecentlyAccessedChargeCreatedMoreThanNinetyMinutesAgo.chargeId + } + + logger.info('No preferred charges found for deletion on session, defaulting to least recently accessed charge') + const leastRecentlyAccessedCharge = chargesSortedByAccessedDate[0] + return leastRecentlyAccessedCharge.chargeId +} + /** * Sets session[key] = value for all valid sessions, based on existence of encryption key, * and the existence of relevant cookie on the request @@ -151,6 +199,18 @@ function setSessionVariable (req, key, value) { } } +/** + * @param {Request} req + * @param {string} key + * @param {ChargeState} chargeState + */ +function setSessionChargeState (req, key, chargeState) { + const chargeStateObject = { + data: chargeState.toString() + } + setSessionVariable(req, key, chargeStateObject) +} + /** * Gets value of key from session, based on existence of encryption key * @@ -160,7 +220,24 @@ function setSessionVariable (req, key, value) { */ function getSessionVariable (req, key) { const session = req[getSessionCookieName()] - return session && session[key] + return session && session?.[key] +} + +/** + * @param {Request} req + * @param {string} key + * @returns {ChargeState} + */ +function getSessionChargeState (req, key) { + return chargeStateFromString(getSessionVariable(req, key).data) +} + +/** + * @param {Request} req + * @returns {string} + */ +function getSessionCsrfSecret (req) { + return getSessionVariable(req, 'csrfSecret') } function isSessionPresent (req) { @@ -168,7 +245,13 @@ function isSessionPresent (req) { return session && Object.getOwnPropertyNames(session).length > 0 } -function getSessionVariableNames (req) { +/** + * @param {Request} req + * @returns {string[]} + */ +function getChargesOnSession (req) { const session = req[getSessionCookieName()] - return Object.keys(session) + return Object.keys(session).filter(key => { + return key.startsWith('ch_') && Object.keys(session[key]).includes('data') + }) } diff --git a/app/utils/session.js b/app/utils/session.js index 22ebfe1f5..585057bcb 100644 --- a/app/utils/session.js +++ b/app/utils/session.js @@ -4,14 +4,14 @@ const { PAYMENT_EXTERNAL_ID } = require('@govuk-pay/pay-js-commons').logging.keys const logger = require('../utils/logger')(__filename) const { setLoggingField, getLoggingFields } = require('../utils/logging-fields-helper') -const cookie = require('../utils/cookies') +const cookies = require('../utils/cookies') const createChargeIdSessionKey = function createChargeIdSessionKey (chargeId) { return 'ch_' + chargeId } const retrieve = function retrieve (req, chargeId) { - return cookie.getSessionVariable(req, createChargeIdSessionKey(chargeId)) + return cookies.getSessionVariable(req, createChargeIdSessionKey(chargeId)) } const validateSessionCookie = function validateSessionCookie (req) { @@ -21,7 +21,9 @@ const validateSessionCookie = function validateSessionCookie (req) { if (!chargeId) { logger.error('ChargeId was not found in request', getLoggingFields(req)) return false - } else if (!cookie.getSessionCookieName() || !cookie.isSessionPresent(req)) { + } + const chargeKey = createChargeIdSessionKey(chargeId) + if (!cookies.getSessionCookieName() || !cookies.isSessionPresent(req)) { logger.info('Session cookie is not present', { ...getLoggingFields(req), referrer: req.get('Referrer'), @@ -29,20 +31,33 @@ const validateSessionCookie = function validateSessionCookie (req) { method: req.method }) return false - } else if (!cookie.getSessionVariable(req, 'ch_' + chargeId)) { + } else if (!cookies.getSessionVariable(req, chargeKey)) { logger.info('ChargeId was not found on the session', { ...getLoggingFields(req), referrer: req.get('Referrer'), url: req.originalUrl, method: req.method, - session_keys: cookie.getSessionVariableNames(req) + session_keys: cookies.getChargesOnSession(req) }) return false } logger.info('ChargeId found on session', { ...getLoggingFields(req), - number_of_payments_on_cookie: cookie.getSessionVariableNames(req).length + number_of_payments_on_cookie: cookies.getChargesOnSession(req).length }) + const chargeState = cookies.getSessionChargeState(req, chargeKey) + if (chargeState) { + chargeState.updateAccessedAt() + cookies.setSessionChargeState(req, createChargeIdSessionKey(chargeId), chargeState) + } else { + /* TODO remove this else condition post merge of PP-12546 + / this is here to support any existing payment journeys + */ + logger.warn('ChargeId found on session but charge state was not found', { + ...getLoggingFields(req) + }) + } + return true } diff --git a/server.js b/server.js index ac25395de..bf19c1a15 100644 --- a/server.js +++ b/server.js @@ -18,7 +18,6 @@ const loggingMiddleware = require('./app/middleware/logging-middleware') const router = require('./app/routes') const cookies = require('./app/utils/cookies') const noCache = require('./app/utils/no-cache') -const session = require('./app/utils/session') const i18nConfig = require('./config/i18n') const i18nPayTranslation = require('./config/pay-translation') const Sentry = require('./app/utils/sentry.js').initialiseSentry() @@ -71,9 +70,6 @@ function initialiseGlobalMiddleware (app) { } else { res.locals.analyticsTrackingId = ANALYTICS_TRACKING_ID } - res.locals.session = function () { - return session.retrieve(req, req.chargeId) - } res.locals.nonce = crypto.randomBytes(16).toString('hex') next() }) @@ -85,7 +81,6 @@ function initialiseGlobalMiddleware (app) { app.disable('x-powered-by') - app.use(correlationHeader) } diff --git a/test/controllers/return.controller.test.js b/test/controllers/return.controller.test.js index 780ea4af0..4a5e2df9f 100644 --- a/test/controllers/return.controller.test.js +++ b/test/controllers/return.controller.test.js @@ -6,9 +6,11 @@ const chaiAsPromised = require('chai-as-promised') const assert = require('assert') const sinon = require('sinon') const nock = require('nock') +const { ChargeState, chargeStateFromString } = require('../../app/models/ChargeState') const expect = chai.expect chai.use(chaiAsPromised) +chai.should() const requireReturnController = function () { const mocks = { @@ -26,10 +28,16 @@ const requireReturnController = function () { describe('return controller', function () { let request, response + const chargeId = 'aChargeId' beforeEach(function () { request = { - chargeId: 'aChargeId', + frontend_state: { + [`ch_${chargeId}`]: { + data: new ChargeState().toString() + } + }, + chargeId, headers: { 'x-Request-id': 'unique-id' } } @@ -47,6 +55,8 @@ describe('return controller', function () { } requireReturnController().return(request, response) + const chargeStateUnderTest = chargeStateFromString(request.frontend_state[`ch_${chargeId}`].data) + expect(chargeStateUnderTest.isTerminal).to.equal(true) assert(response.redirect.calledWith('http://a_return_url.com')) }) @@ -57,13 +67,15 @@ describe('return controller', function () { } nock(process.env.CONNECTOR_HOST) - .post('/v1/frontend/charges/aChargeId/cancel') + .post(`/v1/frontend/charges/${chargeId}/cancel`) .reply(204) requireReturnController().return(request, response) .should.be.fulfilled.then(() => { assert(response.redirect.calledWith('http://a_return_url.com')) }).should.notify(done) + const chargeStateUnderTest = chargeStateFromString(request.frontend_state[`ch_${chargeId}`].data) + expect(chargeStateUnderTest.isTerminal).to.equal(true) }) it('should show an error if cancel fails', function (done) { @@ -72,7 +84,7 @@ describe('return controller', function () { return_url: 'http://a_return_url.com' } nock(process.env.CONNECTOR_HOST) - .post('/v1/frontend/charges/aChargeId/cancel') + .post(`/v1/frontend/charges/${chargeId}/cancel`) .reply(500) requireReturnController().return(request, response) diff --git a/test/controllers/secure.controller.test.js b/test/controllers/secure.controller.test.js index 331a642fb..3e64cb370 100644 --- a/test/controllers/secure.controller.test.js +++ b/test/controllers/secure.controller.test.js @@ -9,9 +9,10 @@ const withAnalyticsError = require('../../app/utils/analytics').withAnalyticsErr const { validTokenResponse } = require('../fixtures/payment.fixtures') require('../test-helpers/html-assertions.js') +const { ChargeState, chargeStateFromString } = require('../../app/models/ChargeState') const chargeId = 'dh6kpbb4k82oiibbe4b9haujjk' - +const chargeStateString = new ChargeState(1721300000, 1721300000, false).toString() const findByTokenFailureStub = () => Promise.reject(new Error('UNAUTHORISED')) function getFindByTokenSuccessStub (used, chargeStatus) { @@ -32,10 +33,7 @@ const requireSecureController = function (findByTokenStub, markTokenAsUsedSucces }), '../models/token.js': { markTokenAsUsed: markTokenAsUsedStub - }, - csrf: () => ({ - secretSync: () => 'foo' - }) + } }) } @@ -80,13 +78,58 @@ describe('secure controller', function () { }) describe('and the token is marked as used successfully', function () { - it('should store the service name into the session and redirect', async function () { + it('should store the charge on the session and redirect', async function () { await requireSecureController(getFindByTokenSuccessStub(false, 'CREATED'), true, responseRouter).new(request, response) expect(response.redirect.calledWith(303, paths.generateRoute('card.new', {chargeId: chargeId}))).to.be.true // eslint-disable-line expect(request.frontend_state).to.have.all.keys('ch_' + chargeId) - expect(request.frontend_state.ch_dh6kpbb4k82oiibbe4b9haujjk).to.eql({ - csrfSecret: 'foo' // pragma: allowlist secret - }) + const result = chargeStateFromString(request.frontend_state[`ch_${chargeId}`].data) + expect(result.accessedAt).to.equal(result.createdAt) // when charges are added to the session by the secure controller, createdAt and accessedAt should be the same + expect(result.isTerminal).to.equal(false) + }) + + it('should remove a charge from the session if there are more than 10', async function () { + const data = new ChargeState(1721390050, 1721390080, false).toString() + const shouldRemove = new ChargeState(1721390000, 1721390020, true).toString() + const requestWithTenChargesOnCookie = { + frontend_state: { + ch_1: { + data + }, + ch_2: { + data + }, + ch_3: { + data + }, + ch_4: { + data + }, + ch_5: { + data + }, + ch_6: { + data + }, + ch_7: { + data + }, + ch_8: { + data + }, + ch_9: { + data: shouldRemove + }, + ch_10: { + data + } + }, + params: { chargeTokenId: 1 }, + headers: { 'x-Request-id': 'unique-id' } + } + await requireSecureController(getFindByTokenSuccessStub(false, 'CREATED'), true, responseRouter).new(requestWithTenChargesOnCookie, response) + expect(response.redirect.calledWith(303, paths.generateRoute('card.new', {chargeId: chargeId}))).to.be.true // eslint-disable-line + expect(requestWithTenChargesOnCookie.frontend_state).to.have.all.keys(`ch_${chargeId}`, 'ch_1', 'ch_2', 'ch_3', 'ch_4', 'ch_5', 'ch_6', 'ch_7', 'ch_8', 'ch_10') + expect(requestWithTenChargesOnCookie.frontend_state).to.not.have.key('ch_9') }) }) @@ -118,7 +161,7 @@ describe('secure controller', function () { const requestWithWrongCookie = { frontend_state: { ch_xxxx: { - csrfSecret: 'foo' // pragma: allowlist secret + data: chargeStateString } }, params: { chargeTokenId: 1 }, @@ -134,7 +177,7 @@ describe('secure controller', function () { const requestWithFrontendStateCookie = { frontend_state: { ch_dh6kpbb4k82oiibbe4b9haujjk: { - csrfSecret: 'foo' // pragma: allowlist secret + data: chargeStateString } }, params: { chargeTokenId: 1 }, @@ -149,7 +192,7 @@ describe('secure controller', function () { expect(responseRouter.response.calledWith(requestWithFrontendStateCookie, response, 'AUTHORISATION_SUCCESS', opts)).to.be.true // eslint-disable-line expect(requestWithFrontendStateCookie.frontend_state).to.have.all.keys('ch_' + chargeId) expect(requestWithFrontendStateCookie.frontend_state.ch_dh6kpbb4k82oiibbe4b9haujjk).to.eql({ - csrfSecret: 'foo' // pragma: allowlist secret + data: chargeStateString }) }) }) diff --git a/test/cypress/integration/web-payments/apple-pay.test.cy.js b/test/cypress/integration/web-payments/apple-pay.test.cy.js index aea879b54..d51a526df 100644 --- a/test/cypress/integration/web-payments/apple-pay.test.cy.js +++ b/test/cypress/integration/web-payments/apple-pay.test.cy.js @@ -467,12 +467,12 @@ describe('Apple Pay payment flow', () => { cy.task('setupStubs', createPaymentChargeStubsWelsh) cy.visit(`/secure/${tokenId}`) - // 1. Charge will be created using this id as a token (GET) - // 2. Token will be marked as used (POST) - // 3. Charge will be fetched (GET) - // 4. Service related to charge will be fetched (GET) - // 5. Charge status will be updated (PUT) - // 6. Client will be redirected to /card_details/:chargeId (304) + // 1. Charge will be created using this id as a token (GET) + // 2. Token will be marked as used (POST) + // 3. Charge will be fetched (GET) + // 4. Service related to charge will be fetched (GET) + // 5. Charge status will be updated (PUT) + // 6. Client will be redirected to /card_details/:chargeId (304) cy.location('pathname').should('eq', `/card_details/${chargeId}`) cy.task('clearStubs') diff --git a/test/integration/card-details-errors.ft.test.js b/test/integration/card-details-errors.ft.test.js index 9dbc7aabc..99a370179 100644 --- a/test/integration/card-details-errors.ft.test.js +++ b/test/integration/card-details-errors.ft.test.js @@ -217,7 +217,7 @@ describe('chargeTests', function () { }) it('should return country list when invalid fields submitted', (done) => { - const cookieValue = cookie.create(chargeId, {}) + const cookieValue = cookie.create(chargeId, {}) // empty charge session left here to prove backwards compatability prior to merge of PP-12546, remove once backcompat code is deleted mockSuccessCardIdResponse(defaultCardID) mockSuccessPatchEmail(chargeId) defaultConnectorResponseForGetCharge(chargeId, State.ENTERING_CARD_DETAILS, gatewayAccountId) @@ -231,7 +231,7 @@ describe('chargeTests', function () { }) it('shows an error when a card is submitted with missing fields', function (done) { - const cookieValue = cookie.create(chargeId, {}) + const cookieValue = cookie.create(chargeId, {}) // empty charge session left here to prove backwards compatability prior to merge of PP-12546, remove once backcompat code is deleted mockSuccessCardIdResponse(defaultCardID) mockSuccessPatchEmail(chargeId) defaultConnectorResponseForGetCharge(chargeId, State.ENTERING_CARD_DETAILS, gatewayAccountId) @@ -277,7 +277,7 @@ describe('chargeTests', function () { }) it('shows an error when a card is submitted that is not supported', function (done) { - const cookieValue = cookie.create(chargeId, {}) + const cookieValue = cookie.create(chargeId, {}) // empty charge session left here to prove backwards compatability prior to merge of PP-12546, remove once backcompat code is deleted nock.cleanAll() mockSuccessPatchEmail(chargeId) defaultConnectorResponseForGetCharge(chargeId, State.ENTERING_CARD_DETAILS, gatewayAccountId) @@ -304,7 +304,7 @@ describe('chargeTests', function () { }) it('shows an error when a card is submitted that is not supported withdrawal type', function (done) { - const cookieValue = cookie.create(chargeId, {}) + const cookieValue = cookie.create(chargeId, {}) // empty charge session left here to prove backwards compatability prior to merge of PP-12546, remove once backcompat code is deleted nock.cleanAll() nock(process.env.CARDID_HOST) .post('/v1/api/card', () => { @@ -332,7 +332,7 @@ describe('chargeTests', function () { }) it('preserve card fields, cardholder name, address lines and email when a card is submitted with validation errors', function (done) { - const cookieValue = cookie.create(chargeId, {}) + const cookieValue = cookie.create(chargeId, {}) // empty charge session left here to prove backwards compatability prior to merge of PP-12546, remove once backcompat code is deleted defaultConnectorResponseForGetCharge(chargeId, State.ENTERING_CARD_DETAILS, gatewayAccountId) defaultAdminusersResponseForGetService(gatewayAccountId) mockSuccessPatchEmail(chargeId) diff --git a/test/middleware/csrf.test.js b/test/middleware/csrf.test.js index a2d770467..ab9efbd59 100644 --- a/test/middleware/csrf.test.js +++ b/test/middleware/csrf.test.js @@ -29,8 +29,8 @@ describe('retrieve param test', function () { get: () => null } - const noSession = _.cloneDeep(validGetRequest) - delete noSession.frontend_state.ch_foo + const noCharge = _.cloneDeep(validGetRequest) + delete noCharge.frontend_state.ch_foo const noSecret = _.cloneDeep(validGetRequest) delete noSecret.frontend_state.ch_foo.csrfSecret @@ -45,6 +45,19 @@ 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) + validPostWithSessionSecretAndChargeSecretAndChargeSecretGeneratedToken.frontend_state.csrfSecret = process.env.CSRF_USER_SECRET_TWO + + const validPostWithSessionSecretAndChargeSecretAndSessionSecretGeneratedToken = _.cloneDeep(invalidPost) + validPostWithSessionSecretAndChargeSecretAndSessionSecretGeneratedToken.body.csrfToken = helper.csrfToken(process.env.CSRF_USER_SECRET_TWO) + validPostWithSessionSecretAndChargeSecretAndSessionSecretGeneratedToken.frontend_state.csrfSecret = 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) + validPostWithSessionSecretAndSessionSecretGeneratedToken.frontend_state.csrfSecret = process.env.CSRF_USER_SECRET_TWO + const validPut = _.cloneDeep(invalidPut) validPut.body.csrfToken = helper.csrfToken() @@ -79,7 +92,7 @@ describe('retrieve param test', function () { render.restore() }) - it('should append csrf on get request', function () { + it('should append csrf token to response locals on get request', function () { const resp = _.cloneDeep(response) csrfTokenGeneration(validGetRequest, resp, next) assertValidRequest(next, resp, status, render) @@ -87,7 +100,7 @@ describe('retrieve param test', function () { it('should error if no charge in session', function () { const resp = _.cloneDeep(response) - csrfCheck(noSession, resp, next) + csrfCheck(noCharge, resp, next) assertUnauthorisedRequest(next, resp, status, render) }) @@ -97,20 +110,6 @@ describe('retrieve param test', function () { assertUnauthorisedRequest(next, resp, status, render) }) - it('should be successful on post if valid post and append token to used tokens', function () { - const resp = _.cloneDeep(response) - csrfCheck(validPost, resp, next) - csrfTokenGeneration(validGetRequest, resp, next) - assertValidRequest(next, resp, status, render) - assert.strictEqual(validPost.frontend_state.ch_foo.csrfTokens[0], validPost.body.csrfToken) - }) - - it('should be unsuccessful on post if token is already used', function () { - const resp = _.cloneDeep(response) - csrfCheck(validPost, resp, next) - assertErrorRequest(next, resp, status, render) - }) - it('should error if no csrfToken in post request', function () { const resp = _.cloneDeep(response) csrfCheck(invalidPost, resp, next) @@ -124,10 +123,22 @@ describe('retrieve param test', function () { assertValidRequest(next, resp, status, render) }) - it('should be unsuccessful on put if token is already used', function () { + 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(validPut, resp, next) - assertErrorRequest(next, resp, status, render) + 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 () { diff --git a/test/models/ChargeState.test.js b/test/models/ChargeState.test.js new file mode 100644 index 000000000..8713ad217 --- /dev/null +++ b/test/models/ChargeState.test.js @@ -0,0 +1,75 @@ +const expect = require('chai').expect +const sinon = require('sinon') +const { ChargeState, chargeStateFromString } = require('../../app/models/ChargeState') + +describe('ChargeState model should', () => { + let clock + + beforeEach(() => { + const fixedDate = new Date(1721390000000) // 19 July 2024 11:53:20 + clock = sinon.useFakeTimers(fixedDate) + }) + + afterEach(() => { + clock.restore() + }) + + it('create new ChargeState with no parameters', () => { + const result = new ChargeState() + expect(result.isTerminal).to.equal(false) + expect(result.accessedAt).to.equal(1721390000) + expect(result.createdAt).to.equal(1721390000) + }) + it('create new ChargeState with parameters', () => { + const result = new ChargeState(1721390555, 1721390999, true) + expect(result.isTerminal).to.equal(true) + expect(result.accessedAt).to.equal(1721390999) + expect(result.createdAt).to.equal(1721390555) + }) + it('set isComplete to true and update accessedAt when done is called', () => { + const result = new ChargeState() + expect(result.isTerminal).to.equal(false) + expect(result.createdAt).to.equal(1721390000) + expect(result.accessedAt).to.equal(1721390000) + clock.restore() + clock = sinon.useFakeTimers(1721399999999) + result.markTerminal() + expect(result.createdAt).to.equal(1721390000) + expect(result.isTerminal).to.equal(true) + expect(result.accessedAt).to.equal(1721399999) + }) + it('set accessedAt to now when updateAccessedAt is called', () => { + const result = new ChargeState(1721390000, 1721390000, false) + expect(result.isTerminal).to.equal(false) + expect(result.accessedAt).to.equal(1721390000) + clock.restore() + clock = sinon.useFakeTimers(1721399999999) + result.updateAccessedAt() + expect(result.isTerminal).to.equal(false) + expect(result.accessedAt).to.equal(1721399999) + }) + it('return string representation when toString is called', () => { + const result = new ChargeState().toString() + expect(result).to.equal('1721390000,1721390000,F') + }) + it('return ChargeState representation from string', () => { + const result = chargeStateFromString('1721390000,1721399999,F') + expect(result.isTerminal).to.equal(false) + expect(result.accessedAt).to.equal(1721399999) + expect(result.createdAt).to.equal(1721390000) + }) + const invalidArgs = [ + { input: null }, + { input: undefined }, + { input: '' }, + { input: 'randomstringhere' }, + { input: 'too,many,arg,umen,ts' }, + { input: 'toofew,arguments' } + ] + invalidArgs.forEach(({ input }) => { + it(`return null if chargeStateFromString argument is not valid [${input}]`, () => { + const result = chargeStateFromString(input) + expect(result).to.be.null // eslint-disable-line + }) + }) +}) diff --git a/test/test-helpers/session.js b/test/test-helpers/session.js index db0fde7e3..67e29e48d 100644 --- a/test/test-helpers/session.js +++ b/test/test-helpers/session.js @@ -2,6 +2,7 @@ const clientSessions = require('client-sessions') const cookies = require('../../app/utils/cookies.js') +const { ChargeState } = require('../../app/models/ChargeState') function createSessionChargeKey (chargeId) { return 'ch_' + chargeId @@ -12,10 +13,11 @@ function createReturnUrlKey (chargeId) { } function createSessionWithReturnUrl (chargeId, chargeSession, returnUrl) { - chargeSession = chargeSession || {} + 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 session[createSessionChargeKey(chargeId)] = chargeSession session[createReturnUrlKey(chargeId)] = encodeURIComponent(returnUrl) } diff --git a/test/test-helpers/test-helpers.js b/test/test-helpers/test-helpers.js index f8a845285..5e3a9d623 100644 --- a/test/test-helpers/test-helpers.js +++ b/test/test-helpers/test-helpers.js @@ -336,8 +336,8 @@ module.exports = { throw new Error('Promise was unexpectedly fulfilled.') }, - csrfToken: function () { - return csrf().create(process.env.CSRF_USER_SECRET) + csrfToken: function (secret = process.env.CSRF_USER_SECRET) { + return csrf().create(secret) }, cardTypes diff --git a/test/test.env b/test/test.env index a43c288e8..a44468199 100644 --- a/test/test.env +++ b/test/test.env @@ -4,6 +4,7 @@ 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 diff --git a/test/unit/cookies.test.js b/test/unit/cookies.test.js index a545f7c15..7346933c8 100644 --- a/test/unit/cookies.test.js +++ b/test/unit/cookies.test.js @@ -5,6 +5,7 @@ const { expect } = require('chai') const sinon = require('sinon') const proxyquire = require('proxyquire').noPreserveCache() const { createChargeIdSessionKey } = require('../../app/utils/session') +const { ChargeState, epochSecondsNow } = require('../../app/models/ChargeState') const getCookiesUtil = clientSessionsStub => { if (clientSessionsStub) return proxyquire('../../app/utils/cookies', { 'client-sessions': clientSessionsStub }) @@ -122,6 +123,130 @@ describe('cookie configuration', function () { }) }) +describe('find session charge to delete', () => { + let initialEnvironmentVariables + const now = epochSecondsNow() + const overTwentyFourHoursAgo = (now - 86400) - 120 + const ninetyFiveMinutesAgo = now - 5700 + const ninetyMinutesAgo = now - 5400 + const thirtyMinutesAgo = now - 1800 + const twentyMinutesAgo = now - 1200 + + before(() => { + initialEnvironmentVariables = Object.assign({}, process.env) + }) + + afterEach(() => { + for (const envVar in process.env) { + process.env[envVar] = initialEnvironmentVariables[envVar] + } + }) + + // it returns earliest accessed charge that is older than 24 hours for deletion + // OR + // it returns the earliest accessed completed charge + // OR + // it returns earliest accessed charge that is older than ninety minutes + // OR + // it returns the earliest accessed charge + + it('should return the earliest accessed charge that is older than 24 hours', function () { + const cookies = getCookiesUtil() + + const req = { + frontend_state: { + ch_1: { + data: new ChargeState(ninetyMinutesAgo, (now), false).toString() + }, + ch_2: { + data: new ChargeState(ninetyMinutesAgo, (now - 5), true).toString() + }, + ch_3: { + data: new ChargeState(overTwentyFourHoursAgo, (now - 10), false).toString() + }, + ch_4: { + data: new ChargeState(overTwentyFourHoursAgo, (now - 20), false).toString() + } + } + } + + const result = cookies.findSessionChargeToDelete(req) + expect(result).to.equal('ch_4') + }) + + it('should return the earliest accessed completed charge', function () { + const cookies = getCookiesUtil() + + const req = { + frontend_state: { + ch_1: { + data: new ChargeState(thirtyMinutesAgo, (now), true).toString() + }, + ch_2: { + data: new ChargeState(ninetyFiveMinutesAgo, (now - 5), true).toString() + }, + ch_3: { + data: new ChargeState(thirtyMinutesAgo, (now - 30), true).toString() + }, + ch_4: { + data: new ChargeState(thirtyMinutesAgo, (now - 40), false).toString() + } + } + } + + const result = cookies.findSessionChargeToDelete(req) + expect(result).to.equal('ch_3') + }) + + it('should return the earliest accessed charge that is older than ninety minutes', function () { + const cookies = getCookiesUtil() + + const req = { + frontend_state: { + ch_1: { + data: new ChargeState(thirtyMinutesAgo, (now - 50), false).toString() + }, + ch_2: { + data: new ChargeState(ninetyFiveMinutesAgo, (now - 45), false).toString() + }, + ch_3: { + data: new ChargeState(ninetyFiveMinutesAgo, (now - 30), false).toString() + }, + ch_4: { + data: new ChargeState(ninetyFiveMinutesAgo, (now - 40), false).toString() + } + } + } + + const result = cookies.findSessionChargeToDelete(req) + expect(result).to.equal('ch_2') + }) + + it('should return the earliest accessed charge', function () { + const cookies = getCookiesUtil() + + const req = { + frontend_state: { + ch_1: { + data: new ChargeState(twentyMinutesAgo, (now - 50), false).toString() + }, + ch_2: { + data: new ChargeState(twentyMinutesAgo, (now - 45), false).toString() + }, + ch_3: { + data: new ChargeState(twentyMinutesAgo, (now - 30), false).toString() + }, + ch_4: { + data: new ChargeState(twentyMinutesAgo, (now - 40), false).toString() + } + } + } + + const result = cookies.findSessionChargeToDelete(req) + expect(result).to.equal('ch_1') + }) +}) + describe('setting value on session', function () { let initialEnvironmentVariables before(() => { @@ -251,7 +376,33 @@ describe('getting value from session', function () { delete process.env.SESSION_ENCRYPTION_KEY_2 }) + + it('should return charge keys for session where data object exists', () => { + const cookies = getCookiesUtil() + const req = { + frontend_state: { + ch_1: { + data: 'blah' + }, + ch_2: { + data: 'blah' + }, + ch_3: { + data: 'blah' + }, + ch_4: { + data: 'blah' + }, + ch_5: { + notDataObj: 'blah' + } + } + } + expect(cookies.getChargesOnSession(req)).to.deep.equal(['ch_1', 'ch_2', 'ch_3', 'ch_4']) + expect(cookies.getChargesOnSession(req).length).to.equal(4) + }) }) + describe('removing value from session', function () { it('should remove value from frontend_state', function () { process.env.SESSION_ENCRYPTION_KEY = 'key1key1key1key1' diff --git a/test/unit/session.test.js b/test/unit/session.test.js index 899e107ad..d802c50bd 100644 --- a/test/unit/session.test.js +++ b/test/unit/session.test.js @@ -2,8 +2,10 @@ const { expect } = require('chai') const session = require('../../app/utils/session') +const { ChargeState } = require('../../app/models/ChargeState') const chargeId = 'foo' +const chargeStateString = new ChargeState().toString() const EMPTY_RESPONSE = { params: {}, body: {}, get: () => null } @@ -25,7 +27,7 @@ const VALID_GET_RESPONSE = { params: { chargeId: chargeId }, body: {}, method: 'GET', - frontend_state: { ch_foo: true }, + frontend_state: { ch_foo: { data: chargeStateString } }, get: () => null } @@ -33,7 +35,7 @@ const VALID_POST_RESPONSE = { params: {}, body: { chargeId: chargeId }, method: 'POST', - frontend_state: { ch_foo: true }, + frontend_state: { ch_foo: { data: chargeStateString } }, get: () => null } @@ -46,29 +48,29 @@ describe('session utils ', () => { describe('retrieve ', () => { it('should return session', function () { - expect(session.retrieve(VALID_GET_RESPONSE, chargeId)).to.be.true // eslint-disable-line + expect(session.retrieve(VALID_GET_RESPONSE, chargeId)).to.deep.equal({ data: chargeStateString }) }) }) describe('validateSessionCookie ', () => { it('should return true for GET request with valid session cookie', function () { - expect(session.validateSessionCookie(VALID_GET_RESPONSE)).to.be.true // eslint-disable-line + expect(session.validateSessionCookie(VALID_GET_RESPONSE)).to.be.true // eslint-disable-line }) it('should return true for POST request with valid session cookie', function () { - expect(session.validateSessionCookie(VALID_POST_RESPONSE)).to.be.true // eslint-disable-line + expect(session.validateSessionCookie(VALID_POST_RESPONSE)).to.be.true // eslint-disable-line }) it('should return false if the charge param is not present in params or body', function () { - expect(session.validateSessionCookie(EMPTY_RESPONSE)).to.be.false // eslint-disable-line + expect(session.validateSessionCookie(EMPTY_RESPONSE)).to.be.false // eslint-disable-line }) it('should return false if the charge param is in params but not in session', function () { - expect(session.validateSessionCookie(NO_SESSION_GET_RESPONSE)).to.be.false // eslint-disable-line + expect(session.validateSessionCookie(NO_SESSION_GET_RESPONSE)).to.be.false // eslint-disable-line }) it('should return false if the charge param is in THE BODY but not in session', function () { - expect(session.validateSessionCookie(NO_SESSION_POST_RESPONSE)).to.be.false // eslint-disable-line + expect(session.validateSessionCookie(NO_SESSION_POST_RESPONSE)).to.be.false // eslint-disable-line }) }) })