Cross-origin permissions no longer include subdomains

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mathieuu, Assigned: Ehsan)

Tracking

({addon-compat, regression})

52 Branch
mozilla53
addon-compat, regression
Points:
---

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52+ fixed, firefox53+ fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36

Steps to reproduce:

I am an addon developer and my users have reported my addon not working with Firefox development edition (currently 52) and Firefox nightly (currently 53). 

It works fine with the official release though (version 50).

After some digging I realized that none of the requests to my backend were getting through.

Installing the addon "CORS everywhere" allowed my user to login and access my backend, however API calls to `github.com`still return 404 and no trace appear in the network so I guess Firefox is still blocking those requests for some reasons. I hope it is a collateral damage of CORS permissions and not another one.


Actual results:

Firefox ignored the CORS permissions in `package.json` of addon:

  "permissions": {
    "cross-domain-content": [
      "https://xxxxxx.com",
      "https://github.com"
    ]
  }


Firefox 50 respects that but 52 and 53 don't.


Expected results:

Firefox 52 and 53 should respect CORS.

Comment 1

a year ago
What's your add-on? Is there a simple way to test the issue when your add-on is installed?
Flags: needinfo?(savy.mathieu)
(Reporter)

Updated

a year ago
Summary: Cross-origin permissions not working for addons → Cross-origin permissions no longer include subdomains
(Reporter)

Comment 2

a year ago
Created attachment 8814463 [details]
xpi and sources to build it

Comment 3

a year ago
Created attachment 8814467 [details]
bugcors.xpi
(Reporter)

Comment 4

a year ago
After some digging, I realised what the issue is. It seems that subdomains are no longer included in the cross-origin permissions. With Firefox 50, 

In the attachment 8814463 [details] just above, I built a small addon so you can test that. It injects in every web page the following content script:

```
$.ajax('https://api.github.com/repos/mathieuu/customopengrokq/issues/1').then(function(result) {
  console.error('api.github.com works fine: ', result);
}, function(err) {
  console.error('api.github.com throw an error: ', err);
});

```

The CORS permissions in package.json are:

```
  "permissions": {
    "cross-domain-content": [
      "https://github.com"
    ]
  }
```

Firefox 50 will work fine and execute: console.error('api.github.com works fine: ', result);

But Firefox 52 and 53 will throw an error: console.error('api.github.com throw an error: ', err);

Changing the permissions to https://api.github.com fixes the problem.

I am no longer sure if this is a bug or an intended behaviour however.

Comment 5

a year ago
Yes, I'm able to reproduce it, it see the error in the webconsole. I'll find a regression range.

Comment 6

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a7e664b5f6807eb714476ecdf2af1f9890623d4&tochange=997a19a765c8ad2f0b7dd07f4c8ff4126bfa352b

Ehsan Akhgari — Bug 1307730 - Disallow CORS fetches when we have an expanded principal; r=bzbarsky

Ehsan, is this expected after your patch?
Blocks: 1307730
Status: UNCONFIRMED → NEW
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
Component: Untriaged → DOM
Ever confirmed: true
Flags: needinfo?(savy.mathieu) → needinfo?(ehsan)
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
(Assignee)

Comment 7

a year ago
Yes, this is the intended outcome from bug 1307730.

Please allow me to explain some concepts and why we made this change and how you can fix your add-on accordingly.  Apologies if some of the below is repeating what you already know.

In order to be able to make a valid CORS request, the browser needs to compute an Origin header that describes the origin from which the CORS request is coming from.  Without the ability to construct an Origin header, the browser can't make a valid CORS request.  The Origin header is important because it allows the web server to check the origin of the cross-origin request, and perform the necessary validation to ensure that the request can be permitted.

In Gecko, "principals" are objects that track where specific objects are coming from.  Essentially they encode the origin of a JavaScript object.  These objects are used for our cross-origin security checks.  For example, when running code in a normal tab that has https://foo.com loaded, all of the objects in that tab are attached to a principal that encodes the "https://foo.com" origin.  When Gecko wants to perform a CORS request, it looks at the principal of the code performing the XHR, and constructs the Origin header based on that.

We also have the concept of an expanded principal.  An expanded principal is a special type of principal which annotates code that needs access to more than one origin.  These types of principals never occur in code that's directly related to a web page.  We do however use them for extensions, and for sandboxes.

For example, see the Sandbox created in the test case for bug 1307730: <https://hg.mozilla.org/integration/mozilla-inbound/rev/997a19a765c8#l2.40>.  We're passing two entities in the first argument, and as a result we create a sandbox with an expanded principal that can access both |window| and "https://example.org".  When code that is running inside this sandbox (and hence has the expanded principal associated with it) executes a cross origin XHR, we look at its principal trying to construct an Origin header, but it's unclear which actual origin in the expanded principal should be used here.  Before Firefox 52, we used to throw up our hands and send an "Origin: null" header, which is clearly bogus (but it seems that many web servers in the wild happily accept it!).  Now we reject such requests.

