From 3a42d2b58c0e50b1300ad0175ef3a6f951a04951 Mon Sep 17 00:00:00 2001 From: Aayush Atharva Date: Sun, 20 Sep 2026 19:34:53 +0000 Subject: [PATCH] Fix redirect and HTTP/2 follow-up defects --- .../java/org/asynchttpclient/Request.java | 12 +-- .../asynchttpclient/RequestBuilderBase.java | 5 +- .../netty/NettyResponseFuture.java | 23 +++++ .../netty/channel/ChannelManager.java | 40 +++++++++ .../netty/channel/NettyConnectListener.java | 6 ++ .../intercept/Redirect30xInterceptor.java | 32 ++++--- .../netty/request/NettyRequestSender.java | 7 ++ .../java/org/asynchttpclient/uri/Uri.java | 7 +- .../RedirectCredentialSecurityTest.java | 31 +++++++ .../asynchttpclient/RedirectRefusalTest.java | 74 +++++++++++++++ .../ChannelManagerHttp2WaiterTest.java | 90 +++++++++++++++++++ .../intercept/Redirect30xInterceptorTest.java | 24 +++++ .../Http2ConnectionWaiterGateTest.java | 63 +++++++++++-- .../java/org/asynchttpclient/uri/UriTest.java | 10 +++ 14 files changed, 394 insertions(+), 30 deletions(-) diff --git a/client/src/main/java/org/asynchttpclient/Request.java b/client/src/main/java/org/asynchttpclient/Request.java index e16b47ddc..48a982729 100644 --- a/client/src/main/java/org/asynchttpclient/Request.java +++ b/client/src/main/java/org/asynchttpclient/Request.java @@ -186,9 +186,9 @@ default Boolean getUseAbsoluteRequestDeadline() { /** * Refuses a scheme downgrade on this request and the hops it leads to, even when * {@link AsyncHttpClientConfig#isRefuseSchemeDowngradeOnRedirect()} is off. Tightening only, so - * {@code false} behaves as null and cannot re-enable a hop the client configuration refuses. A filter - * that builds a fresh request rather than deriving one from {@link #toBuilder()} drops it, so the - * client-wide option is what holds a posture across replays. + * {@code false} behaves as null and cannot re-enable a hop the client configuration refuses. + * Once set it holds for the whole exchange, including hops a filter retargets elsewhere and + * requests a filter rebuilds without it. * * @return true to refuse, or null or false to use the config value */ @@ -200,9 +200,9 @@ default Boolean getRefuseSchemeDowngradeOnRedirect() { /** * Refuses a cross-origin body replay on this request and the hops it leads to, even when * {@link AsyncHttpClientConfig#isRefuseCrossOriginBodyOnRedirect()} is off. Tightening only, so - * {@code false} behaves as null and cannot re-enable a hop the client configuration refuses. A filter - * that builds a fresh request rather than deriving one from {@link #toBuilder()} drops it, so the - * client-wide option is what holds a posture across replays. + * {@code false} behaves as null and cannot re-enable a hop the client configuration refuses. + * Once set it holds for the whole exchange, including hops a filter retargets elsewhere and + * requests a filter rebuilds without it. * * @return true to refuse, or null or false to use the config value */ diff --git a/client/src/main/java/org/asynchttpclient/RequestBuilderBase.java b/client/src/main/java/org/asynchttpclient/RequestBuilderBase.java index b6d32008e..f6dd475c8 100644 --- a/client/src/main/java/org/asynchttpclient/RequestBuilderBase.java +++ b/client/src/main/java/org/asynchttpclient/RequestBuilderBase.java @@ -616,7 +616,7 @@ public T setUseAbsoluteRequestDeadline(boolean useAbsoluteRequestDeadline) { } /** - * Refuses a scheme downgrade on this request even when the client allows it. + * Refuses a scheme downgrade on this request and every hop it leads to, even when the client allows it. * * @param refuseSchemeDowngradeOnRedirect true to refuse a redirect off this request that leaves a secured * scheme for one that is not; false leaves @@ -630,7 +630,8 @@ public T setRefuseSchemeDowngradeOnRedirect(boolean refuseSchemeDowngradeOnRedir } /** - * Refuses a cross-origin body replay on this request even when the client allows it. + * Refuses a cross-origin body replay on this request and every hop it leads to, even when the client + * allows it. * * @param refuseCrossOriginBodyOnRedirect true to refuse a redirect off this request that would resend its * content to another origin; false leaves diff --git a/client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java b/client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java index f18e051f4..d80133f8f 100755 --- a/client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java +++ b/client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java @@ -167,6 +167,8 @@ public final class NettyResponseFuture implements ListenableFuture { // Read when a TimeoutsHolder is built, which happens on the caller thread, an event loop or the timer // thread depending on the path, so it is published rather than plain. private volatile boolean useAbsoluteRequestDeadline; + private volatile boolean refuseSchemeDowngradeLatched; + private volatile boolean refuseCrossOriginBodyLatched; public NettyResponseFuture(Request originalRequest, AsyncHandler asyncHandler, @@ -784,6 +786,27 @@ public void setUseAbsoluteRequestDeadline(boolean useAbsoluteRequestDeadline) { this.useAbsoluteRequestDeadline = useAbsoluteRequestDeadline; } + /** + * Folds a request's redirect refusals into the exchange. Monotonic, and never reset: a filter can still + * tighten on a later hop, but rebuilding the request can no longer drop what the caller asked for. + */ + public void tightenRedirectRefusals(Request request) { + if (Boolean.TRUE.equals(request.getRefuseSchemeDowngradeOnRedirect())) { + refuseSchemeDowngradeLatched = true; + } + if (Boolean.TRUE.equals(request.getRefuseCrossOriginBodyOnRedirect())) { + refuseCrossOriginBodyLatched = true; + } + } + + public boolean isRefuseSchemeDowngradeLatched() { + return refuseSchemeDowngradeLatched; + } + + public boolean isRefuseCrossOriginBodyLatched() { + return refuseCrossOriginBodyLatched; + } + public Realm getRealm() { return realm; } 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 9a2f1a19c..1bd9d77ee 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/ChannelManager.java @@ -163,6 +163,13 @@ public class ChannelManager { // rather than hang (its request-timeout is not scheduled yet at this point). See NettyRequestSender's // HTTP/2 deferral. private final ConcurrentHashMap>> http2ConnectionWaiters = new ConcurrentHashMap<>(); + // Hosts a connection has already finished a handshake with as HTTP/1.1. Sticky, because the waiter is + // armed after the handshake that decided it, so an edge alone would fire into an empty waiter set. + // ALPN is per-connection, so this can be wrong for a mixed fleet; registerHttp2Connection clears it. + // Being wrong costs a parked request the sibling connection it might have multiplexed onto (#2214). + // Bounded because nothing prunes it. + private static final int MAX_KNOWN_NON_HTTP2 = 1024; + private final Set knownNonHttp2 = ConcurrentHashMap.newKeySet(); // Set once, permanently, when the client closes and sweeps its waiters (failHttp2ConnectionWaiters). Read // by addHttp2ConnectionWaiter to fail-closed against a request that arms a waiter in the window between the // sweep and nettyTimer.stop() — such a waiter would otherwise be neither woken nor timed out, hanging its @@ -519,6 +526,7 @@ private static Object baseKeyOf(Object partitionKey) { * multiple requests can share the same connection concurrently. */ public void registerHttp2Connection(Object partitionKey, Channel channel) { + knownNonHttp2.remove(baseKeyOf(partitionKey)); Http2ConnectionState state = channel.attr(Http2ConnectionState.HTTP2_STATE_KEY).get(); if (state != null) { state.setPartitionKey(partitionKey); @@ -611,6 +619,31 @@ public void removeHttp2ConnectionWaiter(Object partitionKey, Consumer o }); } + /** + * Records that a handshake for this key settled on HTTP/1.1 and fails anything parked waiting for an + * HTTP/2 connection there. Without it an over-cap request waits out {@code connectTimeout} for a + * connection the handshake has already ruled out. + */ + void http2Unavailable(Object partitionKey) { + Object baseKey = baseKeyOf(partitionKey); + if (pollHttp2SiblingConnection(baseKey) != null) { + // Another IP of this host did negotiate HTTP/2. Marking the host would strand the waiters on a + // connection they can still multiplex onto, so leave the mark and the waiters alone. + return; + } + if (knownNonHttp2.size() < MAX_KNOWN_NON_HTTP2) { + // Past the cap an over-cap request waits out connectTimeout again, which is what it did before + // this existed. Unbounded growth is the worse failure. + knownNonHttp2.add(baseKey); + } + LOGGER.debug("HTTP/2 unavailable for key: {}, failing anything waiting for it", baseKey); + wakeHttp2ConnectionWaiters(partitionKey, null); + } + + public boolean isHttp2KnownUnavailable(Object partitionKey) { + return knownNonHttp2.contains(baseKeyOf(partitionKey)); + } + private void wakeHttp2ConnectionWaiters(Object partitionKey, Channel channel) { Set> waiters = http2ConnectionWaiters.remove(baseKeyOf(partitionKey)); if (waiters != null) { @@ -772,6 +805,7 @@ public void close() { // runs after the (possibly long) graceful EventLoopGroup shutdown, and the nettyTimer that would // otherwise fire their deadline is being stopped in parallel. failHttp2ConnectionWaiters(); + knownNonHttp2.clear(); // Close the resolver group first while the EventLoopGroup is still active, // since Netty DNS resolvers may need a live EventLoop for clean shutdown. if (addressResolverGroup != null) { @@ -1216,6 +1250,8 @@ public void upgradePipelineToHttp2AfterProxyConnect(ChannelPipeline pipeline, Ob && ApplicationProtocolNames.HTTP_2.equals(targetSslHandler.applicationProtocol())) { upgradePipelineToHttp2(pipeline); registerHttp2Connection(partitionKey, pipeline.channel()); + } else if (targetSslHandler != null) { + http2Unavailable(partitionKey); } } @@ -1372,6 +1408,10 @@ public boolean isOpen() { return channelPool.isOpen(); } + boolean isHttp2Enabled() { + return config.isHttp2Enabled(); + } + public boolean isHttp2CleartextEnabled() { return config.isHttp2Enabled() && config.isHttp2CleartextEnabled(); } 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 b725f9d97..5f7814657 100755 --- a/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java +++ b/client/src/main/java/org/asynchttpclient/netty/channel/NettyConnectListener.java @@ -246,6 +246,12 @@ protected void onSuccess(Channel value) { } registerHttp2AndManageSemaphore(channel, semaphore, permit); } + if (!http2Negotiated && !uri.isWebSocket() && channelManager.isHttp2Enabled()) { + // We offered h2 and did not get it, so this connection will never register HTTP/2 + // and a request already waiting for one would wait out its connect timeout. No ALPN + // at all counts the same: RFC 9113 section 3.2 requires it for h2 over TLS. + channelManager.http2Unavailable(future.getPartitionKey()); + } writeRequest(channel); } diff --git a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java index 2f6058af7..df373c32d 100644 --- a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java +++ b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java @@ -71,6 +71,7 @@ import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.SEE_OTHER_303; import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.TEMPORARY_REDIRECT_307; import static org.asynchttpclient.util.HttpUtils.followRedirect; +import static org.asynchttpclient.util.MiscUtils.isEmpty; import static org.asynchttpclient.util.ThrowableUtil.unknownStackTrace; public class Redirect30xInterceptor { @@ -119,6 +120,12 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture int statusCode, Realm realm) throws Exception { if (followRedirect(config, request)) { + String location = response.headers().get(LOCATION); + if (isEmpty(location)) { + // RFC 9110 section 15.4 only redirects when a Location is provided, and an empty one + // resolves back to the current URI. + return false; + } if (future.incrementAndGetCurrentRedirectCount() >= config.getMaxRedirects()) { throw maxRedirectException; @@ -127,6 +134,7 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture future.setInAuth(false); future.setInProxyAuth(false); future.setScramContext(null); + future.tightenRedirectRefusals(request); String originalMethod = request.getMethod(); boolean isPost = originalMethod.equals(POST); @@ -141,8 +149,6 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture (statusCode == SEE_OTHER_303 || legacyPostToGet); boolean keepBody = statusCode != SEE_OTHER_303 && !switchToGet; - HttpHeaders responseHeaders = response.headers(); - String location = responseHeaders.get(LOCATION); // Location resolves against the target URI of the request actually sent on this leg // (RFC 9110 section 15.4, modification 1), and the gates below must judge that same URI. // A 401 or 407 retry leaves the future's own target behind, so do not read it here. @@ -153,12 +159,12 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture boolean schemeDowngrade = currentUri.isSecured() && !newUri.isSecured(); // Refuse before ensureBodyReplayable, whose IOException an IOExceptionFilter would replay. - if (schemeDowngrade && refuseSchemeDowngrade(request)) { + if (schemeDowngrade && refuseSchemeDowngrade(future)) { throw new RedirectRefusedException(Reason.SCHEME_DOWNGRADE, statusCode, currentUri, newUri); } BodyRepresentation bodyRepresentation = keepBody ? selectedBodyRepresentation(request) : BodyRepresentation.NONE; - if (keepBody && refuseCrossOriginBody(request) + if (keepBody && refuseCrossOriginBody(future) && !sameOrigin(currentUri, newUri) && !secureUpgrade(currentUri, newUri) && bodyRepresentation != BodyRepresentation.NONE) { throw new RedirectRefusedException(Reason.CROSS_ORIGIN_BODY, statusCode, currentUri, newUri); @@ -285,22 +291,24 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture } // Tightening only, unlike followRedirect: these are security switches, and a framework layer that builds - // the Request should not be able to void a posture the operator set on the client. - private boolean refuseSchemeDowngrade(Request request) { - return refuseSchemeDowngradeOnRedirect || Boolean.TRUE.equals(request.getRefuseSchemeDowngradeOnRedirect()); + // the Request should not be able to void a posture the operator set on the client or on an earlier hop. + private boolean refuseSchemeDowngrade(NettyResponseFuture future) { + return refuseSchemeDowngradeOnRedirect || future.isRefuseSchemeDowngradeLatched(); } - private boolean refuseCrossOriginBody(Request request) { - return refuseCrossOriginBodyOnRedirect || Boolean.TRUE.equals(request.getRefuseCrossOriginBodyOnRedirect()); + private boolean refuseCrossOriginBody(NettyResponseFuture future) { + return refuseCrossOriginBodyOnRedirect || future.isRefuseCrossOriginBodyLatched(); } /** - * Same scheme, host and effective port, per RFC 6454 section 4. Not {@link Uri#isSameBase(Uri)}, which - * compares hosts with {@link String#equals}. Hosts fold ASCII-only, the {@code i;ascii-casemap} collation - * that step 5 of that section asks for: {@link String#equalsIgnoreCase} + * Same scheme, host and effective port, per RFC 6454 section 4. Hosts fold ASCII-only, the + * {@code i;ascii-casemap} collation that step 5 of that section asks for: {@link String#equalsIgnoreCase} * folds Unicode and would call {@code i.example} equal to a host starting {@code U+0130}, a different * host. Nothing here does IDNA either, so a Unicode host and its A-label read as different origins, * erring towards refusal. + *

+ * Equivalent to {@link Uri#isSameBase(Uri)} today. That one also gates credential stripping and must + * stay at least as strict as this, so tighten this only by tightening that first. */ static boolean sameOrigin(Uri from, Uri to) { return from.getScheme().equals(to.getScheme()) 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 7bef1453e..d205ee739 100755 --- a/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java +++ b/client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java @@ -641,6 +641,7 @@ private NettyResponseFuture newNettyResponseFuture(Request request, Async proxyServer); future.setUseAbsoluteRequestDeadline(useAbsoluteRequestDeadline(config, request)); + future.tightenRedirectRefusals(request); String expectHeader = request.getHeaders().get(EXPECT); if (HttpHeaderValues.CONTINUE.contentEqualsIgnoreCase(expectHeader)) { @@ -1338,6 +1339,10 @@ private ListenableFuture reuseOrDeferHttp2Connection(Request request, Pro if (!request.getUri().isSecured() && !channelManager.isHttp2CleartextEnabled()) { return null; } + // A secured origin can still turn out to speak HTTP/1.1, which the scheme cannot tell us. + if (!config.isHttp2Enabled() || channelManager.isHttp2KnownUnavailable(h2Key)) { + return null; + } new Http2ConnectionWaiter<>(request, proxy, future, asyncHandler, override, semaphoreException).arm(); return future; } @@ -1406,6 +1411,8 @@ void arm() { Channel raced = pollHttp2(h2Key); if (raced != null) { accept(raced); + } else if (channelManager.isHttp2KnownUnavailable(h2Key)) { + accept(null); } } diff --git a/client/src/main/java/org/asynchttpclient/uri/Uri.java b/client/src/main/java/org/asynchttpclient/uri/Uri.java index 1f93f69c0..492f67773 100644 --- a/client/src/main/java/org/asynchttpclient/uri/Uri.java +++ b/client/src/main/java/org/asynchttpclient/uri/Uri.java @@ -15,6 +15,7 @@ */ package org.asynchttpclient.uri; +import io.netty.util.AsciiString; import org.asynchttpclient.util.StringBuilderPool; import org.jetbrains.annotations.Nullable; @@ -214,9 +215,13 @@ public String getAuthority() { return host + ':' + getExplicitPort(); } + /** + * Same scheme, host and effective port. The host folds ASCII-only, the comparison RFC 9110 section 4.2.3 + * asks for; a Unicode fold would make {@code k.example} and a host spelled with U+212A the same base. + */ public boolean isSameBase(Uri other) { return scheme.equals(other.getScheme()) - && host.equals(other.getHost()) + && AsciiString.contentEqualsIgnoreCase(host, other.getHost()) && getExplicitPort() == other.getExplicitPort(); } diff --git a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java index 82d2570b4..96aa58b49 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java @@ -117,6 +117,12 @@ public static void startServers() throws Exception { exchange.close(); }); + serverA.createContext("/redirect-case-differing-host", exchange -> { + exchange.getResponseHeaders().add("Location", "http://LOCALHOST:" + portA + "/final"); + exchange.sendResponseHeaders(302, -1); + exchange.close(); + }); + serverA.createContext("/final", exchange -> { lastAuthHeaderOnA.set(exchange.getRequestHeaders().getFirst("Authorization")); lastCookieHeaderOnA.set(exchange.getRequestHeaders().getFirst("Cookie")); @@ -862,6 +868,31 @@ void portChangeOnSameHostIsTreatedAsCrossOrigin() throws Exception { } } + /** + * A host differing only in ASCII case is the same origin, so credentials are kept. The cookie store is + * off because it lowercases the host itself and would return the cookie either way. + */ + @Test + void caseDifferingHostKeepsCredentials() throws Exception { + DefaultAsyncHttpClientConfig config = new DefaultAsyncHttpClientConfig.Builder() + .setFollowRedirect(true) + .setCookieStore(null) + .build(); + try (DefaultAsyncHttpClient client = new DefaultAsyncHttpClient(config)) { + lastAuthHeaderOnA.set(null); + lastCookieHeaderOnA.set(null); + + client.prepareGet("http://localhost:" + portA + "/redirect-case-differing-host") + .setHeader("Authorization", "Bearer case-differing-token") + .setHeader("Cookie", "session=case-differing-cookie") + .execute() + .get(5, TimeUnit.SECONDS); + + assertEquals("Bearer case-differing-token", lastAuthHeaderOnA.get()); + assertEquals("session=case-differing-cookie", lastCookieHeaderOnA.get()); + } + } + /** * Client-wide credentials set via {@code config.setRealm(...)} must not be sent to a * cross-domain redirect target, even when that target answers 401 to solicit them. The diff --git a/client/src/test/java/org/asynchttpclient/RedirectRefusalTest.java b/client/src/test/java/org/asynchttpclient/RedirectRefusalTest.java index 55799b4c9..25b97fcc8 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectRefusalTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectRefusalTest.java @@ -21,6 +21,7 @@ import jakarta.servlet.http.HttpServletResponse; import org.asynchttpclient.filter.FilterContext; import org.asynchttpclient.filter.IOExceptionFilter; +import org.asynchttpclient.filter.ResponseFilter; import org.asynchttpclient.handler.MaxRedirectException; import org.asynchttpclient.handler.RedirectRefusedException; import org.asynchttpclient.uri.Uri; @@ -75,6 +76,7 @@ public class RedirectRefusalTest extends AbstractBasicTest { private final AtomicBoolean targetHit = new AtomicBoolean(); + private final AtomicBoolean retried = new AtomicBoolean(); private final AtomicReference methodOnTarget = new AtomicReference<>(); private final AtomicReference bodyOnTarget = new AtomicReference<>(); @@ -93,6 +95,7 @@ public void setUpGlobal() throws Exception { @BeforeEach public void resetCaptures() { targetHit.set(false); + retried.set(false); methodOnTarget.set(null); bodyOnTarget.set(null); } @@ -396,6 +399,15 @@ public void theMessageIsBuiltFromBaseUrlsWhoeverConstructsIt() { assertTrue(message.contains("http://127.0.0.1:8080"), message); } + @ParameterizedTest + @ValueSource(strings = {"/no-location", "/empty-location"}) + public void aRedirectWithNoUsableLocationIsDeliveredAsAResponse(String path) throws Exception { + try (AsyncHttpClient client = asyncHttpClient(followingConfig())) { + Response response = client.prepareGet(plain(path)).execute().get(TIMEOUT, TimeUnit.SECONDS); + assertEquals(302, response.getStatusCode()); + } + } + // ---------------------------------------------------------------- per-request override @Test @@ -432,6 +444,58 @@ public void aRequestCannotAllowTheCrossOriginBodyHopWhereTheClientRefuses() thro } } + @Test + public void aFilterThatRebuildsTheRequestCannotRelaxTheOverride() throws Exception { + DefaultAsyncHttpClientConfig.Builder builder = followingConfig() + .addResponseFilter(new ResponseFilter() { + @Override + public FilterContext filter(FilterContext ctx) { + if (ctx.getResponseStatus() != null && ctx.getResponseStatus().getStatusCode() == 503) { + return new FilterContext.FilterContextBuilder<>(ctx) + .request(new RequestBuilder("PUT").setUrl(plain("/cross-origin")) + .setBody("payload").build()) + .replayRequest(true) + .build(); + } + return ctx; + } + }); + + try (AsyncHttpClient client = asyncHttpClient(builder)) { + expectRefusal(client.preparePut(plain("/retry-once")).setBody("payload") + .setRefuseCrossOriginBodyOnRedirect(true)); + assertFalse(targetHit.get(), "a rebuilt request must not drop the caller's refusal"); + } + } + + /** + * The mirror of the test above. The fold is one-way, not a copy, so a rebuilt request can still add a + * refusal the caller never asked for. + */ + @Test + public void aFilterThatRebuildsTheRequestCanStillTightenTheOverride() throws Exception { + DefaultAsyncHttpClientConfig.Builder builder = followingConfig() + .addResponseFilter(new ResponseFilter() { + @Override + public FilterContext filter(FilterContext ctx) { + if (ctx.getResponseStatus() != null && ctx.getResponseStatus().getStatusCode() == 503) { + return new FilterContext.FilterContextBuilder<>(ctx) + .request(new RequestBuilder("PUT").setUrl(plain("/cross-origin")) + .setBody("payload") + .setRefuseCrossOriginBodyOnRedirect(true).build()) + .replayRequest(true) + .build(); + } + return ctx; + } + }); + + try (AsyncHttpClient client = asyncHttpClient(builder)) { + expectRefusal(client.preparePut(plain("/retry-once")).setBody("payload")); + assertFalse(targetHit.get(), "a refusal set on the rebuilt request must reach the exchange"); + } + } + /** * A 303 rebuilds from an empty builder, the only branch where the override is copied across by hand. * Both client arms are off, so a refusal can only come from the override surviving that rebuild. @@ -501,6 +565,16 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques case "/cross-origin-308": redirect(response, 308, "http://127.0.0.1:" + port1 + "/target"); return; + case "/retry-once": + response.setStatus(retried.compareAndSet(false, true) ? 503 : 200); + return; + case "/no-location": + response.setStatus(302); + return; + case "/empty-location": + response.setStatus(302); + response.setHeader("Location", ""); + return; case "/cross-origin": redirect(response, 307, "http://127.0.0.1:" + port1 + "/target"); return; diff --git a/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2WaiterTest.java b/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2WaiterTest.java index e2d4815f1..daefc8428 100644 --- a/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2WaiterTest.java +++ b/client/src/test/java/org/asynchttpclient/netty/channel/ChannelManagerHttp2WaiterTest.java @@ -24,6 +24,7 @@ import org.junit.jupiter.api.Timeout; import java.lang.reflect.Field; +import java.net.InetAddress; import java.util.Map; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -219,6 +220,95 @@ public void removingLastWaiterPrunesEmptyEntry() throws Exception { } } + @Test + @Timeout(unit = TimeUnit.SECONDS, value = 30) + public void http2UnavailableWakesWaitersWithNull() { + Timer timer = new HashedWheelTimer(); + ChannelManager cm = newChannelManager(timer); + try { + AtomicReference woken = new AtomicReference<>(); + AtomicBoolean called = new AtomicBoolean(); + cm.addHttp2ConnectionWaiter(KEY, c -> { + woken.set(c); + called.set(true); + }); + + cm.http2Unavailable(KEY); + + assertTrue(called.get(), "a handshake that settled on HTTP/1.1 must release the waiter"); + assertNull(woken.get(), "the waiter must be failed, not handed a connection"); + } finally { + cm.close(); + timer.stop(); + } + } + + @Test + @Timeout(unit = TimeUnit.SECONDS, value = 30) + public void http2UnavailableIsStickyUntilHttp2Registers() { + Timer timer = new HashedWheelTimer(); + ChannelManager cm = newChannelManager(timer); + Channel channel = new EmbeddedChannel(); + try { + assertFalse(cm.isHttp2KnownUnavailable(KEY)); + + cm.http2Unavailable(KEY); + assertTrue(cm.isHttp2KnownUnavailable(KEY), + "the mark must outlive the handshake that set it, or a later waiter arms into an empty set"); + + cm.registerHttp2Connection(KEY, channel); + assertFalse(cm.isHttp2KnownUnavailable(KEY), "a registration must clear the mark"); + } finally { + channel.close(); + cm.close(); + timer.stop(); + } + } + + /** + * ALPN is per-connection, so one IP of a host settling on HTTP/1.1 says nothing about the others. The + * mark is keyed by host, so it must not be set while a sibling that did negotiate HTTP/2 is registered. + */ + @Test + @Timeout(unit = TimeUnit.SECONDS, value = 30) + public void http2UnavailableDefersToARegisteredSibling() throws Exception { + Timer timer = new HashedWheelTimer(); + ChannelManager cm = newChannelManager(timer); + Channel sibling = new EmbeddedChannel(); + try { + cm.registerHttp2Connection(new RoundRobinPartitionKey(KEY, InetAddress.getByName("127.0.0.1")), sibling); + AtomicBoolean called = new AtomicBoolean(); + cm.addHttp2ConnectionWaiter(KEY, c -> called.set(true)); + + cm.http2Unavailable(new RoundRobinPartitionKey(KEY, InetAddress.getByName("127.0.0.2"))); + + assertFalse(cm.isHttp2KnownUnavailable(KEY), "a live sibling must keep the host usable"); + assertFalse(called.get(), "the waiter can still multiplex onto the sibling, so it must stay parked"); + } finally { + sibling.close(); + cm.close(); + timer.stop(); + } + } + + /** + * The mark is stored under the base key, so a per-IP key marks the host. Without that collapse a waiter + * parked on the host key would never see it. + */ + @Test + @Timeout(unit = TimeUnit.SECONDS, value = 30) + public void http2UnavailableOnOneIpMarksTheWholeHost() throws Exception { + Timer timer = new HashedWheelTimer(); + ChannelManager cm = newChannelManager(timer); + try { + cm.http2Unavailable(new RoundRobinPartitionKey(KEY, InetAddress.getByName("127.0.0.1"))); + assertTrue(cm.isHttp2KnownUnavailable(KEY)); + } finally { + cm.close(); + timer.stop(); + } + } + @SuppressWarnings("unchecked") private static Map waiterMap(ChannelManager cm) throws Exception { Field field = ChannelManager.class.getDeclaredField("http2ConnectionWaiters"); diff --git a/client/src/test/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptorTest.java b/client/src/test/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptorTest.java index 74caa7b21..7c87409bb 100644 --- a/client/src/test/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptorTest.java +++ b/client/src/test/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptorTest.java @@ -18,6 +18,7 @@ import org.asynchttpclient.uri.Uri; import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -95,6 +96,29 @@ public void sameOriginSeparatesSchemeAndPort() { host("https", "example.com", 8443))); } + /** + * sameOrigin gates the body and Uri.isSameBase gates credential stripping. They agree today; if one is + * ever relaxed on its own, the looser gate would send content somewhere the stricter one still treats as + * another origin. This fails when they diverge, whichever way. + */ + @Test + public void sameOriginAndIsSameBaseStayInStep() { + Uri[][] pairs = { + {host("https", "example.com", 443), host("https", "example.com", 443)}, + {host("https", "example.com", -1), host("https", "example.com", 443)}, + {host("https", "example.com", 443), host("https", "EXAMPLE.com", 443)}, + {host("https", "example.com", 443), host("https", "example.com", 8443)}, + {host("https", "example.com", 443), host("http", "example.com", 443)}, + {host("https", "example.com", 443), host("https", "other.example", 443)}, + {host("https", "i.example", 443), host("https", "\u0130.example", 443)}, + {host("https", "k.example", 443), host("https", "\u212A.example", 443)}, + }; + for (Uri[] pair : pairs) { + assertEquals(pair[0].isSameBase(pair[1]), Redirect30xInterceptor.sameOrigin(pair[0], pair[1]), + pair[0] + " vs " + pair[1]); + } + } + @Test public void acceptsTheFollowedRedirectStatuses() { for (int statusCode : new int[]{301, 302, 303, 307, 308}) { diff --git a/client/src/test/java/org/asynchttpclient/netty/request/Http2ConnectionWaiterGateTest.java b/client/src/test/java/org/asynchttpclient/netty/request/Http2ConnectionWaiterGateTest.java index abb56c63f..6d8f2b4cd 100644 --- a/client/src/test/java/org/asynchttpclient/netty/request/Http2ConnectionWaiterGateTest.java +++ b/client/src/test/java/org/asynchttpclient/netty/request/Http2ConnectionWaiterGateTest.java @@ -23,7 +23,11 @@ import org.asynchttpclient.Response; import org.asynchttpclient.exception.TooManyConnectionsException; import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; import org.eclipse.jetty.server.handler.AbstractHandler; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import java.io.IOException; @@ -35,25 +39,66 @@ import static org.asynchttpclient.Dsl.asyncHttpClient; import static org.asynchttpclient.Dsl.config; +import static org.asynchttpclient.test.TestUtils.addHttpConnector; +import static org.asynchttpclient.test.TestUtils.addHttpsConnector; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; /** - * A request that cannot take a connection permit only defers on an HTTP/2 connection that could exist. An - * HTTP/2 connection is registered from ALPN on a secured origin or from an h2c upgrade on a cleartext one, - * so for a cleartext origin with h2c disabled, which is the default, nothing ever registers and the waiter can - * only expire. Arming it there holds the request for the whole {@code connectTimeout} before failing it - * with the permit exception it already had, overriding the {@code acquireFreeChannelTimeout} the caller - * asked for (0 by default: fail fast). + * A request that cannot take a connection permit only defers on an HTTP/2 connection that could exist. One + * is registered from ALPN on a secured origin or from an h2c upgrade on a cleartext one, so neither a + * cleartext origin with h2c disabled (the default) nor an origin whose handshake settled on HTTP/1.1 will + * ever register one. Arming a waiter there holds the request for the whole {@code connectTimeout} before + * failing it with the permit exception it already had, long after the {@code acquireFreeChannelTimeout} + * that brought it down this path expired. */ public class Http2ConnectionWaiterGateTest extends AbstractBasicTest { - private final CountDownLatch release = new CountDownLatch(1); + private volatile CountDownLatch release = new CountDownLatch(1); + + @BeforeEach + public void freshLatch() { + release = new CountDownLatch(1); + } @Override - public AbstractHandler configureHandler() throws Exception { - return new BlockingHandler(); + @BeforeAll + public void setUpGlobal() throws Exception { + server = new Server(); + ServerConnector plain = addHttpConnector(server); + ServerConnector secure = addHttpsConnector(server); + server.setHandler(new BlockingHandler()); + server.start(); + port1 = plain.getLocalPort(); + port2 = secure.getLocalPort(); + } + + /** + * Jetty's TLS connector speaks HTTP/1.1, so the scheme cannot rule HTTP/2 out and only the handshake + * can. Before the fix the over-cap request waited out connectTimeout for a connection ALPN had already + * decided would never be HTTP/2. + */ + @Test + public void permitExhaustedOnAnHttp11TlsOriginFailsWithoutWaitingOutConnectTimeout() throws Exception { + String url = "https://localhost:" + port2 + "/"; + try (AsyncHttpClient client = asyncHttpClient(config() + .setMaxConnections(1) + .setUseInsecureTrustManager(true) + .setConnectTimeout(Duration.ofSeconds(30)))) { + + Future holder = client.prepareGet(url).execute(); + try { + // No isDone() assertion here, unlike the cleartext sibling: the marker only exists once the + // handshake has settled, so this refusal is necessarily asynchronous. + ExecutionException failure = assertThrows(ExecutionException.class, + () -> client.prepareGet(url).execute().get(10, TimeUnit.SECONDS)); + assertInstanceOf(TooManyConnectionsException.class, failure.getCause()); + } finally { + release.countDown(); + } + holder.get(TIMEOUT, TimeUnit.SECONDS); + } } @Test diff --git a/client/src/test/java/org/asynchttpclient/uri/UriTest.java b/client/src/test/java/org/asynchttpclient/uri/UriTest.java index f3fe653c0..ba1da7aa6 100644 --- a/client/src/test/java/org/asynchttpclient/uri/UriTest.java +++ b/client/src/test/java/org/asynchttpclient/uri/UriTest.java @@ -410,4 +410,14 @@ public void testToUrlWithoutUserInfoReturnsTheMemoisedUrlWhenThereIsNone() { assertSame(uri.toUrl(), uri.toUrlWithoutUserInfo()); } + @RepeatedIfExceptionsTest(repeats = 5) + public void testIsSameBaseFoldsHostCaseButOnlyInAscii() { + Uri lower = Uri.create("https://example.com/a"); + assertTrue(lower.isSameBase(Uri.create("https://EXAMPLE.com/b"))); + assertFalse(lower.isSameBase(Uri.create("http://example.com/b"))); + assertFalse(lower.isSameBase(Uri.create("https://example.com:8443/b"))); + assertFalse(Uri.create("https://i.example/a").isSameBase(Uri.create("https://\u0130.example/b"))); + assertFalse(Uri.create("https://k.example/a").isSameBase(Uri.create("https://\u212A.example/b"))); + } + }