[ProxyBypassRules] Move suffix matching outside of ProxyBypassRules

Currently ProxyBypassRules handles suffix matching directly, this is a
platform special case on Linux only, move the logic to the platform
specific code other than handling it in a general place.

Bug: 1030092
Change-Id: I5c30326dc4c88f0067e3369f190c83503f54db24
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2025834
Commit-Queue: Shimi Zhang <ctzsm@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737164}

net/base/scheme_host_port_matcher_rule.cc

@@ -21,6 +21,10 @@ std::string AddBracketsIfIPv6(const IPAddress& ip_address) {
 
 }  // namespace
 
+bool SchemeHostPortMatcherRule::IsHostnamePatternRule() const {
+  return false;
+}
+
 SchemeHostPortMatcherHostnamePatternRule::
     SchemeHostPortMatcherHostnamePatternRule(
         const std::string& optional_scheme,
@@ -62,6 +66,21 @@ std::string SchemeHostPortMatcherHostnamePatternRule::ToString() const {
   return str;
 }
 
+bool SchemeHostPortMatcherHostnamePatternRule::IsHostnamePatternRule() const {
+  return true;
+}
+
+std::unique_ptr<SchemeHostPortMatcherHostnamePatternRule>
+SchemeHostPortMatcherHostnamePatternRule::GenerateSuffixMatchingRule() const {
+  if (!base::StartsWith(hostname_pattern_, "*", base::CompareCase::SENSITIVE)) {
+    return std::make_unique<SchemeHostPortMatcherHostnamePatternRule>(
+        optional_scheme_, "*" + hostname_pattern_, optional_port_);
+  }
+  // return a new SchemeHostPortMatcherHostNamePatternRule with the same data.
+  return std::make_unique<SchemeHostPortMatcherHostnamePatternRule>(
+      optional_scheme_, hostname_pattern_, optional_port_);
+}
+
 SchemeHostPortMatcherIPHostRule::SchemeHostPortMatcherIPHostRule(
     const std::string& optional_scheme,
     const IPEndPoint& ip_end_point)

net/base/scheme_host_port_matcher_rule.h

@@ -29,6 +29,9 @@ class NET_EXPORT SchemeHostPortMatcherRule {
   // Returns a string representation of this rule. The returned string will not
   // match any distinguishable rule of any type.
   virtual std::string ToString() const = 0;
+  // Returns true if |this| is an instance of
+  // SchemeHostPortMatcherHostnamePatternRule.
+  virtual bool IsHostnamePatternRule() const;
 };
 
 // Rule that matches URLs with wildcard hostname patterns, and
@@ -52,6 +55,14 @@ class NET_EXPORT SchemeHostPortMatcherHostnamePatternRule
   // SchemeHostPortMatcherRule implementation:
   SchemeHostPortMatcherResult Evaluate(const GURL& url) const override;
   std::string ToString() const override;
+  bool IsHostnamePatternRule() const override;
+
+  // Generates a new SchemeHostPortMatcherHostnamePatternRule based on the
+  // current rule. The new rule will do suffix matching if the current rule
+  // doesn't. For example, "google.com" would become "*google.com" and match
+  // "foogoogle.com".
+  std::unique_ptr<SchemeHostPortMatcherHostnamePatternRule>
+  GenerateSuffixMatchingRule() const;
 
  private:
   const std::string optional_scheme_;

net/base/scheme_host_port_matcher_rule_unittest.cc

@@ -174,6 +174,61 @@ TEST(SchemeHostPortMatcherRuleTest,
             rule->Evaluate(GURL("http://www.谷歌.cn")));
 }
 
