XMLHttpRequest: Content-Length tests #27837
Conversation
Some tests with progress events, Content-Length, and service workers.
wpt-pr-bot
requested review from
asutherland,
caitp,
jdm,
Manishearth,
mkruisselbrink,
wanderview and
wisniewskit
As the body concept is refactored for #604, XMLHttpRequest will need to use this algorithm for its events. We also want to require browsers to use this algorithm instead of the one defined by HTTP as part of #1156. Tests: web-platform-tests/wpt#10548 & web-platform-tests/wpt#27837.
Together with #1183 this solves the Fetch side of #604. Changes: * No longer set a length for network responses. Callers will have to parse Content-Length themselves. * Make extract set length so requests will set Content-Length correctly. XMLHttpRequest PR: TODO. Tests: web-platform-tests/wpt#27837.
The only information browsers use for progress events for responses is the Content-Length header. Align on that and only use body's length concept for uploads. Tests: web-platform-tests/wpt#27837. Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184.
The only information browsers use for progress events for responses is the Content-Length header. Align on that and only use body's length concept for uploads. Tests: web-platform-tests/wpt#27837. Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184.
| resolve(); | ||
| }); | ||
| }); | ||
| }, `Synthetic response with two Content-Length headers value larger than response body length`); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
- I think I disagree, as we it's only about handing a Response over a thread boundary.
- This does not create a network error in Chrome today for a response coming from the network. Multiple
Content-Lengthheaders with the same value are fine. - If we did want to error for multiple
Content-Lengthheaders 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. - The problem here is that Chrome (and Firefox) return(s)
10000rather than10000, 10000. In Firefox that happens because the header lookup code apparently special casesContent-Length...
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I don't have a strong opinion here and would like to defer to @mfalken. I'm fine with the change overall.
There was a problem hiding this comment.
I filed w3c/ServiceWorker#1571 as follow-up as this matches the current specification.
The only information browsers use for progress events for responses is the Content-Length header. Align on that and only use body's length concept for uploads. Tests: web-platform-tests/wpt#27837. Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184.
| resolve(); | ||
| }); | ||
| }); | ||
| }, `Synthetic response with two Content-Length headers value larger than response body length`); |
There was a problem hiding this comment.
I don't have a strong opinion here and would like to defer to @mfalken. I'm fine with the change overall.
Together with #1183 this solves the Fetch side of #604. Changes: * No longer set a length for network responses. Callers will have to parse Content-Length themselves. * Make extract set length so requests can set Content-Length correctly. XMLHttpRequest PR: whatwg/xhr#317. Tests: web-platform-tests/wpt#27837.
The only information browsers use for progress events for responses is the Content-Length header. Align on that and only use body's length concept for uploads. Tests: web-platform-tests/wpt#27837. Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184. Closes whatwg/fetch#604.
|
@annevk xhr-content-length.window.js is broken on all browsers: Because service workers require https. Can you please update the test to use |
The only information browsers use for progress events for responses is the Content-Length header. Align on that and only use body's length concept for uploads. Tests: web-platform-tests/wpt#27837. Fetch PRs: whatwg/fetch#1183 & whatwg/fetch#1184. Closes whatwg/fetch#604.
Some tests with progress events, Content-Length, and service workers.
Primarily for whatwg/fetch#604.