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

XMLHttpRequest: Content-Length tests #27837

Merged
merged 4 commits into from
Mar 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions fetch/content-length/api-and-duplicate-headers.any.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
promise_test(async t => {
const response = await fetch("resources/identical-duplicates.asis");
assert_equals(response.statusText, "BLAH");
assert_equals(response.headers.get("test"), "x, x");
assert_equals(response.headers.get("content-type"), "text/plain, text/plain");
assert_equals(response.headers.get("content-length"), "6, 6");
const text = await response.text();
assert_equals(text, "Test.\n");
}, "fetch() and duplicate Content-Length/Content-Type headers");

async_test(t => {
const xhr = new XMLHttpRequest();
xhr.open("GET", "resources/identical-duplicates.asis");
xhr.send();
xhr.onload = t.step_func_done(() => {
assert_equals(xhr.statusText, "BLAH");
assert_equals(xhr.getResponseHeader("test"), "x, x");
assert_equals(xhr.getResponseHeader("content-type"), "text/plain, text/plain");
assert_equals(xhr.getResponseHeader("content-length"), "6, 6");
assert_equals(xhr.getAllResponseHeaders(), "content-length: 6, 6\r\ncontent-type: text/plain, text/plain\r\ntest: x, x\r\n");
assert_equals(xhr.responseText, "Test.\n");
});
}, "XMLHttpRequest and duplicate Content-Length/Content-Type headers");
9 changes: 9 additions & 0 deletions fetch/content-length/resources/identical-duplicates.asis
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
HTTP/1.1 200 BLAH
Test: x
Test: x
Content-Type: text/plain
Content-Type: text/plain
Content-Length: 6
Content-Length: 6

Test.
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Service worker for the xhr-content-length test.

self.addEventListener("fetch", event => {
const url = new URL(event.request.url);
const type = url.searchParams.get("type");

if (type === "no-content-length") {
event.respondWith(new Response("Hello!"));
}

if (type === "larger-content-length") {
event.respondWith(new Response("meeeeh", { headers: [["Content-Length", "10000"]] }));
}

if (type === "double-content-length") {
event.respondWith(new Response("meeeeh", { headers: [["Content-Length", "10000"], ["Content-Length", "10000"]] }));
}

if (type === "bogus-content-length") {
event.respondWith(new Response("meeeeh", { headers: [["Content-Length", "test"]] }));
}
});
55 changes: 55 additions & 0 deletions service-workers/service-worker/xhr-content-length.window.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// META: script=resources/test-helpers.sub.js

let frame;

promise_test(async (t) => {
const scope = "resources/empty.html";
const script = "resources/xhr-content-length-worker.js";
const registration = await service_worker_unregister_and_register(t, script, scope);
await wait_for_state(t, registration.installing, "activated");
frame = await with_iframe(scope);
}, "Setup");

promise_test(async t => {
const xhr = new frame.contentWindow.XMLHttpRequest();
xhr.open("GET", "test?type=no-content-length");
xhr.send();
const event = await new Promise(resolve => xhr.onload = resolve);
assert_equals(xhr.getResponseHeader("content-length"), null);
assert_false(event.lengthComputable);
assert_equals(event.total, 0);
assert_equals(event.loaded, xhr.responseText.length);
}, `Synthetic response without Content-Length header`);

promise_test(async t => {
const xhr = new frame.contentWindow.XMLHttpRequest();
xhr.open("GET", "test?type=larger-content-length");
xhr.send();
const event = await new Promise(resolve => xhr.onload = resolve);
assert_equals(xhr.getResponseHeader("content-length"), "10000");
assert_true(event.lengthComputable);
assert_equals(event.total, 10000);
assert_equals(event.loaded, xhr.responseText.length);
}, `Synthetic response with Content-Length header with value larger than response body length`);

promise_test(async t => {
const xhr = new frame.contentWindow.XMLHttpRequest();
xhr.open("GET", "test?type=double-content-length");
xhr.send();
const event = await new Promise(resolve => xhr.onload = resolve);
assert_equals(xhr.getResponseHeader("content-length"), "10000, 10000");
assert_true(event.lengthComputable);
assert_equals(event.total, 10000);
assert_equals(event.loaded, xhr.responseText.length);
}, `Synthetic response with two Content-Length headers value larger than response body length`);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test captured the main weirdness. Somehow Chrome and Firefox (not sure how to run service worker tests in Safari yet) return "10000" despite the synthetic response having two Content-Length headers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfalken what do you think about this behavior? Chromium generates net::ERR_RESPONSE_HEADERS_MULTIPLE_CONTENT_LENGTH for multiple content-length headers in response coming from the network[1], but that seems missing in the service worker case[2].

