-
Notifications
You must be signed in to change notification settings - Fork 0
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
For compressed responses, omit Content-Length
and Content-Encoding
headers
#23
Comments
Removing the headers automatically loses information, and changing behaviour from fetch silently might be a footgun for some. Exposing the raw content is absolutely necessary for non-browser use cases. Maybe in addition to that, have a flag that when true makes the body coherent (by removing these headers)? |
If we're looking to stay as in-line with upstream as possible, then adding a new field for the raw content is probably the best as it keeps all other fields the same as upstream. A flag would still require users to think about compression, because they need to remember enabling it. That would complicate things further. |
I don't think removing the headers is such a good idea. But it would also require to being able to access the raw stream |
@mnot and @jimmywarting: I think there are two related but separate use cases that are being mingled here:
What @Skn0tt accurately points out, is that what we are doing in Fetch upstream right now, is 1) but very poorly. We decompress the stream, but then do not sanitize the rest of the response to reflect what we've done. Nor do we provide any indication to the user that we have performed decompression. This is actually very bad in the scenario where a new What I think you two are saying, is that in scenario 2) it would be detrimental to not expose I think that this issue (properly sanitizing the headers after automatic decompression) is a bug worth adressing upstream in Fetch. I've opened an issue here: whatwg#1729 |
Understand that they're likely to be reluctant to make any behavioural change here if it breaks existing code (which is very likely, somewhere). Fetch is a browser-focused API that made a reasonable assumption that their users had no need for compressed content. That assumption doesn't hold for intermediaries, which often need to handle the 'raw' stream. Adding support for access to the 'raw' stream is pretty straightforward; it's "correcting" the current behaviour of Fetch in a backwards-compatible way that's tricker. That's why I suggested a flag -- acknowledging this isn't a great solution, but it is backwards-compatible. |
Does it need to be a flag? What if, instead of a flag, This would mean two streams to access a shared underlying stream - not sure what performance implications that has for implementors. From what i've seen in the undici implementation, it would be fine as long as reading from the streams is mutually exclusive. From my (cursory) understanding of Deno's implementation, this also sounds doable. |
Adding |
In a world of perfectly-coherent naming, yes |
The
fetch
API is becoming the standard HTTP client for server usage. When proxying back a fetched resource to the user-agent, here's what i'd intuitively do:The URL in this example responds with a gzip-compressed SVG. As specced in 16.1.1.1 of
15.6 HTTP - network fetch
, thebody
stream contains an uncompressed version of the SVG, making compression transparent to the user. This works well for consuming the SVG.However, the spec does not require headers to mirror this decompression. Although the
Response
has uncompressed body, theContent-Length
headers shows it compressed length and theContent-Encoding
header pretends it's compressed. In nodejs/undici#2514 (the issue I originally opened), I've included a repro of what this means if theResponse
is proxied to the user-agent:body
ends up being longer than the headers described. This is not allowed in the HTTP spec, and leads clients like cURL to warn.The current workaround is to manually alter headers on responses from
fetch
:This is cumbersome and should live in library code. It can't live in frameworks, because it's impossible to differentiate a response produced by fetch (content-encoding header, but uncompressed body) from a compressed Response created in user code (same content-encoding header, but compressed body).
Instead of this workaround,
fetch
should require thecontent-length
andcontent-encoding
headers to be deleted when response body is decompressed.Some implementors already do this, including Deno and workerd. Netlify might implement the same for Netlify Functions 2.0.
I've checked undici (Node.js), node-fetch, Chrome and Safari - all of them expose the behaviour explained above, where the headers don't match the body.
An alternative solution for this would be whatwg#1524 - that way, frameworks can use the decompressed body field to tell compressed responses from uncompressed ones.
Summary:
What's the problem? compressed responses having
content-encoding
header, but decompressedbody
leads to incoherentResponse
What's the usecase? mostly reverse proxying
Is it relevant to upstream? Yes, potentially because of service workers.
What's my suggestion for a fix? Upon decompression,
content-length
andcontent-encoding
headers should be deleted.How do engines deal with this? Most have the bug, but Deno and workerd implemented the fix I propose.
The text was updated successfully, but these errors were encountered: