1345788 - CORS doesn't correctly enforce restrictions on headers across redirects

Status: RESOLVED WORKSFORME

(Core :: Networking: HTTP, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

See https://github.com/w3c/web-platform-tests/pull/5070 for tests.

The basic problem is that when doing a redirect that doesn't involve preserving the method, SetupReplacementChannel does NOT copy headers.  So XHR ends up having to copy them manually in XMLHttpRequestMainThread::OnRedirectVerifyCallback (see the mAuthorRequestHeaders.ApplyToChannel call).

But this happens too late, after we've done CORS checks that depend on headers.  In particular, XMLHttpRequestMainThread::OnRedirectVerifyCallback is called from XMLHttpRequestMainThread::AsyncOnChannelRedirect which is called from nsCORSListenerProxy::AsyncOnChannelRedirect.  And nsCORSListenerProxy::AsyncOnChannelRedirect is doing a bunch of checks before calling MLHttpRequestMainThread::AsyncOnChannelRedirect.

As a result, when it calls UpdateChannel and in there calls CheckPreflightNeeded which does this:

  nsresult rv = http->GetRequestHeader(NS_LITERAL_CSTRING("Content-Type"),
                                       contentTypeHeader);

it gets NS_ERROR_NOT_AVAILABLE, because the code that will set the header hasn't run yet.

I don't see how to make this work in the XHR code without changes on the necko side.

Most simply, SetupReplacementChannel could copy over headers on redirect even when preserveMethod is false.  I _think_ that's what https://fetch.spec.whatwg.org/#concept-http-redirect-fetch is specced to do anyway, so the fact that we only copy them for XHR is kinda broken to start with...

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

In addition to the WPT test it would be helpful to add this case to our mochitest XHR+CORS+redirect tests:

https://dxr.mozilla.org/mozilla-central/source/dom/security/test/cors/test_CrossSiteXHR.html#923

+Ehsan since he moved nsCORSListenerProxy into necko.

Also, it seems this probably effects fetch() as well.  We have to set our headers explicitly on redirect there as well:

https://dxr.mozilla.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#742

Comment 2

[Patrick McManus [:mcmanus]]

I haven't spent the time to really think about the CORS/XHR aspect of this.. so I'll comment later on that.

But in general, blindly copying request headers between origins is something that is not done as some of them are generated to be origin specific. (cookies, auth, maybe others..)

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Christoph may have ideas here as well.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yes, I agree that copying all headers is not really right.  But AddHeadersToChannelVisitor already has a IsHeaderBlacklistedForRedirectCopy() check which avoids copying Cookie and some other headers.

In spec terms, https://fetch.spec.whatwg.org/#http-fetch just keeps using the same "request" for the new request.  But when doing the actual network fetch in https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch it clones the request and sets its "internal" headers of various sorts on _that_.  So looks to me like if the fetch consumer sets Cookie that would propagate across redirects, per spec, but Cookie bits set by necko itself would not?  It's a bit hard to tell for sure.

Anyway, the spec definitely seems to have separate concepts of "the headers we started with" and "the headers that go on the wire" an the former are copied on redirect, while the latter are not, afaict.  Anne, is that correct?

We _could_ continue to copy headers in XHR and Fetch, though there's some whack-a-mole involved and afaict per spec the "Accept" header is set early enough that it _does_ get copied, whereas for us I expect we lose the "Accept" header for a load that redirects....  Probably other spec violations too.  In that case, we would need to move the nsCORSListenerProxy checks out of AsyncOnChannelRedirect and into OnRedirectVerifyCallback or something.  I _think_ we could do those there and just pass a non-NS_OK arg to it to get similar behavior to what returning error from AsyncOnChannelRedirect has?  We'd need to make sure that nsCORSListenerProxy itself was a nsIAsyncVerifyRedirectCallback and passed itself to its mOuterNotificationCallbacks AsyncOnChannelRedirect, etc.

Comment 5

[Anne (:annevk)]

Yes, that is correct. Fetch takes care to separate headers that are set at the "API layer" (Accept, Accept-Language, client hints, author request headers) from headers that are set at the "network layer" (think Cookie, Referer, Origin, Host, User-Agent). Only the former are cloned (together with the request body).

Comment 6

[Anne (:annevk)]

As for your Cookie comment, there's no way to set Cookie headers at the "API layer" at the moment. If we ever did add that capability (I wontfixed https://github.com/whatwg/fetch/issues/268 due to lack of implementer interest), I'd expect them to propagate across redirects indeed.

Comment 7

[Anne (:annevk)]

Maybe useful: this difference between headers set at the network layer and API layer also comes into play with service workers. When a service worker gets a request that would never have a Host or Origin header in it. Or Referer for that matter. It would get Accept and client hints. That follows from the same logic.

Comment 8

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 9

[Selena Deckelmann :selenamarie :selena]

Junior - can you tackle this bug?

Comment 10

[Junior [inactive]]

Yes

Comment 12

[Boris Zbarsky [:bzbarsky]]

Hmm. It worries me that the patch in bug 1495880 doesn't actually address the issue that comment 0 describes. It's possible the tests are no longer triggering that issue, of course...

Comment 13

[Junior [inactive]]

Okay, bug 1495880 is about the preflight, and this is about redirection.
Not sure the relationship between web-platform-tests/wpt#5070 and this bug.

On the other hand, it's not clear to me that why preflight causes the main request check it's upload body again, rather than keep it's original headers.

REOPEN for moving the redirection forward.

Comment 14

[Junior [inactive]]

For Content-Type, we copy in the stack.

mozilla::net::HttpBaseChannel::ExplicitSetUploadStream
mozilla::net::HttpBaseChannel::ConfigureReplacementChannel
 mozilla::net::HttpBaseChannel::SetupReplacementChannel

https://searchfox.org/mozilla-central/rev/0815480479c19646f9330377ffd8730515fca4ec/netwerk/protocol/http/HttpBaseChannel.cpp#1011

IMO we're fine with Content-Type .
We send OPTIONS for POST with 307 redirection.

For POST to GET redirection, we already have a spec change to strip the upload body related headers.

We have a special handling for Accept:
https://searchfox.org/mozilla-central/rev/0815480479c19646f9330377ffd8730515fca4ec/netwerk/protocol/http/HttpBaseChannel.cpp#3527-3535

We still suffered from copying headers one-by-one in necko side, but we're fine with the issue in comment.

Comment 15

[Boris Zbarsky [:bzbarsky]]

mozilla::net::HttpBaseChannel::ExplicitSetUploadStream

Why are we reaching that? The whole point is that this is an issue when a redirect happens from a POST to a GET (so 303 or 307), hence the "redirect that doesn't involve preserving the method" bit. In that situation, why are we landing in ExplicitSetUploadStream at all? I'd expect the config passed to ConfigureReplacementChannel to have a null uploadStream in this situation.

For POST to GET redirection, we already have a spec change to strip the upload body related headers.

Ah, interesting.

What about custom headers? Are those copied over for non-XHR fetches when a 303 or 307 happens?

Comment 16

[Junior [inactive]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)

mozilla::net::HttpBaseChannel::ExplicitSetUploadStream

Why are we reaching that? The whole point is that this is an issue when a redirect happens from a POST to a GET (so 303 or 307), hence the "redirect that doesn't involve preserving the method" bit. In that situation, why are we landing in ExplicitSetUploadStream at all? I'd expect the config passed to ConfigureReplacementChannel to have a null uploadStream in this situation.

Oh, no. It reaches for POST->POST redirection. Sorry for confusing.
BTW 307/308 preserve method. 301/302 redirect from POST to GET. 303 redirects every method other than HEAD to GET.
see 4.4.11 https://fetch.spec.whatwg.org/#concept-http-redirect-fetch

For POST to GET redirection, we already have a spec change to strip the upload body related headers.

Ah, interesting.

What about custom headers? Are those copied over for non-XHR fetches when a 303 or 307 happens?

If you were talking about fetch(), It copies.
I believes it's here: https://searchfox.org/mozilla-central/rev/0815480479c19646f9330377ffd8730515fca4ec/dom/fetch/FetchDriver.cpp#1311

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, sorry, I misread ShouldRewriteRedirectToGET. So yes, 301/302/303 are the relevant bits here.

If you were talking about fetch(), It copies.

Ah, manually, like XHR. I guess other APIs can't really set custom headers, so this might not be directly observable, apart from the CORS interaction.

Comment 18

[Junior [inactive]]

To conclude,
we worry about headers are not copied for POST->GET redirection, but
(a) If method preserves, actually we copy Content-Type and Content-Length in necko side only and fetch#977 is going to make this to spec. (i.e., strip Content-Type for POST->GET)
(b) Another major concerns is nsCORSListenerProxy::AsyncOnChannelRedirect could update the channels before XHR and fetch() manually copies headers, but it should manipulate the origin-related headers, which is fine per comment 5 and bug 1240929 comment 19.

And we already copies headers for internal redirections https://searchfox.org/mozilla-central/rev/23d4bffcad365e68d2d45776017056b76ca9a968/netwerk/protocol/http/HttpBaseChannel.cpp#3646-3653

If we're fine with external redirections, it's less risky to mark as WORKSFORME.
What do you think, Boris?

Comment 19

[Boris Zbarsky [:bzbarsky]]

and fetch#977 is going to make this to spec. (i.e., strip Content-Type for POST->GET)

I think this part addresses the original issue I ran into, which was around the lack of copying of Content-Type on POST->GET redirection and how that interacts with CORS.

So yes, given that spec change I agree this is worksforme.