Replicate a parsed feature policy representation so it doesn't need to be parsed in the browser pro…

third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp

Index: third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
diff --git a/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp b/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
index 51cabe384318c6df08f81d54e45eccead7a43177..19532863bf8303881bdba3b0a36889f8fe98719c 100644
--- a/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
+++ b/third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
@@ -27,69 +27,6 @@ const FeaturePolicy::Feature* featureForName(
   return nullptr;
 }
 
-// Converts a list of JSON feature policy items into a mapping of features to
-// whitelists. For future compatibility, unrecognized features are simply
-// ignored, as are unparseable origins. If |messages| is not null, then any
-// errors in the input will cause an error message to be appended to it.
-HashMap<const FeaturePolicy::Feature*,
-        std::unique_ptr<FeaturePolicy::Whitelist>>
-parseFeaturePolicyFromJson(std::unique_ptr<JSONArray> policyItems,
-                           RefPtr<SecurityOrigin> origin,
-                           FeaturePolicy::FeatureList& features,
-                           Vector<String>* messages) {
-  HashMap<const FeaturePolicy::Feature*,
-          std::unique_ptr<FeaturePolicy::Whitelist>>
-      whitelists;
-
-  for (size_t i = 0; i < policyItems->size(); ++i) {
-    JSONObject* item = JSONObject::cast(policyItems->at(i));
-    if (!item) {
-      if (messages)
-        messages->append("Policy is not an object");
-      continue;  // Array element is not an object; skip
-    }
-
-    for (size_t j = 0; j < item->size(); ++j) {
-      JSONObject::Entry entry = item->at(j);
-      String featureName = entry.first;
-      JSONArray* targets = JSONArray::cast(entry.second);
-      if (!targets) {
-        if (messages)
-          messages->append("Whitelist is not an array of strings.");
-        continue;
-      }
-
-      const FeaturePolicy::Feature* feature =
-          featureForName(featureName, features);
-      if (!feature)
-        continue;  // Feature is not recognized; skip
-
-      std::unique_ptr<FeaturePolicy::Whitelist> whitelist(
-          new FeaturePolicy::Whitelist);
-      String targetString;
-      for (size_t j = 0; j < targets->size(); ++j) {
-        if (targets->at(j)->asString(&targetString)) {
-          if (equalIgnoringCase(targetString, "self")) {
-            whitelist->add(origin);
-          } else if (targetString == "*") {
-            whitelist->addAll();
-          } else {
-            KURL originUrl = KURL(KURL(), targetString);
-            if (originUrl.isValid()) {
-              whitelist->add(SecurityOrigin::create(originUrl));
-            }
-          }
-        } else {
-          if (messages)
-            messages->append("Whitelist is not an array of strings.");
-        }
-      }
-      whitelists.set(feature, std::move(whitelist));
-    }
-  }
-  return whitelists;
-}
-
 }  // namespace
 
 // Definitions of all features controlled by Feature Policy should appear here.
@@ -195,20 +132,91 @@ std::unique_ptr<FeaturePolicy> FeaturePolicy::createFromParentPolicy(
                                 getDefaultFeatureList());
 }
 
