[FeaturePolicy] Initial implementation of Feature Policy

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/JSONParser.h"
+#include "platform/json/JSONValues.h"
+#include "platform/weborigin/KURL.h"
+#include "platform/weborigin/SecurityOrigin.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 FeaturePolicyFeature* featureForName(const String& featureName) {
+  for (const FeaturePolicyFeature* feature :
+       FeaturePolicy::getFeatureRegistry()) {
+    if (featureName == feature->featureName)
+      return feature;
+  }
+  return nullptr;
+}
+
+}  // namespace
+
+// Definitions of all features controlled by Feature Policy should appear here.
+const FeaturePolicyFeature kDocumentCookie{"cookie",
+                                           kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kDocumentDomain{"domain",
+                                           kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kDocumentWrite{"docwrite",
+                                          kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kGeolocationFeature{"geolocation",
+                                               kEnableFeatureForSelf};
+const FeaturePolicyFeature kMidiFeature{"midi", kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kNotificationsFeature{"notifications",
+                                                 kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kPaymentFeature{"payment", kEnableFeatureForSelf};
+const FeaturePolicyFeature kPushFeature{"push", kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kSyncScript{"sync-script",
+                                       kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kSyncXHR{"sync-xhr", kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kUsermedia{"usermedia", kEnableFeatureForAllOrigins};
+const FeaturePolicyFeature kVibrateFeature{"vibrate", kEnableFeatureForSelf};
+const FeaturePolicyFeature kWebRTC{"webrtc", kEnableFeatureForAllOrigins};
+
+FeaturePolicy::Whitelist::Whitelist() : m_matchesAllOrigins(false) {}
+
+void FeaturePolicy::Whitelist::addAll() {
+  m_matchesAllOrigins = true;
+}
+
+void FeaturePolicy::Whitelist::add(RefPtr<SecurityOrigin> origin) {
+  m_origins.append(origin);

[dcheng, 2016/10/14 20:33:17]

Nit: std::move()

[iclelland, 2016/10/19 12:51:55]

Done.

+}
+
+bool FeaturePolicy::Whitelist::contains(const SecurityOrigin* origin) const {

[dcheng, 2016/10/14 20:33:17]

Nit: const ref if we never expect this to be null.

[iclelland, 2016/10/19 12:51:55]

Done.

+  if (m_matchesAllOrigins)
+    return true;
+  for (const auto& targetOrigin : m_origins) {
+    if (targetOrigin->isSameSchemeHostPortAndSuborigin(origin))
+      return true;
+  }
+  return false;
+}
+
+String FeaturePolicy::Whitelist::toString() {
+  StringBuilder sb;
+  sb.append("[");
+  if (m_matchesAllOrigins) {
+    sb.append("*");
+  } else {
+    for (size_t i = 0; i < m_origins.size(); ++i) {
+      if (i > 0) {
+        sb.append(", ");
+      }
+      sb.append(m_origins[i]->toString());
+    }
+  }
+  sb.append("]");
+  return sb.toString();
+}
+
+// static
+Vector<const FeaturePolicyFeature*>& FeaturePolicy::getFeatureRegistry() {
+  DEFINE_STATIC_LOCAL(

[dcheng, 2016/10/14 20:33:17]

No need to use a Vector here; range-based for loops work over plain old arrays:

const FeaturePolicy kFeatureRegistry[] = {
  {"cookie", 32, kEnableFeatureForAllOrigins},
  {"domain", 34, kEnableFeatureForAllOrigins},
  ...
};

for (const FeaturePolicyFeature& feature : kFeatureRegistry) {
  ...
}

[iclelland, 2016/10/15 01:55:34]

It's mostly a vector so that it can be appended to in tests (see the constructor
for FeaturePolicyTest) -- there may be a better way, though. I'll see if I can
improve on that. Thanks.

+      Vector<const FeaturePolicyFeature*>, featureRegistry,
+      ({&kDocumentCookie, &kDocumentDomain, &kDocumentWrite,
+        &kGeolocationFeature, &kMidiFeature, &kNotificationsFeature,
+        &kPaymentFeature, &kPushFeature, &kSyncScript, &kSyncXHR, &kUsermedia,
+        &kVibrateFeature, &kWebRTC}));
+  return featureRegistry;
+}
+
+// static
+FeaturePolicy* FeaturePolicy::createFromParentPolicy(
+    const FeaturePolicy* parent,
+    RefPtr<SecurityOrigin> currentOrigin) {
+  FeaturePolicy* newPolicy = new FeaturePolicy(currentOrigin);
+  if (parent) {
+    for (const FeaturePolicyFeature* feature : getFeatureRegistry()) {
+      if (parent->isFeatureEnabledForOrigin(feature, currentOrigin.get())) {
+        newPolicy->m_inheritedFeatures.set(feature, true);
+      } else {
+        newPolicy->m_inheritedFeatures.set(feature, false);
+      }
+    }
+  }
+  return newPolicy;
+}
+
+void FeaturePolicy::addPolicyFromString(const String& policy) {

[raymes, 2016/10/18 02:42:31]

I don't like the "addPolicyFromString" name/approach because I feel it's too
general and so prone to be misused. Could we instead just make this specific and
say "setHeaderPolicy" or "enforceHeaderPolicy"? We can address how to factor in
meta policy as a next step.

[iclelland, 2016/10/19 12:51:55]

Done. Changed to setHeaderPolicy.

+  if (policy.isEmpty())
+    return;

[raymes, 2016/10/18 02:42:31]

Can we have a check:
DCHECK(m_declaredWhitelists.empty()) for now? I think it's good to verify that
this is only called once (I realize that would change with meta, but we can
address that separately :)

[iclelland, 2016/10/19 12:51:55]

Done.

+  for (const auto& whitelist : parse(policy)) {
+    if (isFeatureEnabled(whitelist.key)) {

[raymes, 2016/10/18 02:42:31]

I think this check is complex and hard to verify correctness because under the
hood it's looking at the current declared whitelist, but then ends up changing
it. I think it's not needed for now since we don't have meta policy yet and it's
clearer to see what's going on if we leave it out. That way the
declaredWhitelists matches what was specified in the header policy. If the
feature isn't enabled because of the inherited policy, it will be enforced
correctly in isFeatureEnabledForOrigin.

We can address adding in the meta policy separately (actually I think it's
better to define how to combine header/meta policy into a "declared policy" in
the algorithms doc first).

+      m_declaredWhitelists.set(whitelist.key, whitelist.value);

[raymes, 2016/10/18 02:42:30]

nit: Could we just call this m_headerWhitelists for now? When we add the logic
for meta we can generalize?

[iclelland, 2016/10/19 12:51:55]

Done.

+    }
+  }
+}
+
+bool FeaturePolicy::isFeatureEnabledForOrigin(
+    const FeaturePolicyFeature* feature,
+    const SecurityOrigin* origin) const {

[raymes, 2016/10/18 02:42:31]

It would be nice to link to an algorithm here but I don't think we have a
canonical link yet :)

[iclelland, 2016/10/19 12:51:55]

Acknowledged.

+  if (m_inheritedFeatures.contains(feature)) {

[raymes, 2016/10/18 02:42:31]

I think the behavior if a feature isn't in the map is unclear and so more bug
prone. Can we just initialize all the features in the constructor? If there is
no parent frame then we would initialize all the features to true for the top
level initialized policy. 

[iclelland, 2016/10/19 12:51:55]

Done. That matches the current text in the explainer as well.

+    if (!m_inheritedFeatures.get(feature)) {
+      return false;
+    }
+  }
+  if (m_declaredWhitelists.contains(feature)) {
+    return m_declaredWhitelists.get(feature)->contains(origin);
+  }
+  if (feature->defaultPolicy == kEnableFeatureForAllOrigins) {
+    return true;
+  }
+  if (feature->defaultPolicy == kEnableFeatureForSelf) {
+    return m_origin->isSameSchemeHostPortAndSuborigin(origin);
+  }
+  return false;
+}
+
+bool FeaturePolicy::isFeatureEnabled(
+    const FeaturePolicyFeature* feature) const {
+  return isFeatureEnabledForOrigin(feature, m_origin.get());
+}
+
+FeaturePolicy::FeaturePolicy(PassRefPtr<SecurityOrigin> currentOrigin)

[dcheng, 2016/10/14 20:33:17]

Nit: PassRefPtr is on the way out. Just use RefPtr here and std::move() into
m_origin.

[iclelland, 2016/10/19 12:51:55]

Done.

+    : m_origin(currentOrigin) {}
+
+// Parses a feature policy string into a mapping of features to whitelists. The
+// parse algorithm is deliberately very lenient, and will return as much as it
+// can recognize. Unrecognized features are simply ignored, as are invalid
+// policy items and invalid origins inside of whitelists.
+HeapHashMap<const FeaturePolicyFeature*, Member<FeaturePolicy::Whitelist>>
+FeaturePolicy::parse(const String& policy) {

[raymes, 2016/10/18 02:42:31]

I think parsing code tends to fit better not as a member function. Could we
instead just make this a standalone function and pass the origin of the policy
owner in as a parameter?

[iclelland, 2016/10/19 12:51:55]

Done. Renamed and moved to the anonymous namespace.

+  HeapHashMap<const FeaturePolicyFeature*, Member<Whitelist>> whitelists;
+  std::unique_ptr<JSONValue> policyJSON = parseJSON(policy);
+
+  if (!policyJSON)
+    return whitelists;  // Policy string is not valid JSON
+
+  std::unique_ptr<JSONArray> items = JSONArray::cast(std::move(policyJSON));
+  if (!items)
+    return whitelists;  // Policy string is not an array
+
+  for (size_t i = 0; i < items->size(); ++i) {
+    JSONObject* item = JSONObject::cast(items->at(i));
+    if (!item)
+      continue;  // Array element is not an object; skip

[raymes, 2016/10/18 02:42:31]

Hmm, should we just give up at each of these failure points? I'm open to other
alternatives though. I think we should probably display a console error in each
case if possible though - should we return an error string from the function?

[iclelland, 2016/10/19 12:51:55]

We can't fail at "Feature not recognized", as that would break compatibility
with future iterations of the spec, and not allow other browsers to experiment
with policies on features we don't support.
Similarly, I'm not sure that we should fail because we don't recognize an
origin; there may be legitimate reasons for another browser in a different
environment to use hostnames that we don't currently understand, and I'd like to
just pass over them, knowing that Chrome code will never need to test against
them.

Happy to fail on structural problems, though -- if you don't use JSON, or don't
use the right JSON structure, then something is badly wrong with the policy, and
maybe we shouldn't allow it at all. (With a console error if possible).

(I still need to refactor this to return an error in that case, then)

[raymes, 2016/10/19 23:47:36]

That all sounds good to me :) I'm happy if you want to do this in a followup CL.
Perhaps add a TODO (or file a bug)?

+
+    for (size_t j = 0; j < item->size(); ++j) {
+      JSONObject::Entry entry = item->at(j);
+      String featureName = entry.first;
+      const FeaturePolicyFeature* feature = featureForName(featureName);
+      if (!feature)
+        continue;  // Feature is not recognized; skip
+
+      JSONArray* targets = JSONArray::cast(entry.second);
+      if (!targets)
+        continue;  // Whitelist is not an array; skip
+
+      Whitelist* whitelist = new Whitelist;
+      String targetString;
+      for (size_t j = 0; j < targets->size(); ++j) {
+        if (targets->at(j)->asString(&targetString)) {
+          if (equalIgnoringCase(targetString, "self")) {
+            whitelist->add(m_origin);
+          } else if (targetString == "*") {
+            whitelist->addAll();
+          } else {
+            KURL originUrl = KURL(KURL(), targetString);
+            if (originUrl.isValid()) {
+              whitelist->add(SecurityOrigin::create(originUrl));
+            }
+          }
+        }
+      }
+      whitelists.set(feature, whitelist);
+    }
+  }
+  return whitelists;
+}
+
+String FeaturePolicy::toString() {
+  StringBuilder sb;
+  sb.append("Feature Policy for frame in origin: ");
+  sb.append(m_origin->toString());
+  sb.append("\n");
+  sb.append("Inherited features:\n");
+  for (const auto& inheritedFeature : m_inheritedFeatures) {

[raymes, 2016/10/18 02:42:30]

nit, optional: should we indent the string here for easy reading?

[iclelland, 2016/10/19 12:51:55]

Done. This code is currently just so I can log the state of the policy objects;
I don't know if it will survive into the final product.

+    sb.append(inheritedFeature.key->featureName);
+    sb.append(": ");
+    sb.append(inheritedFeature.value ? "true" : "false");
+    sb.append("\n");
+  }
+  sb.append("Declared whitelists:\n");
+  for (const auto& whitelist : m_declaredWhitelists) {

[raymes, 2016/10/18 02:42:31]

nit: same here

[iclelland, 2016/10/19 12:51:55]

Done.

+    sb.append(whitelist.key->featureName);
+    sb.append(": ");
+    sb.append(whitelist.value->toString());
+    sb.append("\n");
+  }
+  return sb.toString();
+}
+
+DEFINE_TRACE(FeaturePolicy) {
+  visitor->trace(m_declaredWhitelists);
+}
+
+}  // namespace blink