Now, the correct way to make CORS requests is to:

a) decide which origin you want your request to be associated with, and
b) run the XHR operation within the context of that origin.

What this means exactly depends on your add-on, but typically this would involve for example finding the correct window that has a document with the origin you intend to send the XHR from, and get the XMLHttpRequest object from the global object of that window, and perform the XHR that way.

Hope this helps.
Status: NEW → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(ehsan)
Resolution: --- → INVALID
(Reporter)

Comment 8

a year ago
Created attachment 8816635 [details]
xpi to reproduce oauth redirection
(Reporter)

Comment 9

a year ago
Thanks for the very detailed explanation. It allowed us to gain a significant amount of time.

However I think those changes break the way OAuth works.

To authenticate with OAuth the first step is to connect to a backend of your service which redirect you with a 302 status to the website you want to connect to.

In our case, the following requests are emitted by the extension:
(1) https://api.zenhub.io/auth/github  // Returns 302 Moved temporarily and return the URL to authenticate:
(2) https://github.com/login/oauth/authorize?response_type=code&redirect_uri=https://api.zenhub.io/auth/github/callback... // Returns 302 Found
(3) https://api.zenhub.io/auth/github/callback?agent=&code=...  // Return 200

But since Firefox 52 and the change on CORS, step 2 is no longer sent because of an origin header is added to (2)

What happens is the following:
(1) https://api.zenhub.io/auth/github is sent from our content-script and the request headers contains no origin, everything is fine.
The request returns 302 and the URL of (2) https://github.com/login/oauth/authorize?response_type=code&redirect_uri

However the browser redirection between (1) and (2) is now automatically adding to (2) the origin header: https://api.zenhub.io (because the redirection comes from that domain ?). Since (2) has Origin https://api.zenhub.io and the OAuth service of GitHub does not provide a response header allowing that origin, the request throws a CORS error.

I attached an archive called oauth-cors.zip containing an xpi and its source to reproduce to problem. 
- Install it in Firefox Nightly
- Open the remote debugger
- Open github.com in a tab
- In the remote debugger, console tab you should see a Cross-Origin Request Blocked
- In the remote debugger, network tab you should see that (1) https://api.zenhub.io/auth/github was executed without any Origin header and redirected (302) to (2) https://github.com/login/oauth/authorize?response_type=code&redirect_uri=xxxx which was this time sent with the Origin Header: https://api.zenhub.io

Is that an intended behavior? And if it is, is there a way to make OAuth work in an addon.
(Reporter)

Updated

a year ago
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
[Tracking Requested - why for this release]: Breaks some addons.

Thank you for the updated testcase.  Two quick questions:

1) Does the equivalent extension work in Chrome?  What headers is it sending there?
2) In Firefox 50, what do the Origin headers for the requests in the three steps look like?

I just checked, and https://github.com/login/oauth/authorize?response_type=code&redirect_uri is not sending any Access-Control-* headers as far as I can tell, so I assume the real change here is that we used to NOT make a CORS request after the redirect but now do make a CORS request, right?

Ehsan, do you have time to look at this?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Flags: needinfo?(ehsan)
Note that bug 1307730 made us simply deny some CORS fetches; it didn't change whether the fetches are CORS or not.  So I'm a bit confused about how this _used_ to work, unless we used to effectively do a non-CORS fetch and that determination happens after the new "deny it" check we added.
(Reporter)

Comment 12

a year ago
Yes it works in Chrome.

In Chrome and Firefox 50:

(1) https://api.zenhub.io/auth/github is sent manually from the addon without origin
(2) https://github.com/login/oauth/authorize?response_type=code&redirect_uri= is sent by 302 automatic redirection of (1) without origin
(3) ttps://api.zenhub.io/auth/github/callback?agent=&code=0eaf54b8c3a533fba154 is sent without origin


In Firefox 52/53

SAME: (1) https://api.zenhub.io/auth/github is sent manually from the addon without origin
DIFFERENT: (2) https://github.com/login/oauth/authorize?response_type=code&redirect_uri= is sent by 302 automatic redirection of (1) but with `https://api.zenhub.io` as origin
SAME: (3) ttps://api.zenhub.io/auth/github/callback?agent=&code=0eaf54b8c3a533fba154 is sent without origin

Basically, the node of the problem seems to be that Firefox 52 adds the URL of the redirected URL (1) as an origin header of the redirection request (2)
tracking this new regression for 52+
tracking-firefox52: ? → +
tracking-firefox53: ? → +
(Assignee)

Comment 14

a year ago
Firstly, thank you a lot savy for the detailed explanation, this is clearly a bug that I introduced.  I think the reason why I was confused in comment 7 is that I think this regression is misattributed to bug 1307730.  Based on my debugging so far, I think this is actually a regression from bug 1300908.

Here is what's happening:

