[Declarative net request] Allow rules now override other rule types
This CL modifies the behavior of an extension ruleset such that if a
request matches with an |allow| rule for an extension, it will override
the effects of other matching rules for the request from the extension's
ruleset.
Skipping presubmit as the idl parser fails weirdly as seen in
crbug.com/956368
NOPRESUBMIT=true
Bug: 953382
Change-Id: If7571208cda4088d8314b14741dfdd35bc2da2e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1589059
Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#660178}
diff --git a/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc b/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc
index 2230518..38ba6db 100644
--- a/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc
+++ b/chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc
@@ -871,8 +871,8 @@
}
}
-// Tests allowing rules.
-IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest, Allow) {
+// Tests allowing rules for blocks.
+IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest, AllowBlock) {
const int kNumRequests = 6;
TestRule rule = CreateGenericRule();
@@ -916,6 +916,103 @@
}
}
+// Tests allowing rules for redirects.
+IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest, AllowRedirect) {
+ set_has_background_script(true);
+
+ const GURL static_redirect_url = embedded_test_server()->GetURL(
+ "example.com", base::StringPrintf("/pages_with_script/page2.html"));
+
+ const GURL dynamic_redirect_url = embedded_test_server()->GetURL(
+ "abc.com", base::StringPrintf("/pages_with_script/page.html"));
+
+ // Create 2 static and 2 dynamic rules.
+ struct {
+ std::string url_filter;
+ int id;
+ std::string action_type;
+ base::Optional<std::string> redirect_url;
+ } rules_data[] = {
+ {"google.com", 1, "redirect", static_redirect_url.spec()},
+ {"num=1|", 2, "allow", base::nullopt},
+ {"1|", 3, "redirect", dynamic_redirect_url.spec()},
+ {"num=21|", 4, "allow", base::nullopt},
+ };
+
+ std::vector<TestRule> rules;
+ for (const auto& rule_data : rules_data) {
+ TestRule rule = CreateGenericRule();
+ rule.id = rule_data.id;
+ rule.priority = kMinValidPriority;
+ rule.condition->url_filter = rule_data.url_filter;
+ rule.condition->resource_types = std::vector<std::string>({"main_frame"});
+ rule.action->type = rule_data.action_type;
+ rule.action->redirect_url = rule_data.redirect_url;
+ rules.push_back(rule);
+ }
+
+ std::vector<TestRule> static_rules;
+ static_rules.push_back(rules[0]);
+ static_rules.push_back(rules[1]);
+
+ std::vector<TestRule> dynamic_rules;
+ dynamic_rules.push_back(rules[2]);
+ dynamic_rules.push_back(rules[3]);
+
+ ASSERT_NO_FATAL_FAILURE(LoadExtensionWithRules(
+ static_rules, "test_extension", {URLPattern::kAllUrlsPattern}));
+
+ auto get_url = [this](int i) {
+ return embedded_test_server()->GetURL(
+ "google.com",
+ base::StringPrintf("/pages_with_script/page.html?num=%d", i));
+ };
+
+ struct {
+ GURL initial_url;
+ GURL expected_final_url;
+ } static_test_cases[] = {
+ {get_url(0), static_redirect_url},
+ {get_url(1), get_url(1)},
+ };
+
+ for (const auto& test_case : static_test_cases) {
+ GURL url = test_case.initial_url;
+ SCOPED_TRACE(base::StringPrintf("Testing %s", url.spec().c_str()));
+
+ ui_test_utils::NavigateToURL(browser(), url);
+ EXPECT_TRUE(WasFrameWithScriptLoaded(GetMainFrame()));
+
+ GURL final_url = web_contents()->GetLastCommittedURL();
+ EXPECT_EQ(test_case.expected_final_url, final_url);
+ }
+
+ // Now add dynamic rules. These should override static rules in priority.
+ const ExtensionId& extension_id = last_loaded_extension_id();
+ ASSERT_NO_FATAL_FAILURE(AddDynamicRules(extension_id, dynamic_rules));
+
+ // Test that rules follow the priority of: dynamic allow > dynamic redirect >
+ // static allow > static redirect.
+ struct {
+ GURL initial_url;
+ GURL expected_final_url;
+ } dynamic_test_cases[] = {
+ {get_url(1), dynamic_redirect_url},
+ {get_url(21), get_url(21)},
+ };
+
+ for (const auto& test_case : dynamic_test_cases) {
+ GURL url = test_case.initial_url;
+ SCOPED_TRACE(base::StringPrintf("Testing %s", url.spec().c_str()));
+
+ ui_test_utils::NavigateToURL(browser(), url);
+ EXPECT_TRUE(WasFrameWithScriptLoaded(GetMainFrame()));
+
+ GURL final_url = web_contents()->GetLastCommittedURL();
+ EXPECT_EQ(test_case.expected_final_url, final_url);
+ }
+}
+
// Tests that the extension ruleset is active only when the extension is
// enabled.
IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest,
diff --git a/chrome/common/extensions/docs/templates/intros/declarativeNetRequest.html b/chrome/common/extensions/docs/templates/intros/declarativeNetRequest.html
index c9748d6..78720ad 100644
--- a/chrome/common/extensions/docs/templates/intros/declarativeNetRequest.html
+++ b/chrome/common/extensions/docs/templates/intros/declarativeNetRequest.html
@@ -216,7 +216,10 @@
[2] An extension wants to redirect a request, if all the following are true:
<ul>
- <li>The extension has a matching <code>redirect</code> rule.</li>
+ <li>
+ The extension has a matching redirect rule. Note the priority order
+ between the different redirect/allow actions: <br/>dynamic allow > dynamic redirect > static allow > static redirect.
+ </li>
<li>
The page from which the request originated is not allow-listed by the
extension using <a href="#method-addAllowedPages">
@@ -227,7 +230,10 @@
[3] An extension wants to remove a header from a request, if all the following
are true:
<ul>
- <li>The extension has a matching <code>removeHeaders</code> rule.</li>
+ <li>
+ The extension has a matching removeHeaders rule. Note the priority order
+ between the different removeHeaders/allow actions: <br/>dynamic allow > dynamic removeHeaders > static allow > static removeHeaders.
+ </li>
<li>
The page from which the request originated is not allow-listed by the
extension using <a href="#method-addAllowedPages">
@@ -328,8 +334,8 @@
<li>
Consider a navigation to <code>"http://google.com/1234"</code>. Rules with
id (1), (2), and (3) match. Because the request has a matching
- <code>allow</code> rule, the request is not blocked and instead redirected
- to <code>"https://example.com"</code>.
+ <code>allow</code> rule, the request is not blocked nor redirected and
+ continues to <code>"http://google.com/1234"</code>.
</li>
<li>
Consider a navigation to <code>"http://abcd.com"</code>. The rule with id
diff --git a/chrome/test/data/extensions/api_test/declarative_net_request/header_removal/background.js b/chrome/test/data/extensions/api_test/declarative_net_request/header_removal/background.js
index de6ac8e..b1747bf 100644
--- a/chrome/test/data/extensions/api_test/declarative_net_request/header_removal/background.js
+++ b/chrome/test/data/extensions/api_test/declarative_net_request/header_removal/background.js
@@ -227,6 +227,15 @@
});
}
+// Clears the current state by removing rules specified in |ruleIds| and
+// clearing all cookies.
+function clearState(ruleIds, callback) {
+ chrome.declarativeNetRequest.removeDynamicRules(ruleIds, function() {
+ chrome.test.assertNoLastError();
+ checkAndResetCookies().then(callback);
+ });
+}
+
var removeCookieRule = {
id: 1,
condition: {urlFilter: host, resourceTypes: ['main_frame']},
@@ -237,6 +246,11 @@
condition: {urlFilter: host, resourceTypes: ['main_frame']},
action: {type: 'removeHeaders', removeHeadersList: ['setCookie']}
};
+var allowRule = {
+ id: 3,
+ condition: {urlFilter: host, resourceTypes: ['main_frame']},
+ action: {type: 'allow'}
+};
var tests = [
function testCookieWithoutRules() {
@@ -265,15 +279,11 @@
});
},
- function clearState() {
- chrome.declarativeNetRequest.removeDynamicRules([1, 2], function() {
- chrome.test.assertNoLastError();
- checkAndResetCookies().then(chrome.test.succeed);
- });
- },
-
function testAddWebRequestCookie() {
- checkAddWebRequestCookie(false);
+ // First clear the rules and cookies.
+ clearState([1, 2], () => {
+ checkAddWebRequestCookie(false);
+ });
},
function testAddWebRequestCookieWithRules() {
@@ -292,7 +302,13 @@
chrome.declarativeNetRequest.addDynamicRules(rules, function() {
checkAddWebRequestSetCookie(true);
});
- }
+ },
+
+ function testAddWebRequestCookieWithAllowRule() {
+ chrome.declarativeNetRequest.addDynamicRules([allowRule], () => {
+ checkAddWebRequestSetCookie(false);
+ });
+ },
];
chrome.test.getConfig(function(config) {
diff --git a/extensions/browser/api/declarative_net_request/composite_matcher.cc b/extensions/browser/api/declarative_net_request/composite_matcher.cc
index bae2d20..c8c5833 100644
--- a/extensions/browser/api/declarative_net_request/composite_matcher.cc
+++ b/extensions/browser/api/declarative_net_request/composite_matcher.cc
@@ -38,6 +38,14 @@
return true;
}
+bool HasMatchingAllowRule(const RulesetMatcher* matcher,
+ const RequestParams& params) {
+ if (!params.allow_rule_cache.contains(matcher))
+ params.allow_rule_cache[matcher] = matcher->HasMatchingAllowRule(params);
+
+ return params.allow_rule_cache[matcher];
+}
+
} // namespace
CompositeMatcher::CompositeMatcher(MatcherList matchers)
@@ -80,11 +88,12 @@
"SingleExtension");
for (const auto& matcher : matchers_) {
- if (matcher->HasMatchingAllowRule(params))
+ if (HasMatchingAllowRule(matcher.get(), params))
return false;
if (matcher->HasMatchingBlockRule(params))
return true;
}
+
return false;
}
@@ -96,6 +105,8 @@
"SingleExtension");
for (const auto& matcher : matchers_) {
+ if (HasMatchingAllowRule(matcher.get(), params))
+ return false;
if (matcher->HasMatchingRedirectRule(params, redirect_url))
return true;
}
@@ -106,8 +117,12 @@
uint8_t CompositeMatcher::GetRemoveHeadersMask(const RequestParams& params,
uint8_t current_mask) const {
uint8_t mask = current_mask;
- for (const auto& matcher : matchers_)
+ for (const auto& matcher : matchers_) {
+ // The allow rule will override lower priority remove header rules.
+ if (HasMatchingAllowRule(matcher.get(), params))
+ return mask;
mask |= matcher->GetRemoveHeadersMask(params, mask);
+ }
return mask;
}
diff --git a/extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc b/extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc
index d4759ca..a9cf17f 100644
--- a/extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc
+++ b/extensions/browser/api/declarative_net_request/composite_matcher_unittest.cc
@@ -9,6 +9,7 @@
#include <vector>
#include "components/version_info/version_info.h"
+#include "extensions/browser/api/declarative_net_request/constants.h"
#include "extensions/browser/api/declarative_net_request/ruleset_matcher.h"
#include "extensions/browser/api/declarative_net_request/ruleset_source.h"
#include "extensions/browser/api/declarative_net_request/test_utils.h"
@@ -82,8 +83,11 @@
EXPECT_FALSE(composite_matcher->ShouldBlockRequest(google_params));
GURL example_url = GURL("http://example.com");
- RequestParams example_params = google_params;
+ RequestParams example_params;
example_params.url = &example_url;
+ example_params.element_type =
+ url_pattern_index::flat::ElementType_SUBDOCUMENT;
+ example_params.is_third_party = false;
GURL redirect_url;
EXPECT_TRUE(
composite_matcher->ShouldRedirectRequest(example_params, &redirect_url));
@@ -104,6 +108,10 @@
matchers.push_back(std::move(matcher_2));
composite_matcher = std::make_unique<CompositeMatcher>(std::move(matchers));
+ // Reusing request params means that their allow_rule_caches must be cleared.
+ google_params.allow_rule_cache.clear();
+ example_params.allow_rule_cache.clear();
+
// The first ruleset should get more priority.
EXPECT_TRUE(composite_matcher->ShouldBlockRequest(google_params));
EXPECT_TRUE(
@@ -111,5 +119,113 @@
EXPECT_EQ(GURL("http://ruleset1.com"), redirect_url);
}
+// Ensure allow rules in a higher priority matcher override redirect
+// and removeHeader rules from lower priority matchers.
+TEST_F(CompositeMatcherTest, AllowRuleOverrides) {
+ TestRule allow_rule_1 = CreateGenericRule();
+ allow_rule_1.id = kMinValidID;
+ allow_rule_1.condition->url_filter = std::string("google.com");
+ allow_rule_1.action->type = std::string("allow");
+
+ TestRule remove_headers_rule_1 = CreateGenericRule();
+ remove_headers_rule_1.id = kMinValidID + 1;
+ remove_headers_rule_1.condition->url_filter = std::string("example.com");
+ remove_headers_rule_1.action->type = std::string("removeHeaders");
+ remove_headers_rule_1.action->remove_headers_list =
+ std::vector<std::string>({"referer", "setCookie"});
+
+ // Create the first ruleset matcher, which allows requests to google.com and
+ // removes headers from requests to example.com.
+ const size_t kSource1ID = 1;
+ const size_t kSource1Priority = 1;
+ std::unique_ptr<RulesetMatcher> matcher_1;
+ ASSERT_TRUE(CreateVerifiedMatcher(
+ {allow_rule_1, remove_headers_rule_1},
+ CreateTemporarySource(kSource1ID, kSource1Priority), &matcher_1));
+
+ // Now set up rules and the second matcher.
+ TestRule allow_rule_2 = allow_rule_1;
+ allow_rule_2.condition->url_filter = std::string("example.com");
+
+ TestRule redirect_rule_2 = CreateGenericRule();
+ redirect_rule_2.condition->url_filter = std::string("google.com");
+ redirect_rule_2.priority = kMinValidPriority;
+ redirect_rule_2.action->type = std::string("redirect");
+ redirect_rule_2.action->redirect_url = std::string("http://ruleset2.com");
+ redirect_rule_2.id = kMinValidID + 1;
+
+ // Create a second ruleset matcher, which allows requests to example.com and
+ // redirects requests to google.com.
+ const size_t kSource2ID = 2;
+ const size_t kSource2Priority = 2;
+ std::unique_ptr<RulesetMatcher> matcher_2;
+ ASSERT_TRUE(CreateVerifiedMatcher(
+ {allow_rule_2, redirect_rule_2},
+ CreateTemporarySource(kSource2ID, kSource2Priority), &matcher_2));
+
+ // Create a composite matcher with the two 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));
+
+ // Send a request to google.com which should be redirected.
+ GURL google_url = GURL("http://google.com");
+ RequestParams google_params;
+ google_params.url = &google_url;
+ google_params.element_type = url_pattern_index::flat::ElementType_SUBDOCUMENT;
+ google_params.is_third_party = false;
+ GURL redirect_url;
+
+ // The second ruleset should get more priority.
+ EXPECT_TRUE(
+ composite_matcher->ShouldRedirectRequest(google_params, &redirect_url));
+ EXPECT_EQ(GURL("http://ruleset2.com"), redirect_url);
+
+ // Send a request to example.com with headers, expect the allow rule to be
+ // matched and the headers to remain.
+ GURL example_url = GURL("http://example.com");
+ RequestParams example_params;
+ example_params.url = &example_url;
+ example_params.element_type =
+ url_pattern_index::flat::ElementType_SUBDOCUMENT;
+ example_params.is_third_party = false;
+
+ // Expect no headers to be removed.
+ EXPECT_EQ(0u, composite_matcher->GetRemoveHeadersMask(example_params, 0u));
+
+ // Now switch the priority of the two rulesets. This requires re-constructing
+ // the two ruleset matchers.
+ matcher_1.reset();
+ matcher_2.reset();
+ matchers.clear();
+ ASSERT_TRUE(CreateVerifiedMatcher(
+ {allow_rule_1, remove_headers_rule_1},
+ CreateTemporarySource(kSource1ID, kSource2Priority), &matcher_1));
+ ASSERT_TRUE(CreateVerifiedMatcher(
+ {allow_rule_2, redirect_rule_2},
+ CreateTemporarySource(kSource2ID, kSource1Priority), &matcher_2));
+ matchers.push_back(std::move(matcher_1));
+ matchers.push_back(std::move(matcher_2));
+ composite_matcher = std::make_unique<CompositeMatcher>(std::move(matchers));
+
+ // Reusing request params means that their allow_rule_caches must be cleared.
+ google_params.allow_rule_cache.clear();
+ example_params.allow_rule_cache.clear();
+
+ // The first ruleset should get more priority and so the request to google.com
+ // should not be redirected.
+ EXPECT_FALSE(
+ composite_matcher->ShouldRedirectRequest(google_params, &redirect_url));
+
+ // The request to example.com should now have its headers removed.
+ example_params.allow_rule_cache.clear();
+ uint8_t expected_mask =
+ kRemoveHeadersMask_Referer | kRemoveHeadersMask_SetCookie;
+ EXPECT_EQ(expected_mask,
+ composite_matcher->GetRemoveHeadersMask(example_params, 0u));
+}
+
} // namespace declarative_net_request
} // namespace extensions
diff --git a/extensions/browser/api/declarative_net_request/ruleset_matcher.cc b/extensions/browser/api/declarative_net_request/ruleset_matcher.cc
index 0bf27766..780cae8 100644
--- a/extensions/browser/api/declarative_net_request/ruleset_matcher.cc
+++ b/extensions/browser/api/declarative_net_request/ruleset_matcher.cc
@@ -153,6 +153,7 @@
}
RequestParams::RequestParams() = default;
+RequestParams::~RequestParams() = default;
// static
RulesetMatcher::LoadRulesetResult RulesetMatcher::CreateVerifiedMatcher(
diff --git a/extensions/browser/api/declarative_net_request/ruleset_matcher.h b/extensions/browser/api/declarative_net_request/ruleset_matcher.h
index 8031e74..3bacbce 100644
--- a/extensions/browser/api/declarative_net_request/ruleset_matcher.h
+++ b/extensions/browser/api/declarative_net_request/ruleset_matcher.h
@@ -10,6 +10,7 @@
#include <string>
#include <vector>
+#include "base/containers/flat_map.h"
#include "components/url_pattern_index/url_pattern_index.h"
#include "extensions/browser/api/declarative_net_request/flat/extension_ruleset_generated.h"
#include "url/gurl.h"
@@ -26,17 +27,26 @@
struct UrlRuleMetadata;
} // namespace flat
+class RulesetMatcher;
+
// Struct to hold parameters for a network request.
struct RequestParams {
// |info| must outlive this instance.
explicit RequestParams(const WebRequestInfo& info);
RequestParams();
+ ~RequestParams();
// This is a pointer to a GURL. Hence the GURL must outlive this struct.
const GURL* url = nullptr;
url::Origin first_party_origin;
url_pattern_index::flat::ElementType element_type;
bool is_third_party;
+
+ // A map of RulesetMatchers to results of |HasMatchingAllowRule| for this
+ // request. Used as a cache to prevent extra calls to |HasMatchingAllowRule|.
+ mutable base::flat_map<const RulesetMatcher*, bool> allow_rule_cache;
+
+ DISALLOW_COPY_AND_ASSIGN(RequestParams);
};
// RulesetMatcher encapsulates the Declarative Net Request API ruleset
diff --git a/extensions/common/api/declarative_net_request.idl b/extensions/common/api/declarative_net_request.idl
index 5da6d46f..d58ed64 100644
--- a/extensions/common/api/declarative_net_request.idl
+++ b/extensions/common/api/declarative_net_request.idl
@@ -46,8 +46,8 @@
block,
// Redirect the network request.
redirect,
- // Allow the network request. The request won't be blocked even if there
- // is a blocking rule which matches it.
+ // Allow the network request. The request won't be intercepted if there is
+ // an allow rule which matches it.
allow,
// Remove request/response headers from the network request.
removeHeaders
