Closed Bug 1508661 Opened Last year Closed Last year

origin header should not be set for GET and HEAD requests

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 1 obsolete file)

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."
Attached patch origin.patch (obsolete) — Splinter Review
Attachment #9026404 - Flags: review?(bugmail)
Attached patch origin.patchSplinter Review
Attachment #9026404 - Attachment is obsolete: true
Attachment #9026404 - Flags: review?(bugmail)
Attachment #9026427 - Flags: review?(bugmail)
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
Attachment #9026427 - Flags: review?(bugmail) → review+
Priority: -- → P2
That seems to be duplicated with bug 1444278
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
Flags: needinfo?(bugmail)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99c2d9a02b38
origin header should not be set for GET and HEAD requests, r=asuth
https://hg.mozilla.org/mozilla-central/rev/99c2d9a02b38
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Flags: needinfo?(bugmail)
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?

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!

Flags: needinfo?(amarchesini)

All good! Thanks.

Flags: needinfo?(amarchesini)

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?

@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

baku, want to comment here?

Flags: needinfo?(amarchesini)

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

Flags: needinfo?(amarchesini)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.