Skip to content

Commit

Permalink
Content-Security-Policy: throw if directive value lacks necessary quotes
Browse files Browse the repository at this point in the history
Closes [#454].

[#454]: #454
  • Loading branch information
EvanHahn committed Apr 27, 2024
1 parent 8f07435 commit 7b94a6c
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 96 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
49 changes: 20 additions & 29 deletions middlewares/content-security-policy/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<ContentSecurityPolicyOptions>,
Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -216,15 +210,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 @@ -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 =
Expand Down
108 changes: 41 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

0 comments on commit 7b94a6c

Please sign in to comment.