From 5dd64e31fe4b2b6a44ba820df0f2c87ed3b177ad Mon Sep 17 00:00:00 2001 From: Daniel VEGA Date: Tue, 15 Jul 2025 16:35:45 +0200 Subject: [PATCH] [HTTPCLIENT-2383] Make LaxRedirectStrategy to allow redirects if sensitive headers are part of the request --- .../sync/StandardTestClientBuilder.java | 7 ++ .../extension/sync/TestClientBuilder.java | 5 ++ .../client5/testing/sync/TestRedirects.java | 54 ++++++++++++ .../http/impl/LaxRedirectStrategy.java | 11 +++ .../impl/TestDefaultRedirectStrategy.java | 7 -- .../http/impl/TestLaxRedirectStrategy.java | 83 +++++++++++++++++++ 6 files changed, 160 insertions(+), 7 deletions(-) diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/StandardTestClientBuilder.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/StandardTestClientBuilder.java index 15be8f914b..44bae7ab15 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/StandardTestClientBuilder.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/StandardTestClientBuilder.java @@ -42,6 +42,7 @@ import org.apache.hc.client5.http.impl.classic.HttpClientBuilder; import org.apache.hc.client5.http.impl.io.PoolingHttpClientConnectionManagerBuilder; import org.apache.hc.client5.http.io.HttpClientConnectionManager; +import org.apache.hc.client5.http.protocol.RedirectStrategy; import org.apache.hc.client5.http.ssl.DefaultClientTlsStrategy; import org.apache.hc.client5.testing.SSLTestContexts; import org.apache.hc.core5.http.Header; @@ -167,6 +168,12 @@ public TestClientBuilder setUnixDomainSocket(final Path unixDomainSocket) { return this; } + @Override + public TestClientBuilder setRedirectStrategy(final RedirectStrategy redirectStrategy) { + this.clientBuilder.setRedirectStrategy(redirectStrategy); + return this; + } + @Override public TestClient build() throws Exception { final HttpClientConnectionManager connectionManagerCopy = connectionManager != null ? connectionManager : diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/TestClientBuilder.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/TestClientBuilder.java index 775ff47c4b..f84d15de35 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/TestClientBuilder.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/extension/sync/TestClientBuilder.java @@ -37,6 +37,7 @@ import org.apache.hc.client5.http.auth.CredentialsProvider; import org.apache.hc.client5.http.classic.ExecChainHandler; import org.apache.hc.client5.http.io.HttpClientConnectionManager; +import org.apache.hc.client5.http.protocol.RedirectStrategy; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpRequestInterceptor; import org.apache.hc.core5.http.HttpResponseInterceptor; @@ -108,6 +109,10 @@ default TestClientBuilder setUnixDomainSocket(Path unixDomainSocket) { throw new UnsupportedOperationException("Operation not supported by " + getProtocolLevel()); } + default TestClientBuilder setRedirectStrategy(RedirectStrategy redirectStrategy) { + throw new UnsupportedOperationException("Operation not supported by " + getProtocolLevel()); + } + TestClient build() throws Exception; } diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java index 25c9e7f413..1371dd8d0b 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java @@ -50,6 +50,7 @@ import org.apache.hc.client5.http.cookie.BasicCookieStore; import org.apache.hc.client5.http.cookie.CookieStore; import org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy; +import org.apache.hc.client5.http.impl.LaxRedirectStrategy; import org.apache.hc.client5.http.impl.cookie.BasicClientCookie; import org.apache.hc.client5.http.protocol.HttpClientContext; import org.apache.hc.client5.http.protocol.RedirectLocations; @@ -761,4 +762,57 @@ void testCrossSiteRedirectWithSensitiveHeaders(final String headerName) throws E } } + @ParameterizedTest(name = "{displayName}; manually added header: {0}") + @ValueSource(strings = {HttpHeaders.AUTHORIZATION, HttpHeaders.COOKIE}) + void testCrossSiteRedirectWithSensitiveHeadersAndLaxRedirectStrategy(final String headerName) throws Exception { + configureClient(builder -> builder + .setRedirectStrategy(new LaxRedirectStrategy()) + ); + final URIScheme scheme = scheme(); + final TestServer secondServer = new TestServerBootstrap(scheme()) + .register("/random/*", new RandomHandler()) + .build(); + try { + final InetSocketAddress address2 = secondServer.start(); + + final HttpHost redirectTarget = new HttpHost(scheme.name(), "localhost", address2.getPort()); + + configureServer(bootstrap -> bootstrap + .setExchangeHandlerDecorator(requestHandler -> new RedirectingDecorator( + requestHandler, + requestUri -> { + final URI location = new URIBuilder(requestUri) + .setHttpHost(redirectTarget) + .setPath("/random/100") + .build(); + return new Redirect(HttpStatus.SC_MOVED_TEMPORARILY, location.toString()); + })) + .register("/random/*", new RandomHandler()) + ); + + final HttpHost target = startServer(); + + final TestClient client = client(); + final HttpClientContext context = HttpClientContext.create(); + + client.execute(target, + ClassicRequestBuilder.get() + .setHttpHost(target) + .setPath("/oldlocation") + .setHeader(headerName, "custom header") + .build(), + context, + response -> { + Assertions.assertEquals(HttpStatus.SC_OK, response.getCode()); + EntityUtils.consume(response.getEntity()); + return null; + }); + final RedirectLocations redirects = context.getRedirectLocations(); + Assertions.assertNotNull(redirects); + Assertions.assertEquals(1, redirects.size()); + } finally { + secondServer.shutdown(CloseMode.GRACEFUL); + } + } + } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/LaxRedirectStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/LaxRedirectStrategy.java index 49fbf7abee..5eff35b529 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/LaxRedirectStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/LaxRedirectStrategy.java @@ -36,6 +36,7 @@ import org.apache.hc.core5.annotation.ThreadingBehavior; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; @@ -84,6 +85,16 @@ public boolean isRedirected(final HttpRequest request, final HttpResponse respon } } + @Override + public boolean isRedirectAllowed( + final HttpHost currentTarget, + final HttpHost newTarget, + final HttpRequest redirect, + final HttpContext context) { + // Always return true, without checking possible sensitive headers. + return true; + } + protected boolean isRedirectable(final String method) { for (final String m : REDIRECT_METHODS) { if (m.equalsIgnoreCase(method)) { 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 ccbfc916f7..a874b42682 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 @@ -261,13 +261,6 @@ void testRedirectAllowed() throws Exception { BasicRequestBuilder.get("/").build(), null)); - Assertions.assertTrue(redirectStrategy.isRedirectAllowed( - new HttpHost("somehost", 1234), - new HttpHost("somehost", 1234), - BasicRequestBuilder.get("/") - .build(), - null)); - Assertions.assertTrue(redirectStrategy.isRedirectAllowed( new HttpHost("somehost", 1234), new HttpHost("somehost", 1234), diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestLaxRedirectStrategy.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestLaxRedirectStrategy.java index 1df2d0e4c0..366aa06f7d 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestLaxRedirectStrategy.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/TestLaxRedirectStrategy.java @@ -32,10 +32,14 @@ import org.apache.hc.client5.http.classic.methods.HttpHead; import org.apache.hc.client5.http.classic.methods.HttpPost; import org.apache.hc.client5.http.classic.methods.HttpPut; +import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpRequest; import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.support.BasicRequestBuilder; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -81,4 +85,83 @@ private void testIsRedirected(final HttpRequest request, final boolean expected) assertEquals(expected, strategy.isRedirected(request, response, context)); } + + /** + * Test {@link LaxRedirectStrategy#isRedirectAllowed(HttpHost, HttpHost, HttpRequest, HttpContext)}. + * The method should return true for all requests, enabling backward compatibility. + * + **/ + @Test + void testRedirectAllowed() throws Exception { + final LaxRedirectStrategy redirectStrategy = new LaxRedirectStrategy(); + + // Same host and port + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 1234), + BasicRequestBuilder.get("/").build(), + null)); + + // Same host and port with Authorization header + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 1234), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.AUTHORIZATION, "let me pass") + .build(), + null)); + + // Same host and port with Cookie header + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 1234), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.COOKIE, "stuff=blah") + .build(), + null)); + + // Different host and same port + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("someotherhost", 1234), + BasicRequestBuilder.get("/") + .build(), + null)); + + // Different host and same port with Authorization header + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("someotherhost", 1234), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.AUTHORIZATION, "let me pass") + .build(), + null)); + + // Different host and same port with Cookie header + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("someotherhost", 1234), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.COOKIE, "stuff=blah") + .build(), + null)); + + // Same host and different ports with Authorization header + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 80), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.AUTHORIZATION, "let me pass") + .build(), + null)); + + // Same host and different ports with Cookie header + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 80), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.COOKIE, "stuff=blah") + .build(), + null)); + } } \ No newline at end of file