Skip to content

Commit 10f7b78

Browse files
ok2cok2c
committed
Disallow automatic request redirection if the original request contains potentially sensitive headers
1 parent 9b03e6e commit 10f7b78

7 files changed

Lines changed: 278 additions & 22 deletions

File tree

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/AbstractHttpAsyncRedirectsTest.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.apache.hc.client5.http.cookie.CookieStore;
4444
import org.apache.hc.client5.http.impl.cookie.BasicClientCookie;
4545
import org.apache.hc.client5.http.protocol.HttpClientContext;
46+
import org.apache.hc.client5.http.protocol.RedirectLocations;
4647
import org.apache.hc.client5.testing.OldPathRedirectResolver;
4748
import org.apache.hc.client5.testing.extension.async.ClientProtocolLevel;
4849
import org.apache.hc.client5.testing.extension.async.ServerProtocolLevel;
@@ -53,6 +54,7 @@
5354
import org.apache.hc.core5.http.ContentType;
5455
import org.apache.hc.core5.http.Header;
5556
import org.apache.hc.core5.http.HttpException;
57+
import org.apache.hc.core5.http.HttpHeaders;
5658
import org.apache.hc.core5.http.HttpHost;
5759
import org.apache.hc.core5.http.HttpRequest;
5860
import org.apache.hc.core5.http.HttpResponse;
@@ -65,6 +67,8 @@
6567
import org.hamcrest.CoreMatchers;
6668
import org.junit.jupiter.api.Assertions;
6769
import org.junit.jupiter.api.Test;
70+
import org.junit.jupiter.params.ParameterizedTest;
71+
import org.junit.jupiter.params.provider.ValueSource;
6872

6973
abstract class AbstractHttpAsyncRedirectsTest extends AbstractIntegrationTestBase {
7074

@@ -624,4 +628,55 @@ void testCrossSiteRedirect() throws Exception {
624628
}
625629
}
626630

631+
@ParameterizedTest(name = "{displayName}; manually added header: {0}")
632+
@ValueSource(strings = {HttpHeaders.AUTHORIZATION, HttpHeaders.COOKIE})
633+
void testCrossSiteRedirectWithSensitiveHeaders(final String headerName) throws Exception {
634+
final URIScheme scheme = scheme();
635+
final TestAsyncServer secondServer = new TestAsyncServerBootstrap(scheme(), getServerProtocolLevel())
636+
.register("/random/*", AsyncRandomHandler::new)
637+
.build();
638+
try {
639+
final InetSocketAddress address2 = secondServer.start();
640+
641+
final HttpHost redirectTarget = new HttpHost(scheme.name(), "localhost", address2.getPort());
642+
643+
configureServer(bootstrap -> bootstrap
644+
.register("/random/*", AsyncRandomHandler::new)
645+
.setExchangeHandlerDecorator(exchangeHandler -> new RedirectingAsyncDecorator(
646+
exchangeHandler,
647+
requestUri -> {
648+
final String path = requestUri.getPath();
649+
if (path.equals("/oldlocation")) {
650+
final URI location = new URIBuilder(requestUri)
651+
.setHttpHost(redirectTarget)
652+
.setPath("/random/100")
653+
.build();
654+
return new Redirect(HttpStatus.SC_MOVED_TEMPORARILY, location.toString());
655+
}
656+
return null;
657+
})));
658+
final HttpHost target = startServer();
659+
660+
final TestAsyncClient client = startClient();
661+
662+
final HttpClientContext context = HttpClientContext.create();
663+
final Future<SimpleHttpResponse> future = client.execute(
664+
SimpleRequestBuilder.get()
665+
.setHttpHost(target)
666+
.setPath("/oldlocation")
667+
.setHeader(headerName, "custom header")
668+
.build(), context, null);
669+
final HttpResponse response = future.get();
670+
Assertions.assertNotNull(response);
671+
672+
Assertions.assertEquals(HttpStatus.SC_MOVED_TEMPORARILY, response.getCode());
673+
674+
final RedirectLocations redirects = context.getRedirectLocations();
675+
Assertions.assertNotNull(redirects);
676+
Assertions.assertEquals(0, redirects.size());
677+
} finally {
678+
secondServer.shutdown(TimeValue.ofSeconds(5));
679+
}
680+
}
681+
627682
}

‎httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestRedirects.java‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.io.IOException;
3232
import java.io.InterruptedIOException;
3333
import java.net.ConnectException;
34+
import java.net.InetSocketAddress;
3435
import java.net.NoRouteToHostException;
3536
import java.net.URI;
3637
import java.net.UnknownHostException;
@@ -58,6 +59,8 @@
5859
import org.apache.hc.client5.testing.classic.RedirectingDecorator;
5960
import org.apache.hc.client5.testing.extension.sync.ClientProtocolLevel;
6061
import org.apache.hc.client5.testing.extension.sync.TestClient;
62+
import org.apache.hc.client5.testing.extension.sync.TestServer;
63+
import org.apache.hc.client5.testing.extension.sync.TestServerBootstrap;
6164
import org.apache.hc.client5.testing.redirect.Redirect;
6265
import org.apache.hc.core5.http.ClassicHttpRequest;
6366
import org.apache.hc.core5.http.ClassicHttpResponse;
@@ -73,13 +76,17 @@
7376
import org.apache.hc.core5.http.io.HttpRequestHandler;
7477
import org.apache.hc.core5.http.io.entity.EntityUtils;
7578
import org.apache.hc.core5.http.io.entity.StringEntity;
79+
import org.apache.hc.core5.http.io.support.ClassicRequestBuilder;
7680
import org.apache.hc.core5.http.message.BasicHeader;
7781
import org.apache.hc.core5.http.protocol.HttpContext;
82+
import org.apache.hc.core5.io.CloseMode;
7883
import org.apache.hc.core5.net.URIBuilder;
7984
import org.apache.hc.core5.util.TimeValue;
8085
import org.hamcrest.CoreMatchers;
8186
import org.junit.jupiter.api.Assertions;
8287
import org.junit.jupiter.api.Test;
88+
import org.junit.jupiter.params.ParameterizedTest;
89+
import org.junit.jupiter.params.provider.ValueSource;
8390

8491
/**
8592
* Redirection test cases.
@@ -704,4 +711,54 @@ public void handle(final ClassicHttpRequest request,
704711
reqWrapper.getUri());
705712
}
706713

714+
@ParameterizedTest(name = "{displayName}; manually added header: {0}")
715+
@ValueSource(strings = {HttpHeaders.AUTHORIZATION, HttpHeaders.COOKIE})
716+
void testCrossSiteRedirectWithSensitiveHeaders(final String headerName) throws Exception {
717+
final URIScheme scheme = scheme();
718+
final TestServer secondServer = new TestServerBootstrap(scheme())
719+
.register("/random/*", new RandomHandler())
720+
.build();
721+
try {
722+
final InetSocketAddress address2 = secondServer.start();
723+
724+
final HttpHost redirectTarget = new HttpHost(scheme.name(), "localhost", address2.getPort());
725+
726+
configureServer(bootstrap -> bootstrap
727+
.setExchangeHandlerDecorator(requestHandler -> new RedirectingDecorator(
728+
requestHandler,
729+
requestUri -> {
730+
final URI location = new URIBuilder(requestUri)
731+
.setHttpHost(redirectTarget)
732+
.setPath("/random/100")
733+
.build();
734+
return new Redirect(HttpStatus.SC_MOVED_TEMPORARILY, location.toString());
735+
}))
736+
.register("/random/*", new RandomHandler())
737+
);
738+
739+
final HttpHost target = startServer();
740+
741+
final TestClient client = client();
742+
final HttpClientContext context = HttpClientContext.create();
743+
744+
client.execute(target,
745+
ClassicRequestBuilder.get()
746+
.setHttpHost(target)
747+
.setPath("/oldlocation")
748+
.setHeader(headerName, "custom header")
749+
.build(),
750+
context,
751+
response -> {
752+
Assertions.assertEquals(HttpStatus.SC_MOVED_TEMPORARILY, response.getCode());
753+
EntityUtils.consume(response.getEntity());
754+
return null;
755+
});
756+
final RedirectLocations redirects = context.getRedirectLocations();
757+
Assertions.assertNotNull(redirects);
758+
Assertions.assertEquals(0, redirects.size());
759+
} finally {
760+
secondServer.shutdown(CloseMode.GRACEFUL);
761+
}
762+
}
763+
707764
}

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/DefaultRedirectStrategy.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929

3030
import java.net.URI;
3131
import java.net.URISyntaxException;
32+
import java.util.Iterator;
3233
import java.util.Locale;
3334

3435
import org.apache.hc.client5.http.protocol.RedirectStrategy;
@@ -38,6 +39,7 @@
3839
import org.apache.hc.core5.http.Header;
3940
import org.apache.hc.core5.http.HttpException;
4041
import org.apache.hc.core5.http.HttpHeaders;
42+
import org.apache.hc.core5.http.HttpHost;
4143
import org.apache.hc.core5.http.HttpRequest;
4244
import org.apache.hc.core5.http.HttpResponse;
4345
import org.apache.hc.core5.http.HttpStatus;
@@ -59,6 +61,25 @@ public class DefaultRedirectStrategy implements RedirectStrategy {
5961
*/
6062
public static final DefaultRedirectStrategy INSTANCE = new DefaultRedirectStrategy();
6163

64+
@Override
65+
public boolean isRedirectAllowed(
66+
final HttpHost currentTarget,
67+
final HttpHost newTarget,
68+
final HttpRequest redirect,
69+
final HttpContext context) {
70+
if (!currentTarget.equals(newTarget)) {
71+
for (final Iterator<Header> it = redirect.headerIterator(); it.hasNext(); ) {
72+
final Header header = it.next();
73+
if (header.isSensitive()
74+
|| header.getName().equalsIgnoreCase(HttpHeaders.AUTHORIZATION)
75+
|| header.getName().equalsIgnoreCase(HttpHeaders.COOKIE)) {
76+
return false;
77+
}
78+
}
79+
}
80+
return true;
81+
}
82+
6283
@Override
6384
public boolean isRedirected(
6485
final HttpRequest request,

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncRedirectExec.java‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@
5454
import org.apache.hc.core5.http.HttpStatus;
5555
import org.apache.hc.core5.http.Method;
5656
import org.apache.hc.core5.http.ProtocolException;
57+
import org.apache.hc.core5.http.message.BasicHttpRequest;
5758
import org.apache.hc.core5.http.nio.AsyncDataConsumer;
5859
import org.apache.hc.core5.http.nio.AsyncEntityProducer;
5960
import org.apache.hc.core5.http.support.BasicRequestBuilder;
@@ -133,7 +134,6 @@ public AsyncDataConsumer handleResponse(
133134
throw new CircularRedirectException("Circular redirect to '" + redirectUri + "'");
134135
}
135136
}
136-
state.redirectLocations.add(redirectUri);
137137

138138
final AsyncExecChain.Scope currentScope = state.currentScope;
139139
final HttpHost newTarget = URIUtils.extractHost(redirectUri);
@@ -165,16 +165,30 @@ public AsyncDataConsumer handleResponse(
165165
redirectBuilder = BasicRequestBuilder.copy(currentScope.originalRequest);
166166
}
167167
redirectBuilder.setUri(redirectUri);
168+
final BasicHttpRequest redirect = redirectBuilder.build();
169+
170+
final HttpRoute currentRoute = currentScope.route;
171+
final HttpHost currentHost = currentRoute.getTargetHost();
172+
173+
if (!redirectStrategy.isRedirectAllowed(currentHost, newTarget, redirect, clientContext)) {
174+
if (LOG.isDebugEnabled()) {
175+
LOG.debug("{} cannot redirect due to safety restrictions", exchangeId);
176+
}
177+
return asyncExecCallback.handleResponse(response, entityDetails);
178+
}
179+
180+
state.redirectLocations.add(redirectUri);
181+
168182
state.reroute = false;
169183
state.redirectURI = redirectUri;
170-
state.currentRequest = redirectBuilder.build();
184+
state.currentRequest = redirect;
171185

172-
HttpRoute currentRoute = currentScope.route;
173-
if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) {
174-
final HttpRoute newRoute = routePlanner.determineRoute(newTarget, clientContext);
186+
final HttpRoute newRoute;
187+
if (!Objects.equals(currentHost, newTarget)) {
188+
newRoute = routePlanner.determineRoute(newTarget, clientContext);
175189
if (!Objects.equals(currentRoute, newRoute)) {
176190
state.reroute = true;
177-
final AuthExchange targetAuthExchange = clientContext.getAuthExchange(currentRoute.getTargetHost());
191+
final AuthExchange targetAuthExchange = clientContext.getAuthExchange(currentHost);
178192
if (LOG.isDebugEnabled()) {
179193
LOG.debug("{} resetting target auth state", exchangeId);
180194
}
@@ -188,20 +202,21 @@ public AsyncDataConsumer handleResponse(
188202
proxyAuthExchange.reset();
189203
}
190204
}
191-
currentRoute = newRoute;
192205
}
206+
} else {
207+
newRoute = currentRoute;
193208
}
194209
state.currentScope = new AsyncExecChain.Scope(
195210
scope.exchangeId,
196-
currentRoute,
197-
redirectBuilder.build(),
211+
newRoute,
212+
BasicRequestBuilder.copy(redirect).build(),
198213
scope.cancellableDependency,
199214
scope.clientContext,
200215
scope.execRuntime,
201216
scope.scheduler,
202217
scope.execCount);
203218
if (LOG.isDebugEnabled()) {
204-
LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute);
219+
LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, newRoute);
205220
}
206221
return null;
207222
}

‎httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/RedirectExec.java‎

Lines changed: 26 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,6 @@ public ClassicHttpResponse execute(
139139
throw new CircularRedirectException("Circular redirect to '" + redirectUri + "'");
140140
}
141141
}
142-
redirectLocations.add(redirectUri);
143142

144143
final int statusCode = response.getCode();
145144
final ClassicRequestBuilder redirectBuilder;
@@ -163,42 +162,57 @@ public ClassicHttpResponse execute(
163162
redirectBuilder = ClassicRequestBuilder.copy(currentScope.originalRequest);
164163
}
165164
redirectBuilder.setUri(redirectUri);
165+
final ClassicHttpRequest redirect = redirectBuilder.build();
166+
167+
final HttpRoute currentRoute = currentScope.route;
168+
final HttpHost currentHost = currentRoute.getTargetHost();
169+
170+
if (!redirectStrategy.isRedirectAllowed(currentHost, newTarget, redirect, context)) {
171+
if (LOG.isDebugEnabled()) {
172+
LOG.debug("{} cannot redirect due to safety restrictions", exchangeId);
173+
}
174+
return response;
175+
}
176+
177+
redirectLocations.add(redirectUri);
166178

167-
HttpRoute currentRoute = currentScope.route;
168-
if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) {
169-
final HttpRoute newRoute = this.routePlanner.determineRoute(newTarget, context);
179+
final HttpRoute newRoute;
180+
if (!Objects.equals(currentHost, newTarget)) {
181+
newRoute = this.routePlanner.determineRoute(newTarget, context);
170182
if (!Objects.equals(currentRoute, newRoute)) {
171183
if (LOG.isDebugEnabled()) {
172184
LOG.debug("{} new route required", exchangeId);
173185
}
174-
final AuthExchange targetAuthExchange = context.getAuthExchange(currentRoute.getTargetHost());
186+
final AuthExchange targetAuthExchange = context.getAuthExchange(currentHost);
175187
if (LOG.isDebugEnabled()) {
176188
LOG.debug("{} resetting target auth state", exchangeId);
177189
}
178190
targetAuthExchange.reset();
179-
if (currentRoute.getProxyHost() != null) {
180-
final AuthExchange proxyAuthExchange = context.getAuthExchange(currentRoute.getProxyHost());
191+
final HttpHost proxyHost = currentRoute.getProxyHost();
192+
if (proxyHost != null) {
193+
final AuthExchange proxyAuthExchange = context.getAuthExchange(proxyHost);
181194
if (proxyAuthExchange.isConnectionBased()) {
182195
if (LOG.isDebugEnabled()) {
183196
LOG.debug("{} resetting proxy auth state", exchangeId);
184197
}
185198
proxyAuthExchange.reset();
186199
}
187200
}
188-
currentRoute = newRoute;
189201
}
202+
} else {
203+
newRoute = currentRoute;
190204
}
191205

192206
if (LOG.isDebugEnabled()) {
193-
LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute);
207+
LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, newRoute);
194208
}
195209
currentScope = new ExecChain.Scope(
196210
scope.exchangeId,
197-
currentRoute,
198-
redirectBuilder.build(),
211+
newRoute,
212+
ClassicRequestBuilder.copy(redirect).build(),
199213
scope.execRuntime,
200214
scope.clientContext);
201-
currentRequest = redirectBuilder.build();
215+
currentRequest = redirect;
202216
RequestEntityProxy.enhance(currentRequest);
203217

204218
EntityUtils.consume(response.getEntity());

‎httpclient5/src/main/java/org/apache/hc/client5/http/protocol/RedirectStrategy.java‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
import org.apache.hc.core5.annotation.Contract;
3333
import org.apache.hc.core5.annotation.ThreadingBehavior;
3434
import org.apache.hc.core5.http.HttpException;
35+
import org.apache.hc.core5.http.HttpHost;
3536
import org.apache.hc.core5.http.HttpRequest;
3637
import org.apache.hc.core5.http.HttpResponse;
3738
import org.apache.hc.core5.http.protocol.HttpContext;
@@ -71,4 +72,22 @@ URI getLocationURI(
7172
final HttpResponse response,
7273
final HttpContext context) throws HttpException;
7374

75+
/**
76+
* Determines if the given redirect should be executed or the redirect response
77+
* should be returned to the caller without further processing.
78+
* <p>
79+
* It is legal for this method implementation to modify the redirect request
80+
* in order to make it suitable for redirect execution.
81+
* </p>
82+
*
83+
* @since 5.4
84+
*/
85+
default boolean isRedirectAllowed(
86+
HttpHost currentTarget,
87+
HttpHost newTarget,
88+
HttpRequest redirect,
89+
HttpContext context) {
90+
return true;
91+
}
92+
7493
}

0 commit comments

Comments
 (0)