Fix feature policy origin comparison for sandbox.

When 'self' was specified in a feature policy header for a sandboxed
page, the resulting policy would not actually allow the feature in that
page, even when it would otherwise have been allowed. This corrects that
by assigning the correct origin to the policy and accepting that origin
in allowlists.

Bug: 973880, 690520
Change-Id: I93325bf24119068f8138f6e38507598cc30cbb06
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1689958
Commit-Queue: Ian Clelland <iclelland@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675737}

third_party/blink/common/feature_policy/feature_policy.cc

@@ -84,19 +84,14 @@ void FeaturePolicy::Allowlist::Add(const url::Origin& origin,
 
 PolicyValue FeaturePolicy::Allowlist::GetValueForOrigin(
     const url::Origin& origin) const {
-  // This does not handle the case where origin is an opaque origin, which is
-  // also supposed to exist in the allowlist. (The identical opaque origins
-  // should match in that case)
-  // TODO(iclelland): Fix that, possibly by having another flag for
-  // 'matches_self', which will explicitly match the policy's origin.
-  // https://crbug.com/690520
   // |fallback_value_| will either be min (initialized in the parser) value or
   // set to the corresponding value for * origins.
+  auto value = values_.find(origin);
+  if (value != values_.end())
+    return value->second;
   if (origin.opaque())
     return opaque_value_;
-
-  auto value = values_.find(origin);
-  return value == values_.end() ? fallback_value_ : value->second;
+  return fallback_value_;
 }
 
 const PolicyValue& FeaturePolicy::Allowlist::GetFallbackValue() const {
@@ -244,12 +239,6 @@ FeaturePolicy::FeatureState FeaturePolicy::GetFeatureState() const {
 FeaturePolicy::FeaturePolicy(url::Origin origin,
                              const FeatureList& feature_list)
     : origin_(std::move(origin)), feature_list_(feature_list) {
-  if (origin_.opaque()) {
-    // FeaturePolicy was written expecting opaque Origins to be indistinct, but
-    // this has changed. Split out a new opaque origin here, to defend against
-    // origin-equality.
-    origin_ = origin_.DeriveNewOpaqueOrigin();
-  }
 }
 
 FeaturePolicy::~FeaturePolicy() = default;

third_party/blink/common/feature_policy/feature_policy_unittest.cc

@@ -1476,10 +1476,10 @@ TEST_F(FeaturePolicyTest, TestSandboxedFramePolicyForAllOrigins) {
   EXPECT_TRUE(
       policy2->IsFeatureEnabledForOrigin(kDefaultOnFeature, sandboxed_origin));
   EXPECT_TRUE(policy2->IsFeatureEnabled(kDefaultSelfFeature));
+  EXPECT_TRUE(policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
+                                                 sandboxed_origin));
   EXPECT_FALSE(
       policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature, origin_a_));
-  EXPECT_FALSE(policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
-                                                  sandboxed_origin));
 }
 
 TEST_F(FeaturePolicyTest, TestSandboxedFramePolicyForOpaqueSrcOrigin) {
@@ -1508,10 +1508,10 @@ TEST_F(FeaturePolicyTest, TestSandboxedFramePolicyForOpaqueSrcOrigin) {
   EXPECT_TRUE(
       policy2->IsFeatureEnabledForOrigin(kDefaultOnFeature, sandboxed_origin));
   EXPECT_TRUE(policy2->IsFeatureEnabled(kDefaultSelfFeature));
+  EXPECT_TRUE(policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
+                                                 sandboxed_origin));
   EXPECT_FALSE(
       policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature, origin_a_));
-  EXPECT_FALSE(policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
-                                                  sandboxed_origin));
 }
 
 TEST_F(FeaturePolicyTest, TestSandboxedFrameFromHeaderPolicy) {
@@ -1539,10 +1539,10 @@ TEST_F(FeaturePolicyTest, TestSandboxedFrameFromHeaderPolicy) {
   std::unique_ptr<FeaturePolicy> policy2 = CreateFromParentWithFramePolicy(
       policy1.get(), frame_policy, sandboxed_origin);
   EXPECT_TRUE(policy2->IsFeatureEnabled(kDefaultSelfFeature));
+  EXPECT_TRUE(policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
+                                                 sandboxed_origin));
   EXPECT_FALSE(
       policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature, origin_a_));
