Add full support for filesystem URLs.

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 ac648114aaed54e93ec873b92dbd71afe384ae42..a224d38d811d96ec2b17d34657cb8ad3ba8edcb9 100644
--- a/chrome/common/extensions/url_pattern.cc
+++ b/chrome/common/extensions/url_pattern.cc
@@ -96,12 +96,14 @@ bool IsValidPortForScheme(const std::string scheme, const std::string& port) {
 
 URLPattern::URLPattern()
     : valid_schemes_(SCHEME_NONE),
+      valid_inner_schemes_(SCHEME_NONE),
       match_all_urls_(false),
       match_subdomains_(false),
       port_("*") {}
 
 URLPattern::URLPattern(int valid_schemes)
     : valid_schemes_(valid_schemes),
+      valid_inner_schemes_(valid_schemes),
       match_all_urls_(false),
       match_subdomains_(false),
       port_("*") {}
@@ -110,6 +112,7 @@ URLPattern::URLPattern(int valid_schemes, const std::string& pattern)
     // Strict error checking is used, because this constructor is only
     // appropriate when we know |pattern| is valid.
     : valid_schemes_(valid_schemes),
+      valid_inner_schemes_(valid_schemes),
       match_all_urls_(false),
       match_subdomains_(false),
       port_("*") {
@@ -141,28 +144,43 @@ URLPattern::ParseResult URLPattern::Parse(const std::string& pattern) {
   }
 
   // Parse out the scheme.
-  size_t scheme_end_pos = pattern.find(chrome::kStandardSchemeSeparator);
-  bool has_standard_scheme_separator = true;
-
-  // Some urls also use ':' alone as the scheme separator.
-  if (scheme_end_pos == std::string::npos) {
-    scheme_end_pos = pattern.find(':');
-    has_standard_scheme_separator = false;
-  }
-
+  size_t scheme_end_pos = pattern.find(':');
   if (scheme_end_pos == std::string::npos)
     return PARSE_ERROR_MISSING_SCHEME_SEPARATOR;
 
+  bool has_standard_scheme_separator = false;
+  if (scheme_end_pos + 2 < pattern.length() &&
+      pattern[scheme_end_pos + 1] == '/' &&
+      pattern[scheme_end_pos + 2] == '/') {
+    has_standard_scheme_separator = true;
+  }
+
   if (!SetScheme(pattern.substr(0, scheme_end_pos)))
     return PARSE_ERROR_INVALID_SCHEME;
 
   bool standard_scheme = IsStandardScheme(scheme_);
+
+  if (scheme_ == chrome::kFileSystemScheme) {

[Aaron Boodman, 2012/03/12 23:17:13]

Yeah, would prefer this was something like if (SupportsInnerSchemes(scheme_)) {
...

here.

[ericu, 2012/03/13 21:58:47]

Done.

+    size_t inner_scheme_start_pos = scheme_end_pos + 1;
+    scheme_end_pos = pattern.find(chrome::kStandardSchemeSeparator,
+        inner_scheme_start_pos);
+    if (scheme_end_pos == std::string::npos)
+      return PARSE_ERROR_MISSING_SCHEME_SEPARATOR;
+    has_standard_scheme_separator = true;
+    if (!SetInnerScheme(
+            pattern.substr(inner_scheme_start_pos,
+                           scheme_end_pos - inner_scheme_start_pos)))
+      return PARSE_ERROR_INVALID_SCHEME;

[Aaron Boodman, 2012/03/12 23:17:13]

PARSE_ERROR_INVALID_INNER_SCHEME seems better here. 

[ericu, 2012/03/13 21:58:47]

Done.

+    standard_scheme = IsStandardScheme(inner_scheme_);
+  }
+
   if (standard_scheme != has_standard_scheme_separator)

[Aaron Boodman, 2012/03/12 23:17:13]

I still think this method has gotten too long and warrants splitting up. It
doesn't seem that hard to do while you are in here.

I am imagining methods like:

ParseResult ParseScheme(const std::string& input, int* end_pos);
ParseResult ParseInnerScheme(const std::string& input, int* end_pos);

[ericu, 2012/03/13 21:58:47]

Done.

     return PARSE_ERROR_WRONG_SCHEME_SEPARATOR;
 
   // Advance past the scheme separator.
   scheme_end_pos +=
-      (standard_scheme ? strlen(chrome::kStandardSchemeSeparator) : 1);
+      (has_standard_scheme_separator ?
+          strlen(chrome::kStandardSchemeSeparator) : 1);
   if (scheme_end_pos >= pattern.size())
     return PARSE_ERROR_EMPTY_HOST;
 
@@ -184,6 +202,11 @@ URLPattern::ParseResult URLPattern::Parse(const std::string& pattern) {
       // e.g. file://localhost/foo is equal to file:///foo.
       path_start_pos = host_end_pos;
     }
+  } else if (scheme_ == chrome::kFileSystemScheme &&
+      inner_scheme_ == chrome::kFileScheme) {
+    // We can't distinguish between a host and a storage type in a
+    // filesystem:file URL, so just let it be part of the path.
+    path_start_pos = host_start_pos;
   } else {
     size_t host_end_pos = pattern.find(kPathSeparator, host_start_pos);
 
@@ -230,6 +253,12 @@ URLPattern::ParseResult URLPattern::Parse(const std::string& pattern) {
 void URLPattern::SetValidSchemes(int valid_schemes) {
   spec_.clear();
   valid_schemes_ = valid_schemes;
+  valid_inner_schemes_ = valid_schemes;
+}
+
+void URLPattern::SetValidInnerSchemes(int valid_schemes) {
+  spec_.clear();
+  valid_inner_schemes_ = valid_schemes;
 }
 
 void URLPattern::SetHost(const std::string& host) {
@@ -256,12 +285,26 @@ void URLPattern::SetMatchSubdomains(bool val) {
 
 bool URLPattern::SetScheme(const std::string& scheme) {
   spec_.clear();
-  scheme_ = scheme;
-  if (scheme_ == "*") {
+  inner_scheme_.clear();
+  if (scheme == "*") {
     valid_schemes_ &= (SCHEME_HTTP | SCHEME_HTTPS);
-  } else if (!IsValidScheme(scheme_)) {
+  } else if (!IsValidScheme(scheme)) {
+    return false;
+  }
+  scheme_ = scheme;
+  return true;
+}
+
+bool URLPattern::SetInnerScheme(const std::string& inner_scheme) {
+  spec_.clear();
+  DCHECK(scheme_ == chrome::kFileSystemScheme);
+  if (inner_scheme == "*") {
+    valid_inner_schemes_ = SCHEME_HTTP | SCHEME_HTTPS | SCHEME_FTP |

[Aaron Boodman, 2012/03/12 23:17:13]

I am not sure why SetScheme modifies the valid schemes. That does not make much
sense to me. Seems like it should be controlled separately. Same comment here.

[Aaron Boodman, 2012/03/12 23:17:13]

Why this particular interpretation of "*"? Note that it is different than the
one in SetScheme().

[ericu, 2012/03/13 21:58:47]

I implemented it based on the comment in url_pattern.h, which contradicts the
code in SetScheme; I could match SetScheme instead and fix the comment if you
think that's better.  Or I could make SetScheme match the comment.

What do you think?

[ericu, 2012/03/13 21:58:47]

I'm OK with changing the behavior--there aren't all that many callers--but
perhaps it belongs in a different CL?  That's really nothing to do with
supporting filesystem URLs, and this is already not a small change.

+        SCHEME_EXTENSION;  // Exclude file: and chrome:.
+  } else if (!IsValidInnerScheme(inner_scheme)) {
     return false;
   }
+  inner_scheme_ = inner_scheme;
   return true;
 }
 
@@ -277,6 +320,23 @@ bool URLPattern::IsValidScheme(const std::string& scheme) const {
   return false;
 }
 
+bool URLPattern::IsValidInnerScheme(const std::string& scheme) const {
+  // kFileSystemScheme is never a valid inner scheme, even if using SCHEME_ALL.
+  if (scheme == chrome::kFileSystemScheme)
+    return false;
+
+  if (valid_inner_schemes_ == SCHEME_ALL)
+    return true;
+
+  for (size_t i = 0; i < arraysize(kValidSchemes); ++i) {
+    if (scheme == kValidSchemes[i] &&
+        (valid_inner_schemes_ & kValidSchemeMasks[i]))
+      return true;
+  }
+
+  return false;
+}
+
 void URLPattern::SetPath(const std::string& path) {
   spec_.clear();
   path_ = path;
@@ -287,7 +347,9 @@ void URLPattern::SetPath(const std::string& path) {
 
 bool URLPattern::SetPort(const std::string& port) {
   spec_.clear();
-  if (IsValidPortForScheme(scheme_, port)) {
+  std::string scheme =
+      (scheme_ == chrome::kFileSystemScheme) ?  inner_scheme_ : scheme_;
+  if (IsValidPortForScheme(scheme, port)) {
     port_ = port;
     return true;
   }
@@ -301,8 +363,19 @@ bool URLPattern::MatchesURL(const GURL& test) const {
   if (match_all_urls_)
     return true;
 
-  return MatchesSecurityOriginHelper(test) &&
-         MatchesPath(test.PathForRequest());
+  DCHECK(!test.SchemeIsFileSystem() || test.inner_url());
+  if (scheme_ == chrome::kFileSystemScheme &&
+      (!test.SchemeIsFileSystem() ||
+       !MatchesInnerScheme(test.inner_url()->scheme())))

[Aaron Boodman, 2012/03/12 23:17:13]

Style guide requires curly braces here.

[ericu, 2012/03/13 21:58:47]

Done.

+    return false;
+
+  std::string path = test.PathForRequest();
+
+  // Filesystem URLs have the first path segment on the inner_url.
+  if (test.SchemeIsFileSystem())
+    path = test.inner_url()->path() + path;
+
+  return MatchesSecurityOriginHelper(test) && MatchesPath(path);
 }
 
 bool URLPattern::MatchesSecurityOrigin(const GURL& test) const {
@@ -322,6 +395,14 @@ bool URLPattern::MatchesScheme(const std::string& test) const {
   return scheme_ == "*" || test == scheme_;
 }
 
+bool URLPattern::MatchesInnerScheme(const std::string& test) const {
+  DCHECK(scheme_ == chrome::kFileSystemScheme);
+  if (!IsValidInnerScheme(test))
+    return false;
+
+  return inner_scheme_ == "*" || test == inner_scheme_;
+}
+
 bool URLPattern::MatchesHost(const std::string& host) const {
   std::string test(chrome::kHttpScheme);
   test += chrome::kStandardSchemeSeparator;
@@ -387,10 +468,18 @@ const std::string& URLPattern::GetAsString() const {
 
   bool standard_scheme = IsStandardScheme(scheme_);

[Aaron Boodman, 2012/03/12 23:17:13]

Can you rename this to 'is_standard_scheme' while you're in here?

[ericu, 2012/03/13 21:58:47]

Done.

 
-  std::string spec = scheme_ +
-      (standard_scheme ? chrome::kStandardSchemeSeparator : ":");
+  std::string spec = scheme_;
+  std::string scheme = scheme_;
+  bool add_standard_separator = standard_scheme;
+  if (scheme_ == chrome::kFileSystemScheme) {
+    spec += ":" + inner_scheme_;
+    add_standard_separator = true;
+    standard_scheme = IsStandardScheme(inner_scheme_);
+    scheme = inner_scheme_;
+  }
+  spec += (add_standard_separator ? chrome::kStandardSchemeSeparator : ":");
 
-  if (scheme_ != chrome::kFileScheme && standard_scheme) {
+  if (scheme != chrome::kFileScheme && standard_scheme) {
     if (match_subdomains_) {
       spec += "*";
       if (!host_.empty())
@@ -452,11 +541,18 @@ bool URLPattern::MatchesAnyScheme(
 }
 
 bool URLPattern::MatchesSecurityOriginHelper(const GURL& test) const {
-  // Ignore hostname if scheme is file://.
-  if (scheme_ != chrome::kFileScheme && !MatchesHost(test))
+  // Ignore hostname if scheme is file:// or filesystem:file://.
+  std::string scheme = scheme_;
+  const GURL* test_url = &test;
+  if (scheme == chrome::kFileSystemScheme) {
+    scheme = inner_scheme_;
+    test_url = test.inner_url();
+    DCHECK(test_url);
+  }
+  if (scheme != chrome::kFileScheme && !MatchesHost(*test_url))
     return false;
 
-  if (!MatchesPort(test.EffectiveIntPort()))
+  if (!MatchesPort(test_url->EffectiveIntPort()))
     return false;
 
   return true;