1273138 - WebExtensions: webRequest listeners do not fire for XMLHttpRequest requests generated by the extension

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P2)

Description

[dw-dev]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:

Whilst migrating my Print Edit add-on to the new WebExtensions API, I have discovered an incompatibility between the WebExtensions API and the Chrome Extensions API.

The webRequest listeners (onBeforeRequest, onBeforeSendHeaders and onSendHeaders) do not fire for XMLHttpRequest GET requests in the background script, content scripts or web page scripts.

This incompatibility is NOT noted in the WebExtensions documentation: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities.

I have tested the following code in both background scripts and content scripts, using both Firefox Nightly and Chrome:

    var xhr = new Object();

    xhr = new XMLHttpRequest();
                    
    xhr.open("GET",<external URL>,true);
    xhr.responseType = "arraybuffer";
    xhr.timeout = 2000;
    xhr.onload = onloadImage;
    xhr.onerror = onerrorImage;
    xhr.ontimeout = ontimeoutImage;
                    
    xhr.send();



Actual results:

In Firefox Nightly, the GET request is successful, but the webRequest listeners are NOT fired.

In Chrome, the GET request is successful, and the webRequest listeners are fired with the ResourceType set to "xmlhttprequest".



Expected results:

The GET request should succeed, and the webRequest listeners should fire with the ResourceType set to "xmlhttpreuest".

Comment 1

[:shell escalante]

can we get an example add-on that this is happening on?

Comment 2

[dw-dev]

Attachment: Print Edit WE add-on (development version)

Comment 3

[dw-dev]

This happens with my Print Edit WE add-on (now attached).

I need to explain a little bit about how Print Edit WE operates.

When the toolbar button is clicked, Print Edit WE creates a new (duplicate) tab in which to do the editing.

When edit mode is entered, Print Edit WE blocks all web page HTTP requests using webRequest listeners, to avoid the page changing while it is being edited and to avoid any transactions with the server. 

However, the 'Save As HTML Complete' command needs to convert all of the HTML <img> images and CSS url(...) images into base 64 data URIs.  The image files are loaded using XMLHttpRequest GET requests in the background script - and these requests need to avoid the webRequest blocking. 

In Chrome, these GET requests cause the webRequest listeners to fire, but they can be identified and allowed because details.tabId is set to -1.

In Firefox, these GET requests do NOT fire the webRequest listeners, so they are always allowed.  This is a bug, but does not cause a problem for Print Edit WE.

However, while testing Print Edit WE in Firefox, I discovered that XMLHttpRequest GET requests from content scripts and (I think) web page scripts also do NOT fire the webRequest listeners. This seems a more serious bug, especially if it applies to web page scripts, because extensions have no control over the requests made by web page scripts.

Comment 4

[Nancy Grossman]

Attachment: Demo WE: log webRequest events

A simple demo. This extension logs all `chrome.webRequest` events; `xhrGet(sUrl)` from the background context to confirm no events for XMLHttpRequest.

Comment 5

[ncla]

Can confirm this bug. If I send requests to domains that I have permissions for from content script, it works just fine, but if I send the same request from background script the onBeforeSendHeaders listener does not fire.

Another weird, perhaps unintentional behavior I noticed, is that the listener will work for domains that you do not have permission for: if a website (for which you have permissions) sends a request to a domain that you do not have permissions for (e.g. images on cdn subdomain, or tracking scripts), the listener will capture those requests.

Comment 7

[george]

Any chance this bug can get fast tracked? It was mentioned in irc that this issue had not been prioritized. It is the last issue blocking release of our chrome extension with 150k users to AMO.

Are there any work arounds to this problem? Thought of proxying requests to a different context but blocked due to CORS issues.

Comment 8

[Kris Maglione [:kmag]]

Why exactly do you need to intercept requests generated by your own extension?

Comment 9

[george]

We need the ability to adjust request/response headers to silo requests. For instance to control what cookies are sent and set on a request response lifecycle.

Comment 10

[Jan Biniok]

> Why exactly do you need to intercept requests generated by your own extension?

This would allow extensions
1) to set unsafe headers,
2) to modify response headers (i.e. add a final URL header based on webRequest.onRedirect),
3) to use a feature that is present in Google Chrome ;)
4) and it helps the Tampermonkey Firefox extension to realize some more features of the Chrome extension (7M users)

