HTTPCLIENT-2372 - Normalize HttpHost port comparison to treat implicit default ports as equal (#643)

Author: arturobernalg

httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java

@@ -32,6 +32,7 @@
 import java.util.Iterator;
 import java.util.Locale;
 
+import org.apache.hc.client5.http.SchemePortResolver;
 import org.apache.hc.client5.http.protocol.RedirectStrategy;
 import org.apache.hc.client5.http.utils.URIUtils;
 import org.apache.hc.core5.annotation.Contract;
@@ -56,18 +57,44 @@
 @Contract(threading = ThreadingBehavior.STATELESS)
 public class DefaultRedirectStrategy implements RedirectStrategy {
 
+    private final SchemePortResolver schemePortResolver;
+
     /**
      * Default instance of {@link DefaultRedirectStrategy}.
      */
     public static final DefaultRedirectStrategy INSTANCE = new DefaultRedirectStrategy();
 
+    /**
+     * Creates a new {@code DefaultRedirectStrategy} using the given {@link SchemePortResolver}.
+     * If {@code schemePortResolver} is {@code null}, this will default to
+     * {@link DefaultSchemePortResolver#INSTANCE}.
+     *
+     * @param schemePortResolver the resolver to use for determining default ports
+     * @since 5.6
+     */
+    public DefaultRedirectStrategy(final SchemePortResolver schemePortResolver) {
+        this.schemePortResolver = schemePortResolver != null ? schemePortResolver : DefaultSchemePortResolver.INSTANCE;
+    }
+
+    /**
+     * Creates a new {@code DefaultRedirectStrategy} with the default
+     * {@link DefaultSchemePortResolver#INSTANCE}.
+     *
+     * @since 5.6
+     */
+    public DefaultRedirectStrategy() {
+        this(null);
+    }
+
     @Override
     public boolean isRedirectAllowed(
             final HttpHost currentTarget,
             final HttpHost newTarget,
             final HttpRequest redirect,
             final HttpContext context) {
-        if (!currentTarget.equals(newTarget)) {
+
+        // If authority (host + effective port) differs, strip sensitive headers
+        if (!isSameAuthority(currentTarget, newTarget)) {
             for (final Iterator<Header> it = redirect.headerIterator(); it.hasNext(); ) {
                 final Header header = it.next();
                 if (header.isSensitive()
@@ -80,6 +107,20 @@ public boolean isRedirectAllowed(
         return true;
     }
 
+    private boolean isSameAuthority(final HttpHost h1, final HttpHost h2) {
+        if (h1 == null || h2 == null) {
+            return false;
+        }
+        final String host1 = h1.getHostName();
+        final String host2 = h2.getHostName();
+        if (!host1.equalsIgnoreCase(host2)) {
+            return false;
+        }
+        final int port1 = schemePortResolver.resolve(h1);
+        final int port2 = schemePortResolver.resolve(h2);
+        return port1 == port2;
+    }
+
     @Override
     public boolean isRedirected(
             final HttpRequest request,

httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/H2AsyncClientBuilder.java

@@ -836,7 +836,7 @@ public CloseableHttpAsyncClient build() {
         if (!redirectHandlingDisabled) {
             RedirectStrategy redirectStrategyCopy = this.redirectStrategy;
             if (redirectStrategyCopy == null) {
-                redirectStrategyCopy = DefaultRedirectStrategy.INSTANCE;
+                redirectStrategyCopy = schemePortResolver != null ? new DefaultRedirectStrategy(schemePortResolver) : DefaultRedirectStrategy.INSTANCE;
             }
             execChainDefinition.addFirst(
                     new AsyncRedirectExec(routePlannerCopy, redirectStrategyCopy),

httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java

@@ -1023,7 +1023,7 @@ public CloseableHttpAsyncClient build() {
         if (!redirectHandlingDisabled) {
             RedirectStrategy redirectStrategyCopy = this.redirectStrategy;
             if (redirectStrategyCopy == null) {
-                redirectStrategyCopy = DefaultRedirectStrategy.INSTANCE;
+                redirectStrategyCopy = schemePortResolver != null ? new DefaultRedirectStrategy(schemePortResolver) : DefaultRedirectStrategy.INSTANCE;
             }
             execChainDefinition.addFirst(
                     new AsyncRedirectExec(routePlannerCopy, redirectStrategyCopy),

httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java

@@ -1011,7 +1011,7 @@ public CloseableHttpClient build() {
         if (!redirectHandlingDisabled) {
             RedirectStrategy redirectStrategyCopy = this.redirectStrategy;
             if (redirectStrategyCopy == null) {
-                redirectStrategyCopy = DefaultRedirectStrategy.INSTANCE;
+                redirectStrategyCopy = schemePortResolver != null ? new DefaultRedirectStrategy(schemePortResolver) : DefaultRedirectStrategy.INSTANCE;
             }
             execChainDefinition.addFirst(
                     new RedirectExec(routePlannerCopy, redirectStrategyCopy),

httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java

@@ -324,4 +324,48 @@ void testRedirectAllowed() throws Exception {
                 null));
     }
 
+
+
+
+    @Test
+    void testRedirectAllowedDefaultPortNormalization() {
+        final DefaultRedirectStrategy redirectStrategy = new DefaultRedirectStrategy();
+
+        // HTTPS with explicit 443 vs HTTPS with no port (defaults to 443)
+        final HttpHost explicitHttps = new HttpHost("https", "example.com", 443);
+        final HttpHost implicitHttps = new HttpHost("https", "example.com", -1);
+        Assertions.assertTrue(redirectStrategy.isRedirectAllowed(
+                explicitHttps,
+                implicitHttps,
+                BasicRequestBuilder.get("/")
+                        .addHeader(HttpHeaders.AUTHORIZATION, "token")
+                        .build(),
+                null));
+        Assertions.assertTrue(redirectStrategy.isRedirectAllowed(
+                implicitHttps,
+                explicitHttps,
+                BasicRequestBuilder.get("/")
+                        .addHeader(HttpHeaders.COOKIE, "cookie=123")
+                        .build(),
+                null));
+
+        final HttpHost explicitHttp = new HttpHost("http", "example.org", 80);
+        final HttpHost implicitHttp = new HttpHost("http", "example.org", -1);
+        Assertions.assertTrue(redirectStrategy.isRedirectAllowed(
+                explicitHttp,
+                implicitHttp,
+                BasicRequestBuilder.get("/")
+                        .addHeader(HttpHeaders.AUTHORIZATION, "token123")
+                        .build(),
+                null));
+        Assertions.assertTrue(redirectStrategy.isRedirectAllowed(
+                implicitHttp,
+                explicitHttp,
+                BasicRequestBuilder.get("/")
+                        .addHeader(HttpHeaders.COOKIE, "cookie=abc")
+                        .build(),
+                null));
+    }
+
+
 }