1295660 - Content Script XHR and fetch do not set Origin and Referer to the current content page

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P3)

Description

[The 8472]

Some sites only allow XHR/fetch to retrieve content if correct Referer and Origin headers are set. I'm not referring to CORS restrictions but to server-side deep-linking restrictions.

The page's own XHR is able to issue valid requests, but content scripts associated with the same page are not because they use a different XHR object which sets no referer and a null origin.

This makes the content script XHR less versatile than the untrusted page's XHR.

Comment 1

[Kris Maglione [:kmag]]

I'm not sure that setting the content page as the referrer is the correct behavior here. My gut instinct would be to set the extension script as the origin and referrer by default, but perhaps to allow the extension to override them with the values for the content page, if they so choose.

Comment 2

[The 8472]

I don't think extension-script origin makes much sense for content scripts. It's more reasonable choice for the background page.

Page scripts often want to "intrude" on external APIs and disguise themselves as something allowed to access them, which means need to satisfy server-side checks (user authentication, xsrf protection, deep-linking protection etc.) as if they came from whatever page they would normally come from.

Anyway, an override would be good, but the current default "Origin: null" is quite bad, even no origin header would be preferable.

Comment 3

[Andy McKay]

This has been added to the agenda for the May 30 WebExtensions Triage meeting at 10:30am PT.

Call in info: https://wiki.mozilla.org/Add-ons/Contribute/Triage#Details_.26_How_to_Join
Agenda: https://docs.google.com/document/d/1hKKRpGFIaAaI3G_HfPX2Nk8pCchyhUIKJB9y5sIrVV4/edit

Comment 4

[The 8472]

Also note that the patching of the xrayed window object[0] makes it difficult to safely obtain an xrayed version of the page's XHR instance. I.e. one can't use that as workaround to send the correct referrer and origin.


[0] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionContent.jsm#418-422

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

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

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8874891 [details]
Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

I've investigated this issue a bit and, as anticipated in Comment 4, in Bug 1284020 we have started to overwrite the window.XMLHttpRequest and window.fetch properties accessible from a content script with the related privileged globals (which are coming from the content script "expanded principal" sandbox and so they can successfully retrieve data from a server that doesn't allow the website in its CORS headers), and by doing so we have prevented the content script to easily makes requests that looks exactly like the ones that the webpage itself can create.

In my opinion it would be more than reasonable that a content script can creates such requests but it would be probably more reasonable to give it access to the original webpage window.XMLHttpRequest and window.fetch properties than creating a totally new feature like allowing to explicitly override the request headers through some kind of new API methods and/or options.

I've attached a patch that does exactly this ("keep a copy of the webpage window.XMLHttpRequest and window.fetch as pageXMLHttpRequest and pageFetch and ensures that the content script can use it and creates requests with the expected headers"), but I'd like to get an initial feedback on the proposed approach from Shane, before moving it to an actual "review" stage.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8874891 [details]
Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

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

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8874891 [details]
Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

https://reviewboard.mozilla.org/r/146266/#review150382

::: toolkit/components/extensions/ExtensionContent.jsm:433
(Diff revision 1)
> +        window.pageXMLHttpRequest = window.XMLHttpRequest;
> +        window.pageFetch = window.fetch;
>          window.JSON = JSON;
>          window.XMLHttpRequest = XMLHttpRequest;
>          window.fetch = fetch;
>        `, this.sandbox);

The patch is fine, but I really don't like this.

IIUC, before bug 1284020 in content scripts we had:

Fetch & XMLHttpRequest -> sandbox variant presumably allowing cross-domain (closer to a system xhr?)
window.fetch & window.XMLHttpRequest -> the web pages variant these

bug 1284020 changed both to be the sandbox variant, resulting in no access to the webpage xhr from the content script.  This was done because jquery does this (the reporter likely didn't understand the difference between window.XMLHttpRequest and XMLHttpRequest):

jQuery.ajaxSettings.xhr = function() {
	try {
		return new window.XMLHttpRequest();
	} catch ( e ) {}
};

Now we want to store the original window.XMLHttpRequest in a non-standard way.

I'm beginning to feel like bug 1284020 was not the right fix, that jquery was the problem there.  I'm not certain we should be trying to make jquery work this way.  I'm more inclined to backout 1284020 than add another xhr/fetch variable into the mix.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Kris, I'm curious what you think about backing out bug 1284020 per my comments above.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> Kris, I'm curious what you think about backing out bug 1284020 per my
> comments above.

I'd prefer not to modify window.XMLHttpRequest, but I don't think we can revert that change without causing a lot of breakage. And after bug 1208775 is fixed, there won't be a distinction between `window.XMLHttpRequest` and the global XMLHttpRequest, so it would only be a short term fix at best.

Comment 11

[Shane Caraveo (:mixedpuppy)]

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #10)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> > Kris, I'm curious what you think about backing out bug 1284020 per my
> > comments above.
> 
> I'd prefer not to modify window.XMLHttpRequest, 

We wouldn't be modifying it, we'd be removing the modification of it that was made.

> but I don't think we can
> revert that change without causing a lot of breakage. And after bug 1208775
> is fixed, there won't be a distinction between `window.XMLHttpRequest` and
> the global XMLHttpRequest, so it would only be a short term fix at best.

What breakage other than 3rd party libraries?

In any case though, given bug 1208775 a different solution would be necessary, 15 months though since that's been touched.  Should that be a we+ bug?


Straw man.  Anything we overwrite (ie. JSON, XHR, Fetch) could be added to a content object.

Cu.evalInSandbox(`
  content = {
    XMLHttpRequest: window.XMLHttpRequest,
    fetch: window.fetch,
    JSON: window.JSON,
  }
  window.JSON = JSON;
  window.XMLHttpRequest = XMLHttpRequest;
  window.fetch = fetch;
`);

Would that survive changes from bug 1208775?

Comment 12

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8874891 [details]
Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

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

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8874891 [details]
Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

https://reviewboard.mozilla.org/r/146266/#review193294

::: toolkit/components/extensions/test/mochitest/file_page_xhr.html:28
(Diff revision 3)
> +  });
> +
> +  postMessage({
> +    type: "testPageGlobals",
> +    hasPageXMLHttpRequest: !!window.pageXMLHttpRequest,
> +    hasPageFetch: !!window.pageFetch,

these seem usused

Comment 14

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8874891 [details]
Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

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

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8874891 [details]
Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

https://reviewboard.mozilla.org/r/146266/#review193294

> these seem usused

yep, it is not needed anymore, thanks for catching it (I should have run a grep on the rest of the patch after removing the not needed assertions in the test).

I've removed it in the updated patch (and I've also applied another minor tweak to the test, to fix the eslint errors related to accessing the `content` object from the content script)

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8874891 [details]
Bug 1295660 - Content Scripts should be able to make requests that looks as coming from the webpage.

https://reviewboard.mozilla.org/r/146266/#review195168

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7656b78bbc44
Content Scripts should be able to make requests that looks as coming from the webpage. r=mixedpuppy

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7656b78bbc44

Comment 19

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 20

[Will Bamberg [:wbamberg]]

I wasn't sure where to put this, but the content scripts guide seemed like the best place: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_scripts#XHR_and_Fetch.

Please let me know if we need anything else.

Comment 21

[Luca Greco [:rpl] [:luca] [:lgreco]]

Thanks will, the new section linked above looks correctly describes the behavior and how to choose when extension content scripts may want (or need) to use content.XMLHttpRequest or content.fetch instead of window.XMLHttpRequest or window.fetch.