From a5f654ef3a8b30a2c195a92bb64955199a11f515 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Wed, 4 Jun 2025 21:04:14 +0200 Subject: [PATCH] HTTPCLIENT-2372 - Normalize HttpHost port comparison to treat implicit default ports as equal --- .../http/impl/DefaultRedirectStrategy.java | 43 +++++++++++++++++- .../http/impl/async/H2AsyncClientBuilder.java | 2 +- .../impl/async/HttpAsyncClientBuilder.java | 2 +- .../http/impl/classic/HttpClientBuilder.java | 2 +- .../impl/TestDefaultRedirectStrategy.java | 44 +++++++++++++++++++ 5 files changed, 89 insertions(+), 4 deletions(-) diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java index dbfaed4c44..9aeb28abac 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java +++ b/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
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, diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/H2AsyncClientBuilder.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/H2AsyncClientBuilder.java index 1e85bc955c..56de4b0401 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/H2AsyncClientBuilder.java +++ b/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), diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java index 933b8ae781..4f56b46804 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/HttpAsyncClientBuilder.java +++ b/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), diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java index 4ba68b9192..3f871ff902 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/HttpClientBuilder.java +++ b/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), diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java index 406cb12772..ccbfc916f7 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestDefaultRedirectStrategy.java +++ b/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)); + } + + }