1345225 - Warn when webRequest filters don't overlap with host permissions

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P2)

Description

[Kris Maglione [:kmag]]

We should warn when an extension tries to add a webRequest listener which will never be called because its filters don't match any host permissions.

For instance, the following should not trigger a warning:

  permissions: "http://example.com/"
  addListener({urls: ["<all_urls>"]})

But the following should:

  permissions: "http://example.com/"
  addListener({urls: ["http://example.org/*"]})

Comment 1

[Tomislav Jovanovic :zombie]

A nice algorithm problem..  ;)

Comment 2

[Tomislav Jovanovic :zombie]

Attachment: globsOverlap.js

> A nice algorithm problem..  ;)

Nope.  Hosts permissions don't have the path component, so this is not as interesting of a problem as I expected -- namely, testing if two glob patterns can possibly overlap.

Alas, I noticed this only after writing the function, so I'm uploading it here, along with some basic tests, in case we ever need similar functionality.

Comment 3

[Tomislav Jovanovic :zombie]

Attachment: Bug 1345225 - Check if webRequest filters overlap with host permissions

Review commit: https://reviewboard.mozilla.org/r/123698/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123698/

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

https://reviewboard.mozilla.org/r/123698/#review126124

Looks good, but I'm worried about domain substring matching.

::: toolkit/modules/addons/MatchPattern.jsm:117
(Diff revision 1)
> +    if (!this.schemes.some(scheme => other.schemes.includes(scheme))) {
> +      return false;
> +    }
> +    return this.host === other.host ||
> +           (this.host[0] === "*" && other.host.endsWith(this.host.slice(2))) ||
> +           (other.host[0] === "*" && this.host.endsWith(other.host.slice(2)));

Close, but what about `*.foo.com` vs. `barfoo.com`?

::: toolkit/modules/tests/xpcshell/test_MatchPattern.js:95
(Diff revision 1)
>    // Match url with fragments.
>    pass({url: "http://mozilla.org/base#some-fragment", pattern: "http://mozilla.org/base"});
>  }
> +
> +function test_overlaps() {
> +

Nit: Please remove blank line. I'm pretty sure ESLint will complain.

::: toolkit/modules/tests/xpcshell/test_MatchPattern.js:103
(Diff revision 1)
> +    const f = new MatchPattern(filter);
> +    return h.overlapsIgnoringPath(f) && f.overlapsIgnoringPath(h);
> +  }
> +
> +  function pass({hosts, filter}) {
> +    do_check_true(test(hosts, filter), `Expected to overlap: ${hosts}, ${filter}`);

Nit: s/do_check_true/ok/

::: toolkit/modules/tests/xpcshell/test_MatchPattern.js:107
(Diff revision 1)
> +  function pass({hosts, filter}) {
> +    do_check_true(test(hosts, filter), `Expected to overlap: ${hosts}, ${filter}`);
> +  }
> +
> +  function fail({hosts, filter}) {
> +    do_check_false(test(hosts, filter), `Expected no overlap: ${hosts}, ${filter}`);

Nit: s/do_check_false(/ok(!/

Comment 5

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123698/diff/1-2/

Comment 6

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

https://reviewboard.mozilla.org/r/123698/#review126324

::: toolkit/modules/addons/MatchPattern.jsm:112
(Diff revision 2)
>    },
> +
> +  // Tests if this can possibly overlap with the |other| SingleMatchPattern.
> +  overlapsIgnoringPath(other) {
> +    return this.schemes.some(scheme => other.schemes.includes(scheme)) &&
> +           (this.hostMatch(other) || other.hostMatch(this));

D'oh, for missing the substring vs subdomain distinction.

I started adding the necesery checks, but it quickly got less elegant -- and hinting at the similar code from `getHostMatcher()` above.

The end result is so simple that it almost feels too good to be true?  ;)

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

https://reviewboard.mozilla.org/r/123698/#review127430

Sorry, two more things:

1) Since you wrote this, Andrew landed optional permissions, so we also need to consider `extension.optionalOrigins` when checking host permissions.

2) We should probably check that *all* of the URL filters match, rather than *some*. I can't think of a good reason why you'd want any of your URL filters to never match one of your actual host permissions.

Comment 8

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123698/diff/2-3/

Comment 9

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

https://reviewboard.mozilla.org/r/123698/#review127430

1) Assumed we don't need a warning when a filter only matches an optional host permission (granted or not).

