eclipse-vertx / vert.x

Vert.x is a tool-kit for building reactive applications on the JVM
http://vertx.io
Other
14.26k stars 2.07k forks source link

Vert.x does not respond appropriately for HTTP/1.1 request containing no Host header #5204

Closed wmorrison365fr closed 4 months ago

wmorrison365fr commented 4 months ago

Version

4.5.7

Context

An error occurred while processing an HTTP/1.1 request with no Host header. The error occurred because we didn't expect the authority() to be non-null (and hadn't properly checked):

java.lang.NullPointerException: Cannot invoke "io.vertx.rxjava3.core.net.HostAndPort.host()" because the return value of "io.vertx.rxjava3.core.http.HttpServerRequest.authority()" is null

We've fixed this in our application, but were surprised that Vert.x doesn't handle validation of an HTTP/1.1 request with no Host header, especially as the spec RFC9112 indicates that servers should respond with a 400 BadRequest response:

A client MUST send a Host header field (Section 7.2 of [HTTP]) in all HTTP/1.1 request messages. If the target URI includes an authority component, then a client MUST send a field value for Host that is identical to that authority component, excluding any userinfo subcomponent and its "@" delimiter (Section 4.2 of [HTTP]). If the authority component is missing or undefined for the target URI, then a client MUST send a Host header field with an empty field value.

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 line or a Host header field with an invalid field value.

Do you have a reproducer?

Not at the moment but can do so if

Steps to reproduce

We used telnet to fire a request with no Host. Note though that our application reads the authority() (trying to align HTTP/1.1 and HTTP/2 requests):

$ telnet ig.example.com 8083
Trying ::1...
Connected to localhost.
Escape character is '^]'.
GET /hello HTTP/1.1

HTTP/1.1 500 Internal Server Error
Content-Length: 0

Extra

This looks like the likely spot for handling such an error: HttpServerRequest#DEFAULT_INVALID_REQUEST_HANDLER. This would be in collaboration with the connection first checking for a Host header and creating its own UnknownHostException if none present.

vietj commented 4 months ago

I do have mixed feelings about this change because it might break applications although that is the proper way to according to HTTP. Perhaps this could be achieved in vertx-web instead of vertx-core

vietj commented 4 months ago

e.g. in RoutingContextImpl we already have some checks here:

    if (path == null || path.isEmpty()) {
      // HTTP paths must start with a '/'
      fail(400);
    } else if (path.charAt(0) != '/') {
      // For compatiblity we return `Not Found` when a path does not start with `/`
      fail(404);
    }

I believe we should improve this and have this done in this project instead

vietj commented 4 months ago

https://github.com/vert-x3/vertx-web/pull/2612