Skip to content

Reject fully qualified names and wildcard imports - #2338

Merged
hyperxpro merged 1 commit into
mainfrom
checkstyle
Sep 20, 2026
Merged

hyperxpro merged 1 commit into
mainfrom
checkstyle

Conversation

@hyperxpro

Copy link
Copy Markdown
Member

Motivation:

Nothing enforced import hygiene, so fully qualified class names and wildcard imports had accumulated across main and test sources. JDK 11 also caps Error Prone at 2.31.0, and newer JDKs were silently inheriting that cap.

Modification:

Turn on Error Prone's UnnecessarilyFullyQualified and WildcardImport as errors. Test sources compile with only those two checks, keeping NullAway off there. Replace the fully qualified names already in the tree with imports and expand the wildcard imports.

Raise Error Prone to 2.42.0 on JDK 17 and 2.50.0 on JDK 21+, ignore both it and NullAway in Dependabot since a minor bump drops JDK 11, and add a JDK 21 static analysis step to compile-and-check.

Result:

Fully qualified class names and wildcard imports fail the build on every JDK, and CI runs a maintained Error Prone on newer JDKs.

@hyperxpro
hyperxpro merged commit 59d9a53 into main Sep 20, 2026
17 of 19 checks passed
@hyperxpro
hyperxpro deleted the checkstyle branch September 20, 2026 19:07
@mkurz mkurz mentioned this pull request Nov 4, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant