From e2cff33193d77b16c72c613a696fe917baf691c3 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Sun, 19 Nov 2023 11:14:34 +0100 Subject: [PATCH] SEE OTHER redirect handling fix --- .../http/impl/async/AsyncRedirectExec.java | 9 ++-- .../http/impl/classic/RedirectExec.java | 8 +-- .../http/impl/classic/TestRedirectExec.java | 52 +++++++++++++++++++ 3 files changed, 63 insertions(+), 6 deletions(-) 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 b7c1712db..225ff1efb 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 @@ -90,6 +90,7 @@ private static class State { volatile URI redirectURI; volatile int maxRedirects; volatile int redirectCount; + volatile HttpRequest originalRequest; volatile HttpRequest currentRequest; volatile AsyncEntityProducer currentEntityProducer; volatile RedirectLocations redirectLocations; @@ -150,7 +151,7 @@ public AsyncDataConsumer handleResponse( redirectBuilder = BasicRequestBuilder.get(); state.currentEntityProducer = null; } else { - redirectBuilder = BasicRequestBuilder.copy(scope.originalRequest); + redirectBuilder = BasicRequestBuilder.copy(state.originalRequest); } break; case HttpStatus.SC_SEE_OTHER: @@ -158,15 +159,16 @@ public AsyncDataConsumer handleResponse( redirectBuilder = BasicRequestBuilder.get(); state.currentEntityProducer = null; } else { - redirectBuilder = BasicRequestBuilder.copy(scope.originalRequest); + redirectBuilder = BasicRequestBuilder.copy(state.originalRequest); } break; default: - redirectBuilder = BasicRequestBuilder.copy(scope.originalRequest); + redirectBuilder = BasicRequestBuilder.copy(state.originalRequest); } redirectBuilder.setUri(redirectUri); state.reroute = false; state.redirectURI = redirectUri; + state.originalRequest = redirectBuilder.build(); state.currentRequest = redirectBuilder.build(); if (!Objects.equals(currentRoute.getTargetHost(), newTarget)) { @@ -270,6 +272,7 @@ public void execute( final State state = new State(); state.maxRedirects = config.getMaxRedirects() > 0 ? config.getMaxRedirects() : 50; state.redirectCount = 0; + state.originalRequest = scope.originalRequest; state.currentRequest = request; state.currentEntityProducer = entityProducer; state.redirectLocations = redirectLocations; 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 cc622fe38..827c0c7e1 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 @@ -108,6 +108,7 @@ public ClassicHttpResponse execute( final RequestConfig config = context.getRequestConfig(); final int maxRedirects = config.getMaxRedirects() > 0 ? config.getMaxRedirects() : 50; + ClassicHttpRequest originalRequest = scope.originalRequest; ClassicHttpRequest currentRequest = request; ExecChain.Scope currentScope = scope; for (int redirectCount = 0;;) { @@ -153,18 +154,18 @@ public ClassicHttpResponse execute( if (Method.POST.isSame(request.getMethod())) { redirectBuilder = ClassicRequestBuilder.get(); } else { - redirectBuilder = ClassicRequestBuilder.copy(scope.originalRequest); + redirectBuilder = ClassicRequestBuilder.copy(originalRequest); } break; case HttpStatus.SC_SEE_OTHER: if (!Method.GET.isSame(request.getMethod()) && !Method.HEAD.isSame(request.getMethod())) { redirectBuilder = ClassicRequestBuilder.get(); } else { - redirectBuilder = ClassicRequestBuilder.copy(scope.originalRequest); + redirectBuilder = ClassicRequestBuilder.copy(originalRequest); } break; default: - redirectBuilder = ClassicRequestBuilder.copy(scope.originalRequest); + redirectBuilder = ClassicRequestBuilder.copy(originalRequest); } redirectBuilder.setUri(redirectUri); @@ -201,6 +202,7 @@ public ClassicHttpResponse execute( if (LOG.isDebugEnabled()) { LOG.debug("{} redirecting to '{}' via {}", exchangeId, redirectUri, currentRoute); } + originalRequest = redirectBuilder.build(); currentRequest = redirectBuilder.build(); RequestEntityProxy.enhance(currentRequest); diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java index a59ca241f..5095497c3 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestRedirectExec.java @@ -56,6 +56,8 @@ import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.ProtocolException; +import org.apache.hc.core5.http.io.support.ClassicRequestBuilder; +import org.apache.hc.core5.http.io.support.ClassicResponseBuilder; import org.apache.hc.core5.http.message.BasicClassicHttpResponse; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -357,6 +359,56 @@ public void testRedirectProtocolException() throws Exception { Mockito.verify(response1).close(); } + @Test + public void testPutSeeOtherRedirect() throws Exception { + final HttpRoute route = new HttpRoute(target); + final URI targetUri = new URI("http://localhost:80/stuff"); + final ClassicHttpRequest request = ClassicRequestBuilder.put() + .setUri(targetUri) + .setEntity("stuff") + .build(); + final HttpClientContext context = HttpClientContext.create(); + + final URI redirect1 = new URI("http://localhost:80/see-something-else"); + final ClassicHttpResponse response1 = ClassicResponseBuilder.create(HttpStatus.SC_SEE_OTHER) + .addHeader(HttpHeaders.LOCATION, redirect1.toASCIIString()) + .build(); + final URI redirect2 = new URI("http://localhost:80/other-stuff"); + final ClassicHttpResponse response2 = ClassicResponseBuilder.create(HttpStatus.SC_MOVED_PERMANENTLY) + .addHeader(HttpHeaders.LOCATION, redirect2.toASCIIString()) + .build(); + final ClassicHttpResponse response3 = ClassicResponseBuilder.create(HttpStatus.SC_OK) + .build(); + + Mockito.when(chain.proceed( + HttpRequestMatcher.matchesRequestUri(targetUri), + ArgumentMatchers.any())).thenReturn(response1); + Mockito.when(chain.proceed( + HttpRequestMatcher.matchesRequestUri(redirect1), + ArgumentMatchers.any())).thenReturn(response2); + Mockito.when(chain.proceed( + HttpRequestMatcher.matchesRequestUri(redirect2), + ArgumentMatchers.any())).thenReturn(response3); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, endpoint, context); + final ClassicHttpResponse finalResponse = redirectExec.execute(request, scope, chain); + Assertions.assertEquals(200, finalResponse.getCode()); + + final ArgumentCaptor reqCaptor = ArgumentCaptor.forClass(ClassicHttpRequest.class); + Mockito.verify(chain, Mockito.times(3)).proceed(reqCaptor.capture(), ArgumentMatchers.same(scope)); + + final List allValues = reqCaptor.getAllValues(); + Assertions.assertNotNull(allValues); + Assertions.assertEquals(3, allValues.size()); + final ClassicHttpRequest request1 = allValues.get(0); + final ClassicHttpRequest request2 = allValues.get(1); + final ClassicHttpRequest request3 = allValues.get(2); + Assertions.assertSame(request, request1); + Assertions.assertEquals(request1.getMethod(), "PUT"); + Assertions.assertEquals(request2.getMethod(), "GET"); + Assertions.assertEquals(request3.getMethod(), "GET"); + } + private static class HttpRequestMatcher implements ArgumentMatcher { private final URI expectedRequestUri;