Send the request body again when a redirection keeps the request method - #6348
Send the request body again when a redirection keeps the request method#6348jnbdz wants to merge 1 commit into
Conversation
The body of a QUERY request is buffered and sent again when the request is redirected, but the body of any other request is dropped: a custom redirect handler following a 307 or 308 redirection of a POST or PUT request sends the redirected request without its body, while RFC 9110 requires those redirections to keep the method and the body. Buffer the body of any request whose method can carry one, within the existing max buffered size, and send it again whenever the redirected request keeps the method. When the body exceeds the limit, redirections other than 303 are not followed, as documented for QUERY, since they may need the body. The redirected request now buffers its own body instead of sharing the list of the previous one, which appended the body a second time when the request was redirected again. The default redirect handler policy is unchanged: 307 and 308 redirections of methods other than GET, HEAD and QUERY are still only followed by a custom redirect handler.
|
@jnbdz are you using an LLM to assist your contributions ? |
| /** | ||
| * @return whether the body of a request using {@code method} is sent again when a redirection keeps the method | ||
| */ | ||
| private static boolean canRedirectBody(HttpMethod method) { |
There was a problem hiding this comment.
I think this should be the reverse, we should only redirect bodies when the method is QUERY or POST or PUT
| /** | ||
| * A client following 307 and 308 redirections with the same method, like an HTTP client compliant with RFC 9110 would do. | ||
| */ | ||
| private HttpClientAgent keepMethodRedirectClient() { |
There was a problem hiding this comment.
should be named preservingMethodRedirectClient
| .await(); | ||
| } | ||
|
|
||
| private void testFollowRedirectWithBodyKeepingMethod(HttpMethod method, int statusCode, int bodySize) throws Exception { |
| client.request(opts).compose(req -> { | ||
| req.setFollowRedirects(true); | ||
| req.setChunked(true); | ||
| req.write(chunk1); |
There was a problem hiding this comment.
the first write and end should be composed futures
| handler.tryFail(new VertxException("Cannot follow the " + getMethod() + " redirection: the request body " + | ||
| "exceeds the maximum size that can be buffered for redirections (" + maxRedirectBufferSize + " bytes)", true)); | ||
| next.reset(0); | ||
| return; |
There was a problem hiding this comment.
I am not fan of this return, we can avoid it ?
| for (Buffer b : bodyBuffer) { | ||
| composite.addComponent(true, ((BufferInternal) b).getByteBuf()); | ||
| if (getMethod().equals(next.getMethod()) && canRedirectBody(getMethod())) { | ||
| // The redirection keeps the method, so the body is sent again |
There was a problem hiding this comment.
"preserves" the method, not "keeps"
| @@ -63,6 +63,7 @@ public class HttpClientRequestImpl extends HttpClientRequestBase implements Http | |||
| private String traceOperation; | |||
| private int maxRedirectBufferSize = HttpClientOptions.DEFAULT_MAX_REDIRECT_BUFFERED_SIZE; | |||
| private List<Buffer> bodyBuffer; | |||
There was a problem hiding this comment.
that should be renamed "bufferedBody" instead
| bodyBuffer.add(buff.copy()); | ||
| } | ||
| if (currentLen + buff.length() > maxRedirectBufferSize) { | ||
| bodyBufferDiscarded = true; |
There was a problem hiding this comment.
why do we need to introduce bodyBufferDiscarded insted of using followRedirects ? it is not clear, adding another variable increases the complexity
|
if we only add support for new methods, I don't get why the implementation of HttpClientRequest needs so many changes, can you avoid changing the implementation to keep changes to the strict minimum ? |
|
I also believe that this should be configurable and we should think more about the security implications of such redirections. QUERY introduces a clear semantic of redirecting a payload, for other HTTP methods it is not clearly defined, specially regarding cross origin redirection. So until we have clearly defined and analyzed the security implications of this change, I am not in favor to implement and make this available out of the box. |
2 participants
Motivation
Since #6212 the body of a
QUERYrequest is buffered (withinClientRedirectConfig#setMaxBufferedSize) and sent again when the request is redirected. The body of any other request is still dropped: a custom redirect handler that follows a307or308redirection of aPOSTorPUTrequest, as RFC 9110 requires for those status codes, ends up sending the redirected request with an empty body. This is what quarkusio/quarkus#41751 reports for the Quarkus REST Client, and @vietj said there that an extension of the redirect handling in Vert.x 5 would be welcome if there was demand for it.Changes
GET,HEADandCONNECT) is buffered whilefollowRedirectsis set, within the existing max buffered size, and sent again whenever the redirected request keeps the method. This is the QUERY mechanism generalised, nothing else changes for QUERY;303are not followed, as already documented for QUERY, since they may keep the method and need the body. A303still turns the request into a bodylessGETand is followed;307and308redirections of methods other thanGET,HEADandQUERYare still only followed by a custom handler. Javadoc and thehttp.adocredirection section are updated accordingly.Tests
Five tests in
HttpTestuse a redirect handler that keeps the method on307/308:POSTon307andPUTon308with a body, a chunkedPOSTwritten in two buffers, a body over the limit on307(the307is returned to the caller) and a body over the limit on303(followed asGET). The first four fail onmaster, all pass with this change, and the existingtestFollowRedirect*tests as well asHttp1xTest,Http2TestandHttp1xTLSTest(1024 tests) pass on both HTTP versions.