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 80cac6d4f..8efc7cb68 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 @@ -33,10 +33,15 @@ import org.asynchttpclient.netty.channel.PrincipalScopedPartitionKey; import org.asynchttpclient.netty.request.NettyRequestSender; import io.netty.handler.codec.http2.Http2StreamChannel; +import org.asynchttpclient.request.body.generator.FileBodyGenerator; +import org.asynchttpclient.request.body.multipart.InputStreamPart; +import org.asynchttpclient.request.body.multipart.Part; import org.asynchttpclient.uri.Uri; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.File; +import java.io.IOException; import java.util.HashSet; import java.util.Set; @@ -56,7 +61,6 @@ import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.SEE_OTHER_303; import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.TEMPORARY_REDIRECT_307; import static org.asynchttpclient.util.HttpUtils.followRedirect; -import static org.asynchttpclient.util.MiscUtils.isNonEmpty; import static org.asynchttpclient.util.ThrowableUtil.unknownStackTrace; public class Redirect30xInterceptor { @@ -132,13 +136,28 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture LOGGER.debug("Stripping credentials on redirect to {}", newUri); } - final RequestBuilder requestBuilder = new RequestBuilder(switchToGet ? GET : originalMethod) - .setChannelPoolPartitioning(request.getChannelPoolPartitioning()) + final RequestBuilder requestBuilder; + if (keepBody) { + ensureBodyReplayable(request); + requestBuilder = request.toBuilder(); + if (!sameBase) { + // An explicitly resolved address and virtual host belong to the previous target. + requestBuilder.setAddress(null); + requestBuilder.setVirtualHost(null); + } + } else { + requestBuilder = new RequestBuilder(switchToGet ? GET : originalMethod) + .setChannelPoolPartitioning(request.getChannelPoolPartitioning()) + .setLocalAddress(request.getLocalAddress()) + .setNameResolver(request.getNameResolver()) + .setProxyServer(request.getProxyServer()) + .setRangeOffset(request.getRangeOffset()); + } + + requestBuilder.setMethod(switchToGet ? GET : originalMethod) .setFollowRedirect(true) - .setLocalAddress(request.getLocalAddress()) - .setNameResolver(request.getNameResolver()) - .setProxyServer(request.getProxyServer()) .setRealm(stripAuth ? null : request.getRealm()) + .setHeaders(propagatedHeaders(request, realm, keepBody, stripAuth)) .setRequestTimeout(request.getRequestTimeout()) .setReadTimeout(request.getReadTimeout()); @@ -154,27 +173,10 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture if (stripAuth) { future.setRealm(null); future.setProxyRealm(null); + // Request.toBuilder copies Cookie objects separately from the Cookie header. + requestBuilder.resetCookies(); } - if (keepBody) { - requestBuilder.setCharset(request.getCharset()); - if (isNonEmpty(request.getFormParams())) { - requestBuilder.setFormParams(request.getFormParams()); - } else if (request.getStringData() != null) { - requestBuilder.setBody(request.getStringData()); - } else if (request.getByteData() != null) { - requestBuilder.setBody(request.getByteData()); - } else if (request.getByteBufferData() != null) { - requestBuilder.setBody(request.getByteBufferData()); - } else if (request.getBodyGenerator() != null) { - requestBuilder.setBody(request.getBodyGenerator()); - } else if (isNonEmpty(request.getBodyParts())) { - requestBuilder.setBodyParts(request.getBodyParts()); - } - } - - requestBuilder.setHeaders(propagatedHeaders(request, realm, keepBody, stripAuth)); - // in case of a redirect from HTTP to HTTPS, future // attributes might change final boolean initialConnectionKeepAlive = future.isKeepAlive(); @@ -192,7 +194,7 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture } } - if (sameBase) { + if (sameBase && !keepBody) { // we can only assume the virtual host is still valid if the baseUrl is the same requestBuilder.setVirtualHost(request.getVirtualHost()); } @@ -229,10 +231,50 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture return false; } + 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"); + } + } + + File file = selectedBodyFile(request); + 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 (request.getByteData() != null + || request.getCompositeByteData() != null + || request.getStringData() != null + || request.getByteBufferData() != null + || request.getByteBufData() != null + || request.getStreamData() != null + || !request.getFormParams().isEmpty() + || !request.getBodyParts().isEmpty()) { + return null; + } + if (request.getFile() != null) { + return request.getFile(); + } + return request.getBodyGenerator() instanceof FileBodyGenerator + ? ((FileBodyGenerator) request.getBodyGenerator()).getFile() + : null; + } + private static HttpHeaders propagatedHeaders(Request request, Realm realm, boolean keepBody, boolean stripAuthorization) { - HttpHeaders headers = request.getHeaders() - .remove(HOST) - .remove(CONTENT_LENGTH); + HttpHeaders headers = request.getHeaders().copy().remove(HOST); + + // A raw InputStream has no intrinsic length from which NettyRequestFactory can rebuild this header. + // Preserve a caller-supplied value when the stream itself is replayed. + if (!keepBody || request.getStreamData() == null) { + headers.remove(CONTENT_LENGTH); + } if (!keepBody) { headers.remove(CONTENT_TYPE); diff --git a/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java b/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java index 461c7a06a..03141ffc4 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectBodyTest.java @@ -16,32 +16,61 @@ package org.asynchttpclient; import io.github.artsok.RepeatedIfExceptionsTest; +import io.netty.buffer.ByteBuf; +import io.netty.buffer.Unpooled; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.io.IOUtils; +import org.asynchttpclient.filter.FilterContext; +import org.asynchttpclient.filter.ResponseFilter; +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 java.io.ByteArrayInputStream; +import java.io.FilterInputStream; import java.io.IOException; +import java.io.InputStream; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Duration; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; +import static java.nio.charset.StandardCharsets.UTF_8; +import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_LENGTH; 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.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; 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 List receivedContentLengths = new CopyOnWriteArrayList<>(); private static volatile boolean redirectAlreadyPerformed; private static volatile String receivedContentType; + private static volatile Path fileToDeleteBeforeRedirect; @BeforeEach public void setUp() { + receivedContentLengths.clear(); redirectAlreadyPerformed = false; receivedContentType = null; + fileToDeleteBeforeRedirect = null; } @Override @@ -50,9 +79,14 @@ public AbstractHandler configureHandler() throws Exception { @Override public void handle(String pathInContext, Request request, HttpServletRequest httpRequest, HttpServletResponse httpResponse) throws IOException { + byte[] body = IOUtils.toByteArray(request.getInputStream()); + receivedContentLengths.add(String.valueOf(httpRequest.getHeader(CONTENT_LENGTH.toString()))); String redirectHeader = httpRequest.getHeader("X-REDIRECT"); if (redirectHeader != null && !redirectAlreadyPerformed) { redirectAlreadyPerformed = true; + if (fileToDeleteBeforeRedirect != null) { + Files.deleteIfExists(fileToDeleteBeforeRedirect); + } httpResponse.setStatus(Integer.valueOf(redirectHeader)); httpResponse.setContentLength(0); httpResponse.setHeader(LOCATION.toString(), getTargetUrl()); @@ -60,12 +94,9 @@ public void handle(String pathInContext, Request request, HttpServletRequest htt } else { receivedContentType = request.getContentType(); httpResponse.setStatus(200); - int len = request.getContentLength(); - httpResponse.setContentLength(len); - if (len > 0) { - byte[] buffer = new byte[len]; - IOUtils.read(request.getInputStream(), buffer); - httpResponse.getOutputStream().write(buffer); + httpResponse.setContentLength(body.length); + if (body.length > 0) { + httpResponse.getOutputStream().write(body); } } httpResponse.getOutputStream().flush(); @@ -121,4 +152,243 @@ public void regular307KeepsBody() throws Exception { assertEquals(receivedContentType, contentType); } } + + @RepeatedIfExceptionsTest(repeats = 5) + public void redirectPreservesPerRequestSettings() throws Exception { + Duration readTimeout = Duration.ofSeconds(7); + long rangeOffset = 41L; + List observedReadTimeouts = new CopyOnWriteArrayList<>(); + List observedRangeOffsets = new CopyOnWriteArrayList<>(); + ResponseFilter observer = new ResponseFilter() { + @Override + public FilterContext filter(FilterContext ctx) { + observedReadTimeouts.add(ctx.getRequest().getReadTimeout()); + observedRangeOffsets.add(ctx.getRequest().getRangeOffset()); + return ctx; + } + }; + + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true).addResponseFilter(observer))) { + Response response = c.preparePost(getTargetUrl()) + .setReadTimeout(readTimeout) + .setRangeOffset(rangeOffset) + .setBody(REDIRECT_BODY) + .setHeader("X-REDIRECT", "307") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + + assertArrayEquals(REDIRECT_BODY, response.getResponseBodyAsBytes()); + assertEquals(List.of(readTimeout, readTimeout), observedReadTimeouts); + assertEquals(List.of(rangeOffset, rangeOffset), observedRangeOffsets); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void compositeByteArray307KeepsBody() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + byte[] first = "redirect ".getBytes(UTF_8); + byte[] second = "body".getBytes(UTF_8); + + Response response = execute307(c.preparePost(getTargetUrl()).setBody(Arrays.asList(first, second))); + + assertRedirectBody(response); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void byteBuf307KeepsBody() throws Exception { + ByteBuf body = Unpooled.wrappedBuffer(REDIRECT_BODY); + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + Response response = execute307(c.preparePost(getTargetUrl()).setBody(body)); + + assertRedirectBody(response); + assertEquals(1, body.refCnt(), "the caller must retain ownership of its ByteBuf"); + } finally { + body.release(); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void resettableInputStream307KeepsBody() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + Response response = execute307(c.preparePost(getTargetUrl()).setBody(new ByteArrayInputStream(REDIRECT_BODY))); + + assertRedirectBody(response); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void inputStream307PreservesExplicitContentLength() throws Exception { + try (InputStream body = new ByteArrayInputStream(REDIRECT_BODY); + AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + Response response = execute307(c.preparePost(getTargetUrl()) + .setHeader(CONTENT_LENGTH, REDIRECT_BODY.length) + .setBody(body)); + + assertRedirectBody(response); + String expectedLength = Integer.toString(REDIRECT_BODY.length); + assertEquals(List.of(expectedLength, expectedLength), receivedContentLengths); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void nonResettableInputStream307FailsPromptly() 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, + () -> 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()); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void fileInputStream307FailsPromptly() throws Exception { + Path bodyFile = Files.createTempFile("ahc-redirect-stream-", ".bin"); + try { + Files.write(bodyFile, REDIRECT_BODY); + try (InputStream body = Files.newInputStream(bodyFile); + AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + ExecutionException thrown = assertThrows(ExecutionException.class, + () -> 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()); + } + } finally { + Files.deleteIfExists(bodyFile); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void file307KeepsBody() throws Exception { + Path body = Files.createTempFile("ahc-redirect-body-", ".bin"); + try { + Files.write(body, REDIRECT_BODY); + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + Response response = execute307(c.preparePost(getTargetUrl()).setBody(body.toFile())); + + assertRedirectBody(response); + } + } finally { + Files.deleteIfExists(body); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void vanishedFile307FailsPromptly() throws Exception { + Path body = Files.createTempFile("ahc-redirect-vanished-", ".bin"); + try { + Files.write(body, REDIRECT_BODY); + fileToDeleteBeforeRedirect = body; + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + ExecutionException thrown = assertThrows(ExecutionException.class, + () -> execute307(c.preparePost(getTargetUrl()).setBody(body.toFile()))); + + IOException cause = assertInstanceOf(IOException.class, thrown.getCause()); + assertEquals("Redirect request body file " + body.toAbsolutePath() + + " is not a file or does not exist", cause.getMessage()); + } + } finally { + Files.deleteIfExists(body); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void coexistingFileAndByteArray308UsesByteArray() throws Exception { + Path file = Files.createTempFile("ahc-redirect-precedence-", ".bin"); + try { + Files.write(file, "wrong file body".getBytes(UTF_8)); + fileToDeleteBeforeRedirect = file; + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + Response response = c.preparePost(getTargetUrl()) + .setBody(file.toFile()) + .setBody(REDIRECT_BODY) + .setHeader(CONTENT_TYPE, CONTENT_TYPE_VALUE) + .setHeader("X-REDIRECT", "308") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + + assertRedirectBody(response); + } + } finally { + Files.deleteIfExists(file); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void formParams307KeepBody() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + Response response = c.preparePost(getTargetUrl()) + .addFormParam("field", "value") + .setHeader("X-REDIRECT", "307") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + + assertEquals("field=value", response.getResponseBody()); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void multipart307KeepsBody() throws Exception { + try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + Response response = c.preparePost(getTargetUrl()) + .addBodyPart(new StringPart("field", "multipart value")) + .setHeader("X-REDIRECT", "307") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + + assertTrue(response.getResponseBody().contains("multipart value")); + } + } + + @RepeatedIfExceptionsTest(repeats = 5) + public void inputStreamMultipart307FailsPromptly() throws Exception { + Path bodyFile = Files.createTempFile("ahc-redirect-multipart-", ".bin"); + try { + Files.write(bodyFile, REDIRECT_BODY); + try (InputStream body = Files.newInputStream(bodyFile); + AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { + ExecutionException thrown = assertThrows(ExecutionException.class, + () -> c.preparePost(getTargetUrl()) + .addBodyPart(new InputStreamPart("file", body, bodyFile.getFileName().toString(), + REDIRECT_BODY.length, CONTENT_TYPE_VALUE)) + .setHeader("X-REDIRECT", "307") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS)); + + IOException cause = assertInstanceOf(IOException.class, thrown.getCause()); + assertEquals("Multipart InputStream body part 'file' cannot be replayed after redirect", + cause.getMessage()); + } + } finally { + Files.deleteIfExists(bodyFile); + } + } + + private static Response execute307(BoundRequestBuilder requestBuilder) throws Exception { + return requestBuilder + .setHeader(CONTENT_TYPE, CONTENT_TYPE_VALUE) + .setHeader("X-REDIRECT", "307") + .execute() + .get(TIMEOUT, TimeUnit.SECONDS); + } + + private static void assertRedirectBody(Response response) { + assertArrayEquals(REDIRECT_BODY, response.getResponseBodyAsBytes()); + assertEquals(CONTENT_TYPE_VALUE, receivedContentType); + } } diff --git a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java index daa9676d4..74e30c7fc 100644 --- a/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java +++ b/client/src/test/java/org/asynchttpclient/RedirectCredentialSecurityTest.java @@ -535,6 +535,26 @@ void crossDomainRedirectStripsCookieHeader() throws Exception { } } + @Test + void crossDomainRedirectStripsCookieObject() throws Exception { + DefaultAsyncHttpClientConfig config = new DefaultAsyncHttpClientConfig.Builder() + .setFollowRedirect(true) + .build(); + try (DefaultAsyncHttpClient client = new DefaultAsyncHttpClient(config)) { + lastCookieHeaderOnA.set(null); + lastCookieHeaderOnB.set(null); + + client.prepareGet("http://127.0.0.1:" + portA + "/redirect-to-b") + .addCookie(new DefaultCookie("session", "abc123")) + .execute() + .get(5, TimeUnit.SECONDS); + + assertEquals("session=abc123", lastCookieHeaderOnA.get()); + assertNull(lastCookieHeaderOnB.get(), + "Cookie objects must not be copied to a cross-domain redirect target"); + } + } + /** * Same-origin redirect (same host and port) should preserve the Cookie header. */