Skip to content

Use original method in redirects for HEAD / OPTIONS requests - #1736

Merged
TomGranot merged 1 commit into
AsyncHttpClient:masterfrom
TomGranot:bugfix/setfollowredirect-follow-the-method
Jan 5, 2021
Merged

TomGranot merged 1 commit into
AsyncHttpClient:masterfrom
TomGranot:bugfix/setfollowredirect-follow-the-method

Conversation

@TomGranot

Copy link
Copy Markdown
Contributor

Originally, when setFollowRedirect is set to true (like in Head302Test), the expected flow is HEAD --> GET (based on the behaviour of Redirect30xInterceptor - see 3c25a42 for the reasoning).

Following a change to the interceptor (to fix #1728), Head302Test started going on an infinite loop caused by the way the handler in the test was set up (to account for the original HEAD --> GET flow and not the new HEAD --> HEAD --> HEAD.... flow).

This PR does 3 things:

  • Changes Redirect30xInterceptor to set all redirects of a HEAD request to be HEADs as well
  • Change Head302Test to account for the new flow
  • Notes a flaky test in AsyncStreamHandlerTest that was found during work on the PR

Originally, when setFollowRedirect is set to true (like in Head302Test), the expected flow is HEAD --> GET (based on the behaviour of Redirect30xInterceptor - see AsyncHttpClient@3c25a42 for the reasoning).

Following a change to the interceptor (to fix AsyncHttpClient#1728), Head302Test started going on an infinite loop caused by the way the handler in the test was set up (to account for the original HEAD --> GET flow and not the new HEAD --> HEAD --> HEAD.... flow).

This PR does 3 things:

* Changes Redirect30xInterceptor to set all redirects of a HEAD request to be HEADs as well
* Change Head302Test to account for the new flow
* Notes a flaky test in AsyncStreamHandlerTest that was found during work on the PR
@TomGranot

TomGranot commented Sep 28, 2020

Copy link
Copy Markdown
Contributor Author

Failing tests:

Tests run: 885, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 196.279 sec <<< FAILURE! - in TestSuite
testMaxTotalConnections(org.asynchttpclient.channel.MaxTotalConnectionTest)  Time elapsed: 0.103 sec  <<< FAILURE!
java.lang.AssertionError: expected [null] but found [java.net.UnknownHostException: DNS name not found [response code 3]]
	at org.asynchttpclient.channel.MaxTotalConnectionTest.testMaxTotalConnections(MaxTotalConnectionTest.java:108)
asyncOptionsTest(org.asynchttpclient.AsyncStreamHandlerTest)  Time elapsed: 0.303 sec  <<< FAILURE!
java.lang.AssertionError: expected [4] but found [5]
	at org.asynchttpclient.AsyncStreamHandlerTest.lambda$null$17(AsyncStreamHandlerTest.java:461)
	at org.asynchttpclient.AsyncStreamHandlerTest.lambda$asyncOptionsTest$18(AsyncStreamHandlerTest.java:437)
	at org.asynchttpclient.AsyncStreamHandlerTest.asyncOptionsTest(AsyncStreamHandlerTest.java:436)

The first one does not fail locally - looks like something on the build server itself. @slandelle if you can re-run it I'd appreciate it.

The second one is flaky (#1735).

@TomGranot

Copy link
Copy Markdown
Contributor Author

@slandelle If you get a minute to re-jig travis-ci that would be great - see my comments above re the tests.

@TomGranot
TomGranot merged commit bd7b5bd into AsyncHttpClient:master Jan 5, 2021
@TomGranot
TomGranot deleted the bugfix/setfollowredirect-follow-the-method branch January 5, 2021 07:26
hyperxpro pushed a commit that referenced this pull request Sep 9, 2026
## Summary

- Limit AHC's historical 301/302 POST-to-GET rewrite to POST requests.
- Keep request content whenever a redirect preserves the request method,
except for 303's explicit body-dropping behavior.
- Validate only the body representation that AHC selected for
transmission and reject consumed, non-resettable streams before
connecting to the redirect target.
- Cover standard methods, extension methods, caller-added redirect
statuses, and cross-origin behavior.

## Problem

`Redirect30xInterceptor` applied the historical POST-to-GET behavior for
301 and non-strict 302 responses to every method other than GET, HEAD,
and OPTIONS. A PUT, PATCH, DELETE, or extension-method request therefore
became a bodyless GET after either redirect.

The body decision was also coupled to a fixed list of methods and status
codes. GET, HEAD, and OPTIONS requests can mechanically carry bodies in
AHC, but those bodies were dropped on 301 and 302 even though the
methods were retained. A caller-added status in the public mutable
`REDIRECT_STATUSES` set likewise retained the method while silently
dropping its body.

[RFC 9110 sections
15.4.2](https://www.rfc-editor.org/rfc/rfc9110.html#section-15.4.2) and
[15.4.3](https://www.rfc-editor.org/rfc/rfc9110.html#section-15.4.3)
scope the compatibility allowance to changing POST to GET. They do not
permit rewriting every other method in the same way. [RFC 10008 section
2.5](https://www.rfc-editor.org/rfc/rfc10008.html#section-2.5)
explicitly requires QUERY not to use the POST exceptions.

## Change

Apply the legacy 301/302 rewrite only when the original method is POST.
Derive body handling from the method decision: keep the body whenever
the method is preserved, except for 303, which drops the body
regardless. This differs deliberately from checking whether a request is
"not POST": a POST receiving a caller-added status such as 300 also
keeps its method and must therefore keep its body.

The resulting behavior is:

- POST on 301 or non-strict 302: switch to GET and drop the body.
- POST on strict 302: retain POST and the body.
- PUT, PATCH, DELETE, QUERY, and extension methods on 301 or 302: retain
the method and body.
- GET, HEAD, and OPTIONS on 301 or 302: retain the method and any
explicitly attached body.
- 303: drop the body; methods that require rewriting switch to GET,
while GET, HEAD, and OPTIONS remain unchanged.
- 307 and 308: retain the method and body under the existing policy.
- Caller-added redirect statuses: retain the body whenever the method is
retained.

This is a deliberate compatibility change. It corrects the standards
scope and removes method-preserving, body-dropping combinations while
retaining the long-established POST behavior for 301 and non-strict 302.

## Redirect-body validation

The replay checks now use one private `BodyRepresentation` selection
that mirrors `NettyRequestFactory.body()` precedence. This matters
because some request-builder setters leave lower-priority
representations in place. Validation now examines only the body AHC
selected for transmission; for example, a stale multipart
`InputStreamPart` no longer rejects a redirect whose selected body is a
byte array.

Selected raw `InputStream` bodies and `InputStreamBodyGenerator`
instances that report no mark/reset support are rejected before AHC
connects to the redirect target only if the stream has already been
consumed. An early redirect in response to `Expect: 100-continue` can
leave the stream untouched, so the target can still receive its first
transmission without reset support. This preflight is intentionally
partial: a stream may report mark support yet be closed or fail to reset
after the first send. The existing write-time replay guard remains the
final authority for those cases. A generic `BodyGenerator` can also
produce an unknown-length or non-repeatable body, but discovering that
would require calling its one-shot `createBody()` early, so its existing
write-time behavior is unchanged.

## Redirect security

For a cross-origin 301 or 302, newly preserved request content is
replayed to the redirect target. This follows the body-replay trust
model AHC already uses for strict 302, 307, and 308. Existing redirect
security continues to strip Authorization, Proxy-Authorization, Realm
credentials, user-supplied Cookie headers, and Cookie objects when the
origin changes. Tests assert that credentials reach the original server,
do not reach the target, and that the method, content type, and body
reach the target intact.

The same consideration applies to HTTPS-to-HTTP redirects: this pull
request can replay content on 301 or 302 that the previous method/body
rewrite discarded. Gating only `keepBody` on a scheme downgrade would
preserve the method while silently deleting its payload, reproducing the
data-corruption shape this change removes. If AHC adopts a downgrade
restriction, it should refuse the redirect itself and apply uniformly to
all keep-body statuses, including strict 302, 307, and 308. That
transport-policy decision is left to a focused follow-up; this pull
request does not alter AHC's existing downgrade policy.

## History checked

The broad conversion is established behavior rather than a recent
accident. Issue #989 requested browser-compatible 301 handling, issue
#1042 retained the body drop for POST, and pull request #1736 later
exempted HEAD and OPTIONS from method rewriting. This change retains the
established POST rule while making body preservation follow the
resulting method decision. A search did not find an existing issue or
pull request specifically correcting PUT, PATCH, DELETE, extension
methods, or caller-added redirect statuses.

## Compatibility

There is no public API change.

The following redirect behavior changes intentionally:

- PUT, PATCH, DELETE, and extension methods on 301 and non-strict 302
retain their original method and content instead of becoming bodyless
GET requests.
- GET, HEAD, and OPTIONS requests with explicitly attached content
retain it on 301 and 302 instead of sending a bodyless second request.
- Requests followed through caller-added redirect statuses retain
content whenever their method is retained, including POST on a
registered 300.
- A selected raw `InputStream` or `InputStreamBodyGenerator` that has
already been consumed and lacks mark/reset support now fails before
connecting to the redirect target. For newly preserved non-POST 301/302
requests, this replaces the previous silent success as a bodyless GET
with an explicit replay failure. Existing keep-body redirects fail
earlier and use the new redirect-level error message. Untouched streams
deferred by `Expect: 100-continue` remain usable at the redirect target.
Streams that advertise mark support but cannot actually reset still fail
at write time.
- A request with a replayable selected body and a stale, lower-priority
non-replayable representation no longer fails validation.
- Cross-origin and HTTPS-to-HTTP 301/302 redirects can now receive
content that the old body-dropping behavior suppressed; credential
stripping remains unchanged.

POST on 301 and non-strict 302, strict-302 method policy, 303 method
policy, and 307/308 method policy remain unchanged.

## 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

- [x] The new focused regressions reproduced eight body-loss failures
for GET, HEAD, OPTIONS, and caller-added 300 before the generalized body
rule was applied.
- [x] The selected-body coexistence regression reproduced the stale
multipart `InputStreamPart` false rejection before validation was
aligned with outbound-body precedence.
- [x] Both deferred-stream regressions failed with the unconditional
preflight and pass with the consumption check. They assert no stream
reads or body bytes at the origin before its 307 response and exact body
bytes at the target, for raw `InputStream` and
`InputStreamBodyGenerator` bodies.
- [x] `./mvnw -pl client
-Dtest=RedirectBodyTest,RedirectCredentialSecurityTest test` on JDK 11:
73 tests passed.
- [x] `./mvnw clean verify` on JDK 11: BUILD SUCCESS (full reactor,
including tests, Javadocs, artifact signing, coverage, and Revapi).

Generated with OpenAI Codex.

---------

Co-authored-by: OpenAI Codex <codex@openai.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When Request Method is "HEAD" and setFollowRedirect is set to true. The method of the second redirect request is GET.

1 participant