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

Closes [#454].

[#454]: #454

Author: EvanHahn

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
 

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<ContentSecurityPolicyOptions>,
@@ -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<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;
@@ -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 =

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", () => {