From 19710f89056e1ed5d65b012945a8e77b5a0072ad Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Tue, 10 Dec 2024 13:17:45 -0500 Subject: [PATCH 1/6] HttpServerRequest::receiveContent() never emits any value nor completes when HTTP/1.1 TLS Upgrade (RFC-2817) kicks in Signed-off-by: Andriy Redko --- .../http/server/HttpServerOperations.java | 4 ++ .../netty/http/client/HttpClientTest.java | 61 +++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java index 790e4083db..a1c02f2467 100644 --- a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java +++ b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java @@ -814,6 +814,10 @@ else if (msg instanceof HttpRequest) { } else { request.release(); + // HTTP/1.1 TLS Upgrade (RFC-2817) on empty requests (GET/HEAD/OPTIONS) + if (!isHttp2() && request.headers().contains(HttpHeaderNames.UPGRADE)) { + onInboundComplete(); + } } if (isHttp2()) { //force auto read to enable more accurate close selection now inbound is done diff --git a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java index 21c3e1b693..5702536a53 100644 --- a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java +++ b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java @@ -73,6 +73,7 @@ import io.netty.handler.codec.compression.Brotli; import io.netty.handler.codec.http.DefaultFullHttpResponse; import io.netty.handler.codec.http.HttpClientCodec; +import io.netty.handler.codec.http.HttpContent; import io.netty.handler.codec.http.HttpContentDecompressor; import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpHeaderValues; @@ -82,6 +83,7 @@ import io.netty.handler.codec.http.HttpResponseEncoder; import io.netty.handler.codec.http.HttpResponseStatus; import io.netty.handler.codec.http.HttpVersion; +import io.netty.handler.codec.http.LastHttpContent; import io.netty.handler.logging.LogLevel; import io.netty.handler.ssl.SslContext; import io.netty.handler.ssl.SslContextBuilder; @@ -101,7 +103,10 @@ import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.ValueSource; import org.reactivestreams.Publisher; +import org.reactivestreams.Subscriber; + import reactor.core.publisher.Flux; +import reactor.core.publisher.FluxSink; import reactor.core.publisher.Mono; import reactor.core.publisher.Sinks; import reactor.netty.BaseHttpTest; @@ -1662,6 +1667,37 @@ void testIssue632() throws Exception { assertThat(latch.await(30, TimeUnit.SECONDS)).isTrue(); } + @Test + void testIssue3538() throws Exception { + disposableServer = + createServer() + .protocol(HttpProtocol.H2C, HttpProtocol.HTTP11) + .route(r -> r.get("/", (req, res) -> { + final EchoAction action = new EchoAction(); + + req + .receiveContent().switchIfEmpty(Mono.just(LastHttpContent.EMPTY_LAST_CONTENT)) + .subscribe(action); + + return res.sendObject(action); + } + )) + .bindNow(); + assertThat(disposableServer).isNotNull(); + + final ByteBuf content = createHttpClientForContextWithPort() + .protocol(HttpProtocol.HTTP11) + .headers(h -> + h.add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE) + .add(HttpHeaderNames.UPGRADE, "TLS/1.2")) + .get() + .uri("/") + .responseContent() + .blockLast(Duration.ofSeconds(30)); + + assertThat(content).isNull(); + } + @Test void testIssue694() { disposableServer = @@ -3513,4 +3549,29 @@ void testDeleteMethod(boolean chunked) { .expectComplete() .verify(Duration.ofSeconds(5)); } + + private static class EchoAction implements Publisher, Consumer { + private final Publisher sender; + private volatile FluxSink emitter; + + EchoAction() { + this.sender = Flux.create(emitter -> this.emitter = emitter); + } + + @Override + public void accept(HttpContent message) { + if (message instanceof LastHttpContent) { + emitter.complete(); + } + else { + emitter.next(message.retain()); + } + } + + @Override + public void subscribe(Subscriber s) { + sender.subscribe(s); + } + + } } From cda3662e4f0f42ecd09c57cbee08eefd83be38da Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 11 Dec 2024 09:57:38 -0500 Subject: [PATCH 2/6] Add setAutoRead(true) before the completion callback Signed-off-by: Andriy Redko --- .../java/reactor/netty/http/server/HttpServerOperations.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java index a1c02f2467..92f1e63bee 100644 --- a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java +++ b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java @@ -816,6 +816,8 @@ else if (msg instanceof HttpRequest) { request.release(); // HTTP/1.1 TLS Upgrade (RFC-2817) on empty requests (GET/HEAD/OPTIONS) if (!isHttp2() && request.headers().contains(HttpHeaderNames.UPGRADE)) { + //force auto read to enable more accurate close selection now inbound is done + channel().config().setAutoRead(true); onInboundComplete(); } } From 298725ce9e3e2f88d21948c007506454732d3a8a Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 11 Dec 2024 12:35:54 -0500 Subject: [PATCH 3/6] Add test case for HTTP/1.1 TLS Upgrade (RFC-2817) with body Signed-off-by: Andriy Redko --- .../netty/http/client/HttpClientTest.java | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java index 5702536a53..1300ac9b6a 100644 --- a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java +++ b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java @@ -1698,6 +1698,40 @@ void testIssue3538() throws Exception { assertThat(content).isNull(); } + @Test + void testIssue3538GetWithPayload() throws Exception { + disposableServer = + createServer() + .protocol(HttpProtocol.H2C, HttpProtocol.HTTP11) + .route(r -> r.get("/", (req, res) -> { + final EchoAction action = new EchoAction(); + + req + .receiveContent().switchIfEmpty(Mono.just(LastHttpContent.EMPTY_LAST_CONTENT)) + .subscribe(action); + + return res.sendObject(action); + } + )) + .bindNow(); + assertThat(disposableServer).isNotNull(); + + // The H2C max content length is 0 by default (no content is expected), + // so the request is rejected with HTTP/413 Content Too Large + StepVerifier.create(createHttpClientForContextWithPort() + .protocol(HttpProtocol.HTTP11) + .headers(h -> + h.add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE) + .add(HttpHeaderNames.UPGRADE, "TLS/1.2")) + .request(HttpMethod.GET) + .send((req, res) -> res.sendString(Mono.just("testIssue3538"))) + .uri("/") + .response((r, buf) -> Mono.just(r.status().code()))) + .expectNextMatches(status -> status == 413) + .expectComplete() + .verify(Duration.ofSeconds(30)); + } + @Test void testIssue694() { disposableServer = @@ -3561,6 +3595,9 @@ private static class EchoAction implements Publisher, Consumer 0) { + emitter.next(message.retain()); + } emitter.complete(); } else { From d33486c3bcd85125c8b4f112dd1fdbea5ff70334 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Thu, 12 Dec 2024 12:30:14 -0500 Subject: [PATCH 4/6] Add test case for HTTP/1.1 TLS Upgrade (RFC-2817) with body and H2C max content length) Signed-off-by: Andriy Redko --- .../http/server/HttpServerOperations.java | 6 +++ .../netty/http/client/HttpClientTest.java | 44 ++++++++++++++++--- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java index 92f1e63bee..fafa414c16 100644 --- a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java +++ b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java @@ -826,6 +826,12 @@ else if (msg instanceof HttpRequest) { channel().config().setAutoRead(true); onInboundComplete(); } + else if (!isHttp2() && request.headers().contains(HttpHeaderNames.UPGRADE)) { + // HTTP/1.1 TLS Upgrade (RFC-2817) requests (GET/HEAD/OPTIONS) with empty / non-empty payload + // No need to call onInboundComplete(), it will be triggered by onInboundNext(...) + // since this is the last HTTP content + stopReadTimeout(); + } } } else if (msg instanceof LastHttpContent) { diff --git a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java index 1300ac9b6a..e5ca0883d9 100644 --- a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java +++ b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java @@ -72,6 +72,7 @@ import io.netty.channel.unix.DomainSocketAddress; import io.netty.handler.codec.compression.Brotli; import io.netty.handler.codec.http.DefaultFullHttpResponse; +import io.netty.handler.codec.http.DefaultHttpContent; import io.netty.handler.codec.http.HttpClientCodec; import io.netty.handler.codec.http.HttpContent; import io.netty.handler.codec.http.HttpContentDecompressor; @@ -1732,6 +1733,39 @@ void testIssue3538GetWithPayload() throws Exception { .verify(Duration.ofSeconds(30)); } + @Test + void testIssue3538GetWithPayloadAndH2cMaxContentLength() throws Exception { + disposableServer = + createServer() + .protocol(HttpProtocol.H2C, HttpProtocol.HTTP11) + .httpRequestDecoder(spec -> spec.h2cMaxContentLength(100)) + .route(r -> r.get("/", (req, res) -> { + final EchoAction action = new EchoAction(); + + req + .receiveContent().switchIfEmpty(Mono.just(LastHttpContent.EMPTY_LAST_CONTENT)) + .subscribe(action); + + return res.sendObject(action); + } + )) + .bindNow(); + assertThat(disposableServer).isNotNull(); + + final ByteBuf content = createHttpClientForContextWithPort() + .protocol(HttpProtocol.HTTP11) + .headers(h -> + h.add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE) + .add(HttpHeaderNames.UPGRADE, "TLS/1.2")) + .request(HttpMethod.GET) + .send((req, res) -> res.sendString(Mono.just("testIssue3538"))) + .uri("/") + .responseContent() + .blockLast(Duration.ofSeconds(30)); + + assertThat(content).isNotNull(); + } + @Test void testIssue694() { disposableServer = @@ -3594,15 +3628,13 @@ private static class EchoAction implements Publisher, Consumer 0) { + emitter.next(new DefaultHttpContent(message.content().retain())); + } + if (message instanceof LastHttpContent) { - if (message.content().readableBytes() > 0) { - emitter.next(message.retain()); - } emitter.complete(); } - else { - emitter.next(message.retain()); - } } @Override From b3060a3205a546b6560f6e9420c5b504d82e572a Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Mon, 16 Dec 2024 09:23:12 -0500 Subject: [PATCH 5/6] Always call onInboundComplete in case of HTTP/1.1 and TLS upgrade flow Signed-off-by: Andriy Redko --- .../netty/http/server/HttpServerOperations.java | 11 +++-------- .../reactor/netty/http/client/HttpClientTest.java | 1 - 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java index fafa414c16..8dbce85f56 100644 --- a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java +++ b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java @@ -814,12 +814,6 @@ else if (msg instanceof HttpRequest) { } else { request.release(); - // HTTP/1.1 TLS Upgrade (RFC-2817) on empty requests (GET/HEAD/OPTIONS) - if (!isHttp2() && request.headers().contains(HttpHeaderNames.UPGRADE)) { - //force auto read to enable more accurate close selection now inbound is done - channel().config().setAutoRead(true); - onInboundComplete(); - } } if (isHttp2()) { //force auto read to enable more accurate close selection now inbound is done @@ -828,9 +822,10 @@ else if (msg instanceof HttpRequest) { } else if (!isHttp2() && request.headers().contains(HttpHeaderNames.UPGRADE)) { // HTTP/1.1 TLS Upgrade (RFC-2817) requests (GET/HEAD/OPTIONS) with empty / non-empty payload - // No need to call onInboundComplete(), it will be triggered by onInboundNext(...) - // since this is the last HTTP content stopReadTimeout(); + //force auto read to enable more accurate close selection now inbound is done + channel().config().setAutoRead(true); + onInboundComplete(); } } } diff --git a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java index e5ca0883d9..e48abac5d4 100644 --- a/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java +++ b/reactor-netty-http/src/test/java/reactor/netty/http/client/HttpClientTest.java @@ -3641,6 +3641,5 @@ public void accept(HttpContent message) { public void subscribe(Subscriber s) { sender.subscribe(s); } - } } From 3daaae30c829e762bcaa0682d24aa5642238361c Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Mon, 16 Dec 2024 12:31:30 -0500 Subject: [PATCH 6/6] Simplify the upgrade flow checks Signed-off-by: Andriy Redko --- .../java/reactor/netty/http/server/HttpServerOperations.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java index 8dbce85f56..a420862efa 100644 --- a/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java +++ b/reactor-netty-http/src/main/java/reactor/netty/http/server/HttpServerOperations.java @@ -820,7 +820,7 @@ else if (msg instanceof HttpRequest) { channel().config().setAutoRead(true); onInboundComplete(); } - else if (!isHttp2() && request.headers().contains(HttpHeaderNames.UPGRADE)) { + else if (request.headers().contains(HttpHeaderNames.UPGRADE)) { // HTTP/1.1 TLS Upgrade (RFC-2817) requests (GET/HEAD/OPTIONS) with empty / non-empty payload stopReadTimeout(); //force auto read to enable more accurate close selection now inbound is done