Don't allow the following content settings patterns:

- Patterns with file scheme and non empty host
- File patterns that user a wildcard '*' symbol in their path
- "file:///"
- Patterns with an IP address and a domain wildcard.

Add tests to check that "[*.]", "http://[*.]", ... are valid patterns.

BUG=104414
TEST=ContentSettingsPattern*


Review URL: http://codereview.chromium.org/8676020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@111546 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 54 insertions, 31 deletions

diff --git a/chrome/common/content_settings_pattern.cc b/chrome/common/content_settings_pattern.cc
index 05a0fe9..16ec56f 100644
--- a/chrome/common/content_settings_pattern.cc
+++ b/chrome/common/content_settings_pattern.cc
@@ -141,7 +141,8 @@ BuilderInterface* ContentSettingsPattern::Builder::Invalid() {
 ContentSettingsPattern ContentSettingsPattern::Builder::Build() {
   if (!is_valid_)
     return ContentSettingsPattern();
-  Canonicalize(&parts_);
+  if (!Canonicalize(&parts_))
+    return ContentSettingsPattern();
   if (use_legacy_validate_) {
     is_valid_ = LegacyValidate(parts_);
   } else {
@@ -151,7 +152,7 @@ ContentSettingsPattern ContentSettingsPattern::Builder::Build() {
 }
 
 // static
-void ContentSettingsPattern::Builder::Canonicalize(PatternParts* parts) {
+bool ContentSettingsPattern::Builder::Canonicalize(PatternParts* parts) {
   // Canonicalize the scheme part.
   const std::string scheme(StringToLowerASCII(parts->scheme));
   parts->scheme = scheme;
@@ -166,6 +167,8 @@ void ContentSettingsPattern::Builder::Canonicalize(PatternParts* parts) {
   const std::string host(parts->host);
   url_canon::CanonHostInfo host_info;
   std::string canonicalized_host(net::CanonicalizeHost(host, &host_info));
+  if (host_info.IsIPAddress() && parts->has_domain_wildcard)
+    return false;
   canonicalized_host = net::TrimEndingDot(canonicalized_host);
 
   parts->host = "";
@@ -174,31 +177,44 @@ void ContentSettingsPattern::Builder::Canonicalize(PatternParts* parts) {
     // Valid host.
     parts->host += canonicalized_host;
   }
+  return true;
 }
 
 // static
 bool ContentSettingsPattern::Builder::Validate(const PatternParts& parts) {
-  // If the pattern is for a "file-pattern" test if it is valid.
-  if (parts.scheme == std::string(chrome::kFileScheme) &&
-      !parts.is_scheme_wildcard &&
-      parts.host.empty() &&
-      parts.port.empty())
-    return true;
+  // Sanity checks first: {scheme, port} wildcards imply empty {scheme, port}.
+  if ((parts.is_scheme_wildcard && !parts.scheme.empty()) ||
+      (parts.is_port_wildcard && !parts.port.empty())) {
+    NOTREACHED();
+    return false;
+  }
+
+  // file:// URL patterns have an empty host and port.
+  if (parts.scheme == std::string(chrome::kFileScheme))
+    return parts.host.empty() &&
+           parts.port.empty() &&
+           !parts.path.empty() &&
+           parts.path != std::string("/") &&
+           parts.path.find("*") == std::string::npos;
 
   // If the pattern is for an extension URL test if it is valid.
   if (parts.scheme == std::string(chrome::kExtensionScheme) &&
-      !parts.is_scheme_wildcard &&
       !parts.host.empty() &&
       !parts.has_domain_wildcard &&
       parts.port.empty() &&
-      !parts.is_port_wildcard)
+      !parts.is_port_wildcard) {
     return true;
+  }
 
   // Non-file patterns are invalid if either the scheme, host or port part is
   // empty.
   if ((parts.scheme.empty() && !parts.is_scheme_wildcard) ||
       (parts.host.empty() && !parts.has_domain_wildcard) ||
-      (parts.port.empty() && !parts.is_port_wildcard))
+      (parts.port.empty() && !parts.is_port_wildcard)) {
+    return false;
+  }
+
+  if (parts.host.find("*") != std::string::npos)
     return false;
 
   // Test if the scheme is supported or a wildcard.
diff --git a/chrome/common/content_settings_pattern.h b/chrome/common/content_settings_pattern.h
index 187ec24..3664851 100644
--- a/chrome/common/content_settings_pattern.h
+++ b/chrome/common/content_settings_pattern.h
@@ -219,8 +219,9 @@ class ContentSettingsPattern {
      virtual ContentSettingsPattern Build() OVERRIDE;
     private:
      // Canonicalizes the pattern parts so that they are ASCII only, either
-     // in original (if it was already ASCII) or punycode form.
-     static void Canonicalize(PatternParts* parts);
+     // in original (if it was already ASCII) or punycode form. Returns true if
+     // the canonicalization was successful.
+     static bool Canonicalize(PatternParts* parts);
 
      // Returns true when the pattern |parts| represent a valid pattern.
      static bool Validate(const PatternParts& parts);
diff --git a/chrome/common/content_settings_pattern_parser.cc b/chrome/common/content_settings_pattern_parser.cc
index 437ecb1..e54834a 100644
--- a/chrome/common/content_settings_pattern_parser.cc
+++ b/chrome/common/content_settings_pattern_parser.cc
@@ -140,14 +140,8 @@ void PatternParser::Parse(const std::string& pattern_spec,
       builder->WithDomainWildcard();
     } else if (StartsWithASCII(host, kDomainWildcard, true)) {
       host = host.substr(kDomainWildcardLength);
-      // If the host still contains a wildcard symbol then it is invalid.
-      if (host.find(kHostWildcard) != std::string::npos) {
-        builder->Invalid();
-        return;
-      } else {
-        builder->WithDomainWildcard();
-        builder->WithHost(host);
-      }
+      builder->WithDomainWildcard();
+      builder->WithHost(host);
     } else {
       // If the host contains a wildcard symbol then it is invalid.
       if (host.find(kHostWildcard) != std::string::npos) {
diff --git a/chrome/common/content_settings_pattern_unittest.cc b/chrome/common/content_settings_pattern_unittest.cc
index 8bad3ea..254df17 100644
--- a/chrome/common/content_settings_pattern_unittest.cc
+++ b/chrome/common/content_settings_pattern_unittest.cc
@@ -172,13 +172,19 @@ TEST(ContentSettingsPatternTest, FromString_WithNoWildcards) {
 }
 
 TEST(ContentSettingsPatternTest, FromString_FilePatterns) {
-  EXPECT_TRUE(Pattern("file:///").IsValid());
-  EXPECT_STREQ("file:///",
-               Pattern("file:///").ToString().c_str());
-  EXPECT_TRUE(Pattern("file:///").Matches(
-      GURL("file:///")));
-  EXPECT_FALSE(Pattern("file:///").Matches(
-      GURL("file:///tmp/test.html")));
+  EXPECT_FALSE(Pattern("file:///").IsValid());
+
+  // Non-empty domains aren't allowed in file patterns.
+  EXPECT_FALSE(Pattern("file://foo/").IsValid());
+
+  // Domain wildcards aren't allowed in file patterns.
+  EXPECT_FALSE(Pattern("file://*/").IsValid());
+  EXPECT_FALSE(Pattern("file://[*.]/").IsValid());
+
+  // These specify a path that contains '*', which isn't allowed to avoid
+  // user confusion.
+  EXPECT_FALSE(Pattern("file:///*").IsValid());
+  EXPECT_FALSE(Pattern("file:///foo/bar/*").IsValid());
 
   EXPECT_TRUE(Pattern("file:///tmp/test.html").IsValid());
   EXPECT_STREQ("file:///tmp/file.html",
@@ -208,6 +214,10 @@ TEST(ContentSettingsPatternTest, FromString_WithIPAdresses) {
   EXPECT_TRUE(Pattern("https://192.168.0.1:8080").IsValid());
   EXPECT_STREQ("https://192.168.0.1:8080",
                Pattern("https://192.168.0.1:8080").ToString().c_str());
+
+  // Subdomain wildcards should be only valid for hosts, not for IP addresses.
+  EXPECT_FALSE(Pattern("[*.]127.0.0.1").IsValid());
+
   // IPv6
   EXPECT_TRUE(Pattern("[::1]").IsValid());
   EXPECT_STREQ("[::1]", Pattern("[::1]").ToString().c_str());
@@ -302,9 +312,11 @@ TEST(ContentSettingsPatternTest, FromString_WithWildcards) {
       GURL("http://www.example.com/")));
 
   // Patterns with host wildcard
-  // TODO(markusheintz): Should these patterns be allowed?
-  // EXPECT_TRUE(Pattern("http://*").IsValid());
-  // EXPECT_TRUE(Pattern("http://*:8080").IsValid());
+  EXPECT_TRUE(Pattern("[*.]").IsValid());
+  EXPECT_TRUE(Pattern("http://*").IsValid());
+  EXPECT_TRUE(Pattern("http://[*.]").IsValid());
+  EXPECT_EQ(std::string("http://*"), Pattern("http://[*.]").ToString());
+  EXPECT_TRUE(Pattern("http://*:8080").IsValid());
   EXPECT_TRUE(Pattern("*://*").IsValid());
   EXPECT_STREQ("*", Pattern("*://*").ToString().c_str());
 }