Thank you for reporting.

At https://fetch.spec.whatwg.org/#http-redirect-fetch, there is an item

  If CORS flag is set and actualResponse’s location URL’s origin is not same origin with request’s current url’s origin, then set request’s origin to a unique opaque origin.

In this case, CORS flag is not set, so keeping the original origin is the correct behavior. What do you think?

I haven't really considered redirects for the non-CORS Origin header case so what the specification says might well be wrong (unless whoever reviewed that change did carefully consider it, which I doubt).

Note that per

> If the CORS flag is set, httpRequest’s method is neither `GET` nor `HEAD`, or httpRequest’s mode is "websocket", then append `Origin`/httpRequest’s origin, serialized and UTF-8 encoded, to httpRequest’s header list.

You wouldn't keep the header for the final request in this case given that it's a GET request.

That begs the question though what should happen if the method is preserved, as with 307 or 308. It doesn't seem good to keep exposing the original origin across various origins as-if it was responsible.

Thank you, you're right. We shouldn't add an origin http header for the GET request.

+avimehta@google.com who is the reporter of issue 746687. avimehta@google.com, please note that this is a public bug.

+davidben@ as well.

My "fix"[1] at the original bug is not needed any more on M62 because of [2] (this was the right fix for the original bug fyi). 
I would like to revert [1] on M62 to resolve this issue. I confirmed that issue 746687 didn't reproduce when I reverted [1] locally. Please note that [1] is merged to M61 but [2] is not.

In order to correct the reported behavior on M61, we need to

 - merge [2] to M61, and
 - revert [1] on M61.

bulkneets@, can you tell me how severe your situation is?

1: d46fe0bb849482f98ef849419dcad2efe0064fe5
2: dacc670f59624580b516fb299150c4ea8a7b7f72

Note that even if you fix that, there's still a problem here.

Say I do a POST from origin A to origin B. And origin B does a 307 redirect to origin C. And A, B, and C are all cross-origin from each other. That means we end up doing a POST to C. But the reported origin ends up being A, which is wrong as B is liable. That's why we set the origin to null for CORS in such cases. It seems we should do something similar for this POST scenario, no?

(For posterity, d46fe0bb849482f98ef849419dcad2efe0064fe5 should be 322fef8774794f175c4f049d233f974598ef92fa. Apparently there's a mirror of just //net?? I dunno.)

Ugh, looks like this is a bit of a mess of ad-hoc behaviors everywhere.

tl;dr:
1. I think I agree with the plan in comment #5. Merge dacc670f59624580b516fb299150c4ea8a7b7f72 and then revert 322fef8774794f175c4f049d233f974598ef92fa.
2. Additionally, net::URLRequest's logic to change origin to "null" does not match the CORS version of this logic due to the CORS flag. We should probably fix this. I believe this would remove the need for (1) but I think I now believe (1) makes us much self-consistent anyway.
3. Yeesh, this is a mess. It would be nice if Blink folks and Net folks could figure out a more coherent picture for redirects.


My understanding when I reviewed 322fef8774794f175c4f049d233f974598ef92fa was that behavior conflicted with CORS setting things to "null" on cross-origin POST -> GET requests, but it seems even in M60 that works fine? So perhaps I misunderstood that. I see in net-internals that requests actually get aborted and restarted, so I'm guessing Blink is restarting network requests when it needs to do anything particularly funny?

For CORS requests, none of Chrome, Safari, and Firefox strips the header on POST -> GET and all convert to "null" on cross-origin redirects.


For non-CORS requests:

In M60, Chrome strips it on a POST -> GET redirect (same- or cross-origin) and converts to null on a cross-origin POST -> POST redirect. (If cross-origin POST -> GET, the stripping logic wins.) Converting to "null" was originally added in 5fe460ffa7a39f010134cf7c5ca5311f274103e3 ( issue #465517 ) for precisely the reason in comment #7.

In M61, after 322fef8774794f175c4f049d233f974598ef92fa, the POST -> GET special-case was removed. Now we retain the header on same-origin POST -> GET redirects and, on cross-origin POST -> GET, we replace it with "null" like POST -> POST. This is the behavior change which tickled this bug.

Comparing to other browsers:

Firefox doesn't send non-CORS Origin headers at all. I take it the origin (hah) of Origin as a CSRF prevention mechanism never caught on widely?

Safari does, so they're a useful data point. On same-origin non-CORS redirects, they do *not* strip the header, POST -> POST or POST -> GET. On our end, that header wasn't stripped for a particularly strong reason: we were stripping Content-Type and I think someone included Origin in there since it was a POST-only header. Thus 322fef8774794f175c4f049d233f974598ef92fa was simpler and aligned us better with Safari...

However, I apparently forgot to test Safari on cross-origin non-CORS redirects. I have now. Safari strips the Origin header entirely on all cross-origin redirects, POST -> POST or POST -> GET. So there's the discrepancy.

Safari behavior means stripping the header on cross-origin POST -> POST, which I think is a bad idea. Since sites using Origin as CSRF protection must treat a missing header as a browser which doesn't implement this, this just means bouncing through a cross-origin 307 lets you bypass it all.

Our M60 behavior is awkwardly inconsistent with CORS and Safari but is a combination that didn't break the web before.

Our M61 behavior is more consistent with Safari, but apparently breaks the reporter's site. I'm less inclined now to care too much about matching Safari's behavior given this cross-origin hole. Thus, let's go back to the M60 behavior. Stripping it on GET is also more consistent with the "no Origin header on non-CORS GETs" rule. That logic in //net is unaware of the CORS stuff, but apparently Blink manages to make it work anyway, I guess with restarts? (We probably should make this cleaner...)


That said: "Fixing" the reporter's site by stripping it on GET is weird. Suppose they used a 307 redirect instead. In either M60 or M61, *that* would still send "null". Specifically, the scenario is A fetches A which redirects to B. That is a cross-origin redirect, but it's weird for the Origin header to turn into null since the origin is unambiguously A at this point. The CORS version of this behavior does *not* have this problem because of the CORS flag. That appears to track the first time we leave the request's origin. We probably should make the URLRequest version of this logic match.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/957d722f8c8216f619539af66801cb23d4267063

commit 957d722f8c8216f619539af66801cb23d4267063
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Thu Aug 31 08:40:49 2017

Revert "Set null origin in cross origin redirect even when method is altered"

This reverts commit 322fef8774794f175c4f049d233f974598ef92fa.

The removed logic is needed for "no-cors" POST (more specifically,
non GET / PUT) requests. The logic is harmful for "cors" POST
 (non GET / PUT) requests, but we don't need to process such
requests (at least made by blink), as blink handles such "cors" 
requests by itself.

Original change's description:
> Set null origin in cross origin redirect even when method is altered
> 
> URLRequest::Redirect modifies the origin header of the request as
> follows:
> 
>  1. If the method is altered (typically in response to 301/302), it
>     removes the origin header.
>  2. If the redirect is a cross-origin redirect, it sets a null
>     origin header.
> 
> This CL removes the first item for better conformance with the fetch
> spec.
> 
> Bug: 746687
> Change-Id: I577e6a5c4f00d78083deb3063e4285be00954cfe
> Reviewed-on: https://chromium-review.googlesource.com/595300
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: David Benjamin <davidben@chromium.org>
> Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#492897}

TBR=davidben@chromium.org,tyoshino@chromium.org,yhirano@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 746687,  760487 
Change-Id: Iccc610f5ccc48bde5d5baa6eafa11ed0e075aa19
Reviewed-on: https://chromium-review.googlesource.com/643406
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498789}
[modify] https://crrev.com/957d722f8c8216f619539af66801cb23d4267063/net/url_request/url_request.cc
[modify] https://crrev.com/957d722f8c8216f619539af66801cb23d4267063/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/957d722f8c8216f619539af66801cb23d4267063/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-leak-path-on-redirect-expected.txt
[modify] https://crrev.com/957d722f8c8216f619539af66801cb23d4267063/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-allowed-with-redirect-expected.txt
[modify] https://crrev.com/957d722f8c8216f619539af66801cb23d4267063/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-default-ignored-with-redirect-expected.txt
[modify] https://crrev.com/957d722f8c8216f619539af66801cb23d4267063/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-allowed-with-redirect-expected.txt

> I take it the origin (hah) of Origin as a CSRF prevention mechanism never caught on widely?

Firefox does plan to add it still: https://bugzilla.mozilla.org/show_bug.cgi?id=446344.

> We probably should make the URLRequest version of this logic match.

And the standard as well then somehow. Seems rather unfortunate to require a whole extra flag just for no-cors POST 307/308 redirects, but so be it I guess.

Filed https://github.com/whatwg/fetch/issues/593.

> Seems rather unfortunate to require a whole extra flag just for no-cors POST 307/308 redirects, but so be it I guess.

Could we perhaps use the same flag? The Fetch spec is a bit more complicated than I can wrap my head around, but if the flag is just "have we left the request's original origin yet?", that would give the same behavior for both.

Do you even need a flag, actually? It seems to just be:

if newURL.origin != oldURL.origin && request.origin != oldURL.origin {
  request.origin = null
}

It needs to be a flag, otherwise we'd say this POST came from A to the final A: A -> B -> A.

And it cannot be the same flag as we also use the CORS flag to enforce the CORS check which we don't want to do for "no-cors" requests.

What about the snippet I wrote above? I don't think that mess up A -> B -> A? At B -> A, the origin would have been set to a unique opaque origin already, which means request.origin != oldURL.origin.

I don't see how when going from B to A origin would have been an opaque origin already, but it would become one before the request was made, indeed. I think you might be correct that we don't need a flag here then. That would be nice.

Er, yes, that one. I got confused. :-)

I appreciate the quick fix,
I'm glad that it will not take a long time like sendBeacon's case.

This problem is caused by the generic CSRF block/CORS policy.
These rules are often written in httpd conf or middleware of web framework.

- 1. Block cross origin referrer header + POST method
- 2. Block unexpected origin header + ANY method

Unlike referrer's case, CORS GET response may contain sensitive data, so GET was also restricted when creating CORS policy.
These rules have versatility and were thought to have no side effects. (So it was applied to all path of our web app)

Block by Referrer rule is very popular, and CORS block policy is rare than Referrer rule.

In original, ancient Origin header concept https://tools.ietf.org/html/rfc6454
"The user agent MAY include an Origin header field in any HTTP request."

So, should I rewrite rule 2?

If origin header is also sent for a simple cross origin GET request like referrer header, rule 2 will be broken.

I want to know that,
is there standardized spec for "no Origin header on non-CORS GETs" rule?

Fixing via revert is largely to get things back to their original state. I think the bigger nuisance is that the switching to null needs to be more nuanced. See my comments above. But that's yet another behavior change and fixing a regression on branch caused by one behavior change by yet another behavior change sounds like a bad plan. Thus reverting for now is safer. :-)

That said, it seems omitting it on GETs and stripping from POST to GET is also corroborated by the specification here, so I think it's actually correct.
https://fetch.spec.whatwg.org/#origin-header

I'd like to merge [3] and [4] to M61.

See #0, #5 and #8 for details. [4] is fairly new, but I think requesting merge sooner than later is better because M61 is near-to-stable. Please also note that both [3] and [4] are revert changes.

3: https://chromium.googlesource.com/chromium/src/+/dacc670f59624580b516fb299150c4ea8a7b7f72
4: https://chromium.googlesource.com/chromium/src/+/957d722f8c8216f619539af66801cb23d4267063

Comment 8:

> My understanding when I reviewed 322fef8774794f175c4f049d233f974598ef92fa was that behavior conflicted with CORS setting things to "null" on cross-origin POST -> GET requests, but it seems even in M60 that works fine? So perhaps I misunderstood that. I see in net-internals that requests actually get aborted and restarted, so I'm guessing Blink is restarting network requests when it needs to do anything particularly funny?

One thing I'd like to note about the implementation details of Chrome (Blink) is that fetch() and XHR abort the ongoing fetching and create a new request in order to follow cross-origin (CORS flag set) redirect, but the others e.g. <img>, <form>, etc. don't do that.

So, fetch() and XHR never use the URLRequest's redirect handling logic for cross-origin (CORS flag set) cases. The other stuffs i.e. ones using ResourceFetcher's CORS logic directly are depending on URLRequest's redirect following logic.

yhirano@ has changed sendBeacon to do the same as the latter from M61 (by https://chromium-review.googlesource.com/c/chromium/src/+/541158) from directly using WebURLLoaderImpl (until M60). But regardless of the change, sendBeacon has been depending on URLRequest's redirect following logic.

And, regardless of what Blink has been doing inside the redirect handling callbacks, due to issue 665766, nothing has been propagated to net/ when one uses URLRequest's redirect following logic. I.e. only logics in the browser process have been able to modify the Origin header on redirect following and URLRequest::Redirect() has been taking that role.

This bug requires manual review: We are only 3 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 10:

> > I take it the origin (hah) of Origin as a CSRF prevention mechanism never caught on widely?
> 
> Firefox does plan to add it still: https://bugzilla.mozilla.org/show_bug.cgi?id=446344.

 Issue 641620  is one for Chrome, though it's suspended.

Comment 8:

> In M60, Chrome strips it on a POST -> GET redirect (same- or cross-origin) and converts to null on a cross-origin POST -> POST redirect. (If cross-origin POST -> GET, the stripping logic wins.) Converting to "null" was originally added in 5fe460ffa7a39f010134cf7c5ca5311f274103e3 (  issue #465517  ) for precisely the reason in comment #7.

To implement the step 10 of https://fetch.spec.whatwg.org/#http-redirect-fetch, it used origin comparison for the "actualResponse’s location URL’s origin is not same origin with request’s current url’s origin," part but the remaining part:

  extra_request_headers_.HasHeader(HttpRequestHeaders::kOrigin)

doesn't perfectly correspond to the "CORS flag is set" part. It sets the Origin header to null mistakenly even when it was same-origin before following the redirect (even for same-origin request, the Origin header is sent).

This is one of the causes of the regression.

We're only 3 days away from Stable and plan is to cut M61 Stable RC today (09/01). 

Per comment #18, #19 & #22 discussion is still going on. Could you pls provide quick justification on why we need following CL merges this late in M61 cycle as bug is mark as P2 and not a Stable blocker?

3: https://chromium.googlesource.com/chromium/src/+/dacc670f59624580b516fb299150c4ea8a7b7f72 (62.0.3195.0)
4: https://chromium.googlesource.com/chromium/src/+/957d722f8c8216f619539af66801cb23d4267063 (62.0.3202.0)

Also is this only applicable to Mac?

Please note we're only taking absolutely critical merges in.Thank you.

> > > I take it the origin (hah) of Origin as a CSRF prevention mechanism never caught on widely?
> > 
> > Firefox does plan to add it still: https://bugzilla.mozilla.org/show_bug.cgi?id=446344.
> 
>  Issue 641620  is one for Chrome, though it's suspended.

We already implement Origin as CSRF protection and have implemented it for a very long time. (Although, judging by this bug, a lot of the details are a bit of mess.) The Mozilla bug is about implementing it at all.  Issue #641620  appears to be about relaxing the GET constraint.

Ah, sorry, more platforms are affected.

>#23

Without [3] and [4] Chrome 61 may confuse some servers handling form submissions. I'm not sure how many servers are affected exactly, but the impact may be large.

Approved merge to M61.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93282fc8594114f26a3aa3406f33a1e39eda6906

commit 93282fc8594114f26a3aa3406f33a1e39eda6906
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Fri Sep 01 18:06:36 2017

Revert "Set null origin in cross origin redirect even when method is altered"

This reverts commit 322fef8774794f175c4f049d233f974598ef92fa.

The removed logic is needed for "no-cors" POST (more specifically,
non GET / PUT) requests. The logic is harmful for "cors" POST
 (non GET / PUT) requests, but we don't need to process such
requests (at least made by blink), as blink handles such "cors"
requests by itself.

Original change's description:
> Set null origin in cross origin redirect even when method is altered
>
> URLRequest::Redirect modifies the origin header of the request as
> follows:
>
>  1. If the method is altered (typically in response to 301/302), it
>     removes the origin header.
>  2. If the redirect is a cross-origin redirect, it sets a null
>     origin header.
>
> This CL removes the first item for better conformance with the fetch
> spec.
>
> Bug: 746687
> Change-Id: I577e6a5c4f00d78083deb3063e4285be00954cfe
> Reviewed-on: https://chromium-review.googlesource.com/595300
> Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
> Reviewed-by: David Benjamin <davidben@chromium.org>
> Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#492897}

TBR=davidben@chromium.org, tyoshino@chromium.org, yhirano@chromium.org


(cherry picked from commit 957d722f8c8216f619539af66801cb23d4267063)

Bug: 746687,  760487 
Change-Id: Iccc610f5ccc48bde5d5baa6eafa11ed0e075aa19
Reviewed-on: https://chromium-review.googlesource.com/643406
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#498789}
Reviewed-on: https://chromium-review.googlesource.com/647729
Cr-Commit-Position: refs/branch-heads/3163@{#1070}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/93282fc8594114f26a3aa3406f33a1e39eda6906/net/url_request/url_request.cc
[modify] https://crrev.com/93282fc8594114f26a3aa3406f33a1e39eda6906/net/url_request/url_request_unittest.cc
[modify] https://crrev.com/93282fc8594114f26a3aa3406f33a1e39eda6906/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-leak-path-on-redirect-expected.txt
[modify] https://crrev.com/93282fc8594114f26a3aa3406f33a1e39eda6906/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-allowed-with-redirect-expected.txt
[modify] https://crrev.com/93282fc8594114f26a3aa3406f33a1e39eda6906/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-default-ignored-with-redirect-expected.txt
[modify] https://crrev.com/93282fc8594114f26a3aa3406f33a1e39eda6906/third_party/WebKit/LayoutTests/http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-allowed-with-redirect-expected.txt

Also merged:

https://chromium.googlesource.com/chromium/src.git/+/b60787e90053820ff2caf85c8d1d0389a10a0df9

Filed issue #762069 for the remaining weirdness around non-CORS cross-origin redirects.