normalizeName should not throw

[pke]

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.

[JakeChampion]

@pke, could you post an example of the response header which was causing the error to be thrown please?

[nawaf-gmc]

j

[pke]

@JakeChampion just a header like "a header with spaces"

[JakeChampion]

@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 : res.setHeader('a header with spaces', 'hello there');

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}`);
});

[pke]

I'll try. Maybe using a reverse proxy like nginx that adds a header to each response.

[milgner]

I came up with a small Go app that can do this:

package main

import (
	"fmt"
	"net/http"
)

func crazyHeader(w http.ResponseWriter, req *http.Request) {
  w.Header().Set("This is defunct", "I am sorry")
  fmt.Fprintf(w, "Can you read this?")
}

func main() {
	http.HandleFunc("/", crazyHeader)
	http.ListenAndServe(":1337", nil)
}

It's now running on http://cronos.illunis.net:1337/

[JakeChampion]

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) }

Chrome's native fetch is the same as Firefox:

Safari is the same as well:

[JakeChampion]

@pke, If you are happy to submit a pull-request which changes this to a console.warn as you originally suggested, I would accept it and release the change to npm 👍

[pke]

Will do tonight! Thanks for consideration. And thanks to @milgner for setting up the POC server.

[pke]

@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?

[JakeChampion]

@pke of course. The line which calls parseHeaders is this one: https://github.com/github/fetch/blob/8f48ca686f9564bf52704e0e4b410ca970b17031/fetch.js#L518

[pke]

@milgner any idea how to test this in #950 without relying on the external server?