Skip to content

Commit 83d603c

Browse files
ok2cok2c
committed
SEE OTHER redirect handling fix
1 parent 3486b47 commit 83d603c

3 files changed

Lines changed: 63 additions & 6 deletions

File tree

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ private static class State {
9090
volatile URI redirectURI;
9191
volatile int maxRedirects;
9292
volatile int redirectCount;
93+
volatile HttpRequest originalRequest;
9394
volatile HttpRequest currentRequest;
9495
volatile AsyncEntityProducer currentEntityProducer;
9596
volatile RedirectLocations redirectLocations;
@@ -150,23 +151,24 @@ public AsyncDataConsumer handleResponse(
150151
redirectBuilder = BasicRequestBuilder.get();
151152
state.currentEntityProducer = null;
152153
} else {
153-
redirectBuilder = BasicRequestBuilder.copy(scope.originalRequest);
154+
redirectBuilder = BasicRequestBuilder.copy(state.originalRequest);
154155
}
155156
break;
156157
case HttpStatus.SC_SEE_OTHER:
157158
if (!Method.GET.isSame(request.getMethod()) && !Method.HEAD.isSame(request.getMethod())) {
158159
redirectBuilder = BasicRequestBuilder.get();
159160
state.currentEntityProducer = null;
160161
} else {
161-
redirectBuilder = BasicRequestBuilder.copy(scope.originalRequest);
162+
redirectBuilder = BasicRequestBuilder.copy(state.originalRequest);
162163
}
163164
break;
164165
default:
165-
redirectBuilder = BasicRequestBuilder.copy(scope.originalRequest);
166+
redirectBuilder = BasicRequestBuilder.copy(state.originalRequest);
166167
}
167168
redirectBuilder.setUri(redirectUri);
168169
state.reroute = false;
169170
state.redirectURI = redirectUri;
171+
state.originalRequest = redirectBuilder.build();
170172
state.currentRequest = redirectBuilder.build();
171173

