clients/kotlin: apply audit findings — per-call HTTP timeout + token redaction (cc54cfb → next)
MEDIUM: - B1: per-call HTTP timeout on /run via Ktor request-scoped timeout block — RunRequest.timeoutSecs > defaultTimeout no longer HTTP-disconnects LOW: - L3: AppToken.toString() redacts plaintext token (preserves null distinguishability) - L4: uploadFile validates filename has no control chars; typed IllegalArgumentException upfront - L5: RunResult.resultAsObjectOrNull / resultAsTextOrNull added (matched KDoc claim) - L1/L2: KDoc + README docs for symlink-follow + TOCTOU on uploadFile Dep: - ktor 2.3.12 → 2.3.13 — clears CVE-2024-49580 (HttpCache, plugin not used) by version-range Tests added: runHttpTimeoutHonorsPerCallTimeoutSecs, appTokenToStringRedactsTokenWhenSet (+ null preserve), uploadFileRejectsControlCharFilename, runResultAsObjectOrNull/AsTextOrNull, revokeTokenEmptyName, closeIdempotent. Audit: memory/clawdforge-audits/kotlin-cc54cfb.md
This commit is contained in:
parent
ebbd7cc553
commit
3c77ef523e
5 changed files with 254 additions and 4 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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\"",
|
||||
)
|
||||
},
|
||||
)
|
||||
|
|
|
|||
|
|
@ -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<String> = 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
|
||||
|
|
|
|||
|
|
@ -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<IllegalArgumentException> {
|
||||
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<IllegalArgumentException> { it.revokeToken("") }
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun closeIdempotent() = runTest {
|
||||
val c = client { respond("""{}""", HttpStatusCode.OK, jsonContentType) }
|
||||
c.close()
|
||||
// Second close must not throw.
|
||||
c.close()
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue