"hostHeader.startsWith is not a function" for host header array

[pimterry]

• Version: v10.15.3 & v12.9.1, I suspect many others
• Platform: Linux zapp 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: HTTP

Passing the host header as an array to http.get results in hostHeader.startsWith is not a function.

This happens for arrays with multiple items, which is invalid of course, or a single item, which the docs & rest of the API seems to imply is valid; other header APIs treat setting header values of [x] and x as equivalent everywhere afaict.

Repro:

> http.get('http://example.com', { headers: { host: ['example.com'] } });
Thrown:
TypeError: hostHeader.startsWith is not a function
    at calculateServerName (_http_agent.js:247:20)
    at Agent.addRequest (_http_agent.js:162:26)
    at new ClientRequest (_http_client.js:276:16)
    at request (http.js:44:10)
    at Object.get (http.js:48:15)

It's arguable that this shouldn't be allowed at all, but it is convenient. In my case, I'm reducing to build headers from an array of inputs, and storing all values as arrays during that process, since it's easy & consistent. To avoid this bug I have to special case the host header and undo that. Other libraries including node-fetch do the same.

If we do want to consider this invalid, it would be nice if there was at least a clearer error. For example, setting the host header to undefined:

> http.get('http://example.com', { headers: { host: undefined } });
Thrown:
TypeError [ERR_HTTP_INVALID_HEADER_VALUE]: Invalid value "undefined" for header "host"
    at ClientRequest.setHeader (_http_outgoing.js:488:3)
    at new ClientRequest (_http_client.js:221:14)
    at request (http.js:44:10)
    at Object.get (http.js:48:15)

The error itself is thrown in _http_agent here: https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L247

[lpinca]

We could do something like this:

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index e1cfa6d7fc..225e8ccfa2 100644
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -238,8 +238,10 @@ Agent.prototype.createSocket = function createSocket(req, options, cb) {
 
 function calculateServerName(options, req) {
   let servername = options.host;
-  const hostHeader = req.getHeader('host');
+  let hostHeader = req.getHeader('host');
   if (hostHeader) {
+    if (Array.isArray(hostHeader))
+      hostHeader = hostHeader[0];
     // abc => abc
     // abc:123 => abc
     // [::1] => ::1

but I'm not very happy with it. If the user provides multiple values the host header will be added multiple times unless we special case it.

node/lib/_http_outgoing.js

Lines 429 to 433 in 8217990

	if (value.length < 2 || !isCookieField(key)) {
	for (var i = 0; i < value.length; i++)
	storeHeader(self, state, key, value[i], validate);
	return;
	}

I think it makes more sense to not accept an array but to have a better error message we have again to special case it.

[gntem]

I was looking into this indeed doesn't make sense to allow arrays and as the Host value can be only one host. it would be better as mentioned in the comment above to just throw errors.

diff --git a/lib/_http_agent.js b/lib/_http_agent.js
index e1cfa6d7fc..f36878a2b9 100644
--- a/lib/_http_agent.js
+++ b/lib/_http_agent.js
@@ -27,7 +27,11 @@ const net = require('net');
 const EventEmitter = require('events');
 const debug = require('internal/util/debuglog').debuglog('http');
 const { async_id_symbol } = require('internal/async_hooks').symbols;
-
+const {
+  codes: {
+    ERR_HTTP_INVALID_HEADER_VALUE,
+  },
+} = require('internal/errors');
 // New Agent code.
 
 // The largest departure from the previous implementation is that
@@ -240,6 +244,10 @@ function calculateServerName(options, req) {
   let servername = options.host;
   const hostHeader = req.getHeader('host');
   if (hostHeader) {
+    if (Array.isArray(hostHeader)) {
+      throw new ERR_HTTP_INVALID_HEADER_VALUE(hostHeader, 'host');
+    }
+
     // abc => abc
     // abc:123 => abc
     // [::1] => ::1

if everyone is ok with this I can make a PR and provide the tests for it.

[lpinca]

@gntem go ahead.

[pimterry]

While rejecting array hosts completely would definitely be a big improvement, if possible it would be nice to allow host arrays that have just one element as well.

It's useful for many of us who're building requests dynamically or from user input. In such cases, it's useful to keep all header values as arrays for consistency, and for the rest of the API that works fine, even for other header where only one value is allowed (e.g. content-length).

Totally agree of course that arrays that actually contain multiple values should be rejected.

If host is going to be a special header that can never be an array though, it's being treated as different to every other header, and everybody downstream will have to include their own hacky special cases to handle that (as libraries like node-fetch already have to: https://github.com/bitinn/node-fetch/blob/master/src/headers.js#L335-L340).

It'd be easy to avoid this and still make the API safe:

if (Array.isArray(hostHeader)) {
  if (hostHeader.length === 1) {
    hostHeader = hostHeader[0];
  } else {
    throw new ERR_HTTP_INVALID_HEADER_VALUE(hostHeader, 'host');
  }
}

[lpinca]

@pimterry I think it is reasonable. Feel free to open a PR.

[dfloresgonz]

I still the get error on node12.x (aws)

[Trott]

I still the get error on node12.x (aws)

Don't use an array for the value of the host header. In 12.x and below, it will result in this error. In 13.x and later, the error is more clear but it still throws.

NO: http.get('http://example.com', { headers: { host: ['example.com'] } });

YES: http.get('http://example.com', { headers: { host: 'example.com' } });