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 b1c70c257..464252173 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 @@ -50,6 +50,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; @@ -111,11 +112,19 @@ 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 = GET.equals(originalMethod) || + OPTIONS.equals(originalMethod) || HEAD.equals(originalMethod); + boolean strict302 = statusCode == FOUND_302 && config.isStrict302Handling(); + 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..a70f9c39a 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 = "QUERY"; 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 461c7a06a..57f48b93c 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java @@ -19,17 +19,22 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.io.IOUtils; +import org.asynchttpclient.request.body.generator.ByteArrayBodyGenerator; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.handler.AbstractHandler; import org.junit.jupiter.api.BeforeEach; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.concurrent.TimeUnit; import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE; 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.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; @@ -37,11 +42,13 @@ public class RedirectBodyTest extends AbstractBasicTest { private static volatile boolean redirectAlreadyPerformed; private static volatile String receivedContentType; + private static volatile String receivedMethod; @BeforeEach public void setUp() { redirectAlreadyPerformed = false; receivedContentType = null; + receivedMethod = null; } @Override @@ -59,6 +66,7 @@ public void handle(String pathInContext, Request request, HttpServletRequest htt } else { receivedContentType = request.getContentType(); + receivedMethod = request.getMethod(); httpResponse.setStatus(200); int len = request.getContentLength(); httpResponse.setContentLength(len); @@ -82,6 +90,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); } } @@ -94,6 +103,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); } } @@ -106,10 +116,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))) { @@ -118,7 +142,101 @@ 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(StandardCharsets.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); + } + } + + 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); + } + } } diff --git a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java index daa9676d4..fbf93d536 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; @@ -64,6 +65,11 @@ public class RedirectCredentialSecurityTest { private static final AtomicReference bodyOn307Target = 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<>(); @@ -186,6 +192,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")); @@ -508,6 +532,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.