The default bug view has changed. See this FAQ.

Support moz-extension: urls in MatchPattern

NEW
Assigned to

Status

()

Toolkit
WebExtensions: Request Handling
P2
normal
11 months ago
17 days ago

People

(Reporter: aswan, Assigned: mixedpuppy)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 months 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

11 months ago
No longer depends on: 1269180

Comment 1

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

Comment 2

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

Comment 3

5 months 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

5 months ago
Created attachment 8805315 [details]
test_ext_webrequest_mozextension.html

failing test.
Assignee: nobody → mixedpuppy

Updated

5 months ago
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Priority: -- → P2
(Assignee)

Comment 5

5 months ago
Created attachment 8807760 [details] [diff] [review]
webReqMozExtension

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

5 months ago
Attachment #8807760 - Flags: feedback?(kmaglione+bmo)
(Assignee)

Updated

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

Updated

5 months ago
Duplicate of this bug: 1269341

Updated

5 months ago
Blocks: 1226547

Comment 7

5 months 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

5 months 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.

Comment 9

5 months ago
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

4 months 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

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

Comment 13

4 months ago
Created attachment 8811033 [details] [diff] [review]
webReqMozExtension

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

4 months 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

4 months 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

4 months 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.

Updated

3 months ago
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 months ago
Whiteboard: triaged [design decision needed] → triaged
(Assignee)

Comment 21

a month 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.

Updated

18 days ago
Duplicate of this bug: 1344754

Comment 23

18 days 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

17 days 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.
You need to log in before you can comment on or make changes to this bug.