http: missing host header violation

[jasnell]

Per RFC 7230: "A server MUST respond with a 400 (Bad Request) status code to any HTTP/1.1 request message that lacks a Host header field and to any request message that contains more than one Host header field or a Host header field with an invalid field-value."

Node currently ignores this requirement. To test, create a simple server:

http.createServer(function(req,res) {
  res.end('ok');
}).listen(8080);

First, test the missing Host header

$ telnet localhost 8080
GET / HTTP/1.1

HTTP/1.1 200 OK

Second, test duplicate Host headers:

$ telnet localhost 8080
GET / HTTP/1.1
Host: A
Host: B

HTTP/1.1 200 OK

Third, test malformed Host headers:

$ telnet localhost 8080
GET / HTTP/1.1
Host: A, B

HTTP/1.1 200 OK

[bnoordhuis]

See my comment here: enforcing that requirement is moving into policy territory.

[jasnell]

Strongly disagree. There are very good reasons for the spec including these kinds of requirements. Namely, a server that does not handle the Host header appropriately can be tricked into disclosing sensitive information about the server itself (https://support.microsoft.com/en-us/kb/967342). While Node itself is not giving up any information currently, an application building on top of Node may not be prepared to deal appropriately with the missing header. In fact, it's reasonable for the application to expect Node to be handling this kind of thing for them. It's a simple check.

[rvagg]

I'm torn on this one, I see the argument for adding such a check, however Host is only meaningful in Node applications that actually read the host header, I suspect not a whole lot do because they are typically single-purposed and not akin to nginx, Apache or IIS (all of which do 400 on missing [and malformed?] Host). If an application does check Host then it'd be doing whatever checks make sense to it and if you're implementing something that's doing virtual-host style routing then you'd be implementing your own logic for that routing anyway which is where any such checks should be done.

So, I could probably be +0 for adding a check in core for the sake of correctness but that'd be all it's about, matching the header to a correct value has to be an application-level thing by definition. Core has no ability to decide what's correct and what's not except for malformed, missing or duplicate but none of those go to the security argument mentioned above.

[jasnell]

At most I'd suggest returning a 400 if the header is missing. If it's duplicated, I'd pick the first. Then, as you suggest, leave any further validation of the content to the application

[rvagg]

There's a philosophical discussion here that at least TSC members should tune in to, see also #3096. I suspect we're going to have to either come up with a clearer definition of what we're providing to users with the http module or make a compromise of some kind to resolve disagreements around this functionality. There's also a bunch of related issues for HTTP that both @jasnell and @ChALkeR have been cataloging that go to similar questions about what guarantees the http module should and should not be providing.

[ChALkeR]

@rvagg There is no reason to force everyone who is using http to re-implement the same basic validation logic. Imo, restrictions imposed by corresponding RFCs should be enforced (especially in the cases where omitting them could lead to security problems in applications). As it looks to me, most of the current modules do not expect invalid values coming from the http module (meaning that they do not check for it and everything is in the hands of the user).

[gtramontina]

I have to agree with @ChALkeR on this one. These restrictions can be read as "expected behavior" of any http server implementation.

[bnoordhuis]

It's not that clear cut. There are plenty instances where the spec is at odds with the real world.

[jasnell]

While historically true, the updated RFC7230 has been updated to be a much closer match to real world implementation and needs.

[LewisJEllis]

While I very much wish node were fully http compliant, that's somewhat idealist and few servers really are to begin with. Much of the non-compliant behavior in the wild, by node and many others, is just a matter of "when it was implemented, that behavior wasn't well-specified by the standard." See RFC7230 Appendix A.2 which summarizes many such gaps in the old RFC2616 that have been filled. For this reason, I can't take too much of an issue with the "discard duplicates" behavior in @jasnell's second test, for which the correct behavior was only specified in RFC7230 5.4 quoted above. (But I would prefer to return a 400.)

@jasnell's first test, on the other hand, isn't just a matter of previously unspecified behavior, and I don't think it's some hairy matter of spec vs real world either. An HTTP/1.1 request without a host header is not an HTTP/1.1 request. Requiring a host header so as to enable multitenancy was one of the primary points of HTTP/1.1, and the following bit from RFC7230 Appendix A.1.1 (an only slightly modified carryover from RFC2616 19.6.1.1) makes that very clear:

The requirements that clients and servers support the Host header
field (Section 5.4), report an error if it is missing from an
HTTP/1.1 request, and accept absolute URIs (Section 5.3) are among
the most important changes defined by HTTP/1.1.

Apparently node is missing one of "the most important changes defined by HTTP/1.1"!

If I'm implementing a web application framework and I see that message.httpVersion === '1.1', I might very well neglect to check for message.headers['host'] == null. If I want to be HTTP compliant, I instead need to figure out exactly which half of the spec I can rely on the http module to enforce for me, and then go enforce the other half myself.

I don't know why I might not want to be HTTP compliant, but if I did, surely I'd want total freedom in it instead of being surrounded by an incomplete fence that gets in my way from time to time by following that pesky "protocol."

It seems that, whether one wants to be HTTP compliant or not, the current state of the http module isn't ideal. I very much like @bnoordhuis's idea on #3096 as an answer for that. A high-level module, thoroughly enforcing standards for users not wishing to do so themselves, built on top of a low-level "building blocks" module, for people who know what they're doing, would better serve both uses.

If we can't get that far, can we at least give a 400 to any HTTP/1.1 request which omits the host header? I just can't see any reason to keep the current behavior in that case.

[ChALkeR]

Labeling as security-related. This is not a vulnerability in Node.js itself of course, but this is one of those things that could cause security issues in real-world apps.