1271354 - Support moz-extension: urls in MatchPattern

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P2)

Description

[Andrew Swan [:aswan]]

+++ This bug was initially created as a clone of Bug #1269180 +++

But 1269180 was originally opened to report errors in the browser console originating from the MatchPattern constructor.
The original bug has multiple causes, but one of them is that the extension being tested tries to use the webRequest api to monitor loading of extension-local resources (i.e., moz-extension://... urls).  This is currently unimplemented, this bug is to track work to design and implement that functionality.

Comment 1

[Andy McKay]

I'm curious why you'd want to monitor loading of extension local resources using webRequest.

Comment 2

[Andy McKay]

Is this something you are getting in bug 1273138 mixedpuppy and we should dupe?

Comment 3

[Shane Caraveo (:mixedpuppy)]

Well, matchpattern fails on moz-extension, and fetch (within an extension background script) is also not working with moz-extension.  Neither are covered by bug 1273138.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: test_ext_webrequest_mozextension.html

failing test.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: webReqMozExtension

A fix with working tests.  However...looking for feedback on the matchpattern change.

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8807760 [details] [diff] [review]
webReqMozExtension

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

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_mozextension.html
@@ +60,5 @@
> +        fetch(browser.extension.getURL("finished.html")).then(() => {
> +          browser.test.notifyPass("fetch recieved");
> +          browser.test.sendMessage("done");
> +        }, () => {
> +          browser.test.fail("fetch recieved");

Please use `notifyFail` instead, and `awaitFinish` rather than `awaitMessage("done")` below.

::: toolkit/modules/addons/MatchPattern.jsm
@@ +17,5 @@
>  this.EXPORTED_SYMBOLS = ["MatchPattern", "MatchGlobs", "MatchURLFilters"];
>  
>  /* globals MatchPattern, MatchGlobs */
>  
> +const PERMITTED_SCHEMES = ["http", "https", "file", "ftp", "data", "moz-extension"];

If we do this, I think we want to restrict extensions to matching URLs for their own extension. At least for now.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #7)

> If we do this, I think we want to restrict extensions to matching URLs for
> their own extension. At least for now.

That would prevent addons monitoring other addons (original use case in comment 0).  It would also require a different test.

Comment 9

[Kris Maglione [:kmag]]

It would, but there's still a lot of disagreement about whether extensions should be able to monitor other extensions, and this is currently blocking other bugs, so I'd rather handle the non-controversial use case first.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1271354 support moz-extension in webrequests,

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

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

https://reviewboard.mozilla.org/r/92062/#review92148

Sorry, but security has already weighed in on this. Until we have a security model for cross-extension permissions, we need to restrict this to the extension's own URLs.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Sorry, I didn't actually mean to push that to review.  I think I got overly eager updating patches.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: webReqMozExtension

This patch automatically adds the extension url as a whitelist and webrequest match pattern and protects against the addition of extension urls otherwise.

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #11)
> Comment on attachment 8809482 [details]
> Bug 1271354 support moz-extension in webrequests,

> Sorry, but security has already weighed in on this. Until we have a security
> model for cross-extension permissions, we need to restrict this to the
> extension's own URLs.

This has been on my mind.  Is there anything recorded around the reasons for this?  Who on the security team decided this?  It's hard to imagine requests from a webextension is going to be more sensitive than all the pages I normally visit (given all_urls permission).  OTOH allowing something like ublock the ability to monitor communications by other addons could be useful.  I'm also uncertain how (otherwise given some kind of ability to do it) an alternate tab manager addon would even work without being able to query all tabs, including about: tabs.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Oh, what I left out of the above question/comment is that we could require "moz-extension" to be explicit in the permissions for an addon to be able to use that with other APIs (tabs.query, webrequest, etc).

Comment 16

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> This has been on my mind.  Is there anything recorded around the reasons for
> this?  Who on the security team decided this?

Yes, but the bugs in question are not public.

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> Oh, what I left out of the above question/comment is that we could require
> "moz-extension" to be explicit in the permissions for an addon to be able to
> use that with other APIs (tabs.query, webrequest, etc).

We could, except that the hosts for moz-extension: URLs aren't predictable, so
they'd have to request blanket permission for the entire scheme.

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8811033 [details] [diff] [review]
webReqMozExtension

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

The general idea seems sound, but can we instead pass the whitelisted prefixes to the MatchPattern constructor, and have it automatically apply it for the <all_urls> permission and whitelist it when doing moz-extension: checks?

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #16)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> > This has been on my mind.  Is there anything recorded around the reasons for
> > this?  Who on the security team decided this?
> 
> Yes, but the bugs in question are not public.

Can I get cc'd on them?

> (In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> > Oh, what I left out of the above question/comment is that we could require
> > "moz-extension" to be explicit in the permissions for an addon to be able to
> > use that with other APIs (tabs.query, webrequest, etc).
> 
> We could, except that the hosts for moz-extension: URLs aren't predictable,
> so
> they'd have to request blanket permission for the entire scheme.

Yes, that is the idea.

Comment 20

[Tomislav Jovanovic :zombie]

Turning on host permissions checking in bug 1311815 means that test_webRequest_background_events [1] needs to match against the moz-extension URIs, or get disabled.

Therefore, if bug 1311815 lands before this, this bug should include fixing and enabling the test.

1) searchfox.org/mozilla-central/search?q=test_webRequest_background_events

Comment 21

[Shane Caraveo (:mixedpuppy)]

(In reply to Tomislav Jovanovic :zombie from comment #20)
> Turning on host permissions checking in bug 1311815 means that
> test_webRequest_background_events [1] needs to match against the
> moz-extension URIs, or get disabled.
> 
> Therefore, if bug 1311815 lands before this, this bug should include fixing
> and enabling the test.
> 
> 1) searchfox.org/mozilla-central/search?q=test_webRequest_background_events

An addon should not require permission to itself in order to make a request.  Further that test has all_urls permission.

Comment 23

[sankar88.aj]

Hello All,

I raised bug: 1344754, which was redirected as DUP to this bug. 

I have ported my chrome extension to Firefox web extension, would like to release it to customers. It is blocked by this bug. How? --> My extension need to identify proxy server to perform authentication. I can identify the proxy server only during re-direct. The API browser.webRequest.onBeforeRedirect.addListener didn't listen to any redirect, ultimately failed my extension at the nacent stage. I already see 10 months of discussion in this bug, which should have been done before supporting API. 

Question: When is the probable fix date for this bug. Will there by support for <all_urls> ? If not, I've no other choice except to drop my extension plan, since extension has to communicate with different server address and they keep on changing as per release.

Comment 24

[Shane Caraveo (:mixedpuppy)]

(In reply to sankar88.aj from comment #23)
> Hello All,
> 
> I raised bug: 1344754, which was redirected as DUP to this bug. 

I de-duped, but still closed your bug, see my comments there.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

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

Comment 26

[Kris Maglione [:kmag]]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

https://reviewboard.mozilla.org/r/92062/#review135486

::: toolkit/modules/addons/MatchPattern.jsm:22
(Diff revision 2)
>  this.EXPORTED_SYMBOLS = ["MatchPattern", "MatchGlobs", "MatchURLFilters"];
>  
>  /* globals MatchPattern, MatchGlobs */
>  
>  const PERMITTED_SCHEMES = ["http", "https", "file", "ftp", "data"];
> -const PERMITTED_SCHEMES_REGEXP = PERMITTED_SCHEMES.join("|");
> +const PERMITTED_SCHEMES_REGEXP = PERMITTED_SCHEMES.concat("moz-extension").join("|");

[...PERMITTED_SCHEMES, "moz-extension"].join("|")

::: toolkit/modules/addons/MatchPattern.jsm:77
(Diff revision 2)
>        this.schemes = [];
>        return;
>      }
>  
> +    // We disallow the host to be * for moz-extension URLs.
> +    if (match[2] == "*" && this.schemes[0] == "moz-extension") {

I'd rather we not allow to modify other extension URLs, even if they specify them explicitly.

Comment 27

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] from comment #26)

> ::: toolkit/modules/addons/MatchPattern.jsm:77
> (Diff revision 2)
> >        this.schemes = [];
> >        return;
> >      }
> >  
> > +    // We disallow the host to be * for moz-extension URLs.
> > +    if (match[2] == "*" && this.schemes[0] == "moz-extension") {
> 
> I'd rather we not allow to modify other extension URLs, even if they specify
> them explicitly.

I'm actually not certain this is even necessary since we double check manifest permissions via overlapsPermissions, as long as we disallow moz-ext in manifest permissions.  If it is required this gets complicated to check due to how and where MatchPattern is being used.

Comment 28

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #27)
> (In reply to Kris Maglione [:kmag] from comment #26)
> > I'd rather we not allow to modify other extension URLs, even if they specify
> > them explicitly.
> 
> I'm actually not certain this is even necessary since we double check
> manifest permissions via overlapsPermissions, as long as we disallow moz-ext
> in manifest permissions.

OK, let's do that, then.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

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

Comment 30

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

https://reviewboard.mozilla.org/r/92062/#review135486

> I'd rather we not allow to modify other extension URLs, even if they specify them explicitly.

Tests show that an addon cannot use permissions to other addons.

Comment 31

[Kris Maglione [:kmag]]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

https://reviewboard.mozilla.org/r/92062/#review151534

::: toolkit/components/extensions/Extension.jsm:577
(Diff revision 3)
>  
>        this.permissions.add(perm);
>      }
> +
> +    // An extension always gets permission to its own url.
> +    let matcher = new MatchPattern(this.getURL("*"), {ignorePath: true});

Nit: No `"*"` please.

::: toolkit/components/extensions/Extension.jsm:578
(Diff revision 3)
> +    whitelist.push(matcher);
> +    this.permissions.add(matcher.pattern);

How does this not cause permissions tests to fail?

::: toolkit/components/extensions/Extension.jsm:579
(Diff revision 3)
>      }
> +
> +    // An extension always gets permission to its own url.
> +    let matcher = new MatchPattern(this.getURL("*"), {ignorePath: true});
> +    whitelist.push(matcher);
> +    this.permissions.add(matcher.pattern);

Shouldn't be necessary.

::: toolkit/components/extensions/MatchPattern.cpp:300
(Diff revision 3)
>    nsCOMPtr<nsIAtom> scheme = NS_AtomizeMainThread(StringHead(aPattern, index));
>    if (scheme == nsGkAtoms::_asterisk) {
>      mSchemes = AtomSet::Get<WILDCARD_SCHEMES>();
>    } else if (permittedSchemes->Contains(scheme)) {
>      mSchemes = new AtomSet({scheme});
> +  } else if (scheme == nsGkAtoms::moz_extension) {

Nit: Please just add this as an `||` clause to the above condition.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_mozextension.html:17
(Diff revision 3)
> +<body>
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +let peakAchu;

Hm.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_mozextension.html:67
(Diff revision 3)
> +
> +  let waitForConsole = new Promise(resolve => {
> +    SimpleTest.monitorConsole(resolve, messages);
> +  });
> +
> +  extension.startup();

`await` please.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_mozextension.html:69
(Diff revision 3)
> +    SimpleTest.monitorConsole(resolve, messages);
> +  });
> +
> +  extension.startup();
> +  await extension.awaitFinish("loaded");
> +  extension.unload();

`await`

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_mozextension.html:148
(Diff revision 3)
> +        return;
> +      }
> +      browser.test.log(`tab created ${tabId} ${JSON.stringify(tabInfo)} ${tab.url}`);
> +      let tabs = await browser.tabs.query({url: browser.extension.getURL("*")});
> +      browser.test.assertEq(1, tabs.length, "got one tab");
> +      browser.test.assertEq(tabs.length && tabs[0].id, tab.id, "got the right tab");

Nit: *correct

::: toolkit/modules/addons/MatchPattern.jsm:125
(Diff revision 3)
> -this.MatchPattern = function(pat) {
> +this.MatchPattern = function(pat, extensionUrl) {
>    this.pat = pat;
>    if (!pat) {
>      this.matchers = [];
>    } else if (pat instanceof String || typeof(pat) == "string") {
>      this.matchers = [new SingleMatchPattern(pat)];
>    } else {
>      this.matchers = pat.map(p => new SingleMatchPattern(p));
>    }
> +  if (extensionUrl) {
> +    this.matchers.push(new SingleMatchPattern(extensionUrl));
> +  }

This doesn't seem to be used anymore.

Comment 32

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

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

Comment 33

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

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

Comment 34

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

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

Comment 35

[Shane Caraveo (:mixedpuppy)]

Status on this: there are a bunch of permission related test failures, which I started to fix in the tests.  However, one of the failures has to do with permissions on reload of an extension and I'm not certain that should be worked around in tests.  I haven't finished investigating.

Comment 36

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

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

Comment 37

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/92062/diff/7-8/

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

https://reviewboard.mozilla.org/r/92062/#review160052

Comment 39

[Kris Maglione [:kmag]]

Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

https://reviewboard.mozilla.org/r/92062/#review160056

Sorry, didn't mean to reset the review flag there.

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60823f6c03f2
support moz-extension in webrequests, r=kmag

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/60823f6c03f2

Comment 42

[Shane Caraveo (:mixedpuppy)]

Even I stumbled over this for a while.  We need to document how an addon can use webrequest to monitor requests on its own urls.  Specifically, even if the addon has <all_urls> permission, it must specify the moz-ext url in the filters when adding a listener.  Here's how:

let uriMatch = browser.extension.getURL("*");
browser.webRequest.onBeforeRequest.addListener(details => {
  ...
}, {urls: ["<all_urls>", uriMatch]}, ["blocking"]);

Comment 43

[Will Bamberg [:wbamberg]]

I can't make this work (Firefox Nightly, 57.0a1).

manifest.json:
--------------
{

  "manifest_version": 2,
  "name": "example1",
  "version": "1.0",

  "browser_action": {
    "default_icon": "icons/on.svg",
    "default_title": "on"
  },

  "background": {
    "scripts": ["background.js"]
  },
  
  "permissions": ["webRequest", "<all_urls>"]

}

--------------
background.js:
--------------
function logURL(requestDetails) {
  console.log("Loading: " + requestDetails.url);
}

browser.webRequest.onBeforeRequest.addListener(
  logURL,
  {urls: ["<all_urls>", browser.extension.getURL("*")]}
);

browser.browserAction.onClicked.addListener(() => {
  browser.tabs.create({
    url: "my-page.html"
  });
})

--------------

I see logging when I visit any http/https page. When I click the icon the packaged page "my-page.html" is opened, but I don't see any logging. What am I doing wrong?

Comment 44

[Shane Caraveo (:mixedpuppy)]

Look at toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_mozextension.html specifically the test_webRequest_mozextension_fetch and test_webRequest_mozextension_tab_query tests.  I'll see if I can get around to testing what you tried.

Comment 45

[Will Bamberg [:wbamberg]]

It works with fetch(), which is what the test code uses. But apparently not with browser.tabs.create():

  browser.webRequest.onBeforeRequest.addListener(
    logURL,
    {urls: ["<all_urls>", browser.extension.getURL("*")]}
  );

  browser.tabs.create({
    url: "https://example.com"                             // gets intercepted
  });
  browser.tabs.create({
    url: browser.extension.getURL("my-page.html")          // does not get intercepted
  });
  fetch(browser.extension.getURL("https://example.com"));  // gets intercepted
  fetch(browser.extension.getURL("my-page.html"));         // gets intercepted

Comment 46

[Shane Caraveo (:mixedpuppy)]

There are caveats on snooping on web-ext loads.  

web-extension requests are not http requests and so do not use httpchannel (even when accessed via fetch).  We kind of fake the request via the content policy manager (which is the only way we see non-http requests).  So the first is you only get onBeforeRequest and onCompleted or onErrorOccurred for those requests.  The second is that if it is initiated from a system principal, the extension will not see the request at all.  This is why you do not see the request when opening the page from tabs.create.  

You do see the request for example.com when opened via tabs.create because that is a real http request and we handle that from different code paths.

Comment 47

[Will Bamberg [:wbamberg]]

So I've tried to document various aspects of this:

* extensions automatically get their own URL as a host permission: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions#Host_permissions

* tabs.query can include "moz-extension://..." in `url`: I've added a couple of examples for this: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/query#Examples

* you can listen for requests to your own URLs, with some caveats: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest#Intercepting_extension_requests

Please let me know if this covers it, or if I've got something wrong.

Comment 48

[Shane Caraveo (:mixedpuppy)]

That looks good.

Comment 49

[K Mozej]

https://bugzilla.mozilla.org/show_bug.cgi?id=1271354#c11

Regarding the above, is there any open bug about allowing WebExtensions to block/monitor requests from other WebExtensions?