Content-Security-Policy: warn if directive value lacks necessary quotes

helmetjs · Apr 27, 2024 · 6475da1 · 6475da1
1 parent 892ed40
commit 6475da1
Show file tree

Hide file tree

Showing 3 changed files with 104 additions and 8 deletions.
diff --git 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
@@ -56,6 +56,18 @@ const DEFAULT_DIRECTIVES: Record<
   "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 =>
@@ -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<object>, 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
@@ -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({