From a22fa8f89a091d8241247b1e8d7ac85815f48ba3 Mon Sep 17 00:00:00 2001 From: Harley Harris Date: Tue, 9 Jul 2024 14:38:35 +0100 Subject: [PATCH 1/5] add express-validator dependency --- package-lock.json | 26 ++++++++++++++++++++++++++ package.json | 1 + 2 files changed, 27 insertions(+) diff --git a/package-lock.json b/package-lock.json index 73af7d4..9391d4d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,6 +15,7 @@ "crypto": "^1.0.1", "express": "^4.18.2", "express-rate-limit": "^7.3.1", + "express-validator": "^7.1.0", "govuk-frontend": "^4.8.0", "helmet": "^7.0.0", "nunjucks": "^3.2.4" @@ -3131,6 +3132,18 @@ "express": "4 || 5 || ^5.0.0-beta.1" } }, + "node_modules/express-validator": { + "version": "7.1.0", + "resolved": "https://registry.npmjs.org/express-validator/-/express-validator-7.1.0.tgz", + "integrity": "sha512-ePn6NXjHRZiZkwTiU1Rl2hy6aUqmi6Cb4/s8sfUsKH7j2yYl9azSpl8xEHcOj1grzzQ+UBEoLWtE1s6FDxW++g==", + "dependencies": { + "lodash": "^4.17.21", + "validator": "~13.12.0" + }, + "engines": { + "node": ">= 8.0.0" + } + }, "node_modules/express/node_modules/debug": { "version": "2.6.9", "resolved": "https://registry.npmjs.org/debug/-/debug-2.6.9.tgz", @@ -4577,6 +4590,11 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/lodash": { + "version": "4.17.21", + "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", + "integrity": "sha512-v2kDEe57lecTulaDIuNTPy3Ry4gLGJ6Z1O3vE1krgXZNrsQ+LFTGHVxVjcXPs17LhbZVGedAJv8XZ1tvj5FvSg==" + }, "node_modules/lodash.memoize": { "version": "4.1.2", "resolved": "https://registry.npmjs.org/lodash.memoize/-/lodash.memoize-4.1.2.tgz", @@ -6201,6 +6219,14 @@ "integrity": "sha512-ASFBup0Mz1uyiIjANan1jzLQami9z1PoYSZCiiYW2FczPbenXc45FZdBZLzOT+r6+iciuEModtmCti+hjaAk0A==", "dev": true }, + "node_modules/validator": { + "version": "13.12.0", + "resolved": "https://registry.npmjs.org/validator/-/validator-13.12.0.tgz", + "integrity": "sha512-c1Q0mCiPlgdTVVVIJIrBuxNicYE+t/7oKeI9MWLj3fh/uq2Pxh/3eeWbVZ4OcGW1TUf53At0njHw5SMdA3tmMg==", + "engines": { + "node": ">= 0.10" + } + }, "node_modules/vary": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/vary/-/vary-1.1.2.tgz", diff --git a/package.json b/package.json index 8d7d6a2..39c3872 100644 --- a/package.json +++ b/package.json @@ -45,6 +45,7 @@ "crypto": "^1.0.1", "express": "^4.18.2", "express-rate-limit": "^7.3.1", + "express-validator": "^7.1.0", "govuk-frontend": "^4.8.0", "helmet": "^7.0.0", "nunjucks": "^3.2.4" From c2be4ea6be7c278f61aa8c9d33e2cb3009c066aa Mon Sep 17 00:00:00 2001 From: Harley Harris Date: Tue, 9 Jul 2024 14:53:30 +0100 Subject: [PATCH 2/5] fix spelling in error.controller file and test --- src/controller/error.controller.ts | 2 +- test/unit/controller/error.controller.spec.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/controller/error.controller.ts b/src/controller/error.controller.ts index 7b649a2..d387f7f 100644 --- a/src/controller/error.controller.ts +++ b/src/controller/error.controller.ts @@ -9,7 +9,7 @@ export const errorNotFound = (_req: Request, res: Response) => { export const errorHandler = (err: Error, _req: Request, res: Response, _next: NextFunction) => { const statusCode = !res.statusCode || res.statusCode === 200 ? 500 : res.statusCode; - const errorMessage = err.message || 'An error has occured. Re-routing to the error screen'; + const errorMessage = err.message || 'An error has occurred. Re-routing to the error screen'; log.error(`Error ${statusCode}: ${errorMessage}`); res.status(statusCode).render(config.ERROR_PAGE); diff --git a/test/unit/controller/error.controller.spec.ts b/test/unit/controller/error.controller.spec.ts index 50c7f9b..7a39e6c 100644 --- a/test/unit/controller/error.controller.spec.ts +++ b/test/unit/controller/error.controller.spec.ts @@ -83,7 +83,8 @@ describe('Error controller tests', () => { test('should log alternate error message', () => { const res = mockResponse(); MOCK_ERROR.message = ''; - const errorLogMessage = 'Error 500: An error has occured. Re-routing to the error screen'; + const errorLogMessage = + 'Error 500: An error has occurred. Re-routing to the error screen'; errorHandler(MOCK_ERROR, req, res, next); From 1d956cb3162c3fc99cc05f374db73e7f7f6f6876 Mon Sep 17 00:00:00 2001 From: Harley Harris Date: Tue, 9 Jul 2024 15:15:21 +0100 Subject: [PATCH 3/5] add validation-test page with unit/integration tests --- src/config/index.ts | 4 +- src/controller/validation-test.controller.ts | 15 +++ src/middleware/validation.middleware.ts | 40 ++++++++ src/model/validation.model.ts | 9 ++ src/routes/index.ts | 2 + src/routes/validation-test.ts | 19 ++++ src/validation/error.messages.ts | 4 + src/validation/validation-test.validation.ts | 10 ++ src/views/error-list.html | 9 ++ src/views/include/error-list.html | 9 ++ src/views/layout.html | 10 +- src/views/validation-test.html | 36 +++++++ .../routes/validation-test.spec.ts | 74 ++++++++++++++ test/mock/data.ts | 3 + test/mock/log.mock.ts | 4 + test/mock/session.mock.ts | 1 + test/mock/text.mock.ts | 3 + .../validation-test.controller.spec.ts | 59 +++++++++++ .../middleware/validation.middleware.spec.ts | 98 +++++++++++++++++++ 19 files changed, 407 insertions(+), 2 deletions(-) create mode 100644 src/controller/validation-test.controller.ts create mode 100644 src/middleware/validation.middleware.ts create mode 100644 src/model/validation.model.ts create mode 100644 src/routes/validation-test.ts create mode 100644 src/validation/error.messages.ts create mode 100644 src/validation/validation-test.validation.ts create mode 100644 src/views/error-list.html create mode 100644 src/views/include/error-list.html create mode 100644 src/views/validation-test.html create mode 100644 test/integration/routes/validation-test.spec.ts create mode 100644 test/mock/log.mock.ts create mode 100644 test/mock/session.mock.ts create mode 100644 test/unit/controller/validation-test.controller.spec.ts create mode 100644 test/unit/middleware/validation.middleware.spec.ts diff --git a/src/config/index.ts b/src/config/index.ts index 03d13c7..0f6c743 100644 --- a/src/config/index.ts +++ b/src/config/index.ts @@ -14,10 +14,12 @@ export const SERVICE_NAME = 'Node Prototype'; export const LANDING_PAGE = 'info'; export const NOT_FOUND = 'page-not-found'; export const ERROR_PAGE = 'error'; +export const VALIDATION_TEST = 'validation-test'; // Routing paths +export const ROOT_URL = '/'; export const LANDING_URL = '/info'; - export const INFO_URL = '/info'; +export const VALIDATION_TEST_URL = '/validation-test'; export const HEALTHCHECK_URL = '/healthcheck'; export const SERVICE_URL = `${BASE_URL}${LANDING_URL}`; diff --git a/src/controller/validation-test.controller.ts b/src/controller/validation-test.controller.ts new file mode 100644 index 0000000..d2e3dff --- /dev/null +++ b/src/controller/validation-test.controller.ts @@ -0,0 +1,15 @@ +import { Request, Response } from 'express'; +import { log } from '../utils/logger'; +import * as config from '../config'; + +export const get = (_req: Request, res: Response) => { + return res.render(config.VALIDATION_TEST); +}; + +export const post = (req: Request, res: Response) => { + const firstName = req.body.first_name; + + log.info(`First Name: ${firstName}`); + + return res.redirect(config.LANDING_PAGE); +}; diff --git a/src/middleware/validation.middleware.ts b/src/middleware/validation.middleware.ts new file mode 100644 index 0000000..4167025 --- /dev/null +++ b/src/middleware/validation.middleware.ts @@ -0,0 +1,40 @@ +import { NextFunction, Request, Response } from 'express'; +import { validationResult, FieldValidationError } from 'express-validator'; + +import { log } from '../utils/logger'; +import { FormattedValidationErrors } from '../model/validation.model'; + +export const checkValidations = ( + req: Request, + res: Response, + next: NextFunction +) => { + try { + const errorList = validationResult(req); + + if (!errorList.isEmpty()) { + const template_path = req.path.substring(1); + const errors = formatValidationError( + errorList.array() as FieldValidationError[] + ); + + log.info(`Validation error on ${template_path} page`); + + return res.render(template_path, { ...req.body, errors }); + } + + return next(); + } catch (err: any) { + log.error(err.message); + next(err); + } +}; + +const formatValidationError = (errorList: FieldValidationError[]) => { + const errors = { errorList: [] } as FormattedValidationErrors; + errorList.forEach((e: FieldValidationError) => { + errors.errorList.push({ href: `#${e.path}`, text: e.msg }); + errors[e.path] = { text: e.msg }; + }); + return errors; +}; diff --git a/src/model/validation.model.ts b/src/model/validation.model.ts new file mode 100644 index 0000000..90c5bc1 --- /dev/null +++ b/src/model/validation.model.ts @@ -0,0 +1,9 @@ +import { ErrorMessages } from '../validation/error.messages'; + +export interface FormattedValidationErrors { + [key: string]: any; + errorList: { + href: string; + text: ErrorMessages; + }[]; +} diff --git a/src/routes/index.ts b/src/routes/index.ts index 01e515a..54fadf5 100644 --- a/src/routes/index.ts +++ b/src/routes/index.ts @@ -3,11 +3,13 @@ import { Router } from 'express'; import { logger } from '../middleware/logger.middleware'; import healthcheckRouter from './healthcheck'; import infoRouter from './info'; +import validationTestRouter from './validation-test'; const router = Router(); router.use(logger); router.use(healthcheckRouter); router.use(infoRouter); +router.use(validationTestRouter); export default router; diff --git a/src/routes/validation-test.ts b/src/routes/validation-test.ts new file mode 100644 index 0000000..c2c33f4 --- /dev/null +++ b/src/routes/validation-test.ts @@ -0,0 +1,19 @@ +import { Router } from 'express'; + +import { checkValidations } from '../middleware/validation.middleware'; +import { validationTest } from '../validation/validation-test.validation'; +import { get, post } from '../controller/validation-test.controller'; + +import * as config from '../config'; + +const validationTestRouter = Router(); + +validationTestRouter.get(config.VALIDATION_TEST_URL, get); +validationTestRouter.post( + config.VALIDATION_TEST_URL, + ...validationTest, + checkValidations, + post +); + +export default validationTestRouter; diff --git a/src/validation/error.messages.ts b/src/validation/error.messages.ts new file mode 100644 index 0000000..af363bb --- /dev/null +++ b/src/validation/error.messages.ts @@ -0,0 +1,4 @@ +export enum ErrorMessages { + TEST_INFO_ERROR = 'INFO PAGE ERROR MESSAGE TEST', + TEST_FIRST_NAME = 'Enter your first name', +} diff --git a/src/validation/validation-test.validation.ts b/src/validation/validation-test.validation.ts new file mode 100644 index 0000000..236c03e --- /dev/null +++ b/src/validation/validation-test.validation.ts @@ -0,0 +1,10 @@ +import { body } from 'express-validator'; + +import { ErrorMessages } from './error.messages'; + +export const validationTest = [ + body('first_name') + .not() + .isEmpty({ ignore_whitespace: true }) + .withMessage(ErrorMessages.TEST_FIRST_NAME), +]; diff --git a/src/views/error-list.html b/src/views/error-list.html new file mode 100644 index 0000000..16c525c --- /dev/null +++ b/src/views/error-list.html @@ -0,0 +1,9 @@ +{% if errors and errors.errorList and errors.errorList.length > 0 %} + {{ govukErrorSummary({ + titleText: "There is a problem", + errorList: errors.errorList if errors, + attributes: { + "tabindex": "0" + } + }) }} +{% endif %} diff --git a/src/views/include/error-list.html b/src/views/include/error-list.html new file mode 100644 index 0000000..16c525c --- /dev/null +++ b/src/views/include/error-list.html @@ -0,0 +1,9 @@ +{% if errors and errors.errorList and errors.errorList.length > 0 %} + {{ govukErrorSummary({ + titleText: "There is a problem", + errorList: errors.errorList if errors, + attributes: { + "tabindex": "0" + } + }) }} +{% endif %} diff --git a/src/views/layout.html b/src/views/layout.html index 81ec36c..178a059 100644 --- a/src/views/layout.html +++ b/src/views/layout.html @@ -2,7 +2,11 @@ {% from "govuk/components/footer/macro.njk" import govukFooter %} {% from "govuk/components/header/macro.njk" import govukHeader %} - +{% from "govuk/components/error-summary/macro.njk" import govukErrorSummary %} +{% from "govuk/components/back-link/macro.njk" import govukBackLink %} +{% from "govuk/components/input/macro.njk" import govukInput %} +{% from "govuk/components/textarea/macro.njk" import govukTextarea %} +{% from "govuk/components/button/macro.njk" import govukButton %} {% block head %} @@ -22,6 +26,10 @@ }) }} {% endblock %} +{% block content %} +{% endblock %} + + {% block footer %} {{ govukFooter({ meta: { diff --git a/src/views/validation-test.html b/src/views/validation-test.html new file mode 100644 index 0000000..2534367 --- /dev/null +++ b/src/views/validation-test.html @@ -0,0 +1,36 @@ +{% extends "layout.html" %} + +{% block content %} +
+
+

Validation Test

+ +

Submit field empty to test validation functionality.

+ + {% include "include/error-list.html" %} + +
+ + {{ govukInput({ + errorMessage: errors.first_name if errors, + label: { + text: "First Name", + classes: "govuk-label--m" + }, + classes: "govuk-input--width-10", + id: "first_name", + name: "first_name", + value: first_name + }) }} + + {{ govukButton({ + text: "Submit", + attributes: { + "id": "submit" + } + }) }} + +
+
+
+{% endblock %} \ No newline at end of file diff --git a/test/integration/routes/validation-test.spec.ts b/test/integration/routes/validation-test.spec.ts new file mode 100644 index 0000000..c854e26 --- /dev/null +++ b/test/integration/routes/validation-test.spec.ts @@ -0,0 +1,74 @@ +jest.mock('../../../src/middleware/logger.middleware'); +jest.mock('../../../src/utils/logger'); + +import { jest, beforeEach, describe, expect, test } from '@jest/globals'; +import { Request, Response, NextFunction } from 'express'; +import request from 'supertest'; + +import app from '../../../src/app'; +import * as config from '../../../src/config'; +import { logger } from '../../../src/middleware/logger.middleware'; +import { log } from '../../../src/utils/logger'; + +import { + MOCK_REDIRECT_MESSAGE, + MOCK_GET_VALIDATION_TEST_RESPONSE, + MOCK_POST_VALIDATION_TEST_RESPONSE, +} from '../../mock/text.mock'; +import { MOCK_POST_VALIDATION_TEST } from '../../mock/data'; + +const mockedLogger = logger as jest.Mock; +mockedLogger.mockImplementation( + (_req: Request, _res: Response, next: NextFunction) => next() +); + +describe('validation-test endpoint integration tests', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('GET tests', () => { + test('renders the validation-test page', async () => { + const res = await request(app).get(config.VALIDATION_TEST_URL); + + expect(res.status).toEqual(200); + expect(res.text).toContain(MOCK_GET_VALIDATION_TEST_RESPONSE); + expect(mockedLogger).toHaveBeenCalledTimes(1); + }); + }); + + describe('POST tests', () => { + test('Should redirect to landing page after POST request', async () => { + const res = await request(app) + .post(config.VALIDATION_TEST_URL) + .send(MOCK_POST_VALIDATION_TEST); + + expect(res.status).toEqual(302); + expect(res.text).toContain(MOCK_REDIRECT_MESSAGE); + expect(mockedLogger).toHaveBeenCalledTimes(1); + }); + + test('Should render the same page with error messages after POST request', async () => { + const res = await request(app) + .post(config.VALIDATION_TEST_URL) + .send({ + first_name: '', + }); + + expect(res.status).toEqual(200); + expect(res.text).toContain(MOCK_GET_VALIDATION_TEST_RESPONSE); + expect(mockedLogger).toHaveBeenCalledTimes(1); + }); + + test('Should log the First Name and More Details on POST request.', async () => { + const mockLog = log.info as jest.Mock; + const res = await request(app) + .post(config.VALIDATION_TEST_URL) + .send(MOCK_POST_VALIDATION_TEST); + + expect(mockLog).toBeCalledWith(MOCK_POST_VALIDATION_TEST_RESPONSE); + expect(res.text).toContain(MOCK_REDIRECT_MESSAGE); + expect(mockedLogger).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/test/mock/data.ts b/test/mock/data.ts index 90cf46c..fe0315b 100644 --- a/test/mock/data.ts +++ b/test/mock/data.ts @@ -4,6 +4,9 @@ import * as config from '../../src/config'; import express from 'express'; export const GET_REQUEST_MOCK = { method: 'GET', path: '/test' }; +export const MOCK_POST_VALIDATION_TEST = { first_name: 'example' }; + +export const MOCK_POST_INFO = { test: 'test' }; export const MOCK_CORS_VALUE = { origin: [config.CDN_HOST, config.BASE_URL], diff --git a/test/mock/log.mock.ts b/test/mock/log.mock.ts new file mode 100644 index 0000000..66ff3b4 --- /dev/null +++ b/test/mock/log.mock.ts @@ -0,0 +1,4 @@ +import { log } from '../../src/utils/logger'; + +export const mockLogInfo = log.info as jest.Mock; +export const mockLogErrorRequest = log.errorRequest as jest.Mock; diff --git a/test/mock/session.mock.ts b/test/mock/session.mock.ts new file mode 100644 index 0000000..6434d1d --- /dev/null +++ b/test/mock/session.mock.ts @@ -0,0 +1 @@ +export const mockID = 'c3931b00-a8b4-4d2d-a165-b9b4d148cd88'; diff --git a/test/mock/text.mock.ts b/test/mock/text.mock.ts index 4d79fd3..40b3e06 100644 --- a/test/mock/text.mock.ts +++ b/test/mock/text.mock.ts @@ -7,3 +7,6 @@ export const MOCK_SERVER_ERROR = 'Pipe 3000 requires elevated privileges'; export const MOCK_ERROR_MESSAGE = 'Something has went wrong'; export const MOCK_WRONG_URL = '/infooo'; export const MOCK_VIEWS_PATH = '/path/to/views'; +export const MOCK_REDIRECT_MESSAGE = 'Found. Redirecting to info'; +export const MOCK_GET_VALIDATION_TEST_RESPONSE = 'Submit field empty to test validation functionality.'; +export const MOCK_POST_VALIDATION_TEST_RESPONSE = 'First Name: example'; diff --git a/test/unit/controller/validation-test.controller.spec.ts b/test/unit/controller/validation-test.controller.spec.ts new file mode 100644 index 0000000..bc9be65 --- /dev/null +++ b/test/unit/controller/validation-test.controller.spec.ts @@ -0,0 +1,59 @@ +jest.mock('../../../src/utils/logger'); + +import { describe, expect, afterEach, test, jest } from '@jest/globals'; +import { Request, Response } from 'express'; + +import { get, post } from '../../../src/controller/validation-test.controller'; +import * as config from '../../../src/config'; +import { log } from '../../../src/utils/logger'; + +import { MOCK_POST_VALIDATION_TEST } from '../../mock/data'; +import { MOCK_POST_VALIDATION_TEST_RESPONSE } from '../../mock/text.mock'; + +const req = { + body: MOCK_POST_VALIDATION_TEST, +} as Request; + +const mockResponse = () => { + const res = {} as Response; + res.render = jest.fn().mockReturnValue(res) as any; + res.redirect = jest.fn().mockReturnValue(res) as any; + return res; +}; + +describe('Validation test controller test suites', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + describe('validation test GET tests', () => { + test('should render validation test page', () => { + const res = mockResponse(); + + get(req, res); + + expect(res.render).toHaveBeenCalledWith(config.VALIDATION_TEST); + }); + }); + + describe('validation-test POST tests', () => { + test('should redirect to landing-page on POST request', () => { + const res = mockResponse(); + + post(req, res); + + expect(res.redirect).toBeCalledWith(config.LANDING_PAGE); + }); + test('should log GitHub handle and More Details on POST request', () => { + const res = mockResponse(); + + const mockLogInfo = log.info as jest.Mock; + + post(req, res); + + expect(mockLogInfo).toHaveBeenCalledWith( + MOCK_POST_VALIDATION_TEST_RESPONSE + ); + }); + }); +}); diff --git a/test/unit/middleware/validation.middleware.spec.ts b/test/unit/middleware/validation.middleware.spec.ts new file mode 100644 index 0000000..88ea52e --- /dev/null +++ b/test/unit/middleware/validation.middleware.spec.ts @@ -0,0 +1,98 @@ +jest.mock('express-validator'); +jest.mock('../../../src/utils/logger'); +jest.mock('../../../src/utils/validateFilepath.ts'); + +import { + describe, + expect, + test, + jest, + afterEach, + beforeEach, +} from '@jest/globals'; +import { Request, Response, NextFunction } from 'express'; +import { validationResult } from 'express-validator'; + +import * as config from '../../../src/config'; + +import { checkValidations } from '../../../src/middleware/validation.middleware'; +import { ErrorMessages } from '../../../src/validation/error.messages'; +import { MOCK_ERROR, MOCK_POST_VALIDATION_TEST } from '../../mock/data'; +import { log } from '../../../src/utils/logger'; + +const validationResultMock = validationResult as unknown as jest.Mock; +const logInfoMock = log.info as jest.Mock; +const logErrorMock = log.error as jest.Mock; + +const mockResponse = () => { + const res = {} as Response; + res.render = jest.fn() as any; + return res; +}; +const mockRequest = () => { + const req = {} as Request; + req.path = config.VALIDATION_TEST_URL; + req.body = MOCK_POST_VALIDATION_TEST; + return req; +}; +const next = jest.fn() as NextFunction; + +describe('Validation Middleware test suites', () => { + let res: any, req: any; + + beforeEach(() => { + res = mockResponse(); + req = mockRequest(); + }); + + afterEach(() => { + jest.resetAllMocks(); + }); + + test('Should call next if errorList is empty', () => { + validationResultMock.mockImplementationOnce(() => { + return { isEmpty: () => true }; + }); + checkValidations(req, res, next); + + expect(logInfoMock).toBeCalledTimes(0); + expect(logErrorMock).toBeCalledTimes(0); + expect(next).toHaveBeenCalledTimes(1); + }); + + test(`should call res.render with ${config.VALIDATION_TEST} view if errorList is not empty`, () => { + const fieldKey = 'first_name'; + validationResultMock.mockImplementationOnce(() => { + return { + isEmpty: () => false, + array: () => [{ path: fieldKey, msg: ErrorMessages.TEST_FIRST_NAME }], + }; + }); + req.body[fieldKey] = ''; + checkValidations(req, res, next); + + expect(logInfoMock).toHaveBeenCalledTimes(1); + expect(logInfoMock).toHaveBeenCalledWith( + `Validation error on ${config.VALIDATION_TEST} page` + ); + expect(res.render).toHaveBeenCalledTimes(1); + expect(res.render).toHaveBeenCalledWith(config.VALIDATION_TEST, { + ...req.body, + errors: { + errorList: [{ href: `#${fieldKey}`, text: ErrorMessages.TEST_FIRST_NAME }], + [fieldKey]: { text: ErrorMessages.TEST_FIRST_NAME }, + }, + }); + }); + + test('should catch the error log error message and call next(err)', () => { + validationResultMock.mockImplementationOnce(() => { + throw new Error(MOCK_ERROR.message); + }); + checkValidations(req, res, next); + + expect(next).toHaveBeenCalledTimes(1); + expect(logErrorMock).toHaveBeenCalledTimes(1); + expect(logErrorMock).toHaveBeenCalledWith(MOCK_ERROR.message); + }); +}); From 8ac3cc87591578252a56c11fa9ae76b3b2b28997 Mon Sep 17 00:00:00 2001 From: Harley Harris Date: Tue, 9 Jul 2024 15:20:20 +0100 Subject: [PATCH 4/5] add validateFilepath to fix uncontrolled path vulnerability --- src/middleware/validation.middleware.ts | 4 +- src/utils/validateFilepath.ts | 17 +++++++ .../middleware/validation.middleware.spec.ts | 3 ++ test/unit/utils/validateFilepath.spec.ts | 46 +++++++++++++++++++ 4 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 src/utils/validateFilepath.ts create mode 100644 test/unit/utils/validateFilepath.spec.ts diff --git a/src/middleware/validation.middleware.ts b/src/middleware/validation.middleware.ts index 4167025..0ccbece 100644 --- a/src/middleware/validation.middleware.ts +++ b/src/middleware/validation.middleware.ts @@ -3,6 +3,7 @@ import { validationResult, FieldValidationError } from 'express-validator'; import { log } from '../utils/logger'; import { FormattedValidationErrors } from '../model/validation.model'; +import { validateFilepath } from '../utils/validateFilepath'; export const checkValidations = ( req: Request, @@ -13,7 +14,8 @@ export const checkValidations = ( const errorList = validationResult(req); if (!errorList.isEmpty()) { - const template_path = req.path.substring(1); + const validatedFilepath = validateFilepath(req.path); + const template_path = validatedFilepath.substring(1); const errors = formatValidationError( errorList.array() as FieldValidationError[] ); diff --git a/src/utils/validateFilepath.ts b/src/utils/validateFilepath.ts new file mode 100644 index 0000000..b965028 --- /dev/null +++ b/src/utils/validateFilepath.ts @@ -0,0 +1,17 @@ +import path from 'path'; +import * as config from '../config'; +import { Request } from 'express'; + +// https://codeql.github.com/codeql-query-help/javascript/js-path-injection/ + +export const validateFilepath = (reqPath: Request['path']): string => { + + // normalise req.path, then check the path starts with root url as per linked guidance + const normalisedPath = path.resolve(config.ROOT_URL, reqPath); + + if (normalisedPath.startsWith(config.ROOT_URL)) { + return normalisedPath; + } + + throw new Error('Filepath validation failed'); +}; diff --git a/test/unit/middleware/validation.middleware.spec.ts b/test/unit/middleware/validation.middleware.spec.ts index 88ea52e..4440d3c 100644 --- a/test/unit/middleware/validation.middleware.spec.ts +++ b/test/unit/middleware/validation.middleware.spec.ts @@ -19,8 +19,10 @@ import { checkValidations } from '../../../src/middleware/validation.middleware' import { ErrorMessages } from '../../../src/validation/error.messages'; import { MOCK_ERROR, MOCK_POST_VALIDATION_TEST } from '../../mock/data'; import { log } from '../../../src/utils/logger'; +import { validateFilepath } from '../../../src/utils/validateFilepath'; const validationResultMock = validationResult as unknown as jest.Mock; +const validateFilepathMock = validateFilepath as jest.Mock; const logInfoMock = log.info as jest.Mock; const logErrorMock = log.error as jest.Mock; @@ -69,6 +71,7 @@ describe('Validation Middleware test suites', () => { }; }); req.body[fieldKey] = ''; + validateFilepathMock.mockImplementationOnce(() => config.VALIDATION_TEST_URL); checkValidations(req, res, next); expect(logInfoMock).toHaveBeenCalledTimes(1); diff --git a/test/unit/utils/validateFilepath.spec.ts b/test/unit/utils/validateFilepath.spec.ts new file mode 100644 index 0000000..3c907da --- /dev/null +++ b/test/unit/utils/validateFilepath.spec.ts @@ -0,0 +1,46 @@ +import { describe, afterEach, expect, test, jest } from '@jest/globals'; + +import path from 'path'; + +import { validateFilepath } from '../../../src/utils/validateFilepath'; +import * as config from '../../../src/config/index'; + +describe('validateFilepath util tests', () => { + + afterEach(() => { + jest.resetAllMocks(); + }); + + test(`should return a valid filepath which starts with ${config.ROOT_URL}`, () => { + + const validFilepath = '/home'; + + const normalisedPath = validateFilepath(validFilepath); + + const expectedNormalisedPath = '/home'; + expect(normalisedPath).toBe(expectedNormalisedPath); + + }); + + test(`should normalise a malicious filepath and return the normalised path if it starts with ${config.ROOT_URL}`, () => { + + const maliciousPathTraveralFilepath = '../../../home'; + + const normalisedPath = validateFilepath(maliciousPathTraveralFilepath); + + const expectedNormalisedPath = '/home'; + expect(normalisedPath).toBe(expectedNormalisedPath); + + }); + + test(`should throw error if filepath does not start with ${config.ROOT_URL}`, () => { + + const spyPathResolve = jest.spyOn(path, 'resolve'); + const filePathNotStartingWithRootUrl = 'home'; + spyPathResolve.mockReturnValue(filePathNotStartingWithRootUrl); + + expect(() => validateFilepath(filePathNotStartingWithRootUrl)).toThrow(); + + }); + +}); From 5c7c7668a11fda050c72272610c653dee9c2e184 Mon Sep 17 00:00:00 2001 From: Harley Harris Date: Wed, 10 Jul 2024 11:21:45 +0100 Subject: [PATCH 5/5] remove duplicate error-list component --- src/views/error-list.html | 9 --------- 1 file changed, 9 deletions(-) delete mode 100644 src/views/error-list.html diff --git a/src/views/error-list.html b/src/views/error-list.html deleted file mode 100644 index 16c525c..0000000 --- a/src/views/error-list.html +++ /dev/null @@ -1,9 +0,0 @@ -{% if errors and errors.errorList and errors.errorList.length > 0 %} - {{ govukErrorSummary({ - titleText: "There is a problem", - errorList: errors.errorList if errors, - attributes: { - "tabindex": "0" - } - }) }} -{% endif %}