diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncRedirectsTest.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncRedirectsTest.java index 9071ea5230..61ff411b0b 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncRedirectsTest.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncRedirectsTest.java @@ -43,6 +43,7 @@ import org.apache.hc.client5.http.cookie.CookieStore; 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; import org.apache.hc.client5.testing.OldPathRedirectResolver; import org.apache.hc.client5.testing.extension.async.ClientProtocolLevel; import org.apache.hc.client5.testing.extension.async.ServerProtocolLevel; @@ -53,6 +54,7 @@ import org.apache.hc.core5.http.ContentType; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpException; +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; @@ -65,6 +67,8 @@ import org.hamcrest.CoreMatchers; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; abstract class AbstractHttpAsyncRedirectsTest extends AbstractIntegrationTestBase { @@ -624,4 +628,55 @@ void testCrossSiteRedirect() throws Exception { } } + @ParameterizedTest(name = "{displayName}; manually added header: {0}") + @ValueSource(strings = {HttpHeaders.AUTHORIZATION, HttpHeaders.COOKIE}) + void testCrossSiteRedirectWithSensitiveHeaders(final String headerName) throws Exception { + final URIScheme scheme = scheme(); + final TestAsyncServer secondServer = new TestAsyncServerBootstrap(scheme(), getServerProtocolLevel()) + .register("/random/*", AsyncRandomHandler::new) + .build(); + try { + final InetSocketAddress address2 = secondServer.start(); + + final HttpHost redirectTarget = new HttpHost(scheme.name(), "localhost", address2.getPort()); + + configureServer(bootstrap -> bootstrap + .register("/random/*", AsyncRandomHandler::new) + .setExchangeHandlerDecorator(exchangeHandler -> new RedirectingAsyncDecorator( + exchangeHandler, + requestUri -> { + final String path = requestUri.getPath(); + if (path.equals("/oldlocation")) { + final URI location = new URIBuilder(requestUri) + .setHttpHost(redirectTarget) + .setPath("/random/100") + .build(); + return new Redirect(HttpStatus.SC_MOVED_TEMPORARILY, location.toString()); + } + return null; + }))); + final HttpHost target = startServer(); + + final TestAsyncClient client = startClient(); + + final HttpClientContext context = HttpClientContext.create(); + final Future future = client.execute( + SimpleRequestBuilder.get() + .setHttpHost(target) + .setPath("/oldlocation") + .setHeader(headerName, "custom header") + .build(), context, null); + final HttpResponse response = future.get(); + Assertions.assertNotNull(response); + + Assertions.assertEquals(HttpStatus.SC_MOVED_TEMPORARILY, response.getCode()); + + final RedirectLocations redirects = context.getRedirectLocations(); + Assertions.assertNotNull(redirects); + Assertions.assertEquals(0, redirects.size()); + } finally { + secondServer.shutdown(TimeValue.ofSeconds(5)); + } + } + } 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 2ba0926bb6..8b67437cad 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 @@ -31,6 +31,7 @@ import java.io.IOException; import java.io.InterruptedIOException; import java.net.ConnectException; +import java.net.InetSocketAddress; import java.net.NoRouteToHostException; import java.net.URI; import java.net.UnknownHostException; @@ -58,6 +59,8 @@ import org.apache.hc.client5.testing.classic.RedirectingDecorator; import org.apache.hc.client5.testing.extension.sync.ClientProtocolLevel; import org.apache.hc.client5.testing.extension.sync.TestClient; +import org.apache.hc.client5.testing.extension.sync.TestServer; +import org.apache.hc.client5.testing.extension.sync.TestServerBootstrap; import org.apache.hc.client5.testing.redirect.Redirect; import org.apache.hc.core5.http.ClassicHttpRequest; import org.apache.hc.core5.http.ClassicHttpResponse; @@ -73,13 +76,17 @@ import org.apache.hc.core5.http.io.HttpRequestHandler; import org.apache.hc.core5.http.io.entity.EntityUtils; import org.apache.hc.core5.http.io.entity.StringEntity; +import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; import org.apache.hc.core5.http.message.BasicHeader; import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.io.CloseMode; import org.apache.hc.core5.net.URIBuilder; import org.apache.hc.core5.util.TimeValue; import org.hamcrest.CoreMatchers; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; /** * Redirection test cases. @@ -704,4 +711,54 @@ public void handle(final ClassicHttpRequest request, reqWrapper.getUri()); } + @ParameterizedTest(name = "{displayName}; manually added header: {0}") + @ValueSource(strings = {HttpHeaders.AUTHORIZATION, HttpHeaders.COOKIE}) + void testCrossSiteRedirectWithSensitiveHeaders(final String headerName) throws Exception { + 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_MOVED_TEMPORARILY, response.getCode()); + EntityUtils.consume(response.getEntity()); + return null; + }); + final RedirectLocations redirects = context.getRedirectLocations(); + Assertions.assertNotNull(redirects); + Assertions.assertEquals(0, redirects.size()); + } finally { + secondServer.shutdown(CloseMode.GRACEFUL); + } + } + } 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 895ec73979..dbfaed4c44 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 @@ -29,6 +29,7 @@ import java.net.URI; import java.net.URISyntaxException; +import java.util.Iterator; import java.util.Locale; import org.apache.hc.client5.http.protocol.RedirectStrategy; @@ -38,6 +39,7 @@ import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpException; 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; @@ -59,6 +61,25 @@ public class DefaultRedirectStrategy implements RedirectStrategy { */ public static final DefaultRedirectStrategy INSTANCE = new DefaultRedirectStrategy(); + @Override + public boolean isRedirectAllowed( + final HttpHost currentTarget, + final HttpHost newTarget, + final HttpRequest redirect, + final HttpContext context) { + if (!currentTarget.equals(newTarget)) { + for (final Iterator
it = redirect.headerIterator(); it.hasNext(); ) { + final Header header = it.next(); + if (header.isSensitive() + || header.getName().equalsIgnoreCase(HttpHeaders.AUTHORIZATION) + || header.getName().equalsIgnoreCase(HttpHeaders.COOKIE)) { + return false; + } + } + } + return true; + } + @Override public boolean isRedirected( final HttpRequest request, diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java index 5d44d68cfb..c65b7bba76 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java @@ -54,6 +54,7 @@ import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; import org.apache.hc.core5.http.ProtocolException; +import org.apache.hc.core5.http.message.BasicHttpRequest; import org.apache.hc.core5.http.nio.AsyncDataConsumer; import org.apache.hc.core5.http.nio.AsyncEntityProducer; import org.apache.hc.core5.http.support.BasicRequestBuilder; @@ -133,7 +134,6 @@ public AsyncDataConsumer handleResponse( throw new CircularRedirectException("Circular redirect to '" + redirectUri + "'"); } } - state.redirectLocations.add(redirectUri); final AsyncExecChain.Scope currentScope = state.currentScope; final HttpHost newTarget = URIUtils.extractHost(redirectUri); @@ -165,16 +165,30 @@ public AsyncDataConsumer handleResponse( redirectBuilder = BasicRequestBuilder.copy(currentScope.originalRequest); } redirectBuilder.setUri(redirectUri); + final BasicHttpRequest redirect = redirectBuilder.build(); + + final HttpRoute currentRoute = currentScope.route; + final HttpHost currentHost = currentRoute.getTargetHost(); + + if (!redirectStrategy.isRedirectAllowed(currentHost, newTarget, redirect, clientContext)) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} cannot redirect due to safety restrictions", exchangeId); + } + return asyncExecCallback.handleResponse(response, entityDetails); + } + + state.redirectLocations.add(redirectUri); + state.reroute = false; state.redirectURI = redirectUri; - state.currentRequest = redirectBuilder.build(); + state.currentRequest = redirect; - HttpRoute currentRoute = currentScope.route; - if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) { - final HttpRoute newRoute = routePlanner.determineRoute(newTarget, clientContext); + final HttpRoute newRoute; + if (!Objects.equals(currentHost, newTarget)) { + newRoute = routePlanner.determineRoute(newTarget, clientContext); if (!Objects.equals(currentRoute, newRoute)) { state.reroute = true; - final AuthExchange targetAuthExchange = clientContext.getAuthExchange(currentRoute.getTargetHost()); + final AuthExchange targetAuthExchange = clientContext.getAuthExchange(currentHost); if (LOG.isDebugEnabled()) { LOG.debug("{} resetting target auth state", exchangeId); } @@ -188,20 +202,21 @@ public AsyncDataConsumer handleResponse( proxyAuthExchange.reset(); } } - currentRoute = newRoute; } + } else { + newRoute = currentRoute; } state.currentScope = new AsyncExecChain.Scope( scope.exchangeId, - currentRoute, - redirectBuilder.build(), + newRoute, + BasicRequestBuilder.copy(redirect).build(), scope.cancellableDependency, scope.clientContext, scope.execRuntime, scope.scheduler, scope.execCount); if (LOG.isDebugEnabled()) { - LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute); + LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, newRoute); } return null; } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java index e3d6547695..892c374228 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java @@ -139,7 +139,6 @@ public ClassicHttpResponse execute( throw new CircularRedirectException("Circular redirect to '" + redirectUri + "'"); } } - redirectLocations.add(redirectUri); final int statusCode = response.getCode(); final ClassicRequestBuilder redirectBuilder; @@ -163,21 +162,35 @@ public ClassicHttpResponse execute( redirectBuilder = ClassicRequestBuilder.copy(currentScope.originalRequest); } redirectBuilder.setUri(redirectUri); + final ClassicHttpRequest redirect = redirectBuilder.build(); + + final HttpRoute currentRoute = currentScope.route; + final HttpHost currentHost = currentRoute.getTargetHost(); + + if (!redirectStrategy.isRedirectAllowed(currentHost, newTarget, redirect, context)) { + if (LOG.isDebugEnabled()) { + LOG.debug("{} cannot redirect due to safety restrictions", exchangeId); + } + return response; + } + + redirectLocations.add(redirectUri); - HttpRoute currentRoute = currentScope.route; - if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) { - final HttpRoute newRoute = this.routePlanner.determineRoute(newTarget, context); + final HttpRoute newRoute; + if (!Objects.equals(currentHost, newTarget)) { + newRoute = this.routePlanner.determineRoute(newTarget, context); if (!Objects.equals(currentRoute, newRoute)) { if (LOG.isDebugEnabled()) { LOG.debug("{} new route required", exchangeId); } - final AuthExchange targetAuthExchange = context.getAuthExchange(currentRoute.getTargetHost()); + final AuthExchange targetAuthExchange = context.getAuthExchange(currentHost); if (LOG.isDebugEnabled()) { LOG.debug("{} resetting target auth state", exchangeId); } targetAuthExchange.reset(); - if (currentRoute.getProxyHost() != null) { - final AuthExchange proxyAuthExchange = context.getAuthExchange(currentRoute.getProxyHost()); + final HttpHost proxyHost = currentRoute.getProxyHost(); + if (proxyHost != null) { + final AuthExchange proxyAuthExchange = context.getAuthExchange(proxyHost); if (proxyAuthExchange.isConnectionBased()) { if (LOG.isDebugEnabled()) { LOG.debug("{} resetting proxy auth state", exchangeId); @@ -185,20 +198,21 @@ public ClassicHttpResponse execute( proxyAuthExchange.reset(); } } - currentRoute = newRoute; } + } else { + newRoute = currentRoute; } if (LOG.isDebugEnabled()) { - LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute); + LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, newRoute); } currentScope = new ExecChain.Scope( scope.exchangeId, - currentRoute, - redirectBuilder.build(), + newRoute, + ClassicRequestBuilder.copy(redirect).build(), scope.execRuntime, scope.clientContext); - currentRequest = redirectBuilder.build(); + currentRequest = redirect; RequestEntityProxy.enhance(currentRequest); EntityUtils.consume(response.getEntity()); diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectStrategy.java index 5fade49600..432dad5fc3 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectStrategy.java @@ -32,6 +32,7 @@ import org.apache.hc.core5.annotation.Contract; import org.apache.hc.core5.annotation.ThreadingBehavior; import org.apache.hc.core5.http.HttpException; +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.protocol.HttpContext; @@ -71,4 +72,22 @@ URI getLocationURI( final HttpResponse response, final HttpContext context) throws HttpException; + /** + * Determines if the given redirect should be executed or the redirect response + * should be returned to the caller without further processing. + *

