fix(netty): preserve all redirect body types - #2316
Conversation
Redirect30xInterceptor copied only six request-body representations when a redirect retained the body. Composite byte arrays, ByteBufs, InputStreams, and Files therefore became empty requests on 307, 308, and strict 302 redirects. Copy the representation selected for the original request, following NettyRequestFactory's precedence. Resettable streams can then replay, while non-resettable streams fail promptly through the existing replay guard. Cover all four omissions byte-for-byte, pin coexistence precedence, retain the caller-owned ByteBuf, and keep form and multipart replay behavior. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
|
We use ahc in https://github.com/playframework/play-ws and I am in the process of upgrading to v3 - and found some thing worth adressing. |
| 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 |
There was a problem hiding this comment.
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.
| requestBuilder.setBody(request.getByteBufferData()); | ||
| } else if (request.getByteBufData() != null) { | ||
| requestBuilder.setBody(request.getByteBufData()); | ||
| } else if (request.getStreamData() != null) { |
There was a problem hiding this comment.
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.
| } else if (request.getByteBufData() != null) { | ||
| requestBuilder.setBody(request.getByteBufData()); | ||
| } else if (request.getStreamData() != null) { | ||
| requestBuilder.setBody(request.getStreamData()); |
There was a problem hiding this comment.
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?
| requestBuilder.setBody(request.getStreamData()); | ||
| } else if (isNonEmpty(request.getFormParams())) { | ||
| requestBuilder.setFormParams(request.getFormParams()); | ||
| } else if (isNonEmpty(request.getBodyParts())) { |
There was a problem hiding this comment.
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.setFormParams(request.getFormParams()); | ||
| } else if (isNonEmpty(request.getBodyParts())) { | ||
| requestBuilder.setBodyParts(request.getBodyParts()); | ||
| } else if (request.getFile() != null) { |
There was a problem hiding this comment.
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.
| } else if (isNonEmpty(request.getBodyParts())) { | ||
| requestBuilder.setBodyParts(request.getBodyParts()); | ||
| } else if (request.getFile() != null) { | ||
| requestBuilder.setBody(request.getFile()); |
There was a problem hiding this comment.
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?
| ExecutionException thrown = assertThrows(ExecutionException.class, | ||
| () -> execute307(c.preparePost(getTargetUrl()).setBody(body))); | ||
|
|
||
| assertInstanceOf(IOException.class, thrown.getCause()); |
There was a problem hiding this comment.
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.
| public void multipart307KeepsBody() throws Exception { | ||
| try (AsyncHttpClient c = asyncHttpClient(config().setFollowRedirect(true))) { | ||
| Response response = c.preparePost(getTargetUrl()) | ||
| .addBodyPart(new StringPart("field", "multipart value")) |
There was a problem hiding this comment.
StringPart is replayable so this passes either way. The one that breaks is InputStreamPart with a declared length, please add that one.
2 participants
Summary
ByteBuf,InputStream, andFilebodies when following redirects whose policy retains the request body.NettyRequestFactoryselected for the original request.ByteBufownership, form-parameter replay, and multipart replay.Problem
Redirect30xInterceptorrebuilds 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 arraysByteBufInputStreamFileThe redirected request therefore kept its method and Content-Type but sent zero bytes. The
Filecase is especially risky for uploads because the target can accept an apparently valid empty PUT or POST.This is a pre-existing omission. AHC issue #1643 previously fixed the same class of bug for multipart bodies. The copy chain was carried through pull request #1843 without a policy discussion. Focused searches found no existing issue or pull request covering these four representations.
Change
Extract the redirect body copy into one helper and keep its selection order aligned with
NettyRequestFactory.body. This matters because aRequestcan retain aFileorBodyGeneratoralongside another representation; the redirect must copy the representation that the original request actually sent.Composite byte arrays, caller-owned
ByteBufs, and files are repeatable. A resettableInputStream, such asByteArrayInputStream, also replays. A consumed stream that cannot be reset now reaches the existing fail-fast guard added in #2312 and completes the future withIOException; that is preferable to silently succeeding with an empty body.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 change: a non-resettable
InputStreamon a keep-body redirect previously completed successfully after sending an empty redirected request. It now completes the request future exceptionally withIOException. Callers that accidentally relied on the empty request will observe an exception, but the failure is explicit instead of silently losing the configured content.AI disclosure
OpenAI Codex on behalf of Matthias Kurz. The commit includes
Co-Authored-By: OpenAI Codex <codex@openai.com>perAGENTS.md.Test plan
upstream/main, the focused suite reproduced five failures: four body types arrived as zero bytes and a non-resettable stream incorrectly completed successfully../mvnw -pl client -Dtest=RedirectBodyTest teston JDK 11: 12 tests passed, including Netty leak detection../mvnw clean verifyon JDK 11: BUILD SUCCESS (full reactor, including tests, Javadocs, artifact signing, coverage, and Revapi).Generated with OpenAI Codex.