Rest client 307 redirect doesn't keep original request headers

[jdussouillez]

Describe the bug

The bug is following issue #34644

With Quarkus v3.2.1+, a POST request done on an endpoint responding HTTP 307 now sends a POST request on the new URL. The problem now is that the original request headers are lost and not sent to the new URL.

It works fine with curl and Postman. Unfortunately I couldn't find any documentation about headers after redirect in the RFCs.

Expected behavior

When sending the new request after a redirect, the HTTP method and the headers must be kept from the original request (method is OK since #34644 but headers are lost).

-> POST /token, headers=[H1, H2]
<- HTTP 307, Location=/token2
-> POST /token2, headers=[H1, H2]

Actual behavior

The headers are not sent in the request after the redirect.

-> POST /token, headers=[H1, H2]
<- HTTP 307, Location=/token2
-> POST /token2, headers=[]

How to Reproduce?

Reproducer: https://github.com/jdussouillez/quarkus-rest-client-post-redirect-post/tree/missing-headers

Same as #34644 but use missing-headers branch.

The server requires "X-Api-Version" header or will fail with "Bad request". Using curl it works fine:

Using Quarkus Rest client it fails with 400 because the "X-Api-Version" header wasn't sent in the second request.

Output of uname -a or ver

No response

Output of java -version

openjdk version "17.0.7"

GraalVM version (if different from Java)

No response

Quarkus version or git rev

3.2.2.Final

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[quarkus-bot]

/cc @Sgitario (rest-client), @cescoffier (rest-client), @geoand (rest-client)

[geoand]

I wonder what the spec says about this... I don't recall ever hearing about header handling although it does sound reasonable

[jdussouillez]

Yeah I looked in both RFCs mentioned here but nothing says if the headers must be sent again or not.

[cescoffier]

I don't think we can forward all headers blindly. Some may contain sensible content which should not be forwarded. So, it would require a list of the headers to not forward (or a more prescriptive approach where we list the headers to forward, but that could be annoying)

[jdussouillez]

So, it would require a list of the headers to not forward (or a more prescriptive approach where we list the headers to forward, but that could be annoying)

@geoand imagined a more flexible solution (but a bit heavy) here by using an AdvancedRedirectHandler to customize the request sent after the redirect, using the original request and redirect response informations. I guess headers could also be overridden with this solution.

Something like:

// ...
RedirectRequest of(ResteasyReactiveClientRequestContext reqContext, Response redirectResponse) {
    return new RedirectResponse() {
        @Override
        public URI uri() {
            return null; // null means the default value (redirectResponse's "Location" header value)
        }

        @Override
        public String httpMethod() {
            return null; // null means the original HTTP method (reqContext.getMethod())
        }

        @Override
        public MultivaluedMap<String, Object> headers() {
            return null; // null means the request headers (reqContext.getHeaders()) but you can remove/add stuff here
        }
    };
}
// ...

[geoand]

Right, that design would allow for such a use case.

I'll leave it to @Sgitario and @cescoffier to decide if it's complexity is warranted.

[cescoffier]

I think it's a good approach. I'm fearing we start introducing dozens of parameters to capture everything, while a single programmatic approach would work.

[Sgitario]

why would we need the AdvancedRedirectHandler implementation? We could provide default methods in the existing RedirectHandler like:

public interface RedirectHandler {
    int DEFAULT_PRIORITY = 5000;

    URI handle(Response response);

    default int getPriority() {
        return Optional.ofNullable(this.getClass().getAnnotation(Priority.class)).map(Priority::value).orElse(DEFAULT_PRIORITY);
    }

    default Set<String> propagateHeaders() {
        return Set.of();
    }

    default MultivaluedMap<String, Object> addHeaders(MultivaluedMap<String, Object> requestHeaders) {
        return new MultivaluedHashMap<>();
    }
}

I've added two new methods:

• propagateHeaders to explicitly set the header names to propagate from the header
• addHeaders to add headers (we could also provide here the request headers)

[geoand]

That could certainly be an option, I would personally not prefer it since it is almost certainly not what we would do if we were designing the API from scratch.

[jdussouillez]

Any updates?

[geoand]

This slipped through the cracks unfortunately

…

On Tue, Oct 3, 2023, 11:08 Junior Dussouillez ***@***.***> wrote: Any updates? — Reply to this email directly, view it on GitHub <#35126 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABBMDP7FYK3Z2TRWACOUNCTX5PIXVAVCNFSM6AAAAAA27HYH72VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBUGU2DSNJRG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jdussouillez]

Bumping. It would be nice to have this (to remove manual HTTP requests and use REST client instead)

[geoand]

I'll try and check this soon

[geoand]

Can you try #41485?

It introduces the new AdvancedRedirectHandler which you can be used as shown in the new AdvancedRedirectTest

[jdussouillez]

@geoand Thanks, the AdvancedRedirectHandler works fine!

See my reproducer's "quarkus-v3.13" branch

---

But now I see another issue: the POST body is not sent to the redirect request. Note that it is also an issue with RedirectHandler's URI handle(Response response) method.

Is there a way to do this using the new AdvancedRedirectHandler? I guess it's not that easy because Vert.x RequestOptions does not handle body, only method, headers, URL, etc.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307

The method and the body of the original request are reused to perform the redirected request.
[...]
The only difference between 307 and 302 is that 307 guarantees that the method and the body will not be changed when the redirected request is made.

Reproducer: https://github.com/jdussouillez/quarkus-rest-client-post-redirect-post/tree/quarkus-v3.13 (client gets a 400 instead of 200 because body is null)

[geoand]

If Vert'x is not handling doing this, there is not much we can do in Quarkus - see that AdvancedRedirectHandler is just used to setup a Vert.x object which is set to io.vertx.core.http.HttpClientBuilder#withRedirectHandler

[jdussouillez]

I created #41751 as this issue is closed and the new feature works fine.