Part 3.1: Is policy list subsumed under subsuming policy?

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/frame/csp/CSPDirectiveList.h"
 
 #include "bindings/core/v8/SourceLocation.h"
 #include "core/dom/Document.h"
 #include "core/dom/SecurityContext.h"
 #include "core/dom/SpaceSplitString.h"
@@ skipping 1124 matching lines @@
                                ContentSecurityPolicy::TreatAsPublicAddress)) {
     treatAsPublicAddress(name, value);
   } else if (equalIgnoringCase(name, ContentSecurityPolicy::RequireSRIFor) &&
              m_policy->experimentalFeaturesEnabled()) {
     parseRequireSRIFor(name, value);
   } else {
     m_policy->reportUnsupportedDirective(name);
   }
 }
 
+SourceListDirectiveVector CSPDirectiveList::getSourceList(
+    const char* name,
+    CSPDirectiveListVector policies) {
+  SourceListDirectiveVector sourceListDirectives;
+  if (name == ContentSecurityPolicy::ScriptSrc) {

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

Perhaps you could put the loop on the outside of all these `if`s? That is:

```
for (const auto& policy : policies) {
  if (name == ContentSecurityPolicy::FrameSrc) {

  } else if (...) {

  }
}
```

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

I thought it would be better to first get into the right branch based on the
name and then knowing that iterate over policies because otherwise, it would be
less efficient to check name on each iteration.

But let me know if it would be better to put the loop outside for readability
purposes?

[Mike West, 2016/11/17 11:25:15]

We're expecting sites to have some small number of policies; I don't think the
additional work would amount to much.
It seems to me to be easier to read a single `for` loop wrapping a big `if` than
to read a big `if` wrapping many `for` loops. I won't insist on it if you prefer
the current structure, but I think I'd prefer that way of doing things.

+    for (const auto& policy : policies) {
+      if (policy->m_scriptSrc.get()) {
+        sourceListDirectives.append(policy->m_scriptSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }

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

This could be compressed to something like:

```
if (SourceListDirective* directive = operativeDirective(m_scriptSrc.get()))
  sourceListDirectives.append(*directive);
```

+    }
+  } else if (name == ContentSecurityPolicy::ObjectSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_objectSrc.get()) {
+        sourceListDirectives.append(policy->m_objectSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::FrameSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_frameSrc.get()) {
+        sourceListDirectives.append(policy->m_frameSrc);
+      } else if (policy->m_childSrc.get()) {
+        sourceListDirectives.append(policy->m_childSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::ImgSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_imgSrc.get()) {
+        sourceListDirectives.append(policy->m_imgSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::StyleSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_styleSrc.get()) {
+        sourceListDirectives.append(policy->m_styleSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::FontSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_fontSrc.get()) {
+        sourceListDirectives.append(policy->m_fontSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::MediaSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_mediaSrc.get()) {
+        sourceListDirectives.append(policy->m_mediaSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::ConnectSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_connectSrc.get()) {
+        sourceListDirectives.append(policy->m_connectSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::ChildSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_childSrc.get()) {
+        sourceListDirectives.append(policy->m_childSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::ManifestSrc) {
+    for (const auto& policy : policies) {
+      if (policy->m_manifestSrc.get()) {
+        sourceListDirectives.append(policy->m_manifestSrc);
+      } else if (policy->m_defaultSrc.get()) {
+        sourceListDirectives.append(policy->m_defaultSrc);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::FrameAncestors) {
+    for (const auto& policy : policies) {
+      if (policy->m_frameAncestors.get()) {
+        sourceListDirectives.append(policy->m_frameAncestors);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::BaseURI) {
+    for (const auto& policy : policies) {
+      if (policy->m_baseURI.get()) {
+        sourceListDirectives.append(policy->m_baseURI);
+      }
+    }
+  } else if (name == ContentSecurityPolicy::FormAction) {
+    for (const auto& policy : policies) {
+      if (policy->m_formAction.get()) {
+        sourceListDirectives.append(policy->m_formAction);
+      }
+    }
+  }
+  return sourceListDirectives;
+}
+
+bool CSPDirectiveList::subsumes(CSPDirectiveListVector other) {
+  const char* directives[] = {
+      // Fetch Directives
+      ContentSecurityPolicy::ChildSrc, ContentSecurityPolicy::ConnectSrc,
+      ContentSecurityPolicy::FontSrc, ContentSecurityPolicy::FrameSrc,
+      ContentSecurityPolicy::ImgSrc, ContentSecurityPolicy::ManifestSrc,
+      ContentSecurityPolicy::MediaSrc, ContentSecurityPolicy::ObjectSrc,
+      ContentSecurityPolicy::ScriptSrc, ContentSecurityPolicy::StyleSrc,
+      // Document Directives
+      ContentSecurityPolicy::BaseURI,
+      // Navigation Directives
+      ContentSecurityPolicy::FrameAncestors, ContentSecurityPolicy::FormAction};
+
+  for (const auto& directive : directives) {
+    // There should only be one SourceListDirective for each dirctive in
+    // Embedding-CSP.
+    SourceListDirectiveVector requiredList =
+        getSourceList(directive, CSPDirectiveListVector(1, this));
+    if (requiredList.size() == 0)
+      continue;
+    SourceListDirective* required = requiredList[0];
+    // Aggregate all serialized source lists of the returned CSP into a vector
+    // based on a directive type, defaulting accordingly (for example, to
+    // `default-src`)
+    SourceListDirectiveVector returned = getSourceList(directive, other);
+    // TODO(amalika): Add checks for plugin-types, sandbox, disown-opener,
+    // navigation-to, worker-src.
+    if (!required->subsumes(returned))
+      return false;
+  }
+
+  return true;
+}
+
 DEFINE_TRACE(CSPDirectiveList) {
   visitor->trace(m_policy);
   visitor->trace(m_pluginTypes);
   visitor->trace(m_baseURI);
   visitor->trace(m_childSrc);
   visitor->trace(m_connectSrc);
   visitor->trace(m_defaultSrc);
   visitor->trace(m_fontSrc);
   visitor->trace(m_formAction);
   visitor->trace(m_frameAncestors);
   visitor->trace(m_frameSrc);
   visitor->trace(m_imgSrc);
   visitor->trace(m_mediaSrc);
   visitor->trace(m_manifestSrc);
   visitor->trace(m_objectSrc);
   visitor->trace(m_scriptSrc);
   visitor->trace(m_styleSrc);
   visitor->trace(m_workerSrc);
 }
 
 }  // namespace blink