1: https://source.chromium.org/chromium/chromium/src/+/master:net/http/http_stream_parser.cc;l=1043?q=RESPONSE_HEADERS_MULTIPLE_CONTENT_LENGTH%20file:net%2F&ss=chromium
2: https://source.chromium.org/chromium/chromium/src/+/master:content/common/service_worker/service_worker_loader_helpers.cc;l=89;drc=56c01dcef367cdbda4e2592a9d0d897f8272fdc7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without really knowing about the "response smuggling attack" cited in the comment, I would tend to say Chrome should treat responses from the SW and from the network as much as possible, so I think we should throw the same network error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think I disagree, as we it's only about handing a Response over a thread boundary.
  2. This does not create a network error in Chrome today for a response coming from the network. Multiple Content-Length headers with the same value are fine.
  3. If we did want to error for multiple Content-Length headers in a synthetic response where the headers have different values, we have to do a lot more work in writing those kind of conditions down.
  4. The problem here is that Chrome (and Firefox) return(s) 10000 rather than 10000, 10000. In Firefox that happens because the header lookup code apparently special cases Content-Length...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yutakahirano @mfalken is there a way we can move this forward? I suppose I can add a test for the network equivalent (which will likely also fail, but not because the response is rejected, as per above), though it's a bit out-of-scope for what I was going for it does seem good to test. Anything else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I went ahead and added a test for this and it seems that Chrome doesn't have any problems with duplicates from the network (but Firefox and Safari do). Convincing enough?

Copy link
Contributor

@yutakahirano yutakahirano Mar 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion here and would like to defer to @mfalken. I'm fine with the change overall.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed w3c/ServiceWorker#1571 as follow-up as this matches the current specification.


promise_test(async t => {
const xhr = new frame.contentWindow.XMLHttpRequest();
xhr.open("GET", "test?type=bogus-content-length");
xhr.send();
const event = await new Promise(resolve => xhr.onload = resolve);
assert_equals(xhr.getResponseHeader("content-length"), "test");
assert_false(event.lengthComputable);
assert_equals(event.total, 0);
assert_equals(event.loaded, xhr.responseText.length);
}, `Synthetic response with bogus Content-Length header`);
8 changes: 6 additions & 2 deletions xhr/event-upload-progress.any.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@ const remote = get_host_info().HTTP_REMOTE_ORIGIN + "/xhr/resources/corsenabled.
[remote, redirect].forEach(url => {
async_test(test => {
const client = new XMLHttpRequest();
client.upload.onprogress = test.step_func_done();
const data = "On time: " + url;
client.upload.onprogress = test.step_func_done(e => {
assert_true(e.lengthComputable);
assert_equals(e.total, data.length);
});
client.onload = test.unreached_func();
client.open("POST", url);
client.send("On time: " + url);
client.send(data);
}, "Upload events registered on time (" + url + ")");
});

Expand Down
31 changes: 31 additions & 0 deletions xhr/request-content-length.any.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
async_test(test => {
const client = new XMLHttpRequest();
const data = "This is 22 bytes long.";
let happened = false;
client.upload.onprogress = test.step_func(e => {
assert_true(e.lengthComputable);
assert_equals(e.total, data.length);
happened = true;
});
client.onload = test.step_func_done(() => {
assert_true(happened);
assert_true(client.responseText.includes(`Content-Length: ${data.length}`));
});
client.open("POST", "resources/echo-headers.py");
client.send(data);
}, "Uploads need to set the Content-Length header");

async_test(test => {
const client = new XMLHttpRequest();
const data = "blah";
const url = URL.createObjectURL(new Blob([data]));
client.open("GET", url);
client.send();
client.onload = test.step_func_done(e => {
assert_true(e.lengthComputable);
assert_equals(e.total, data.length);
assert_equals(e.loaded, data.length);
assert_equals(client.responseText, data);
assert_equals(client.getResponseHeader("Content-Length"), String(data.length));
});
}, "Fetched blob: URLs set the Content-Length header");