Repository navigation
Fail fast when the scheme rules out HTTP/2 - #2334
Merged
Merged
Conversation
hyperxpro
force-pushed
the
fix/h2-connection-waiter-gate
branch
from
September 19, 2026 20:16
cadb948 to
5f7c0fc
Compare
hyperxpro
added a commit
that referenced
this pull request
Sep 20, 2026
Motivation: Four follow-ups from #2333 and #2334. 1. A 3xx whose `Location` is missing or empty NPEs in `UriParser` instead of reaching the handler. 2. A `ResponseFilter` that rebuilds the request drops the caller's redirect refusal, so the hops past a replay are no longer covered. 3. An over-cap request to an `https` origin whose handshake settled on HTTP/1.1 waits out `connectTimeout` — the half of #2334 left open. 4. `Uri.isSameBase` compares hosts with `String.equals`, so a redirect differing only in host case reads as cross-origin and strips `Authorization` and `Cookie`. Modification: 1. Treat a missing or empty `Location` as not a redirect. RFC 9110 section 15.4 only redirects when one is provided, and an empty one resolves back to the current URI. 2. Fold each hop's refusals into the exchange, monotonically: a rebuilt request can tighten the posture but not relax it. 3. Mark a host HTTP/1.1 when ALPN settles there, fail anything parked on it, and gate the waiter on that mark. Defer to a sibling that did negotiate HTTP/2, since ALPN is per-connection. 4. Fold the host with `AsciiString.contentEqualsIgnoreCase`, the comparison RFC 9110 section 4.2.3 asks for. A Unicode fold would make `k.example` and a host spelled with U+212A the same base. Result: 1. The 3xx is delivered as a response. 2. A refusal holds for the whole exchange, including hops a filter retargets. 3. The request fails with the permit exception it already had, instead of waiting out `connectTimeout`. 4. Credentials survive a case-differing same-origin redirect.
5 tasks done
hyperxpro
added a commit
that referenced
this pull request
Nov 6, 2026
## Summary - Fixes #2358. This is the [`fix/proxy-hop-alpn`](main...fix/proxy-hop-alpn) branch rebased onto `main`, with its two commits and their authorship kept, plus six commits of mine, three of them after review. - The TLS connection to an HTTPS proxy no longer offers `h2` in ALPN, and h2c prior knowledge is no longer sent to an HTTP proxy. Both connections only ever carry HTTP/1.1. ## Problem 1. **HTTPS proxy:** the TLS connection to an `HTTPS` proxy derived `http2Allowed` from the proxy's URI, so with HTTP/2 enabled it offered `h2`. A proxy that accepts `h2` then receives the HTTP/1.1 `CONNECT`, or the absolute-URI request, and drops the connection (`RemotelyClosedException`). Proxies that don't accept `h2` are not affected. 2. **HTTP proxy with h2c:** with `http2CleartextEnabled`, an `http://` target behind an HTTP proxy got the HTTP/2 connection preface (`PRI * HTTP/2.0`) sent to the proxy, which never agreed to HTTP/2. SOCKS is not affected, since there the peer is the origin. Both are reproduced by the tests below on `main`, against test proxies (an h2-capable TLS `CONNECT` proxy and a stub HTTP proxy) rather than a real proxy product. ## Change - **[`04f167c`](04f167c), rebased:** the HTTPS proxy connection passes `http2Allowed = false`, so `DefaultSslEngineFactory` uses its http/1.1-only context. If the connection still negotiates `h2`, which a user-supplied context can, this commit logged a warning; the request now fails with the reason instead (see below). `main` already has the 5-argument `addSslHandler` (#2342), so of the `ChannelManager` change only the Javadoc is left. - **[`0b4f999`](0b4f999), rebased:** h2c is only used when the socket's peer is the origin, that is without a proxy or through SOCKS. The check joins `main`'s handling of connections a login authenticates (#2342), so its "instead of h2c" warning is only logged when h2c would otherwise be used. The test imports `Http2Headers`, since `main` rejects fully qualified names (#2338). - **Don't wait for h2c behind an HTTP proxy (mine):** #2334 fails a request refused a connection permit at once when no HTTP/2 connection could ever be registered for it. A cleartext origin behind an HTTP or HTTPS proxy is now such a case too. On `main`, the check only looked at `http2CleartextEnabled`, so with it enabled such a request waited out `connectTimeout` behind either proxy type. - **Let the h2c proxy test check the response (mine):** the stub HTTP proxy now answers, and the test checks that the request completes over HTTP/1.1. It no longer waits out its 10-second timeout. - **Correct the proxy tests' comments (mine):** the TLS test proxy's Javadoc no longer claims that it checks the HTTP/2 preface, and both tests use ASCII only. After review: - **Fail the connect when the proxy selects h2:** if the TLS connection to an HTTPS proxy still negotiates h2, as an engine from a user-supplied `SslContext` can, the connect now fails at once, before anything is written to the proxy, with an exception that names the cause. Before, this was only logged, and the request repeated the handshake once per retry and then failed with a bare `Remotely closed`. The failure is not retried in any state, also not after an earlier retry or a closed pooled connection, since a new connection would select h2 again. - **Test the h2c waiter through SOCKS and HTTPS:** an over-cap h2c request through SOCKS must wait for the first connection and complete on it, and an `http` target behind an HTTPS proxy must be refused at once. - **Document every connection kept on HTTP/1.1:** `SslEngineFactory` and `DefaultSslEngineFactory` now list all three cases of `http2Allowed = false`: WebSocket, the connection to an HTTPS proxy, and a connection that a login authenticates (NTLM, Kerberos or SPNEGO, or any proxy login on a `CONNECT` tunnel or through SOCKS). The h2c comments say that prior knowledge in RFC 9113 is about the immediate peer, and that it is `http2CleartextEnabled` that limits it to origins. Test comments corrected. Unchanged: - the connection to the origin through a `CONNECT` tunnel still negotiates `h2`, also behind an HTTPS proxy; - h2c without a proxy and through SOCKS; - WebSocket staying on HTTP/1.1; - connections a login authenticates staying off the HTTP/2 registry. ## Not in scope A user-supplied `SslContext` is used as-is for every connection, so if it offers `h2`, the proxy connection can still negotiate it, as a `wss://` connection already can. The request then fails with the reason (see above). To keep h2 to origins, a custom `SslEngineFactory` can offer only http/1.1 when it receives `http2Allowed = false`, which it does for the proxy connection. ## Release note With `http2CleartextEnabled`, requests through an HTTP forward proxy now use HTTP/1.1 rather than h2c, including when that proxy supports h2c. ## AI disclosure Claude Code on behalf of Matthias Kurz. The two rebased commits keep their original authorship; Claude Code did the rebase and the conflict resolution, which their messages describe. All eight commits include `Co-Authored-By` per `AGENTS.md`. ## Test plan - [x] Fail on `main`, pass with this PR: - `ProxyTunnelHttp2Test.httpsProxyHopMustNotNegotiateHttp2`: on `main` the proxy negotiates `h2`, then `Remotely closed`. With the PR, it also checks that the origin behind the HTTPS proxy still gets HTTP/2. - `ProxyH2cTest.h2cIsNotSpokenToAnHttpProxy`: on `main` the proxy receives `PRI * HTTP/2.0`. - `Http2ConnectionWaiterGateTest.permitExhaustedOnCleartextOriginBehindAnHttpProxyFailsWithoutWaitingOutConnectTimeout` and `...BehindAnHttpsProxy...`: fail without my first commit, with the refused request still pending. - `ProxyTunnelHttp2Test.httpsProxyHopThatSelectsHttp2FailsTheConnect`: before the fail-fast commit, the request failed with `RemotelyClosedException`. Checks the exception, that the proxy receives no bytes after selecting h2, one handshake per request, and that the connection permit is returned. - `ProxyHopAlpnConnectListenerTest`: drives the connect listener with a proxy connection that selected h2, in the `NEW`, `RECONNECTED` and `POOLED` states. With the failure going through `onFailure()`, the latter two were retried. Checks that nothing is retried, the exception, that the channel is closed, and that the permit is returned once. - [x] The new waiter tests catch the changes described in review: without the `isSocks()` exemption, `ProxyH2cTest.overCapH2cRequestThroughASocksProxyWaitsForTheFirstConnection` fails; with HTTPS proxies let through, the HTTPS-proxy waiter test fails. - [x] Pass on both: the other five `ProxyTunnelHttp2Test` tests (HTTP/2 to the origin through a `CONNECT` proxy, tunnel reuse, concurrent requests, SOCKS, `maxConnectionsPerHost`) and `ProxyH2cTest.h2cIsSpokenThroughASocksProxy`. - [x] `./mvnw -B -ntp -Dgpg.skip=true clean verify` on JDK 11, merged with `main` `3e95ec8c2`: 1,919 tests, 0 failures, 0 errors, 11 skipped; Revapi passed. No test-skipping flags were used. - [x] `ProxyTunnelHttp2Test`, `ProxyH2cTest`, `Http2ConnectionWaiterGateTest`, `ConnectionOrientedAuthConnectListenerTest` and `ProxyHopAlpnConnectListenerTest` compiled on JDK 11 and run on JDK 17, 21 and 25 (Maven Surefire's `-Djvm`): 21 tests each, 0 failures. --------- Co-authored-by: Aayush Atharva <aayush@shieldblaze.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
A request refused a connection permit arms an
Http2ConnectionWaiterthat only a registered HTTP/2 connection can complete. On a cleartext origin with h2c disabled, no registration site can ever fire for that key, so the waiter can only expire: the request is held forconnectTimeoutand then fails with the permit exception it already had, ignoring theacquireFreeChannelTimeoutthe caller configured (0 by default: fail fast). A deferred request has noTimeoutsHoldereither, so its ownrequestTimeoutand absolute deadline go unenforced for the whole wait: a callerasking for 200ms waits 5 seconds.
Any client talking to a cleartext origin with
maxConnectionsset stalls on every over-cap request. Introduced in #2144 and carried through #2227, so present since 3.0.8, though only the non-blocking waiter shape in 3.0.12 and 3.0.13 is addressedhere; the earlier shapes blocked the caller thread and were fixed in 3.0.11.
Modification:
Return early, before arming the waiter, when the origin is not secured and cleartext HTTP/2 is disabled. With the default partitioning that is the exact complement of the sites that register an HTTP/2 connection: ALPN on a secured origin, h2c prior knowledge, and the post-CONNECT tunnel. The guard sits after
pollHttp2, so a cleartext request that does find a live HTTP/2 channel still multiplexes; only the waiting is gated.Result:
An over-cap request to such an origin fails immediately with
TooManyConnectionsExceptioninstead of waiting outconnectTimeout, and the failure is delivered on the caller's thread rather than on the client's shared timerthread.
NettyRequestThrottleTimeoutTestdrops from ~33s to ~7s.