If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

UNCONFIRMED
Unassigned
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Request Handling
P3
normal
UNCONFIRMED
a year ago
8 days ago

People

(Reporter: The 8472, Unassigned, NeedInfo)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [design-decision-needed]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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.
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.
Component: WebExtensions → WebExtensions: Request Handling
Summary: Content Script XHR and fetch do not set correct Referer and Origin headers → Content Script XHR and fetch do not set Origin and Referer to the current content page
Whiteboard: [design-decision-needed]
(Reporter)

Comment 2

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

Updated

a year ago
Priority: -- → P3
Whiteboard: [design-decision-needed] → [design-decision-needed]triaged
(Reporter)

Updated

8 months ago
See Also: → bug 1322113

Comment 3

4 months ago
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
(Reporter)

Comment 4

4 months ago
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
Flags: needinfo?(lgreco)
Comment hidden (mozreview-request)

Comment 6

4 months ago
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.
Flags: needinfo?(lgreco)
Attachment #8874891 - Flags: feedback?(mixedpuppy)
Comment hidden (mozreview-request)

Updated

4 months ago
Attachment #8874891 - Flags: feedback?(mixedpuppy)

Comment 8

4 months ago
mozreview-review
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.
Kris, I'm curious what you think about backing out bug 1284020 per my comments above.
Flags: needinfo?(kmaglione+bmo)
Attachment #8874891 - Flags: feedback?(mixedpuppy)
(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.
Flags: needinfo?(kmaglione+bmo)
(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?
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.