fix(csp): allow duplicate report-* directives (#151)

* Added duplicate report-* directives detection

* addressed some nits

Author: argl

scan

@@ -7,6 +7,8 @@ const DIRECTIVES_DISALLOWED_IN_META = [
   "report-uri",
   "sandbox",
 ];
+const ALLOWED_DUPLICATE_KEYS = new Set(["report-uri", "report-to"]);
+export const DUPLICATE_WARNINGS_KEY = "_observatory_duplicate_key_warnings";
 
 /**
  * Parse CSP from meta tags, weeding out directives
@@ -24,6 +26,10 @@ export function parseCspMeta(cspList) {
 }
 
 /**
+ * The returned Map has the directive as the key and a Set of sources as the value.
+ * If there are allowed duplicates detected, the first one is kept and the rest are discarded,
+ * and an entry in the final Map is added with the key "_observatory_duplicate_key_warnings"
+ * and the directive's name as the value.
  *
  * @param {string[]} cspList
  * @returns {Map<string, Set<string>>}
@@ -44,6 +50,8 @@ export function parseCsp(cspList) {
 
   /**  @type {Map<string, {source: string, index: number, keep: boolean}[]>} */
   const csp = new Map();
+  /**  @type {Set<string>} */
+  const duplicate_warnings = new Set();
 
   for (const [policyIndex, policy] of cleanCspList.entries()) {
     const directiveSeenBeforeThisPolicy = new Set();
@@ -59,11 +67,16 @@ export function parseCsp(cspList) {
       const directive = directiveEntry.toLowerCase();
 
       // While technically valid in that you just use the first entry, we are saying that repeated
-      // directives are invalid so that people notice it
+      // directives are invalid so that people notice it. The exception are duplicate report-uri
+      // and report-to directives, which we allow.
       if (directiveSeenBeforeThisPolicy.has(directive)) {
-        throw new Error(
-          `Duplicate directive ${directive} in policy ${policyIndex}`
-        );
+        if (ALLOWED_DUPLICATE_KEYS.has(directive)) {
+          duplicate_warnings.add(directive);
+        } else {
+          throw new Error(
+            `Duplicate directive ${directive} in policy ${policyIndex}`
+          );
+        }
       } else {
         directiveSeenBeforeThisPolicy.add(directive);
       }
@@ -146,7 +159,9 @@ export function parseCsp(cspList) {
         : new Set(["'none'"]),
     ])
   );
-
+  if (duplicate_warnings.size) {
+    finalCsp.set(DUPLICATE_WARNINGS_KEY, duplicate_warnings);
+  }
   return finalCsp;
 }
 

src/analyzer/cspParser.js

@@ -4,7 +4,11 @@ import {
 } from "../../headers.js";
 import { Requests, Policy, BaseOutput } from "../../types.js";
 import { Expectation } from "../../types.js";
-import { parseCsp, parseCspMeta } from "../cspParser.js";
+import {
+  DUPLICATE_WARNINGS_KEY,
+  parseCsp,
+  parseCspMeta,
+} from "../cspParser.js";
 import { getHttpHeaders } from "../utils.js";
 
 const DANGEROUSLY_BROAD = new Set([
@@ -47,6 +51,7 @@ export class CspOutput extends BaseOutput {
     Expectation.CspImplementedWithUnsafeEval,
     Expectation.CspImplementedWithUnsafeInline,
     Expectation.CspImplementedWithInsecureScheme,
+    Expectation.CspImplementedButDuplicateDirectives,
     Expectation.CspHeaderInvalid,
     Expectation.CspNotImplemented,
     Expectation.CspNotImplementedButReportingEnabled,
@@ -301,6 +306,7 @@ export function contentSecurityPolicyTest(
   );
 
   // Check to see if the test passed or failed
+  // If it passed, report any duplicate report-uri/report-to directives
   if (
     [
       expectation,
@@ -310,10 +316,17 @@ export function contentSecurityPolicyTest(
     ].includes(output.result)
   ) {
     output.pass = true;
+    if (csp.has(DUPLICATE_WARNINGS_KEY)) {
+      output.result = Expectation.CspImplementedButDuplicateDirectives;
+    }
   }
 
   output.data = {};
   for (const [key, value] of csp) {
+    // filter out the duplicate warnings key from the parsed CSP
+    if (key === DUPLICATE_WARNINGS_KEY) {
+      continue;
+    }
     output.data[key] = [...value].sort();
   }
 

src/analyzer/tests/csp.js

@@ -214,6 +214,16 @@ export const SCORE_TABLE = new Map([
       </p>`,
     },
   ],
+  [
+    Expectation.CspImplementedButDuplicateDirectives,
+    {
+      description: `<p>
+        Content Security Policy (CSP) implemented, but contains duplicate directives.
+        </p>`,
+      modifier: 0,
+      recommendation: `<p>Remove duplicate directives from the CSP</p>`,
+    },
+  ],
 
   // Cookies
   [

src/grader/charts.js

@@ -106,6 +106,8 @@ export const Expectation = {
   CspNotImplemented: "csp-not-implemented",
   CspNotImplementedButReportingEnabled:
     "csp-not-implemented-but-reporting-enabled",
+  CspImplementedButDuplicateDirectives:
+    "csp-implemented-but-duplicate-directives",
 
   // SUBRESOURCE INTEGRITY
 

src/types.js

@@ -196,11 +196,28 @@ describe("Content Security Policy Parser", function () {
     }
   });
 
-  it("should parse this header correctly", function () {
+  it("should parse this example header correctly", function () {
     let policy = [
       "default-src 'self' blob: https://*.cnn.com:* http://*.cnn.com:* *.cnn.io:* *.cnn.net:* *.turner.com:* *.turner.io:* *.ugdturner.com:* courageousstudio.com *.vgtf.net:*; script-src 'unsafe-eval' 'unsafe-inline' 'self' *; style-src 'unsafe-inline' 'self' blob: *; child-src 'self' blob: *; frame-src 'self' *; object-src 'self' *; img-src 'self' data: blob: *; media-src 'self' data: blob: *; font-src 'self' data: *; connect-src 'self' data: *; frame-ancestors 'self' https://*.cnn.com:* http://*.cnn.com https://*.cnn.io:* http://*.cnn.io:* *.turner.com:* courageousstudio.com;",
     ];
     const res = parseCsp(policy);
     assert(res);
   });
+
+  it("should parse a policy with duplicate report-uri entries and report those duplicates", function () {
+    let policy = [
+      "report-uri https://www.dropbox.com/csp_log?policy_name=metaserver-whitelist ; report-uri https://www.dropbox.com/csp_log?policy_name=metaserver-dynamic",
+    ];
+    const res = parseCsp(policy);
+    assert(res);
+    assert(res.has("_observatory_duplicate_key_warnings"));
+    assert(res.get("_observatory_duplicate_key_warnings")?.has("report-uri"));
+  });
+  it("should parse a policy with duplicate report-to entries and report those duplicates", function () {
+    let policy = ["report-to some_name ; report-to some_other_name"];
+    const res = parseCsp(policy);
+    assert(res);
+    assert(res.has("_observatory_duplicate_key_warnings"));
+    assert(res.get("_observatory_duplicate_key_warnings")?.has("report-to"));
+  });
 });