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
normalizeName should not throw #930
Comments
@pke, could you post an example of the response header which was causing the error to be thrown please? |
This comment has been minimized.
This comment has been minimized.
@JakeChampion just a header like "a header with spaces" |
@pke I am having a hard time replicating the issue, mainly because I can not build a server which creates a header name containing spaces. Could you create a server which does that or do you know of a public one we can test against? I want to replicate the issue so that I can see what native fetch implementations do, such as the one in Firefox. Here is my attempt at creating an example server using NodeJS' http module, if you look at the logs, you can see that the http module throws an error when it gets to the codeon line 8 : const http = require('http');
const hostname = '127.0.0.1';
const server = http.createServer((req, res) => {
res.statusCode = 200;
res.setHeader('Content-Type', 'text/plain');
res.setHeader('a header with spaces', 'hello there');
res.end('Hello, World!\n');
});
server.listen(process.env.PORT || 3000, () => {
console.log(`Server running at port ${process.env.PORT || 3000}`);
}); |
I'll try. Maybe using a reverse proxy like nginx that adds a header to each response. |
I came up with a small Go app that can do this:
It's now running on http://cronos.illunis.net:1337/ |
Thanks @milgner. Firefox's native fetch does not error and instead ignores the header, I tested this by going to http://cronos.illunis.net:1337 and running this in the browser console: fetch.toString()
response = await fetch('http://cronos.illunis.net:1337')
for (let name of response.headers.entries()) { console.log(name) } |
@pke, If you are happy to submit a pull-request which changes this to a |
Will do tonight! Thanks for consideration. And thanks to @milgner for setting up the POC server. |
@JakeChampion one question: Just printing the invalid header would still add it to the headers array. So the behaviour would not be same like for fetch in Chrome and firefox. It's the easiest fix. To completely be on-par with mentioned browsers we would have to NOT add the header, which means the function should still throw, but the response header processing loop should protect itself when calling addHeader. I have yet to find the code where the response headers are parsed. Could you give me a hint please? |
@pke of course. |
https://github.com/github/fetch/blob/8f48ca686f9564bf52704e0e4b410ca970b17031/fetch.js#L53
I just ran into an issue with a rogue server that sends a white space delimited response header. My tests never caught this issue, because node-fetch safely ignores invalid headers and just omits them from the
response.headers
.I believe invalid header names should not render the whole response invalid and non-parsable.
I therefore propose to to merely emit a
console.warn
with the field name in question and its value.The text was updated successfully, but these errors were encountered: