From aa98cf72a1d692ca36c9c0fc0b138490a8c66b66 Mon Sep 17 00:00:00 2001 From: zainZzKk <51957827+Zainzzkk@users.noreply.github.com> Date: Tue, 6 Aug 2024 12:41:37 +0100 Subject: [PATCH] chore(EMS-3708): code scanning - Server-side URL redirect (#2882) * chore(EMS-3708): fix code scanning issue with unvalidated req.originalUrl * chore(EMS-3708): code fixes * chore(EMS-3707): code fixes * chore(EMS-3707): test fix * chore(EMS-3707): console.error fix --------- Co-authored-by: Zain Kassam --- src/ui/server/constants/regex.ts | 14 ++++ .../controllers/redirects/index.test.ts | 32 ++++++-- src/ui/server/controllers/redirects/index.ts | 18 ++++- .../is-valid-req-original-url/index.test.ts | 73 +++++++++++++++++++ .../is-valid-req-original-url/index.ts | 22 ++++++ 5 files changed, 152 insertions(+), 7 deletions(-) create mode 100644 src/ui/server/helpers/is-valid-req-original-url/index.test.ts create mode 100644 src/ui/server/helpers/is-valid-req-original-url/index.ts diff --git a/src/ui/server/constants/regex.ts b/src/ui/server/constants/regex.ts index 56f0eccae3..322a8bbfb1 100644 --- a/src/ui/server/constants/regex.ts +++ b/src/ui/server/constants/regex.ts @@ -30,6 +30,20 @@ export const REGEX = { */ NUMBER_HYPHEN_SPACE: /^[- \d]+$/, + /* + * VALID_REQUEST_ORIGINAL_URL: + * Regex that allows only: + * - numbers. + * - letters. + * - a hyphen. + * - a question mark. + * - a slash. + * - a dash. + * - &. + * Note: \d is exactly the same as [0-9] + */ + VALID_REQUEST_ORIGINAL_URL: /^[A-Za-z-=&?/\d]+$/, + /** * SPACE_AND_HYPHEN: * Regex that allows only: diff --git a/src/ui/server/controllers/redirects/index.test.ts b/src/ui/server/controllers/redirects/index.test.ts index de25dc6657..031e6a81fc 100644 --- a/src/ui/server/controllers/redirects/index.test.ts +++ b/src/ui/server/controllers/redirects/index.test.ts @@ -1,8 +1,11 @@ import get from '.'; import generateRedirectUrl from '../../helpers/generate-redirect-url'; +import { ROUTES } from '../../constants'; import { Request, Response } from '../../../types'; import { mockReq, mockRes } from '../../test-mocks'; +const { PROBLEM_WITH_SERVICE } = ROUTES.INSURANCE; + describe('controllers/redirects', () => { let req: Request; let res: Response; @@ -12,12 +15,31 @@ describe('controllers/redirects', () => { res = mockRes(); }); - it('should redirect to a new URL via generateRedirectUrl helper function', () => { - get(req, res); + describe('when the req.originalUrl is valid', () => { + beforeEach(() => { + req = mockReq(); + }); + + it('should redirect to a new URL via generateRedirectUrl helper function', () => { + get(req, res); + + const expected = generateRedirectUrl(req.originalUrl); + + expect(res.redirect).toHaveBeenCalledTimes(1); + expect(res.redirect).toHaveBeenCalledWith(expected); + }); + }); + + describe('when the req.original is invalid', () => { + beforeEach(() => { + req.originalUrl = ''; + }); - const expected = generateRedirectUrl(req.originalUrl); + it(`should redirect to a ${PROBLEM_WITH_SERVICE}`, () => { + get(req, res); - expect(res.redirect).toHaveBeenCalledTimes(1); - expect(res.redirect).toHaveBeenCalledWith(expected); + expect(res.redirect).toHaveBeenCalledTimes(1); + expect(res.redirect).toHaveBeenCalledWith(PROBLEM_WITH_SERVICE); + }); }); }); diff --git a/src/ui/server/controllers/redirects/index.ts b/src/ui/server/controllers/redirects/index.ts index ee821a0131..dde882a9af 100644 --- a/src/ui/server/controllers/redirects/index.ts +++ b/src/ui/server/controllers/redirects/index.ts @@ -1,15 +1,29 @@ import generateRedirectUrl from '../../helpers/generate-redirect-url'; +import isValidReqOriginalUrl from '../../helpers/is-valid-req-original-url'; +import { ROUTES } from '../../constants'; import { Request, Response } from '../../../types'; +const { PROBLEM_WITH_SERVICE } = ROUTES.INSURANCE; + /** * get - * Redirect an MVP_INSURANCE_ROOT URL to an INSURANCE_ROOT URL + * Redirect an MVP_INSURANCE_ROOT URL to an INSURANCE_ROOT URL if req.originalUrl is valid + * Redirect to PROBLEM_WITH_SERVICE if req.originalUrl is invalid * @param {Express.Request} Express request * @param {Express.Response} Express response * @returns {Express.Response.redirect} */ const get = (req: Request, res: Response) => { - const newUrl = generateRedirectUrl(req.originalUrl); + const { originalUrl } = req; + + const validUrl = isValidReqOriginalUrl(originalUrl); + + if (!validUrl) { + console.error('Invalid original URL detected in "redirects" controller: %s', originalUrl); + return res.redirect(PROBLEM_WITH_SERVICE); + } + + const newUrl = generateRedirectUrl(originalUrl); return res.redirect(newUrl); }; diff --git a/src/ui/server/helpers/is-valid-req-original-url/index.test.ts b/src/ui/server/helpers/is-valid-req-original-url/index.test.ts new file mode 100644 index 0000000000..ec157f3497 --- /dev/null +++ b/src/ui/server/helpers/is-valid-req-original-url/index.test.ts @@ -0,0 +1,73 @@ +import isValidReqOriginalUrl from '.'; + +describe('helpers/is-valid-req-original-url', () => { + describe('when provided url has invalid special characters', () => { + it('should return false', () => { + const mockUrl = 'abc!'; + + const result = isValidReqOriginalUrl(mockUrl); + + expect(result).toEqual(false); + }); + }); + + describe('when provided url is an empty string', () => { + it('should return false', () => { + const mockUrl = ''; + + const result = isValidReqOriginalUrl(mockUrl); + + expect(result).toEqual(false); + }); + }); + + describe('when provided url contains a space', () => { + it('should return false', () => { + const mockUrl = 'abc '; + + const result = isValidReqOriginalUrl(mockUrl); + + expect(result).toEqual(false); + }); + }); + + describe('when provided url only contains letters', () => { + it('should return true', () => { + const mockUrl = 'abc'; + + const result = isValidReqOriginalUrl(mockUrl); + + expect(result).toEqual(true); + }); + }); + + describe('when provided url contains letters and numbers', () => { + it('should return true', () => { + const mockUrl = 'abc123'; + + const result = isValidReqOriginalUrl(mockUrl); + + expect(result).toEqual(true); + }); + }); + + describe('when provided url only contains numbers', () => { + it('should return true', () => { + const mockUrl = '123'; + + const result = isValidReqOriginalUrl(mockUrl); + + expect(result).toEqual(true); + }); + }); + + describe('when provided url contains letters, numbers, "/", "?", "&", "-" and "="', () => { + it('should return true', () => { + const mockUrl = '123abc/?&-='; + + const result = isValidReqOriginalUrl(mockUrl); + + expect(result).toEqual(true); + }); + }); +}); diff --git a/src/ui/server/helpers/is-valid-req-original-url/index.ts b/src/ui/server/helpers/is-valid-req-original-url/index.ts new file mode 100644 index 0000000000..303198bb46 --- /dev/null +++ b/src/ui/server/helpers/is-valid-req-original-url/index.ts @@ -0,0 +1,22 @@ +import Joi from 'joi'; +import { REGEX } from '../../constants'; + +/** + * Checks if the provided original URL is valid. + * checks against NUMBER_ALPHA_HYPHEN_DASH_QUESTION_EQUALS_AND regex + * if error on validation, returns false + * otherwise, return true + * @param {string} originalUrl - The original URL to validate. + * @returns {Boolean} + */ +const isValidReqOriginalUrl = (originalUrl: string) => { + const joiString = Joi.string(); + + const schema = () => joiString.regex(REGEX.VALID_REQUEST_ORIGINAL_URL).required(); + + const validation = schema().validate(originalUrl); + + return !validation.error; +}; + +export default isValidReqOriginalUrl;