On Sat, 6 Dec 2025 12:49:34 GMT, EunHyunsu <[email protected]> wrote: >> ### Summary >> This patch addresses the issue where `HttpResponse.body()` returns `null` >> for 407 responses when using HTTPS through a proxy, while HTTP requests >> correctly return the response body. >> >> ### Problem >> When an HTTPS request receives a 407 Proxy Authentication Required >> response, the response body is discarded during CONNECT tunnel >> establishment. This is inconsistent with HTTP behavior where the body is >> properly returned. >> >> **Root cause:** >> - HTTPS uses `MultiExchange<Void>` for CONNECT requests >> - The body is explicitly ignored via `ignoreBody()` on 407 responses >> - No mechanism exists to preserve the body for later retrieval >> >> ### Proposed Solution >> I propose the following changes to preserve and return the 407 response >> body: >> >> 1. **PlainTunnelingConnection.java**: Change `MultiExchange<Void>` to >> `MultiExchange<byte[]>` and read the body on 407 responses instead of >> ignoring it >> >> 2. **ProxyAuthenticationRequired.java**: Add `proxyResponseBody` field to >> carry the body bytes through the exception >> >> 3. **Exchange.java**: Cache both the proxy response and body, then return >> them when the application calls `body()` >> >> ### Testing >> Added comprehensive test (`ProxyAuthHttpTest.java`) covering: >> - Basic HTTP and HTTPS 407 responses >> - Multiple `BodyHandler` types: `ofString()`, `ofByteArray()`, >> `ofInputStream()`, `ofLines()` >> - Response headers validation >> >> **Test results**: 38/38 passed >> >> ### Notes >> - This change only affects 407 responses; all other flows remain unchanged >> - The cached body is cleared after first use to prevent reuse >> - No changes to public APIs; internal implementation only >> >> I'd appreciate any feedback on this approach. If there's a better way to >> handle this, I'm happy to revise. > > EunHyunsu has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains six commits: > > - fix whitespace issue > - Merge branch 'master' into JDK-8328894 > - 8328894: Add test for 407 response body with various handlers > - 8328894: Cache and return 407 response body to application > - 8328894: Add proxyResponseBody field to preserve 407 body > - 8328894: Read 407 response body in HTTPS CONNECT tunneling
> I can't find the remarks you mentioned. Could you point me to them? @ehs208, if only I wouldn't have forgotten to press the `Submit review` button on June 5. 🙈 I've re-reviewed the changes and shared my "suggestions". I'd appreciate that if you can take them into account. Note that we need to involve @dfuch at some point anyway, since 407 handling is not the most trivial part of the code base. We might be overlooking certain details. Nevertheless, I believe my feedback will carry us closer to the finish line. Note that the changes I suggested are not mere improvements, but also fixes; otherwise some tests were failing. Some more fixes: - [Fix `ForbiddenHeadTest`](https://github.com/user-attachments/files/30744193/05-fix-ForbiddenHeadTest.patch) src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 31: > 29: import java.net.ProtocolException; > 30: import java.net.http.HttpClient.Version; > 31: import java.net.http.HttpHeaders; Unused imports. I suggest reverting all cosmetic/styling/redundant changes, in particular, the ones in `PlainTunnelingConnection`. They create noise in the VCS history and make reviews difficult. src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 251: > 249: public void cancel() {} > 250: }); > 251: return subscriber.getBody().toCompletableFuture(); Doesn't this need to be `MinimalFuture.of(subscriber.getBody())`? src/java.net.http/share/classes/jdk/internal/net/http/Exchange.java line 491: > 489: // Don't set bodyIgnored if we have a cached body > 490: if (cachedProxyBody == null) { > 491: bodyIgnored = MinimalFuture.completedFuture(null); `CompletableFuture<Void> bodyIgnored` is essentially never used, except being treated as a boolean flag. It can very well be just a `boolean`. Plus, we now have 3 `volatile` state variables: 1. `bodyIgnored` (which is effectively equivalent to `cachedProxyBody == null`) 2. `cachedProxyBody` 3. `cachedProxyResponse` I share the following proposals for your consideration: - [Collapse 3 states into 1](https://github.com/user-attachments/files/30744775/01-collapse-states.patch) - [Fix the custom `Flow.Subscription`](https://github.com/user-attachments/files/30744118/02-improve-subscription.patch) src/java.net.http/share/classes/jdk/internal/net/http/PlainTunnelingConnection.java line 1: > 1: /* This change set is super excessive. It is impossible to see you actually changed 3-4 LoC. Consider [this patch reducing your changes to bare minimum](https://github.com/user-attachments/files/30737808/03-simplify-PTC.patch). src/java.net.http/share/classes/jdk/internal/net/http/ProxyAuthenticationRequired.java line 43: > 41: } > 42: > 43: public ProxyAuthenticationRequired(Response proxyResponse, byte[] > proxyResponseBody) { This is an internal class, we don't need to preserve backward compatibility, we can just remove the old `ProxyAuthenticationRequired#new(Response)` ctor. test/jdk/java/net/httpclient/ProxyAuthHttpTest.java line 1: > 1: /* IMHO, this test can benefit a rewrite. See [this proposal](https://github.com/user-attachments/files/30742742/04-simplify-test.patch). ------------- Changes requested by vyazici (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/28232#pullrequestreview-4313572303 PR Review Comment: https://git.openjdk.org/jdk/pull/28232#discussion_r3362491394 PR Review Comment: https://git.openjdk.org/jdk/pull/28232#discussion_r3261857578 PR Review Comment: https://git.openjdk.org/jdk/pull/28232#discussion_r3719399060 PR Review Comment: https://git.openjdk.org/jdk/pull/28232#discussion_r3719451162 PR Review Comment: https://git.openjdk.org/jdk/pull/28232#discussion_r3362614222 PR Review Comment: https://git.openjdk.org/jdk/pull/28232#discussion_r3720331692
