Skip to content

Commit 49230f5

Browse files
mkurzcodexhyperxpro
authored
fix(netty): preserve all redirect body types (#2316)
## Summary - Build keep-body redirects from the original request, preserving every supported body representation and per-request setting. - Clear target-specific routing and credential state when a redirect crosses an origin, including separately stored Cookie objects. - Preserve an explicit `Content-Length` when replaying a raw `InputStream` or an unknown-length `InputStreamBodyGenerator`. - Replay resettable streams and fail promptly for consumed raw streams, streamed multipart parts, and vanished files that cannot be replayed safely. - Pin body bytes, headers, body-selection precedence, caller-owned `ByteBuf` references, and the new failure modes with focused tests. ## Problem `Redirect30xInterceptor` rebuilds a request when it follows a strict 302, 307, or 308 redirect. Its keep-body copy chain handled form parameters, strings, byte arrays, `ByteBuffer`, body generators, and multipart bodies, but omitted four real request send paths: - `List<byte[]>` / composite byte arrays - Netty `ByteBuf` - `InputStream` - `File` The redirected request therefore kept its method and Content-Type but sent zero bytes. The `File` case is especially risky for uploads because the target can accept an apparently valid empty PUT or POST. Reconstructing the request field by field also omitted unrelated per-request state such as the read timeout and range offset, and maintaining a second body-selection chain alongside `NettyRequestFactory` made future drift likely. This is a pre-existing omission. AHC issue [#1643](#1643) previously fixed the same class of bug for multipart bodies. The copy chain was carried through pull request [#1843](#1843) without a policy discussion. Focused searches found no existing issue or pull request covering these four representations. ## Change Build a keep-body redirect with `request.toBuilder()` and then replace only redirect-specific state. This preserves all current and future body representations and per-request options without duplicating `NettyRequestFactory.body`. Headers are copied before redirect-only values are removed, so the original request is not mutated. On a cross-origin redirect, the copied request drops the previous resolved address, virtual host, realm, authorization headers, and Cookie objects before the cookie store adds cookies that legitimately match the new URI. The body is not covered by that boundary. It follows the existing keep-body policy, which means a `File` or `InputStream` body that a cross-origin redirect leg previously received as empty is now sent in full, and a target that keeps redirecting can receive it once per hop up to `maxRedirects`. That is the same exposure byte arrays, strings, form parameters, and multipart bodies already have today. Composite byte arrays, caller-owned `ByteBuf`s, and files are repeatable. A resettable `InputStream`, such as `ByteArrayInputStream`, also replays. A caller-supplied `Content-Length` is retained for a raw `InputStream` or an `InputStreamBodyGenerator` without a declared length, because neither has an intrinsic size from which to recompute it. A consumed stream that cannot be reset reaches the existing fail-fast guard added in #2312 and completes the future with `IOException`; that is preferable to silently succeeding with an empty body. An `InputStreamPart` is closed by the first multipart send and has no equivalent replay guard, so a keep-body redirect now fails promptly instead of risking a hang or incomplete multipart request. A selected `File` or `FileBodyGenerator` is also checked before dispatching the redirect; if it disappeared after the first send, the future fails with `IOException` before a target pooled channel can be removed and an unchecked constructor exception can escape. The validation follows `NettyRequestFactory` precedence so a sticky `File` field is ignored when a higher-priority body representation was actually sent. The change does not alter which methods or status codes keep a body, nor does it introduce a new cross-origin policy. It makes the existing strict-302, 307, and 308 behavior complete for every supported request-body representation. ## Compatibility There is no public API change. Requests that previously sent an empty body on a keep-body redirect now resend their configured body. **Behavior changes:** - A non-resettable `InputStream` on a keep-body redirect previously completed successfully after sending an empty redirected request. It now completes the request future exceptionally with `IOException`. This includes `FileInputStream`, which is closed after the first send and cannot be reset for replay. - A multipart `InputStreamPart` now fails promptly with `IOException` when a keep-body redirect requires replay. Reusing its already-consumed and closed stream could previously hang or send incomplete multipart content. - A selected file that disappears between the first request and redirect now fails with `IOException` before redirect dispatch rather than allowing an unchecked `IllegalArgumentException` to escape while constructing the next request. - A `File` or `InputStream` body is now sent on a keep-body redirect to a different origin, where the redirected request previously carried no body. Credentials are still stripped at that boundary, but the payload is not. Callers that accidentally relied on an empty or incomplete redirected request will observe an exception, but the failure is explicit instead of silently losing configured content. There is no public API change. ## AI disclosure OpenAI Codex on behalf of Matthias Kurz. The commit includes `Co-Authored-By: OpenAI Codex <codex@openai.com>` per `AGENTS.md`. ## Test plan - [x] On untouched `upstream/main`, the focused suite reproduced five failures: four body types arrived as zero bytes and a non-resettable stream incorrectly completed successfully. - [x] Before the generator fix, a one-argument `InputStreamBodyGenerator` sent `Content-Length: 13` on the first leg and no `Content-Length` on the redirected leg. - [x] `./mvnw -pl client -Dtest=RedirectBodyTest,RedirectCredentialSecurityTest test` on JDK 11: 40 tests passed, including Netty leak detection. - [x] `./mvnw clean verify` on JDK 11: 1,496 tests passed and Revapi completed without failures (`BUILD SUCCESS`). Generated with OpenAI Codex. --------- Co-authored-by: OpenAI Codex <codex@openai.com> Co-authored-by: Aayush Atharva <24762260+hyperxpro@users.noreply.github.com>
1 parent 492eb2d commit 49230f5

3 files changed

Lines changed: 446 additions & 36 deletions

File tree

client/src/main/java/org/asynchttpclient/netty/handler/intercept/Redirect30xInterceptor.java

Lines changed: 99 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import io.netty.handler.codec.http.HttpStatusClass;
2222
import io.netty.handler.codec.http.HttpUtil;
2323
import io.netty.handler.codec.http.cookie.Cookie;
24+
import io.netty.handler.codec.http2.Http2StreamChannel;
2425
import org.asynchttpclient.AsyncHttpClientConfig;
2526
import org.asynchttpclient.Realm;
2627
import org.asynchttpclient.Realm.AuthScheme;
@@ -32,11 +33,16 @@
3233
import org.asynchttpclient.netty.channel.ChannelManager;
3334
import org.asynchttpclient.netty.channel.PrincipalScopedPartitionKey;
3435
import org.asynchttpclient.netty.request.NettyRequestSender;
35-
import io.netty.handler.codec.http2.Http2StreamChannel;
36+
import org.asynchttpclient.request.body.generator.FileBodyGenerator;
37+
import org.asynchttpclient.request.body.generator.InputStreamBodyGenerator;
38+
import org.asynchttpclient.request.body.multipart.InputStreamPart;
39+
import org.asynchttpclient.request.body.multipart.Part;
3640
import org.asynchttpclient.uri.Uri;
3741
import org.slf4j.Logger;
3842
import org.slf4j.LoggerFactory;
3943

44+
import java.io.File;
45+
import java.io.IOException;
4046
import java.util.HashSet;
4147
import java.util.Set;
4248

@@ -56,7 +62,6 @@
5662
import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.SEE_OTHER_303;
5763
import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.TEMPORARY_REDIRECT_307;
5864
import static org.asynchttpclient.util.HttpUtils.followRedirect;
59-
import static org.asynchttpclient.util.MiscUtils.isNonEmpty;
6065
import static org.asynchttpclient.util.ThrowableUtil.unknownStackTrace;
6166

6267
public class Redirect30xInterceptor {
@@ -132,13 +137,28 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
132137
LOGGER.debug("Stripping credentials on redirect to {}", newUri);
133138
}
134139

135-
final RequestBuilder requestBuilder = new RequestBuilder(switchToGet ? GET : originalMethod)
136-
.setChannelPoolPartitioning(request.getChannelPoolPartitioning())
140+
final RequestBuilder requestBuilder;
141+
if (keepBody) {
142+
ensureBodyReplayable(request);
143+
requestBuilder = request.toBuilder();
144+
if (!sameBase) {
145+
// An explicitly resolved address and virtual host belong to the previous target.
146+
requestBuilder.setAddress(null);
147+
requestBuilder.setVirtualHost(null);
148+
}
149+
} else {
150+
requestBuilder = new RequestBuilder(switchToGet ? GET : originalMethod)
151+
.setChannelPoolPartitioning(request.getChannelPoolPartitioning())
152+
.setLocalAddress(request.getLocalAddress())
153+
.setNameResolver(request.getNameResolver())
154+
.setProxyServer(request.getProxyServer())
155+
.setRangeOffset(request.getRangeOffset());
156+
}
157+
158+
requestBuilder.setMethod(switchToGet ? GET : originalMethod)
137159
.setFollowRedirect(true)
138-
.setLocalAddress(request.getLocalAddress())
139-
.setNameResolver(request.getNameResolver())
140-
.setProxyServer(request.getProxyServer())
141160
.setRealm(stripAuth ? null : request.getRealm())
161+
.setHeaders(propagatedHeaders(request, realm, keepBody, stripAuth))
142162
.setRequestTimeout(request.getRequestTimeout())
143163
.setReadTimeout(request.getReadTimeout());
144164

@@ -154,27 +174,10 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
154174
if (stripAuth) {
155175
future.setRealm(null);
156176
future.setProxyRealm(null);
177+
// Request.toBuilder copies Cookie objects separately from the Cookie header.
178+
requestBuilder.resetCookies();
157179
}
158180

159-
if (keepBody) {
160-
requestBuilder.setCharset(request.getCharset());
161-
if (isNonEmpty(request.getFormParams())) {
162-
requestBuilder.setFormParams(request.getFormParams());
163-
} else if (request.getStringData() != null) {
164-
requestBuilder.setBody(request.getStringData());
165-
} else if (request.getByteData() != null) {
166-
requestBuilder.setBody(request.getByteData());
167-
} else if (request.getByteBufferData() != null) {
168-
requestBuilder.setBody(request.getByteBufferData());
169-
} else if (request.getBodyGenerator() != null) {
170-
requestBuilder.setBody(request.getBodyGenerator());
171-
} else if (isNonEmpty(request.getBodyParts())) {
172-
requestBuilder.setBodyParts(request.getBodyParts());
173-
}
174-
}
175-
176-
requestBuilder.setHeaders(propagatedHeaders(request, realm, keepBody, stripAuth));
177-
178181
// in case of a redirect from HTTP to HTTPS, future
179182
// attributes might change
180183
final boolean initialConnectionKeepAlive = future.isKeepAlive();
@@ -192,7 +195,7 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
192195
}
193196
}
194197

