1280692 - sendBeacon should use 'no-cors' per default

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Ilya Grigorik]

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.

Comment 1

[Patrick McManus [:mcmanus]]

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..

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(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

Comment 3

[Anne (:annevk)]

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.

Comment 4

[Ilya Grigorik]

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..

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Reclassifying as DOM:Security and putting in the backlog so we can fix the issue.

Comment 6

[Anne (:annevk)]

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).

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1280692_update_sendbeacon_to_no_cors.patch

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.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1280692_update_sendbeacon_redirect_test.patch

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1280692_update_sendbeacon_origin_header_test.patch

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1280692_remove_redundant_sendbeacon_tests.patch

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58ac676a98da

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a477bb461f3c
https://hg.mozilla.org/mozilla-central/rev/52730f426ea0
https://hg.mozilla.org/mozilla-central/rev/670d4c99c1c7
https://hg.mozilla.org/mozilla-central/rev/08079892bec5

Comment 19

[Chris Mills [:cmills]]

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.