Fix some minor feature policy parsing issues

This CL makes the following changes to the feature policy parsing code:

  1- ParsedFeaturePolicyDeclaration holds a sorted vector of unique
     |origins|.
  2- AllowList uses std::set instead of std::vector.
  3- When parsing for list of origins, in case of matching all origins
     (*), the current set of origins is cleared.
  4- When comparing ParsedFeaturePolicyDeclaration, if both
     declarations include '*' then the set of origins are not compared.

The noticeable outcome of the CL is that parsed policy will ignore
repeated origins and will be sorted. This would make the feature lookup
algorithm more efficient.

Bug: 710324
Change-Id: I5c67ee2d6cff891304781bea0998e07739006a2e
Reviewed-on: https://chromium-review.googlesource.com/c/1161753Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Ian Clelland <iclelland@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610887}

content/browser/site_per_process_browsertest.cc

@@ -483,6 +483,9 @@ blink::ParsedFeaturePolicy CreateFPHeader(
   DCHECK(!origins.empty());
   for (const GURL& origin : origins)
     result[0].origins.push_back(url::Origin::Create(origin));
+  // We expect the parsed features to be sorted so that they can reliably be
+  // compared.
+  std::sort(result[0].origins.begin(), result[0].origins.end());
   return result;
 }
 

third_party/blink/common/feature_policy/feature_policy.cc

