Closed Bug 1312864 Opened 3 years ago Closed Last year

Allow redirects for requests that require preflight

Categories

(Core :: DOM: Security, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: nottheoilrig, Assigned: vinoth)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

The Fetch Standard's editor confirms that the current version of the spec allows redirects for requests that require preflight [1].
I'm interested in updating Firefox accordingly.
In general, the approach I have in mind is to drop this test [2] from nsCORSListenerProxy::CheckPreflightNeeded()
Related to this is bug 1111834. It disallowed redirects for requests that require preflight, which was the old behavior prescribed by the spec.

[1] https://github.com/whatwg/fetch/commit/0d9a4db8bc02251cc9e391543bb3c1322fb882f2
[2] https://hg.mozilla.org/mozilla-central/file/tip/netwerk/protocol/http/nsCORSListenerProxy.cpp#l1039
Component: Untriaged → DOM: Security
Product: Firefox → Core
Anne, just to confirm. The spec update is that a request that required a preflight is then allowed to be redirected right? In more detail, perform a CORS preflight, then do the actual request which is then allowed to be redirected, right? If so, then the code in this patch is slightly wrong as far as I can tell, because CheckPreflightNeeded() checks if we need to set up a preflight channel.
Flags: needinfo?(annevk)
Let me try to explain by example.

Say https://example.org/1 redirects to httsp://example.org/2 which redirects to https://example.org/3.

I want to fetch https://example.org/1 from https://other.example/ using the DELETE method or something that requires a preflight. The result will be:

  preflight /1 (response here is still required to be 2xx)
  DELETE /1 (redirect response)

  preflight /2
  DELETE /2

  preflight /3
  DELETE /3

