-
Notifications
You must be signed in to change notification settings - Fork 518
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
[fetch] Omit Content-Length
and Content-Encoding
headers for compressed responses
#2514
Comments
This issue is related to #1462, but slightly different. It's getting more relevant with frameworks like Next.js and + hosting providers like Netlify moving towards a Request/Response-based API Handlers. In Next.js for example, the Route handler below looks innocent, but will break in production because of this issue. export const GET(req: Request) {
return fetch("https://www.gatsbyjs.com/Gatsby-Logo.svg")
} |
What does the spec say? This might be more of a WinterCG issue? |
As I outlined under "Additional Context", from my reading of the spec, it doesn't specify the behaviour around this. WinterCG's fork of the spec also doesn't have changes in the respective section. |
I understand the issue but I don't think changing this is worth it considering there is a very easy way workaround that you mentioned. The spec doesn't tell us to remove these headers, which makes sense because proxying isn't a concern for browser fetch. There's no solution where we stay in bounds with the spec, and therefore other platforms. |
I wouldn't consider the workaround easy. Compression is not top-of-mind for most developers, this is typically left to frameworks and hosting providers. I've outlined that they can't apply the workaround on their end, though. Could you elaborate on the browser fetch argument? My understanding is that undici is designed for usage in Node.js, where proxying is a concern, and not for other platforms. |
undici is, fetch isn't. |
if you only want to support node, I'd recommend undici.request which doesn't decompress the body for you |
What i'm reading out of your messages is that removing the If we brought this case to WinterCG and they end up making a change to their fork of fetch spec, would undici follow that? |
|
We try to follow WinterCG |
Sounds good! I'll try to get the thoughts of WinterCG on this. |
I don't think we should give a definitive answer on that... there hasn't been a case where we explicitly changed undici to conform with wintercg.
|
This would solve...
When fetching a compressed resource, one receives a
Response
object with:body
content-length
header containing the length of the compressed representationcontent-encoding
header that says the response is encodedHeaders are proclaiming that the body is compressed, but the body is decompressed!
When proxying this response back to a client without adapting the headers, this means the
body
stream will be longer than thecontent-length
header proclaimed, and an invalid HTTP message is sent. HTTP Clients like cURL throw an error on this.The same is true for
content-encoding
- clients will try to decompress the body, but it's not compressed.Expand to see a repro 🐱
This server proxies a gzip-compressed SVG Logo.
Requesting it in cURL without the workaround results the
Excess found in a read
error, and the connection is closed before the full SVG is transmitted.After adding the workaround, it works fine.
As can be seen in the repro above, one workaround is to remove the problematic header from the response. This reconciles the response, because the uncompressed
.body
is now accompanied by headers that correctly describe it as being uncompressed.Having this check within business logic is cumbersome, it should live in library code. Ideally, it would live in frameworks, but there's a problem: Based off nothing but a
Response
object, it's impossible to differentiate a response produced byfetch
(content-encoding
header, but uncompressed body) from a compressedResponse
created in user code (samecontent-encoding
header, but compressed body). So without further additions to theResponse
class, it's impossible to perform this reconcilation after theResponse
is created.Instead, this should live within
fetch
.The implementation should look like...
Add
headersList.delete("content-length"); headersList.delete("content-encoding")
around here:undici/lib/fetch/index.js
Lines 2158 to 2178 in b918b7a
This is probably a breaking change for some usecases, so it should probably be released under flag and rolled out in a major release.
I have also considered...
There's a userland workaround I mentioned above, but it has to live within frameworks code.
Alternatively, API frameworks could monkey-patch the global
fetch
, but this is ugly for all sorts of reasons.The best solution would be to add a
Response#compressedBody
field that contains the raw body sent over wire, without going through decompression. In a proxy usecase, this means that both decompression and compression can be skipped if the upstream service supports the compression mechanism accepted by the user-agent. This would be a much bigger API change, though. One of the Undici maintainers opened whatwg/fetch#1524 about this a year ago.Additional context
In the Fetch Spec, it's explicitly called out that the current approach makes
Content-Length
unreliable:I'm unsure about how to interpret this, but it could be seen as a reason to drop the
Content-Length
header entirely. If it's unreliable, why include it?Even if the proposed behaviour is not fully inline with the spec, I think the proxying usecase is good enough of a reason to deviate from it, similar to the other things mentioned in https://github.com/nodejs/undici#specification-compliance.
The proposed behaviour is what Deno implements as well.
The text was updated successfully, but these errors were encountered: