diff --git a/clients/kotlin/README.md b/clients/kotlin/README.md index 46b97ed..b63b7e9 100644 --- a/clients/kotlin/README.md +++ b/clients/kotlin/README.md @@ -157,6 +157,41 @@ unmodified, so structured concurrency (timeouts, parent-cancel) keep working. `InputProvider` backed by the file's `InputStream`. A 100 MB file uses roughly the buffer size of heap, not 100 MB. +A few caller-trust assumptions worth flagging: + +- **Symlinks are followed.** `Files.isRegularFile` resolves symlinks by + default, so a symlink-to-`/etc/passwd` (or anywhere else readable by the + process) will be accepted and streamed upstream. If you're relaying + paths from untrusted input, resolve them yourself with `path.toRealPath()` + and reject anything outside an allowlisted root before calling + `uploadFile`. +- **Don't mutate the file mid-upload.** The size is sampled once for the + multipart Content-Length, then the stream is opened separately. Live + edits between those two reads will produce a truncated or padded body + on the wire. +- **Filename safety.** The leaf filename is interpolated into a + `Content-Disposition` header. Control characters (CR, LF, NUL, ...) in + the filename are rejected upfront with `IllegalArgumentException`; + embedded `"` is stripped. Anything else is passed through verbatim. + +## Security + +- **CVE-2024-49580 (`ktor-client-core`).** Cleared by the Ktor 2.3.13 bump. + The vulnerable code path is in the `HttpCache` plugin, which the SDK + doesn't install — but if you reach into `ForgeOptions.configure` and + install `HttpCache` yourself on a client linked against an older Ktor, + you'd expose yourself. +- **CVE-2025-29904 (`ktor-server-*`).** Server-side request smuggling. The + SDK only depends on `ktor-client-*`, so this is not exposed by the + client surface. There is no 2.3.x backport from JetBrains; if/when this + SDK migrates to Ktor 3.x it will pick up the patch alongside. +- **Bearer token never logged.** `ForgeClient` is a regular class (no + auto-`toString()`); `ForgeOptions` doesn't carry the token. Ktor's + `Logging` plugin is not installed by default. `AppToken.toString()` + redacts the plaintext when it's set. If you're consuming this library + and adding logging, double-check you're not capturing `Authorization` + headers. + ## Builds & tests ```bash diff --git a/clients/kotlin/build.gradle.kts b/clients/kotlin/build.gradle.kts index c3d2280..4802d9d 100644 --- a/clients/kotlin/build.gradle.kts +++ b/clients/kotlin/build.gradle.kts @@ -24,7 +24,7 @@ kotlin { explicitApi() } -val ktorVersion = "2.3.12" +val ktorVersion = "2.3.13" dependencies { api("org.jetbrains.kotlinx:kotlinx-coroutines-core:1.8.1") diff --git a/clients/kotlin/src/main/kotlin/com/clawdforge/ForgeClient.kt b/clients/kotlin/src/main/kotlin/com/clawdforge/ForgeClient.kt index f7b17f6..bad62f3 100644 --- a/clients/kotlin/src/main/kotlin/com/clawdforge/ForgeClient.kt +++ b/clients/kotlin/src/main/kotlin/com/clawdforge/ForgeClient.kt @@ -5,6 +5,7 @@ import io.ktor.client.call.body import io.ktor.client.engine.cio.CIO import io.ktor.client.plugins.HttpTimeout import io.ktor.client.plugins.contentnegotiation.ContentNegotiation +import io.ktor.client.plugins.timeout import io.ktor.client.plugins.defaultRequest import io.ktor.client.request.HttpRequestBuilder import io.ktor.client.request.delete @@ -129,8 +130,20 @@ public class ForgeClient @JvmOverloads public constructor( model = request.model ?: options.defaultModel, timeoutSecs = request.timeoutSecs ?: options.defaultTimeout.inWholeSeconds.toInt(), ) + // B1: per-call HTTP timeout that tracks RunRequest.timeoutSecs. + // The global HttpTimeout install acts as a floor for non-/run paths + // (defaultTimeout + requestMargin); this block overrides it for /run + // so a caller-supplied `timeoutSecs` larger than the default doesn't + // HTTP-disconnect while the server is still doing useful work. + val timeoutSecs = effective.timeoutSecs + ?: options.defaultTimeout.inWholeSeconds.toInt() + val callTimeoutMs = (timeoutSecs * 1000L) + options.requestMargin.inWholeMilliseconds return wrapTransport("POST /run") { val resp = http.post("$baseUrl/run") { + timeout { + requestTimeoutMillis = callTimeoutMs + socketTimeoutMillis = callTimeoutMs + } authHeader() contentType(ContentType.Application.Json) setBody(effective) @@ -148,13 +161,35 @@ public class ForgeClient @JvmOverloads public constructor( * `InputStream`. So a 100 MB upload will use roughly the buffer size, * not 100 MB of heap. * + * **Symlinks:** `Files.isRegularFile(path)` follows symlinks by default, + * so a symlink that points at a regular file will be accepted and its + * target streamed upstream. Treat [path] as caller-trusted. If you're + * relaying paths from untrusted input, resolve them yourself first + * (e.g. `path.toRealPath()`) and reject anything outside an allowlisted + * root. + * + * **TOCTOU:** the file's [Files.size] is sampled once at call time and + * sent as the multipart Content-Length, then the stream is opened + * separately. If the file is mutated between those two reads the upload + * will be truncated or padded. Don't modify [path] while [uploadFile] + * is in flight. + * * @param ttlSecs server-clamped to `[60, 86400]`. Pass `null` for the * server default of 3600. + * @throws IllegalArgumentException if [path] is not a regular file or if + * its leaf filename contains an ISO control character (CR, LF, + * NUL, ...). */ public suspend fun uploadFile(path: Path, ttlSecs: Int? = null): FileToken { require(Files.isRegularFile(path)) { "not a regular file: $path" } + val rawName = path.fileName?.toString() ?: "upload" + // L4: surface a typed IllegalArgumentException upfront instead of + // letting Ktor's IllegalHeaderValueException fire deep in the engine. + require(rawName.none { it.isISOControl() }) { + "filename contains control character: $rawName" + } + val safeName = rawName.replace("\"", "") val size = Files.size(path) - val filename = path.fileName?.toString() ?: "upload" return wrapTransport("POST /files") { val resp = http.submitFormWithBinaryData( @@ -171,7 +206,7 @@ public class ForgeClient @JvmOverloads public constructor( headers = io.ktor.http.Headers.build { append( HttpHeaders.ContentDisposition, - "filename=\"${filename.replace("\"", "")}\"", + "filename=\"$safeName\"", ) }, ) diff --git a/clients/kotlin/src/main/kotlin/com/clawdforge/Models.kt b/clients/kotlin/src/main/kotlin/com/clawdforge/Models.kt index 0b1bff1..30c8244 100644 --- a/clients/kotlin/src/main/kotlin/com/clawdforge/Models.kt +++ b/clients/kotlin/src/main/kotlin/com/clawdforge/Models.kt @@ -3,6 +3,8 @@ package com.clawdforge import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable import kotlinx.serialization.json.JsonElement +import kotlinx.serialization.json.JsonObject +import kotlinx.serialization.json.JsonPrimitive /** * Parsed body of `GET /healthz`. @@ -84,6 +86,13 @@ public data class FileToken( * One entry from `GET /admin/tokens`, also returned (with [token] populated) * by `POST /admin/tokens`. The plaintext [token] is only present at create * time and cannot be retrieved later. + * + * `toString()` is overridden to redact the plaintext [token] when it's set + * — `data class` would otherwise auto-generate a `toString()` that prints + * the secret on any log line, exception message, or REPL inspection. The + * `null` vs set distinction is preserved (`token=null` for list-row form, + * `token=***` for post-create form) so consumers can still tell which + * shape they're holding. */ @Serializable public data class AppToken( @@ -91,7 +100,26 @@ public data class AppToken( val token: String? = null, @SerialName("ip_cidrs") val ipCidrs: List = emptyList(), @SerialName("created_at") val createdAt: Long? = null, -) +) { + override fun toString(): String = + "AppToken(name=$name, token=${if (token != null) "***" else "null"}, " + + "ipCidrs=$ipCidrs, createdAt=$createdAt)" +} + +/** + * Null-safe accessor for [RunResult.result] when the server returned a + * parsed JSON object — equivalent to `result as? JsonObject`. Returns + * `null` if the server returned a string or any other JSON shape. + */ +public fun RunResult.resultAsObjectOrNull(): JsonObject? = result as? JsonObject + +/** + * Null-safe accessor for [RunResult.result] when the server returned a + * plain string. Returns `null` if the server returned an object/array, + * a non-string primitive (number/boolean), or `JsonNull`. + */ +public fun RunResult.resultAsTextOrNull(): String? = + (result as? JsonPrimitive)?.takeIf { it.isString }?.content /** Body for `POST /admin/tokens`. */ @Serializable diff --git a/clients/kotlin/src/test/kotlin/com/clawdforge/ForgeClientTest.kt b/clients/kotlin/src/test/kotlin/com/clawdforge/ForgeClientTest.kt index e5b6b8d..9ec777b 100644 --- a/clients/kotlin/src/test/kotlin/com/clawdforge/ForgeClientTest.kt +++ b/clients/kotlin/src/test/kotlin/com/clawdforge/ForgeClientTest.kt @@ -9,6 +9,7 @@ import io.ktor.http.HttpHeaders import io.ktor.http.HttpMethod import io.ktor.http.HttpStatusCode import io.ktor.http.headersOf +import kotlinx.coroutines.delay import kotlinx.coroutines.test.runTest import kotlinx.serialization.json.JsonObject import kotlinx.serialization.json.JsonPrimitive @@ -16,6 +17,8 @@ import kotlinx.serialization.json.boolean import kotlinx.serialization.json.int import kotlinx.serialization.json.jsonObject import kotlinx.serialization.json.jsonPrimitive +import java.nio.file.Files +import java.nio.file.Path import kotlin.io.path.createTempFile import kotlin.io.path.writeText import kotlin.test.Test @@ -23,7 +26,9 @@ import kotlin.test.assertContains import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNotNull +import kotlin.test.assertNull import kotlin.test.assertTrue +import kotlin.time.Duration.Companion.seconds /** * Unit tests for [ForgeClient] using Ktor's [MockEngine] — no real network. @@ -303,4 +308,151 @@ class ForgeClientTest { assertEquals(true, (obj["approx"] as JsonPrimitive).boolean) } } + + // ------------------------------------------------------------------ + // B1 regression: per-call HTTP timeout tracks RunRequest.timeoutSecs + // ------------------------------------------------------------------ + + @Test + fun runHttpTimeoutHonorsPerCallTimeoutSecs() = runTest { + // Global floor: defaultTimeout=5s + requestMargin=1s = 6s. + // Server delays 8s before responding. + // Caller passes timeoutSecs=20 => per-call window = 21s, plenty of headroom. + // Without B1 fix: HttpRequestTimeoutException at 6s -> ForgeTransportException. + // With B1 fix: succeeds. + val engine = MockEngine { req -> + assertEquals("/run", req.url.encodedPath) + delay(8.seconds) + respond( + """{"ok":true,"result":"done","duration_ms":8000,"stop_reason":"end_turn"}""", + HttpStatusCode.OK, + jsonContentType, + ) + } + val c = ForgeClient( + baseUrl = "http://forge.test", + token = "cf_test", + options = ForgeOptions( + engine = engine, + defaultTimeout = 5.seconds, + requestMargin = 1.seconds, + ), + ) + c.use { + val r = it.run(RunRequest(prompt = "long-running", timeoutSecs = 20)) + assertEquals("done", r.result.jsonPrimitive.content) + } + } + + // ------------------------------------------------------------------ + // L3 regression: AppToken.toString() redacts plaintext token + // ------------------------------------------------------------------ + + @Test + fun appTokenToStringRedactsTokenWhenSet() { + val t = AppToken( + name = "x", + token = "cf_secret", + ipCidrs = listOf("10.0.0.0/8"), + createdAt = 1700000000L, + ) + val s = t.toString() + assertTrue(!s.contains("cf_secret"), "toString must not leak plaintext token: $s") + assertContains(s, "token=***") + assertContains(s, "name=x") + } + + @Test + fun appTokenToStringPreservesNullToken() { + val t = AppToken(name = "x", token = null, ipCidrs = emptyList(), createdAt = null) + val s = t.toString() + assertContains(s, "token=null") + assertTrue(!s.contains("token=***"), "null-token form must NOT show ***: $s") + } + + // ------------------------------------------------------------------ + // L4 regression: uploadFile rejects control-char filenames upfront + // ------------------------------------------------------------------ + + @Test + fun uploadFileRejectsControlCharFilename() = runTest { + // Build a real on-disk file with a control-char in its leaf name. + // POSIX allows newline in filenames; if the filesystem refuses, + // skip via assumption (test environment fail-soft). + val dir = Files.createTempDirectory("forge-bad-name") + val badPath: Path = try { + val p = dir.resolve("foo\nbar") + Files.createFile(p) + p + } catch (e: Exception) { + // Filesystem refuses newline -> skip. + return@runTest + } + val c = client { respond("""{}""", HttpStatusCode.OK, jsonContentType) } + c.use { + val ex = assertFailsWith { + it.uploadFile(badPath) + } + assertContains(ex.message ?: "", "control character") + } + } + + // ------------------------------------------------------------------ + // L5 regression: resultAsObjectOrNull / resultAsTextOrNull helpers + // ------------------------------------------------------------------ + + @Test + fun runResultAsObjectOrNullExtractsJsonObject() = runTest { + val c = client { + respond( + """{"ok":true,"result":{"k":"v"},"duration_ms":1,"stop_reason":"end_turn"}""", + HttpStatusCode.OK, + jsonContentType, + ) + } + c.use { + val r = it.run(RunRequest(prompt = "p")) + val obj = r.resultAsObjectOrNull() + assertNotNull(obj) + assertEquals("v", (obj["k"] as JsonPrimitive).content) + // Object response is not a string; text accessor returns null. + assertNull(r.resultAsTextOrNull()) + } + } + + @Test + fun runResultAsTextOrNullExtractsString() = runTest { + val c = client { + respond( + """{"ok":true,"result":"plain","duration_ms":1,"stop_reason":"end_turn"}""", + HttpStatusCode.OK, + jsonContentType, + ) + } + c.use { + val r = it.run(RunRequest(prompt = "p")) + assertEquals("plain", r.resultAsTextOrNull()) + assertNull(r.resultAsObjectOrNull()) + } + } + + // ------------------------------------------------------------------ + // Coverage gaps from audit memo + // ------------------------------------------------------------------ + + @Test + fun revokeTokenEmptyName() = runTest { + val c = client { respond("""{}""", HttpStatusCode.OK, jsonContentType) } + c.use { + assertFailsWith { it.revokeToken("") } + } + } + + @Test + fun closeIdempotent() = runTest { + val c = client { respond("""{}""", HttpStatusCode.OK, jsonContentType) } + c.close() + // Second close must not throw. + c.close() + } }