The six responses all need to allow CORS of course.
Flags: needinfo?(annevk)
Priority: -- → P3
Whiteboard: [domsecurity-backlog]
(In reply to Christoph Kerschbaumer [:ckerschb (limited availability)] from comment #2)
> If so, then the code in this patch is slightly wrong as
> far as I can tell, because CheckPreflightNeeded() checks if we need to set
> up a preflight channel.

The code in the patch currently disallows redirects for requests that require preflight by raising a network error when a subsequent preflight is needed:

  preflight /1
  DELETE /1

  preflight /2 <- HERE
  DELETE /2

  preflight /3
  DELETE /3

After the change, CheckPreflightNeeded() still sets doPreflight = true, so it proceeds with the subsequent preflight. (This is the desired behavior, according to the current spec.)
Do I understand your feedback correctly?
Comment on attachment 8804463 [details]
Bug 1312864 - Allow redirects for requests that require preflight.

Sorry for the lag of response here. Unfortunately I don't have the bandwidth to look at this patch at the moment and I am about to be on PTO. To avoid an even longer wait I'll try to forward the review request.

Ben, any chance I could convince you to review those changes?
Attachment #8804463 - Flags: review?(ckerschb) → review?(bkelly)
Okay, thank you for trying to forward the review request.
Jack, does your change effect this test?

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/api/cors/cors-preflight-redirect.html
  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/fetch/api/cors/cors-preflight-redirect.js

It seems like it should.  You can run this with:

  ./mach web-platform-tests fetch/api/cors/cors-preflight-redirect.html

The code looks ok, but I'd like to see a test as well.
Assignee: nobody → jack
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(jack)
Comment on attachment 8804463 [details]
Bug 1312864 - Allow redirects for requests that require preflight.

Dropping flag for now until the test can be sorted out.  Sorry for the delay.
Attachment #8804463 - Flags: review?(bkelly)
This bug is preventing users of Shaka Player from using HTTP redirects for load balancing [1].  Media applications that do this kind of load balancing currently cannot be run in Firefox.

We created a short page to reproduce.  Simply load the repro page [2] in Firefox and wait a few seconds for the request to fire.  You should see "OK, status = 200", indicating that the cross-origin redirect was followed.  In Firefox, you see "FAILED", indicating that the redirect was not allowed by the browser.

This spec change has already been implemented by Chrome & Edge.

[1] https://github.com/google/shaka-player/issues/666
[2] http://storage.googleapis.com/shaka-demo-assets/_bugs/cors_redirect/index.html
Hi, i'd like if you fix this bug. I need to use Firefox, because i'm also using WebRTC with IdP, that works only with Firefox.
Is anyone still working on this bug?
NI myself to check this again.  Seems things died off over the test in comment 7.
Flags: needinfo?(bkelly)
Actually, Christoph, do you have time to look at this now, by any chance?
Flags: needinfo?(bkelly) → needinfo?(ckerschb)
(In reply to Ben Kelly [:bkelly] from comment #13)
> Actually, Christoph, do you have time to look at this now, by any chance?

Sorry, but I have some P1 stuff on my plate for FF57. If it's super urgent, I can probably find someone within the team to take a look. Alternatively, maybe someone from the Necko folks could take this one on. Sorry :-(
Flags: needinfo?(ckerschb)
It seems that nobody has looked at this in a long time.  Can anybody subscribed to this issue nominate someone who might be able to fix it?
Ran across this issue when implementing an oauth token exchange via the Authorization header, which is pretty standard, but that header isn't among the "simple headers" list so it requires cors. We wanted to have the backend issue a 303 redirect for the account call if the access token successfully authenticated to an existing user, and after a number of double-takes on the spec, noticed that firefox doesn't yet support it. Anyway, I'm sure we can find a workaround, but it'd be lovely to get a sense of whether this is on anybody's radar. Thanks~!
Joey, Elle, thanks for the ping. It's on our radar. The main problem has been finding someone who has the time to work on this.
Flags: needinfo?(mrbkap)
Flags: needinfo?(ckerschb)
@Jack: thanks for the initial patch and sorry for the long wait here, if you wanna drag that patch over the finishing line yourself, please feel free to leave a comment. Otherwise :vino is going to take a look at this next week.
Assignee: jack → cegvinoth
Flags: needinfo?(ckerschb)
Priority: P3 → P2
Whiteboard: [domsecurity-backlog] → [domsecurity-active]
As much as I'd like to carry this across the finish line, I haven't been able to make time for it. Thank you for taking it!
Flags: needinfo?(jack)
Attachment #8804463 - Attachment is obsolete: true
Comment on attachment 8988667 [details]
Bug 1312864 - Allow redirects for requests that require preflight

Please kindly review the patch and let me know if changes are needed.
Attachment #8988667 - Flags: review?(ckerschb)
Comment on attachment 8988667 [details]
Bug 1312864 - Allow redirects for requests that require preflight

Just noticed that few more wpt tests need to changed. Will request for review after making those changes.
Attachment #8988667 - Flags: review?(ckerschb)
Comment on attachment 8988667 [details]
Bug 1312864 - Allow redirects for requests that require preflight

Please kindly review the patch and let me know if changes are needed.
Corresponding TRY server push for this patch,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=089d535f2c50efd2c29c627af55b1131ecc4b7df
Attachment #8988667 - Flags: review?(ckerschb)
Comment on attachment 8988667 [details]
Bug 1312864 - Allow redirects for requests that require preflight

Yeah, I think that looks correct to me. Blake can you take another look at that to make sure we are good here?
Attachment #8988667 - Flags: review?(mrbkap)
Attachment #8988667 - Flags: review?(ckerschb)
Attachment #8988667 - Flags: review+
Comment on attachment 8988667 [details]
Bug 1312864 - Allow redirects for requests that require preflight

Blake Kaplan (:mrbkap) has approved the revision.

https://phabricator.services.mozilla.com/D1875
Attachment #8988667 - Flags: review+
Comment on attachment 8988667 [details]
Bug 1312864 - Allow redirects for requests that require preflight

Removing duplicate ,since already got r+ from blake.
Attachment #8988667 - Flags: review?(mrbkap)
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c932af8a13fd
Allow redirects for requests that require preflight
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c932af8a13fd
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Flags: needinfo?(mrbkap)
You need to log in before you can comment on or make changes to this bug.