-  EXPECT_FALSE(policy2->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
-                                                  sandboxed_origin));
 }
 
 TEST_F(FeaturePolicyTest, TestSandboxedPolicyIsNotInherited) {
@@ -1626,12 +1626,12 @@ TEST_F(FeaturePolicyTest, TestSandboxedPolicyCanBePropagated) {
   EXPECT_TRUE(policy3->IsFeatureEnabledForOrigin(kDefaultOnFeature,
                                                  sandboxed_origin_2));
   EXPECT_TRUE(policy3->IsFeatureEnabled(kDefaultSelfFeature));
+  EXPECT_TRUE(policy3->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
+                                                 sandboxed_origin_2));
   EXPECT_FALSE(
       policy3->IsFeatureEnabledForOrigin(kDefaultSelfFeature, origin_a_));
   EXPECT_FALSE(policy3->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
                                                   sandboxed_origin_1));
-  EXPECT_FALSE(policy3->IsFeatureEnabledForOrigin(kDefaultSelfFeature,
-                                                  sandboxed_origin_2));
 }
 
 TEST_F(FeaturePolicyTest, TestUndefinedFeaturesInFramePolicy) {

third_party/blink/renderer/core/feature_policy/feature_policy_parser.cc

@@ -273,7 +273,6 @@ ParsedFeaturePolicy FeaturePolicyParser::Parse(
         } else if (target_is_opaque) {
           allowlist.opaque_value = value;
         } else {
-          DCHECK(!target_origin.opaque());
           values[target_origin] = value;
         }
       }

third_party/blink/web_tests/external/wpt/feature-policy/policy-extends-to-sandbox.html

+<!DOCTYPE html>
+<title>Feature policy treats opaque origins correctly</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<body>
+<script>
+  "use strict";
+  async_test(t => {
+    let frame = document.createElement('iframe');
+    frame.src = "/feature-policy/resources/sandbox-self.html";
+    frame.allow = "fullscreen";
+    frame.sandbox = "allow-scripts";
+
+    var handle_message = t.step_func(evt => {
+      if (evt.source === frame.contentWindow) {
+        assert_equals(evt.data.child, true, "'self' in header should match origin of sandboxed frame.");
+        assert_equals(evt.data.grandchild, false, "Opaque origins should not match each other.");
+        document.body.removeChild(frame);
+        window.removeEventListener('message', handle_message);
+        t.done();
+      }
+    });
+    window.addEventListener('message', handle_message);
+    document.body.appendChild(frame);
+  });
+</script>

third_party/blink/web_tests/external/wpt/feature-policy/resources/nested-sandbox.html

+<!DOCTYPE html>
+<title>Return fullscreen feature policy state</title>
+<script>
+  "use strict";
+  window.onload = () => {
+    window.parent.postMessage(document.featurePolicy.allowedFeatures().includes("fullscreen"),"*");
+  };
+</script>

third_party/blink/web_tests/external/wpt/feature-policy/resources/sandbox-self.html

+<!DOCTYPE html>
+<title>Return fullscreen feature policy state from self and a sandboxed child frame</title>
+<script>
+  "use strict";
+  window.onload = () => {
+    let frame = document.createElement('iframe');
+    frame.src = "/feature-policy/resources/nested-sandbox.html";
+    frame.sandbox = "allow-scripts";
+
+    var handle_message = evt => {
+      if (evt.source === frame.contentWindow) {
+        window.parent.postMessage({
+          "child": document.featurePolicy.allowedFeatures().includes("fullscreen"),
+          "grandchild": evt.data
+        },"*");
+        document.body.removeChild(frame);
+        window.removeEventListener('message', handle_message);
+      }
+    };
+    window.addEventListener('message', handle_message);
+    document.body.appendChild(frame);
+  };
+</script>

third_party/blink/web_tests/external/wpt/feature-policy/resources/sandbox-self.html.headers

+Feature-Policy: fullscreen 'self'