Use extension match pattern syntax in content settings extension API

chrome/common/extensions/url_pattern.cc

Index: chrome/common/extensions/url_pattern.cc
diff --git a/chrome/common/extensions/url_pattern.cc b/chrome/common/extensions/url_pattern.cc
index 6749ab8632bef945643b225c8f250476ffc58717..4e5f31773677ae1e6f146911b268ae9a45a72db0 100644
--- a/chrome/common/extensions/url_pattern.cc
+++ b/chrome/common/extensions/url_pattern.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/common/extensions/url_pattern.h"
 
+#include "base/string_number_conversions.h"
 #include "base/string_piece.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
@@ -48,6 +49,8 @@ const char* kParseErrorInvalidHostWildcard = "Invalid host wildcard.";
 const char* kParseErrorEmptyPath = "Empty path.";
 const char* kParseErrorHasColon =
     "Ports are not supported in URL patterns. ':' may not be used in a host.";
+const char* kParseErrorInvalidPort =
+    "Invalid port.";
 
 // Message explaining each URLPattern::ParseResult.
 const char* kParseResultMessages[] = {
@@ -58,7 +61,8 @@ const char* kParseResultMessages[] = {
   kParseErrorEmptyHost,
   kParseErrorInvalidHostWildcard,
   kParseErrorEmptyPath,
-  kParseErrorHasColon
+  kParseErrorHasColon,
+  kParseErrorInvalidPort,
 };
 
 COMPILE_ASSERT(URLPattern::NUM_PARSE_RESULTS == arraysize(kParseResultMessages),
@@ -75,24 +79,38 @@ bool IsStandardScheme(const std::string& scheme) {
       url_parse::Component(0, static_cast<int>(scheme.length())));
 }
 
+bool IsValidPort(const std::string& port) {
+  if (port.empty() || port == "*")
+    return true;
+  int parsed_port;

[Sam Kerner (Chrome), 2011/06/28 18:12:39]

I think you need to set an intial value here (int parsed_port = -1) to avoid one
of our builders giving a compile error.

[bauerb at google, 2011/06/28 22:36:52]

No, StringToInt always sets its argument (to zero if there's a parsing error).

[Sam Kerner (Chrome), 2011/06/29 13:36:34]

I am not saying your code is wrong.  I am saying that some compilers are not
smart enough to see that, and when you commit you will find that a builder
breaks with an error about parsed_port being used without being initialized.

[Bernhard Bauer, 2011/06/29 13:53:18]

Hm, fair enough.

+  if (!base::StringToInt(port, &parsed_port))
+    return false;
+  return (parsed_port >= 0) && (parsed_port < 65536);
+}
+
 }  // namespace
 
 URLPattern::URLPattern()
     : valid_schemes_(SCHEME_NONE),
       match_all_urls_(false),
-      match_subdomains_(false) {}
+      match_subdomains_(false),
+      port_("*") {}
 
 URLPattern::URLPattern(int valid_schemes)
-    : valid_schemes_(valid_schemes), match_all_urls_(false),
-      match_subdomains_(false) {}
+    : valid_schemes_(valid_schemes),
+      match_all_urls_(false),
+      match_subdomains_(false),
+      port_("*") {}
 
 URLPattern::URLPattern(int valid_schemes, const std::string& pattern)
-    : valid_schemes_(valid_schemes), match_all_urls_(false),
-      match_subdomains_(false) {
+    : valid_schemes_(valid_schemes),
+      match_all_urls_(false),
+      match_subdomains_(false),
+      port_("*") {
 
   // Strict error checking is used, because this constructor is only
   // appropriate when we know |pattern| is valid.
-  if (PARSE_SUCCESS != Parse(pattern, PARSE_STRICT))
+  if (PARSE_SUCCESS != Parse(pattern, ERROR_ON_PORTS))
     NOTREACHED() << "URLPattern is invalid: " << pattern;
 }
 
@@ -101,9 +119,6 @@ URLPattern::~URLPattern() {
 
 URLPattern::ParseResult URLPattern::Parse(const std::string& pattern,
                                           ParseOption strictness) {
-  CHECK(strictness == PARSE_LENIENT ||
-        strictness == PARSE_STRICT);
-
   // Special case pattern to match every valid URL.
   if (pattern == kAllUrlsPattern) {
     match_all_urls_ = true;
@@ -180,19 +195,30 @@ URLPattern::ParseResult URLPattern::Parse(const std::string& pattern,
     }
     host_ = JoinString(host_components, '.');
 
-    // No other '*' can occur in the host, though. This isn't necessary, but is
-    // done as a convenience to developers who might otherwise be confused and
-    // think '*' works as a glob in the host.
-    if (host_.find('*') != std::string::npos)
-      return PARSE_ERROR_INVALID_HOST_WILDCARD;
-
     path_start_pos = host_end_pos;
   }
 
   SetPath(pattern.substr(path_start_pos));
 
-  if (strictness == PARSE_STRICT && host_.find(':') != std::string::npos)
-    return PARSE_ERROR_HAS_COLON;
+  size_t port_pos = host_.find(':');

[Sam Kerner (Chrome), 2011/06/28 18:12:39]

Pattern http://foo.com:123/* has a host of "foo.com:123" with PARSE_LENIENT, and
"foo.com" with IGNORE_PORTS.  

Because ':' is not a valid character in a DNS host name, the old behavior means
the pattern never matched any host.  I am nervious about changing behavior,
because it means installed extensions will suddenly start doing things they did
not do before.  On the other hand, any developer who tried to load them in
chrome for the last few releases got an error.

Matt, what do you think?  The case this matters is a pattern in the manifest,
such as a host permission.

I think all the new code uses USE_PORTS, so you could change IGNORE_PORTS to the
old behavior and we could figure this out in a separate change.

[Matt Perry, 2011/06/28 21:37:07]

I agree. It seems unlikely to matter, but to be on the safe side I think we
should keep the behavior of IGNORE_PORTS.

[Bernhard Bauer, 2011/06/29 13:53:18]

Done.

+  if (port_pos != std::string::npos) {
+    if (strictness == ERROR_ON_PORTS)
+      return PARSE_ERROR_HAS_COLON;
+
+    if (strictness == USE_PORTS) {
+      std::string port = host_.substr(port_pos + 1);
+      if (!SetPort(port))
+        return PARSE_ERROR_INVALID_PORT;
+    }
+
+    host_ = host_.substr(0, port_pos);
+  }
+
+  // No other '*' can occur in the host, though. This isn't necessary, but is
+  // done as a convenience to developers who might otherwise be confused and
+  // think '*' works as a glob in the host.
+  if (host_.find('*') != std::string::npos)
+    return PARSE_ERROR_INVALID_HOST_WILDCARD;
 
   return PARSE_SUCCESS;
 }
@@ -226,6 +252,14 @@ void URLPattern::SetPath(const std::string& path) {
   ReplaceSubstringsAfterOffset(&path_escaped_, 0, "?", "\\?");
 }
 
+bool URLPattern::SetPort(const std::string& port) {
+  if (IsValidPort(port)) {
+    port_ = port;
+    return true;
+  }
+  return false;
+}
+
 bool URLPattern::MatchesURL(const GURL &test) const {
   if (!MatchesScheme(test.scheme()))
     return false;
@@ -240,6 +274,9 @@ bool URLPattern::MatchesURL(const GURL &test) const {
   if (!MatchesPath(test.PathForRequest()))
     return false;
 
+  if (!MatchesPort(test.port()))

[Matt Perry, 2011/06/28 21:37:07]

do you want to handle matching a "default port"? like if the pattern was
http://google.com:80/foo, does MatchesURL("http://google.com/foo") match it?

If so, you have to check if test.port() == -1 (or use EffectiveIntPort()).

[bauerb at google, 2011/06/28 22:36:52]

Ooh! Yes, I want that, thanks!

+    return false;
+
   return true;
 }
 
@@ -296,6 +333,13 @@ bool URLPattern::MatchesPath(const std::string& test) const {
   return true;
 }
 
+bool URLPattern::MatchesPort(const std::string& test) const {
+  if (!IsValidPort(test))

[Matt Perry, 2011/06/28 21:37:07]

this test seems unnecessary. port_ must be a valid port, since we don't allow it
to be set otherwise, so if port_ == test, test is a valid port.

[Bernhard Bauer, 2011/06/29 13:53:18]

But we don't want a port wildcard to match an invalid port?

[Matt Perry, 2011/06/29 17:18:47]

Oh OK, fair enough.

+    return false;
+
+  return port_ == "*" || port_ == test;
+}
+
 std::string URLPattern::GetAsString() const {
   if (match_all_urls_)
     return kAllUrlsPattern;
@@ -314,6 +358,11 @@ std::string URLPattern::GetAsString() const {
 
     if (!host_.empty())
       spec += host_;
+
+    if (port_ != "*") {
+      spec += ":";
+      spec += port_;
+    }
   }
 
   if (!path_.empty())
@@ -331,6 +380,9 @@ bool URLPattern::OverlapsWith(const URLPattern& other) const {
   if (!MatchesHost(other.host()) && !other.MatchesHost(host_))
     return false;
 
+  if (port_ != "*" && other.port() != "*" && port_ != other.port())
+    return false;
+
   // We currently only use OverlapsWith() for the patterns inside
   // URLPatternSet. In those cases, we know that the path will have only a
   // single wildcard at the end. This makes figuring out overlap much easier. It