Closed Bug 1280692 Opened 3 years ago Closed 3 years ago

sendBeacon should use 'no-cors' per default

Categories

(Core :: DOM: Security, defect)

44 Branch
All
Unspecified
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: igrigorik, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-followup-2016-07-05, [domsecurity-active])

Attachments

(4 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36

Steps to reproduce:

Test page: http://output.jsbin.com/suxagi/latest/quiet


Actual results:

Firefox WPT trace: http://www.webpagetest.org/result/160617_SX_1138/1/details/ -the request to bit.ly is made but the redirect is not followed. 

By comparison, Chrome follows the redirect chain:
http://www.webpagetest.org/result/160617_9Y_113E/1/details/


Expected results:

Firefox should follow the redirect chain.
Component: Untriaged → DOM
Product: Firefox → Core
Hardware: Unspecified → All
Thanks Ilya.

basically the test sends a POST beacon to bit.ly which issues a 301 redirect cross origin to httpbin.org and gecko cancels the httpbin.org load.

The beacon channel is opened with  nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS
https://dxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1334

afaict by looking at this in a debugger the redirect verifyer gets upset when the bit.ly 301 doesn't contain an Access-Control-Allow-Origin response header.

that seems wrong - it should be looking at the httpbin transaction (which does indeed contain ACAO: *).

Maybe christoph or Richard can sort it out..
Flags: needinfo?(rlb)
Flags: needinfo?(ckerschb)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: btpp-followup-2016-07-05
(In reply to Patrick McManus [:mcmanus] from comment #1)
> that seems wrong - it should be looking at the httpbin transaction (which
> does indeed contain ACAO: *).

I am not so sure about that. Our implementation checks for the right headers on the old (initial) channel [1] as well as on the redirected (new) channel [2]. Surely browsers implement same origin to cross origin redirects differently, and the spec [3] doesn't really help "...it is expected that APIs that will utilize this policy will have to handle a same origin request that results in a redirect that is cross-origin in a special way." But I am not convinced that the Chrome behavior is actually more correct than Firefox' behavior.

Given that we have been checking for the right headers on the initial channel [1] for so many years makes me doubt that that this is a bug but rather an intentional decision we made (predates my time though).

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#655
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#464
[3] https://www.w3.org/TR/cors/#cors-api-specification-redirect
Flags: needinfo?(ckerschb)
Note that [3] is not the specification, https://fetch.spec.whatwg.org/ is. TR/cors/ is broken.

If the redirect only includes `Access-Control-Allow-Origin: *` though that is not going to work as the request includes credentials.
We've landed updates to the Beacon spec to address this issue:
- For background see: https://github.com/w3c/beacon/issues/32
- Relevant spec update: https://github.com/w3c/beacon/pull/33/files

The behavior has now been fixed in Edge and works as previously in Chrome. Would be great to get FF aligned as well :) This is a blocking issue for a few large customers experimenting with sendBeacon who've blacklisted FF for time being..
Reclassifying as DOM:Security and putting in the backlog so we can fix the issue.
Component: DOM → DOM: Security
Summary: sendBeacon does not follow redirects → sendBeacon does not follow redirects due to missing Access-Control-Allow-Origin header
Whiteboard: btpp-followup-2016-07-05 → btpp-followup-2016-07-05, [domsecurity-backlog]
Note the discussion in https://github.com/w3c/beacon/pull/34 in particular. sendBeacon() probably never should have used CORS to begin with. It should just mimic <form method=POST> (and any Blob MIME type that goes beyond that should probably raise an exception; the idea that we'd use CORS at that point seems way magical).
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Flags: needinfo?(rlb)
Summary: sendBeacon does not follow redirects due to missing Access-Control-Allow-Origin header → sendBeacon should use 'no-cors' per default
Whiteboard: btpp-followup-2016-07-05, [domsecurity-backlog] → btpp-followup-2016-07-05, [domsecurity-active]
As explained in comment 4 and comment 6 sendBeacon should not use cors per default but only use it for blobs. Instead of throwing an error for blobs, I suppose we should just do CORS for blobs, we already have a testcase for that (test_beaconPreflightWithCustomContentType.html).

This change caused multiple tests to be outdated (patches will be attached soonish):
a) test_beaconCORSRedirect.html
We should keep that test (rename to beaconRedirect.html) and make sure that beacon does *not* use CORS and follows cross origin redirects.

b) test_beaconOriginHeader.html
Even though this test has become slightly redundant I suppose we can keep it and just flip the logic around to make sure the origin header is not send for beacon.

c) test_beaconPreflight.html, and test_beaconPreflightFailure.html
Those tests are completely redundant now because we don't do CORS for sendBeacon anymore.

d) test_beaconPreflightWithCustomContentType.html
We should keep this test because it tests CORS for blobs. If we decide to throw an error for blobs, then we would have to update that test as well.
Attachment #8773243 - Flags: review?(jonas)
Comment on attachment 8773243 [details] [diff] [review]
bug_1280692_update_sendbeacon_to_no_cors.patch

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

This seems fine with me, but only if we are sure that Chrome is actually going to do it. Chrome has a history of saying that they will make changes that are potentially backwards incompatible, but then not following through to actually make them for one reason or another.
Attachment #8773243 - Flags: review?(jonas)
Comment on attachment 8773244 [details] [diff] [review]
bug_1280692_update_sendbeacon_redirect_test.patch

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

rs=me. I didn't actually look at the test to make sure that it verifies the right thing. But if it passes and you feel confident that it's correct, then that's good with me.
Attachment #8773244 - Flags: review?(jonas) → review+
Comment on attachment 8773245 [details] [diff] [review]
bug_1280692_update_sendbeacon_origin_header_test.patch

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

rs=me. I didn't actually look at the test to make sure that it verifies the right thing. But if it passes and you feel confident that it's correct, then that's good with me.
Attachment #8773245 - Flags: review?(jonas) → review+
Comment on attachment 8773246 [details] [diff] [review]
bug_1280692_remove_redundant_sendbeacon_tests.patch

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

rs=me. I didn't actually look at the test to make sure that it verifies the right thing. But if it passes and you feel confident that it's correct, then that's good with me.
Attachment #8773246 - Flags: review?(jonas) → review+
Blocks: 1289387
(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #12)
> This seems fine with me, but only if we are sure that Chrome is actually
> going to do it. Chrome has a history of saying that they will make changes
> that are potentially backwards incompatible, but then not following through
> to actually make them for one reason or another.

It seems that Chrome has always been using 'no-cors' for sendBeacon -Anne filed Bug 1289387 as a follow up so we can land the changesets within this bug which definitely makes Firefox more compatible.
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a477bb461f3c
Update sendBeacon to use 'no-cors' per default. r=sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/52730f426ea0
Update sendBeacon redirect test. r=sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/670d4c99c1c7
Update sendBeacon origin header test. r=sicking
https://hg.mozilla.org/integration/mozilla-inbound/rev/08079892bec5
Remove redundant sendBeacon tests due to spec update of using 'no-cors' per default. r=sicking
Depends on: 1290065
Removing ddn. I'm not so sure there is anything to mention in the documentation after all, as surely this is expected behaviour? Let me know if I'm wrong.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.