Closed Bug 1345225 Opened 7 years ago Closed 7 years ago

Warn when webRequest filters don't overlap with host permissions

Categories

(WebExtensions :: Request Handling, defect, P2)

Product:
WebExtensions
Component:
Request Handling
Type:
defect

Tracking

(firefox54 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: kmag, Assigned: zombie)

Details

(Whiteboard: triaged)

Attachments

(3 files)

globsOverlap.js
1.32 KB, application/javascript
Details
Bug 1345225 - Check if webRequest filters overlap with host permissions
59 bytes, text/x-review-board-request
kmag
: review+
gchang
: approval-mozilla-aurora+
Details
bug-1345225-aurora.patch
8.37 KB, patch
Details | Diff | Splinter Review 
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/*"]})
Priority: -- → P2
Whiteboard: triaged
A nice algorithm problem..  ;)
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Attached file 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.
Attachment #8851399 - Flags: review?(kmaglione+bmo)
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(!/
Attachment #8851399 - Flags: review?(kmaglione+bmo)
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?  ;)
webextensions: --- → ?
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.
Attachment #8851399 - Flags: review?(kmaglione+bmo)
webextensions: ? → ---
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 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.
Attachment #8851399 - Flags: review?(kmaglione+bmo) → review+
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.. ;)
Attachment #8851399 - Flags: review+ → review?(kmaglione+bmo)
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.
Attachment #8851399 - Flags: review?(kmaglione+bmo) → review+
Keywords: checkin-needed
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2356d602d2e
Check if webRequest filters overlap with host permissions r=kmag
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b2356d602d2e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Attachment #8851399 - Flags: approval-mozilla-aurora?
Comment on attachment 8851399 [details]
Bug 1345225 - Check if webRequest filters overlap with host permissions

Fix an extension compatibility issue and include tests. Aurora54+.
Attachment #8851399 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Flags: needinfo?(tomica)
(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
Flags: needinfo?(tomica)
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.
Attachment #8851399 - Flags: checkin?
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.
Attachment #8851399 - Flags: checkin?
Needs a rebased patch for Aurora.
Flags: needinfo?(tomica)
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.
Flags: needinfo?(tomica)
Rebased patch for aurora.
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: