See http://fetch.spec.whatwg.org/#redirect-steps step 6. Patch coming up.
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.
Whiteboard: [need review][CORS-testsuite] → [CORS-testsuite]
Target Milestone: --- → mozilla20
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.