KTOR-8697 Add fallback when the redirect response has no Location header #5050
Conversation
WalkthroughConsolidates redirect handling inside the loop, adds explicit handling for missing Location headers, moves redirect event emission after validating Location, adds HTTPS downgrade guard and Authorization header stripping on cross-authority redirects, and expands tests and test server with a route lacking Location to validate fallback behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
ktor-test-server/src/main/kotlin/test/server/tests/Redirect.kt (1)
66-68: Route to simulate missing Location header looks good.This gives us a deterministic 302 without
Locationto exercise the client fallback. Tiny nit: consider being explicit about content type for clarity in test logs.Apply if you want the response to explicitly advertise text/plain:
- call.respondText("withoutLocationHeader", status = HttpStatusCode.Found) + call.respondText( + text = "withoutLocationHeader", + status = HttpStatusCode.Found + )ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.kt (1)
157-163: Assert the request URL to prove no redirect attempt happened.Strengthen the test by asserting we stayed on the original URL (helps prevent regressions that silently attempt a follow-up to some default host).
Apply this diff:
client.prepareGet("$TEST_URL_BASE/withoutLocationHeader").execute { assertEquals(HttpStatusCode.Found, it.status) assertEquals("withoutLocationHeader", it.bodyAsText()) + assertEquals( + "$TEST_URL_BASE/withoutLocationHeader", + it.call.request.url.toString(), + "The client should return the original call without attempting a redirect" + ) }ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRedirect.kt (2)
74-77: Use the actual request URL in the log for missing Location.
context.urlalways points to the initial request; for multi-hop chains this can confuse tracing. Prefer the current call’s URL.Apply this diff:
- if (location == null) { - LOGGER.trace("Received redirect response with no location header for request ${context.url}") - return call - } + if (location == null) { + LOGGER.trace("Received redirect response with no Location header for request ${call.request.url}") + return call + }
79-81: Log the source of the redirect using the current URL.This improves clarity when debugging multi-hop redirects.
Apply this diff:
- client.monitor.raise(HttpResponseRedirectEvent, call.response) - LOGGER.trace("Received redirect response to $location for request ${context.url}") + client.monitor.raise(HttpResponseRedirectEvent, call.response) + LOGGER.trace("Received redirect response to $location for request ${call.request.url}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRedirect.kt(1 hunks)ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.kt(1 hunks)ktor-test-server/src/main/kotlin/test/server/tests/Redirect.kt(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{kt,kts}: Follow Kotlin official style guide
Use star imports for io.ktor.* packages (configured in .editorconfig)
Max line length: 120 characters
Document all public APIs including parameters, return types, and exceptions
Mark internal APIs with @internalapi annotation
Error handling follows Kotlin conventions with specific Ktor exceptions
Files:
ktor-test-server/src/main/kotlin/test/server/tests/Redirect.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRedirect.kt
**/*.{kt,kts,json,yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
Indent: 4 spaces (2 for JSON, YAML)
Files:
ktor-test-server/src/main/kotlin/test/server/tests/Redirect.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRedirect.kt
**/test/**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
Tests organized by platform in module's test directories
Files:
ktor-test-server/src/main/kotlin/test/server/tests/Redirect.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.kt
**/test/**/*Test.kt
📄 CodeRabbit inference engine (CLAUDE.md)
Follow test naming pattern: DescribeWhatIsBeingTested
Files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.kt
🧬 Code graph analysis (2)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.kt (1)
ktor-client/ktor-client-test-base/common/src/io/ktor/client/test/base/ClientLoader.kt (1)
clientTests(33-57)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRedirect.kt (1)
ktor-client/ktor-client-core/common/src/io/ktor/client/request/HttpRequest.kt (1)
takeFromWithExecutionContext(172-176)
🔇 Additional comments (3)
ktor-test-server/src/main/kotlin/test/server/tests/Redirect.kt (1)
7-7: Import addition is correct and scoped.
io.ktor.http.*is required forHttpStatusCode.Found; no issues.ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/HttpRedirectTest.kt (1)
151-156: Enabling HttpRedirect in this test is essential; good catch.The configuration mirrors real usage and ensures we exercise the plugin path.
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/HttpRedirect.kt (1)
70-71: Redirect condition centralized inside the loop — good.Avoids redundant checks and keeps exit logic in one place.
| var call = origin | ||
| var requestBuilder = context | ||
| val originProtocol = origin.request.url.protocol | ||
| val originAuthority = origin.request.url.authority | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compare against the previous hop, not the original, for HTTPS downgrade and Authorization stripping.
Using originProtocol/originAuthority (captured once before the loop) can miss security-relevant transitions later in the chain:
- HTTPS downgrade: a chain like HTTP → HTTPS → HTTP won’t be blocked if the very first hop was HTTP.
- Authorization leakage: if a redirect goes A → B (auth removed) then B → A, the current code compares the new target to A (the origin) and may keep Authorization that was added for B.
Safer behavior is to compare the “next” target to the protocol/authority of the current hop (call.request.url).
Apply this diff:
var call = origin
var requestBuilder = context
- val originProtocol = origin.request.url.protocol
- val originAuthority = origin.request.url.authority
while (true) {
+ // Protocol/authority of the current hop
+ val previousProtocol = call.request.url.protocol
+ val previousAuthority = call.request.url.authority
if (!call.response.status.isRedirect()) return call
val location = call.response.headers[HttpHeaders.Location]
if (location == null) {
- LOGGER.trace("Received redirect response with no location header for request ${context.url}")
+ LOGGER.trace("Received redirect response with no Location header for request ${call.request.url}")
return call
}
client.monitor.raise(HttpResponseRedirectEvent, call.response)
- LOGGER.trace("Received redirect response to $location for request ${context.url}")
+ LOGGER.trace("Received redirect response to $location for request ${call.request.url}")
requestBuilder = HttpRequestBuilder().apply {
takeFromWithExecutionContext(requestBuilder)
url.parameters.clear()
url.takeFrom(location)
/**
* Disallow redirect with a security downgrade.
*/
- if (!allowHttpsDowngrade && originProtocol.isSecure() && !url.protocol.isSecure()) {
- LOGGER.trace("Can not redirect ${context.url} because of security downgrade")
- return call
- }
+ if (!allowHttpsDowngrade && previousProtocol.isSecure() && !url.protocol.isSecure()) {
+ LOGGER.trace("Blocked redirect from ${call.request.url} to $location due to HTTPS downgrade")
+ return call
+ }
- if (originAuthority != url.authority) {
+ if (previousAuthority != url.authority) {
headers.remove(HttpHeaders.Authorization)
- LOGGER.trace("Removing Authorization header from redirect for ${context.url}")
+ LOGGER.trace(
+ "Removing Authorization header for cross-authority redirect: " +
+ "${previousAuthority} -> ${url.buildString()}"
+ )
}
}
call = proceed(requestBuilder)
}Also applies to: 90-98
|
a flaky test caused the failure. 😅 |
Subsystem
Client, HttpRedirect
Motivation
KTOR-8697 HttpRedirect: The client is redirected when no Location header in response
Currently, if the
HttpRedirectplugin receives a response with a redirect status code (e.g., 302 Found) but noLocationheader, it incorrectly attempts to redirect tolocalhost.The expected behavior is to halt the redirection process and return the response directly to the user, allowing them to handle it.
Solution
The solution modifies the
HttpRedirectplugin to gracefully handle cases where aLocationheader is missing from a redirect response.Locationheader at the beginning of the redirection logic.HttpClientCallinstead of proceeding with a faulty redirect.testRedirectWithoutLocationHeaderFallback, has been added to ensure this fallback mechanism works as expected.