Fix redirect and HTTP/2 follow-up defects

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
      */

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

client/src/main/java/org/asynchttpclient/netty/NettyResponseFuture.java

@@ -167,6 +167,8 @@ public final class NettyResponseFuture<V> implements ListenableFuture<V> {
     // 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<V> 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;
     }

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<Object, Set<Consumer<Channel>>> 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<Object> 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<Channel> 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<Consumer<Channel>> 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();
     }

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);
                 }
 

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.
+     * <p>
+     * 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())

client/src/main/java/org/asynchttpclient/netty/request/NettyRequestSender.java

@@ -641,6 +641,7 @@ private <T> NettyResponseFuture<T> 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 <T> ListenableFuture<T> 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);
             }
         }
 

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;
 
@@ -174,7 +175,7 @@
     }
 
     /**
      * @return [scheme]://[hostname](:[port])/path. Port is omitted if it matches the scheme's default one.
      */
     public String toBaseUrl() {
         StringBuilder sb = StringBuilderPool.DEFAULT.stringBuilder();
@@ -214,9 +215,13 @@
         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();
     }
 
@@ -253,7 +258,7 @@
     }
 
     @Override
     public boolean equals(Object obj) {
         if (this == obj) {
             return true;
         }

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