Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v8.0.0 #453

Merged
merged 7 commits into from
Sep 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
86 changes: 36 additions & 50 deletions middlewares/content-security-policy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,22 @@ interface ContentSecurityPolicy {

const dangerouslyDisableDefaultSrc = Symbol("dangerouslyDisableDefaultSrc");

const DEFAULT_DIRECTIVES: Record<
const SHOULD_BE_QUOTED: ReadonlySet<string> = 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<ContentSecurityPolicyDirectiveValue>
> = {
> => ({
"default-src": ["'self'"],
"base-uri": ["'self'"],
"font-src": ["'self'", "https:", "data:"],
Expand All @@ -54,47 +66,27 @@ const DEFAULT_DIRECTIVES: Record<
"script-src-attr": ["'none'"],
"style-src": ["'self'", "https:", "'unsafe-inline'"],
"upgrade-insecure-requests": [],
};

const SHOULD_BE_QUOTED: ReadonlySet<string> = 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());

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<object>, 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<ContentSecurityPolicyOptions>,
Expand All @@ -109,7 +101,7 @@ function normalizeDirectives(
const directivesExplicitlyDisabled = new Set<string>();

for (const rawDirectiveName in rawDirectives) {
if (!has(rawDirectives, rawDirectiveName)) {
if (!Object.hasOwn(rawDirectives, rawDirectiveName)) {
continue;
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -219,15 +208,16 @@ function getHeaderValue(
res: ServerResponse,
normalizedDirectives: Readonly<NormalizedDirectives>,
): 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;
Expand All @@ -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 =
Expand Down
4 changes: 2 additions & 2 deletions middlewares/strict-transport-security/index.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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".)',
);
}
Expand Down
3 changes: 1 addition & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,6 @@
},
"type": "module",
"engines": {
"node": ">=16.0.0"
"node": ">=18.0.0"
}
}
118 changes: 51 additions & 67 deletions test/content-security-policy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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", () => {
Expand Down Expand Up @@ -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<string>).push("ignored.example");

const two = getDefaultDirectives();
expect(two).not.toHaveProperty("worker-src");
expect(two["img-src"]).not.toContain("ignored.example");
});
});
2 changes: 1 addition & 1 deletion test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading