Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redirect on preflighted CORS requests generally impossible #204

Closed
nschloe opened this issue Jan 22, 2016 · 22 comments
Closed

Redirect on preflighted CORS requests generally impossible #204

nschloe opened this issue Jan 22, 2016 · 22 comments
Labels
addition/proposal New features or enhancements needs tests Moving the issue forward requires someone to write tests

Comments

@nschloe
Copy link

nschloe commented Jan 22, 2016

(From the mailing list.)

With the given state of the standard, it is impossible to design APIs that use redirection on authenticated resources and allow access by clients implementing the standard.

The reason for this is that redirects on preflight CORS requests are generally forbidden. An older version of the standard says

7.1.5 Cross-Origin Request with Preflight
If the response has an HTTP status code that is not in the 2xx range
Apply the network error steps.

I cannot find this passage in the latest revision, but it's perhaps been rephrased. (Am I right?)

This restriction seems too strict as it disallows valid (RESTful) use patterns.

Opinions?

@annevk annevk added the addition/proposal New features or enhancements label Jan 22, 2016
@annevk
Copy link
Member

annevk commented Jan 22, 2016

I think you are correct, that check seems to have gone missing somehow. I wonder why nobody caught that thus far. Will need to investigate when I have some more time.

I guess as-is the specification actually supports your use case.

@nschloe
Copy link
Author

nschloe commented Jan 22, 2016

I guess as-is the specification actually supports your use case.

It'd be great if we could further substantiate this. I'd then go ahead and file feature requests in Chrome and Firefox which both seem to go by the old error-on-non-2?? rule right now.

@jdm
Copy link
Member

jdm commented Jan 22, 2016

I read through the CORS-preflight-fetch steps, and it doesn't appear to actually follow redirects. It gets a single HTTP response back from the HTTP-network-or-cache step, but there's no processing that occurs as far as I can tell.

@nschloe
Copy link
Author

nschloe commented Jan 22, 2016

@jdm The preflight doesn't follow redirects alright. This is about the actual GET request that follows the preflight.

@nschloe
Copy link
Author

nschloe commented Jan 26, 2016

The respective section in the latest revision says:

  • Otherwise, request's redirect mode is "follow", run these substeps:
    1. If request's mode is "cors", request's origin is not same origin with locationURL's origin, and locationURL includes credentials, return a network error.
    2. If the CORS flag is set and locationURL includes credentials, return a network error.
    3. [...]

If "locationURL includes credentials" describes URLs of the type http(s)://username:[email protected], this should be fine.

@annevk
Copy link
Member

annevk commented Feb 15, 2016

I'm not sure what you mean by your last comment @nschloe.

As for the original report, as far as I can tell the standard already support CORS requests that require a preflight to follow redirects. Basically each request in the chain will be preceded by a CORS-preflight request to the same URL that expects a 2xx response. Then the actual CORS request will be made and for that the response code does not matter (i.e., 307 is okay), as long as it passes the CORS check.

So I guess what remains is someone filing bugs on browsers and hoping they implement these better semantics.

@nschloe
Copy link
Author

nschloe commented Feb 15, 2016

@annevk Indeed.

So I guess what remains is someone filing bugs

Platform tests for this are needed as well, the respective bug report is here.

@annevk
Copy link
Member

annevk commented Feb 15, 2016

I think we can close this then and leave this up to browsers.

@hillbrad, @mikewest, @wseltzer this "change" makes https://www.w3.org/TR/cors/ even more obsolete than it already is. Can someone rescind it?

@annevk
Copy link
Member

annevk commented Mar 4, 2016

@hillbrad, @mikewest, @wseltzer, ping.

@hillbrad
Copy link

hillbrad commented Mar 9, 2016

So, waaaay back on August 27th, I requested that the CORS REC be updated to
indicate that work since 2013 has continued in Fetch. @wseltzer, whatever
happened to that?

On Fri, Mar 4, 2016 at 3:12 AM Anne van Kesteren [email protected]
wrote:

@hillbrad https://github.com/hillbrad, @mikewest
https://github.com/mikewest, @wseltzer https://github.com/wseltzer,
ping.


Reply to this email directly or view it on GitHub
#204 (comment).

@wseltzer
Copy link

@hillbrad, @annevk looking into it, thanks.

@annevk
Copy link
Member

annevk commented Mar 25, 2016

@wseltzer any updates?

@annevk
Copy link
Member

annevk commented May 4, 2016

Going to close this since the actual issue is fixed. W3C not clearly communicating the state of their documents is not really something that needs to be tracked here.

@annevk annevk closed this as completed May 4, 2016
@annevk
Copy link
Member

annevk commented May 4, 2016

(Note that it's not technically fixed, ever since Fetch was written this has been possible. I simply forgot to add the restriction that the original version had, which we no longer want to have.)

@annevk
Copy link
Member

annevk commented Aug 4, 2016

Okay, so what I missed was that Fetch does copy this restriction after all, by setting redirect mode to "error".

@youennf pointed this out in #198 (comment).

So I'm going to reopen this and then I'm going to change Fetch to not do that since we have since determined that it is secure. And with the upcoming origin-wide CORS preflights it will be relatively cheap to have redirects that require a preflight.

@kopax
Copy link

kopax commented Oct 7, 2016

Any update ? why is this closed ?

@annevk
Copy link
Member

annevk commented Oct 7, 2016

Because it is fixed in the standard per the referenced commit.

@kopax
Copy link

kopax commented Oct 7, 2016

I have this error

(index):1 XMLHttpRequest cannot load http://localhost:8080/login?username=dka&password=dka. The request was redirected to 'http://localhost:8080/', which is disallowed for cross-origin requests that require preflight.

when trying to authenticate in my spring application. Is there anyway to pybass the issue ? I need to have custom header for the X-XSRF-TOKEN.

@annevk
Copy link
Member

annevk commented Oct 7, 2016

You should file an issue with the user agent.

@MattHartz
Copy link

Hi,

I'm receiving a 307 on my preflight request and I'm not sure how to catch that promise and redirect to it's "location". Any ideas on this?

Thanks!

@annevk
Copy link
Member

annevk commented Jan 18, 2017

I noticed this is still a mess. In particular the Gecko bug is closed but not fixed. There are also existing CORS tests on WPT that contradict the standardized behavior. And I'm not sure if there's a WebKit or Edge bug filed. Reopening until that is all resolved.

@annevk annevk reopened this Jan 18, 2017
@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Jan 18, 2017
bors-servo pushed a commit to servo/servo that referenced this issue Mar 10, 2017
Allow for redirects after a CORS-preflight

~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204).

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ~#14787 and~ #14519
- [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](#14519 (comment)).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15889)
<!-- Reviewable:end -->
bors-servo pushed a commit to servo/servo that referenced this issue Mar 10, 2017
Allow for redirects after a CORS-preflight

~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204).

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ~#14787 and~ #14519
- [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](#14519 (comment)).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15889)
<!-- Reviewable:end -->
@annevk
Copy link
Member

annevk commented Mar 13, 2017

I'm going to close this issue. Current status:

(I also ended up filing https://bugs.webkit.org/show_bug.cgi?id=169550 and https://bugzilla.mozilla.org/show_bug.cgi?id=1346747 based on the new failure tests.)

@annevk annevk closed this as completed Mar 13, 2017
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 13, 2017
…m ferjm:issue-14519-cors-preflight); r=avadacatavra

~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204).

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ~#14787 and~ #14519
- [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](servo/servo#14519 (comment)).

Source-Repo: https://github.com/servo/servo
Source-Revision: c90d3d2f06c03d153b2be3ffa9a941cbfe8508cc

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 8a0dde32f10f530604eec26929466f886c6cd610
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this issue Mar 15, 2017
…m ferjm:issue-14519-cors-preflight); r=avadacatavra

~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204).

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ~#14787 and~ #14519
- [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](servo/servo#14519 (comment)).

Source-Repo: https://github.com/servo/servo
Source-Revision: c90d3d2f06c03d153b2be3ffa9a941cbfe8508cc
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue Mar 16, 2017
…m ferjm:issue-14519-cors-preflight); r=avadacatavra

~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204).

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ~#14787 and~ #14519
- [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](servo/servo#14519 (comment)).

Source-Repo: https://github.com/servo/servo
Source-Revision: c90d3d2f06c03d153b2be3ffa9a941cbfe8508cc
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…m ferjm:issue-14519-cors-preflight); r=avadacatavra

~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204).

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ~#14787 and~ #14519
- [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](servo/servo#14519 (comment)).

Source-Repo: https://github.com/servo/servo
Source-Revision: c90d3d2f06c03d153b2be3ffa9a941cbfe8508cc

UltraBlame original commit: 7458bd403ad69e8c8348c11de0c8a30db7678843
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…m ferjm:issue-14519-cors-preflight); r=avadacatavra

~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204).

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ~#14787 and~ #14519
- [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](servo/servo#14519 (comment)).

Source-Repo: https://github.com/servo/servo
Source-Revision: c90d3d2f06c03d153b2be3ffa9a941cbfe8508cc

UltraBlame original commit: 7458bd403ad69e8c8348c11de0c8a30db7678843
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…m ferjm:issue-14519-cors-preflight); r=avadacatavra

~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204).

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix ~#14787 and~ #14519
- [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](servo/servo#14519 (comment)).

Source-Repo: https://github.com/servo/servo
Source-Revision: c90d3d2f06c03d153b2be3ffa9a941cbfe8508cc

UltraBlame original commit: 7458bd403ad69e8c8348c11de0c8a30db7678843
@whatwg whatwg deleted a comment from JefriReynaldi Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs tests Moving the issue forward requires someone to write tests
Development

No branches or pull requests

7 participants