172174
if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) {
@@ -270,6 +272,7 @@ public void execute(
270272
final State state = new State();
271273
state.maxRedirects = config.getMaxRedirects() > 0 ? config.getMaxRedirects() : 50;
272274
state.redirectCount = 0;
275+
state.originalRequest = scope.originalRequest;
273276
state.currentRequest = request;
274277
state.currentEntityProducer = entityProducer;
275278
state.redirectLocations = redirectLocations;

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

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ public ClassicHttpResponse execute(
108108

109109
final RequestConfig config = context.getRequestConfig();
110110
final int maxRedirects = config.getMaxRedirects() > 0 ? config.getMaxRedirects() : 50;
111+
ClassicHttpRequest originalRequest = scope.originalRequest;
111112
ClassicHttpRequest currentRequest = request;
112113
ExecChain.Scope currentScope = scope;
113114
for (int redirectCount = 0;;) {
@@ -153,18 +154,18 @@ public ClassicHttpResponse execute(
153154
if (Method.POST.isSame(request.getMethod())) {
154155
redirectBuilder = ClassicRequestBuilder.get();
155156
} else {
156-
redirectBuilder = ClassicRequestBuilder.copy(scope.originalRequest);
157+
redirectBuilder = ClassicRequestBuilder.copy(originalRequest);
157158
}
158159
break;
159160
case HttpStatus.SC_SEE_OTHER:
160161
if (!Method.GET.isSame(request.getMethod()) && !Method.HEAD.isSame(request.getMethod())) {
161162
redirectBuilder = ClassicRequestBuilder.get();
162163
} else {
163-
redirectBuilder = ClassicRequestBuilder.copy(scope.originalRequest);
164+
redirectBuilder = ClassicRequestBuilder.copy(originalRequest);
164165
}
165166
break;
166167
default:
167-
redirectBuilder = ClassicRequestBuilder.copy(scope.originalRequest);
168+
redirectBuilder = ClassicRequestBuilder.copy(originalRequest);
168169
}
169170
redirectBuilder.setUri(redirectUri);
170171

@@ -201,6 +202,7 @@ public ClassicHttpResponse execute(
201202
if (LOG.isDebugEnabled()) {
202203
LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute);
203204
}
205+
originalRequest = redirectBuilder.build();
204206
currentRequest = redirectBuilder.build();
205207
RequestEntityProxy.enhance(currentRequest);
206208

‎httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@
5656
import org.apache.hc.core5.http.HttpHost;
5757
import org.apache.hc.core5.http.HttpStatus;
5858
import org.apache.hc.core5.http.ProtocolException;
59+
import org.apache.hc.core5.http.io.support.ClassicRequestBuilder;
60+
import org.apache.hc.core5.http.io.support.ClassicResponseBuilder;
5961
import org.apache.hc.core5.http.message.BasicClassicHttpResponse;
6062
import org.junit.jupiter.api.Assertions;
6163
import org.junit.jupiter.api.BeforeEach;
@@ -357,6 +359,56 @@ public void testRedirectProtocolException() throws Exception {
357359
Mockito.verify(response1).close();
358360
}
359361

362+
@Test
363+
public void testPutSeeOtherRedirect() throws Exception {
364+
final HttpRoute route = new HttpRoute(target);
365+
final URI targetUri = new URI("http://localhost:80/stuff");
366+
final ClassicHttpRequest request = ClassicRequestBuilder.put()
367+
.setUri(targetUri)
368+
.setEntity("stuff")
369+
.build();
370+
final HttpClientContext context = HttpClientContext.create();
371+
372+
final URI redirect1 = new URI("http://localhost:80/see-something-else");
373+
final ClassicHttpResponse response1 = ClassicResponseBuilder.create(HttpStatus.SC_SEE_OTHER)
374+
.addHeader(HttpHeaders.LOCATION, redirect1.toASCIIString())
375+
.build();
376+
final URI redirect2 = new URI("http://localhost:80/other-stuff");
377+
final ClassicHttpResponse response2 = ClassicResponseBuilder.create(HttpStatus.SC_MOVED_PERMANENTLY)
378+
.addHeader(HttpHeaders.LOCATION, redirect2.toASCIIString())
379+
.build();
380+
final ClassicHttpResponse response3 = ClassicResponseBuilder.create(HttpStatus.SC_OK)
381+
.build();
382+
383+
Mockito.when(chain.proceed(
384+
HttpRequestMatcher.matchesRequestUri(targetUri),
385+
ArgumentMatchers.any())).thenReturn(response1);
386+
Mockito.when(chain.proceed(
387+
HttpRequestMatcher.matchesRequestUri(redirect1),
388+
ArgumentMatchers.any())).thenReturn(response2);
389+
Mockito.when(chain.proceed(
390+
HttpRequestMatcher.matchesRequestUri(redirect2),
391+
ArgumentMatchers.any())).thenReturn(response3);
392+
393+
final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context);
394+
final ClassicHttpResponse finalResponse = redirectExec.execute(request, scope, chain);
395+
Assertions.assertEquals(200, finalResponse.getCode());
396+
397+
final ArgumentCaptor<ClassicHttpRequest> reqCaptor = ArgumentCaptor.forClass(ClassicHttpRequest.class);
398+
Mockito.verify(chain, Mockito.times(3)).proceed(reqCaptor.capture(), ArgumentMatchers.same(scope));
399+
400+
final List<ClassicHttpRequest> allValues = reqCaptor.getAllValues();
401+
Assertions.assertNotNull(allValues);
402+
Assertions.assertEquals(3, allValues.size());
403+
final ClassicHttpRequest request1 = allValues.get(0);
404+
final ClassicHttpRequest request2 = allValues.get(1);
405+
final ClassicHttpRequest request3 = allValues.get(2);
406+
Assertions.assertSame(request, request1);
407+
Assertions.assertEquals(request1.getMethod(), "PUT");
408+
Assertions.assertEquals(request2.getMethod(), "GET");
409+
Assertions.assertEquals(request3.getMethod(), "GET");
410+
}
411+
360412
private static class HttpRequestMatcher implements ArgumentMatcher<ClassicHttpRequest> {
361413

362414
private final URI expectedRequestUri;

0 commit comments

Comments
 (0)