fix(netty): preserve QUERY across redirects by mkurz · Pull Request #2317 · AsyncHttpClient/async-http-client

mkurz · 2026-08-26T00:04:04Z

Summary

Preserve the QUERY method and request content when following 301, 302, 307, and 308 redirects, as required by RFC 10008.
Keep the existing 303 behavior, which follows the redirect with GET and drops the request content.
Add a HttpConstants.Methods.QUERY constant and name the redirect-policy decisions explicitly.
Cover every QUERY redirect status, strict and non-strict 302 handling, existing POST/PUT/PATCH/DELETE behavior, repeatable and non-repeatable generated bodies, and cross-origin redirects.

Problem

Redirect30xInterceptor treated every method other than GET, HEAD, and OPTIONS like POST when handling 301 and non-strict 302 responses. As a result, a QUERY request was changed to GET and its query content and Content-Type were dropped.

RFC 10008 section 2.5 explicitly says that the POST-to-GET exceptions for 301 and 302 do not apply to QUERY. A QUERY request must instead be repeated with its content for 301, 302, 307, and 308. Only a 303 response calls for a GET request to the redirect target.

Change

Recognize QUERY in the redirect policy so 301 and 302 retain the original method and body. The existing 307, 308, 303, HEAD, OPTIONS, and POST behavior is unchanged. The implementation uses named decisions rather than embedding the QUERY exception in one compound expression.

Cross-origin QUERY redirects retain their method, content, and Content-Type, as required to repeat the query. This means a QUERY body now crosses origins on 301 and 302 where the old, incorrect GET rewrite dropped it. AHC already uses that body-replay trust model for 307 and 308. Existing redirect security still strips Authorization, Realm credentials, and user-supplied Cookie headers before sending the request to the new origin. A separate, representation-independent policy that refuses cross-origin keep-body redirects could be considered, but this conformance fix does not currently add one.

The broader pre-existing behavior that converts PUT, PATCH, DELETE, and other methods to GET after 301 and non-strict 302 responses is deliberately out of scope. It changes established behavior for existing users and is handled in a separate follow-up branch, fix/non-post-redirects.

That follow-up handles QUERY as an ordinary non-POST method and therefore supersedes this pull request's QUERY-specific interceptor condition if both land. Keeping this narrow pull request separate still allows the standardized QUERY behavior to land even if the broader compatibility change is rejected; the public method constant and QUERY-specific regression coverage remain useful either way.

This adds the public HttpConstants.Methods.QUERY string constant. It is an additive, user-facing API for constructing QUERY requests; there is no incompatible API change.

AI disclosure

OpenAI Codex on behalf of Matthias Kurz. The commits include Co-Authored-By: OpenAI Codex <codex@openai.com> per AGENTS.md.

Test plan

Reproduced the failure on JDK 11 with ./mvnw -pl client -Dtest=RedirectBodyTest#query301KeepsMethodAndBody test.
./mvnw -pl client -Dtest=RedirectBodyTest test on JDK 11.
./mvnw -pl client -Dtest=RedirectBodyTest,RedirectCredentialSecurityTest test on JDK 11: 57 tests passed.
./mvnw clean verify on JDK 11: BUILD SUCCESS (full reactor, including tests, Javadocs, artifact signing, coverage, and Revapi).

Generated with OpenAI Codex.

mkurz · 2026-08-26T00:09:56Z

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.

hyperxpro · 2026-08-26T18:32:12Z

+                        (statusCode == SEE_OTHER_303 || (!isQuery && legacyRedirectToGet));
+                boolean keepBody = queryRedirect ||
+                        statusCode == TEMPORARY_REDIRECT_307 || statusCode == PERMANENT_REDIRECT_308 ||


This sends the QUERY body to whatever host the 301 points at. We strip Authorization and Cookie when the origin changes, but with QUERY the body is the query, so a cached or injected 301 gets the whole thing where before it got an empty GET. 307 and 308 already behave like this, but 301 and 302 are the cacheable ones. Can we require sameBase for the body here, or gate it on a config flag?

Agreed that this needs an explicit policy decision. My preference for this conformance fix is to retain AHC’s existing keep-body redirect policy: AHC already replays request bodies across origins for 307, 308, and strict 302 while stripping credentials.

Requiring sameBase only for QUERY on 301/302 cannot safely mean dropping the body: that would preserve the QUERY method while silently changing the query itself, contrary to RFC 10008.

On the config-flag option: I think that is the right mechanism if we want this restriction, but it should apply uniformly to every cross-origin keep-body redirect, not only QUERY on 301/302, and it should refuse the redirect rather than strip the body. Your caching point is well taken: when a 301/302 response is cached, it can repeatedly cause cross-origin body replay without a new redirect response. That strengthens the case for a configurable cross-origin body-replay policy; I do not think it argues for QUERY following a different policy from every other body-bearing method.

