Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(EMS-3708): code scanning - Server-side URL redirect #2882

Merged
merged 8 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/ui/server/constants/regex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
32 changes: 27 additions & 5 deletions src/ui/server/controllers/redirects/index.test.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand All @@ -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);
});
});
});
18 changes: 16 additions & 2 deletions src/ui/server/controllers/redirects/index.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,31 @@
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);
Zainzzkk marked this conversation as resolved.
Show resolved Hide resolved
}

const newUrl = generateRedirectUrl(originalUrl);

return res.redirect(newUrl);

Check warning

Code scanning / CodeQL

Server-side URL redirect Medium

Untrusted URL redirection depends on a
user-provided value
.
};

export default get;
73 changes: 73 additions & 0 deletions src/ui/server/helpers/is-valid-req-original-url/index.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
22 changes: 22 additions & 0 deletions src/ui/server/helpers/is-valid-req-original-url/index.ts
Original file line number Diff line number Diff line change
@@ -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.
ttbarnes marked this conversation as resolved.
Show resolved Hide resolved
* @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;
ttbarnes marked this conversation as resolved.
Show resolved Hide resolved
Loading