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..f25f65d42d3ba7075919a460224a2681d6e657be 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,52 @@ 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;
+  size_t standard_scheme_end_pos =
+      pattern.find(chrome::kStandardSchemeSeparator);
+  bool has_standard_scheme_separator =
+      (standard_scheme_end_pos != std::string::npos);
 
   // 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;
-  }
-
-  if (scheme_end_pos == std::string::npos)
+  size_t simple_scheme_end_pos = pattern.find(':');
+  bool has_simple_scheme_separator =
+      (simple_scheme_end_pos != std::string::npos) &&
+      (!has_standard_scheme_separator ||
+       simple_scheme_end_pos < standard_scheme_end_pos);
+
+  size_t scheme_end_pos;

[Aaron Boodman, 2012/02/15 21:31:18]

Please always initialize primitives. I wish we had a clang plugin for this.

[Aaron Boodman, 2012/02/15 21:31:18]

So is it true at this point that has_simple_scheme_separator and
has_standard_scheme_separator are exclusive? If so, it would be nice to put a
CHECK to that effect to remind the reader.

[ericu, 2012/02/16 01:42:56]

Done.

[ericu, 2012/02/16 01:42:56]

No.  It used to be true, but now we have "filesystem:http://" which tests
positive for both.  As long as the ':' comes before a '://', it counts.

+
+  if (has_simple_scheme_separator)
+    scheme_end_pos = simple_scheme_end_pos;
+  else if (has_standard_scheme_separator)
+    scheme_end_pos = standard_scheme_end_pos;
+  else
     return PARSE_ERROR_MISSING_SCHEME_SEPARATOR;
 
   if (!SetScheme(pattern.substr(0, scheme_end_pos)))

[Aaron Boodman, 2012/02/15 21:31:18]

Would it be simpler to restructure parsing the scheme bit like this (in
psuedo-code):

scheme_separator_start = input.find(':');
scheme_separator_end = scheme_separator_start + 1;
if (input.length > (scheme_separator_start + 2) &&
    input[scheme_separator_start + 1] == '/' &&
    input[scheme_separator_start + 2] == '/')) {
  scheme_separator_end = scheme_separator_start + 3;
}

scheme = input.substr(0, scheme_separator_start);
scheme_separator = input.substr(scheme_separator_start, scheme_separator_end);

< validate legit scheme/separator combinations >

< if outer scheme is filesystem, parse inner scheme >

[ericu, 2012/02/16 01:42:56]

Thanks for the suggestion; I think it's a lot clearer now.  Some of the
complexity got in there when I was trying to keep supporting CrOS's
"filesystem:*" wildcards here, instead of converting them off in extension-land
before calling in.

     return PARSE_ERROR_INVALID_SCHEME;
 
+  if (!has_simple_scheme_separator && has_standard_scheme_separator &&

[Aaron Boodman, 2012/02/15 21:31:18]

This method is getting pretty hairy. Is it possible to break it up into parts?

[ericu, 2012/02/16 01:42:56]

Do you still think it's necessary after the rewrite?

+      scheme_ == chrome::kFileSystemScheme)
+    return PARSE_ERROR_WRONG_SCHEME_SEPARATOR;
+
+  if (has_simple_scheme_separator && has_standard_scheme_separator) {
+    if (scheme_ != chrome::kFileSystemScheme)
+      return PARSE_ERROR_WRONG_SCHEME_SEPARATOR;
+    if (!SetInnerScheme(
+        pattern.substr(simple_scheme_end_pos + 1,
+            standard_scheme_end_pos - simple_scheme_end_pos - 1)))
+      return PARSE_ERROR_INVALID_SCHEME;
+    scheme_end_pos = standard_scheme_end_pos;
+  }
+
   bool standard_scheme = IsStandardScheme(scheme_);
   if (standard_scheme != has_standard_scheme_separator)
     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 +211,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 +262,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 +294,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& scheme) {

[Aaron Boodman, 2012/02/15 21:31:18]

It would be more clear to name this param "inner_scheme".

[ericu, 2012/02/16 01:42:56]

Done.

+  spec_.clear();
+  DCHECK(scheme_ == chrome::kFileSystemScheme);
+  if (scheme == "*") {
+    valid_inner_schemes_ = SCHEME_HTTP | SCHEME_HTTPS | SCHEME_FTP |

[Aaron Boodman, 2012/02/15 21:31:18]

This looks weird. In your travels, did you come to understand why SetScheme()
modifies valid_schemes_?

[ericu, 2012/02/16 01:42:56]

It only does so if you're putting in a wildcard.  The comment in the header says
"The '*' scheme pattern excludes file URLs.".  The code for SetScheme also rules
out ftp; I wasn't sure if that was a bug, but it disagrees with the comments.  I
left FTP in, but they should probably match.  Which way should we go?

+        SCHEME_EXTENSION;  // Exclude file: and chrome:.
+  } else if (!IsValidInnerScheme(scheme)) {
     return false;
   }
+  inner_scheme_ = scheme;
   return true;
 }
 
@@ -277,6 +329,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 +356,10 @@ void URLPattern::SetPath(const std::string& path) {
 
 bool URLPattern::SetPort(const std::string& port) {
   spec_.clear();
-  if (IsValidPortForScheme(scheme_, port)) {
+  std::string scheme = scheme_;

[Aaron Boodman, 2012/02/15 21:31:18]

More clear to use a ternary operator here?

[ericu, 2012/02/16 01:42:56]

Done.

+  if (scheme == chrome::kFileSystemScheme)
+    scheme = inner_scheme_;
+  if (IsValidPortForScheme(scheme, port)) {
     port_ = port;
     return true;
   }
@@ -301,8 +373,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())))
+    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 +405,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 +478,18 @@ const std::string& URLPattern::GetAsString() const {
 
   bool standard_scheme = IsStandardScheme(scheme_);
 
-  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 +551,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;