req.hostname: check first comma-delimited host

[iconoeugen]

Fixes#3494

[dsingleton]

We (Deliveroo) ran into this issue recently. We're working around it for now, but it would be great to get merged/released. Anything we can do to help?

[dougwilson]

This is planned to be merged in the next minor release. We are currently working though 3 different security reports since Jan 1 and that is where are bandwidth is at, as we're trying to get security issues addresses ASAP. I hope that makes sense. Depending on if a security update will require a new minor version, the next minor should be released mid Feb.

[wesleytodd]

Can someone link me to a spec which defines this as valid? I cannot see anything about multiple host values being allowed in these:

https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.23
https://tools.ietf.org/html/rfc7230#section-5.4

[dougwilson]

So this is for the header X-Forwarded-Host, different from Host. Unfortunately the X-Forwarded-Host head has no specification, and is ad-hoc however these forwarding proxies are implementing them. They would always only use a single value, and suddenly with multiple reports it seems maybe there is some new forwarding software out there adding multiple values to the header.

[dsingleton]

Depending on if a security update will require a new minor version, the next minor should be released mid Feb.

Sounds more than reasonable. Thank you 😄

[wesleytodd]

Oh, I miss-read the code change, my bad. I am still unsure what we should support here. The Forwarded header is the spec to use for the future, and technically any user could define their own value in X-Forwarded-Host. In nginx configurations, the basic way I have seen is like this:

proxy_set_header X-Forwarded-Host $host;

But in a configuration like this, they could as well have written:

proxy_set_header X-Forwarded-Host my-server.com,$host;

And without a spec, I am not sure we should be custom tailoring our support without some documentation from very popular reverse proxies like nginx or HAproxy, before doing these kind of things, because we have to support them once we add them

[dougwilson]

I think the original issue said it was happening on Openshift in some way. But yes, perhaps at least getting a link to product documentation could be useful.

[dougwilson]

I am working to clean up this pull request to land in 4.17. In particular, the added test in this PR actually passed against current master without the fix, so it would seem the test doesn't actually validate the change made here. I believe that is because req.hostname doesn't include the port number, and since the test as the first element with a port number, it cuts off the rest anyway. Edit nevermind.

[dougwilson]

I just added a few more tests to the PR and merged.