HTTPCLIENT-2383 Bypass sensitive-header check in LaxRedirectStrategy (#676)

* HTTPCLIENT-2383 Bypass sensitive-header check in LaxRedirectStrategy. Override isRedirectAllowed(...) to always return true, ensuring LaxRedirectStrategy follows redirects regardless of Authorization or other sensitive headers.

* [HTTPCLIENT-2383] Make LaxRedirectStrategy to allow redirects if sensitive headers are part of the request

---------

Co-authored-by: Daniel VEGA <[email protected]>

Author: arturobernalg

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 :

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

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

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 allow, regardless of sensitive headers
+        return true;
+    }
+
     protected boolean isRedirectable(final String method) {
         for (final String m : REDIRECT_METHODS) {
             if (m.equalsIgnoreCase(method)) {

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),

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

@@ -32,13 +32,18 @@
 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;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -68,6 +73,24 @@ void testIsRedirectedWithNonRedirectMethod() {
         testIsRedirected(new HttpPut("/put"), false);
     }
 
+    @Test
+    void testIsRedirectAllowedAlwaysTrue() {
+        final LaxRedirectStrategy strategy = new LaxRedirectStrategy();
+        final HttpContext context = mock(HttpContext.class);
+        final HttpHost current = new HttpHost("http", "localhost", 80);
+        final HttpHost next = new HttpHost("http", "example.com", 80);
+        // Create a request with an Authorization header
+        final HttpRequest request = new HttpGet("/test");
+        request.addHeader(HttpHeaders.AUTHORIZATION, "Bearer token");
+
+        // Even with sensitive headers and target change, LaxRedirectStrategy should allow it
+        assertTrue(
+                strategy.isRedirectAllowed(current, next, request, context),
+                "LaxRedirectStrategy should always allow redirects regardless of sensitive headers"
+        );
+    }
+
+
     private void testIsRedirected(final HttpRequest request, final boolean expected) {
         final LaxRedirectStrategy strategy = new LaxRedirectStrategy();
         final HttpResponse response = mock(HttpResponse.class);
@@ -81,4 +104,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));
+    }
 }