Declarative Net Request: Limit the no. of rules that an extension can…

… provide.

This CL limits the no. of rules that an extension can provide to 30000. If an
extension provides more rules than this limit, those rules will be ignored and
an install warning will be surfaced to the user upon installation on the
extensions page. These won't re-surface once the browser restarts for packed
extensions.

BUG=767977

Change-Id: I95c824f4233dd7c0b77393fa817522b9b1c70681
Reviewed-on: https://chromium-review.googlesource.com/1198010
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588257}

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

@@ -20,6 +20,7 @@
 #include "extensions/browser/api/declarative_net_request/parse_info.h"
 #include "extensions/browser/api/declarative_net_request/test_utils.h"
 #include "extensions/browser/test_extension_registry_observer.h"
+#include "extensions/common/api/declarative_net_request.h"
 #include "extensions/common/api/declarative_net_request/test_utils.h"
 #include "extensions/common/file_util.h"
 #include "extensions/common/install_warning.h"
@@ -293,7 +294,7 @@ TEST_P(RuleIndexingTest, InvalidJSONRule) {
   extension_loader()->set_ignore_manifest_warnings(true);
   LoadAndExpectSuccess(1 /* rules count */);
 
-  // TODO(crbug.com/696822): CrxInstaller reloads the extension after moving it,
+  // TODO(crbug.com/879355): CrxInstaller reloads the extension after moving it,
   // which causes it to lose the install warning. This should be fixed.
   if (GetParam() != ExtensionLoadType::PACKED) {
     ASSERT_EQ(1u, extension()->install_warnings().size());
@@ -304,6 +305,42 @@ TEST_P(RuleIndexingTest, InvalidJSONRule) {
   }
 }
 
+// Ensure that we can add up to MAX_NUMBER_OF_RULES.
+TEST_P(RuleIndexingTest, RuleCountLimitMatched) {
+  namespace dnr_api = extensions::api::declarative_net_request;
+  TestRule rule = CreateGenericRule();
+  for (int i = 0; i < dnr_api::MAX_NUMBER_OF_RULES; ++i) {
+    rule.id = kMinValidID + i;
+    rule.condition->url_filter = std::to_string(i);
+    AddRule(rule);
+  }
+  LoadAndExpectSuccess(dnr_api::MAX_NUMBER_OF_RULES);
+}
+
+// Ensure that we get an install warning on exceeding the rule count limit.
+TEST_P(RuleIndexingTest, RuleCountLimitExceeded) {
+  namespace dnr_api = extensions::api::declarative_net_request;
+  TestRule rule = CreateGenericRule();
+  for (int i = 1; i <= dnr_api::MAX_NUMBER_OF_RULES + 1; ++i) {
+    rule.id = kMinValidID + i;
+    rule.condition->url_filter = std::to_string(i);
+    AddRule(rule);
+  }
+
+  extension_loader()->set_ignore_manifest_warnings(true);
+  LoadAndExpectSuccess(dnr_api::MAX_NUMBER_OF_RULES);
+
+  // TODO(crbug.com/879355): CrxInstaller reloads the extension after moving it,
+  // which causes it to lose the install warning. This should be fixed.
+  if (GetParam() != ExtensionLoadType::PACKED) {
+    ASSERT_EQ(1u, extension()->install_warnings().size());
+    EXPECT_EQ(InstallWarning(kRuleCountExceeded,
+                             manifest_keys::kDeclarativeNetRequestKey,
+                             manifest_keys::kDeclarativeRuleResourcesKey),
+              extension()->install_warnings()[0]);
+  }
+}
+
 TEST_P(RuleIndexingTest, InvalidJSONFile) {
   set_persist_invalid_json_file();
   // The error is returned by the JSON parser we use. Hence just test that an

extensions/browser/api/declarative_net_request/constants.cc

@@ -31,6 +31,9 @@ const char kErrorPersisting[] = "*: Rules file could not be parsed.";
 const char kErrorListNotPassed[] = "*: Rules file must contain a list.";
 const char kErrorNonAscii[] =
     "*: Rule at index * cannot have non-ascii characters as part of \"*\" key.";
+
+const char kRuleCountExceeded[] =
+    "Declarative Net Request: Rule count exceeded. Some rules were ignored.";
 const char kRulesNotParsedWarning[] =
     "Declarative Net Request: Not all rules were successfully parsed.";
 

extensions/browser/api/declarative_net_request/constants.h

@@ -47,7 +47,8 @@ extern const char kErrorPersisting[];
 extern const char kErrorListNotPassed[];
 extern const char kErrorNonAscii[];
 
-// Rule parsing install warnings.
+// Rule indexing install warnings.
+extern const char kRuleCountExceeded[];
 extern const char kRulesNotParsedWarning[];
 
 // Histogram names.

extensions/browser/api/declarative_net_request/utils.cc

@@ -153,7 +153,8 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
     return ParseInfo(ParseResult::ERROR_LIST_NOT_PASSED);
 
   FlatRulesetIndexer indexer;
-  bool all_rules_parsed = true;
+  base::Optional<InstallWarning> install_warning;
+  const size_t rule_count_limit = dnr_api::MAX_NUMBER_OF_RULES;
   base::ElapsedTimer timer;
   {
     std::set<int> id_set;  // Ensure all ids are distinct.
@@ -165,8 +166,8 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
 
       // Ignore rules which can't be successfully parsed and show an install
       // warning for them.
-      if (!parsed_rule) {
-        all_rules_parsed = false;
+      if (!parsed_rule && !install_warning) {
+        install_warning = InstallWarning(kRulesNotParsedWarning);
         continue;
       }
 
@@ -180,6 +181,11 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
       if (parse_result != ParseResult::SUCCESS)
         return ParseInfo(parse_result, i);
 
+      if (indexer.indexed_rules_count() >= rule_count_limit) {
+        install_warning = InstallWarning(kRuleCountExceeded);
+        break;
+      }
+
       indexer.AddUrlRule(indexed_rule);
     }
   }
@@ -189,10 +195,10 @@ ParseInfo IndexAndPersistRulesImpl(const base::Value& rules,
   if (!PersistRuleset(extension, indexer.GetData(), ruleset_checksum))
     return ParseInfo(ParseResult::ERROR_PERSISTING_RULESET);
 
-  if (!all_rules_parsed) {
-    warnings->push_back(InstallWarning(
-        kRulesNotParsedWarning, manifest_keys::kDeclarativeNetRequestKey,
-        manifest_keys::kDeclarativeRuleResourcesKey));
+  if (install_warning) {
+    install_warning->key = manifest_keys::kDeclarativeNetRequestKey;
+    install_warning->specific = manifest_keys::kDeclarativeRuleResourcesKey;
+    warnings->push_back(*install_warning);
   }
 
   UMA_HISTOGRAM_TIMES(kIndexAndPersistRulesTimeHistogram, timer.Elapsed());

extensions/common/api/declarative_net_request.idl

@@ -155,5 +155,9 @@ namespace declarativeNetRequest {
   interface Properties {
     // The maximum number of allowed pages that an extension can add.
     [value=100] static long MAX_NUMBER_OF_ALLOWED_PAGES();
+
+    // The maximum number of rules that an extension can specify in the rule
+    // resources file. Any excess rules will be ignored.
+    [value=30000] static long MAX_NUMBER_OF_RULES();
   };
 };