-void FeaturePolicy::setHeaderPolicy(const String& policy,
-                                    Vector<String>* messages) {
-  DCHECK(m_headerWhitelists.isEmpty());
+// static
+WebVector<WebFeaturePolicy::ParsedWhitelist> FeaturePolicy::parseFeaturePolicy(

[iclelland, 2016/11/23 20:20:28]

As a new public interface, it would be good to have explicit coverage of this in
the unit tests. At least we could verify that the strings are pulled out
correctly, and that the right number of items are seen.

[raymes, 2016/11/30 06:25:17]

Done - I added a test.

+    const String& policy,
+    const SecurityOrigin* origin,
+    Vector<String>* messages) {
+  Vector<WebFeaturePolicy::ParsedWhitelist> whitelists;
+
+  if (origin->isUnique())

[iclelland, 2016/11/23 20:20:28]

This is a new check -- as far as I can tell, a unique origin would mean that
'self' is not resolvable to any specific origin. Is our only response in that
situation to reject the policy completely?

(And if we have to, then we should have a couple of tests for it -- a unit test
here, and maybe a browsertest where one frame has an opaque origin)

[raymes, 2016/11/30 06:25:17]

Yeah it is a new check. You're right - let me remove it for now because this CL
is already quite large. We can consider it more in followups.

+    return whitelists;
+
   // Use a reasonable parse depth limit; the actual maximum depth is only going
   // to be 4 for a valid policy, but we'll give the featurePolicyParser a chance
   // to report more specific errors, unless the string is really invalid.
-  std::unique_ptr<JSONArray> policyJSON = parseJSONHeader(policy, 50);
-  if (!policyJSON) {
+  std::unique_ptr<JSONArray> policyItems = parseJSONHeader(policy, 50);
+  if (!policyItems) {
     if (messages)
       messages->append("Unable to parse header");
-    return;
+    return whitelists;
+  }
+
+  for (size_t i = 0; i < policyItems->size(); ++i) {
+    JSONObject* item = JSONObject::cast(policyItems->at(i));
+    if (!item) {
+      if (messages)
+        messages->append("Policy is not an object");
+      continue;  // Array element is not an object; skip
+    }
+
+    for (size_t j = 0; j < item->size(); ++j) {
+      JSONObject::Entry entry = item->at(j);
+      String featureName = entry.first;
+      JSONArray* targets = JSONArray::cast(entry.second);
+      if (!targets) {
+        if (messages)
+          messages->append("Whitelist is not an array of strings.");
+        continue;
+      }
+
+      WebFeaturePolicy::ParsedWhitelist whitelist;
+      whitelist.featureName = featureName;
+      Vector<WebString> origins;
+      String targetString;
+      for (size_t j = 0; j < targets->size(); ++j) {
+        if (targets->at(j)->asString(&targetString)) {
+          if (equalIgnoringCase(targetString, "self")) {
+            origins.append(origin->toString());
+          } else if (targetString == "*") {
+            whitelist.matchesAllOrigins = true;
+          } else {
+            KURL originUrl = KURL(KURL(), targetString);
+            if (originUrl.isValid())

[iclelland, 2016/11/23 20:20:28]

This might as well be
if (KURL(KURL(), targetString).isValid())

but I'm wondering if there's a way to store the parsed origins here, instead of
the raw strings

[raymes, 2016/11/29 08:20:34]

Good point - we can probably use a WebSecurityOrigin and url::Origin? I've
changed it to use those.

+              origins.append(targetString);
+          }
+        } else {
+          if (messages)
+            messages->append("Whitelist is not an array of strings.");
+        }
+      }
+      whitelist.origins = origins;
+      whitelists.append(whitelist);
+    }
+  }
+  return whitelists;
+}
+
+void FeaturePolicy::setHeaderPolicy(
+    const WebVector<WebFeaturePolicy::ParsedWhitelist>& policy) {
+  DCHECK(m_headerWhitelists.isEmpty());
+  for (const WebFeaturePolicy::ParsedWhitelist& parsedWhitelist : policy) {

[iclelland, 2016/11/23 20:20:28]

This method seems like it's mostly a conversion function between
WebFeaturePolicy::ParsedWhiteList and FeaturePolicy::Whitelist. Would it look
cleaner to express it as such? Something like

//static
std::unique_ptr<Whitelist> Whitelist::from(const
WebFeaturePolicy::ParsedWhitelist&)

[raymes, 2016/11/29 08:20:34]

I haven't made this change yet (and my connection to corp is being really slow)
but it may be worth looking at the other changes to make sure they're ok :)

[raymes, 2016/11/30 06:25:17]

Done now.

+    const FeaturePolicy::Feature* feature =
+        featureForName(parsedWhitelist.featureName, m_features);
+    if (!feature)
+      continue;
+    std::unique_ptr<Whitelist> whitelist(new FeaturePolicy::Whitelist);
+    if (parsedWhitelist.matchesAllOrigins) {
+      whitelist->addAll();
+    } else {
+      for (const WebString& origin : parsedWhitelist.origins) {
+        KURL originUrl = KURL(KURL(), origin);
+        if (originUrl.isValid())
+          whitelist->add(SecurityOrigin::create(originUrl));
+      }
+    }
+    m_headerWhitelists.set(feature, std::move(whitelist));
   }
-  m_headerWhitelists = parseFeaturePolicyFromJson(
-      std::move(policyJSON), m_origin, m_features, messages);
 }
 
 bool FeaturePolicy::isFeatureEnabledForOrigin(