Part 3.1: Is policy list subsumed under subsuming policy?

third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp

Index: third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
index 2fcde837e88b691ee4073c580e1ce9c8423735d5..4708195c7a922ecf452b7b027affa836313fa257 100644
--- a/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/CSPDirectiveListTest.cpp
@@ -435,4 +435,65 @@ TEST_F(CSPDirectiveListTest, workerSrc) {
   }
 }
 
+TEST_F(CSPDirectiveListTest, SubsumesBasedOnCSPSourcesOnly) {
+  struct TestCase {
+    const std::vector<const char*> policies;
+    bool expected;
+  } cases[] = {
+      // The lists, which are at least as restrictive as A, are subsumed.

[Mike West, 2016/11/14 14:21:23]

This test says that they're not subsumed (`false` all the way through)? Is it
possible that you implemented `isSubsumedBy`?

+      {{""}, false},
+      {{"script-src http://example.com"}, false},
+      {{"img-src http://example.com"}, false},
+      {{"script-src http://*.one.com"}, false},
+      {{"img-src https://one.com http://two.com/imgs/"}, false},
+      {{"default-src http://example.com"}, false},
+      {{"default-src https://one.com http://two.com/imgs/"}, false},
+      {{"default-src http://one.com"}, false},
+      {{"script-src http://*.one.com; img-src http://two.com/"}, false},
+      {{"script-src http://*.one.com", "img-src http://one.com"}, false},
+      {{"script-src http://*.one.com", "script-src https://two.com"}, false},
+      {{"script-src http://*.random.com", "script-src https://random.com"},
+       false},
+      {{"script-src http://one.com", "script-src https://random.com"}, false},
+      {{"script-src http://*.random.com; default-src http://one.com "
+        "http://two.com/imgs/",
+        "default-src https://random.com"},
+       false},
+      // The lists, which are not as restrictive as A, are not subsumed.
+      {{"default-src https://one.com"}, true},
+      {{"default-src http://random.com",
+        "default-src https://non-random.com:*"},
+       true},
+      {{"script-src http://*.one.com; img-src https://one.com"}, true},
+      {{"script-src http://*.one.com; img-src https://one.com "
+        "http://two.com/imgs/"},
+       true},
+      {{"script-src http://*.one.com",
+        "img-src https://one.com http://two.com/imgs/"},
+       true},
+      {{"script-src http://*.random.com; default-src https://one.com "
+        "http://two.com/imgs/",
+        "default-src https://else.com"},
+       true},
+      {{"script-src http://*.random.com; default-src https://one.com "
+        "http://two.com/imgs/",
+        "default-src https://one.com"},
+       true},
+  };
+
+  Member<CSPDirectiveList> A = createList(
+      "script-src http://*.one.com; img-src https://one.com "
+      "http://two.com/imgs/",
+      ContentSecurityPolicyHeaderTypeReport);

[Mike West, 2016/11/14 14:21:23]

I think we'll have more confidence in the final result if we test against a
wider variety of policies. For example, does an empty |A| subsume things? What
about an |A| that only defines `script-src`? What about a directive that doesn't
appear in the policy like `font-src`? What about keyword sources? What about
'*'? What about 'none'? etc.

[amalika, 2016/11/15 13:17:18]

Since we did not implement keywords yet, I did not add any tests but made sure
to name this one `SubsumesBasedOnCSPSourcesOnly`. It would be then important to
check policies with intermix of keywords + CSPSources.

But I added reversed testing: to check if the first policy from listB subsumes A
and GetSourceList test.

+
+  for (const auto& test : cases) {
+    HeapVector<Member<CSPDirectiveList>> listB;
+    for (const auto& policy : test.policies) {
+      listB.append(createList(policy, ContentSecurityPolicyHeaderTypeReport));
+    }
+
+    EXPECT_EQ(test.expected, A->subsumes(listB));
+  }
+}

[Mike West, 2016/11/14 14:21:23]

I'd like to see more test coverage. You don't explicitly test `getSources` for
example, and you don't test any all of the cascading behaviors (e.g. `frame-src`
-> `child-src` -> `default-src` vs. `script-src` -> `default-src` vs.
`frame-ancestors`).

+
 }  // namespace blink