From 9c2c5edaf514fe84594024710c03ab3b7b0fbed1 Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Thu, 20 Jul 2023 15:27:20 +0200 Subject: [PATCH] feat: Ignore invalid header parts * patch: initial ideas around how to ignore invalid accept header parts. not fully tested, plenty of cleanup to do * patch: finish removing commented code. finalize unit tests for headerUtil methods that now ignore bad header parts rather than throwing as before * patch: remove @throws comments in HeaderUtil and update some comments to account for changes to returned values * feat: Give option for header parsing to be strict --------- Co-authored-by: Charlie Blevins --- src/util/HeaderUtil.ts | 222 +++++++++++++++++------------- test/unit/util/HeaderUtil.test.ts | 119 +++++++++++----- 2 files changed, 211 insertions(+), 130 deletions(-) diff --git a/src/util/HeaderUtil.ts b/src/util/HeaderUtil.ts index 915ed6879..5845d765e 100644 --- a/src/util/HeaderUtil.ts +++ b/src/util/HeaderUtil.ts @@ -140,6 +140,9 @@ const mediaRange = new RegExp(`${tchar.source}+/${tchar.source}+`, 'u'); * Replaces all double quoted strings in the input string with `"0"`, `"1"`, etc. * @param input - The Accept header string. * + * @throws {@link BadRequestHttpError} + * Thrown if invalid characters are detected in a quoted string. + * * @returns The transformed string and a map with keys `"0"`, etc. and values the original string that was there. */ export function transformQuotedStrings(input: string): { result: string; replacements: Record } { @@ -163,6 +166,8 @@ export function transformQuotedStrings(input: string): { result: string; replace * Splits the input string on commas, trims all parts and filters out empty ones. * * @param input - Input header string. + * + * @returns An array of trimmed strings. */ export function splitAndClean(input: string): string[] { return input.split(',') @@ -175,44 +180,67 @@ export function splitAndClean(input: string): string[] { * * @param qvalue - Input qvalue string (so "q=...."). * - * @throws {@link BadRequestHttpError} - * Thrown on invalid syntax. + * @returns true if q value is valid, false otherwise. */ -function testQValue(qvalue: string): void { - if (!/^(?:(?:0(?:\.\d{0,3})?)|(?:1(?:\.0{0,3})?))$/u.test(qvalue)) { - logger.warn(`Invalid q value: ${qvalue}`); - throw new BadRequestHttpError( - `Invalid q value: ${qvalue} does not match ( "0" [ "." 0*3DIGIT ] ) / ( "1" [ "." 0*3("0") ] ).`, - ); +function isValidQValue(qvalue: string): boolean { + return /^(?:(?:0(?:\.\d{0,3})?)|(?:1(?:\.0{0,3})?))$/u.test(qvalue); +} + +/** + * Converts a qvalue to a number. + * Returns 1 if the value is not a valid number or 1 if it is more than 1. + * Returns 0 if the value is negative. + * Otherwise, the parsed value is returned. + * + * @param qvalue - Value to convert. + */ +function parseQValue(qvalue: string): number { + const result = Number(qvalue); + if (Number.isNaN(result) || result >= 1) { + return 1; + } + if (result < 0) { + return 0; + } + return result; +} + +/** + * Logs a warning to indicate there was an invalid value. + * Throws a {@link BadRequestHttpError} in case `strict` is `true`. + * + * @param message - Message to log and potentially put in the error. + * @param strict - `true` if an error needs to be thrown. + */ +function handleInvalidValue(message: string, strict: boolean): void | never { + logger.warn(message); + if (strict) { + throw new BadRequestHttpError(message); } } /** - * Parses a list of split parameters and checks their validity. + * Parses a list of split parameters and checks their validity. Parameters with invalid + * syntax are ignored and not returned. * * @param parameters - A list of split parameters (token [ "=" ( token / quoted-string ) ]) * @param replacements - The double quoted strings that need to be replaced. - * - * - * @throws {@link BadRequestHttpError} - * Thrown on invalid parameter syntax. + * @param strict - Determines if invalid values throw errors (`true`) or log warnings (`false`). Defaults to `false`. * * @returns An array of name/value objects corresponding to the parameters. */ -export function parseParameters(parameters: string[], replacements: Record): +export function parseParameters(parameters: string[], replacements: Record, strict = false): { name: string; value: string }[] { - return parameters.map((param): { name: string; value: string } => { + return parameters.reduce<{ name: string; value: string }[]>((acc, param): { name: string; value: string }[] => { const [ name, rawValue ] = param.split('=').map((str): string => str.trim()); // Test replaced string for easier check // parameter = token "=" ( token / quoted-string ) // second part is optional for certain parameters if (!(token.test(name) && (!rawValue || /^"\d+"$/u.test(rawValue) || token.test(rawValue)))) { - logger.warn(`Invalid parameter value: ${name}=${replacements[rawValue] || rawValue}`); - throw new BadRequestHttpError( - `Invalid parameter value: ${name}=${replacements[rawValue] || rawValue} ` + - `does not match (token ( "=" ( token / quoted-string ))?). `, - ); + handleInvalidValue(`Invalid parameter value: ${name}=${replacements[rawValue] || rawValue} ` + + `does not match (token ( "=" ( token / quoted-string ))?). `, strict); + return acc; } let value = rawValue; @@ -220,8 +248,9 @@ export function parseParameters(parameters: string[], replacements: Record): Accept { +function parseAcceptPart(part: string, replacements: Record, strict: boolean): Accept | undefined { const [ range, ...parameters ] = part.split(';').map((param): string => param.trim()); // No reason to test differently for * since we don't check if the type exists if (!mediaRange.test(range)) { - logger.warn(`Invalid Accept range: ${range}`); - throw new BadRequestHttpError( - `Invalid Accept range: ${range} does not match ( "*/*" / ( token "/" "*" ) / ( token "/" token ) )`, + handleInvalidValue( + `Invalid Accept range: ${range} does not match ( "*/*" / ( token "/" "*" ) / ( token "/" token ) )`, strict, ); + return; } let weight = 1; @@ -258,13 +287,16 @@ function parseAcceptPart(part: string, replacements: Record): Ac if (name === 'q') { // Extension parameters appear after the q value map = extensionParams; - testQValue(value); - weight = Number.parseFloat(value); + if (!isValidQValue(value)) { + handleInvalidValue(`Invalid q value for range ${range}: ${value + } does not match ( "0" [ "." 0*3DIGIT ] ) / ( "1" [ "." 0*3("0") ] ).`, strict); + } + weight = parseQValue(value); } else { if (!value && map !== extensionParams) { - logger.warn(`Invalid Accept parameter ${name}`); - throw new BadRequestHttpError(`Invalid Accept parameter ${name}: ` + - `Accept parameter values are not optional when preceding the q value`); + handleInvalidValue(`Invalid Accept parameter ${name}: ` + + `Accept parameter values are not optional when preceding the q value`, strict); + return; } map[name] = value || ''; } @@ -282,14 +314,13 @@ function parseAcceptPart(part: string, replacements: Record): Ac /** * Parses an Accept-* header where each part is only a value and a weight, so roughly /.*(q=.*)?/ separated by commas. + * The returned weights default to 1 if no q value is found or the q value is invalid. * @param input - Input header string. - * - * @throws {@link BadRequestHttpError} - * Thrown on invalid qvalue syntax. + * @param strict - Determines if invalid values throw errors (`true`) or log warnings (`false`). Defaults to `false`. * * @returns An array of ranges and weights. */ -function parseNoParameters(input: string): AcceptHeader[] { +function parseNoParameters(input: string, strict = false): AcceptHeader[] { const parts = splitAndClean(input); return parts.map((part): AcceptHeader => { @@ -297,12 +328,15 @@ function parseNoParameters(input: string): AcceptHeader[] { const result = { range, weight: 1 }; if (qvalue) { if (!qvalue.startsWith('q=')) { - logger.warn(`Only q parameters are allowed in ${input}`); - throw new BadRequestHttpError(`Only q parameters are allowed in ${input}`); + handleInvalidValue(`Only q parameters are allowed in ${input}`, strict); + return result; } const val = qvalue.slice(2); - testQValue(val); - result.weight = Number.parseFloat(val); + if (!isValidQValue(val)) { + handleInvalidValue(`Invalid q value for range ${range}: ${val + } does not match ( "0" [ "." 0*3DIGIT ] ) / ( "1" [ "." 0*3("0") ] ).`, strict); + } + result.weight = parseQValue(val); } return result; }).sort((left, right): number => right.weight - left.weight); @@ -314,17 +348,25 @@ function parseNoParameters(input: string): AcceptHeader[] { * Parses an Accept header string. * * @param input - The Accept header string. + * @param strict - Determines if invalid values throw errors (`true`) or log warnings (`false`). Defaults to `false`. * - * @throws {@link BadRequestHttpError} - * Thrown on invalid header syntax. - * - * @returns An array of {@link Accept} objects, sorted by weight. + * @returns An array of {@link Accept} objects, sorted by weight. Accept parts + * with invalid syntax are ignored and removed from the returned array. */ -export function parseAccept(input: string): Accept[] { +export function parseAccept(input: string, strict = false): Accept[] { // Quoted strings could prevent split from having correct results const { result, replacements } = transformQuotedStrings(input); + return splitAndClean(result) - .map((part): Accept => parseAcceptPart(part, replacements)) + .reduce((acc, part): Accept[] => { + const partOrUndef = parseAcceptPart(part, replacements, strict); + + if (partOrUndef !== undefined) { + acc.push(partOrUndef); + } + + return acc; + }, []) .sort((left, right): number => right.weight - left.weight); } @@ -332,70 +374,65 @@ export function parseAccept(input: string): Accept[] { * Parses an Accept-Charset header string. * * @param input - The Accept-Charset header string. + * @param strict - Determines if invalid values throw errors (`true`) or log warnings (`false`). Defaults to `false`. * - * @throws {@link BadRequestHttpError} - * Thrown on invalid header syntax. - * - * @returns An array of {@link AcceptCharset} objects, sorted by weight. + * @returns An array of {@link AcceptCharset} objects, sorted by weight. Invalid ranges + * are ignored and not returned. */ -export function parseAcceptCharset(input: string): AcceptCharset[] { +export function parseAcceptCharset(input: string, strict = false): AcceptCharset[] { const results = parseNoParameters(input); - results.forEach((result): void => { + return results.filter((result): boolean => { if (!token.test(result.range)) { - logger.warn(`Invalid Accept-Charset range: ${result.range}`); - throw new BadRequestHttpError( - `Invalid Accept-Charset range: ${result.range} does not match (content-coding / "identity" / "*")`, + handleInvalidValue( + `Invalid Accept-Charset range: ${result.range} does not match (content-coding / "identity" / "*")`, strict, ); + return false; } + return true; }); - return results; } /** * Parses an Accept-Encoding header string. * * @param input - The Accept-Encoding header string. + * @param strict - Determines if invalid values throw errors (`true`) or log warnings (`false`). Defaults to `false`. * - * @throws {@link BadRequestHttpError} - * Thrown on invalid header syntax. - * - * @returns An array of {@link AcceptEncoding} objects, sorted by weight. + * @returns An array of {@link AcceptEncoding} objects, sorted by weight. Invalid ranges + * are ignored and not returned. */ -export function parseAcceptEncoding(input: string): AcceptEncoding[] { +export function parseAcceptEncoding(input: string, strict = false): AcceptEncoding[] { const results = parseNoParameters(input); - results.forEach((result): void => { + return results.filter((result): boolean => { if (!token.test(result.range)) { - logger.warn(`Invalid Accept-Encoding range: ${result.range}`); - throw new BadRequestHttpError(`Invalid Accept-Encoding range: ${result.range} does not match (charset / "*")`); + handleInvalidValue(`Invalid Accept-Encoding range: ${result.range} does not match (charset / "*")`, strict); + return false; } + return true; }); - return results; } /** * Parses an Accept-Language header string. * * @param input - The Accept-Language header string. + * @param strict - Determines if invalid values throw errors (`true`) or log warnings (`false`). Defaults to `false`. * - * @throws {@link BadRequestHttpError} - * Thrown on invalid header syntax. - * - * @returns An array of {@link AcceptLanguage} objects, sorted by weight. + * @returns An array of {@link AcceptLanguage} objects, sorted by weight. Invalid ranges + * are ignored and not returned. */ -export function parseAcceptLanguage(input: string): AcceptLanguage[] { +export function parseAcceptLanguage(input: string, strict = false): AcceptLanguage[] { const results = parseNoParameters(input); - results.forEach((result): void => { + return results.filter((result): boolean => { // (1*8ALPHA *("-" 1*8alphanum)) / "*" if (result.range !== '*' && !/^[a-zA-Z]{1,8}(?:-[a-zA-Z0-9]{1,8})*$/u.test(result.range)) { - logger.warn( - `Invalid Accept-Language range: ${result.range}`, - ); - throw new BadRequestHttpError( - `Invalid Accept-Language range: ${result.range} does not match ((1*8ALPHA *("-" 1*8alphanum)) / "*")`, + handleInvalidValue( + `Invalid Accept-Language range: ${result.range} does not match ((1*8ALPHA *("-" 1*8alphanum)) / "*")`, strict, ); + return false; } + return true; }); - return results; } // eslint-disable-next-line max-len @@ -405,24 +442,21 @@ const rfc1123Date = /^(?:Mon|Tue|Wed|Thu|Fri|Sat|Sun), \d{2} (?:Jan|Feb|Mar|Apr| * Parses an Accept-DateTime header string. * * @param input - The Accept-DateTime header string. + * @param strict - Determines if invalid values throw errors (`true`) or log warnings (`false`). Defaults to `false`. * - * @returns An array with a single {@link AcceptDatetime} object. + * @returns An array with a single {@link AcceptDatetime} object, + * or an empty array if a range in an invalid format is detected. */ -export function parseAcceptDateTime(input: string): AcceptDatetime[] { - const results: AcceptDatetime[] = []; +export function parseAcceptDateTime(input: string, strict = false): AcceptDatetime[] { const range = input.trim(); - if (range) { - if (!rfc1123Date.test(range)) { - logger.warn( - `Invalid Accept-DateTime range: ${range}`, - ); - throw new BadRequestHttpError( - `Invalid Accept-DateTime range: ${range} does not match the RFC1123 format`, - ); - } - results.push({ range, weight: 1 }); + if (!range) { + return []; } - return results; + if (!rfc1123Date.test(range)) { + handleInvalidValue(`Invalid Accept-DateTime range: ${range} does not match the RFC1123 format`, strict); + return []; + } + return [{ range, weight: 1 }]; } /** diff --git a/test/unit/util/HeaderUtil.test.ts b/test/unit/util/HeaderUtil.test.ts index e8eb77f74..049d2ac05 100644 --- a/test/unit/util/HeaderUtil.test.ts +++ b/test/unit/util/HeaderUtil.test.ts @@ -53,32 +53,63 @@ describe('HeaderUtil', (): void => { ]); }); - it('rejects Accept Headers with invalid types.', async(): Promise => { - expect((): any => parseAccept('*')).toThrow('Invalid Accept range:'); - expect((): any => parseAccept('"bad"/text')).toThrow('Invalid Accept range:'); - expect((): any => parseAccept('*/\\bad')).toThrow('Invalid Accept range:'); - expect((): any => parseAccept('*/*')).not.toThrow('Invalid Accept range:'); + it('ignores Accept Headers with invalid types.', async(): Promise => { + expect(parseAccept('*')).toEqual([]); + expect(parseAccept('"bad"/text')).toEqual([]); + expect(parseAccept('*/\\bad')).toEqual([]); + expect(parseAccept('*/*')).toEqual([{ + parameters: { extension: {}, mediaType: {}}, range: '*/*', weight: 1, + }]); }); - it('rejects Accept Headers with invalid q values.', async(): Promise => { - expect((): any => parseAccept('a/b; q=text')).toThrow('Invalid q value:'); - expect((): any => parseAccept('a/b; q=0.1234')).toThrow('Invalid q value:'); - expect((): any => parseAccept('a/b; q=1.1')).toThrow('Invalid q value:'); - expect((): any => parseAccept('a/b; q=1.000')).not.toThrow(); - expect((): any => parseAccept('a/b; q=0.123')).not.toThrow(); + it('ignores the weight of Accept Headers with q values it can not parse.', async(): Promise => { + expect(parseAccept('a/b; q=text')).toEqual([{ + range: 'a/b', weight: 1, parameters: { extension: {}, mediaType: {}}, + }]); + // Invalid Q value but can be parsed + expect(parseAccept('a/b; q=0.1234')).toEqual([{ + range: 'a/b', weight: 0.1234, parameters: { extension: {}, mediaType: {}}, + }]); + expect(parseAccept('a/b; q=1.1')).toEqual([{ + range: 'a/b', weight: 1, parameters: { extension: {}, mediaType: {}}, + }]); + expect(parseAccept('a/b; q=1.000')).toEqual([{ + range: 'a/b', weight: 1, parameters: { extension: {}, mediaType: {}}, + }]); + expect(parseAccept('a/b; q=-5')).toEqual([{ + range: 'a/b', weight: 0, parameters: { extension: {}, mediaType: {}}, + }]); + expect(parseAccept('a/b; q=0.123')).toEqual([{ + range: 'a/b', weight: 0.123, parameters: { extension: {}, mediaType: {}}, + }]); }); - it('rejects Accept Headers with invalid parameters.', async(): Promise => { - expect((): any => parseAccept('a/b; a')).toThrow('Invalid Accept parameter'); - expect((): any => parseAccept('a/b; a=\\')).toThrow('Invalid parameter value'); - expect((): any => parseAccept('a/b; q=1 ; a=\\')).toThrow('Invalid parameter value'); - expect((): any => parseAccept('a/b; q=1 ; a')).not.toThrow('Invalid Accept parameter'); + it('ignores Accept Headers with invalid parameters.', async(): Promise => { + expect(parseAccept('a/b; a')).toEqual([{ + range: 'a/b', weight: 1, parameters: { extension: {}, mediaType: {}}, + }]); + expect(parseAccept('a/b; a=\\')).toEqual([{ + range: 'a/b', weight: 1, parameters: { extension: {}, mediaType: {}}, + }]); + expect(parseAccept('a/b; q=1 ; a=\\')).toEqual([{ + range: 'a/b', weight: 1, parameters: { extension: {}, mediaType: {}}, + }]); + expect(parseAccept('a/b; q=1 ; a')).toEqual([{ + // eslint-disable-next-line id-length + range: 'a/b', weight: 1, parameters: { extension: { a: '' }, mediaType: {}}, + }]); }); it('rejects Accept Headers with quoted parameters.', async(): Promise => { expect((): any => parseAccept('a/b; a="\\""')).not.toThrow(); expect((): any => parseAccept('a/b; a="\\\u007F"')).toThrow('Invalid quoted string in header:'); }); + + it('rejects invalid values when strict mode is enabled.', async(): Promise => { + expect((): any => parseAccept('"bad"/text', true)).toThrow(BadRequestHttpError); + expect((): any => parseAccept('a/b; q=text', true)).toThrow(BadRequestHttpError); + expect((): any => parseAccept('a/b; a', true)).toThrow(BadRequestHttpError); + }); }); describe('#parseCharset', (): void => { @@ -89,10 +120,14 @@ describe('HeaderUtil', (): void => { ]); }); - it('rejects invalid Accept-Charset Headers.', async(): Promise => { - expect((): any => parseAcceptCharset('a/b')).toThrow('Invalid Accept-Charset range:'); - expect((): any => parseAcceptCharset('a; q=text')).toThrow('Invalid q value:'); - expect((): any => parseAcceptCharset('a; c=d')).toThrow('Only q parameters are allowed'); + it('ignores invalid Accept-Charset Headers.', async(): Promise => { + expect(parseAcceptCharset('a/b')).toEqual([]); + expect(parseAcceptCharset('a; q=text')).toEqual([{ range: 'a', weight: 1 }]); + expect(parseAcceptCharset('a; c=d')).toEqual([{ range: 'a', weight: 1 }]); + }); + + it('rejects invalid values when strict mode is enabled.', async(): Promise => { + expect((): any => parseAcceptCharset('a/b', true)).toThrow(BadRequestHttpError); }); }); @@ -109,10 +144,14 @@ describe('HeaderUtil', (): void => { ]); }); - it('rejects invalid Accept-Encoding Headers.', async(): Promise => { - expect((): any => parseAcceptEncoding('a/b')).toThrow('Invalid Accept-Encoding range:'); - expect((): any => parseAcceptEncoding('a; q=text')).toThrow('Invalid q value:'); - expect((): any => parseAcceptCharset('a; c=d')).toThrow('Only q parameters are allowed'); + it('ignores invalid Accept-Encoding Headers.', async(): Promise => { + expect(parseAcceptEncoding('a/b')).toEqual([]); + expect(parseAcceptEncoding('a; q=text')).toEqual([{ range: 'a', weight: 1 }]); + expect(parseAcceptCharset('a; c=d')).toEqual([{ range: 'a', weight: 1 }]); + }); + + it('rejects invalid values when strict mode is enabled.', async(): Promise => { + expect((): any => parseAcceptEncoding('a/b', true)).toThrow(BadRequestHttpError); }); }); @@ -125,16 +164,20 @@ describe('HeaderUtil', (): void => { ]); }); - it('rejects invalid Accept-Language Headers.', async(): Promise => { - expect((): any => parseAcceptLanguage('a/b')).toThrow('Invalid Accept-Language range:'); - expect((): any => parseAcceptLanguage('05-a')).toThrow('Invalid Accept-Language range:'); - expect((): any => parseAcceptLanguage('a--05')).toThrow('Invalid Accept-Language range:'); - expect((): any => parseAcceptLanguage('a-"a"')).toThrow('Invalid Accept-Language range:'); - expect((): any => parseAcceptLanguage('a-05')).not.toThrow('Invalid Accept-Language range:'); - expect((): any => parseAcceptLanguage('a-b-c-d')).not.toThrow('Invalid Accept-Language range:'); + it('ignores invalid Accept-Language Headers.', async(): Promise => { + expect(parseAcceptLanguage('a/b')).toEqual([]); + expect(parseAcceptLanguage('05-a')).toEqual([]); + expect(parseAcceptLanguage('a--05')).toEqual([]); + expect(parseAcceptLanguage('a-"a"')).toEqual([]); + expect(parseAcceptLanguage('a-05')).toEqual([{ range: 'a-05', weight: 1 }]); + expect(parseAcceptLanguage('a-b-c-d')).toEqual([{ range: 'a-b-c-d', weight: 1 }]); - expect((): any => parseAcceptLanguage('a; q=text')).toThrow('Invalid q value:'); - expect((): any => parseAcceptCharset('a; c=d')).toThrow('Only q parameters are allowed'); + expect(parseAcceptLanguage('a; q=text')).toEqual([{ range: 'a', weight: 1 }]); + expect(parseAcceptCharset('a; c=d')).toEqual([{ range: 'a', weight: 1 }]); + }); + + it('rejects invalid values when strict mode is enabled.', async(): Promise => { + expect((): any => parseAcceptLanguage('a/b', true)).toThrow(BadRequestHttpError); }); }); @@ -150,9 +193,13 @@ describe('HeaderUtil', (): void => { expect(parseAcceptDateTime(' ')).toEqual([]); }); - it('rejects invalid Accept-DateTime Headers.', async(): Promise => { - expect((): any => parseAcceptDateTime('a/b')).toThrow('Invalid Accept-DateTime range:'); - expect((): any => parseAcceptDateTime('30 May 2007')).toThrow('Invalid Accept-DateTime range:'); + it('ignores invalid Accept-DateTime Headers.', async(): Promise => { + expect(parseAcceptDateTime('a/b')).toEqual([]); + expect(parseAcceptDateTime('30 May 2007')).toEqual([]); + }); + + it('rejects invalid values when strict mode is enabled.', async(): Promise => { + expect((): any => parseAcceptLanguage('a/b', true)).toThrow(BadRequestHttpError); }); });