KTOR-6837 Fix client URL handling for invalid DefaultRequest.host #5207
Conversation
WalkthroughThis PR introduces URL normalization for invalid hosts containing special characters ('/', '?', '#'), adds validation warnings in the default request configuration, and improves URL-to-NSURL conversion on Darwin platforms. The changes ensure consistent URL handling across CIO, core, and Darwin engines. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ktor-client/ktor-client-cio/common/src/io/ktor/client/engine/cio/utils.kt (1)
41-68: Normalize URL inwriteHeadersis good; consider doing the same instartTunnelUsing
request.url.rebuildIfNeeded()inwriteHeadersensures both the request line and Host header are based on a normalized URL, fixing the CIO behavior whenhostincorrectly contains path/query/fragment. That part looks solid.However,
startTunnelstill usesrequest.url.hostWithPortdirectly, so an invalidhostcan still leak into theCONNECTrequest line and Host header when going through an HTTP proxy.You can reuse the same normalization there with minimal risk:
internal suspend fun startTunnel( request: HttpRequestData, output: ByteWriteChannel, input: ByteReadChannel ) { val builder = RequestResponseBuilder() try { - val hostWithPort = request.url.hostWithPort + val url = request.url.rebuildIfNeeded() + val hostWithPort = url.hostWithPort builder.requestLine(HttpMethod("CONNECT"), hostWithPort, HttpProtocolVersion.HTTP_1_1.toString()) builder.headerLine(HttpHeaders.Host, hostWithPort)This would make the proxy CONNECT path consistent with the rest of CIO’s URL handling for the same class of misconfigurations.
Also applies to: 217-256
🧹 Nitpick comments (3)
ktor-client/ktor-client-cio/common/src/io/ktor/client/engine/cio/CIOEngine.kt (1)
109-143: URL normalization inselectEndpointandUrl.rebuildIfNeededlooks correctNormalizing the
Urlbefore endpoint selection viarebuildIfNeeded()ensures CIO no longer tries to open connections to hosts that accidentally include path/query/fragment, while remaining a no-op for valid hosts. The extension’sUrl(this.toString())strategy is appropriate here givenUrl.toString()produces a full URL that can be reparsed into clean host/port/path components.If you want to slightly improve readability, you could rename the local in
selectEndpoint(e.g.,val normalizedUrl = url.rebuildIfNeeded()) and use that below, to avoid shadowing the parameter name, but it’s not functionally necessary.Also applies to: 146-152
ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinUrlUtils.kt (1)
22-64: Darwin URL reconstruction is comprehensive and aligns with the normalization strategyThe new
with(Url(this.toString()))block correctly rebuilds NSURLComponents from a normalizedUrl, covering user/password, host, port, path, query (including multi-valued parameters), and fragment, while preserving a cheap fast-path when everything is already encoded. This also implicitly fixes cases where the originalhostcontained path/query/fragment, bringing Darwin behavior in line with the CIO normalization.Given the very similar logic in the legacy Darwin file, you might later consider factoring this into a shared helper to avoid drift, but the current change is functionally sound.
ktor-client/ktor-client-darwin-legacy/darwin/src/io/ktor/client/engine/darwin/internal/legacy/DarwinLegacyUrlUtils.kt (1)
22-64: Legacy Darwin URL handling now matches the main Darwin implementationThe legacy
Url.toNSUrl()implementation mirrors the updated non-legacy version, including the reparsing step viaUrl(this.toString())and full population of NSURLComponents (user, password, host, port, path, query items, fragment). That gives the legacy Darwin engine the same robustness against malformedhostvalues and partial encoding.As with the main Darwin file, this duplication could be reduced via a shared helper, but functionally this looks correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ktor-client/ktor-client-cio/common/src/io/ktor/client/engine/cio/CIOEngine.kt(2 hunks)ktor-client/ktor-client-cio/common/src/io/ktor/client/engine/cio/utils.kt(1 hunks)ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/DefaultRequest.kt(1 hunks)ktor-client/ktor-client-darwin-legacy/darwin/src/io/ktor/client/engine/darwin/internal/legacy/DarwinLegacyUrlUtils.kt(1 hunks)ktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinUrlUtils.kt(1 hunks)ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ConnectionTest.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: yschimke
Repo: ktorio/ktor PR: 4013
File: ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/Android14URLConnectionFactory.kt:24-26
Timestamp: 2025-09-30T07:52:14.769Z
Learning: In the Ktor Android HTTP client engine (ktor-client-android), prefer using `URI.create(urlString).toURL()` over the `URL(urlString)` constructor when opening connections with Android's HttpEngine, as it avoids deprecated APIs and the different exception behavior (IllegalArgumentException vs MalformedURLException) is acceptable.
📚 Learning: 2025-09-30T07:52:14.769Z
Learnt from: yschimke
Repo: ktorio/ktor PR: 4013
File: ktor-client/ktor-client-android/jvm/src/io/ktor/client/engine/android/Android14URLConnectionFactory.kt:24-26
Timestamp: 2025-09-30T07:52:14.769Z
Learning: In the Ktor Android HTTP client engine (ktor-client-android), prefer using `URI.create(urlString).toURL()` over the `URL(urlString)` constructor when opening connections with Android's HttpEngine, as it avoids deprecated APIs and the different exception behavior (IllegalArgumentException vs MalformedURLException) is acceptable.
Applied to files:
ktor-client/ktor-client-cio/common/src/io/ktor/client/engine/cio/CIOEngine.ktktor-client/ktor-client-darwin/darwin/src/io/ktor/client/engine/darwin/internal/DarwinUrlUtils.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ConnectionTest.ktktor-client/ktor-client-cio/common/src/io/ktor/client/engine/cio/utils.ktktor-client/ktor-client-darwin-legacy/darwin/src/io/ktor/client/engine/darwin/internal/legacy/DarwinLegacyUrlUtils.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: The headersOf() function from io.ktor.http package is available through wildcard imports like `import io.ktor.http.*`, so no explicit import statement is needed when using wildcard imports from that package.
Applied to files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ConnectionTest.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Gobley Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module requires the kotlin("plugin.atomicfu") plugin as a dependency, so the atomicfu plugin should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ConnectionTest.kt
📚 Learning: 2025-06-23T12:49:56.883Z
Learnt from: CR
Repo: ktorio/ktor PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-06-23T12:49:56.883Z
Learning: Error handling should follow Kotlin conventions and use specific Ktor exceptions.
Applied to files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ConnectionTest.kt
📚 Learning: 2025-05-30T06:45:52.309Z
Learnt from: rururux
Repo: ktorio/ktor PR: 4896
File: ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt:1-12
Timestamp: 2025-05-30T06:45:52.309Z
Learning: In Ktor test files, particularly in the ktor-client/ktor-client-core/jvm/test/ directory, test files follow the convention of not including explicit package declarations. This is consistent across test files like CachingCacheStorageTest.kt and should be maintained for consistency.
Applied to files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ConnectionTest.kt
📚 Learning: 2025-08-14T15:17:11.466Z
Learnt from: zibet27
Repo: ktorio/ktor PR: 5044
File: ktor-client/ktor-client-webrtc/ktor-client-webrtc-rs/build.gradle.kts:12-12
Timestamp: 2025-08-14T15:17:11.466Z
Learning: The Cargo plugin (dev.gobley.cargo) used in the Ktor WebRTC RS module depends on the kotlin("plugin.atomicfu") plugin, so atomicfu should not be removed even if there are no direct kotlinx.atomicfu imports in the module's source code.
Applied to files:
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ConnectionTest.kt
🔇 Additional comments (2)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/DefaultRequest.kt (1)
210-221: Host validation warning behavior looks correctThe additional warning on
hostwhen it includes/,?, or#matches the documented misuse (host-with-path) and keeps behavior backward compatible by still assigning the value. The message is clear and points users tourl(...)/url { ... }instead. No functional issues spotted here.ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/ConnectionTest.kt (1)
9-9: New test effectively covers the invalid DefaultRequest.host scenarioThe test accurately simulates
DefaultRequest.hostcontaining a path segment and verifies that requests still succeed after the engine-side normalization. The use ofdefaultRequestfrom the plugins package andclientTestsscaffolding is consistent with the rest of the suite. Relying onTEST_SERVER.removePrefix("http://")is acceptable given existing usage of TEST_SERVER in this file.Also applies to: 52-65
Subsystem
Client, DefaultRequest, CIO, Darwin
Motivation
KTOR-6837
Solution
DefaultRequest.hostallows setting non-host values, which can include path, query, or fragment (e.g., "httpbin.org/status"), and this produces an invalid URL where the host field contains path segments. Most engines recover by parsing the full URL string and sending a correct request, but CIO and Darwin use the host field directly and therefore fail.Since this behaviour could be used, the solution is to add a warning now and throw an exception for an invalid host in DefaultRequest in 4.0.0. Also, normalize URLs in CIO and Darwin by reparsing from url.toString() only when the host is invalid, aligning their behavior with that of other client engines.