diff --git a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java index 1b18cd974..1450f2361 100644 --- a/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java +++ b/client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java @@ -56,6 +56,7 @@ import static org.asynchttpclient.util.HttpConstants.Methods.GET; import static org.asynchttpclient.util.HttpConstants.Methods.HEAD; import static org.asynchttpclient.util.HttpConstants.Methods.OPTIONS; +import static org.asynchttpclient.util.HttpConstants.Methods.QUERY; import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.FOUND_302; import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.MOVED_PERMANENTLY_301; import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.PERMANENT_REDIRECT_308; @@ -116,11 +117,20 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture future.setScramContext(null); String originalMethod = request.getMethod(); - boolean switchToGet = !originalMethod.equals(GET) && - !originalMethod.equals(OPTIONS) && - !originalMethod.equals(HEAD) && - (statusCode == MOVED_PERMANENTLY_301 || statusCode == SEE_OTHER_303 || statusCode == FOUND_302 && !config.isStrict302Handling()); - boolean keepBody = statusCode == TEMPORARY_REDIRECT_307 || statusCode == PERMANENT_REDIRECT_308 || statusCode == FOUND_302 && config.isStrict302Handling(); + boolean isQuery = QUERY.equals(originalMethod); + boolean methodAlreadyPreserved = originalMethod.equals(GET) || + originalMethod.equals(OPTIONS) || originalMethod.equals(HEAD); + boolean strict302 = statusCode == FOUND_302 && config.isStrict302Handling(); + // RFC 10008 section 2.5 excludes QUERY from the legacy POST-to-GET behavior. + boolean queryRedirect = isQuery && + (statusCode == MOVED_PERMANENTLY_301 || statusCode == FOUND_302); + boolean legacyRedirectToGet = statusCode == MOVED_PERMANENTLY_301 || + (statusCode == FOUND_302 && !strict302); + boolean switchToGet = !methodAlreadyPreserved && + (statusCode == SEE_OTHER_303 || (!isQuery && legacyRedirectToGet)); + boolean keepBody = queryRedirect || + statusCode == TEMPORARY_REDIRECT_307 || statusCode == PERMANENT_REDIRECT_308 || + strict302; HttpHeaders responseHeaders = response.headers(); String location = responseHeaders.get(LOCATION); diff --git a/client/src/main/java/org/asynchttpclient/util/HttpConstants.java b/client/src/main/java/org/asynchttpclient/util/HttpConstants.java index 4a1a12865..38fc140f7 100644 --- a/client/src/main/java/org/asynchttpclient/util/HttpConstants.java +++ b/client/src/main/java/org/asynchttpclient/util/HttpConstants.java @@ -33,6 +33,7 @@ public static final class Methods { public static final String PATCH = HttpMethod.PATCH.name(); public static final String POST = HttpMethod.POST.name(); public static final String PUT = HttpMethod.PUT.name(); + public static final String QUERY = HttpMethod.QUERY.name(); public static final String TRACE = HttpMethod.TRACE.name(); private Methods() { diff --git a/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java b/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java index 1360091e0..6ab7fd3e2 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java @@ -23,12 +23,15 @@ import org.apache.commons.io.IOUtils; import org.asynchttpclient.filter.FilterContext; import org.asynchttpclient.filter.ResponseFilter; +import org.asynchttpclient.request.body.generator.ByteArrayBodyGenerator; import org.asynchttpclient.request.body.generator.InputStreamBodyGenerator; import org.asynchttpclient.request.body.multipart.InputStreamPart; import org.asynchttpclient.request.body.multipart.StringPart; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import java.io.ByteArrayInputStream; import java.io.FilterInputStream; @@ -49,6 +52,9 @@ import static io.netty.handler.codec.http.HttpHeaderNames.LOCATION; import static org.asynchttpclient.Dsl.asyncHttpClient; import static org.asynchttpclient.Dsl.config; +import static org.asynchttpclient.util.HttpConstants.Methods.GET; +import static org.asynchttpclient.util.HttpConstants.Methods.POST; +import static org.asynchttpclient.util.HttpConstants.Methods.QUERY; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; @@ -64,6 +70,7 @@ public class RedirectBodyTest extends AbstractBasicTest { private static final List receivedContentLengths = new CopyOnWriteArrayList<>(); private static volatile boolean redirectAlreadyPerformed; private static volatile String receivedContentType; + private static volatile String receivedMethod; private static volatile Path fileToDeleteBeforeRedirect; @BeforeEach @@ -71,6 +78,7 @@ public void setUp() { receivedContentLengths.clear(); redirectAlreadyPerformed = false; receivedContentType = null; + receivedMethod = null; fileToDeleteBeforeRedirect = null; } @@ -94,6 +102,7 @@ public void handle(String pathInContext, Request request, HttpServletRequest htt } else { receivedContentType = request.getContentType(); + receivedMethod = request.getMethod(); httpResponse.setStatus(200); httpResponse.setContentLength(body.length); if (body.length > 0) { @@ -114,6 +123,7 @@ public void regular301LosesBody() throws Exception { Response response = c.preparePost(getTargetUrl()).setHeader(CONTENT_TYPE, contentType).setBody(body).setHeader("X-REDIRECT", "301").execute().get(TIMEOUT, TimeUnit.SECONDS); assertEquals(response.getResponseBody(), ""); + assertEquals(GET, receivedMethod); assertNull(receivedContentType); } } @@ -126,6 +136,7 @@ public void regular302LosesBody() throws Exception { Response response = c.preparePost(getTargetUrl()).setHeader(CONTENT_TYPE, contentType).setBody(body).setHeader("X-REDIRECT", "302").execute().get(TIMEOUT, TimeUnit.SECONDS); assertEquals(response.getResponseBody(), ""); + assertEquals(GET, receivedMethod); assertNull(receivedContentType); } } @@ -138,10 +149,24 @@ public void regular302StrictKeepsBody() throws Exception { Response response = c.preparePost(getTargetUrl()).setHeader(CONTENT_TYPE, contentType).setBody(body).setHeader("X-REDIRECT", "302").execute().get(TIMEOUT, TimeUnit.SECONDS); assertEquals(response.getResponseBody(), body); + assertEquals(POST, receivedMethod); assertEquals(receivedContentType, contentType); } } + @RepeatedIfExceptionsTest(repeats = 5) + public void regular303SwitchesToGetAndLosesBody() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + String body = "hello there"; + String contentType = "text/plain; charset=UTF-8"; + + Response response = c.preparePost(getTargetUrl()).setHeader(CONTENT_TYPE, contentType).setBody(body).setHeader("X-REDIRECT", "303").execute().get(TIMEOUT, TimeUnit.SECONDS); + assertEquals("", response.getResponseBody()); + assertEquals(GET, receivedMethod); + assertNull(receivedContentType); + } + } + @RepeatedIfExceptionsTest(repeats = 5) public void regular307KeepsBody() throws Exception { try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { @@ -150,10 +175,138 @@ public void regular307KeepsBody() throws Exception { Response response = c.preparePost(getTargetUrl()).setHeader(CONTENT_TYPE, contentType).setBody(body).setHeader("X-REDIRECT", "307").execute().get(TIMEOUT, TimeUnit.SECONDS); assertEquals(response.getResponseBody(), body); + assertEquals(POST, receivedMethod); assertEquals(receivedContentType, contentType); } } + @RepeatedIfExceptionsTest(repeats = 5) + public void regular308KeepsBody() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + String body = "hello there"; + String contentType = "text/plain; charset=UTF-8"; + + Response response = c.preparePost(getTargetUrl()).setHeader(CONTENT_TYPE, contentType).setBody(body).setHeader("X-REDIRECT", "308").execute().get(TIMEOUT, TimeUnit.SECONDS); + assertEquals(body, response.getResponseBody()); + assertEquals(POST, receivedMethod); + assertEquals(contentType, receivedContentType); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void query301KeepsMethodAndBody() throws Exception { + queryRedirectKeepsMethodAndBody(301, false); + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void query302KeepsMethodAndBody() throws Exception { + queryRedirectKeepsMethodAndBody(302, false); + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void query302StrictKeepsMethodAndBody() throws Exception { + queryRedirectKeepsMethodAndBody(302, true); + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void query303SwitchesToGetAndDropsBody() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + String body = "hello there"; + String contentType = "text/plain; charset=UTF-8"; + + Response response = c.prepare(QUERY, getTargetUrl()) + .setHeader(CONTENT_TYPE, contentType) + .setBody(body) + .setHeader("X-REDIRECT", "303") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + assertEquals("", response.getResponseBody()); + assertEquals(GET, receivedMethod); + assertNull(receivedContentType); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void query307KeepsMethodAndBody() throws Exception { + queryRedirectKeepsMethodAndBody(307, false); + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void query308KeepsMethodAndBody() throws Exception { + queryRedirectKeepsMethodAndBody(308, false); + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void query301KeepsRepeatableBodyGenerator() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + byte[] body = "hello there".getBytes(UTF_8); + String contentType = "text/plain; charset=UTF-8"; + + Response response = c.prepare(QUERY, getTargetUrl()) + .setHeader(CONTENT_TYPE, contentType) + .setBody(new ByteArrayBodyGenerator(body)) + .setHeader("X-REDIRECT", "301") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + assertEquals("hello there", response.getResponseBody()); + assertEquals(QUERY, receivedMethod); + assertEquals(contentType, receivedContentType); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void query301WithNonRepeatableBodyGeneratorFailsPromptly() throws Exception { + try (InputStream body = new FilterInputStream(new ByteArrayInputStream(REDIRECT_BODY)) { + @Override + public boolean markSupported() { + return false; + } + + @Override + public synchronized void reset() throws IOException { + throw new IOException("reset not supported"); + } + }; + AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + ExecutionException thrown = assertThrows(ExecutionException.class, + () -> c.prepare(QUERY, getTargetUrl()) + .setBody(new InputStreamBodyGenerator(body)) + .setHeader("X-REDIRECT", "301") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS)); + + IOException cause = assertInstanceOf(IOException.class, thrown.getCause()); + assertEquals("HTTP/1 request body InputStream already consumed and cannot be reset for a retry", + cause.getMessage()); + } + } + + @ParameterizedTest(name = "{0} on {1} keeps the existing GET rewrite") + @CsvSource({ + "PUT, 301", + "PUT, 302", + "PATCH, 301", + "PATCH, 302", + "DELETE, 301", + "DELETE, 302" + }) + public void putPatchAndDelete301And302KeepExistingBehavior(String method, int statusCode) throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + String body = "hello there"; + String contentType = "text/plain; charset=UTF-8"; + + Response response = c.prepare(method, getTargetUrl()) + .setHeader(CONTENT_TYPE, contentType) + .setBody(body) + .setHeader("X-REDIRECT", Integer.toString(statusCode)) + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + assertEquals("", response.getResponseBody()); + assertEquals(GET, receivedMethod); + assertNull(receivedContentType); + } + } + @RepeatedIfExceptionsTest(repeats = 5) public void redirectPreservesPerRequestSettings() throws Exception { Duration readTimeout = Duration.ofSeconds(7); @@ -424,6 +577,25 @@ public void inputStreamMultipart307FailsPromptly() throws Exception { } } + private void queryRedirectKeepsMethodAndBody(int statusCode, boolean strict302Handling) throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config() + .setFollowRedirect(true) + .setStrict302Handling(strict302Handling))) { + String body = "hello there"; + String contentType = "text/plain; charset=UTF-8"; + + Response response = c.prepare(QUERY, getTargetUrl()) + .setHeader(CONTENT_TYPE, contentType) + .setBody(body) + .setHeader("X-REDIRECT", Integer.toString(statusCode)) + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + assertEquals(body, response.getResponseBody()); + assertEquals(QUERY, receivedMethod); + assertEquals(contentType, receivedContentType); + } + } + private static Response execute307(BoundRequestBuilder requestBuilder) throws Exception { return requestBuilder .setHeader(CONTENT_TYPE, CONTENT_TYPE_VALUE) diff --git a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java index 99119925e..faada9961 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java @@ -36,6 +36,7 @@ import java.util.concurrent.atomic.AtomicReference; import static org.asynchttpclient.Dsl.basicAuthRealm; +import static org.asynchttpclient.util.HttpConstants.Methods.QUERY; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -65,6 +66,11 @@ public class RedirectCredentialSecurityTest { private static final AtomicReference cookieOn307Target = new AtomicReference<>(); private static final AtomicReference authOn308Target = new AtomicReference<>(); private static final AtomicReference bodyOn308Target = new AtomicReference<>(); + private static final AtomicReference query301AuthOnTarget = new AtomicReference<>(); + private static final AtomicReference query301CookieOnTarget = new AtomicReference<>(); + private static final AtomicReference query301ContentTypeOnTarget = new AtomicReference<>(); + private static final AtomicReference query301MethodOnTarget = new AtomicReference<>(); + private static final AtomicReference query301BodyOnTarget = new AtomicReference<>(); private static final AtomicReference lastCookieHeaderOnA = new AtomicReference<>(); private static final AtomicReference lastCookieHeaderOnB = new AtomicReference<>(); private static final AtomicReference cookieAtChainStep2 = new AtomicReference<>(); @@ -189,6 +195,24 @@ public static void startServers() throws Exception { exchange.close(); }); + serverA.createContext("/redirect-query-301-to-b", exchange -> { + exchange.getRequestBody().readAllBytes(); + exchange.getResponseHeaders().add("Location", "http://127.0.0.1:" + portB + "/target-query-301"); + exchange.sendResponseHeaders(301, -1); + exchange.close(); + }); + + serverB.createContext("/target-query-301", exchange -> { + query301AuthOnTarget.set(exchange.getRequestHeaders().getFirst("Authorization")); + query301CookieOnTarget.set(exchange.getRequestHeaders().getFirst("Cookie")); + query301ContentTypeOnTarget.set(exchange.getRequestHeaders().getFirst("Content-Type")); + query301MethodOnTarget.set(exchange.getRequestMethod()); + query301BodyOnTarget.set(new String(exchange.getRequestBody().readAllBytes(), StandardCharsets.UTF_8)); + exchange.sendResponseHeaders(200, 0); + exchange.getResponseBody().close(); + exchange.close(); + }); + // Endpoint reused by the HTTPS-to-HTTP downgrade test (target on server B over plain HTTP) serverB.createContext("/target-after-downgrade", exchange -> { authAfterHttpsDowngrade.set(exchange.getRequestHeaders().getFirst("Authorization")); @@ -511,6 +535,36 @@ void redirect308CrossDomainStripsAuthButPreservesBody() throws Exception { } } + @Test + void query301CrossOriginStripsCredentialsAndPreservesRequest() throws Exception { + DefaultAsyncHttpClientConfig config = new DefaultAsyncHttpClientConfig.Builder() + .setFollowRedirect(true) + .build(); + try (DefaultAsyncHttpClient client = new DefaultAsyncHttpClient(config)) { + query301AuthOnTarget.set(null); + query301CookieOnTarget.set(null); + query301ContentTypeOnTarget.set(null); + query301MethodOnTarget.set(null); + query301BodyOnTarget.set(null); + + client.prepare(QUERY, "http://127.0.0.1:" + portA + "/redirect-query-301-to-b") + .setHeader("Authorization", "Bearer secret-token") + .setHeader("Cookie", "session=secret-session") + .setHeader("Content-Type", "application/query") + .setBody("sensitive-query") + .execute() + .get(5, TimeUnit.SECONDS); + + assertNull(query301AuthOnTarget.get(), + "Authorization must be stripped on a cross-origin QUERY redirect"); + assertNull(query301CookieOnTarget.get(), + "Cookie must be stripped on a cross-origin QUERY redirect"); + assertEquals(QUERY, query301MethodOnTarget.get()); + assertEquals("application/query", query301ContentTypeOnTarget.get()); + assertEquals("sensitive-query", query301BodyOnTarget.get()); + } + } + /** * Cross-domain redirect (different port) must strip a user-supplied Cookie header. * Regression test for GHSA-fmxf-pm6p-7xgm.