diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fa82bb..1103e45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Changed + +- `Content-Security-Policy` middleware now warns if a directive should have quotes but does not, such as `self` instead of `'self'`. This will be an error in future versions. See [#454](https://github.com/helmetjs/helmet/issues/454) + ## 7.1.0 - 2023-11-07 ### Added diff --git a/middlewares/content-security-policy/index.ts b/middlewares/content-security-policy/index.ts index 3fe9df8..e8b65ce 100644 --- a/middlewares/content-security-policy/index.ts +++ b/middlewares/content-security-policy/index.ts @@ -56,6 +56,18 @@ const DEFAULT_DIRECTIVES: Record< "upgrade-insecure-requests": [], }; +const SHOULD_BE_QUOTED: ReadonlySet = new Set([ + "none", + "self", + "strict-dynamic", + "report-sample", + "inline-speculation-rules", + "unsafe-inline", + "unsafe-eval", + "unsafe-hashes", + "wasm-unsafe-eval", +]); + const getDefaultDirectives = () => ({ ...DEFAULT_DIRECTIVES }); const dashify = (str: string): string => @@ -64,6 +76,23 @@ const dashify = (str: string): string => const isDirectiveValueInvalid = (directiveValue: string): boolean => /;|,/.test(directiveValue); +const shouldDirectiveValueEntryBeQuoted = ( + 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 has = (obj: Readonly, key: string): boolean => Object.prototype.hasOwnProperty.call(obj, key); @@ -138,13 +167,17 @@ function normalizeDirectives( } else { directiveValue = rawDirectiveValue; } + for (const element of directiveValue) { - if (typeof element === "string" && isDirectiveValueInvalid(element)) { - throw new Error( - `Content-Security-Policy received an invalid directive value for ${JSON.stringify( - directiveName, - )}`, - ); + if (typeof element === "string") { + if (isDirectiveValueInvalid(element)) { + throw new Error( + `Content-Security-Policy received an invalid directive value for ${JSON.stringify( + directiveName, + )}`, + ); + } + warnIfDirectiveValueEntryShouldBeQuoted(element); } } @@ -192,8 +225,13 @@ function getHeaderValue( normalizedDirectives.forEach((rawDirectiveValue, directiveName) => { let directiveValue = ""; for (const element of rawDirectiveValue) { - directiveValue += - " " + (element instanceof Function ? element(req, res) : element); + if (typeof element === "function") { + const newElement = element(req, res); + warnIfDirectiveValueEntryShouldBeQuoted(newElement); + directiveValue += " " + newElement; + } else { + directiveValue += " " + element; + } } if (!directiveValue) { diff --git a/test/content-security-policy.test.ts b/test/content-security-policy.test.ts index 0e1b185..db07587 100644 --- a/test/content-security-policy.test.ts +++ b/test/content-security-policy.test.ts @@ -7,6 +7,20 @@ import contentSecurityPolicy, { dangerouslyDisableDefaultSrc, } from "../middlewares/content-security-policy"; +const shouldBeQuoted = [ + "none", + "self", + "strict-dynamic", + "report-sample", + "inline-speculation-rules", + "unsafe-inline", + "unsafe-eval", + "unsafe-hashes", + "wasm-unsafe-eval", + "nonce-abc123", + "sha256-ks9D5epDKP+c2x6DrkuHmhmfKkOM/HZ+pOlzdWbI91k=", +]; + async function checkCsp({ middlewareArgs, expectedHeader = "content-security-policy", @@ -350,6 +364,21 @@ 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( @@ -383,6 +412,29 @@ describe("Content-Security-Policy middleware", () => { }); }); + 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.`, + ); + } + }); + it("throws if default-src is missing", () => { expect(() => { contentSecurityPolicy({