Before bug 1300908, we had an expanded principal on the channel behind the XHR object.  So by the time that we got to <http://searchfox.org/mozilla-central/rev/36bfd0a8da5490274ca49c100957244253845e52/netwerk/protocol/http/nsCORSListenerProxy.cpp#928> with this callstack <https://gist.github.com/ehsan/6717e4d0aa986dcfe19895da9ea416ec> (that is, when the OAuth redirect happens), the security checks used to pass because the add-on had declared the following in its package.json:

  "permissions": {
    "cross-domain-content": [
      "https://github.com",
      "https://api.github.com",
      "https://api.zenhub.io"
    ]
  }

and we were trying to check a URI like this: <https://github.com/login/oauth/authorize?response_type=code&redirect_uri=https%3A%2F%2Fapi.zenhub.io%2Fauth%2Fgithub%2Fcallback%3Fagent%3D&scope=user%3Aemail%2Crepo&client_id=e7206457a9446ef325a6>

But after that bug, we now have a regular codebase principal for https://api.zenhub.io/ there so the security checks fail, and we end up setting up CORS headers and things start to go south from there.

The real goal of bug 1300908 was to affect the principal of the resulting document from the XHR, not really the principal that determines whether CORS headers must be sent.  I have a patch that does that explicitly and it makes the test add-on in attachment 8816635 [details] work.
Assignee: nobody → ehsan
Blocks: 1300908
No longer blocks: 1307730
tracking-firefox52: + → ?
tracking-firefox53: + → ?
Flags: needinfo?(ehsan)
(Assignee)

Comment 15

a year ago
Created attachment 8818750 [details] [diff] [review]
When an XHR is made with an expanded principal, use the expanded principal for CORS security checks

We should only ensure that the resulting document doesn't end up
inheriting the expanded principal.
(Assignee)

Comment 16

a year ago
Boris, do you mind reviewing this please?  Thank you!
Flags: needinfo?(bzbarsky)
Comment on attachment 8818750 [details] [diff] [review]
When an XHR is made with an expanded principal, use the expanded principal for CORS security checks

Ah, yes, now that we have separate triggeringPrincipal and principalToInherit, this is nice and simple.  And we introduced that in 52, so we can even backport this sanely.  ;)

It would probably make more sense to move the entire block that computes resultingDocumentPrincipal to right before it's actually used.  And maybe add a comment about how we want to use mPrincipal as the triggering principal, but don't want to inherit it as the document's principal, to make it clear what's going on?

r=me with that.

Also, my review queue is unlocked again, fwiw.
Flags: needinfo?(bzbarsky)
Attachment #8818750 - Flags: review+
tracking 52+ as new regression
tracking-firefox52: ? → +
tracking-firefox53: ? → +
(Assignee)

Comment 19

a year ago
Created attachment 8818992 [details] [diff] [review]
When an XHR is made with an expanded principal, use the expanded principal for CORS security checks

This time with a test that examines the scenario in this bug.
Attachment #8818992 - Flags: review?(bzbarsky)
(Assignee)

Updated

a year ago
Attachment #8818750 - Attachment is obsolete: true
Comment on attachment 8818992 [details] [diff] [review]
When an XHR is made with an expanded principal, use the expanded principal for CORS security checks

Huh.  Diff decide the other way around on which part moved...  OK.

r=me
Attachment #8818992 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 21

a year ago
Oh, wow!  Quite misleading.  :-)
(Assignee)

Updated

a year ago
Keywords: addon-compat
(Assignee)

Comment 22

a year ago
Comment on attachment 8818992 [details] [diff] [review]
When an XHR is made with an expanded principal, use the expanded principal for CORS security checks

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1300908
[User impact if declined]: This bug can break some add-ons, such as CORS Everywhere, and possibly others.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Landing it right now, but the automated test should be sufficient.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Because it's quite narrowly scoped and well understood.
[String changes made/needed]: None.
Attachment #8818992 - Flags: approval-mozilla-aurora?

Comment 23

a year ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e107a84c18
When an XHR is made with an expanded principal, use the expanded principal for CORS security checks; r=bzbarsky

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a5e107a84c18
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 25

a year ago
savy, any chance you could please retest this add-on with a new build of Firefox Nightly which you can get from here? <https://www.mozilla.org/en-US/firefox/channel/desktop/#nightly>  Please note that the first build which should have my patch should become available tomorrow.

Thank you!
Flags: needinfo?(savy.mathieu)
(Reporter)

Comment 26

a year ago
Looks like it fixed it. Thanks for the fix.

Will this patch be also pushed to Firefox 52?
> Will this patch be also pushed to Firefox 52?

Ehsan requested approval from release drivers to do that, yes.  I expect it will be granted, and then this will be pushed to 52.

Thank you again for reopening this and for the clear description of the problem!
Comment on attachment 8818992 [details] [diff] [review]
When an XHR is made with an expanded principal, use the expanded principal for CORS security checks

fix CORS/XHR regression for some extensions in aurora52
Flags: needinfo?(savy.mathieu)
Attachment #8818992 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 29

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/49e8216994f0
status-firefox52: affected → fixed
You need to log in before you can comment on or make changes to this bug.