[DNR] Improve request method rule conditions for non-HTTP(s) requests

We previously added HTTP request method based matching for
declarativeNetRequest rules, but non-HTTP(s) requests were treated as
having the GET request method. With this change, we instead consider
non-HTTP(s) requests to have a special NON_HTTP request method. The
result is that rules with a requestMethods condition will no longer
match non-HTTP(s) requests. Rules with an excludedRequestMethods
condition will continue to match non-HTTP(s) requests, even if every
HTTP request method is excluded.

Bug: 1013583
Change-Id: Ib140894cba86e7fd59861fe2594c2b449ae4fdeb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2940865
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Kelvin Jiang <kelvinjiang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#893014}

chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc

@@ -5924,6 +5924,92 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest,
   }
 }
 
+// Tests that the "requestMethods" and "excludedRequestMethods" properties of a
+// rule condition are considered properly for non-HTTP(s) requests.
+IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest,
+                       BlockRequests_NonHTTPMethods) {
+  // Load an extension with some DNR rules that have different request method
+  // conditions.
+  std::vector<TestRule> rules;
+
+  TestRule rule1 = CreateGenericRule(1);
+  rule1.condition->url_filter = "default";
+  rules.push_back(rule1);
+
+  TestRule rule2 = CreateGenericRule(2);
+  rule2.condition->url_filter = "all_methods";
+  rule2.condition->request_methods = {"delete", "get",  "head", "options",
+                                      "patch",  "post", "put"};
+  rules.push_back(rule2);
+
+  TestRule rule3 = CreateGenericRule(3);
+  rule3.condition->url_filter = "some_methods";
+  rule3.condition->request_methods = {"get", "put"};
+  rules.push_back(rule3);
+
+  TestRule rule4 = CreateGenericRule(4);
+  rule4.condition->url_filter = "all_methods_excluded";
+  rule4.condition->excluded_request_methods = {
+      "delete", "get", "head", "options", "patch", "post", "put"};
+  rules.push_back(rule4);
+
+  TestRule rule5 = CreateGenericRule(5);
+  rule5.condition->url_filter = "some_methods_excluded";
+  rule5.condition->excluded_request_methods = {"get", "put"};
+  rules.push_back(rule5);
+
+  ASSERT_NO_FATAL_FAILURE(LoadExtensionWithRules(rules));
+
+  // Start a web socket test server.
+  net::SpawnedTestServer websocket_test_server(
+      net::SpawnedTestServer::TYPE_WS, net::GetWebSocketTestDataDirectory());
+  ASSERT_TRUE(websocket_test_server.Start());
+  std::string websocket_url =
+      websocket_test_server.GetURL("echo-with-no-extension").spec();
+
+  const char kOpenWebSocketsScript[] = R"(
+    {
+      const websocketUrl = "%s";
+      const testCases = ["default", "all_methods", "some_methods",
+                         "all_methods_excluded", "some_methods_excluded"];
+
+      let blockedTestCases = [];
+
+      Promise.allSettled(
+        testCases.map(testCase =>
+          new Promise(resolve =>
+          {
+            let websocket = new WebSocket(websocketUrl + "?" + testCase);
+            websocket.addEventListener("open", event =>
+            {
+              websocket.close();
+              resolve();
+            });
+            websocket.addEventListener("error", event =>
+            {
+              blockedTestCases.push(testCase);
+              resolve();
+            });
+          })
+        )
+      ).then(() =>
+      {
+        window.domAutomationController.send(blockedTestCases.sort().join());
+      });
+    }
+  )";
+
+  content::RenderFrameHost* main_frame = GetMainFrame();
+
+  std::string actual_blocked;
+  EXPECT_TRUE(content::ExecuteScriptAndExtractString(
+      main_frame,
+      base::StringPrintf(kOpenWebSocketsScript, websocket_url.c_str()),
+      &actual_blocked));
+  EXPECT_EQ("all_methods_excluded,default,some_methods_excluded",
+            actual_blocked);
+}
+
 // Tests that FLEDGE requests can be blocked by the declarativeNetRequest API,
 // and that if they try to redirect requests, the request is blocked, instead of
 // being redirected.

components/subresource_filter/core/common/indexed_ruleset.cc

@@ -54,12 +54,12 @@ VerifyStatus GetVerifyStatus(const uint8_t* buffer,
 
 // RulesetIndexer --------------------------------------------------------------
 
-const int RulesetIndexer::kIndexedFormatVersion = 30;
+const int RulesetIndexer::kIndexedFormatVersion = 31;
 
 // This static assert is meant to catch cases where
 // url_pattern_index::kUrlPatternIndexFormatVersion is incremented without
 // updating RulesetIndexer::kIndexedFormatVersion.
-static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 9,
+static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 10,
               "kUrlPatternIndexFormatVersion has changed, make sure you've "
               "also updated RulesetIndexer::kIndexedFormatVersion above.");
 

components/url_pattern_index/flat/url_pattern_index.fbs

