Skip to content

fix(netty): preserve all redirect body types #2316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mkurz wants to merge 1 commit into
base: main
Choose a base branch
from
+182 −20
Open

fix(netty): preserve all redirect body types #2316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,20 +147,7 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
}

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());
}
copyBody(requestBuilder, request);
}

requestBuilder.setHeaders(propagatedHeaders(request, realm, keepBody, stripAuth));
Expand Down Expand Up @@ -219,6 +206,35 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
return false;
}

private static void copyBody(RequestBuilder requestBuilder, Request request) {
requestBuilder.setCharset(request.getCharset());

// Keep this precedence aligned with NettyRequestFactory.body. A Request can retain a File or

@hyperxpro hyperxpro Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we build this from request.toBuilder() and override method, uri, headers and realm after? RequestBuilderBase already copies all the body fields, plus charset, rangeOffset and readTimeout.

The builder above copies setRequestTimeout but not setReadTimeout, so a per-request read timeout silently falls back to the config default after the first redirect. toBuilder() would fix that too, and this wouldn't drift out of sync with NettyRequestFactory.body again.

// BodyGenerator alongside another representation, so the redirect must copy the representation
// that the original request actually sent.
if (request.getByteData() != null) {
requestBuilder.setBody(request.getByteData());
} else if (request.getCompositeByteData() != null) {
requestBuilder.setBody(request.getCompositeByteData());
} else if (request.getStringData() != null) {
requestBuilder.setBody(request.getStringData());
} else if (request.getByteBufferData() != null) {
requestBuilder.setBody(request.getByteBufferData());
} else if (request.getByteBufData() != null) {
requestBuilder.setBody(request.getByteBufData());
} else if (request.getStreamData() != null) {

@hyperxpro hyperxpro Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WriteProgressListener closes the stream after the first leg, so this only replays if close() is a no-op. ByteArrayInputStream is fine, Files.newInputStream(...) and anything buffered will fail the reset() in NettyInputStreamBody.

We used to send an empty body on the redirect leg, so this turns a silent bug into an IOException for the common case. That's the right trade I think, but it needs a release note, and a test with a real FileInputStream and not just ByteArrayInputStream.

requestBuilder.setBody(request.getStreamData());

@hyperxpro hyperxpro Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

propagatedHeaders removes CONTENT_LENGTH and the rebuilt NettyInputStreamBody doesn't have a length, so a stream that went out with Content-Length on the first leg goes out chunked on the second. Some targets won't accept a chunked request body. Can we carry the original length over?

} else if (isNonEmpty(request.getFormParams())) {
requestBuilder.setFormParams(request.getFormParams());
} else if (isNonEmpty(request.getBodyParts())) {

@hyperxpro hyperxpro Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InputStreamMultipartPart closes its stream once it has written it, and we don't have a consumed check here like the one NettyInputStreamBody got in #2312.

So for an InputStreamPart the replayed request still advertises the declared Content-Length, but transferContentTo sees -1 straight away and writes nothing. The server then waits for a body that never arrives. Should we detect the parts we can't replay and fail fast the same way?

requestBuilder.setBodyParts(request.getBodyParts());
} else if (request.getFile() != null) {

@hyperxpro hyperxpro Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today the File is dropped on redirect, so a 307 to another origin sends nothing. After this we upload the whole file to whatever host Location points at, and a hostile target can bounce us maxRedirects times to get several copies.

Same origin is fine. Cross origin I'd like to be a deliberate decision, and same for getStreamData() above.

requestBuilder.setBody(request.getFile());

@hyperxpro hyperxpro Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NettyFileBody throws IllegalArgumentException if the file is gone by now, and we're on the event loop here. Nothing in newNettyRequestAndResponseFuture catches it, so it ends up in exceptionCaught after drainChannelAndOffer already gave the old channel back to the pool, and then we close a connection another request might be using. Check the file here and abort the future with an IOException instead?

} else if (request.getBodyGenerator() != null) {
requestBuilder.setBody(request.getBodyGenerator());
}
}

private static HttpHeaders propagatedHeaders(Request request, Realm realm, boolean keepBody, boolean stripAuthorization) {
HttpHeaders headers = request.getHeaders()
.remove(HOST)
Expand Down
158 changes: 152 additions & 6 deletions client/src/test/java/org/asynchttpclient/RedirectBodyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,43 @@
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.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.util.Arrays;
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_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 volatile boolean redirectAlreadyPerformed;
private static volatile String receivedContentType;

Expand All @@ -50,6 +68,7 @@ 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());
String redirectHeader = httpRequest.getHeader("X-REDIRECT");
if (redirectHeader != null && !redirectAlreadyPerformed) {
redirectAlreadyPerformed = true;
Expand All @@ -60,12 +79,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();
Expand Down Expand Up @@ -121,4 +137,134 @@ public void regular307KeepsBody() throws Exception {
assertEquals(receivedContentType, contentType);
}
}

@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 nonResettableInputStream307FailsPromptly() throws Exception {
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");
}
};
try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) {
ExecutionException thrown = assertThrows(ExecutionException.class,
() -> execute307(c.preparePost(getTargetUrl()).setBody(body)));

assertInstanceOf(IOException.class, thrown.getCause());

@hyperxpro hyperxpro Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any channel-level failure is an IOException too, so this still passes if the replay guard in NettyInputStreamBody goes away. Assert on the message instead. The FilterInputStream is also never closed.

}
}

@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 coexistingFileAndByteArray308UsesByteArray() throws Exception {
Path file = Files.createTempFile("ahc-redirect-precedence-", ".bin");
try {
Files.write(file, "wrong file body".getBytes(UTF_8));
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"))

@hyperxpro hyperxpro Aug 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StringPart is replayable so this passes either way. The one that breaks is InputStreamPart with a declared length, please add that one.

.setHeader("X-REDIRECT", "307")
.execute()
.get(TIMEOUT, TimeUnit.SECONDS);

assertTrue(response.getResponseBody().contains("multipart value"));
}
}

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);
}
}
Loading