Would you prefer that uniform policy to be designed as part of this PR, or is retaining AHC’s existing trust model acceptable here while we discuss the broader policy separately?

hyperxpro · 2026-08-26T18:32:13Z

+                boolean switchToGet = !methodAlreadyPreserved &&
+                        (statusCode == SEE_OTHER_303 || (!isQuery && legacyRedirectToGet));
+                boolean keepBody = queryRedirect ||
+                        statusCode == TEMPORARY_REDIRECT_307 || statusCode == PERMANENT_REDIRECT_308 ||


The block this feeds, at line 158, covers six of the body kinds. getFile(), getStreamData(), getByteBufData() and getCompositeByteData() all fall through, and QUERY is not POST/PUT/PATCH so we do not set ZERO_CONTENT_LENGTH either. A QUERY with a File body then gets redirected with its Content-Type and no content at all, which is the case this PR is meant to fix. It is pre existing for 307 and 308, but this PR is what makes it reachable for QUERY.

Its order does not match NettyRequestFactory.body() either. That one takes byteData before formParams and bodyParts before bodyGenerator, we do the reverse, so a request carrying both sends a different body after the redirect than it did on the first hop.

This is now fixed by #2316, which has merged into main and is included in this branch after rebasing. Redirect body replay is centralized in copyBody, covers every request-body representation, and follows the same precedence as NettyRequestFactory.body(). The QUERY keep-body path therefore no longer drops File, InputStream, ByteBuf, or composite byte-array bodies or selects a different coexisting representation.

hyperxpro · 2026-08-26T18:32:13Z

+                boolean strict302 = statusCode == FOUND_302 && config.isStrict302Handling();
+                boolean queryRedirect = isQuery &&
+                        (statusCode == MOVED_PERMANENTLY_301 || statusCode == FOUND_302);
+                boolean legacyRedirectToGet = statusCode == MOVED_PERMANENTLY_301 ||


queryRedirect and legacyRedirectToGet are each read once, on the next line. methodAlreadyPreserved also reads backwards: it is false for QUERY, but the method is preserved for QUERY on 301. Did you consider scoping the rewrite to POST instead? RFC 9110 only defines it for POST, so QUERY would need no special case at all. It changes PUT and DELETE too so probably its own PR, but then you are not adding a branch you have to take out again later.

I have a local follow-up branch which already implements this: the legacy rewrite is scoped to POST, PUT/PATCH/DELETE/custom methods are tested on 301 and 302, and the QUERY-specific condition in this PR becomes unnecessary.

There are two possible ways to sequence it: land this narrow PR and follow it with the broader change, or close this PR and open the broader one with the QUERY constant and regression tests carried over. I lean toward the latter given your point about not adding a branch that is removed again, but the narrow PR would allow QUERY conformance to land independently if the compatibility change for existing PUT/PATCH/DELETE users needs more discussion. Which would you prefer?

hyperxpro · 2026-08-26T18:32:13Z

@@ -111,11 +112,19 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
                future.setScramContext(null);

                String originalMethod = request.getMethod();


Can you put the RFC 10008 2.5 reference in a comment here? Every other odd thing in this method says why it is there, and this is the least obvious line in it.

Added the RFC 10008 section 2.5 explanation directly above queryRedirect in d3f4d9a.

hyperxpro · 2026-08-26T18:32:13Z

+                boolean isQuery = QUERY.equals(originalMethod);
+                boolean methodAlreadyPreserved = GET.equals(originalMethod) ||


Nit: these three read originalMethod.equals(GET) before. getMethod() is never null, so can we leave them as they were and keep the diff to the actual change?

Done in 8171795. The three pre-existing comparisons use originalMethod.equals(...) again; only the new QUERY comparison retains its new form.

hyperxpro · 2026-08-26T18:32:13Z

        public static final String PATCH = HttpMethod.PATCH.name();
        public static final String POST = HttpMethod.POST.name();
        public static final String PUT = HttpMethod.PUT.name();
+        public static final String QUERY = "QUERY";


Netty 4.2.17 already has HttpMethod.QUERY, so this can be derived like the others.

Suggested change

public static final String QUERY = "QUERY";

public static final String QUERY = HttpMethod.QUERY.name();

Done in f6a09ef: Methods.QUERY is now derived from HttpMethod.QUERY.name().

hyperxpro · 2026-08-26T18:32:13Z

    }
+
+    @RepeatedIfExceptionsTest(repeats = 5)
+    public void regular308KeepsBody() throws Exception {


The condition you rewrote also drives PUT, PATCH and DELETE, but every new test is POST or QUERY. One parameterized test over those three on 301 and 302 would pin the behavior the PR says it is not touching.

Added in a977a51. The parameterized test covers PUT, PATCH, and DELETE across both 301 and non-strict 302 and asserts the current GET rewrite, empty body, and removed Content-Type. If the broader POST-scoped rewrite lands later, these assertions will require an explicit update.

hyperxpro · 2026-08-26T18:32:13Z

+    }
+
+    @RepeatedIfExceptionsTest(repeats = 5)
+    public void query301KeepsRepeatableBodyGenerator() throws Exception {


Can we get one with a non repeatable generator too? A QUERY on 301 used to go out as a bodiless GET and complete, now it replays a consumed stream and fails the future through the guard from #2312. ByteArrayBodyGenerator is repeatable so this test does not see it.

Added in 69393b5. The test uses a non-empty, non-resettable InputStreamBodyGenerator and asserts the exact replay-guard IOException. Before the QUERY change, the request became a bodyless GET and completed, so the test discriminates the changed behavior.

hyperxpro · 2026-08-26T18:32:13Z

+                    "Cookie must be stripped on a cross-origin QUERY redirect");
+            assertEquals(QUERY, query301MethodOnTarget.get());
+            assertEquals("application/query", query301ContentTypeOnTarget.get());
+            assertEquals("sensitive-query", query301BodyOnTarget.get());


This locks in the cross origin body replay I asked about in the interceptor. If we keep it that is fine, but I would rather decide that first than have a test assert it.

Agreed. The test is intended to make the selected policy explicit, not decide it implicitly. If we retain AHC’s existing cross-origin keep-body policy, it should remain because it verifies both request preservation and credential stripping. If you prefer the uniform configuration discussed in the interceptor thread to be added here, I will update the implementation and this test together.

RFC 10008 requires QUERY requests to retain their method and content across 301, 302, 307, and 308 redirects. AHC treated QUERY like POST on 301 and non-strict 302, changing it to GET and dropping its content. Preserve QUERY while keeping the established POST and 303 behavior. Expose the standardized method constant and cover every redirect status, strict 302, repeatable bodies, and cross-origin credential stripping. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

Point the non-obvious QUERY exception at the RFC section that requires preserving its method and content across 301 and 302. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

Keep the existing original-method comparison style so the redirect policy diff contains only behavior needed for QUERY. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

Use Netty's standardized QUERY method constant just like the other HTTP method strings instead of duplicating its literal value. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

Exercise PUT, PATCH, and DELETE across 301 and 302 so the narrow QUERY change cannot accidentally alter their established GET rewrite. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

Pin the compatibility consequence of preserving QUERY on 301: a consumed body generator that cannot reset now fails explicitly instead of completing as a bodyless GET. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

mkurz · 2026-09-01T09:28:01Z

@hyperxpro I have addressed all concrete review feedback in five follow-up commits and rerun ./mvnw clean verify successfully. The implementation is ready from my side.

Two policy decisions remain:

For cross-origin body replay, my preference is to retain AHC’s existing keep-body policy in this conformance fix and discuss a representation-independent option that refuses all cross-origin keep-body redirects separately.
For scope, I have a local branch that limits the legacy rewrite to POST and therefore subsumes the QUERY-specific interceptor condition here. I lean toward replacing this PR with that broader change, but keeping this PR allows QUERY conformance to land independently if the PUT/PATCH/DELETE compatibility change needs more discussion.

Please let me know which scope you prefer and whether retaining the existing cross-origin policy is acceptable here.

mkurz mentioned this pull request Aug 26, 2026

Add HTTP QUERY method support (RFC 10008) playframework/play-ws#1366

Merged

6 tasks

hyperxpro requested changes Aug 26, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

mkurz force-pushed the fix/query-redirects branch from b8c4db4 to 4596aa0 Compare August 31, 2026 22:44

mkurz requested a review from hyperxpro August 31, 2026 22:51

This comment was marked as outdated.

Sign in to view

mkurz and others added 5 commits September 1, 2026 09:38

Document QUERY redirect rationale

d3f4d9a

Point the non-obvious QUERY exception at the RFC section that requires preserving its method and content across 301 and 302. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

Minimize redirect condition changes

8171795

Keep the existing original-method comparison style so the redirect policy diff contains only behavior needed for QUERY. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

Derive QUERY from Netty

f6a09ef

Use Netty's standardized QUERY method constant just like the other HTTP method strings instead of duplicating its literal value. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

Pin existing non-QUERY redirect behavior

a977a51

Exercise PUT, PATCH, and DELETE across 301 and 302 so the narrow QUERY change cannot accidentally alter their established GET rewrite. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>

		@@ -111,11 +112,19 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?>
		future.setScramContext(null);

		String originalMethod = request.getMethod();

		boolean isQuery = QUERY.equals(originalMethod);
		boolean methodAlreadyPreserved = GET.equals(originalMethod) \|\|

	public static final String QUERY = "QUERY";
	public static final String QUERY = HttpMethod.QUERY.name();

Uh oh!

Conversation

mkurz commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Change

AI disclosure

Test plan

Uh oh!

mkurz commented Aug 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkurz Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

mkurz commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mkurz commented Aug 26, 2026 •

edited

Loading

mkurz Sep 1, 2026 •

edited

Loading