Content-Security-Policy: better error when value should be quoted

It's easy to forget to quote directive value entries like `'self'` and
`'none'`. Someone [recently ran into this][0] and was confused by the
error message.

This makes the error message clearer by telling you that something needs
to be quoted.

```diff
-...invalid directive value for "img-src"
+...invalid directive value for "img-src". "self" should be quoted
```

[0]: #482

Author: EvanHahn

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## Unreleased
+
+### Changed
+
+- `Content-Security-Policy` gives a better error when a directive value, like `self`, should be quoted. See [#482](https://github.com/helmetjs/helmet/issues/482)
+
 ## 8.0.0
 
 ### Changed

middlewares/content-security-policy/index.ts

@@ -71,22 +71,37 @@ const getDefaultDirectives = (): Record<
 const dashify = (str: string): string =>
   str.replace(/[A-Z]/g, (capitalLetter) => "-" + capitalLetter.toLowerCase());
 
-const isDirectiveValueInvalid = (directiveValue: string): boolean =>
-  /;|,/.test(directiveValue);
-
-const isDirectiveValueEntryInvalid = (directiveValueEntry: string): boolean =>
-  SHOULD_BE_QUOTED.has(directiveValueEntry) ||
-  directiveValueEntry.startsWith("nonce-") ||
-  directiveValueEntry.startsWith("sha256-") ||
-  directiveValueEntry.startsWith("sha384-") ||
-  directiveValueEntry.startsWith("sha512-");
-
-const invalidDirectiveValueError = (directiveName: string): Error =>
-  new Error(
-    `Content-Security-Policy received an invalid directive value for ${JSON.stringify(
-      directiveName,
-    )}`,
-  );
+const assertDirectiveValueIsValid = (
+  directiveName: string,
+  directiveValue: string,
+): void => {
+  if (/;|,/.test(directiveValue)) {
+    throw new Error(
+      `Content-Security-Policy received an invalid directive value for ${JSON.stringify(
+        directiveName,
+      )}`,
+    );
+  }
+};
+
+const assertDirectiveValueEntryIsValid = (
+  directiveName: string,
+  directiveValueEntry: string,
+): void => {
+  if (
+    SHOULD_BE_QUOTED.has(directiveValueEntry) ||
+    directiveValueEntry.startsWith("nonce-") ||
+    directiveValueEntry.startsWith("sha256-") ||
+    directiveValueEntry.startsWith("sha384-") ||
+    directiveValueEntry.startsWith("sha512-")
+  ) {
+    throw new Error(
+      `Content-Security-Policy received an invalid directive value for ${JSON.stringify(
+        directiveName,
+      )}. ${JSON.stringify(directiveValueEntry)} should be quoted`,
+    );
+  }
+};
 
 function normalizeDirectives(
   options: Readonly<ContentSecurityPolicyOptions>,
@@ -161,13 +176,9 @@ function normalizeDirectives(
     }
 
     for (const element of directiveValue) {
-      if (
-        typeof element === "string" &&
-        (isDirectiveValueInvalid(element) ||
-          isDirectiveValueEntryInvalid(element))
-      ) {
-        throw invalidDirectiveValueError(directiveName);
-      }
+      if (typeof element !== "string") continue;
+      assertDirectiveValueIsValid(directiveName, element);
+      assertDirectiveValueEntryIsValid(directiveName, element);
     }
 
     result.set(directiveName, directiveValue);
@@ -215,21 +226,18 @@ function getHeaderValue(
     for (const element of rawDirectiveValue) {
       if (typeof element === "function") {
         const newElement = element(req, res);
-        if (isDirectiveValueEntryInvalid(newElement)) {
-          return invalidDirectiveValueError(directiveName);
-        }
+        assertDirectiveValueEntryIsValid(directiveName, newElement);
         directiveValue += " " + newElement;
       } else {
         directiveValue += " " + element;
       }
     }
 
-    if (!directiveValue) {
-      result.push(directiveName);
-    } else if (isDirectiveValueInvalid(directiveValue)) {
-      return invalidDirectiveValueError(directiveName);
-    } else {
+    if (directiveValue) {
+      assertDirectiveValueIsValid(directiveName, directiveValue);
       result.push(`${directiveName}${directiveValue}`);
+    } else {
+      result.push(directiveName);
     }
   }
 

test/content-security-policy.test.ts

@@ -373,13 +373,7 @@ describe("Content-Security-Policy middleware", () => {
   });
 
   it("throws if any directive values are invalid", () => {
-    const invalidValues = [
-      ";",
-      ",",
-      "hello;world",
-      "hello,world",
-      ...shouldBeQuoted,
-    ];
+    const invalidValues = [";", ",", "hello;world", "hello,world"];
     for (const invalidValue of invalidValues) {
       assert.throws(
         () => {
@@ -397,6 +391,23 @@ describe("Content-Security-Policy middleware", () => {
         },
       );
     }
+
+    for (const invalidDirectiveEntry of shouldBeQuoted) {
+      assert.throws(
+        () => {
+          contentSecurityPolicy({
+            useDefaults: false,
+            directives: {
+              "default-src": "'self'",
+              "something-else": [invalidDirectiveEntry],
+            },
+          });
+        },
+        {
+          message: `Content-Security-Policy received an invalid directive value for "something-else". "${invalidDirectiveEntry}" should be quoted`,
+        },
+      );
+    }
   });
 
   it("errors if any directive values are invalid when a function returns", async () => {
@@ -421,19 +432,14 @@ describe("Content-Security-Policy middleware", () => {
               // 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 isOk = err.message.startsWith(
+                'Content-Security-Policy received an invalid directive value for "default-src"',
               );
+              res.end(JSON.stringify(isOk));
             },
           );
 
-        await supertest(app).get("/").expect(500, {
-          helmetTestError: true,
-          message:
-            'Content-Security-Policy received an invalid directive value for "default-src"',
-        });
+        await supertest(app).get("/").expect(200, "true");
       }),
     );
   });