@@ -70,6 +70,7 @@ enum RequestMethod : ubyte (bit_flags) {
   PATCH,
   POST,
   PUT,
+  NON_HTTP
   // Note: Update the default value for `request_methods` field in UrlRule, on
   // adding/removing values from this enum.
 }
@@ -87,7 +88,7 @@ table UrlRule {
   element_types : ushort = 8191;
 
   // A bitmask of RequestMethod. Equals RequestMethod_ANY by default.
-  request_methods : ushort = 127;
+  request_methods : ushort = 255;
 
   // A bitmask of ActivationType. Disables all activation types by default.
   activation_types : ubyte = 0;

components/url_pattern_index/url_pattern_index.h

@@ -85,7 +85,7 @@ int CompareDomains(base::StringPiece lhs_domain, base::StringPiece rhs_domain);
 // Increase this value when introducing an incompatible change to the
 // UrlPatternIndex schema (flat/url_pattern_index.fbs). url_pattern_index
 // clients can use this as a signal to rebuild rulesets.
-constexpr int kUrlPatternIndexFormatVersion = 9;
+constexpr int kUrlPatternIndexFormatVersion = 10;
 
 // The class used to construct an index over the URL patterns of a set of URL
 // rules. The rules themselves need to be converted to FlatBuffers format by the

extensions/browser/api/declarative_net_request/indexed_ruleset_format_version_unittest.cc

@@ -153,7 +153,7 @@ TEST_F(IndexedRulesetFormatVersionTest, CheckVersionUpdated) {
   EXPECT_EQ(StripCommentsAndWhitespace(kFlatbufferSchemaExpected),
             StripCommentsAndWhitespace(flatbuffer_schema))
       << "Schema change detected; update this test and the schema version.";
-  EXPECT_EQ(21, GetIndexedRulesetFormatVersionForTesting())
+  EXPECT_EQ(22, GetIndexedRulesetFormatVersionForTesting())
       << "Update this test if you update the schema version.";
 }
 

extensions/browser/api/declarative_net_request/request_params.cc

@@ -65,8 +65,12 @@ flat_rule::ElementType GetElementType(WebRequestResourceType web_request_type) {
 }
 
 // Maps an HTTP request method string to flat_rule::RequestMethod.
-// TODO(kzar): Return `flat_rule::RequestMethod_NONE` for non-HTTP requests.
+// Returns `flat::RequestMethod_NON_HTTP` for non-HTTP(s) requests.
-flat_rule::RequestMethod GetRequestMethod(const std::string& method) {
+flat_rule::RequestMethod GetRequestMethod(bool http_or_https,
+                                          const std::string& method) {
+  if (!http_or_https)
+    return flat_rule::RequestMethod_NON_HTTP;
+
   using net::HttpRequestHeaders;
   static const base::NoDestructor<
       base::flat_map<base::StringPiece, flat_rule::RequestMethod>>
@@ -173,7 +177,7 @@ RequestParams::RequestParams(const WebRequestInfo& info)
     : url(&info.url),
       first_party_origin(info.initiator.value_or(url::Origin())),
       element_type(GetElementType(info.web_request_type)),
-      method(GetRequestMethod(info.method)),
+      method(GetRequestMethod(info.url.SchemeIsHTTPOrHTTPS(), info.method)),
       parent_routing_id(info.parent_routing_id),
       embedder_conditions_matcher(base::BindRepeating(DoEmbedderConditionsMatch,
                                                       info.frame_data.tab_id)) {

extensions/browser/api/declarative_net_request/utils.cc

@@ -43,12 +43,12 @@ namespace dnr_api = api::declarative_net_request;
 // url_pattern_index.fbs. Whenever an extension with an indexed ruleset format
 // version different from the one currently used by Chrome is loaded, the
 // extension ruleset will be reindexed.
-constexpr int kIndexedRulesetFormatVersion = 21;
+constexpr int kIndexedRulesetFormatVersion = 22;
 
 // This static assert is meant to catch cases where
 // url_pattern_index::kUrlPatternIndexFormatVersion is incremented without
 // updating kIndexedRulesetFormatVersion.
-static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 9,
+static_assert(url_pattern_index::kUrlPatternIndexFormatVersion == 10,
               "kUrlPatternIndexFormatVersion has changed, make sure you've "
               "also updated kIndexedRulesetFormatVersion above.");
 

extensions/common/api/declarative_net_request.idl

@@ -261,7 +261,10 @@ namespace declarativeNetRequest {
 
     // List of HTTP request methods which the rule can match. An empty list is
     // not allowed.
-    // Note: Non HTTP requests are considered to have the "get" request method.
+    //
+    // Note: Specifying a <code>requestMethods</code> rule condition will also
+    // exclude non-HTTP(s) requests, whereas specifying
+    // <code>excludedRequestMethods</code> will not.
     RequestMethod[]? requestMethods;
 
     // List of request methods which the rule won't match. Only one of