From c62f23ddf2710a6fd81d9f69be2ba1fad59c5f9a Mon Sep 17 00:00:00 2001 From: Najam Ul Saqib Date: Thu, 18 Jan 2024 18:18:32 +0500 Subject: [PATCH 1/7] Require Node 18+ See [#444][0] and [#450][1]. [0]: https://github.com/helmetjs/helmet/issues/444 [1]: https://github.com/helmetjs/helmet/pull/450 Signed-off-by: Najam Ul Saqib Co-authored-by: Evan Hahn --- CHANGELOG.md | 6 ++++++ package-lock.json | 3 +-- package.json | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 845ab73..5027a08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## 8.0.0 + +### Removed + +- **Breaking:** Drop support for Node 16 and 17. Node 18+ is now required + ## 7.2.0 - 2024-09-28 ### Changed 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" } } From f4f4952b554309cee36b707f2b1eb96402497bc8 Mon Sep 17 00:00:00 2001 From: Sohrab Chegini Date: Sat, 27 Apr 2024 18:55:16 +0330 Subject: [PATCH 2/7] Strict-Transport-Security: increase max-age to 1 year See [#457] and [#459]. [#457]: https://github.com/helmetjs/helmet/issues/457 [#459]: https://github.com/helmetjs/helmet/pull/459 --- CHANGELOG.md | 4 ++++ middlewares/strict-transport-security/index.ts | 2 +- test/index.test.ts | 2 +- test/strict-transport-security.test.ts | 10 +++++----- 4 files changed, 11 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5027a08..3eead06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## 8.0.0 +### Changed + +- **Breaking:** `Strict-Transport-Security` now has a max-age of 365 days, up from 180 + ### Removed - **Breaking:** Drop support for Node 16 and 17. Node 18+ is now required diff --git a/middlewares/strict-transport-security/index.ts b/middlewares/strict-transport-security/index.ts index ea242ba..fa6e773 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; 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..dc1d780 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", }); }); From 5bc961e4a952f6d021210fde4257c0d2f6e9c8d6 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Sat, 27 Apr 2024 16:43:59 +0000 Subject: [PATCH 3/7] Content-Security-Policy can now use Object.hasOwn We can use this now that we require Node 18+. --- middlewares/content-security-policy/index.ts | 5 +---- tsconfig.json | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/middlewares/content-security-policy/index.ts b/middlewares/content-security-policy/index.ts index e8b65ce..5fb9eb0 100644 --- a/middlewares/content-security-policy/index.ts +++ b/middlewares/content-security-policy/index.ts @@ -93,9 +93,6 @@ const warnIfDirectiveValueEntryShouldBeQuoted = (value: string): void => { } }; -const has = (obj: Readonly, key: string): boolean => - Object.prototype.hasOwnProperty.call(obj, key); - function normalizeDirectives( options: Readonly, ): NormalizedDirectives { @@ -109,7 +106,7 @@ function normalizeDirectives( const directivesExplicitlyDisabled = new Set(); for (const rawDirectiveName in rawDirectives) { - if (!has(rawDirectives, rawDirectiveName)) { + if (!Object.hasOwn(rawDirectives, rawDirectiveName)) { continue; } 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"] From 07f72c0ec2f408a63c4a65e158343bfb7cf22b2e Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Sat, 27 Apr 2024 17:13:01 +0000 Subject: [PATCH 4/7] Content-Security-Policy: throw if directive value lacks necessary quotes Closes [#454]. [#454]: https://github.com/helmetjs/helmet/issues/454 --- CHANGELOG.md | 1 + middlewares/content-security-policy/index.ts | 49 ++++----- test/content-security-policy.test.ts | 108 +++++++------------ 3 files changed, 62 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eead06..dd7d7f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### 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) ### Removed 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 00dfe6f..897c6af 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", () => { From 2e5fb1b117e46989980e04cac7002be210bc9b81 Mon Sep 17 00:00:00 2001 From: Sohrab Chegini Date: Sun, 28 Apr 2024 18:03:52 +0330 Subject: [PATCH 5/7] HSTS: throw when misspelling "includeSubDomains" option See [#462] and [#464]. [#462]: https://github.com/helmetjs/helmet/issues/462 [#464]: https://github.com/helmetjs/helmet/pull/464 --- CHANGELOG.md | 1 + middlewares/strict-transport-security/index.ts | 2 +- test/strict-transport-security.test.ts | 9 +++------ 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dd7d7f3..c035acd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - **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:** `Strict-Transport-Security` now throws an error when "includeSubDomains" option is misspelled. This was previously a warning ### Removed diff --git a/middlewares/strict-transport-security/index.ts b/middlewares/strict-transport-security/index.ts index fa6e773..18c45cf 100644 --- a/middlewares/strict-transport-security/index.ts +++ b/middlewares/strict-transport-security/index.ts @@ -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/test/strict-transport-security.test.ts b/test/strict-transport-security.test.ts index dc1d780..73cf33f 100644 --- a/test/strict-transport-security.test.ts +++ b/test/strict-transport-security.test.ts @@ -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".)', ); }); From a603b0bebd82703fa970c125d031416ce3f820ba Mon Sep 17 00:00:00 2001 From: Sohrab Chegini Date: Mon, 29 Apr 2024 18:31:33 +0330 Subject: [PATCH 6/7] `getDefaultDirectives` should do a deep copy See [#463] and [#465]. [#463]: https://github.com/helmetjs/helmet/issues/463 [#465]: https://github.com/helmetjs/helmet/pull/465 --- CHANGELOG.md | 1 + middlewares/content-security-policy/index.ts | 2 +- test/content-security-policy.test.ts | 10 ++++++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c035acd..0b776e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - **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 diff --git a/middlewares/content-security-policy/index.ts b/middlewares/content-security-policy/index.ts index d336631..38cc436 100644 --- a/middlewares/content-security-policy/index.ts +++ b/middlewares/content-security-policy/index.ts @@ -68,7 +68,7 @@ const SHOULD_BE_QUOTED: ReadonlySet = new Set([ "wasm-unsafe-eval", ]); -const getDefaultDirectives = () => ({ ...DEFAULT_DIRECTIVES }); +const getDefaultDirectives = () => structuredClone(DEFAULT_DIRECTIVES); const dashify = (str: string): string => str.replace(/[A-Z]/g, (capitalLetter) => "-" + capitalLetter.toLowerCase()); diff --git a/test/content-security-policy.test.ts b/test/content-security-policy.test.ts index 897c6af..b4fc44f 100644 --- a/test/content-security-policy.test.ts +++ b/test/content-security-policy.test.ts @@ -581,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"); + }); }); From deb5569373f711cd4d0c9d7a607f20c390c62c2b Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Sat, 28 Sep 2024 22:24:10 +0000 Subject: [PATCH 7/7] CSP: speed up `getDefaultDirectives` I wrote a simple benchmarking script: import * as helmet from "./index.ts"; console.time("getting"); for (let i = 0; i < 1_000_000; i++) { helmet.contentSecurityPolicy.getDefaultDirectives(); } console.timeEnd("getting"); On my machine, this took about 4.5 seconds before the change. Now, it averages about 32 milliseconds. --- middlewares/content-security-policy/index.ts | 32 +++++++++----------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/middlewares/content-security-policy/index.ts b/middlewares/content-security-policy/index.ts index 38cc436..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 = () => structuredClone(DEFAULT_DIRECTIVES); +}); const dashify = (str: string): string => str.replace(/[A-Z]/g, (capitalLetter) => "-" + capitalLetter.toLowerCase());