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

🐛 Bug Report — fetch response with multiple content-length values yields "internal error" #2101

Closed
jasnell opened this issue May 9, 2024 · 0 comments · Fixed by #2111

Comments

@jasnell
Copy link
Member

jasnell commented May 9, 2024

When fetch receives a response with multiple content-length headers, the runtime throws an unhelpful "internal error".

Reproduction:

// Node.js script because workers does not let us set the content-length manually:
import { createServer } from 'node:http';
createServer((req, res) => {
  res.writeHead(200, {
    'content-length': [4,5,6],
  });
}).listen(8081);
// Then in workers
const resp = await fetch('http://localhost:8081');

The runtime will log an internal error indicating "invalid Content-Length header value" but does not propagate a useful error up to the user.

Per the spec, fetch should be tolerant of multiple content-length headers without failing and the spec includes an algorithm to extract the content length (https://fetch.spec.whatwg.org/#content-length-header) .. ideally we whould be using that to determine if the content-length is invalid. In the repro example case above, the content-length would be invalid given the inconsistent values but a more useful error should be reported.

In the case there are multiple content-length headers with the same value, e.g. content-length': [5,5,5], the runtime will report this as an invalid content-length and an opaque "internal error". Per the fetch spec, the extracted valid content length here is 5.

Note that per all the specs, having multiple content-length headers with different values is absolutely invalid and should result in an error (it just needs to be a useful one). RFC 9110 indicates that it is acceptable to have multiple content-length headers with the same value but that implementations have latitude to decide how to handle it. The fetch spec algorithm is tolerant of it and will normalize those duplicates away. Other implementations do, in fact, throw an error (like kj). If we are going to continue treating this case as an error rather than follow the fetch spec algorithm, we need to at least make it a useful error rather than a generic "internal error"

This might need to be addressed at the kj-http level more than in the runtime but opening the issue here to track.

jasnell added a commit that referenced this issue May 10, 2024
Previously an invalid content-length would result in an unhelpful
"internal error" message to the user. This intercepts that error
and replaces it with a more helpful message.

Longer term, it would be idea to align this more with the fetch
spec but for now, improving the error message is a good first step.

Fixes: #2101
jasnell added a commit that referenced this issue May 10, 2024
Previously an invalid content-length would result in an unhelpful
"internal error" message to the user. This intercepts that error
and replaces it with a more helpful message.

Longer term, it would be idea to align this more with the fetch
spec but for now, improving the error message is a good first step.

Fixes: #2101
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant