Support moz-extension: urls in MatchPattern

RESOLVED FIXED in Firefox 56

Status

defect
P2
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: aswan, Assigned: mixedpuppy)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [outreach], triaged)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
+++ 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.
(Reporter)

Updated

3 years ago
No longer depends on: 1269180

Comment 1

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

Comment 2

3 years ago
Is this something you are getting in bug 1273138 mixedpuppy and we should dupe?
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 3

3 years ago
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.
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 4

3 years ago
failing test.
Assignee: nobody → mixedpuppy

Updated

3 years ago
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Priority: -- → P2
(Assignee)

Comment 5

3 years ago
Posted patch webReqMozExtension (obsolete) — Splinter Review
A fix with working tests.  However...looking for feedback on the matchpattern change.
Attachment #8805315 - Attachment is obsolete: true
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

3 years ago
Attachment #8807760 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Updated

3 years ago
Summary: Support webRequest on moz-extension: urls → Support moz-extension: urls in MatchPattern
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1269341

Updated

3 years ago
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.
Attachment #8807760 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 8

3 years ago
(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.
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.
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)

Comment 11

3 years ago
mozreview-review
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.
Attachment #8809482 - Flags: review?(kmaglione+bmo) → review-
(Assignee)

Comment 12

3 years ago
Sorry, I didn't actually mean to push that to review.  I think I got overly eager updating patches.
(Assignee)

Comment 13

3 years ago
Posted patch webReqMozExtension (obsolete) — Splinter Review
This patch automatically adds the extension url as a whitelist and webrequest match pattern and protects against the addition of extension urls otherwise.
Attachment #8807760 - Attachment is obsolete: true
Attachment #8809482 - Attachment is obsolete: true
Attachment #8811033 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 14

3 years ago
(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.
(Assignee)

Comment 15

3 years ago
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).
(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 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?
Attachment #8811033 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Comment 18

3 years ago
(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.
Duplicate of this bug: 1328245
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

Updated

2 years ago
Whiteboard: triaged [design decision needed] → triaged
(Assignee)

Comment 21

2 years ago
(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.
Duplicate of this bug: 1344754

Comment 23

2 years ago
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.
(Assignee)

Comment 24

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8811033 - Attachment is obsolete: true

Comment 26

2 years ago
mozreview-review
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.
Attachment #8809482 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 27

2 years ago
(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.
(Assignee)

Updated

2 years ago
Flags: needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
(Assignee)

Comment 30

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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.
Attachment #8809482 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Updated

2 years ago
Whiteboard: triaged → [outreach], triaged [awe:uBlock0@raymondhill.net]
(Assignee)

Comment 35

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

2 years ago
mozreview-review
Comment on attachment 8809482 [details]
Bug 1271354 support moz-extension in webrequests,

https://reviewboard.mozilla.org/r/92062/#review160052
Attachment #8809482 - Flags: review+ → review?(kmaglione+bmo)

Comment 39

2 years ago
mozreview-review
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.
Attachment #8809482 - Flags: review?(kmaglione+bmo) → review+

Comment 40

2 years ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/60823f6c03f2
support moz-extension in webrequests, r=kmag

Comment 41

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60823f6c03f2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 42

2 years ago
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"]);
Keywords: dev-doc-needed

Updated

2 years ago
Whiteboard: [outreach], triaged [awe:uBlock0@raymondhill.net] → [outreach], triaged
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?
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 44

2 years ago
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.
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
(Assignee)

Comment 46

2 years ago
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.
Flags: needinfo?(mixedpuppy) → needinfo?(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.
Flags: needinfo?(wbamberg) → needinfo?(mixedpuppy)
(Assignee)

Comment 48

2 years ago
That looks good.
Flags: needinfo?(mixedpuppy)

Comment 49

a year ago
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?

Updated

a year ago
See Also: → 1463440

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.