+ * It is legal for this method implementation to modify the redirect request + * in order to make it suitable for redirect execution. + *

+ * + * @since 5.4 + */ + default boolean isRedirectAllowed( + HttpHost currentTarget, + HttpHost newTarget, + HttpRequest redirect, + HttpContext context) { + return true; + } + } 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 79f27f527f..406cb12772 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 @@ -33,10 +33,12 @@ import org.apache.hc.client5.http.protocol.HttpClientContext; import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.ProtocolException; import org.apache.hc.core5.http.message.BasicHttpResponse; +import org.apache.hc.core5.http.support.BasicRequestBuilder; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -249,4 +251,77 @@ void testUseAbsoluteLocation() throws Exception { Assertions.assertEquals(URI.create("http://localhost/foo;bar=baz"), locationURI); } + @Test + void testRedirectAllowed() throws Exception { + final DefaultRedirectStrategy redirectStrategy = new DefaultRedirectStrategy(); + + 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), + BasicRequestBuilder.get("/") + .build(), + null)); + + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 1234), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.AUTHORIZATION, "let me pass") + .build(), + null)); + + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 1234), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.COOKIE, "stuff=blah") + .build(), + null)); + + Assertions.assertTrue(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("someotherhost", 1234), + BasicRequestBuilder.get("/") + .build(), + null)); + + Assertions.assertFalse(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("someotherhost", 1234), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.AUTHORIZATION, "let me pass") + .build(), + null)); + + Assertions.assertFalse(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("someotherhost", 1234), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.COOKIE, "stuff=blah") + .build(), + null)); + + Assertions.assertFalse(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 80), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.AUTHORIZATION, "let me pass") + .build(), + null)); + + Assertions.assertFalse(redirectStrategy.isRedirectAllowed( + new HttpHost("somehost", 1234), + new HttpHost("somehost", 80), + BasicRequestBuilder.get("/") + .addHeader(HttpHeaders.COOKIE, "stuff=blah") + .build(), + null)); + } + }