Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTTP requests with no Host header accepted #39033

Closed
asta12 opened this issue Jun 14, 2021 · 4 comments
Closed

HTTP requests with no Host header accepted #39033

asta12 opened this issue Jun 14, 2021 · 4 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@asta12
Copy link

asta12 commented Jun 14, 2021

  • Version: v16.3.0
  • Platform: Microsoft Windows NT 10.0.19042.0 x64
  • Subsystem: http

What steps will reproduce the bug?

This is a bug in the http module. If this is the wrong place to report this, please direct me to the right place.

The following code sets up a simple hello world node server. It listens to port 8080:

const http = require('http');

http.createServer((request, response) => {
  response.end('<html><body><h1>Hello, World!</h1></body></html>');
}).listen(8080);

If we send the following request with the command below (using echo and nc):

GET / HTTP/1.1
Connection: close


echo -ne "GET / HTTP/1.1\r\nConnection: close\r\n\r\n" | nc localhost 8080

We get the following response:

HTTP/1.1 200 OK
Date: Mon, 14 Jun 2021 16:23:18 GMT
Connection: close
Content-Length: 48

<html><body><h1>Hello, World!</h1></body></html>

How often does it reproduce? Is there a required condition?

It happens all the time.

What is the expected behavior?

The expected behavior is for node to answer with a 400 Bad Request response.

What do you see instead?

It answers with a 200 OK response.

Additional information

According to RFC 7230 a request containing no Host header should respond with a 400 (Bad Request).

"A server MUST respond with a 400 (Bad Request) status code to any HTTP/1.1 request message that lacks a Host header field [...]" - https://datatracker.ietf.org/doc/html/rfc7230#section-5.4

(Found by Asta Olofsson and Mattias Grenfeldt)
@Linkgoron
Copy link
Member

Linkgoron commented Jun 14, 2021 •
edited
Loading

This was actually raised a few years ago, here #3094

@asta12
Copy link
Author

asta12 commented Jun 15, 2021

I read the issue. The discussion in the thread seemed to lean towards adding a check for the Host header, but then it was closed and the there is no check for the Host header as of today. What happened? What is the view of the nodejs team today regarding this?

@Linkgoron Linkgoron added the http Issues or PRs related to the http subsystem. label Jun 16, 2021
@wwwzbwcom wwwzbwcom mentioned this issue Jul 9, 2021
Open
@marco-ippolito marco-ippolito mentioned this issue Nov 23, 2022
Closed
ShogunPanda pushed a commit to ShogunPanda/node that referenced this issue Nov 30, 2022
@wwwzbwcom @lpinca
@mscdex @ShogunPanda
http: server check Host header, to meet RFC 7230 5.4 requirement
4587866 
Author:
PR-URL: nodejs#45597
Fixes: nodejs#39033
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
ErickWendel pushed a commit to ErickWendel/node that referenced this issue Nov 30, 2022
@wwwzbwcom @lpinca
@mscdex @ErickWendel
http: server check Host header, to meet RFC 7230 5.4 requirement
7594324 
PR-URL: nodejs#45597
Fixes: nodejs#39033
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
Co-authored-by: mscdex <mscdex@users.noreply.github.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@awwright
Copy link
Contributor

awwright commented Nov 30, 2023 •
edited
Loading

I was just hit by this (an application suddenly stopped handling errors correctly) and I don't think this should have been merged.

The 'http' module is not a complete implementation of HTTP, but merely of the HTTP/1.1 framing, and this rule is not one of those framing rules.

Fully implementing the framing rules means is possible to receive a message, pass it, and the beginning of the next message will never be ambiguous, or misinterpreted by another (compliant) parser.

There's many, many requirements in HTTP, for example that "An origin server MUST generate an Allow header field in a 405 (Method Not Allowed) response". Unless Node.js is going to start enforcing all of these, then the HTTP parser should limit itself to enforcing the message framing rules. (#34066 is such a requirement.)

@marco-ippolito
Copy link
Member

marco-ippolito commented Nov 30, 2023 •
edited
Loading

you can pass the option requireHostHeader: false in HttpServer to disable this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http: server check Host header in request, to meet RFC 7230 5.4 requirement marco-ippolito/node
4 participants
@awwright @Linkgoron @marco-ippolito @asta12