1171248 - Add MatchPattern support to WebRequest module

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

This patch adds support for Chrome's match patterns [1] rather than regexps for URL filtering, which is what we're using now. I also fixed a random bug.

[1] https://developer.chrome.com/extensions/match_patterns

Comment 1

[Dave Townsend [:mossop]]

Comment on attachment 8614968 [details] [diff] [review]
patch

Review of attachment 8614968 [details] [diff] [review]:
-----------------------------------------------------------------

This really needs some MatchPattern tests before it should land.

::: toolkit/modules/addons/MatchPattern.jsm
@@ +9,5 @@
> +const PERMITTED_SCHEMES = ["http", "https", "file", "ftp"];
> +
> +// This function converts a glob pattern (containing * and possibly ?
> +// as wildcards) to a regular expression.
> +function globToRegexp(pat, allowQuestion)

allowQuestion is never true. Do we need it?

@@ +20,5 @@
> +  } else {
> +    pat = pat.replace(/\?/g, "\\?");
> +  }
> +  pat = pat.replace(/\*/g, ".*");
> +  return new RegExp(pat);

new RegExp("^" + pat + "$")

@@ +34,5 @@
> +    this.path = new RegExp('.*');
> +  } else if (!pat) {
> +    this.scheme = [];
> +  } else {
> +    let re = new RegExp("^(http|https|file|ftp|\\*):\\/\\/(\\*|\\*\\.[^*/]+|[^*/]+)/(.*)$");

You don't need to escape the / characters in here

@@ +42,5 @@
> +      this.scheme = [];
> +    }
> +
> +    if (match[1] == '*') {
> +      this.scheme = PERMITTED_SCHEMES;

This doesn't match Chrome's behaviour which only allows https or http for '*'

@@ +46,5 @@
> +      this.scheme = PERMITTED_SCHEMES;
> +    } else {
> +      this.scheme = [match[1]];
> +    }
> +    this.host = globToRegexp(match[2], false);

The alternative to generating a full regexp for the host is that it's always either an exact string match or a suffix match. Maybe not worth changing since you had to write this code for the path case anyway.

@@ +57,5 @@
> +    if (this.scheme.indexOf(uri.scheme) == -1) {
> +      return false;
> +    }
> +
> +    if (!this.host.exec(uri.host)) {

The Chrome docs aren't specific about what to do in the case of ports being present. Maybe hostPort is a better choice here?

@@ +61,5 @@
> +    if (!this.host.exec(uri.host)) {
> +      return false;
> +    }
> +
> +    if (!this.path.exec(uri.path)) {

Here and about, test is quicker.

@@ +81,5 @@
> +  }
> +}
> +
> +MatchPattern.prototype = {
> +  matches(uri) {

Better add a note that this expects an nsIURI

@@ +86,5 @@
> +    for (let matcher of this.matchers) {
> +      if (matcher.matches(uri)) {
> +        return true;
> +      }
> +      return false;

This is only checking the first matcher!

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

Thanks Dave! The allowQuestion bit is to support glob patterns:
https://developer.chrome.com/extensions/content_scripts#match-patterns-globs
I haven't actually implemented this yet, but I'd like to do so. It seems like the code is worth keeping.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f4a6c06549

Comment 4

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f8f4a6c06549

Comment 5

[Will Bamberg [:wbamberg]]

I've updated the examples in WebRequest.jsm to use MatchPatterns, and added a little section on how to construct match patterns, and pointed to the match patterns doc for details of the string syntax.

I'll mark this dev-doc-complete, but let me know if you see anything else that needs adding.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks!