Skip to content

Disallow automatic request redirection if the original request contains potentially sensitive headers #624

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ok2c merged 1 commit into from
Mar 17, 2025
Merged

Disallow automatic request redirection if the original request contains potentially sensitive headers #624

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions ...ing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncRedirectsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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 {

Expand Down Expand Up @@ -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<SimpleHttpResponse> 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));
}
}

}
57 changes: 57 additions & 0 deletions httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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.
Expand Down Expand Up @@ -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);
}
}

}
21 changes: 21 additions & 0 deletions httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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<Header> 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,
Expand Down
35 changes: 25 additions & 10 deletions httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}
Expand All @@ -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;
}
Expand Down
38 changes: 26 additions & 12 deletions httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -163,42 +162,57 @@ 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);
}
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());
Expand Down
19 changes: 19 additions & 0 deletions httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
* <p>
* It is legal for this method implementation to modify the redirect request
* in order to make it suitable for redirect execution.
* </p>
*
* @since 5.4
*/
default boolean isRedirectAllowed(
HttpHost currentTarget,
HttpHost newTarget,
HttpRequest redirect,
HttpContext context) {
return true;
}

}
Loading