diff --git a/CHANGELOG.md b/CHANGELOG.md index 845ab73..0b776e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Changelog +## 8.0.0 + +### Changed + +- **Breaking:** `Strict-Transport-Security` now has a max-age of 365 days, up from 180 +- **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) +- **Breaking:** `Content-Security-Policy`'s `getDefaultDirectives` now returns a deep copy. This only affects users who were mutating the result +- **Breaking:** `Strict-Transport-Security` now throws an error when "includeSubDomains" option is misspelled. This was previously a warning + +### Removed + +- **Breaking:** Drop support for Node 16 and 17. Node 18+ is now required + ## 7.2.0 - 2024-09-28 ### Changed diff --git a/middlewares/content-security-policy/index.ts b/middlewares/content-security-policy/index.ts index e8b65ce..ee4505b 100644 --- a/middlewares/content-security-policy/index.ts +++ b/middlewares/content-security-policy/index.ts @@ -39,10 +39,22 @@ interface ContentSecurityPolicy { const dangerouslyDisableDefaultSrc = Symbol("dangerouslyDisableDefaultSrc"); -const DEFAULT_DIRECTIVES: Record< +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 = (): Record< string, Iterable -> = { +> => ({ "default-src": ["'self'"], "base-uri": ["'self'"], "font-src": ["'self'", "https:", "data:"], @@ -54,21 +66,7 @@ const DEFAULT_DIRECTIVES: Record< "script-src-attr": ["'none'"], "style-src": ["'self'", "https:", "'unsafe-inline'"], "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 => str.replace(/[A-Z]/g, (capitalLetter) => "-" + capitalLetter.toLowerCase()); @@ -76,25 +74,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 has = (obj: Readonly, key: string): boolean => - Object.prototype.hasOwnProperty.call(obj, key); +const invalidDirectiveValueError = (directiveName: string): Error => + new Error( + `Content-Security-Policy received an invalid directive value for ${JSON.stringify( + directiveName, + )}`, + ); function normalizeDirectives( options: Readonly, @@ -109,7 +101,7 @@ function normalizeDirectives( const directivesExplicitlyDisabled = new Set(); for (const rawDirectiveName in rawDirectives) { - if (!has(rawDirectives, rawDirectiveName)) { + if (!Object.hasOwn(rawDirectives, rawDirectiveName)) { continue; } @@ -169,15 +161,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); } } @@ -219,15 +208,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; @@ -237,17 +227,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/middlewares/strict-transport-security/index.ts b/middlewares/strict-transport-security/index.ts index ea242ba..18c45cf 100644 --- a/middlewares/strict-transport-security/index.ts +++ b/middlewares/strict-transport-security/index.ts @@ -1,6 +1,6 @@ import type { IncomingMessage, ServerResponse } from "http"; -const DEFAULT_MAX_AGE = 180 * 24 * 60 * 60; +const DEFAULT_MAX_AGE = 365 * 24 * 60 * 60; export interface StrictTransportSecurityOptions { maxAge?: number; @@ -29,7 +29,7 @@ function getHeaderValueFromOptions( ); } if ("includeSubdomains" in options) { - console.warn( + throw new Error( 'Strict-Transport-Security middleware should use `includeSubDomains` instead of `includeSubdomains`. (The correct one has an uppercase "D".)', ); } diff --git a/package-lock.json b/package-lock.json index 6642c30..d441540 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5,7 +5,6 @@ "requires": true, "packages": { "": { - "name": "helmet", "version": "7.2.0", "devDependencies": { "@eslint/js": "^9.11.1", @@ -30,7 +29,7 @@ "typescript-eslint": "^8.7.0" }, "engines": { - "node": ">=16.0.0" + "node": ">=18.0.0" } }, "node_modules/@ampproject/remapping": { diff --git a/package.json b/package.json index 5b2365d..bc088c2 100644 --- a/package.json +++ b/package.json @@ -35,6 +35,6 @@ }, "type": "module", "engines": { - "node": ">=16.0.0" + "node": ">=18.0.0" } } diff --git a/test/content-security-policy.test.ts b/test/content-security-policy.test.ts index 00dfe6f..b4fc44f 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", () => { @@ -607,4 +581,14 @@ describe("getDefaultDirectives", () => { contentSecurityPolicy.getDefaultDirectives, ); }); + + it("returns a new copy each time", () => { + const one = getDefaultDirectives(); + one["worker-src"] = ["ignored.example"]; + (one["img-src"] as Array).push("ignored.example"); + + const two = getDefaultDirectives(); + expect(two).not.toHaveProperty("worker-src"); + expect(two["img-src"]).not.toContain("ignored.example"); + }); }); diff --git a/test/index.test.ts b/test/index.test.ts index ed9738c..5604e25 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -35,7 +35,7 @@ describe("helmet", () => { "cross-origin-resource-policy": "same-origin", "origin-agent-cluster": "?1", "referrer-policy": "no-referrer", - "strict-transport-security": "max-age=15552000; includeSubDomains", + "strict-transport-security": "max-age=31536000; includeSubDomains", "x-content-type-options": "nosniff", "x-dns-prefetch-control": "off", "x-download-options": "noopen", diff --git a/test/strict-transport-security.test.ts b/test/strict-transport-security.test.ts index fa132e6..73cf33f 100644 --- a/test/strict-transport-security.test.ts +++ b/test/strict-transport-security.test.ts @@ -3,10 +3,10 @@ import strictTransportSecurity from "../middlewares/strict-transport-security"; describe("Strict-Transport-Security middleware", () => { it('by default, sets max-age to 180 days and adds "includeSubDomains"', async () => { - expect(15552000).toStrictEqual(180 * 24 * 60 * 60); + expect(31536000).toStrictEqual(365 * 24 * 60 * 60); const expectedHeaders = { - "strict-transport-security": "max-age=15552000; includeSubDomains", + "strict-transport-security": "max-age=31536000; includeSubDomains", }; await check(strictTransportSecurity(), expectedHeaders); @@ -45,20 +45,20 @@ describe("Strict-Transport-Security middleware", () => { it("disables subdomains with the includeSubDomains option", async () => { await check(strictTransportSecurity({ includeSubDomains: false }), { - "strict-transport-security": "max-age=15552000", + "strict-transport-security": "max-age=31536000", }); }); it("can enable preloading", async () => { await check(strictTransportSecurity({ preload: true }), { "strict-transport-security": - "max-age=15552000; includeSubDomains; preload", + "max-age=31536000; includeSubDomains; preload", }); }); it("can explicitly disable preloading", async () => { await check(strictTransportSecurity({ preload: false }), { - "strict-transport-security": "max-age=15552000; includeSubDomains", + "strict-transport-security": "max-age=31536000; includeSubDomains", }); }); @@ -87,12 +87,9 @@ describe("Strict-Transport-Security middleware", () => { }); it("logs a warning when using the mis-capitalized `includeSubdomains` parameter", () => { - jest.spyOn(console, "warn").mockImplementation(() => {}); - - strictTransportSecurity({ includeSubdomains: false } as any); - - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn).toHaveBeenCalledWith( + expect(() => + strictTransportSecurity({ includeSubdomains: false } as any), + ).toThrow( 'Strict-Transport-Security middleware should use `includeSubDomains` instead of `includeSubdomains`. (The correct one has an uppercase "D".)', ); }); diff --git a/tsconfig.json b/tsconfig.json index b3f2c75..a54d412 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -10,7 +10,7 @@ "noUncheckedIndexedAccess": true, "noUncheckedSideEffectImports": true, "strict": true, - "target": "es6", + "target": "es2022", "outDir": "." }, "exclude": ["node_modules"]