From adb2399dcd5a2a9eab6764cba0fd24ceb1511b12 Mon Sep 17 00:00:00 2001 From: Aayush Atharva Date: Fri, 18 Sep 2026 19:52:39 +0000 Subject: [PATCH] Reject invalid HTTP/2 settings when the client is built --- .../DefaultAsyncHttpClientConfig.java | 13 ++ .../netty/channel/ChannelManager.java | 73 +++++++----- .../netty/channel/NettyConnectListener.java | 16 ++- .../netty/request/NettyRequestSender.java | 11 +- .../channel/Http2ConfigValidationTest.java | 111 ++++++++++++++++++ 5 files changed, 188 insertions(+), 36 deletions(-) create mode 100644 client/src/test/java/org/asynchttpclient/netty/channel/Http2ConfigValidationTest.java diff --git a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java index 7095a4cbc..a35f5505a 100644 --- a/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java +++ b/client/src/main/java/org/asynchttpclient/DefaultAsyncHttpClientConfig.java @@ -454,6 +454,19 @@ private DefaultAsyncHttpClientConfig(// http throw new IllegalArgumentException("HTTP/2 initial window size must be non-negative"); } + if (http2HeaderTableSize < 0) { + throw new IllegalArgumentException("HTTP/2 header table size must be non-negative"); + } + + // Netty's Http2Settings accepts 0, but the HPACK decoder built from it rejects anything below 1. + if (http2MaxHeaderListSize < 1) { + throw new IllegalArgumentException("HTTP/2 max header list size must be positive"); + } + + if (http2PingInterval == null) { + throw new IllegalArgumentException("HTTP/2 ping interval must not be null"); + } + this.allocator = allocator; this.nettyTimer = nettyTimer; this.threadFactory = threadFactory; diff --git a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java index 774b97e41..9a2f1a19c 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java @@ -1039,8 +1039,9 @@ protected void initChannel(Channel channel) throws Exception { * walked to the end to say no. *

* They part on close, where Netty's own teardown strips the pipeline and leaves the attribute: a closed - * connection answers yes here and would have answered no to a lookup. Callers reach this while deciding - * what to do with a channel they have just taken from the pool, having checked it is active. + * connection answers yes here and would have answered no to a lookup. That divergence is harmless at the + * one internal call site, where a true answer only skips storing the per-request future on the parent, + * and the next statement re-checks liveness and routes a closed channel to handleUnexpectedClosedChannel. */ public static boolean isHttp2(Channel channel) { return channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get() != null; @@ -1067,34 +1068,11 @@ public Http2Handler getHttp2Handler() { * The per-stream {@link Http2Handler} is added separately on each stream child channel. */ public void upgradePipelineToHttp2(ChannelPipeline pipeline) { - // Remove HTTP/1.1 specific handlers - if (pipeline.get(HTTP_CLIENT_CODEC) != null) { - pipeline.remove(HTTP_CLIENT_CODEC); - } - if (pipeline.get(INFLATER_HANDLER) != null) { - pipeline.remove(INFLATER_HANDLER); - } - if (pipeline.get(CHUNKED_WRITER_HANDLER) != null) { - pipeline.remove(CHUNKED_WRITER_HANDLER); - } - if (pipeline.get(AHC_HTTP_HANDLER) != null) { - pipeline.remove(AHC_HTTP_HANDLER); - } - - // Attach HTTP/2 connection state for MAX_CONCURRENT_STREAMS tracking and GOAWAY draining. Its - // presence is also what marks the connection as HTTP/2; see isHttp2. Attached before the handlers - // rather than after them, so that there is no instant at which the pipeline speaks HTTP/2 and the - // connection does not yet say so -- a write landing there would take the HTTP/1.1 branch onto an - // HTTP/2 pipeline. Nothing can reach this channel that early today; the ordering is what keeps that - // from being something each new caller has to know. - Http2ConnectionState state = new Http2ConnectionState(); - int configMaxStreams = config.getHttp2MaxConcurrentStreams(); - if (configMaxStreams > 0) { - // Client's own cap; the server-advertised value (applied by the http2-settings-listener below) - // can only lower the effective limit, never raise it above this. - state.setClientMaxConcurrentStreams(configMaxStreams); - } - pipeline.channel().attr(Http2ConnectionState.HTTP2_STATE_KEY).set(state); + // Constructed before the pipeline is touched, so that a failure here leaves the connection intact + // as HTTP/1.1 rather than half-upgraded. Http2Settings rejects an out-of-range value and the HPACK + // decoder built from it rejects a non-positive max header list size; the config constructor rejects + // both ahead of this, and the callers below fail the connection rather than letting a throw escape, + // so this is defence in depth rather than the guard those two are. // Add HTTP/2 frame codec (handles connection preface, SETTINGS, PING, flow control, etc.) Http2Settings settings = new Http2Settings() @@ -1109,8 +1087,6 @@ public void upgradePipelineToHttp2(ChannelPipeline pipeline) { ClientHttp2FrameCodecBuilder frameCodecBuilder = new ClientHttp2FrameCodecBuilder(); Http2FrameCodec frameCodec = frameCodecBuilder.initialSettings(settings).build(); - pipeline.channel().attr(SuspensionAwareHttp2LocalFlowController.CHANNEL_KEY) - .set(frameCodecBuilder.flowController()); // Http2MultiplexHandler creates a child channel per HTTP/2 stream. // Server-push streams are rejected with RST_STREAM(REFUSED_STREAM). @@ -1123,6 +1099,39 @@ protected void initChannel(Channel ch) { } }); + // HTTP/2 connection state for MAX_CONCURRENT_STREAMS tracking and GOAWAY draining. Its presence on + // the channel is also what marks the connection as HTTP/2; see isHttp2. + Http2ConnectionState state = new Http2ConnectionState(); + int configMaxStreams = config.getHttp2MaxConcurrentStreams(); + if (configMaxStreams > 0) { + // Client's own cap; the server-advertised value (applied by the http2-settings-listener below) + // can only lower the effective limit, never raise it above this. + state.setClientMaxConcurrentStreams(configMaxStreams); + } + + // Remove HTTP/1.1 specific handlers + if (pipeline.get(HTTP_CLIENT_CODEC) != null) { + pipeline.remove(HTTP_CLIENT_CODEC); + } + if (pipeline.get(INFLATER_HANDLER) != null) { + pipeline.remove(INFLATER_HANDLER); + } + if (pipeline.get(CHUNKED_WRITER_HANDLER) != null) { + pipeline.remove(CHUNKED_WRITER_HANDLER); + } + if (pipeline.get(AHC_HTTP_HANDLER) != null) { + pipeline.remove(AHC_HTTP_HANDLER); + } + + pipeline.channel().attr(SuspensionAwareHttp2LocalFlowController.CHANNEL_KEY) + .set(frameCodecBuilder.flowController()); + + // Marked before the handlers rather than after them, so there is no instant at which the pipeline + // speaks HTTP/2 and the connection does not yet say so: a write landing there would take the + // HTTP/1.1 branch onto an HTTP/2 pipeline. Nothing can reach this channel that early today, since + // it is registered and pooled only once this method returns. + pipeline.channel().attr(Http2ConnectionState.HTTP2_STATE_KEY).set(state); + pipeline.addLast(HTTP2_FRAME_CODEC, frameCodec); pipeline.addLast(HTTP2_MULTIPLEX, multiplexHandler); diff --git a/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java b/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java index cc03f3407..7849dac02 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java @@ -236,7 +236,14 @@ protected void onSuccess(Channel value) { + "(RFC 8441) is not supported — continuing on HTTP/1.1", uri); } if (http2Negotiated && !uri.isWebSocket()) { - channelManager.upgradePipelineToHttp2(channel.pipeline()); + try { + channelManager.upgradePipelineToHttp2(channel.pipeline()); + } catch (Exception upgradeError) { + // This runs inside a future listener, which logs and discards anything thrown. + // Fail the connection instead, or the request waits for its timeout. + NettyConnectListener.this.onFailure(channel, upgradeError); + return; + } registerHttp2AndManageSemaphore(channel, semaphore, permit); } writeRequest(channel); @@ -259,7 +266,12 @@ protected void onFailure(Throwable cause) { // h2c (cleartext HTTP/2 prior knowledge): upgrade to HTTP/2 without TLS. WebSocket (ws://) is // excluded for the same RFC 8441 reason as the TLS path above — it stays on HTTP/1.1. if (!uri.isSecured() && channelManager.isHttp2CleartextEnabled() && !uri.isWebSocket()) { - channelManager.upgradePipelineToHttp2(channel.pipeline()); + try { + channelManager.upgradePipelineToHttp2(channel.pipeline()); + } catch (Exception upgradeError) { + onFailure(channel, upgradeError); + return; + } registerHttp2AndManageSemaphore(channel, semaphore, permit); } writeRequest(channel); diff --git a/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java b/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java index daff6e383..91a57cfdb 100755 --- a/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java +++ b/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java @@ -1616,8 +1616,15 @@ public void call() { whenHandshaked.addListener(f -> { if (f.isSuccess()) { if (!nextRequest.getUri().isWebSocket()) { - channelManager.upgradePipelineToHttp2AfterProxyConnect( - channel.pipeline(), future.getPartitionKey()); + try { + channelManager.upgradePipelineToHttp2AfterProxyConnect( + channel.pipeline(), future.getPartitionKey()); + } catch (Exception upgradeError) { + // Thrown inside a future listener, which logs and discards it. + // Fail the future instead, or the request waits for its timeout. + abort(channel, future, upgradeError); + return; + } } sendNextRequest(nextRequest, future); } else { diff --git a/client/src/test/java/org/asynchttpclient/netty/channel/Http2ConfigValidationTest.java b/client/src/test/java/org/asynchttpclient/netty/channel/Http2ConfigValidationTest.java new file mode 100644 index 000000000..1b0d4e09e --- /dev/null +++ b/client/src/test/java/org/asynchttpclient/netty/channel/Http2ConfigValidationTest.java @@ -0,0 +1,111 @@ +/* + * Copyright (c) 2026 AsyncHttpClient Project. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.asynchttpclient.netty.channel; + +import io.netty.channel.embedded.EmbeddedChannel; +import io.netty.handler.codec.http.HttpClientCodec; +import io.netty.util.HashedWheelTimer; +import io.netty.util.Timer; +import org.asynchttpclient.AsyncHttpClientConfig; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import java.lang.reflect.Proxy; + +import static org.asynchttpclient.Dsl.config; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * An HTTP/2 setting Netty will reject has to be rejected where the client is built, not where a connection is + * upgraded. The upgrade runs inside a Netty future listener, which logs whatever escapes it and carries on, so + * a throw there reaches nobody: the request waits out its timeout instead of failing. + *

+ * These pin both halves. The config constructor refuses what Netty would refuse, and the upgrade builds + * everything that can fail before touching the pipeline, so a connection is never left stripped of its + * HTTP/1.1 handlers with no HTTP/2 handlers to replace them. + */ +class Http2ConfigValidationTest { + + private Timer timer; + private ChannelManager channelManager; + private EmbeddedChannel channel; + + @AfterEach + void tearDown() { + if (channel != null) { + channel.finishAndReleaseAll(); + } + if (channelManager != null) { + channelManager.close(); + } + if (timer != null) { + timer.stop(); + } + } + + @Test + void aNegativeHeaderTableSizeIsRejectedWhenTheClientIsBuilt() { + assertThrows(IllegalArgumentException.class, () -> config().setHttp2HeaderTableSize(-1).build()); + } + + @Test + void aMaxHeaderListSizeBelowOneIsRejectedWhenTheClientIsBuilt() { + // Netty's Http2Settings accepts 0 here. The HPACK decoder built from those settings does not, so + // without this check the throw lands later, part-way through the upgrade rather than at Http2Settings. + assertThrows(IllegalArgumentException.class, () -> config().setHttp2MaxHeaderListSize(0).build()); + } + + @Test + void aNullPingIntervalIsRejectedWhenTheClientIsBuilt() { + // Read at the very end of the upgrade, after the connection preface is already on the wire. + assertThrows(IllegalArgumentException.class, () -> config().setHttp2PingInterval(null).build()); + } + + @Test + void anUpgradeThatFailsLeavesTheConnectionOnHttp11() { + // The checks above make this unreachable through the builder, so come in the way that bypasses them: + // the getters are declared on the AsyncHttpClientConfig interface, and a custom implementation never + // runs the constructor that validates them. + timer = new HashedWheelTimer(); + channelManager = new ChannelManager(configReportingMaxHeaderListSize(0), timer); + + channel = new EmbeddedChannel(); + channel.pipeline().addLast(ChannelManager.HTTP_CLIENT_CODEC, new HttpClientCodec()); + + assertThrows(IllegalArgumentException.class, + () -> channelManager.upgradePipelineToHttp2(channel.pipeline())); + + assertNotNull(channel.pipeline().get(ChannelManager.HTTP_CLIENT_CODEC), + "a failed upgrade must leave the HTTP/1.1 codec in place"); + assertNull(channel.pipeline().get(ChannelManager.HTTP2_MULTIPLEX), + "and must not have installed the multiplex handler"); + assertFalse(ChannelManager.isHttp2(channel), + "and must not have marked the connection as HTTP/2"); + } + + private static AsyncHttpClientConfig configReportingMaxHeaderListSize(int value) { + AsyncHttpClientConfig delegate = config().build(); + return (AsyncHttpClientConfig) Proxy.newProxyInstance( + Http2ConfigValidationTest.class.getClassLoader(), + new Class[]{AsyncHttpClientConfig.class}, + (proxy, method, args) -> "getHttp2MaxHeaderListSize".equals(method.getName()) + ? value + : method.invoke(delegate, args)); + } +}