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

 // Copyright 2016 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 "platform/feature_policy/FeaturePolicy.h"
 
 #include "platform/json/JSONValues.h"
 #include "platform/network/HTTPParsers.h"
 #include "platform/weborigin/KURL.h"
 #include "platform/weborigin/SecurityOrigin.h"
 #include "wtf/PtrUtil.h"
 #include "wtf/text/StringBuilder.h"
 
 namespace blink {
 
 namespace {
 
 // Given a string name, return the matching feature struct, or nullptr if it is
 // not the name of a policy-controlled feature.
 const FeaturePolicy::Feature* featureForName(
     const String& featureName,
     FeaturePolicy::FeatureList& features) {
   for (const FeaturePolicy::Feature* feature : features) {
     if (featureName == feature->featureName)
       return feature;
   }
   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.
 const FeaturePolicy::Feature kDocumentCookie{
     "cookie", FeaturePolicy::FeatureDefault::EnableForAll};
 const FeaturePolicy::Feature kDocumentDomain{
     "domain", FeaturePolicy::FeatureDefault::EnableForAll};
 const FeaturePolicy::Feature kDocumentWrite{
     "docwrite", FeaturePolicy::FeatureDefault::EnableForAll};
 const FeaturePolicy::Feature kGeolocationFeature{
@@ skipping 85 matching lines @@
 }
 
 // static
 std::unique_ptr<FeaturePolicy> FeaturePolicy::createFromParentPolicy(
     const FeaturePolicy* parent,
     RefPtr<SecurityOrigin> currentOrigin) {
   return createFromParentPolicy(parent, std::move(currentOrigin),
                                 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;
   }
-  m_headerWhitelists = parseFeaturePolicyFromJson(
-      std::move(policyJSON), m_origin, m_features, messages);
+
+  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));
+  }
 }
 
 bool FeaturePolicy::isFeatureEnabledForOrigin(
     const FeaturePolicy::Feature& feature,
     const SecurityOrigin& origin) const {
   DCHECK(m_inheritedFeatures.contains(&feature));
   if (!m_inheritedFeatures.get(&feature)) {
     return false;
   }
   if (m_headerWhitelists.contains(&feature)) {
@@ skipping 36 matching lines @@
     sb.append("  ");
     sb.append(whitelist.key->featureName);
     sb.append(": ");
     sb.append(whitelist.value->toString());
     sb.append("\n");
   }
   return sb.toString();
 }
 
 }  // namespace blink