Comment 11

[Kris Maglione [:kmag]]

(In reply to Jan Biniok from comment #10)
> 1) to set unsafe headers,

If that's something we want to allow, then we should allow the headers to be set on the XMLHttpRequest directly.

> 3) to use a feature that is present in Google Chrome ;)

That's not a particularly good reason to prioritize something.

> 4) and it helps the Tampermonkey Firefox extension to realize some more
> features of the Chrome extension (7M users)

Such as?

Comment 12

[george]

(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to Jan Biniok from comment #10)
> > 1) to set unsafe headers,
> 
> If that's something we want to allow, then we should allow the headers to be
> set on the XMLHttpRequest directly.
> 
> > 3) to use a feature that is present in Google Chrome ;)
> 
> That's not a particularly good reason to prioritize something.
> 

Seems like a great reason to me, maintaining portability when there is no work around. Shouldn't bugs with supported APIs be the highest priority if there is no work around?

Comment 13

[Jan Biniok]

> If that's something we want to allow, then we should allow the headers to be set on the XMLHttpRequest directly.

Of course, that'd be an option. However, (shared) extensions then need browser sniffing to do it the right way. 

> That's not a particularly good reason to prioritize something.

I know, that's why the smiley, but nevertheless it's something to consider.

> Such as?

1) Greasemonkey and Tampermonkey offer an API called GM_xmlhttpRequest which often is used in combination with unsafe headers (user-agent, cookie, referrer, ...), but that could be solved by a direct modification as stated above.

2) For security and clarification reasons TM requires all scripts to declare the URLs that should be connectable via GM_xmlhttpRequest. This also includes a check of all redirect URLs, but that's not possible at the moment.

Comment 14

[Jan Biniok]

> Such as?

3) And there are a lot of scripts that rely on the GM_xmlhttpRequest finalUrl property

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: wrxhrfix

I've traced this down specifically to the fact that background scripts using xhr have no tabId and thus will not be fed to any webrequest listeners.  As well, any XHR from traditional addons or system level will not have a browser associated with them and will not be given to webrequest listeners.  This patch  would remove these restrictions, but need to understand what the motivation behind these restrictions were.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: wrxhrfix

Slightly more correct, set tabId if if it is -1, to match chrome behavior.

Comment 17

[Tomislav Jovanovic :zombie]

(In reply to Shane Caraveo (:mixedpuppy) from comment #15)
> This patch  would remove these restrictions, but need to understand what the
> motivation behind these restrictions were.

I think that was on purpose, to limit web extensions to being able to affect only web content, and not browser code (like Firefox updates) or other addons.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8802763 [details] [diff] [review]
wrxhrfix

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

I think this change should be fine unless someone disagrees. I don't think there was a specific reason to skip non-tab loads. We should have a test for it though.

::: toolkit/components/extensions/ext-webRequest.js
@@ -26,5 @@
> -      if (!data.browser) {
> -        return;
> -      }
> -
> -      let tabId = TabManager.getBrowserId(data.browser);

This code was never supposed to exist in the first place. I added the "fill-browser-data" thing in response to a review comment and forgot to remove it.

@@ -53,5 @@
>        }
>  
> -      // Fills in tabId typically.
> -      let result = {};
> -      extensions.emit("fill-browser-data", data.browser, data2, result);

So please leave this part as-is and instead change the listener code here to unconditionally store the tabId:
http://searchfox.org/mozilla-central/rev/703b663355467293fad148ab7c2c5ee2b878e4d9/browser/components/extensions/ext-tabs.js#98

Comment 19

[Shane Caraveo (:mixedpuppy)]

Ok, permissions probably need to interact here somehow as well, I'll work that out.  As well, addons like ublock/adblock may use this is the filtering interface, and will want to be able to filter all requests, so I do think that neither restrictions currently in the code are what we want.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Attachment: wrxhrfix

This properly allows webRequest listeners access to non browser/tab requests.  It also attempts (will need sec-review) to prevent listener access to requests originating from system principal (avoiding issues discussed in Bug 1279371).

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8802763 [details] [diff] [review]
wrxhrfix

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

