diff --git a/CHANGELOG.md b/CHANGELOG.md index fdb726c..44c91bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## 8.0.0 +### Changed + +- **Breaking:** `Content-Security-Policy` middleware now throws an error if a directive should have quotes but does not, such as `self` instead of `'self'`. See [#454](https://github.com/helmetjs/helmet/issues/454) + ### Removed - **Breaking:** Drop support for Node 16 and 17. Node 18+ is now required diff --git a/middlewares/content-security-policy/index.ts b/middlewares/content-security-policy/index.ts index 5fb9eb0..d336631 100644 --- a/middlewares/content-security-policy/index.ts +++ b/middlewares/content-security-policy/index.ts @@ -76,22 +76,19 @@ const dashify = (str: string): string => const isDirectiveValueInvalid = (directiveValue: string): boolean => /;|,/.test(directiveValue); -const shouldDirectiveValueEntryBeQuoted = ( - directiveValueEntry: string, -): boolean => +const isDirectiveValueEntryInvalid = (directiveValueEntry: string): boolean => SHOULD_BE_QUOTED.has(directiveValueEntry) || directiveValueEntry.startsWith("nonce-") || directiveValueEntry.startsWith("sha256-") || directiveValueEntry.startsWith("sha384-") || directiveValueEntry.startsWith("sha512-"); -const warnIfDirectiveValueEntryShouldBeQuoted = (value: string): void => { - if (shouldDirectiveValueEntryBeQuoted(value)) { - console.warn( - `Content-Security-Policy got directive value \`${value}\` which should be single-quoted and changed to \`'${value}'\`. This will be an error in future versions of Helmet.`, - ); - } -}; +const invalidDirectiveValueError = (directiveName: string): Error => + new Error( + `Content-Security-Policy received an invalid directive value for ${JSON.stringify( + directiveName, + )}`, + ); function normalizeDirectives( options: Readonly, @@ -166,15 +163,12 @@ function normalizeDirectives( } for (const element of directiveValue) { - if (typeof element === "string") { - if (isDirectiveValueInvalid(element)) { - throw new Error( - `Content-Security-Policy received an invalid directive value for ${JSON.stringify( - directiveName, - )}`, - ); - } - warnIfDirectiveValueEntryShouldBeQuoted(element); + if ( + typeof element === "string" && + (isDirectiveValueInvalid(element) || + isDirectiveValueEntryInvalid(element)) + ) { + throw invalidDirectiveValueError(directiveName); } } @@ -216,15 +210,16 @@ function getHeaderValue( res: ServerResponse, normalizedDirectives: Readonly, ): string | Error { - let err: undefined | Error; const result: string[] = []; - normalizedDirectives.forEach((rawDirectiveValue, directiveName) => { + for (const [directiveName, rawDirectiveValue] of normalizedDirectives) { let directiveValue = ""; for (const element of rawDirectiveValue) { if (typeof element === "function") { const newElement = element(req, res); - warnIfDirectiveValueEntryShouldBeQuoted(newElement); + if (isDirectiveValueEntryInvalid(newElement)) { + return invalidDirectiveValueError(directiveName); + } directiveValue += " " + newElement; } else { directiveValue += " " + element; @@ -234,17 +229,13 @@ function getHeaderValue( if (!directiveValue) { result.push(directiveName); } else if (isDirectiveValueInvalid(directiveValue)) { - err = new Error( - `Content-Security-Policy received an invalid directive value for ${JSON.stringify( - directiveName, - )}`, - ); + return invalidDirectiveValueError(directiveName); } else { result.push(`${directiveName}${directiveValue}`); } - }); + } - return err ? err : result.join(";"); + return result.join(";"); } const contentSecurityPolicy: ContentSecurityPolicy = diff --git a/test/content-security-policy.test.ts b/test/content-security-policy.test.ts index db07587..5f6d41c 100644 --- a/test/content-security-policy.test.ts +++ b/test/content-security-policy.test.ts @@ -348,7 +348,13 @@ describe("Content-Security-Policy middleware", () => { }); it("throws if any directive values are invalid", () => { - const invalidValues = [";", ",", "hello;world", "hello,world"]; + const invalidValues = [ + ";", + ",", + "hello;world", + "hello,world", + ...shouldBeQuoted, + ]; for (const invalidValue of invalidValues) { expect(() => { contentSecurityPolicy({ @@ -364,75 +370,43 @@ describe("Content-Security-Policy middleware", () => { } }); - it("at call time, warns if directive values lack quotes when they should", () => { - jest.spyOn(console, "warn").mockImplementation(() => {}); - - contentSecurityPolicy({ - directives: { defaultSrc: shouldBeQuoted }, - }); - - expect(console.warn).toHaveBeenCalledTimes(shouldBeQuoted.length); - for (const directiveValue of shouldBeQuoted) { - expect(console.warn).toHaveBeenCalledWith( - `Content-Security-Policy got directive value \`${directiveValue}\` which should be single-quoted and changed to \`'${directiveValue}'\`. This will be an error in future versions of Helmet.`, - ); - } - }); - it("errors if any directive values are invalid when a function returns", async () => { - const app = connect() - .use( - contentSecurityPolicy({ - useDefaults: false, - directives: { - defaultSrc: ["'self'", () => "bad;value"], - }, - }), - ) - .use( - ( - err: Error, - _req: IncomingMessage, - res: ServerResponse, - // eslint-disable-next-line @typescript-eslint/no-unused-vars - _next: () => void, - ) => { - res.statusCode = 500; - res.setHeader("Content-Type", "application/json"); - res.end( - JSON.stringify({ helmetTestError: true, message: err.message }), + const badDirectiveValueEntries = ["bad;value", ...shouldBeQuoted]; + + await Promise.all( + badDirectiveValueEntries.map(async (directiveValueEntry) => { + const app = connect() + .use( + contentSecurityPolicy({ + useDefaults: false, + directives: { + defaultSrc: ["'self'", () => directiveValueEntry], + }, + }), + ) + .use( + ( + err: Error, + _req: IncomingMessage, + res: ServerResponse, + // eslint-disable-next-line @typescript-eslint/no-unused-vars + _next: () => void, + ) => { + res.statusCode = 500; + res.setHeader("Content-Type", "application/json"); + res.end( + JSON.stringify({ helmetTestError: true, message: err.message }), + ); + }, ); - }, - ); - - await supertest(app).get("/").expect(500, { - helmetTestError: true, - message: - 'Content-Security-Policy received an invalid directive value for "default-src"', - }); - }); - - it("at request time, warns if directive values lack quotes when they should", async () => { - jest.spyOn(console, "warn").mockImplementation(() => {}); - - const app = connect() - .use( - contentSecurityPolicy({ - directives: { defaultSrc: shouldBeQuoted }, - }), - ) - .use((_req: IncomingMessage, res: ServerResponse) => { - res.end(); - }); - await supertest(app).get("/").expect(200); - - expect(console.warn).toHaveBeenCalledTimes(shouldBeQuoted.length); - for (const directiveValue of shouldBeQuoted) { - expect(console.warn).toHaveBeenCalledWith( - `Content-Security-Policy got directive value \`${directiveValue}\` which should be single-quoted and changed to \`'${directiveValue}'\`. This will be an error in future versions of Helmet.`, - ); - } + await supertest(app).get("/").expect(500, { + helmetTestError: true, + message: + 'Content-Security-Policy received an invalid directive value for "default-src"', + }); + }), + ); }); it("throws if default-src is missing", () => {