vert-x3 / vertx-web

HTTP web applications for Vert.x
Apache License 2.0
1.11k stars 535 forks source link

HTTP 1.0 request causes "Unhandled exception in router" #2629

Closed b8b closed 2 months ago

b8b commented 4 months ago

Version

4.5.8

Context

When using a Router to handle http requests, sending an HTTP 1.0 request without host header triggers an exception.

Do you have a reproducer?

    @Test
    fun testHttp10() {
        val vertx = Vertx.vertx()
        val router = Router.router(vertx)
        router.get("/").handler { ctx ->
            ctx.response().end("hello\n")
        }
        runBlocking(vertx.dispatcher()) {
            val server = vertx.createHttpServer()
                .requestHandler(router)
                .listen(0)
                .coAwait()
            val client = vertx.createNetClient()
                .connect(server.actualPort(), "localhost")
                .coAwait()
            val rx = client.toReceiveChannel(vertx)
            val tx = client.toSendChannel(vertx)
            tx.send(Buffer.buffer("GET / HTTP/1.0\r\n" +
                    "Connection: close\r\n" +
                    "\r\n"))
            val answer = rx.receive().toString(Charsets.UTF_8)
            client.close()
            server.close()
            println(answer)
            assertTrue(answer.startsWith("200"))
        }
    }

Additional observation

An h2 request with host header causes an NPE in the Router code.

b8b commented 4 months ago

RoutingContextImpl.java#83

All requests without host header are rejected with code 400. Technically, for HTTP/1.0 this might not be completely correct. It is possible to handle such a request with a low level Handler (and implement ugly workaround).

NPE in Router code

This is actually not h2 related. It happens when trying to access RoutingContext#request().authority() from within a failure handler (regardless of allowForward settings).

java.lang.NullPointerException: Cannot invoke "io.vertx.core.net.HostAndPort.host()" because "authority" is null
    at io.vertx.ext.web.impl.ForwardedParser.setHostAndPort(ForwardedParser.java:195)
    at io.vertx.ext.web.impl.ForwardedParser.calculate(ForwardedParser.java:114)
    at io.vertx.ext.web.impl.ForwardedParser.authority(ForwardedParser.java:81)
    at io.vertx.ext.web.impl.HttpServerRequestWrapper.authority(HttpServerRequestWrapper.java:188)
    at TestVertx.testHttp10$lambda$0(TestVertx.kt:76)
    at io.vertx.ext.web.impl.RoutingContextImplBase.unhandledFailure(RoutingContextImplBase.java:235)

The 2 issues are closely related. Documentation for HttpServerRequest#authority() informs explicitly about a nullable return value. HttpServerRequestWrapper#authority() should return null instead of throwing an NPE. Also, I see no strict reason to reject requests with null authority - it should always be handled by the user anyway.

tsegismont commented 4 months ago

Would you like to contribute a fix for this?

b8b commented 4 months ago

Would you like to contribute a fix for this?

Yes sure. I have created a commit in my fork: https://github.com/b8b/vertx-web/commit/a7edbe4531957a899c55703a3f586e25fa7c3497

Is it good to target the 4.x branch with a pull request or should it rather target the master branch?

tsegismont commented 4 months ago

Yes sure. I have created a commit in my fork: b8b@a7edbe4

Thank you !

Is it good to target the 4.x branch with a pull request or should it rather target the master branch?

We need to fix both the 4.x and master branches.

tsegismont commented 2 months ago

Fixed by #2640