I'm pretty sure we still want to prevent WebExtensions from intercepting background requests from chrome code, particularly for things like add-on updates and OCSP checks. So we should probably restrict this by loading principal, and add some tests for those cases.

Comment 22

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Kris, did you look at the other patch?

Comment 23

[mmun]

Why Prevent *listening in* on requests originating from system principal to prevent *tinkering* with OCSP, app and addon updates, etc.?  Is it not possible to support read only access while rejecting attempts to modify, cancel, and such?  Is it not possible to have a separate permission that controls the level of access a WebExtension is granted?

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8803571 [details] [diff] [review]
wrxhrfix

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

Sorry, I followed the previous feedback link from an email.

f+ on the approach, but please add tests to make sure that system requests actually aren't accessible.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_background_events.html
@@ +23,5 @@
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: [
> +        "webRequest",
> +        "<all_urls>"

Nit: Trailing comma.

@@ +38,5 @@
> +      ];
> +
> +      function listener(name, details) {
> +        browser.test.assertTrue(eventNames.includes(name), `recieved ${name}`);
> +        eventNames.splice(eventNames.indexOf(name), 1);

Nit: (same as below) Please use a Set.

@@ +39,5 @@
> +
> +      function listener(name, details) {
> +        browser.test.assertTrue(eventNames.includes(name), `recieved ${name}`);
> +        eventNames.splice(eventNames.indexOf(name), 1);
> +        if (eventNames.length < 1)

Nit: Add braces.

@@ +43,5 @@
> +        if (eventNames.length < 1)
> +          browser.test.sendMessage("done");
> +      }
> +
> +      eventNames.forEach(function (name) {

Nit: A for-of loop would be preferable, but failing that, an arrow function.

@@ +65,5 @@
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {
> +      permissions: [
> +        "webRequest",
> +        "<all_urls>"

Nit: Add trailing comma.

@@ +80,5 @@
> +      ];
> +
> +      function listener(name, details) {
> +        browser.test.assertTrue(eventNames.includes(name), `recieved ${name}`);
> +        eventNames.splice(eventNames.indexOf(name), 1);

Nit: Please use a Set, and then `.has(...)` and `.delete(...)`

@@ +86,5 @@
> +
> +      eventNames.forEach(function (name) {
> +        browser.webRequest[name].addListener(
> +          listener.bind(null, name),
> +          { urls: ["https://example.com/*"] }

Nit: No space inside braces, and please move closing ) to this line.

@@ +91,5 @@
> +        );
> +      });
> +
> +      fetch("https://example.com/example.txt").then(() => {
> +        browser.test.assertTrue(eventNames.length == 0, "messages recieved");

`assertEq(0, ...)`

Comment 25

[Kris Maglione [:kmag]]

(In reply to mmun from comment #23)
> Why Prevent *listening in* on requests originating from system principal to
> prevent *tinkering* with OCSP, app and addon updates, etc.?  Is it not
> possible to support read only access while rejecting attempts to modify,
> cancel, and such?  Is it not possible to have a separate permission that
> controls the level of access a WebExtension is granted?

OCSP requests were just an example. If there's a good use case for extensions needing to monitor certain system requests, then it might be worth considering. But that's a different bug.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1273138 fix WebRequest for background pages,

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

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8804502 [details]
Bug 1273138 fix WebRequest for background pages,

https://reviewboard.mozilla.org/r/88446/#review87506

Looks good. I'm mainly just worried we need a few more tests for the chrome privilege checks.

::: toolkit/components/extensions/ext-webRequest.js:29
(Diff revision 1)
> -      }
> -
> +      let URI = Services.io.newURI(data.originUrl, null, null);
> +      let principal = Services.scriptSecurityManager.createCodebasePrincipal(URI, {});

The more I think about this, the more I think we should just check the loading/triggering principal directly (which I guess means changing WebRequest.jsm to include it). There are too many ways the principal -> URL -> principal round-trip could go wrong.

::: toolkit/components/extensions/test/mochitest/.eslintrc.js:19
(Diff revision 1)
> +    "ChromeWorker": true,
> +    "onmessage": true,

Please keep sorted. And DOM globals go in the top group, and should be `false` (not modifiable), rather than `true`.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_background_events.html:17
(Diff revision 1)
> +<body>
> +
> +<script type="text/javascript">
> +"use strict";
> +
> +SimpleTest.requestCompleteLog();

Please remove.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_background_events.html:37
(Diff revision 1)
> +      function listener(name, details) {
> +        // If we get anything, we failed.
> +        browser.test.fail(`recieved ${name}`);
> +      }

Should probably check that we do actually get notifications for requests from (e.g.) a non-privileged worker so we know this is valid.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_background_events.html:52
(Diff revision 1)
> +  yield new Promise(resolve => {
> +    let worker = new ChromeWorker("webrequest_chromeworker.js");
> +    worker.onmessage = event => {
> +      is(event.data, "Done!", "Wrong message!");
> +      resolve();
> +    };
> +    worker.postMessage("go");
> +  });

We should also test requests made from this context, and from an XHR and/or fetch triggered by a JSM.

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_background_events.html:43
(Diff revision 1)
> +      ]);
> +
> +      function listener(name, details) {
> +        browser.test.assertTrue(eventNames.has(name), `recieved ${name}`);
> +        eventNames.delete(name);
> +        if (eventNames.size < 1) {

`== 0` please. Any other size that's `< 1` implies the world has gone mad.

Comment 28

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8804502 [details]
Bug 1273138 fix WebRequest for background pages,

https://reviewboard.mozilla.org/r/88446/#review87506

> The more I think about this, the more I think we should just check the loading/triggering principal directly (which I guess means changing WebRequest.jsm to include it). There are too many ways the principal -> URL -> principal round-trip could go wrong.

The originUrl checked here is "loadInfo.triggeringPrincipal || loadInfo.loadingPrincipal", but I can move that down into WebRequest.jsm to use those directly.

> Should probably check that we do actually get notifications for requests from (e.g.) a non-privileged worker so we know this is valid.

The other test file uses a serviceWorker for a non-privileged test.

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8804502 [details]
Bug 1273138 fix WebRequest for background pages,

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

Comment 30

[Kris Maglione [:kmag]]

Comment on attachment 8804502 [details]
Bug 1273138 fix WebRequest for background pages,

https://reviewboard.mozilla.org/r/88446/#review88222

::: toolkit/modules/addons/WebRequest.jsm:548
(Diff revision 2)
>            fromCache: getData(channel).fromCache,
>          };
>  
>          if (loadInfo) {
>            let originPrincipal = loadInfo.triggeringPrincipal || loadInfo.loadingPrincipal;
> -          if (originPrincipal && originPrincipal.URI) {
> +          if (originPrincipal) {

This check isn't actually necessary anymore. There will always be both a `loadingPrincipal` and a `triggeringPrincipal` now.

::: toolkit/modules/addons/WebRequest.jsm:549
(Diff revision 2)
> +            commonData.isSystemPrincipal = Services.scriptSecurityManager
> +                                                   .isSystemPrincipal(originPrincipal);

I think we probably want `isSystemPrincipal(loadingPrincipal) || isSystemPrincipal(triggeringPrincipal)`, just to be safe.

Comment 31

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8804502 [details]
Bug 1273138 fix WebRequest for background pages,

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

Comment 32

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8804502 [details]
Bug 1273138 fix WebRequest for background pages,

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

Comment 33

[Shane Caraveo (:mixedpuppy)]

https://reviewboard.mozilla.org/r/88444/diff/3-4/

Kris, note the change to the test file in the above interdiff.  Using the blob directly resulted in an onErrorOccured, even before changes in this bug, however they were silently dropped because that event (at least in this case) had no tabId.  This error occurred only on non-e10s.  blob.content is a Blob instance, which I was under the impression could be used in xhr.send.  Feeling like that doesn't make sense.  Wondering what you think.

Comment 34

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d82d1000473

Comment 35

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8804502 [details]
Bug 1273138 fix WebRequest for background pages,

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

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e357271c0f7
fix WebRequest for background pages, r=kmag

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e357271c0f7

Comment 38

[Chris Mills [:cmills]]

If this has been fixed, then I don't think it still needs to be documented? Let me know if you disagree, and if so, wat you think needs to be done.