195-
if (sameBase) {
198+
if (sameBase && !keepBody) {
196199
// we can only assume the virtual host is still valid if the baseUrl is the same
197200
requestBuilder.setVirtualHost(request.getVirtualHost());
198201
}
@@ -229,10 +232,76 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
229232
return false;
230233
}
231234

235+
private static void ensureBodyReplayable(Request request) throws IOException {
236+
for (Part part : request.getBodyParts()) {
237+
if (part instanceof InputStreamPart) {
238+
throw new IOException("Multipart InputStream body part '" + part.getName()
239+
+ "' cannot be replayed after redirect");
240+
}
241+
}
242+
243+
File file = selectedBodyFile(request);
244+
if (file != null && !file.isFile()) {
245+
throw new IOException("Redirect request body file " + file.getAbsolutePath()
246+
+ " is not a file or does not exist");
247+
}
248+
}
249+
250+
private static File selectedBodyFile(Request request) {
251+
// Keep this precedence aligned with NettyRequestFactory.body. A File can remain set alongside a
252+
// higher-priority representation, so only validate it when the original request actually sent it.
253+
if (hasBodyBeforeFile(request)) {
254+
return null;
255+
}
256+
if (request.getFile() != null) {
257+
return request.getFile();
258+
}
259+
return request.getBodyGenerator() instanceof FileBodyGenerator
260+
? ((FileBodyGenerator) request.getBodyGenerator()).getFile()
261+
: null;
262+
}
263+
264+
private static boolean hasBodyBeforeFile(Request request) {
265+
return hasBodyBeforeStream(request)
266+
|| request.getStreamData() != null
267+
|| !request.getFormParams().isEmpty()
268+
|| !request.getBodyParts().isEmpty();
269+
}
270+
271+
private static boolean hasBodyBeforeStream(Request request) {
272+
return request.getByteData() != null
273+
|| request.getCompositeByteData() != null
274+
|| request.getStringData() != null
275+
|| request.getByteBufferData() != null
276+
|| request.getByteBufData() != null;
277+
}
278+
279+
private static boolean selectedBodyHasUnknownLength(Request request) {
280+
if (hasBodyBeforeStream(request)) {
281+
return false;
282+
}
283+
if (request.getStreamData() != null) {
284+
return true;
285+
}
286+
if (!request.getFormParams().isEmpty()
287+
|| !request.getBodyParts().isEmpty()
288+
|| request.getFile() != null) {
289+
return false;
290+
}
291+
if (request.getBodyGenerator() instanceof InputStreamBodyGenerator) {
292+
return ((InputStreamBodyGenerator) request.getBodyGenerator()).getContentLength() < 0;
293+
}
294+
return request.getBodyGenerator() != null
295+
&& !(request.getBodyGenerator() instanceof FileBodyGenerator);
296+
}
297+
232298
private static HttpHeaders propagatedHeaders(Request request, Realm realm, boolean keepBody, boolean stripAuthorization) {
233-
HttpHeaders headers = request.getHeaders()
234-
.remove(HOST)
235-
.remove(CONTENT_LENGTH);
299+
HttpHeaders headers = request.getHeaders().copy().remove(HOST);
300+
301+
// Preserve an explicit length when the selected stream representation cannot rebuild it.
302+
if (!keepBody || !selectedBodyHasUnknownLength(request)) {
303+
headers.remove(CONTENT_LENGTH);
304+
}
236305

237306
if (!keepBody) {
238307
headers.remove(CONTENT_TYPE);

0 commit comments

Comments
 (0)