2) Renamed the method, since it's not a generic/symetrical "overlap" anymore.

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

https://reviewboard.mozilla.org/r/123698/#review129128

::: toolkit/components/extensions/ext-webRequest.js:90
(Diff revision 3)
> +      if (!context.extension.whiteListedHosts.overlapsFilter(filter2.urls) &&
> +          !context.extension.optionalOrigins.overlapsFilter(filter2.urls)) {

This is close, but I think we need something like `filter.urls.every(url => whitelisted.matches || optional.matches)`, or we'll admit an error for a filter that's partially covered by existing permissions and partially by optional permissions.

Comment 11

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123698/diff/3-4/

Comment 12

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Asking for r? again, given my track record in this bug.. ;)

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

https://reviewboard.mozilla.org/r/123698/#review130602

::: toolkit/modules/addons/MatchPattern.jsm:192
(Diff revision 4)
>  
> +  // Checks if every part of this filter overlaps with
> +  // some of the |hosts| or |optional| permissions MatchPatterns.
> +  overlapsPermissions(hosts, optional) {
> +    const perms = hosts.matchers.concat(optional.matchers);
> +    return this.matchers.length && this.matchers.every(f => perms.some(p => f.overlapsIgnoringPath(p)));

Nit: Can we make this `p => p.overlapsIgnoringPath(f)` instead? I find this a bit hard to parse. Also, probably s/\bf\b/matcher/ or `m` or something.

Comment 14

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123698/diff/4-5/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2356d602d2e
Check if webRequest filters overlap with host permissions r=kmag

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Approval Request Comment
> [Feature/Bug causing the regression]:
Not a regression, but a behavior change from bug 1311815 that impacts extension compat.

> [User impact if declined]:
Possible a high number of extensions might stop working if developers are not notified of the change.

[Is this code covered by automated tests?]:
Yes, solid coverage of existing code, plus new tests for this warning logic.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No.

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It only adds a developer console warning in cases when extension will be impacted by the changed behavior.

[String changes made/needed]:
None.

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Fix an extension compatibility issue and include tests. Aurora54+.

Comment 19

[Carsten Book [:Tomcat]]

has problems to apply to aurora

warning: conflicts while merging toolkit/modules/addons/MatchPattern.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Tomcats-MacBook-Pro-820:mozilla-central Tomcat$ hg revert -a

Comment 20

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123698/diff/5-6/

Comment 21

[Tomislav Jovanovic :zombie]

(In reply to Carsten Book [:Tomcat] from comment #19)
> has problems to apply to aurora

Rebased for aurora.  Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ddc58bdc248

Comment 22

[Tomislav Jovanovic :zombie]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Actually: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4404eee884

Asking for checkin to aurora.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

No need to request checkin on uplifts. There's bug queries for approved patches that haven't been uplifted yet.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Needs a rebased patch for Aurora.

Comment 25

[Tomislav Jovanovic :zombie]

I already rebased to aurora above, and I don't see any more conflicts:


(In reply to Tomislav Jovanovic :zombie from comment #20)
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/123698/diff/5-6/


(In reply to Tomislav Jovanovic :zombie from comment #21)
> Rebased for aurora.  Try push:


(In reply to Tomislav Jovanovic :zombie from comment #22)
> Actually:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4404eee884
> 
> Asking for checkin to aurora.

Comment 26

[Tomislav Jovanovic :zombie]

Attachment: bug-1345225-aurora.patch

Rebased patch for aurora.

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8bad8b5fcd98