+TEST(SchemeHostPortMatcherRuleTest, SuffixMatchingTest) {
+  // foo1.com, suffix matching rule will match www.foo1.com but the original one
+  // doesn't.
+  SchemeHostPortMatcherHostnamePatternRule rule1("", "foo1.com", -1);
+  std::unique_ptr<SchemeHostPortMatcherHostnamePatternRule>
+      suffix_matching_rule = rule1.GenerateSuffixMatchingRule();
+  EXPECT_EQ("foo1.com", rule1.ToString());
+  EXPECT_EQ("*foo1.com", suffix_matching_rule->ToString());
+  EXPECT_EQ(SchemeHostPortMatcherResult::kNoMatch,
+            rule1.Evaluate(GURL("http://www.foo1.com")));
+  EXPECT_EQ(SchemeHostPortMatcherResult::kInclude,
+            suffix_matching_rule->Evaluate(GURL("http://www.foo1.com")));
+
+  // .foo2.com, suffix matching rule will match www.foo2.com but the original
+  // one doesn't.
+  SchemeHostPortMatcherHostnamePatternRule rule2("", ".foo2.com", -1);
+  suffix_matching_rule = rule2.GenerateSuffixMatchingRule();
+  EXPECT_EQ(".foo2.com", rule2.ToString());
+  EXPECT_EQ("*.foo2.com", suffix_matching_rule->ToString());
+  EXPECT_EQ(SchemeHostPortMatcherResult::kNoMatch,
+            rule2.Evaluate(GURL("http://www.foo2.com")));
+  EXPECT_EQ(SchemeHostPortMatcherResult::kInclude,
+            suffix_matching_rule->Evaluate(GURL("http://www.foo2.com")));
+
+  // *foobar.com:80, this is already a suffix matching rule.
+  SchemeHostPortMatcherHostnamePatternRule rule3("", "*foobar.com", 80);
+  suffix_matching_rule = rule3.GenerateSuffixMatchingRule();
+  EXPECT_EQ("*foobar.com:80", rule3.ToString());
+  EXPECT_EQ("*foobar.com:80", suffix_matching_rule->ToString());
+  EXPECT_EQ(SchemeHostPortMatcherResult::kInclude,
+            rule3.Evaluate(GURL("http://www.foobar.com:80")));
+  EXPECT_EQ(SchemeHostPortMatcherResult::kInclude,
+            suffix_matching_rule->Evaluate(GURL("http://www.foobar.com:80")));
+
+  // *.foo, this is already a suffix matching rule.
+  SchemeHostPortMatcherHostnamePatternRule rule4("", "*.foo", -1);
+  suffix_matching_rule = rule4.GenerateSuffixMatchingRule();
+  EXPECT_EQ("*.foo", rule4.ToString());
+  EXPECT_EQ("*.foo", suffix_matching_rule->ToString());
+  EXPECT_EQ(SchemeHostPortMatcherResult::kInclude,
+            rule4.Evaluate(GURL("http://www.foo")));
+  EXPECT_EQ(SchemeHostPortMatcherResult::kInclude,
+            suffix_matching_rule->Evaluate(GURL("http://www.foo")));
+
+  // http://baz, suffix matching works for host part only.
+  SchemeHostPortMatcherHostnamePatternRule rule5("http", "baz", -1);
+  suffix_matching_rule = rule5.GenerateSuffixMatchingRule();
+  EXPECT_EQ("http://baz", rule5.ToString());
+  EXPECT_EQ("http://*baz", suffix_matching_rule->ToString());
+  EXPECT_EQ(SchemeHostPortMatcherResult::kNoMatch,
+            rule5.Evaluate(GURL("http://foobaz")));
+  EXPECT_EQ(SchemeHostPortMatcherResult::kInclude,
+            suffix_matching_rule->Evaluate(GURL("http://foobaz")));
+}
+
 TEST(SchemeHostPortMatcherRuleTest, SchemeHostPortMatcherIPHostRule_IPv4) {
   IPAddress ip_address;
   ignore_result(ip_address.AssignFromIPLiteral("192.168.1.1"));

net/proxy_resolution/parse_proxy_bypass_rules_fuzzer.cc

@@ -18,14 +18,7 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
 
   net::ProxyBypassRules rules;
   std::string input(data, data + size);
-
-  const net::ProxyBypassRules::ParseFormat kFormats[] = {
-      net::ProxyBypassRules::ParseFormat::kDefault,
-      net::ProxyBypassRules::ParseFormat::kHostnameSuffixMatching,
-  };
-
-  for (auto format : kFormats)
-    rules.ParseFromString(input, format);
+  rules.ParseFromString(input);
 
   return 0;
 }

net/proxy_resolution/proxy_bypass_rules.cc

