Origin policy: update feature policy parsing to match the latest spec

This updates the parsing of the feature policy parts of the origin
policy manifest to mostly match the latest spec draft at
https://wicg.github.io/origin-policy/, in particular
https://wicg.github.io/origin-policy/#parsing. That is, it moves away
from "feature-policy": ["... FP string"] to
"features": { "policy": "... FP string" }. This changes the data model
from a list of FP strings to an optional FP string. Additionally, it
removes the failure on parsing errors, as those are no longer in the
spec.

This does not yet properly parse the FP string as a FP directive;
instead it still treats it as a header (so, commas are allowed inside).
A failing test is added for that case, which will be addressed in a
followup CL.

Bug: 751996
Change-Id: I51711ee9381ecfc705683ba0eb870e461fed434e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1965905
Commit-Queue: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Daniel Vogelheim <vogelheim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726494}

content/renderer/render_frame_impl.cc

@@ -1029,9 +1029,11 @@ void FillNavigationParamsOriginPolicy(
   if (head.origin_policy.has_value() && head.origin_policy.value().contents) {
     navigation_params->origin_policy = blink::WebOriginPolicy();
 
-    for (const auto& feature : head.origin_policy.value().contents->features) {
-      navigation_params->origin_policy->features.emplace_back(
-          WebString::FromUTF8(feature));
+    const base::Optional<std::string>& feature_policy =
+        head.origin_policy.value().contents->feature_policy;
+    if (feature_policy) {
+      navigation_params->origin_policy->feature_policy =
+          WebString::FromUTF8(*feature_policy);
     }
 
     for (const auto& csp :

services/network/origin_policy/origin_policy_parser.cc

@@ -40,10 +40,12 @@ bool OriginPolicyParser::DoParse(base::StringPiece policy_contents_text) {
   base::Value* csp = json->FindListKey("content-security-policy");
   bool csp_ok = !csp || ParseContentSecurityPolicies(*csp);
 
-  base::Value* features = json->FindListKey("feature-policy");
-  bool features_ok = !features || ParseFeaturePolicies(*features);
+  base::Value* features = json->FindDictKey("features");
+  if (features) {
+    ParseFeatures(*features);
+  }
 
-  return csp_ok && features_ok;
+  return csp_ok;
 }
 
 bool OriginPolicyParser::ParseContentSecurityPolicies(
@@ -71,17 +73,12 @@ bool OriginPolicyParser::ParseContentSecurityPolicy(const base::Value& csp) {
   return true;
 }
 
-bool OriginPolicyParser::ParseFeaturePolicies(const base::Value& policies) {
-  bool ok = true;
-  for (const auto& feature : policies.GetList()) {
-    ok &= feature.is_string() && ParseFeaturePolicy(feature);
-  }
-  return ok;
-}
+void OriginPolicyParser::ParseFeatures(const base::Value& features) {
+  const std::string* policy = features.FindStringKey("policy");
 
-bool OriginPolicyParser::ParseFeaturePolicy(const base::Value& policy) {
-  policy_contents_->features.push_back(policy.GetString());
-  return true;
+  if (policy) {
+    policy_contents_->feature_policy = *policy;
+  }
 }
 
 }  // namespace network

services/network/origin_policy/origin_policy_parser.h

@@ -29,11 +29,21 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) OriginPolicyParser {
   OriginPolicyParser();
   ~OriginPolicyParser();
 
+  // The older spec treated parsing errors as failures that would cause an
+  // interstitial, so we have a boolean return value to represent that. The
+  // newer spec does not treat parsing errors as failures; see
+  // https://github.com/WICG/origin-policy/issues/49.
+  // TODO(domenic): update everything to the newer spec and remove all boolean
+  // return values.
+
   bool DoParse(base::StringPiece);
+
+  // The following methods are implemented according to the older spec:
   bool ParseContentSecurityPolicies(const base::Value&);
   bool ParseContentSecurityPolicy(const base::Value&);
-  bool ParseFeaturePolicies(const base::Value&);
-  bool ParseFeaturePolicy(const base::Value&);
+
+  // The following method is implemented according to the newer spec:
+  void ParseFeatures(const base::Value&);
 
   OriginPolicyContentsPtr policy_contents_;
 

services/network/origin_policy/origin_policy_parser_unittest.cc

@@ -16,7 +16,7 @@ namespace {
 
 void AssertEmptyPolicy(
     const network::OriginPolicyContentsPtr& policy_contents) {
-  ASSERT_EQ(0u, policy_contents->features.size());
+  ASSERT_FALSE(policy_contents->feature_policy.has_value());
   ASSERT_EQ(0u, policy_contents->content_security_policies.size());
   ASSERT_EQ(0u, policy_contents->content_security_policies_report_only.size());
 }
@@ -155,37 +155,77 @@ TEST(OriginPolicyParser, CSPDispositionAbsent) {
 
 TEST(OriginPolicyParser, FeatureOne) {
   auto policy_contents = OriginPolicyParser::Parse(R"(
-      { "feature-policy": ["geolocation 'self' http://maps.google.com"] } )");
-  ASSERT_EQ(1U, policy_contents->features.size());
+      { "features": { "policy":
+        "geolocation 'self' http://maps.google.com"
+      } } )");
   ASSERT_EQ("geolocation 'self' http://maps.google.com",
-            policy_contents->features[0]);
+            policy_contents->feature_policy);
 }
 
 TEST(OriginPolicyParser, FeatureTwo) {
   auto policy_contents = OriginPolicyParser::Parse(R"(
-      { "feature-policy": ["geolocation 'self' http://maps.google.com",
-                     "camera https://example.com"]} )");
-  ASSERT_EQ(2U, policy_contents->features.size());
-  ASSERT_EQ("geolocation 'self' http://maps.google.com",
-            policy_contents->features[0]);
-  ASSERT_EQ("camera https://example.com", policy_contents->features[1]);
+      { "features": { "policy":
+        "geolocation 'self' http://maps.google.com; camera https://example.com"
+      } } )");
+  ASSERT_EQ(
+      "geolocation 'self' http://maps.google.com; camera https://example.com",
+      policy_contents->feature_policy);
+}
+
+TEST(OriginPolicyParser, FeatureTwoFeatures) {
+  auto policy_contents = OriginPolicyParser::Parse(R"(
+      { "features": { "policy": "geolocation 'self' http://maps.google.com" },
+        "features": { "policy": "camera https://example.com"}
+      } )");
+
+  ASSERT_EQ("camera https://example.com", policy_contents->feature_policy);
 }
 
-TEST(OriginPolicyParser, FeatureTwoPolicies) {
+TEST(OriginPolicyParser, FeatureTwoPolicy) {
   auto policy_contents = OriginPolicyParser::Parse(R"(
-      { "feature-policy": ["geolocation 'self' http://maps.google.com"],
-        "feature-policy": ["camera https://example.com"] } )");
+      { "features": { "policy": "geolocation 'self' http://maps.google.com",
+                      "policy": "camera https://example.com"
+      } } )");
 
-  // TODO(vogelheim): Determine whether this is the correct behaviour.
-  ASSERT_EQ(1U, policy_contents->features.size());
+  ASSERT_EQ("camera https://example.com", policy_contents->feature_policy);
 }
 
+// At this level we don't validate the syntax, so commas get passed through.
+// Integration tests will show that comma-containing policies get discarded,
+// though.
 TEST(OriginPolicyParser, FeatureComma) {
   auto policy_contents = OriginPolicyParser::Parse(R"(
-      { "feature-policy": ["geolocation 'self' http://maps.google.com, camera https://example.com"]} )");
+      { "features": { "policy":
+        "geolocation 'self' http://maps.google.com, camera https://example.com"
+      } } )");
+
+  ASSERT_EQ(
+      "geolocation 'self' http://maps.google.com, camera https://example.com",
+      policy_contents->feature_policy);
+}
 
-  // TODO: Determine what to do with this case !
-  ASSERT_EQ(1U, policy_contents->features.size());
+// Similarly, complete garbage will be passed through; this is expected.
+TEST(OriginPolicyParser, FeatureGarbage) {
+  auto policy_contents = OriginPolicyParser::Parse(R"(
+      { "features": { "policy":
+        "Lorem ipsum! dolor sit amet"
+      } } )");
+
+  ASSERT_EQ("Lorem ipsum! dolor sit amet", policy_contents->feature_policy);
+}
+
+TEST(OriginPolicyParser, FeatureNonDict) {
+  auto policy_contents = OriginPolicyParser::Parse(R"(
+      { "features": "geolocation 'self' http://maps.google.com"
+      } )");
+  ASSERT_FALSE(policy_contents->feature_policy.has_value());
+}
+
+TEST(OriginPolicyParser, FeatureNonString) {
+  auto policy_contents = OriginPolicyParser::Parse(R"(
+      { "features": { "policy": ["geolocation 'self' http://maps.google.com"]
+      } )");
+  ASSERT_FALSE(policy_contents->feature_policy.has_value());
 }
 
 }  // namespace network

services/network/public/cpp/network_ipc_param_traits.h

@@ -116,7 +116,7 @@ IPC_ENUM_TRAITS_MAX_VALUE(network::OriginPolicyState,
                           network::OriginPolicyState::kMaxValue)
 
 IPC_STRUCT_TRAITS_BEGIN(network::OriginPolicyContents)
-  IPC_STRUCT_TRAITS_MEMBER(features)
+  IPC_STRUCT_TRAITS_MEMBER(feature_policy)
   IPC_STRUCT_TRAITS_MEMBER(content_security_policies)
   IPC_STRUCT_TRAITS_MEMBER(content_security_policies_report_only)
 IPC_STRUCT_TRAITS_END()

services/network/public/cpp/origin_policy.cc

@@ -23,10 +23,10 @@ OriginPolicyContents::OriginPolicyContents(const OriginPolicyContents& other) =
     default;
 
 OriginPolicyContents::OriginPolicyContents(
-    const std::vector<std::string>& features,
+    const base::Optional<std::string>& feature_policy,
     const std::vector<std::string>& content_security_policies,
     const std::vector<std::string>& content_security_policies_report_only)
-    : features(features),
+    : feature_policy(feature_policy),
       content_security_policies(content_security_policies),
       content_security_policies_report_only(
           content_security_policies_report_only) {}
@@ -35,7 +35,7 @@ OriginPolicyContents& OriginPolicyContents::operator=(
     const OriginPolicyContents& other) = default;
 
 bool OriginPolicyContents::operator==(const OriginPolicyContents& other) const {
-  return features == other.features &&
+  return feature_policy == other.feature_policy &&
          content_security_policies == other.content_security_policies &&
          content_security_policies_report_only ==
              other.content_security_policies_report_only;

services/network/public/cpp/origin_policy.h

@@ -9,6 +9,7 @@
 #include <string>
 #include <vector>
 
+#include "base/optional.h"
 #include "url/gurl.h"
 
 namespace network {
@@ -51,7 +52,7 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) OriginPolicyContents {
   OriginPolicyContents();
   ~OriginPolicyContents();
   OriginPolicyContents(
-      const std::vector<std::string>& features,
+      const base::Optional<std::string>& feature_policy,
       const std::vector<std::string>& content_security_policies,
       const std::vector<std::string>& content_security_policies_report_only);
 
@@ -61,13 +62,16 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) OriginPolicyContents {
 
   OriginPolicyContentsPtr ClonePtr();
 
-  // The feature policy that is dictated by the origin policy. Each feature
-  // is one member of the array.
+  // The feature policy that is dictated by the origin policy, if any.
   // https://w3c.github.io/webappsec-feature-policy/
-  std::vector<std::string> features;
+  // This is stored as a raw string, so it is not guaranteed to be an actual
+  // feature policy; Blink will attempt to parse and apply it.
+  base::Optional<std::string> feature_policy;
 
   // These two fields together represent the CSP that should be applied to the
-  // origin, based on the origin policy.
+  // origin, based on the origin policy. They are stored as raw strings, so are
+  // not guaranteed to be actual CSPs; Blink will attempt to parse and apply
+  // them.
   // https://w3c.github.io/webappsec-csp/
 
   // The "enforced" portion of the CSP. This CSP is to be treated as having

third_party/blink/public/web/web_origin_policy.h

@@ -16,10 +16,9 @@ namespace blink {
 
 // Origin Policy spec: https://wicg.github.io/origin-policy/
 struct BLINK_EXPORT WebOriginPolicy {
-  // The feature policy that is dictated by the origin policy. Each feature
-  // is one member of the array.
+  // The feature policy that is dictated by the origin policy, if any.
   // https://w3c.github.io/webappsec-feature-policy/
-  WebVector<WebString> features;
+  WebString feature_policy;
 
   // These two fields together represent the CSP that should be applied to the
   // origin, based on the origin policy.

third_party/blink/renderer/core/loader/document_loader.cc

@@ -1384,13 +1384,17 @@ void DocumentLoader::DidCommitNavigation() {
 // origin policy (if any).
 // Headers go first, which means that the per-page headers override the
 // origin policy features.
+//
+// TODO(domenic): we want to treat origin policy feature policy as a single
+// feature policy, not a header serialization, so it should be processed
+// differently.
 void MergeFeaturesFromOriginPolicy(WTF::StringBuilder& feature_policy,
                                    const WebOriginPolicy& origin_policy) {
-  for (const auto& policy : origin_policy.features) {
+  if (!origin_policy.feature_policy.IsNull()) {
     if (!feature_policy.IsEmpty()) {
       feature_policy.Append(',');
     }
-    feature_policy.Append(policy);
+    feature_policy.Append(origin_policy.feature_policy);
   }
 }
 

third_party/blink/web_tests/external/wpt/.well-known/origin-policy/policy-features

-{
-  "feature-policy": [
-    "camera 'self' https://example.org",
-    "geolocation https://example.org/"
-  ]
-}

third_party/blink/web_tests/external/wpt/.well-known/origin-policy/policy-features-comma-in-policy

+{
+  "features": {
+    "policy": "camera 'self' https://example.com/, geolocation 'self' https://example.com/"
+  }
+}

third_party/blink/web_tests/external/wpt/.well-known/origin-policy/policy-features-double-features

+{
+  "features": {
+    "policy": "camera 'self' https://example.com/"
+  },
+  "features": {
+    "policy": "geolocation 'self' https://example.com/"
+  }
+}

third_party/blink/web_tests/external/wpt/.well-known/origin-policy/policy-features-double-policy

+{
+  "features": {
+    "policy": "camera 'self' https://example.com/",
+    "policy": "geolocation 'self' https://example.com/"
+  }
+}

third_party/blink/web_tests/external/wpt/.well-known/origin-policy/policy-features-non-object

+{
+  "features": "camera 'self' https://example.com/"
+}

third_party/blink/web_tests/external/wpt/.well-known/origin-policy/policy-features-non-string

+{
+  "features": {
+    "policy": ["camera 'self' https://example.com/"]
+  }
+}

third_party/blink/web_tests/external/wpt/.well-known/origin-policy/policy-features-valid

+{
+  "features": {
+    "policy": "camera 'self' https://example.com/; geolocation 'self' https://example.com/"
+  }
+}

third_party/blink/web_tests/external/wpt/origin-policy/features/comma-in-policy.https.html

+<!DOCTYPE HTML>
+<meta charset="utf-8">
+<title>Commas in "features/policy" cause parse errors and thus no feature policy</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="helper.js"></script>
+
+<script>
+"use strict";
+runFPTest({ camera: false, geolocation: false });
+</script>

third_party/blink/web_tests/external/wpt/origin-policy/features/comma-in-policy.https.html.headers

+Sec-Origin-Policy: policy=policy-features-comma-in-policy

third_party/blink/web_tests/external/wpt/origin-policy/features/double-features.https.html

+<!DOCTYPE HTML>
+<meta charset="utf-8">
+<title>Of two "features" items only the second counts</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="helper.js"></script>
+
+<script>
+"use strict";
+runFPTest({ camera: false, geolocation: true });
+</script>

third_party/blink/web_tests/external/wpt/origin-policy/features/double-features.https.html.headers

+Sec-Origin-Policy: policy=policy-features-double-features

third_party/blink/web_tests/external/wpt/origin-policy/features/double-policy.https.html

+<!DOCTYPE HTML>
+<meta charset="utf-8">
+<title>Of two "features/policy" items only the second counts</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="helper.js"></script>
+
+<script>
+"use strict";
+runFPTest({ camera: false, geolocation: true });
+</script>

third_party/blink/web_tests/external/wpt/origin-policy/features/double-policy.https.html.headers

+Sec-Origin-Policy: policy=policy-features-double-policy

third_party/blink/web_tests/external/wpt/origin-policy/features/helper.js

+"use strict";
+
+window.runFPTest = ({ camera, geolocation }) => {
+  test(() => {
+    assert_equals(document.featurePolicy.allowsFeature('camera', 'https://example.com/'), camera, 'camera');
+    assert_equals(document.featurePolicy.allowsFeature('geolocation', 'https://example.com/'), geolocation, 'geolocation');
+  });
+};

third_party/blink/web_tests/external/wpt/origin-policy/features/non-object.https.html

+<!DOCTYPE HTML>
+<meta charset="utf-8">
+<title>Non-object "features" member must be ignored</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="helper.js"></script>
+
+<script>
+"use strict";
+runFPTest({ camera: false, geolocation: false });
+</script>

third_party/blink/web_tests/external/wpt/origin-policy/features/non-object.https.html.headers

+Sec-Origin-Policy: policy=policy-features-non-object

third_party/blink/web_tests/external/wpt/origin-policy/features/non-string.https.html

+<!DOCTYPE HTML>
+<meta charset="utf-8">
+<title>Non-string "features/policy" member must be ignored</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="helper.js"></script>
+
+<script>
+"use strict";
+runFPTest({ camera: false, geolocation: false });
+</script>

third_party/blink/web_tests/external/wpt/origin-policy/features/non-string.https.html.headers

+Sec-Origin-Policy: policy=policy-features-non-string

third_party/blink/web_tests/external/wpt/origin-policy/features/valid.https.html

+<!DOCTYPE HTML>
+<meta charset="utf-8">
+<title>Valid "features" member</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="helper.js"></script>
+
+<script>
+"use strict";
+runFPTest({ camera: true, geolocation: true });
+</script>

third_party/blink/web_tests/external/wpt/origin-policy/features/valid.https.html.headers

+Sec-Origin-Policy: policy=policy-features-valid

third_party/blink/web_tests/external/wpt/origin-policy/origin-policy-features.https.tentative.html

-<!DOCTYPE HTML>
-<html>
-<head>
-  <script src='/resources/testharness.js'></script>
-  <script src='/resources/testharnessreport.js'></script>
-</head>
-<body>
-  <script>
-    async_test(t => {
-      assert_false(document.featurePolicy.allowsFeature('geolocation'));
-      assert_true(document.featurePolicy.allowsFeature('camera'));
-      t.done();
-    }, "Origin-Policy-based Feature policy");
-  </script>
-</body>
-</html>

third_party/blink/web_tests/external/wpt/origin-policy/origin-policy-features.https.tentative.html.headers

-Sec-Origin-Policy: policy=policy-features

third_party/blink/web_tests/virtual/origin-policy/external/wpt/origin-policy/features/comma-in-policy.https-expected.txt

+This is a testharness.js-based test.
+FAIL Commas in "features/policy" cause parse errors and thus no feature policy assert_equals: camera expected false but got true
+Harness: the test ran to completion.
+