@@ -40,7 +40,7 @@ ParsedFeaturePolicyDeclaration::ParsedFeaturePolicyDeclaration(
       matches_all_origins(matches_all_origins),
       matches_opaque_src(matches_opaque_src),
       disposition(disposition),
-      origins(origins) {}
+      origins(std::move(origins)) {}
 
 ParsedFeaturePolicyDeclaration::ParsedFeaturePolicyDeclaration(
     const ParsedFeaturePolicyDeclaration& rhs) = default;
@@ -52,16 +52,13 @@ ParsedFeaturePolicyDeclaration::~ParsedFeaturePolicyDeclaration() = default;
 
 bool operator==(const ParsedFeaturePolicyDeclaration& lhs,
                 const ParsedFeaturePolicyDeclaration& rhs) {
-  // This method returns true only when the arguments are actually identical,
-  // including the order of elements in the origins vector.
-  // TODO(iclelland): Consider making this return true when comparing equal-
-  // but-not-identical allowlists, or eliminate those comparisons by maintaining
-  // the allowlists in a normalized form.
-  // https://crbug.com/710324
-  return std::tie(lhs.feature, lhs.matches_all_origins, lhs.origins,
-                  lhs.disposition) == std::tie(rhs.feature,
-                                               rhs.matches_all_origins,
-                                               rhs.origins, rhs.disposition);
+  if (lhs.feature != rhs.feature)
+    return false;
+  if (lhs.disposition != rhs.disposition)
+    return false;
+  if (lhs.matches_all_origins != rhs.matches_all_origins)
+    return false;
+  return lhs.matches_all_origins || (lhs.origins == rhs.origins);
 }
 
 std::unique_ptr<ParsedFeaturePolicy> DirectivesWithDisposition(
@@ -83,7 +80,7 @@ FeaturePolicy::Allowlist::Allowlist(const Allowlist& rhs) = default;
 FeaturePolicy::Allowlist::~Allowlist() = default;
 
 void FeaturePolicy::Allowlist::Add(const url::Origin& origin) {
-  origins_.push_back(origin);
+  origins_.insert(origin);
 }
 
 void FeaturePolicy::Allowlist::AddAll() {
@@ -97,20 +94,21 @@ bool FeaturePolicy::Allowlist::Contains(const url::Origin& origin) const {
   // TODO(iclelland): Fix that, possibly by having another flag for
   // 'matches_self', which will explicitly match the policy's origin.
   // https://crbug.com/690520
-  if (matches_all_origins_)
+  if (matches_all_origins_) {
+    DCHECK(origins_.empty());
     return true;
-  for (const auto& targetOrigin : origins_) {
-    if (!origin.opaque() && targetOrigin.IsSameOriginWith(origin))
-      return true;
   }
-  return false;
+
+  if (origin.opaque())
+    return false;
+  return base::ContainsKey(origins_, origin);
 }
 
 bool FeaturePolicy::Allowlist::MatchesAll() const {
   return matches_all_origins_;
 }
 
-const std::vector<url::Origin>& FeaturePolicy::Allowlist::Origins() const {
+const base::flat_set<url::Origin>& FeaturePolicy::Allowlist::Origins() const {
   return origins_;
 }
 

third_party/blink/public/common/feature_policy/feature_policy.h

@@ -9,6 +9,7 @@
 #include <memory>
 #include <vector>
 
+#include "base/containers/flat_set.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "third_party/blink/public/common/common_export.h"
@@ -108,6 +109,7 @@ struct BLINK_COMMON_EXPORT ParsedFeaturePolicyDeclaration {
   // flag is set instead.
   bool matches_opaque_src;
   mojom::FeaturePolicyDisposition disposition;
+  // An alphabetically sorted list of all the origins allowed.
   std::vector<url::Origin> origins;
 };
 
@@ -147,12 +149,12 @@ class BLINK_COMMON_EXPORT FeaturePolicy {
     // Returns true if the allowlist matches all origins.
     bool MatchesAll() const;
 
-    // Returns list of origins in the allowlist.
-    const std::vector<url::Origin>& Origins() const;
+    // Returns set of origins in the allowlist.
+    const base::flat_set<url::Origin>& Origins() const;
 
    private:
     bool matches_all_origins_;
-    std::vector<url::Origin> origins_;
+    base::flat_set<url::Origin> origins_;
   };
 
   // The FeaturePolicy::FeatureDefault enum defines the default enable state for

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

@@ -156,6 +156,7 @@ ParsedFeaturePolicy ParseFeaturePolicy(
           continue;
         } else if (tokens[i] == "*") {
           allowlist.matches_all_origins = true;
+          origins.clear();
           break;
         } else {
           scoped_refptr<SecurityOrigin> target_origin =
@@ -166,7 +167,10 @@ ParsedFeaturePolicy ParseFeaturePolicy(
             messages->push_back("Unrecognized origin: '" + tokens[i] + "'.");
         }
       }
-      allowlist.origins = origins;
+      std::sort(origins.begin(), origins.end());
+      auto new_end = std::unique(origins.begin(), origins.end());
+      origins.erase(new_end, origins.end());
+      allowlist.origins = std::move(origins);
       allowlists.push_back(allowlist);
     }
   }

third_party/blink/web_tests/external/wpt/feature-policy/feature-policy-header-policy-allowed-for-some.https.sub.html

@@ -16,8 +16,8 @@
   // Test that fullscreen's allowlist is [same_origin, cross_origin, 'https://www.example.com']
   test(function() {
     assert_array_equals(
-      document.policy.getAllowlistForFeature('fullscreen'),
-      [same_origin, cross_origin, 'https://www.example.com']);
+      document.policy.getAllowlistForFeature('fullscreen').sort(),
+      [same_origin, cross_origin, 'https://www.example.com'].sort());
   }, header_policy + ' -- test allowlist is [same_origin, cross_origin, https://www.example.com]');
 
   // Test that fullscreen is allowed on same_origin, some cross_origin subframes.

third_party/blink/web_tests/external/wpt/feature-policy/feature-policy-header-policy-declined.https.sub.html

@@ -17,7 +17,7 @@
   test(function() {
     assert_array_equals(
       document.policy.getAllowlistForFeature('fullscreen'),
-      [cross_origin, 'https://www.example.com']);
+      [cross_origin, 'https://www.example.com'].sort());
   }, header_policy + ' -- test allowlist is [cross_origin, https://www.example.com]');
 
   // Test that fullscreen is disallowed on same_origin, allowed on some cross_origin subframes.

third_party/blink/web_tests/http/tests/feature-policy/policy_main_document.php

@@ -70,8 +70,10 @@ assert_array_equals(
   policy_main.getAllowlistForFeature("payment"), ["http://127.0.0.1:8000"],
   "payment is allowed for self");
 assert_array_equals(
-  policy_main.getAllowlistForFeature("camera"),
-  ["http://127.0.0.1:8000", "https://www.example.com", "https://www.example.net"],
+  policy_main.getAllowlistForFeature("camera").sort(),
+  ["http://127.0.0.1:8000",
+   "https://www.example.com",
+   "https://www.example.net"].sort(),
   "camera is allowed for multiple origins");
 assert_array_equals(
   policy_main.getAllowlistForFeature("midi"), [], "midi is disallowed for all");