@@ -107,8 +107,7 @@ class SubtractImplicitBypassesRule : public SchemeHostPortMatcherRule {
 };
 
 std::unique_ptr<SchemeHostPortMatcherRule> ParseRule(
-    const std::string& raw_untrimmed,
-    ProxyBypassRules::ParseFormat format) {
+    const std::string& raw_untrimmed) {
   std::string raw;
   base::TrimWhitespaceASCII(raw_untrimmed, base::TRIM_ALL, &raw);
 
@@ -181,12 +180,6 @@ std::unique_ptr<SchemeHostPortMatcherRule> ParseRule(
   if (base::StartsWith(raw, ".", base::CompareCase::SENSITIVE))
     raw = "*" + raw;
 
-  // If suffix matching was asked for, make sure the pattern starts with a
-  // wildcard.
-  if (format == ProxyBypassRules::ParseFormat::kHostnameSuffixMatching &&
-      !base::StartsWith(raw, "*", base::CompareCase::SENSITIVE))
-    raw = "*" + raw;
-
   return std::make_unique<SchemeHostPortMatcherHostnamePatternRule>(scheme, raw,
                                                                     port);
 }
@@ -217,6 +210,13 @@ ProxyBypassRules& ProxyBypassRules::operator=(ProxyBypassRules&& rhs) {
   return *this;
 }
 
+void ProxyBypassRules::ReplaceRule(
+    size_t index,
+    std::unique_ptr<SchemeHostPortMatcherRule> rule) {
+  DCHECK_LT(index, rules_.size());
+  rules_[index] = std::move(rule);
+}
+
 bool ProxyBypassRules::Matches(const GURL& url, bool reverse) const {
   // Later rules override earlier rules, so evaluating the rule list can be
   // done by iterating over it in reverse and short-circuiting when a match is
@@ -269,13 +269,12 @@ bool ProxyBypassRules::operator==(const ProxyBypassRules& other) const {
   return true;
 }
 
-void ProxyBypassRules::ParseFromString(const std::string& raw,
-                                       ParseFormat format) {
+void ProxyBypassRules::ParseFromString(const std::string& raw) {
   Clear();
 
   base::StringTokenizer entries(raw, ",;");
   while (entries.GetNext()) {
-    AddRuleFromString(entries.token(), format);
+    AddRuleFromString(entries.token());
   }
 }
 
@@ -294,9 +293,8 @@ void ProxyBypassRules::PrependRuleToBypassSimpleHostnames() {
   rules_.insert(rules_.begin(), std::make_unique<BypassSimpleHostnamesRule>());
 }
 
-bool ProxyBypassRules::AddRuleFromString(const std::string& raw_untrimmed,
-                                         ParseFormat format) {
-  auto rule = ParseRule(raw_untrimmed, format);
+bool ProxyBypassRules::AddRuleFromString(const std::string& raw_untrimmed) {
+  auto rule = ParseRule(raw_untrimmed);
 
   if (rule) {
     rules_.push_back(std::move(rule));

net/proxy_resolution/proxy_bypass_rules.h

@@ -32,22 +32,6 @@ namespace net {
 // MatchesImplicitRules() for details.
 class NET_EXPORT ProxyBypassRules {
  public:
-  // The input format to use when parsing proxy bypass rules. This format
-  // only applies when parsing, since once parsed any serialization will be in
-  // terms of ParseFormat::kDefault.
-  enum class ParseFormat {
-    kDefault,
-
-    // Variation of kDefault that interprets hostname patterns as being suffix
-    // tests rather than hostname tests. For example, "google.com" would be
-    // interpreted as "*google.com" when parsed with this format, and
-    // match "foogoogle.com".
-    //
-    // Only use this format if needed for compatibility when parsing Linux
-    // bypass strings.
-    kHostnameSuffixMatching,
-  };
-
   typedef std::vector<std::unique_ptr<SchemeHostPortMatcherRule>> RuleList;
 
   // Note: This class supports copy constructor and assignment.
@@ -63,6 +47,10 @@ class NET_EXPORT ProxyBypassRules {
   // or delete them.
   const RuleList& rules() const { return rules_; }
 
+  // Replace rule on |index| in the internal RuleList.
+  void ReplaceRule(size_t index,
+                   std::unique_ptr<SchemeHostPortMatcherRule> rule);
+
   // Returns true if the bypass rules indicate that |url| should bypass the
   // proxy. Matching is done using both the explicit rules, as well as a
   // set of global implicit rules.
@@ -78,8 +66,7 @@ class NET_EXPORT ProxyBypassRules {
   // Initializes the list of rules by parsing the string |raw|. |raw| is a
   // comma separated or semi-colon separated list of rules. See
   // AddRuleFromString() to see the specific rule grammar.
-  void ParseFromString(const std::string& raw,
-                       ParseFormat format = ParseFormat::kDefault);
+  void ParseFromString(const std::string& raw);
 
   // Adds a rule that matches a URL when all of the following are true:
   //  (a) The URL's scheme matches |optional_scheme|, if
@@ -107,8 +94,7 @@ class NET_EXPORT ProxyBypassRules {
   // Returns true if the rule was successfully added.
   //
   // For the supported format of bypass rules see //net/docs/proxy.md.
-  bool AddRuleFromString(const std::string& raw,
-                         ParseFormat format = ParseFormat::kDefault);
+  bool AddRuleFromString(const std::string& raw);
 
   // Appends rules that "cancels out" the implicit bypass rules. See
   // GetRulesToSubtractImplicit() for details.

net/proxy_resolution/proxy_bypass_rules_unittest.cc

@@ -292,23 +292,22 @@ TEST(ProxyBypassRulesTest, HTTPOnlyWithWildcard) {
   EXPECT_FALSE(rules.Matches(GURL("https://www.google.com")));
 }
 
-TEST(ProxyBypassRulesTest, UseSuffixMatching) {
+TEST(ProxyBypassRulesTest, DoesNotUseSuffixMatching) {
   ProxyBypassRules rules;
   rules.ParseFromString(
       "foo1.com, .foo2.com, 192.168.1.1, "
-      "*foobar.com:80, *.foo, http://baz, <local>",
-      ProxyBypassRules::ParseFormat::kHostnameSuffixMatching);
+      "*foobar.com:80, *.foo, http://baz, <local>");
   ASSERT_EQ(7u, rules.rules().size());
-  EXPECT_EQ("*foo1.com", rules.rules()[0]->ToString());
+  EXPECT_EQ("foo1.com", rules.rules()[0]->ToString());
   EXPECT_EQ("*.foo2.com", rules.rules()[1]->ToString());
   EXPECT_EQ("192.168.1.1", rules.rules()[2]->ToString());
   EXPECT_EQ("*foobar.com:80", rules.rules()[3]->ToString());
   EXPECT_EQ("*.foo", rules.rules()[4]->ToString());
-  EXPECT_EQ("http://*baz", rules.rules()[5]->ToString());
+  EXPECT_EQ("http://baz", rules.rules()[5]->ToString());
   EXPECT_EQ("<local>", rules.rules()[6]->ToString());
 
   EXPECT_TRUE(rules.Matches(GURL("http://foo1.com")));
-  EXPECT_TRUE(rules.Matches(GURL("http://aaafoo1.com")));
+  EXPECT_FALSE(rules.Matches(GURL("http://aaafoo1.com")));
   EXPECT_FALSE(rules.Matches(GURL("http://aaafoo1.com.net")));
 }
 

net/proxy_resolution/proxy_config_service_linux.cc

@@ -40,6 +40,23 @@ namespace net {
 
 namespace {
 
+// This turns all rules with a hostname into wildcard matches, which will
+// match not just the indicated hostname but also any hostname that ends with
+// it.
+void RewriteRulesForSuffixMatching(ProxyBypassRules* out) {
+  // Prepend a wildcard (*) to any hostname based rules, provided it isn't an IP
+  // address.
+  for (size_t i = 0; i < out->rules().size(); ++i) {
+    if (!out->rules()[i]->IsHostnamePatternRule())
+      continue;
+
+    const SchemeHostPortMatcherHostnamePatternRule* prev_rule =
+        static_cast<const SchemeHostPortMatcherHostnamePatternRule*>(
+            out->rules()[i].get());
+    out->ReplaceRule(i, prev_rule->GenerateSuffixMatchingRule());
+  }
+}
+
 // Given a proxy hostname from a setting, returns that hostname with
 // an appropriate proxy server scheme prefix.
 // scheme indicates the desired proxy scheme: usually http, with
@@ -199,8 +216,9 @@ ProxyConfigServiceLinux::Delegate::GetConfigFromEnv() {
   }
   // Note that this uses "suffix" matching. So a bypass of "google.com"
   // is understood to mean a bypass of "*google.com".
-  config.proxy_rules().bypass_rules.ParseFromString(
-      no_proxy, ProxyBypassRules::ParseFormat::kHostnameSuffixMatching);
+  config.proxy_rules().bypass_rules.ParseFromString(no_proxy);
+  RewriteRulesForSuffixMatching(&config.proxy_rules().bypass_rules);
+
   return ProxyConfigWithAnnotation(
       config, NetworkTrafficAnnotationTag(traffic_annotation_));
 }
@@ -381,9 +399,7 @@ class SettingGetterImplGSettings
     return false;
   }
 
-  ProxyBypassRules::ParseFormat GetBypassListFormat() override {
-    return ProxyBypassRules::ParseFormat::kDefault;
-  }
+  bool UseSuffixMatching() override { return false; }
 
  private:
   bool GetStringByPath(GSettings* client,
@@ -673,9 +689,7 @@ class SettingGetterImplKDE : public ProxyConfigServiceLinux::SettingGetter {
 
   bool BypassListIsReversed() override { return reversed_bypass_list_; }
 
-  ProxyBypassRules::ParseFormat GetBypassListFormat() override {
-    return ProxyBypassRules::ParseFormat::kHostnameSuffixMatching;
-  }
+  bool UseSuffixMatching() override { return true; }
 
  private:
   void ResetCachedSettings() {
@@ -1141,16 +1155,19 @@ ProxyConfigServiceLinux::Delegate::GetConfigFromSettings() {
   }
 
   // Now the bypass list.
-  auto format = setting_getter_->GetBypassListFormat();
-
   std::vector<std::string> ignore_hosts_list;
   config.proxy_rules().bypass_rules.Clear();
   if (setting_getter_->GetStringList(SettingGetter::PROXY_IGNORE_HOSTS,
                                      &ignore_hosts_list)) {
     for (const auto& rule : ignore_hosts_list) {
-      config.proxy_rules().bypass_rules.AddRuleFromString(rule, format);
+      config.proxy_rules().bypass_rules.AddRuleFromString(rule);
     }
   }
+
+  if (setting_getter_->UseSuffixMatching()) {
+    RewriteRulesForSuffixMatching(&config.proxy_rules().bypass_rules);
+  }
+
   // Note that there are no settings with semantics corresponding to
   // bypass of local names in GNOME. In KDE, "<local>" is supported
   // as a hostname rule.

net/proxy_resolution/proxy_config_service_linux.h

@@ -132,8 +132,11 @@ class NET_EXPORT_PRIVATE ProxyConfigServiceLinux : public ProxyConfigService {
     // allow list rather than block list. (This is KDE-specific.)
     virtual bool BypassListIsReversed() = 0;
 
-    // Returns the format to use when parsing the bypass rules list.
-    virtual ProxyBypassRules::ParseFormat GetBypassListFormat() = 0;
+    // Returns true if bypass rules should evaluate using dumb string suffix
+    // matches on the host. For instance when true, "notgoogle.com" will be
+    // considered a match for "google.com", even though the bypass rule does not
+    // include a wildcard, and the matched host is not a subdomain.
+    virtual bool UseSuffixMatching() = 0;
 
    private:
     DISALLOW_COPY_AND_ASSIGN(SettingGetter);

net/proxy_resolution/proxy_config_service_linux_unittest.cc

@@ -256,9 +256,7 @@ class MockSettingGetter : public ProxyConfigServiceLinux::SettingGetter {
 
   bool BypassListIsReversed() override { return false; }
 
-  ProxyBypassRules::ParseFormat GetBypassListFormat() override {
-    return ProxyBypassRules::ParseFormat::kDefault;
-  }
+  bool UseSuffixMatching() override { return false; }
 
   // Intentionally public, for convenience when setting up a test.
   GSettingsValues values;