[DNR] Add priority checking for allow rules overriding modifyHeaders

This CL adds priority checking between matching modifyHeaders and allow/
allowAllRequests rules in an extension. Currently, if there is a
matching allow/allowAllRequests rule, it will override any removeHeaders
rules since removeHeaders rules don't have priority.

Since modifyHeaders rules have priority, within an extension, the
matching allow/allowAllRequests rule with the highest priority should
override modifyHeaders rules with equal or lesser priority.

Bug: 947591
Change-Id: Ifc8e4e18ea1d33811d4458574e474603cea44461
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2172640
Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#769940}

extensions/browser/api/declarative_net_request/composite_matcher.cc

@@ -120,11 +120,21 @@ ActionInfo CompositeMatcher::GetBeforeRequestAction(
 
   bool notify_request_withheld = false;
   base::Optional<RequestAction> final_action;
+
+  // The priority of the highest priority matching allow or allowAllRequests
+  // rule within this matcher, or base::nullopt otherwise.
+  base::Optional<uint64_t> max_allow_rule_priority;
+
   for (const auto& matcher : matchers_) {
     base::Optional<RequestAction> action =
         matcher->GetBeforeRequestAction(params);
-    params.allow_rule_cache[matcher.get()] =
-        action && action->IsAllowOrAllowAllRequests();
+
+    if (action && action->IsAllowOrAllowAllRequests()) {
+      max_allow_rule_priority =
+          max_allow_rule_priority
+              ? std::max(*max_allow_rule_priority, action->index_priority)
+              : action->index_priority;
+    }
 
     if (action && action->type == RequestAction::Type::REDIRECT) {
       // Redirecting requires host permissions.
@@ -142,6 +152,8 @@ ActionInfo CompositeMatcher::GetBeforeRequestAction(
         GetMaxPriorityAction(std::move(final_action), std::move(action));
   }
 
+  params.allow_rule_max_priority[this] = max_allow_rule_priority;
+
   if (final_action)
     return ActionInfo(std::move(final_action), false);
   return ActionInfo(base::nullopt, notify_request_withheld);
@@ -150,16 +162,19 @@ ActionInfo CompositeMatcher::GetBeforeRequestAction(
 std::vector<RequestAction> CompositeMatcher::GetModifyHeadersActions(
     const RequestParams& params) const {
   std::vector<RequestAction> modify_headers_actions;
+  DCHECK(params.allow_rule_max_priority.contains(this));
 
-  for (const auto& matcher : matchers_) {
-    // TODO(crbug.com/947591): An allow or allowAllRequests rule should override
-    // all equal or lower priority modifyHeaders rules specified by |matcher|.
-    DCHECK(params.allow_rule_cache.contains(matcher.get()));
-    if (params.allow_rule_cache[matcher.get()])
-      return std::vector<RequestAction>();
+  // The priority of the highest priority matching allow or allowAllRequests
+  // rule within this matcher, or base::nullopt if no such rule exists.
+  base::Optional<uint64_t> max_allow_rule_priority =
+      params.allow_rule_max_priority[this];
 
+  for (const auto& matcher : matchers_) {
+    // Plumb |max_allow_rule_priority| into GetModifyHeadersActions so that
+    // modifyHeaders rules with priorities less than or equal to the highest
+    // priority matching allow/allowAllRequests rule are ignored.
     std::vector<RequestAction> actions_for_matcher =
-        matcher->GetModifyHeadersActions(params);
+        matcher->GetModifyHeadersActions(params, max_allow_rule_priority);
 
     modify_headers_actions.insert(
         modify_headers_actions.end(),

extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc

@@ -28,6 +28,7 @@
 
 namespace extensions {
 namespace declarative_net_request {
+namespace {
 
 using PageAccess = PermissionsData::PageAccess;
 using ActionInfo = CompositeMatcher::ActionInfo;
@@ -36,6 +37,28 @@ namespace dnr_api = api::declarative_net_request;
 
 using CompositeMatcherTest = ::testing::Test;
 
+TestRule CreateModifyHeadersRule(
+    int id,
+    int priority,
+    base::Optional<std::string> url_filter,
+    base::Optional<std::string> regex_filter,
+    std::vector<TestHeaderInfo> request_headers_list) {
+  TestRule rule = CreateGenericRule();
+  rule.id = id;
+  rule.priority = priority;
+
+  if (url_filter)
+    rule.condition->url_filter = url_filter;
+  else if (regex_filter) {
+    rule.condition->url_filter.reset();
+    rule.condition->regex_filter = regex_filter;
+  }
+
+  rule.action->type = std::string("modifyHeaders");
+  rule.action->request_headers = std::move(request_headers_list);
+  return rule;
+}
+
 // Ensure that the rules in a CompositeMatcher are in the same priority space.
 TEST_F(CompositeMatcherTest, SamePrioritySpace) {
   // Create the first ruleset matcher. It allows requests to google.com.
@@ -102,25 +125,13 @@ TEST_F(CompositeMatcherTest, GetModifyHeadersActions) {
   // modifyHeaders action is complete.
   ScopedCurrentChannel channel(::version_info::Channel::UNKNOWN);
 
-  auto create_modify_headers_rule =
-      [](int id, int priority, const std::string& url_filter,
-         std::vector<TestHeaderInfo> request_headers_list) {
-        TestRule rule = CreateGenericRule();
-        rule.id = id;
-        rule.priority = priority;
-        rule.condition->url_filter = url_filter;
-        rule.action->type = std::string("modifyHeaders");
-        rule.action->request_headers = std::move(request_headers_list);
-        return rule;
-      };
-
-  TestRule rule_1 = create_modify_headers_rule(
-      kMinValidID, kMinValidPriority, "google.com",
+  TestRule rule_1 = CreateModifyHeadersRule(
+      kMinValidID, kMinValidPriority, "google.com", base::nullopt,
       std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove"),
                                    TestHeaderInfo("header2", "remove")}));
 
-  TestRule rule_2 = create_modify_headers_rule(
-      kMinValidID, kMinValidPriority + 1, "/path",
+  TestRule rule_2 = CreateModifyHeadersRule(
+      kMinValidID, kMinValidPriority + 1, "/path", base::nullopt,
       std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove"),
                                    TestHeaderInfo("header3", "remove")}));
 
@@ -223,6 +234,143 @@ TEST_F(CompositeMatcherTest, GetModifyHeadersActions) {
                            ::testing::Eq(::testing::ByRef(action_2))));
 }
 
+// Tests that GetModifyHeadersActions method omits rules with an equal or lower
+// priority than a matched allow or allowAllRequests rule.
+TEST_F(CompositeMatcherTest, GetModifyHeadersActions_Priority) {
+  using HeaderInfo = RequestAction::HeaderInfo;
+
+  // TODO(crbug.com/947591): Remove the channel override once implementation of
+  // modifyHeaders action is complete.
+  ScopedCurrentChannel channel(::version_info::Channel::UNKNOWN);
+
+  int allow_rule_priority = kMinValidPriority + 1;
+
+  TestRule allow_rule = CreateGenericRule();
+  allow_rule.id = kMinValidID;
+  allow_rule.condition->url_filter = std::string("google.com/1");
+  allow_rule.action->type = std::string("allow");
+  allow_rule.priority = allow_rule_priority;
+
+  TestRule url_rule_1 = CreateModifyHeadersRule(
+      kMinValidID + 1, allow_rule_priority - 1, "google.com", base::nullopt,
+      std::vector<TestHeaderInfo>({TestHeaderInfo("header1", "remove")}));
+
+  TestRule url_rule_2 = CreateModifyHeadersRule(
+      kMinValidID + 2, allow_rule_priority, "google.com", base::nullopt,
+      std::vector<TestHeaderInfo>({TestHeaderInfo("header2", "remove")}));
+
+  TestRule url_rule_3 = CreateModifyHeadersRule(
+      kMinValidID + 3, allow_rule_priority + 1, "google.com", base::nullopt,
+      std::vector<TestHeaderInfo>({TestHeaderInfo("header3", "remove")}));
+
+  TestRule regex_rule_1 = CreateModifyHeadersRule(
+      kMinValidID + 4, allow_rule_priority - 1, base::nullopt, R"(google\.com)",
+      std::vector<TestHeaderInfo>({TestHeaderInfo("header4", "remove")}));
+
+  TestRule regex_rule_2 = CreateModifyHeadersRule(
+      kMinValidID + 5, allow_rule_priority, base::nullopt, R"(google\.com)",
+      std::vector<TestHeaderInfo>({TestHeaderInfo("header5", "remove")}));
+
+  TestRule regex_rule_3 = CreateModifyHeadersRule(
+      kMinValidID + 6, allow_rule_priority + 1, base::nullopt, R"(google\.com)",
+      std::vector<TestHeaderInfo>({TestHeaderInfo("header6", "remove")}));
+
+  const RulesetID kSource1ID(1);
+  std::unique_ptr<RulesetMatcher> matcher_1;
+  ASSERT_TRUE(
+      CreateVerifiedMatcher({allow_rule, url_rule_1, url_rule_2, url_rule_3},
+                            CreateTemporarySource(kSource1ID), &matcher_1));
+
+  const RulesetID kSource2ID(2);
+  std::unique_ptr<RulesetMatcher> matcher_2;
+  ASSERT_TRUE(CreateVerifiedMatcher({regex_rule_1, regex_rule_2, regex_rule_3},
+                                    CreateTemporarySource(kSource2ID),
+                                    &matcher_2));
+
+  // Create a CompositeMatcher with the rulesets.
+  std::vector<std::unique_ptr<RulesetMatcher>> matchers;
+  matchers.push_back(std::move(matcher_1));
+  matchers.push_back(std::move(matcher_2));
+  auto composite_matcher =
+      std::make_unique<CompositeMatcher>(std::move(matchers));
+
+  // Make a request to "http://google.com/1" which matches with all
+  // modifyHeaders rules and |allow_rule|.
+  GURL google_url = GURL("http://google.com/1");
+  RequestParams google_params;
+  google_params.url = &google_url;
+  google_params.element_type = url_pattern_index::flat::ElementType_SUBDOCUMENT;
+  google_params.is_third_party = false;
+
+  // Call GetBeforeRequestAction first to ensure that test and production code
+  // paths are consistent.
+  composite_matcher->GetBeforeRequestAction(google_params,
+                                            PageAccess::kAllowed);
+
+  std::vector<RequestAction> actions =
+      composite_matcher->GetModifyHeadersActions(google_params);
+
+  auto create_action_for_rule =
+      [](const TestRule& rule, const RulesetID& ruleset_id,
+         const std::vector<HeaderInfo>& request_headers) {
+        RequestAction action =
+            CreateRequestActionForTesting(RequestAction::Type::MODIFY_HEADERS,
+                                          *rule.id, *rule.priority, ruleset_id);
+
+        action.request_headers_to_modify = request_headers;
+        return action;
+      };
+
+  RequestAction header_3_action = create_action_for_rule(
+      url_rule_3, kSource1ID,
+      {HeaderInfo("header3", dnr_api::HEADER_OPERATION_REMOVE)});
+  RequestAction header_6_action = create_action_for_rule(
+      regex_rule_3, kSource2ID,
+      {HeaderInfo("header6", dnr_api::HEADER_OPERATION_REMOVE)});
+
+  // For the request to "http://google.com/1", since |url_rule_3| and
+  // |regex_rule_3| are the only rules with a greater priority than
+  // |allow_rule|, "header3" and "header4" should be removed.
+  EXPECT_THAT(actions, ::testing::UnorderedElementsAre(
+                           ::testing::Eq(::testing::ByRef(header_3_action)),
+                           ::testing::Eq(::testing::ByRef(header_6_action))));
+
+  // Make a request to "http://google.com/2" which should match with all
+  // modifyHeaders rules but not |allow_rule|.
+  google_url = GURL("http://google.com/2");
+  google_params.url = &google_url;
+
+  // Call GetBeforeRequestAction first to ensure that test and production code
+  // paths are consistent.
+  composite_matcher->GetBeforeRequestAction(google_params,
+                                            PageAccess::kAllowed);
+  actions = composite_matcher->GetModifyHeadersActions(google_params);
+
+  RequestAction header_1_action = create_action_for_rule(
+      url_rule_1, kSource1ID,
+      {HeaderInfo("header1", dnr_api::HEADER_OPERATION_REMOVE)});
+  RequestAction header_2_action = create_action_for_rule(
+      url_rule_2, kSource1ID,
+      {HeaderInfo("header2", dnr_api::HEADER_OPERATION_REMOVE)});
+  RequestAction header_4_action = create_action_for_rule(
+      regex_rule_1, kSource2ID,
+      {HeaderInfo("header4", dnr_api::HEADER_OPERATION_REMOVE)});
+  RequestAction header_5_action = create_action_for_rule(
+      regex_rule_2, kSource2ID,
+      {HeaderInfo("header5", dnr_api::HEADER_OPERATION_REMOVE)});
+
+  // For the request to "http://google.com/2", "header1" to "header6" should be
+  // removed since all modifyHeaders rules are matched and there is no matching
+  // allow/allowAllRequests rule.
+  EXPECT_THAT(actions, ::testing::UnorderedElementsAre(
+                           ::testing::Eq(::testing::ByRef(header_1_action)),
+                           ::testing::Eq(::testing::ByRef(header_2_action)),
+                           ::testing::Eq(::testing::ByRef(header_3_action)),
+                           ::testing::Eq(::testing::ByRef(header_4_action)),
+                           ::testing::Eq(::testing::ByRef(header_5_action)),
+                           ::testing::Eq(::testing::ByRef(header_6_action))));
+}
+
 // Ensure CompositeMatcher detects requests to be notified based on the rule
 // matched and whether the extenion has access to the request.
 TEST_F(CompositeMatcherTest, NotifyWithholdFromPageAccess) {
@@ -393,5 +541,6 @@ TEST_F(CompositeMatcherTest, GetRedirectUrlFromPriority) {
   }
 }
 
+}  // namespace
 }  // namespace declarative_net_request
 }  // namespace extensions

extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.cc

@@ -84,10 +84,19 @@ ExtensionUrlPatternIndexMatcher::GetAllowAllRequestsAction(
 
 std::vector<RequestAction>
 ExtensionUrlPatternIndexMatcher::GetModifyHeadersActions(
-    const RequestParams& params) const {
+    const RequestParams& params,
+    base::Optional<uint64_t> min_priority) const {
+  // TODO(crbug.com/1083178): Plumb |min_priority| into UrlPatternIndexMatcher
+  // to prune more rules before matching on url filters.
   std::vector<const flat_rule::UrlRule*> rules =
       GetAllMatchingRules(params, flat::IndexType_modify_headers);
 
+  if (min_priority) {
+    base::EraseIf(rules, [&min_priority](const flat_rule::UrlRule* rule) {
+      return rule->priority() <= *min_priority;
+    });
+  }
+
   return GetModifyHeadersActionsFromMetadata(params, rules, *metadata_list_);
 }
 

extensions/browser/api/declarative_net_request/extension_url_pattern_index_matcher.h

@@ -29,7 +29,8 @@ class ExtensionUrlPatternIndexMatcher final : public RulesetMatcherBase {
   // RulesetMatcherBase override:
   ~ExtensionUrlPatternIndexMatcher() override;
   std::vector<RequestAction> GetModifyHeadersActions(
-      const RequestParams& params) const override;
+      const RequestParams& params,
+      base::Optional<uint64_t> min_priority) const override;
   bool IsExtraHeadersMatcher() const override {
     return is_extra_headers_matcher_;
   }

extensions/browser/api/declarative_net_request/regex_rules_matcher.cc

@@ -96,13 +96,20 @@ RegexRulesMatcher::RegexRulesMatcher(const ExtensionId& extension_id,
 RegexRulesMatcher::~RegexRulesMatcher() = default;
 
 std::vector<RequestAction> RegexRulesMatcher::GetModifyHeadersActions(
-    const RequestParams& params) const {
+    const RequestParams& params,
+    base::Optional<uint64_t> min_priority) const {
   const std::vector<RegexRuleInfo>& potential_matches =
       GetPotentialMatches(params);
 
   std::vector<const flat_rule::UrlRule*> rules;
   for (const RegexRuleInfo& info : potential_matches) {
-    if (info.regex_rule->action_type() == flat::ActionType_modify_headers &&
+    // Check for the rule's priority iff |min_priority| is specified.
+    bool has_sufficient_priority =
+        !min_priority ||
+        info.regex_rule->url_rule()->priority() > *min_priority;
+
+    if (has_sufficient_priority &&
+        info.regex_rule->action_type() == flat::ActionType_modify_headers &&
         re2::RE2::PartialMatch(params.url->spec(), *info.regex)) {
       rules.push_back(info.regex_rule->url_rule());
     }

extensions/browser/api/declarative_net_request/regex_rules_matcher.h

@@ -58,7 +58,8 @@ class RegexRulesMatcher final : public RulesetMatcherBase {
   // RulesetMatcherBase override:
   ~RegexRulesMatcher() override;
   std::vector<RequestAction> GetModifyHeadersActions(
-      const RequestParams& params) const override;
+      const RequestParams& params,
+      base::Optional<uint64_t> min_priority) const override;
   bool IsExtraHeadersMatcher() const override {
     return is_extra_headers_matcher_;
   }

extensions/browser/api/declarative_net_request/request_params.h

@@ -22,7 +22,7 @@ namespace extensions {
 struct WebRequestInfo;
 
 namespace declarative_net_request {
-class RulesetMatcher;
+class CompositeMatcher;
 
 // Struct to hold parameters for a network request.
 struct RequestParams {
@@ -44,10 +44,12 @@ struct RequestParams {
   // ID of the parent RenderFrameHost.
   content::GlobalFrameRoutingId parent_routing_id;
 
-  // A map from RulesetMatchers to whether it has a matching allow or
-  // allowAllRequests rule. Used as a cache to prevent additional calls to
+  // A map from CompositeMatcher to the priority of its highest priority
+  // matching allow or allowAllRequests rule if there is one, or base::nullopt
+  // otherwise. Used as a cache to prevent additional calls to
   // GetBeforeRequestAction.
-  mutable base::flat_map<const RulesetMatcher*, bool> allow_rule_cache;
+  mutable base::flat_map<const CompositeMatcher*, base::Optional<uint64_t>>
+      allow_rule_max_priority;
 
   // Lower cased url, used for regex matching. Cached for performance.
   mutable base::Optional<std::string> lower_cased_url_spec;

extensions/browser/api/declarative_net_request/ruleset_matcher.cc

@@ -71,12 +71,13 @@ base::Optional<RequestAction> RulesetMatcher::GetBeforeRequestAction(
 }
 
 std::vector<RequestAction> RulesetMatcher::GetModifyHeadersActions(
-    const RequestParams& params) const {
+    const RequestParams& params,
+    base::Optional<uint64_t> min_priority) const {
   std::vector<RequestAction> modify_header_actions =
-      url_pattern_index_matcher_.GetModifyHeadersActions(params);
+      url_pattern_index_matcher_.GetModifyHeadersActions(params, min_priority);
 
   std::vector<RequestAction> regex_modify_header_actions =
-      regex_matcher_.GetModifyHeadersActions(params);
+      regex_matcher_.GetModifyHeadersActions(params, min_priority);
 
   modify_header_actions.insert(
       modify_header_actions.end(),

extensions/browser/api/declarative_net_request/ruleset_matcher.h

@@ -74,8 +74,12 @@ class RulesetMatcher {
 
   base::Optional<RequestAction> GetBeforeRequestAction(
       const RequestParams& params) const;
+
+  // Returns a list of actions corresponding to all matched
+  // modifyHeaders rules with priority greater than |min_priority| if specified.
   std::vector<RequestAction> GetModifyHeadersActions(
-      const RequestParams& params) const;
+      const RequestParams& params,
+      base::Optional<uint64_t> min_priority) const;
 
   bool IsExtraHeadersMatcher() const;
   size_t GetRulesCount() const;

extensions/browser/api/declarative_net_request/ruleset_matcher_base.h

@@ -41,9 +41,11 @@ class RulesetMatcherBase {
   base::Optional<RequestAction> GetBeforeRequestAction(
       const RequestParams& params) const;
 
-  // Returns a vector of RequestAction for all matching modifyHeaders rules.
+  // Returns a vector of RequestAction for all matching modifyHeaders rules
+  // with priority greater than |min_priority| if specified.
   virtual std::vector<RequestAction> GetModifyHeadersActions(
-      const RequestParams& params) const = 0;
+      const RequestParams& params,
+      base::Optional<uint64_t> min_priority) const = 0;
 
   // Returns whether this modifies "extraHeaders".
   virtual bool IsExtraHeadersMatcher() const = 0;

extensions/browser/api/declarative_net_request/ruleset_matcher_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "extensions/browser/api/declarative_net_request/ruleset_matcher.h"
 
+#include <limits>
 #include <utility>
 #include <vector>
 
@@ -233,7 +234,7 @@ TEST_F(RulesetMatcherTest, ModifyHeaders) {
   params.is_third_party = true;
 
   std::vector<RequestAction> modify_header_actions =
-      matcher->GetModifyHeadersActions(params);
+      matcher->GetModifyHeadersActions(params, 0u /* min_priority */);
 
   RequestAction expected_rule_1_action = CreateRequestActionForTesting(
       RequestAction::Type::MODIFY_HEADERS, *rule_1.id, *rule_1.priority);
@@ -584,7 +585,7 @@ TEST_F(RulesetMatcherTest, RegexRules) {
               matcher->GetBeforeRequestAction(params));
 
     std::vector<RequestAction> modify_header_actions =
-        matcher->GetModifyHeadersActions(params);
+        matcher->GetModifyHeadersActions(params, 0u /* min_priority */);
 
     if (test_case.expected_modify_header_action) {
       EXPECT_THAT(modify_header_actions,
@@ -910,7 +911,8 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) {
     RequestParams params;
     params.url = &url;
 
-    EXPECT_TRUE(matcher->GetModifyHeadersActions(params).empty());
+    EXPECT_TRUE(matcher->GetModifyHeadersActions(params, 0u /* min_priority */)
+                    .empty());
   }
 
   {
@@ -920,7 +922,7 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) {
     params.url = &url;
 
     std::vector<RequestAction> actions =
-        matcher->GetModifyHeadersActions(params);
+        matcher->GetModifyHeadersActions(params, 0u /* min_priority */);
     EXPECT_THAT(actions, testing::UnorderedElementsAre(
                              testing::Eq(testing::ByRef(action_1))));
   }
@@ -932,7 +934,7 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) {
     params.url = &url;
 
     std::vector<RequestAction> actions =
-        matcher->GetModifyHeadersActions(params);
+        matcher->GetModifyHeadersActions(params, 0u /* min_priority */);
     EXPECT_THAT(actions, testing::UnorderedElementsAre(
                              testing::Eq(testing::ByRef(action_2))));
   }
@@ -945,10 +947,18 @@ TEST_F(RulesetMatcherTest, RegexAndFilterListRules_ModifyHeaders) {
     params.url = &url;
 
     std::vector<RequestAction> actions =
-        matcher->GetModifyHeadersActions(params);
+        matcher->GetModifyHeadersActions(params, 0u /* min_priority */);
     EXPECT_THAT(actions, testing::UnorderedElementsAre(
                              testing::Eq(testing::ByRef(action_1)),
                              testing::Eq(testing::ByRef(action_2))));
+
+    // GetModifyHeadersActions specifies a minimum priority greater than the
+    // rules' priority, so no actions should be returned.
+    EXPECT_TRUE(
+        matcher
+            ->GetModifyHeadersActions(
+                params, std::numeric_limits<uint64_t>::max() /* min_priority */)
+            .empty());
   }
 }