ArrayBuffer uses "application/octet-stream" as default Content-Type in impl about beacon HOT 19 CLOSED

Note that this is in theory covered by beacon/headers/header-content-type.html via
testContentTypeHeader(stringToArrayBuffer("123"), "", "ArrayBuffer");

The second parameter is the expected content type. However, the check function does:
assert_true(result.startsWith(contentType), "Correct referrer header result");

When contentType is the emptyString, result.startsWith(contentType) will always return true.

from beacon.

For future context, as I'm tracing this:

• https://bugzilla.mozilla.org/show_bug.cgi?id=1364925 is a followup to 1329298
• Tests from 1364925 landed in web-platform-tests/wpt@a99cfba

Upstreamed test (^) covers the "application/octet-stream" case, plus others. @annevk can we resolve this issue?

Re, emptyString: true, we can probably tighten that.

from beacon.

It would be good to fix the platform tests. WebKit and Blink use "application/octet-stream" in this case.

from beacon.

Fair enough. This is not beacon specific, right? Presumably, Fetch should have tests for this?

from beacon.

No idea if this is covered by Fetch tests.

Also, I don't know about Blink but I know WebKit is not using Fetch internally for Beacon. Fetch tests may therefore not show the issue.

The fact though that it is supposedly covered by one of the Beacon tests. The test just happens to be wrong.

from beacon.

@cdumez we're reworking our implementation of sendBeacon to be based on Fetch, as part of adding support for Fetch keepalive flag -- crbug.com/695939.

The fact though that it is supposedly covered by one of the Beacon tests. The test just happens to be wrong.

I'm confused, which part? The fact that it uses startsWith?

My comment on moving this discussion against Fetch is because you can initiate fetch/XHR/beacon with ArrayBuffer, and we need to agree on the content-type; this is not sendBeacon specific. It is true that not all sendBeacon implementations may rely on Fetch, and to cover that case we can keep the current test, which already covers this case. Is that coherent?

from beacon.

Oh, I indeed misunderstood your comment. Yes, I agree that this discussion should be against Fetch.

I was only arguing for the beacon test that uses startsWith to get fixed. Sorry about the misunderstanding.

from beacon.

Cool, glad we're on the same page :-)

Looking at the test, I don't see a simple way to fix it, short of adding a conditional check?

diff --git a/beacon/headers/header-content-type.html b/beacon/headers/header-content-type.html
index 4912675b79..c338151650 100644
--- a/beacon/headers/header-content-type.html
+++ b/beacon/headers/header-content-type.html
@@ -34,7 +34,11 @@ function testContentTypeHeader(what, contentType, title) {
   promise_test(function(test) {
     assert_true(navigator.sendBeacon(testUrl, what), "SendBeacon Succeeded");
     return pollResult(test, id) .then(result => {
-      assert_true(result.startsWith(contentType), "Correct referrer header result");
+      if (contentType.length == 0) {
+        assert_true(result.length == 0, "Content-Type header for ArrayBuffer should be empty");
+      } else {
+        assert_true(result.startsWith(contentType), "Correct Content-Type header result");
+      }
     });
   }, "Test content-type header for a body " + title);
 }

Would that work?

from beacon.

Maybe, easy enough to find out if you run the test and it starts failing in browsers. However, generally Content-Type's value is always a value that is completely known, other than the boundary parameter of multipart/form-data. It seems you could be much more strict overall here.

And note that this is not a discussion about the desired behavior. This is a discussion about making sure the sendBeacon() tests are correct and implementation bugs are filed. Only if implementations refuse those bugs, can this be moved to Fetch.

from beacon.

@cdumez ptal: web-platform-tests/wpt#6761

@annevk I did, and as expected, it fails in Safari and Chrome; I'm wondering about the code itself.

And note that this is not a discussion about the desired behavior. This is a discussion about making sure the sendBeacon() tests are correct and implementation bugs are filed. Only if implementations refuse those bugs, can this be moved to Fetch.

My point here is that Fetch should probably have a Content-Type test for ArrayBuffer, that's all.

from beacon.

Resolved via web-platform-tests/wpt#6761, closing.

from beacon.

My point here is that Fetch should probably have a Content-Type test for ArrayBuffer, that's all.

Unfortunately, that would probably be a breaking change at this point.

from beacon.

As for the code, my main feedback would be to test the complete values rather than continue with startsWith().

from beacon.

FYI, I am going to align WebKit with Blink for now and use "application/octet-stream". If WebKit does not set a Content-Type header on POST requests then CFNetwork will add one for us unfortunately :(
CFNetwork ends up using "application/x-www-form-urlencoded" which is worse IMHO.

from beacon.

Unfortunately, that would probably be a breaking change at this point.

@annevk Can you clarify what would be a breaking change for Fetch? Because of the CFNetwork behavior, WebKit on Mac/iOS currently sends "application/x-www-form-urlencoded" as Content type for ArrayBuffer/ArrayBufferView payload as well. My plan to send "application/octet-stream" Content-Type for such payloads in WebKit would impact both Fetch and Beacon.

from beacon.

It's a breaking change to use "application/octet-stream" without preflight. It's is a same-origin policy violation. Sure we can make that change, but it seems rather unfortunate.

I guess we can add it to the safelist though and hope for the best...

from beacon.

@annevk Oh, I did not realize not having a Content-Type header meant we did not do a CORS-preflight. I got confused because the Beacon spec sets the mode to "cors" either way. WebKit's implementation of Beacon was doing a CORS preflight already. I think my patch to switch to "application/octet-stream" in WebKit may have indeed changed behavior for Fetch though as we now probably require a preflight (assuming our implementation matches the Fetch spec).

from beacon.

Requiring a preflight for ArrayBuffer and friends is fine too, but I doubt that's what Chrome is doing.

from beacon.

Yes, I think you’re right. We’ll stick with no content type header at WebKit level and work with CFNetwork to make sure a content type header does not get added.

from beacon.