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 1450f2361..b6819efc6 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 @@ -43,6 +43,7 @@ import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.util.HashSet; import java.util.Set; @@ -56,7 +57,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.Methods.POST; 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; @@ -117,20 +118,17 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture future.setScramContext(null); String originalMethod = request.getMethod(); - boolean isQuery = QUERY.equals(originalMethod); + boolean isPost = originalMethod.equals(POST); 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); + // RFC 9110 limits the historical 301/302 POST-to-GET rewrite to POST. + // This also preserves QUERY as required by RFC 10008 section 2.5. + boolean legacyPostToGet = isPost && (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; + (statusCode == SEE_OTHER_303 || legacyPostToGet); + boolean keepBody = statusCode != SEE_OTHER_303 && !switchToGet; HttpHeaders responseHeaders = response.headers(); String location = responseHeaders.get(LOCATION); @@ -243,66 +241,106 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture } private static void ensureBodyReplayable(Request request) throws IOException { - for (Part part : request.getBodyParts()) { - if (part instanceof InputStreamPart) { - throw new IOException("Multipart InputStream body part '" + part.getName() - + "' cannot be replayed after redirect"); + BodyRepresentation bodyRepresentation = selectedBodyRepresentation(request); + if (bodyRepresentation == BodyRepresentation.BODY_PARTS) { + for (Part part : request.getBodyParts()) { + if (part instanceof InputStreamPart) { + throw new IOException("Multipart InputStream body part '" + part.getName() + + "' cannot be replayed after redirect"); + } } } - File file = selectedBodyFile(request); + File file = null; + if (bodyRepresentation == BodyRepresentation.FILE) { + file = request.getFile(); + } else if (bodyRepresentation == BodyRepresentation.FILE_BODY_GENERATOR) { + file = ((FileBodyGenerator) request.getBodyGenerator()).getFile(); + } if (file != null && !file.isFile()) { throw new IOException("Redirect request body file " + file.getAbsolutePath() + " is not a file or does not exist"); } - } - private static File selectedBodyFile(Request request) { - // Keep this precedence aligned with NettyRequestFactory.body. A File can remain set alongside a - // higher-priority representation, so only validate it when the original request actually sent it. - if (hasBodyBeforeFile(request)) { - return null; + InputStream inputStream = null; + if (bodyRepresentation == BodyRepresentation.STREAM_DATA) { + inputStream = request.getStreamData(); + } else if (bodyRepresentation == BodyRepresentation.INPUT_STREAM_BODY_GENERATOR) { + inputStream = ((InputStreamBodyGenerator) request.getBodyGenerator()).getInputStream(); } - if (request.getFile() != null) { - return request.getFile(); + if (inputStream != null && !inputStream.markSupported()) { + throw new IOException("Redirect request body InputStream does not support mark/reset" + + " and cannot be replayed"); } - return request.getBodyGenerator() instanceof FileBodyGenerator - ? ((FileBodyGenerator) request.getBodyGenerator()).getFile() - : null; - } - - private static boolean hasBodyBeforeFile(Request request) { - return hasBodyBeforeStream(request) - || request.getStreamData() != null - || !request.getFormParams().isEmpty() - || !request.getBodyParts().isEmpty(); - } - - private static boolean hasBodyBeforeStream(Request request) { - return request.getByteData() != null - || request.getCompositeByteData() != null - || request.getStringData() != null - || request.getByteBufferData() != null - || request.getByteBufData() != null; } - private static boolean selectedBodyHasUnknownLength(Request request) { - if (hasBodyBeforeStream(request)) { - return false; + private static BodyRepresentation selectedBodyRepresentation(Request request) { + // Keep this precedence aligned with NettyRequestFactory.body. Some setters leave lower-priority + // representations in place, so redirect validation must inspect only the body that was sent. + if (request.getByteData() != null) { + return BodyRepresentation.BYTE_DATA; + } + if (request.getCompositeByteData() != null) { + return BodyRepresentation.COMPOSITE_BYTE_DATA; + } + if (request.getStringData() != null) { + return BodyRepresentation.STRING_DATA; + } + if (request.getByteBufferData() != null) { + return BodyRepresentation.BYTE_BUFFER_DATA; + } + if (request.getByteBufData() != null) { + return BodyRepresentation.BYTE_BUF_DATA; } if (request.getStreamData() != null) { - return true; + return BodyRepresentation.STREAM_DATA; } - if (!request.getFormParams().isEmpty() - || !request.getBodyParts().isEmpty() - || request.getFile() != null) { - return false; + if (!request.getFormParams().isEmpty()) { + return BodyRepresentation.FORM_PARAMS; + } + if (!request.getBodyParts().isEmpty()) { + return BodyRepresentation.BODY_PARTS; + } + if (request.getFile() != null) { + return BodyRepresentation.FILE; + } + if (request.getBodyGenerator() instanceof FileBodyGenerator) { + return BodyRepresentation.FILE_BODY_GENERATOR; } if (request.getBodyGenerator() instanceof InputStreamBodyGenerator) { + return BodyRepresentation.INPUT_STREAM_BODY_GENERATOR; + } + return request.getBodyGenerator() == null + ? BodyRepresentation.NONE + : BodyRepresentation.BODY_GENERATOR; + } + + private static boolean selectedBodyHasUnknownLength(Request request) { + BodyRepresentation bodyRepresentation = selectedBodyRepresentation(request); + if (bodyRepresentation == BodyRepresentation.STREAM_DATA + || bodyRepresentation == BodyRepresentation.BODY_GENERATOR) { + return true; + } + if (bodyRepresentation == BodyRepresentation.INPUT_STREAM_BODY_GENERATOR) { return ((InputStreamBodyGenerator) request.getBodyGenerator()).getContentLength() < 0; } - return request.getBodyGenerator() != null - && !(request.getBodyGenerator() instanceof FileBodyGenerator); + return false; + } + + private enum BodyRepresentation { + BYTE_DATA, + COMPOSITE_BYTE_DATA, + STRING_DATA, + BYTE_BUFFER_DATA, + BYTE_BUF_DATA, + STREAM_DATA, + FORM_PARAMS, + BODY_PARTS, + FILE, + FILE_BODY_GENERATOR, + INPUT_STREAM_BODY_GENERATOR, + BODY_GENERATOR, + NONE } private static HttpHeaders propagatedHeaders(Request request, Realm realm, boolean keepBody, boolean stripAuthorization) { diff --git a/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java b/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java index 6ab7fd3e2..1fb4b7d1b 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java @@ -52,6 +52,7 @@ 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.netty.handler.intercept.Redirect30xInterceptor.REDIRECT_STATUSES; import static org.asynchttpclient.util.HttpConstants.Methods.GET; import static org.asynchttpclient.util.HttpConstants.Methods.POST; import static org.asynchttpclient.util.HttpConstants.Methods.QUERY; @@ -66,9 +67,12 @@ public class RedirectBodyTest extends AbstractBasicTest { private static final byte[] REDIRECT_BODY = "redirect body".getBytes(UTF_8); private static final String CONTENT_TYPE_VALUE = "application/octet-stream"; + private static final String NON_REPLAYABLE_STREAM_MESSAGE = + "Redirect request body InputStream does not support mark/reset and cannot be replayed"; private static final List receivedContentLengths = new CopyOnWriteArrayList<>(); private static volatile boolean redirectAlreadyPerformed; + private static volatile byte[] receivedBody; private static volatile String receivedContentType; private static volatile String receivedMethod; private static volatile Path fileToDeleteBeforeRedirect; @@ -77,6 +81,7 @@ public class RedirectBodyTest extends AbstractBasicTest { public void setUp() { receivedContentLengths.clear(); redirectAlreadyPerformed = false; + receivedBody = null; receivedContentType = null; receivedMethod = null; fileToDeleteBeforeRedirect = null; @@ -101,6 +106,7 @@ public void handle(String pathInContext, Request request, HttpServletRequest htt httpResponse.setHeader(LOCATION.toString(), getTargetUrl()); } else { + receivedBody = body; receivedContentType = request.getContentType(); receivedMethod = request.getMethod(); httpResponse.setStatus(200); @@ -276,34 +282,110 @@ public synchronized void reset() throws IOException { .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()); + assertEquals(NON_REPLAYABLE_STREAM_MESSAGE, cause.getMessage()); } } - @ParameterizedTest(name = "{0} on {1} keeps the existing GET rewrite") + @RepeatedIfExceptionsTest(repeats = 5) + public void put301WithNonRepeatableBodyGeneratorFailsPromptly() 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.preparePut(getTargetUrl()) + .setBody(new InputStreamBodyGenerator(body)) + .setHeader("X-REDIRECT", "301") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS)); + + IOException cause = assertInstanceOf(IOException.class, thrown.getCause()); + assertEquals(NON_REPLAYABLE_STREAM_MESSAGE, cause.getMessage()); + } + } + + @ParameterizedTest(name = "{0} on {1} keeps its method and body") @CsvSource({ "PUT, 301", "PUT, 302", "PATCH, 301", "PATCH, 302", "DELETE, 301", - "DELETE, 302" + "DELETE, 302", + "CUSTOM, 301", + "CUSTOM, 302", + "GET, 301", + "GET, 302", + "HEAD, 301", + "HEAD, 302", + "OPTIONS, 301", + "OPTIONS, 302" }) - public void putPatchAndDelete301And302KeepExistingBehavior(String method, int statusCode) throws Exception { + public void nonPost301And302KeepMethodAndBody(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()) + 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); + assertArrayEquals(body.getBytes(UTF_8), receivedBody); + assertEquals(method, receivedMethod); + assertEquals(contentType, receivedContentType); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void put301AcrossDifferentHostsKeepsMethodAndBody() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + String body = "hello there"; + String contentType = "text/plain; charset=UTF-8"; + String originalUrl = getTargetUrl().replace("localhost", "127.0.0.1"); + + c.preparePut(originalUrl) + .setHeader(CONTENT_TYPE, contentType) + .setBody(body) + .setHeader("X-REDIRECT", "301") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + assertArrayEquals(body.getBytes(UTF_8), receivedBody); + assertEquals("PUT", receivedMethod); + assertEquals(contentType, receivedContentType); + } + } + + @ParameterizedTest(name = "{0} on caller-added 300 keeps its method and body") + @CsvSource({"POST", "PUT"}) + public void callerAddedRedirectStatusKeepsMethodAndBody(String method) throws Exception { + boolean added = REDIRECT_STATUSES.add(300); + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + String body = "hello there"; + String contentType = "text/plain; charset=UTF-8"; + + c.prepare(method, getTargetUrl()) + .setHeader(CONTENT_TYPE, contentType) + .setBody(body) + .setHeader("X-REDIRECT", "300") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + assertArrayEquals(body.getBytes(UTF_8), receivedBody); + assertEquals(method, receivedMethod); + assertEquals(contentType, receivedContentType); + } finally { + if (added) { + REDIRECT_STATUSES.remove(300); + } } } @@ -447,8 +529,7 @@ public synchronized void reset() throws IOException { () -> execute307(c.preparePost(getTargetUrl()).setBody(body))); IOException cause = assertInstanceOf(IOException.class, thrown.getCause()); - assertEquals("HTTP/1 request body InputStream already consumed and cannot be reset for a retry", - cause.getMessage()); + assertEquals(NON_REPLAYABLE_STREAM_MESSAGE, cause.getMessage()); } } @@ -463,8 +544,7 @@ public void fileInputStream307FailsPromptly() throws Exception { () -> execute307(c.preparePost(getTargetUrl()).setBody(body))); IOException cause = assertInstanceOf(IOException.class, thrown.getCause()); - assertEquals("HTTP/1 request body InputStream already consumed and cannot be reset for a retry", - cause.getMessage()); + assertEquals(NON_REPLAYABLE_STREAM_MESSAGE, cause.getMessage()); } } finally { Files.deleteIfExists(bodyFile); @@ -527,6 +607,19 @@ public void coexistingFileAndByteArray308UsesByteArray() throws Exception { } } + @RepeatedIfExceptionsTest(repeats = 5) + public void coexistingMultipartStreamAndByteArray307UsesByteArray() throws Exception { + try (InputStream unusedPart = new ByteArrayInputStream("unused part".getBytes(UTF_8)); + AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + Response response = execute307(c.preparePost(getTargetUrl()) + .setBody(REDIRECT_BODY) + .setBodyParts(List.of(new InputStreamPart("file", unusedPart, "unused.bin", + "unused part".length(), CONTENT_TYPE_VALUE)))); + + assertRedirectBody(response); + } + } + @RepeatedIfExceptionsTest(repeats = 5) public void formParams307KeepBody() throws Exception { try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { diff --git a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java index faada9961..82d2570b4 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java @@ -71,6 +71,13 @@ public class RedirectCredentialSecurityTest { 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 put301AuthOnOriginal = new AtomicReference<>(); + private static final AtomicReference put301CookieOnOriginal = new AtomicReference<>(); + private static final AtomicReference put301AuthOnTarget = new AtomicReference<>(); + private static final AtomicReference put301CookieOnTarget = new AtomicReference<>(); + private static final AtomicReference put301ContentTypeOnTarget = new AtomicReference<>(); + private static final AtomicReference put301MethodOnTarget = new AtomicReference<>(); + private static final AtomicReference put301BodyOnTarget = new AtomicReference<>(); private static final AtomicReference lastCookieHeaderOnA = new AtomicReference<>(); private static final AtomicReference lastCookieHeaderOnB = new AtomicReference<>(); private static final AtomicReference cookieAtChainStep2 = new AtomicReference<>(); @@ -213,6 +220,26 @@ public static void startServers() throws Exception { exchange.close(); }); + serverA.createContext("/redirect-put-301-to-b", exchange -> { + put301AuthOnOriginal.set(exchange.getRequestHeaders().getFirst("Authorization")); + put301CookieOnOriginal.set(exchange.getRequestHeaders().getFirst("Cookie")); + exchange.getRequestBody().readAllBytes(); + exchange.getResponseHeaders().add("Location", "http://127.0.0.1:" + portB + "/target-put-301"); + exchange.sendResponseHeaders(301, -1); + exchange.close(); + }); + + serverB.createContext("/target-put-301", exchange -> { + put301AuthOnTarget.set(exchange.getRequestHeaders().getFirst("Authorization")); + put301CookieOnTarget.set(exchange.getRequestHeaders().getFirst("Cookie")); + put301ContentTypeOnTarget.set(exchange.getRequestHeaders().getFirst("Content-Type")); + put301MethodOnTarget.set(exchange.getRequestMethod()); + put301BodyOnTarget.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")); @@ -565,6 +592,42 @@ void query301CrossOriginStripsCredentialsAndPreservesRequest() throws Exception } } + @Test + void put301CrossOriginReplaysBodyAndStripsCredentials() throws Exception { + DefaultAsyncHttpClientConfig config = new DefaultAsyncHttpClientConfig.Builder() + .setFollowRedirect(true) + .build(); + try (DefaultAsyncHttpClient client = new DefaultAsyncHttpClient(config)) { + put301AuthOnOriginal.set(null); + put301CookieOnOriginal.set(null); + put301AuthOnTarget.set(null); + put301CookieOnTarget.set(null); + put301ContentTypeOnTarget.set(null); + put301MethodOnTarget.set(null); + put301BodyOnTarget.set(null); + + client.preparePut("http://127.0.0.1:" + portA + "/redirect-put-301-to-b") + .setHeader("Authorization", "Bearer secret-token") + .setHeader("Cookie", "session=secret-session") + .setHeader("Content-Type", "application/octet-stream") + .setBody("sensitive-content") + .execute() + .get(5, TimeUnit.SECONDS); + + assertEquals("Bearer secret-token", put301AuthOnOriginal.get(), + "Authorization must be present on the original PUT request"); + assertEquals("session=secret-session", put301CookieOnOriginal.get(), + "Cookie must be present on the original PUT request"); + assertNull(put301AuthOnTarget.get(), + "Authorization must be stripped on a cross-origin PUT redirect"); + assertNull(put301CookieOnTarget.get(), + "Cookie must be stripped on a cross-origin PUT redirect"); + assertEquals("PUT", put301MethodOnTarget.get()); + assertEquals("application/octet-stream", put301ContentTypeOnTarget.get()); + assertEquals("sensitive-content", put301BodyOnTarget.get()); + } + } + /** * Cross-domain redirect (different port) must strip a user-supplied Cookie header. * Regression test for GHSA-fmxf-pm6p-7xgm.