Cross-site redirects of a CORS request should null out the source origin, per spec

RESOLVED FIXED in mozilla20

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

unspecified
mozilla20
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CORS-testsuite])

Attachments

(1 attachment)

Created attachment 684145 [details] [diff] [review]
Cross-site redirects of a CORS request should null out the request origin.
Attachment #684145 - Flags: review?(jonas)
Note: this makes us fail a bunch of the tests in test_CrossSiteXHR.  At first glance, those tests are not following the spec, in that they do cross-origin redirects and expect to get the origin of the original page on the post-redirect request.  I'm going to see what Jonas thinks about whether the spec is nuts here before spending the time to fix that test, though.  ;)
The tests were written for old spec behavior which said to keep the 

The new spec behavior of setting "Origin:null" is better I think. Though I'm a little worried about website breakage. But I think we should give it a try.

I'm not sure I understand the cache-clearing behavior that this changes though.
I won't claim to understand the cache stuff at all.  

What the spec at http://fetch.spec.whatwg.org/#cache-and-network-error-steps (which is what I assume is relevant here, but could be wrong!) says is:

  Remove the entries in the preflight result cache where origin field value is a
  case-sensitive match for source origin and url field value is a case-sensitive match for
  request URL.

and "source origin" is what gets changed on redirects.  I have no idea whether this part of the spec is right, as I said.
Actually, it doesn't really matter if we use mRequestingPrincipal or mOriginPrincipal since you can't redirect requests that require preflights. That's enforced here:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCrossSiteListenerProxy.cpp#513

So I think we should just keep using mRequestingPrincipal.


Also, can we find a better name for mOriginPrincipal? My brain keeps reading that as mOriginalPrincipal which would be the principal which we started with. I.e. the opposite of what it is.
> Actually, it doesn't really matter if we use mRequestingPrincipal or mOriginPrincipal
> since you can't redirect requests that require preflights.

Ah, ok.  Excellent.

> Also, can we find a better name for mOriginPrincipal?

mSourceOriginPrincipal?  mPrincipalForOriginHeader?
mPrincipalForOriginHeader sounds good. Or maybe just mOriginHeaderPrincipal?

Up to you.
Comment on attachment 684145 [details] [diff] [review]
Cross-site redirects of a CORS request should null out the request origin.

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

r=me with that fixed.

::: content/base/src/nsCrossSiteListenerProxy.cpp
@@ +349,5 @@
>                                           nsIPrincipal* aRequestingPrincipal,
>                                           bool aWithCredentials)
>    : mOuterListener(aOuter),
>      mRequestingPrincipal(aRequestingPrincipal),
> +    mOriginPrincipal(aRequestingPrincipal),

Change this to one of the names discussed.

@@ +410,5 @@
> +          // XXXbz is this actually correct?  Or should we use
> +          // mRequestingPrincipal here?  The spec says to use mOriginPrincipal,
> +          // but after a cross-origin redirect that will be something
> +          // compeletely useless...
> +          sPreflightCache->RemoveEntries(uri, mOriginPrincipal);

Leave this as using mRequestingPrincipal. Though feel free to add a comment.
Attachment #684145 - Flags: review?(jonas) → review+
For what it's worth, locally I have it as mOriginHeaderPrincipal.

Still trying to figure out how to fix the tests...
http://hg.mozilla.org/integration/mozilla-inbound/rev/cba447567a56 with the tests fixed/augmented.
Flags: in-testsuite+
Whiteboard: [need review][CORS-testsuite] → [CORS-testsuite]
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/cba447567a56
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.