Maybe use net.isIPv6(hostHeader) here instead?

very minor nit... these can use template strings now.. [${hostHeader}]... not critical tho :-)

Isn't string concatenation faster than template strings?

I want to say that some benchmarks have shown that template strings do not introduce any performance issues, maybe even improve performance. Will have to look and see if I can find them though.

if you wrap this in a common.mustCall(function(req, res) { }), then the hadRequest can be be totally removed. It doesn't look like hadError is used at all, so that can be removed. With those two gone, we can also remove the process.on('exit') listener.

isn't this too heavyweight?

In my initial PR, IPv6 checking was done by hostHeader.indexOf(':'), just as you also suggested in #5308
However, now I agree that net.isIPv6() is a much more reliable way of testing for IPv6.

I don't necessarily think it's too heavyweight, since it relies on uv_inet_pton. I guess it's slightly slower than indexOf, but since it also checks for valid characters inside the string, I believe it's worth the extra cycles.

There were no check for correctness here before, so I assume that there are two possibilities:

1. Host is check before and is valid at this point, so if there is : then it can only possibly be IPv6 address.
2. Host isn't checked before, so it might be invalid. If it has : then it is either IPv6 or invalid. If we wrap invalid host in brackets it would still be invalid, so it doesn't really matter.

isIPv6 involves calling C++ code and also checking if argument is IPv4 before checking if it's IPv6.

Host value doesn't pass through proper validation. It actually relies on either url.parse() to produce a host value, or simply takes options.hostname || options.host. At this stage, especially if options object is passed to the request(), hostHeader could even be www.domain.tld:3000, before reaching my code; (which is not wrong, only unsupported atm). In this case, a simple : check would yield an invalid Host header [www.domain.tld:3000].

Secondly, the RFC says that if (IP is IPv6) then [IP], which in my view makes it mandatory to make absolutely sure that whatever I enclose within square brackets, is an IPv6 address and not anything else (even if invalid).

I've been considering all of the above right from my initial PR, including the fact that indexOf is faster than isIPv6. However, since all points of view express advantages and disadvantages, I also opted for net.isIPv6 in the end, because that allows my fix to avoid type assumptions and ultimately make the code terse, and enforces the RFC statement.

While the above explains my preference for net.isIPv6, if there's consensus on reverting it back to : matching, I'll happily do it.

Secondly, the RFC says that if (IP is IPv6) then [IP], which in my view makes it mandatory to make absolutely sure that whatever I enclose within square bracket

It doesn't say that wrapping invalid host with brackets makes it more invalid, that's all I'm saying.

Doing a quick scan for two :'s may be a simple workaround here. I agree that dropping down to the native code to check is a bit overweight for this.

Yea, I agree. Just ran some benchmarks and net.isIPv6 runs about 3-6 times slower than an indexOf check here.

nit: would just make this const server