Implement chrome-side intent filtering.

components/arc/intent_helper/intent_filter.cc

Index: components/arc/intent_helper/intent_filter.cc
diff --git a/components/arc/intent_helper/intent_filter.cc b/components/arc/intent_helper/intent_filter.cc
new file mode 100644
index 0000000000000000000000000000000000000000..23b72fab00ae7d42eb802ff90a687323ce9d65cf
--- /dev/null
+++ b/components/arc/intent_helper/intent_filter.cc
@@ -0,0 +1,239 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+

[Yusuke Sato, 2016/07/11 04:38:32]

<algorithm> for std::transform, <ctype.h> for tolower is missing? (see also the
comment at line 110.)

[Ben Kwa, 2016/07/11 21:22:58]

Removed both after the switch to base::toLowerASCII.

+#include "intent_filter.h"

[Yusuke Sato, 2016/07/11 04:38:31]

components/arc/intent_helper/intent_filter.h

[Ben Kwa, 2016/07/11 21:22:58]

Done.

+
+#include "url/gurl.h"
+
+namespace arc {
+
+IntentFilter::IntentFilter(
+    const mojom::IntentFilterPtr& mojo_intent_filter)
+{

[Yusuke Sato, 2016/07/11 04:38:32]

... mojo_intent_filter) {

[Ben Kwa, 2016/07/11 21:22:58]

Done.

+  for (const mojom::AuthorityEntryPtr& authorityptr:
+           mojo_intent_filter->data_authorities) {
+    authorities_.push_back(AuthorityEntry(authorityptr));
+  }
+  for (const mojom::PatternMatcherPtr& pattern:
+           mojo_intent_filter->data_paths) {
+    paths_.push_back(PatternMatcher(pattern));
+  }
+  for (const mojom::PatternMatcherPtr& pattern:
+           mojo_intent_filter->data_scheme_specific_parts) {
+    scheme_specific_parts_.push_back(PatternMatcher(pattern));
+  }
+}
+
+IntentFilter::IntentFilter(const IntentFilter& other) = default;
+
+IntentFilter::~IntentFilter() = default;
+
+bool IntentFilter::match(

[Yusuke Sato, 2016/07/11 04:38:32]

Could you let me know a list of function you transcribed from Android (so that I
can compare them side-by-side?)

[Ben Kwa, 2016/07/11 21:22:58]

Direct transcriptions:
- IntentFilter::hasDataSchemeSpecificPart -->
IntentFilter#hasDataSchemeSpecificPart [1]
- IntentFilter::hasDataPath        --> IntentFilter#hasDataPath [2]
- IntentFilter::matchDataAuthority --> IntentFilter#matchDataAuthority [3] 
- AuthorityEntry::match            --> IntentFilter.AuthorityEntry#match [4]

- IntentFilter::match is a simplified transcription of android's
IntentFilter#matchData [5].  Logically it really maps to IntentFilter#match, but
the #matchAction and #matchCategories part of that code are redundant in this
case.

- PatternMatcher::match/matchBlob is a transcription/slight refactoring of
PatternMatcher#matchPattern [6].


1:
https://cs.corp.google.com/android/frameworks/base/core/java/android/content/...
2:
https://cs.corp.google.com/android/frameworks/base/core/java/android/content/...
3:
https://cs.corp.google.com/android/frameworks/base/core/java/android/content/...
4:
(https://cs.corp.google.com/android/frameworks/base/core/java/android/content/...)
5:
https://cs.corp.google.com/android/frameworks/base/core/java/android/content/...
6:
https://cs.corp.google.com/android/frameworks/base/core/java/android/os/Patte...

+    const GURL& url) const {
+  // Chrome-side code only receives view intents for http/https URLs, so this
+  // match code really only implements the matchData part of the android
+  // IntentFilter class.
+  if (!url.SchemeIsHTTPOrHTTPS()) {
+    return false;
+  }
+
+  bool match = false;
+  if (scheme_specific_parts_.size() > 0) {
+    match = hasDataSchemeSpecificPart(url);
+  }
+  if (match == false) {

[rickyz (no longer on Chrome), 2016/07/11 21:27:51]

nit: if (!match)

Or better yet, how about just returning true above and getting rid of this if?

[Ben Kwa, 2016/07/11 22:34:02]

Ack.  I can't believe I wrote that.  Done.

+    // If there isn't any matching ssp, we need to match an authority.
+    if (authorities_.size() > 0) {
+      bool authMatch = matchDataAuthority(url);

[rickyz (no longer on Chrome), 2016/07/11 21:27:51]

Could reduce some nesting with:

match = matchDataAuthority(url) && (paths_.empty() || hasDataPath(url));

[Ben Kwa, 2016/07/11 22:34:02]

Done.

+      if (authMatch) {
+        if (paths_.empty()) {

[rickyz (no longer on Chrome), 2016/07/11 21:27:52]

nit: would be nice to be consistent with .empty() vs. size() > 0 throughout

[Ben Kwa, 2016/07/11 22:34:01]

Done.

+          match = authMatch;
+        } else if (hasDataPath(url)) {
+          match = true;
+        }
+      }
+    }
+  }
+  return match;
+}
+
+bool IntentFilter::hasDataSchemeSpecificPart(
+    const GURL& url) const {
+  // The scheme-specific-part is the content of the URL minus the ref fragment.
+  std::string ssp = url.GetContent();
+  if (url.has_ref()) {

[rickyz (no longer on Chrome), 2016/07/11 21:27:52]

Can we use GURL::ReplaceComponents instead to get rid of the ref?

[Ben Kwa, 2016/07/11 22:34:01]

Done.

+    std::string fragment = url.ref();
+    // Fragment doesn't include the leading '#', so decrement the position by 1
+    // when cutting it out of the ssp.
+    ssp = ssp.substr(0, ssp.rfind(fragment)-1);
+  }
+  for (const PatternMatcher& pattern: scheme_specific_parts_) {
+    if (pattern.match(ssp)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+bool IntentFilter::hasDataPath(
+    const GURL& url) const {
+  const std::string path = url.path();
+  for (const PatternMatcher& pattern: paths_) {
+    if (pattern.match(path)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+bool IntentFilter::matchDataAuthority(
+    const GURL& url) const {
+  for (const AuthorityEntry& authority: authorities_) {
+    if (authority.match(url)) {
+      return true;
+    }
+  }
+  return false;
+}
+
+
+IntentFilter::AuthorityEntry::AuthorityEntry(
+    const mojom::AuthorityEntryPtr& entry):
+    host_(entry->host.get()),
+    port_(entry->port) {
+  // Wildcards are only allowed at the front of the host string.
+  wild_ = host_.length() > 0 && host_[0] == '*';
+  if (wild_) {
+    host_ = host_.substr(1);
+  }
+  std::transform(host_.begin(), host_.end(), host_.begin(), ::tolower);

[Yusuke Sato, 2016/07/11 04:38:32]

base/strings/string_util.cc has ToLowerASCII(). base/i18n/case_conversion.h also
has ToLower(). We should probably use either of them.

[Ben Kwa, 2016/07/11 21:22:58]

I'm going with base::ToLowerASCII for now.  Probably not i18n-friendly, but I
have a bit of research to do into IDN support on android and in chrome's GURL
class before I know how to fix this properly.

[rickyz (no longer on Chrome), 2016/07/11 21:27:52]

According to
https://developer.android.com/reference/android/content/IntentFilter.Authorit...
authority entry matching is case sensitive, so do we even need to lowercase here
and below? Or is this due to Chrome potentially normalizing the URL and always
making it lowercase, so we're explicitly allowing false positives here? If so,
that probably deserves a comment.

[Ben Kwa, 2016/07/11 22:34:01]

So, you're reading that note as prescriptive, but I think it's actually a
warning to developers that URLs need to be case-normalized in order for
authority comparison to work correctly.  And.. uh, docs aside, the authority
matching code is actually case insensitive.  =P  [1]

1:
https://cs.corp.google.com/android/frameworks/base/core/java/android/content/...

+}
+
+bool IntentFilter::AuthorityEntry::match(
+    const GURL& url) const {
+  if (!url.has_host()) return false;
+  std::string host = url.host();
+  std::transform(host.begin(), host.end(), host.begin(), ::tolower);

[Yusuke Sato, 2016/07/11 04:38:32]

same

[Ben Kwa, 2016/07/11 21:22:58]

Done.

+  const int port = url.IntPort();

[rickyz (no longer on Chrome), 2016/07/11 21:27:52]

Do you want EffectiveIntPort here? This returns PORT_UNSPECIFIED for
https://domain/ or http://domain/.

[Ben Kwa, 2016/07/11 22:34:01]

A filter with an explicitly specified default port (e.g. 22) will *not* match a
URL that does not specify an explicit port, so I think the answer is no here
(i.e. we want IntPort).

+
+  if (wild_) {
+    if (host.length() < host_.length()) {
+      return false;
+    }
+    host = host.substr(host.length() - host_.length());

[rickyz (no longer on Chrome), 2016/07/11 21:27:52]

Can we just use base::EndsWith from base/strings/string_util.h here? (Also wow,
base:: is a such a polluted namespace...)

[Ben Kwa, 2016/07/11 22:34:02]

Done.

+  }
+  if (host.compare(host_) != 0) {
+      return false;
+  }
+  if (port_ >= 0) {
+    if (port_ != port) {
+      return false;
+    }
+  }
+  return true;
+}
+
+
+IntentFilter::PatternMatcher::PatternMatcher(
+    const mojom::PatternMatcherPtr& pattern):
+    pattern_(pattern->pattern.get()),
+    match_type_(pattern->type) {
+}
+
+bool IntentFilter::PatternMatcher::match(
+    const std::string& match) const {

[rickyz (no longer on Chrome), 2016/07/11 21:27:52]

I know Android's code uses match as a var name for some of these, but str might
be a less confusing name.

[Ben Kwa, 2016/07/11 22:34:01]

Done.

+  if (match.length() == 0) {
+    return false;
+  }
+  switch (match_type_) {
+    case mojom::PatternType::PATTERN_LITERAL:
+      return pattern_.compare(match) == 0;

[rickyz (no longer on Chrome), 2016/07/11 21:27:52]

Perhaps pattern_ == match might read a little easier here and elsewhere?

[Ben Kwa, 2016/07/11 22:34:02]

Done.

+    case mojom::PatternType::PATTERN_PREFIX:
+      return pattern_.compare(0, match.length(), match);

[rickyz (no longer on Chrome), 2016/07/11 21:27:52]

Should this be == 0? Or better yet, base::StartsWith?

We should probably also have some tests for this code.

[Ben Kwa, 2016/07/11 22:34:01]

Done.

[rickyz (no longer on Chrome), 2016/07/11 23:26:44]

I'd still like to see some Chrome side tests for this code, even if it's mostly
a transcription from Android.

[Ben Kwa, 2016/07/12 16:05:22]

Oops, sorry.  Will add tests in a follow-up.

+    case mojom::PatternType::PATTERN_SIMPLE_GLOB:
+      return matchGlob(match);
+  }
+
+  return false;
+}
+
+bool IntentFilter::PatternMatcher::matchGlob(

[rickyz (no longer on Chrome), 2016/07/09 02:13:34]

This should have a comment saying that it was transcribed to C++ from Android's
PatternMatcher.

I'm afraid of copying/pasting code from Android into Chrome OS, especially given
that this code is known to be pretty buggy/low quality, for example
https://code.google.com/p/android/issues/detail?id=69080.

I haven't looked through this carefully for other bugs or memory safety issues
yet, but given the level of test coverage that this has in Android, I am far
from optimistic:

https://android.googlesource.com/platform/cts/+/master/tests/tests/os/src/and...

[Ben Kwa, 2016/07/11 21:22:58]

I've added a comments noting the origins of this and other code in the file.
As for your concerns about cutting/pasting code.. I don't think any of us are
super happy with the situation, but I'm not sure what other options are
available.  I'm happy to add more checks/tests/whatever you think might be
necessary to make this safer.  LMK if you want to chat more about this.

+    const std::string& match) const {
+  const size_t NP = pattern_.length();
+  const size_t NM = match.length();
+  if (NP == 0) {
+    return NM == 0;
+  }
+  size_t ip = 0, im = 0;
+  char nextChar = pattern_[0];

[Yusuke Sato, 2016/07/11 04:38:32]

What about explicitly checking all the string access to make the function as
safe as the original one? Probably something like this:

 #define CHECK_BOUNDS(s, i) do { if (UNLIKELY((s).length() <= (i))) return
false; } while(false)
 ...
 CHECK_BOUNDS(pattern_, 0);
 char nextChar = pattern_[0];

[Ben Kwa, 2016/07/11 21:22:58]

Done.

+  while (ip < NP && im < NM) {
+    char c = nextChar;
+    ++ip;
+    nextChar = ip < NP ? pattern_[ip] : 0;
+    const bool escaped = (c == '\\');
+    if (escaped) {
+      c = nextChar;
+      ++ip;
+      nextChar = ip < NP ? pattern_[ip] : 0;
+    }
+    if (nextChar == '*') {
+      if (!escaped && c == '.') {
+        if (ip >= (NP-1)) {
+          // At the end with a pattern match
+          return true;
+        }
+        ++ip;
+        nextChar = pattern_[ip];
+        // Consume everything until the next char in the pattern is found.
+        if (nextChar == '\\') {
+          ++ip;
+          nextChar = ip < NP ? pattern_[ip] : 0;
+        }
+        do {
+          if (match[im] == nextChar) {
+            break;
+          }
+          ++im;
+        } while (im < NM);
+        if (im == NM) {
+          // Next char in the pattern didn't exist in the match.
+          return false;
+        }
+        ++ip;
+        nextChar = ip < NP ? pattern_[ip] : 0;
+        ++im;
+      } else {
+        // Consume only characters matching the one before '*'.
+        do {
+          if (match[im] != c) {
+            break;
+          }
+          ++im;
+        } while (im < NM);
+        ++ip;
+        nextChar = ip < NP ? pattern_[ip] : 0;
+      }
+    } else {
+      if (c != '.' && match[im] != c) return false;
+      ++im;
+    }
+  }
+
+  if (ip >= NP && im >= NM) {
+    // Reached the end of both strings
+    return true;
+  }
+
+  // One last check: we may have finished the match string, but still have a
+  // '.*' at the end of the pattern, which is still a match.
+  if (ip == NP-2 &&
+      pattern_[ip] == '.' &&
+      pattern_[ip+1] == '*') {
+    return true;
+  }
+
+  return false;
+}
+
+}  // namespace arc