-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix(netty): preserve QUERY across redirects #2317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,7 @@ | |
| import static org.asynchttpclient.util.HttpConstants.Methods.GET; | ||
| import static org.asynchttpclient.util.HttpConstants.Methods.HEAD; | ||
| import static org.asynchttpclient.util.HttpConstants.Methods.OPTIONS; | ||
| import static org.asynchttpclient.util.HttpConstants.Methods.QUERY; | ||
| import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.FOUND_302; | ||
| import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.MOVED_PERMANENTLY_301; | ||
| import static org.asynchttpclient.util.HttpConstants.ResponseStatusCodes.PERMANENT_REDIRECT_308; | ||
|
|
@@ -116,11 +117,20 @@ public boolean exitAfterHandlingRedirect(Channel channel, NettyResponseFuture<?> | |
| future.setScramContext(null); | ||
|
|
||
| String originalMethod = request.getMethod(); | ||
| boolean switchToGet = !originalMethod.equals(GET) && | ||
| !originalMethod.equals(OPTIONS) && | ||
| !originalMethod.equals(HEAD) && | ||
| (statusCode == MOVED_PERMANENTLY_301 || statusCode == SEE_OTHER_303 || statusCode == FOUND_302 && !config.isStrict302Handling()); | ||
| boolean keepBody = statusCode == TEMPORARY_REDIRECT_307 || statusCode == PERMANENT_REDIRECT_308 || statusCode == FOUND_302 && config.isStrict302Handling(); | ||
| boolean isQuery = QUERY.equals(originalMethod); | ||
| boolean methodAlreadyPreserved = originalMethod.equals(GET) || | ||
| originalMethod.equals(OPTIONS) || originalMethod.equals(HEAD); | ||
| boolean strict302 = statusCode == FOUND_302 && config.isStrict302Handling(); | ||
| // RFC 10008 section 2.5 excludes QUERY from the legacy POST-to-GET behavior. | ||
| boolean queryRedirect = isQuery && | ||
| (statusCode == MOVED_PERMANENTLY_301 || statusCode == FOUND_302); | ||
| boolean legacyRedirectToGet = statusCode == MOVED_PERMANENTLY_301 || | ||
|
Comment on lines
+123
to
+127
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd keep this one. Your follow up also makes PUT, PATCH and DELETE keep their bodies on 301 and 302, and that's a much bigger change to behaviour that's been there since 2015. QUERY is new so nobody depends on it yet, it's the safer one to go first. Also for the squash message, the PR body mentions a |
||
| (statusCode == FOUND_302 && !strict302); | ||
| boolean switchToGet = !methodAlreadyPreserved && | ||
| (statusCode == SEE_OTHER_303 || (!isQuery && legacyRedirectToGet)); | ||
| boolean keepBody = queryRedirect || | ||
| statusCode == TEMPORARY_REDIRECT_307 || statusCode == PERMANENT_REDIRECT_308 || | ||
|
Comment on lines
+130
to
+132
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sends the QUERY body to whatever host the 301 points at. We strip
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine by me, let's keep the existing trust model here. Two things I got wrong above: 301 and 308 are the heuristically cacheable ones, not 301 and 302, and we already replay on 308, so caching isn't the difference. And forget the sameBase idea, you were right, it would keep the method and drop the body. My worry is just that 301 and 302 are what a redirect helper emits by default, so that's what an open redirect gives you, while 307 and 308 have to be asked for. But that's about the interceptor in general, not this PR. I'll open a separate issue.
hyperxpro marked this conversation as resolved.
|
||
| strict302; | ||
|
|
||
| HttpHeaders responseHeaders = response.headers(); | ||
| String location = responseHeaders.get(LOCATION); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |
| import java.util.concurrent.atomic.AtomicReference; | ||
|
|
||
| import static org.asynchttpclient.Dsl.basicAuthRealm; | ||
| import static org.asynchttpclient.util.HttpConstants.Methods.QUERY; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
|
|
@@ -65,6 +66,11 @@ public class RedirectCredentialSecurityTest { | |
| private static final AtomicReference<String> cookieOn307Target = new AtomicReference<>(); | ||
| private static final AtomicReference<String> authOn308Target = new AtomicReference<>(); | ||
| private static final AtomicReference<String> bodyOn308Target = new AtomicReference<>(); | ||
| private static final AtomicReference<String> query301AuthOnTarget = new AtomicReference<>(); | ||
| private static final AtomicReference<String> query301CookieOnTarget = new AtomicReference<>(); | ||
| private static final AtomicReference<String> query301ContentTypeOnTarget = new AtomicReference<>(); | ||
| private static final AtomicReference<String> query301MethodOnTarget = new AtomicReference<>(); | ||
| private static final AtomicReference<String> query301BodyOnTarget = new AtomicReference<>(); | ||
| private static final AtomicReference<String> lastCookieHeaderOnA = new AtomicReference<>(); | ||
| private static final AtomicReference<String> lastCookieHeaderOnB = new AtomicReference<>(); | ||
| private static final AtomicReference<String> cookieAtChainStep2 = new AtomicReference<>(); | ||
|
|
@@ -189,6 +195,24 @@ public static void startServers() throws Exception { | |
| exchange.close(); | ||
| }); | ||
|
|
||
| serverA.createContext("/redirect-query-301-to-b", exchange -> { | ||
| exchange.getRequestBody().readAllBytes(); | ||
| exchange.getResponseHeaders().add("Location", "http://127.0.0.1:" + portB + "/target-query-301"); | ||
| exchange.sendResponseHeaders(301, -1); | ||
| exchange.close(); | ||
| }); | ||
|
|
||
| serverB.createContext("/target-query-301", exchange -> { | ||
| query301AuthOnTarget.set(exchange.getRequestHeaders().getFirst("Authorization")); | ||
| query301CookieOnTarget.set(exchange.getRequestHeaders().getFirst("Cookie")); | ||
| query301ContentTypeOnTarget.set(exchange.getRequestHeaders().getFirst("Content-Type")); | ||
| query301MethodOnTarget.set(exchange.getRequestMethod()); | ||
| query301BodyOnTarget.set(new String(exchange.getRequestBody().readAllBytes(), StandardCharsets.UTF_8)); | ||
| exchange.sendResponseHeaders(200, 0); | ||
| exchange.getResponseBody().close(); | ||
| exchange.close(); | ||
| }); | ||
|
|
||
| // Endpoint reused by the HTTPS-to-HTTP downgrade test (target on server B over plain HTTP) | ||
| serverB.createContext("/target-after-downgrade", exchange -> { | ||
| authAfterHttpsDowngrade.set(exchange.getRequestHeaders().getFirst("Authorization")); | ||
|
|
@@ -511,6 +535,36 @@ void redirect308CrossDomainStripsAuthButPreservesBody() throws Exception { | |
| } | ||
| } | ||
|
|
||
| @Test | ||
| void query301CrossOriginStripsCredentialsAndPreservesRequest() throws Exception { | ||
| DefaultAsyncHttpClientConfig config = new DefaultAsyncHttpClientConfig.Builder() | ||
| .setFollowRedirect(true) | ||
| .build(); | ||
| try (DefaultAsyncHttpClient client = new DefaultAsyncHttpClient(config)) { | ||
| query301AuthOnTarget.set(null); | ||
| query301CookieOnTarget.set(null); | ||
| query301ContentTypeOnTarget.set(null); | ||
| query301MethodOnTarget.set(null); | ||
| query301BodyOnTarget.set(null); | ||
|
|
||
| client.prepare(QUERY, "http://127.0.0.1:" + portA + "/redirect-query-301-to-b") | ||
| .setHeader("Authorization", "Bearer secret-token") | ||
| .setHeader("Cookie", "session=secret-session") | ||
| .setHeader("Content-Type", "application/query") | ||
| .setBody("sensitive-query") | ||
| .execute() | ||
| .get(5, TimeUnit.SECONDS); | ||
|
|
||
| assertNull(query301AuthOnTarget.get(), | ||
| "Authorization must be stripped on a cross-origin QUERY redirect"); | ||
| assertNull(query301CookieOnTarget.get(), | ||
| "Cookie must be stripped on a cross-origin QUERY redirect"); | ||
| assertEquals(QUERY, query301MethodOnTarget.get()); | ||
| assertEquals("application/query", query301ContentTypeOnTarget.get()); | ||
| assertEquals("sensitive-query", query301BodyOnTarget.get()); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Cross-domain redirect (different port) must strip a user-supplied Cookie header. | ||
| * Regression test for GHSA-fmxf-pm6p-7xgm. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.