From 408366faf996daae4f26b2b70892040d4e2c9671 Mon Sep 17 00:00:00 2001 From: David Stone Date: Mon, 3 Nov 2025 18:47:27 +0000 Subject: [PATCH 1/6] Remove old style duplicate error logging --- src/server/plugins/errorPages.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/server/plugins/errorPages.ts b/src/server/plugins/errorPages.ts index 830bf4dbd..f5c960f57 100644 --- a/src/server/plugins/errorPages.ts +++ b/src/server/plugins/errorPages.ts @@ -39,14 +39,6 @@ export default { return h.view('404', viewModel).code(statusCode) } - request.log('error', { - statusCode, - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - data: response.data, - message: response.message, - stack: response.stack - }) - const error = new Error( `HTTP ${statusCode} error: ${response.message}` ) From 3ad00c6c2b0c0e6aaa30112e310205df8b1f611d Mon Sep 17 00:00:00 2001 From: David Stone Date: Tue, 4 Nov 2025 12:45:53 +0000 Subject: [PATCH 2/6] Remove duplicate crumb from context --- src/server/plugins/nunjucks/context.js | 4 +-- src/server/plugins/nunjucks/context.test.js | 30 --------------------- 2 files changed, 1 insertion(+), 33 deletions(-) diff --git a/src/server/plugins/nunjucks/context.js b/src/server/plugins/nunjucks/context.js index 629ec8e04..c748c6bc6 100644 --- a/src/server/plugins/nunjucks/context.js +++ b/src/server/plugins/nunjucks/context.js @@ -3,8 +3,7 @@ import { basename, join } from 'node:path' import { checkFormStatus, - encodeUrl, - safeGenerateCrumb + encodeUrl } from '@defra/forms-engine-plugin/engine/helpers.js' import Boom from '@hapi/boom' import { StatusCodes } from 'http-status-codes' @@ -59,7 +58,6 @@ export function context(request) { serviceName: config.get('serviceName'), serviceVersion: config.get('serviceVersion') }, - crumb: safeGenerateCrumb(request), cspNonce: request?.plugins.blankie?.nonces?.script, currentPath: request ? `${request.path}${request.url.search}` : undefined, previewMode: isPreviewMode ? formState : undefined, diff --git a/src/server/plugins/nunjucks/context.test.js b/src/server/plugins/nunjucks/context.test.js index 46e7c9211..bcd8870ce 100644 --- a/src/server/plugins/nunjucks/context.test.js +++ b/src/server/plugins/nunjucks/context.test.js @@ -105,36 +105,6 @@ describe('Nunjucks context', () => { malformedRequest.server.plugins.crumb.generate ).not.toHaveBeenCalled() }) - - it('should generate crumb when state exists', () => { - const mockCrumb = 'generated-crumb-value' - const validRequest = /** @type {FormRequest} */ ( - /** @type {unknown} */ ({ - server: { - plugins: { - crumb: { - generate: jest.fn().mockReturnValue(mockCrumb) - } - } - }, - plugins: {}, - route: { - settings: { - plugins: {} - } - }, - path: '/test', - url: { search: '' }, - state: {} - }) - ) - - const { crumb } = context(validRequest) - expect(crumb).toBe(mockCrumb) - expect(validRequest.server.plugins.crumb.generate).toHaveBeenCalledWith( - validRequest - ) - }) }) }) From 2ad3b3f2c692dff8f45f28e9391be364c001413c Mon Sep 17 00:00:00 2001 From: David Stone Date: Tue, 4 Nov 2025 13:08:21 +0000 Subject: [PATCH 3/6] Re-send hapi logs to pino so we don't miss internal or plugin logs --- src/server/index.ts | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/server/index.ts b/src/server/index.ts index b4006c0cb..b71f6e9e2 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -10,7 +10,7 @@ import { type FormResponseToolkit, type PluginOptions } from '@defra/forms-engine-plugin/types' -import { type FormDefinition } from '@defra/forms-model' +import { getErrorMessage, type FormDefinition } from '@defra/forms-model' import { Engine as CatboxMemory } from '@hapi/catbox-memory' import { Engine as CatboxRedis } from '@hapi/catbox-redis' import hapi, { @@ -218,6 +218,24 @@ export async function createServer(routeConfig?: RouteConfig) { return h.continue }) + server.events.on('log', (event, tags) => { + if ('error' in tags) { + server.logger.error(event.error, getErrorMessage(event.error)) + return + } + + server.logger.info(event.data) + }) + + server.events.on('request', (request, event, tags) => { + if ('error' in tags) { + request.logger.error(event.error, getErrorMessage(event.error)) + return + } + + request.logger.info(event.data) + }) + await server.register(pluginViews) await server.register(...pluginEngine) await server.register(pluginRouter) From 7e0da02f1ca63669c40163f37c1ea550ff175010 Mon Sep 17 00:00:00 2001 From: David Stone Date: Fri, 7 Nov 2025 17:48:39 +0000 Subject: [PATCH 4/6] Add forward logs plugin --- .../helpers/logging/forward-logs.test.ts | 104 ++++++++++++++++++ .../common/helpers/logging/forward-logs.ts | 45 ++++++++ src/server/index.ts | 22 +--- 3 files changed, 152 insertions(+), 19 deletions(-) create mode 100644 src/server/common/helpers/logging/forward-logs.test.ts create mode 100644 src/server/common/helpers/logging/forward-logs.ts diff --git a/src/server/common/helpers/logging/forward-logs.test.ts b/src/server/common/helpers/logging/forward-logs.test.ts new file mode 100644 index 000000000..9490b835e --- /dev/null +++ b/src/server/common/helpers/logging/forward-logs.test.ts @@ -0,0 +1,104 @@ +import { type RequestEvent } from '@hapi/hapi' +import type pino from 'pino' + +import { forwardLogs } from '~/src/server/common/helpers/logging/forward-logs.js' + +describe('forwardLogs', () => { + let logger: pino.Logger + + beforeEach(() => { + const info = jest.fn() + const error = jest.fn() + + logger = { error, info } as unknown as pino.Logger + }) + + it('logs info with string data', () => { + forwardLogs( + logger, + { + channel: 'app', + timestamp: Date.now().toString(), + tags: ['a', 'b', 'c'], + data: 'My log msg' + } as RequestEvent, + { a: true, b: true, c: true } + ) + + expect(logger.info).toHaveBeenCalledExactlyOnceWith( + 'Channel: app, Tags: [a,b,c], Data: My log msg' + ) + }) + + it('logs info with undefined data', () => { + forwardLogs( + logger, + { + channel: 'app', + timestamp: Date.now().toString(), + tags: ['a', 'b', 'c'] + } as RequestEvent, + { a: true, b: true, c: true } + ) + + expect(logger.info).toHaveBeenCalledExactlyOnceWith( + 'Channel: app, Tags: [a,b,c], Data: type - undefined' + ) + }) + + it('logs info with object data', () => { + forwardLogs( + logger, + { + channel: 'app', + timestamp: Date.now().toString(), + tags: ['a', 'b', 'c'], + data: { some: 'data' } + } as RequestEvent, + { a: true, b: true, c: true } + ) + + expect(logger.info).toHaveBeenCalledExactlyOnceWith( + 'Channel: app, Tags: [a,b,c], Data: type - object' + ) + }) + + it('logs info with function data', () => { + forwardLogs( + logger, + { + channel: 'app', + timestamp: Date.now().toString(), + tags: ['a', 'b', 'c'], + data: () => { + '' + } + } as RequestEvent, + { a: true, b: true, c: true } + ) + + expect(logger.info).toHaveBeenCalledExactlyOnceWith( + 'Channel: app, Tags: [a,b,c], Data: type - function' + ) + }) + + it('logs error with string data', () => { + const error = new Error('Some error') + + forwardLogs( + logger, + { + channel: 'internal', + timestamp: Date.now().toString(), + tags: ['a', 'b', 'c', 'error'], + error + } as RequestEvent, + { a: true, b: true, c: true, error: true } + ) + + expect(logger.error).toHaveBeenCalledWith( + error, + 'Channel: internal, Tags: [a,b,c,error], Error: Some error' + ) + }) +}) diff --git a/src/server/common/helpers/logging/forward-logs.ts b/src/server/common/helpers/logging/forward-logs.ts new file mode 100644 index 000000000..a44379f1e --- /dev/null +++ b/src/server/common/helpers/logging/forward-logs.ts @@ -0,0 +1,45 @@ +import { getErrorMessage } from '@defra/forms-model' +import { + type LogEvent, + type RequestEvent, + type ServerRegisterPluginObject +} from '@hapi/hapi' +import type pino from 'pino' + +export const forwardLogs = ( + logger: pino.Logger, + event: LogEvent | RequestEvent, + tags: Record +) => { + const tagstr = event.tags.join(',') + const message = `Channel: ${event.channel}, Tags: [${tagstr}]` + + if ('error' in tags) { + logger.error( + event.error, + `${message}, Error: ${getErrorMessage(event.error)}` + ) + } else { + const data = + typeof event.data === 'string' + ? event.data + : `type - ${typeof event.data}` + + logger.info(`${message}, Data: ${data}`) + } +} + +export default { + plugin: { + name: 'forward-logs', + register: (server) => { + server.events.on('log', (event, tags) => + forwardLogs(server.logger, event, tags) + ) + + server.events.on('request', (request, event, tags) => + forwardLogs(request.logger, event, tags) + ) + } + } +} satisfies ServerRegisterPluginObject diff --git a/src/server/index.ts b/src/server/index.ts index b71f6e9e2..03fe068fa 100644 --- a/src/server/index.ts +++ b/src/server/index.ts @@ -10,7 +10,7 @@ import { type FormResponseToolkit, type PluginOptions } from '@defra/forms-engine-plugin/types' -import { getErrorMessage, type FormDefinition } from '@defra/forms-model' +import { type FormDefinition } from '@defra/forms-model' import { Engine as CatboxMemory } from '@hapi/catbox-memory' import { Engine as CatboxRedis } from '@hapi/catbox-redis' import hapi, { @@ -25,6 +25,7 @@ import blipp from 'blipp' import { ProxyAgent } from 'proxy-agent' import { config } from '~/src/config/index.js' +import forwardLogs from '~/src/server/common/helpers/logging/forward-logs.js' import { requestLogger } from '~/src/server/common/helpers/logging/request-logger.js' import { requestTracing } from '~/src/server/common/helpers/logging/request-tracing.js' import { buildRedisClient } from '~/src/server/common/helpers/redis-client.js' @@ -180,6 +181,7 @@ export async function createServer(routeConfig?: RouteConfig) { const server = hapi.server(serverOptions()) await server.register(requestLogger) + await server.register(forwardLogs) if (config.get('isProduction')) { prepareSecureContext(server) @@ -218,24 +220,6 @@ export async function createServer(routeConfig?: RouteConfig) { return h.continue }) - server.events.on('log', (event, tags) => { - if ('error' in tags) { - server.logger.error(event.error, getErrorMessage(event.error)) - return - } - - server.logger.info(event.data) - }) - - server.events.on('request', (request, event, tags) => { - if ('error' in tags) { - request.logger.error(event.error, getErrorMessage(event.error)) - return - } - - request.logger.info(event.data) - }) - await server.register(pluginViews) await server.register(...pluginEngine) await server.register(pluginRouter) From 0b2d5b7b6e28de3ee8e4615603177a0bb7ccffba Mon Sep 17 00:00:00 2001 From: David Stone Date: Mon, 10 Nov 2025 11:36:02 +0000 Subject: [PATCH 5/6] Fix tests that rely on enforceCsrf being false --- .../routes/save-and-exit-with-cache.test.js | 4 +++- src/server/routes/save-and-exit.test.js | 4 +++- test/form/cookies.test.js | 24 ++++++++++++------- test/form/save-and-exit.test.js | 3 ++- 4 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/server/routes/save-and-exit-with-cache.test.js b/src/server/routes/save-and-exit-with-cache.test.js index f45054392..d92f38bdb 100644 --- a/src/server/routes/save-and-exit-with-cache.test.js +++ b/src/server/routes/save-and-exit-with-cache.test.js @@ -17,7 +17,9 @@ describe('Save-and-exit check routes', () => { let server beforeAll(async () => { - server = await createServer() + server = await createServer({ + enforceCsrf: false + }) await server.initialize() }) diff --git a/src/server/routes/save-and-exit.test.js b/src/server/routes/save-and-exit.test.js index a45aa9ba1..6ffb54541 100644 --- a/src/server/routes/save-and-exit.test.js +++ b/src/server/routes/save-and-exit.test.js @@ -18,7 +18,9 @@ describe('Save-and-exit check routes', () => { let server beforeAll(async () => { - server = await createServer() + server = await createServer({ + enforceCsrf: false + }) await server.initialize() }) diff --git a/test/form/cookies.test.js b/test/form/cookies.test.js index f6b3e2301..eab5d5700 100644 --- a/test/form/cookies.test.js +++ b/test/form/cookies.test.js @@ -30,7 +30,8 @@ describe(`Cookie banner and analytics`, () => { ])('shows the cookie banner by default', async (path) => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() @@ -59,7 +60,8 @@ describe(`Cookie banner and analytics`, () => { ])('confirms when the user has accepted analytics cookies', async (path) => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() @@ -111,7 +113,8 @@ describe(`Cookie banner and analytics`, () => { ])('confirms when the user has rejected analytics cookies', async (path) => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() @@ -163,7 +166,8 @@ describe(`Cookie banner and analytics`, () => { ])('hides the cookie banner once dismissed', async (path) => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() @@ -220,7 +224,8 @@ describe(`Cookie preferences`, () => { async ({ text, value }) => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() @@ -266,7 +271,8 @@ describe(`Cookie preferences`, () => { test("doesn't show the success banner if the user hasn't been posted from the cookie preferences page", async () => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() @@ -308,7 +314,8 @@ describe(`Cookie preferences`, () => { test('defaults to no if one is not provided', async () => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() @@ -327,7 +334,8 @@ describe(`Cookie preferences`, () => { test('returns bad request for invalid redirect urls', async () => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() diff --git a/test/form/save-and-exit.test.js b/test/form/save-and-exit.test.js index 0e3cd73f2..360faea7d 100644 --- a/test/form/save-and-exit.test.js +++ b/test/form/save-and-exit.test.js @@ -20,7 +20,8 @@ describe('Save and exit', () => { beforeAll(async () => { server = await createServer({ formFileName: 'basic.js', - formFilePath: join(import.meta.dirname, 'definitions') + formFilePath: join(import.meta.dirname, 'definitions'), + enforceCsrf: false }) await server.initialize() From 46260713c9185882ce6ea832d989e32b87694952 Mon Sep 17 00:00:00 2001 From: David Stone Date: Mon, 10 Nov 2025 11:36:29 +0000 Subject: [PATCH 6/6] Default enforceCsrf: true for all environments --- src/config/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/config/index.ts b/src/config/index.ts index 2fb2ae18b..40717c91f 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -55,7 +55,7 @@ export const config = convict({ }, enforceCsrf: { format: Boolean, - default: isProduction, + default: true, env: 'ENFORCE_CSRF' },