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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

See http://fetch.spec.whatwg.org/#redirect-steps step 6.

Patch coming up.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Cross-site redirects of a CORS request should null out the request origin.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

mPrincipalForOriginHeader sounds good. Or maybe just mOriginHeaderPrincipal?

Up to you.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

For what it's worth, locally I have it as mOriginHeaderPrincipal.

Still trying to figure out how to fix the tests...

Comment 10

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cba447567a56 with the tests fixed/augmented.

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cba447567a56