1508661 - origin header should not be set for GET and HEAD requests

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

https://fetch.spec.whatwg.org/

4.5.5.10: "If the CORS flag is set, httpRequest’s method is neither `GET` nor `HEAD`, or httpRequest’s mode is "websocket", then append `Origin`/the result of serializing a request origin with httpRequest, to httpRequest’s header list."

Comment 1

[Andrea Marchesini [:baku]]

Attachment: origin.patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: origin.patch

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9026427 [details] [diff] [review]
origin.patch

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

This is a great cleanup and greening of the fetch tests!  Woo!

Restating:
- Before this patch, InternalRequest::GetRequestConstructorCopy and thereby Request::Constructor always would mark the fetch to send an origin header.  The initializing logic that set mForceOrigin to false would get clobbered in all uses, so they didn't matter.
- This patch changes the logic to instead decide whether to send the header or not based on the GET/HEAD step 4.5.5.10 of HTTP-network-or-cache-fetch.  The mode cannot be "websocket" for FetchDriver and so is not included in the logic.  For the CORS case, the main fetch unsafe header list match isn't possible for the code in question because high up in HTTP fetch we forbid their use in no-cors mode[1].  This just leaves us with CORS mode which is covered by us setting SEC_REQUIRE_CORS_DATA_INHERITS[2] which results in CORS checks being triggered[3] and which explicitly sets the origin flag itself[4].  So we don't need to handle that either.  Probably.

1: https://searchfox.org/mozilla-central/source/dom/fetch/FetchDriver.cpp#464
2: https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/dom/fetch/FetchDriver.cpp#508
3: https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/dom/security/nsContentSecurityManager.cpp#846
4: https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#1037

Comment 4

[Thomas Nguyen (:tnguyen)]

That seems to be duplicated with bug 1444278

Comment 5

[Thomas Nguyen (:tnguyen)]

Oh well, let's do it here, the remaining cors case could be handled in that bugs.

(In reply to Andrew Sutherland [:asuth] from comment #3)
> Comment on attachment 9026427 [details] [diff] [review]
> origin.patch
> 
> Review of attachment 9026427 [details] [diff] [review]:anup and greening of the fetch tests!  Woo!
>
> - This patch changes the logic to instead decide whether to send the header
> or not based on the GET/HEAD step 4.5.5.10 of HTTP-network-or-cache-fetch. 
> The mode cannot be "websocket" for FetchDriver and so is not included in the
> logic.  For the CORS case, the main fetch unsafe header list match isn't
> possible for the code in question because high up in HTTP fetch we forbid
> their use in no-cors mode[1].  This just leaves us with CORS mode which is
> covered by us setting SEC_REQUIRE_CORS_DATA_INHERITS[2] which results in
> CORS checks being triggered[3] and which explicitly sets the origin flag
> itself[4].  So we don't need to handle that either.  Probably.
> 

Andrew, I could see in wpt test
https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/testing/web-platform/tests/fetch/api/redirect/redirect-origin.any.js

We expected origin header is not going to be sent, but the CORS mode (fallback mode) with SEC_REQUIRE_CORS_DATA_INHERITS triggers origin header again. Do you have any idea? Thanks

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99c2d9a02b38
origin header should not be set for GET and HEAD requests, r=asuth

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/99c2d9a02b38

Comment 8

[Chris Mills [:cmills]]

Note to MDN writers:

I have added a note to the Fx65 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#APIs

In terms of other docs, it might be aan idea to add a note to the Fetch/Origin pages?

Comment 9

[Chris Mills [:cmills]]

I've documented this in both places:

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch#Syntax
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Origin

Again, I've not mentioned this in the browser-compat-data, as I thought it was minor, and would be hard to communicate in a non-confusing way.

Let me know if that's OK. Thanks!

Comment 10

[Andrea Marchesini [:baku]]

All good! Thanks.

Comment 11

[Giovanni]

Sorry for commenting on a closed bug, I came here following the link on MDN, and I'm wondering if the new behavior is the correct one.
Indeed, the Fetch spec says:

If the CORS flag is set, httpRequest’s method is neither GET nor HEAD, or httpRequest’s mode is "websocket", then append Origin/the result of serializing a request origin with httpRequest, to httpRequest’s header list

To mean that reads: "if any of those 3 condition is true, set the Origin header"; hence any CORS request should have the Origin header, not just a POST/PUT/DELETE/etc. one.

This is corroborated by the note in Section 3.1 (https://fetch.spec.whatwg.org/#origin-header)
which says:

The Origin header is a version of the Referer [sic] header that does not reveal a path. It is used for all HTTP fetches whose CORS flag is set as well as those where request’s method is neither GET nor HEAD. [...]

And furthermore Section 3.3.2

A CORS request is an HTTP request that includes an Origin header. It cannot be reliably identified as participating in the CORS protocol as the Origin header is also included for all requests whose method is neither GET nor HEAD.

(note the "also")

Am I reading this wrong, is the spec due for a clarification, is the documentation on MDN confusing and indeed Origin is set, or what is actually going on here?

Comment 12

[Steven Penny]

@Giovanni I agree. First this:

If the CORS flag is set, httpRequest’s method is neither GET nor HEAD, or
httpRequest’s mode is "websocket", then append Origin/the result of
serializing a request origin with httpRequest, to httpRequest’s header list

and this:

A CORS request is an HTTP request that includes an Origin header. It cannot be
reliably identified as participating in the CORS protocol as the Origin header
is also included for all requests whose method is neither GET nor HEAD.

these are vague, and frankly flat wrong. When a request is made via fetch, the
Origin header should always be sent, else how will the receiver be able to
block it? If the origin doesnt match the destination, then its a CORS
request. Both these paragraphs make it sound like a CORS request is something
the sender gets to choose. They dont. Any conforming browser should be
sending this header without exception else CORS doesnt mean anything anymore.
This seems pretty blatant and im not sure how this patch got through.

https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS

Comment 13

[Chris Mills [:cmills]]

baku, want to comment here?

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Discussion about the spec issue is now happening on https://github.com/whatwg/fetch/issues/871