fix(tools): don't follow redirects in web_fetch (SSRF) + drop ArrayList<Byte> boxing
This commit is contained in:
+20
-4
@@ -43,6 +43,13 @@ class WebFetchTool(
|
|||||||
private val maxBytes: Long = DEFAULT_MAX_BYTES,
|
private val maxBytes: Long = DEFAULT_MAX_BYTES,
|
||||||
) : Tool, ToolExecutor {
|
) : Tool, ToolExecutor {
|
||||||
|
|
||||||
|
// Do NOT follow redirects. Plane-2 egress (NetworkHostRule) validates the REQUESTED host before
|
||||||
|
// this tool runs; ktor's default redirect-following would then let a server 302 the fetch to an
|
||||||
|
// internal host that was never validated (SSRF). With redirects off, a 3xx never opens that
|
||||||
|
// connection — the model must re-issue web_fetch for the new URL, which re-triggers egress
|
||||||
|
// validation. Derived client shares the injected engine (no separate resource to close).
|
||||||
|
private val client: HttpClient = httpClient.config { followRedirects = false }
|
||||||
|
|
||||||
override val name: String = "web_fetch"
|
override val name: String = "web_fetch"
|
||||||
override val description: String = "Fetch a URL and return its main content as clean markdown."
|
override val description: String = "Fetch a URL and return its main content as clean markdown."
|
||||||
override val tier: Tier = Tier.T2
|
override val tier: Tier = Tier.T2
|
||||||
@@ -77,10 +84,19 @@ class WebFetchTool(
|
|||||||
}
|
}
|
||||||
|
|
||||||
private suspend fun fetch(invocationId: ToolInvocationId, url: String): ToolResult =
|
private suspend fun fetch(invocationId: ToolInvocationId, url: String): ToolResult =
|
||||||
httpClient.prepareGet(url).execute { response ->
|
client.prepareGet(url).execute { response ->
|
||||||
val contentType = response.headers[HttpHeaders.ContentType]
|
val contentType = response.headers[HttpHeaders.ContentType]
|
||||||
val declaredLength = response.headers[HttpHeaders.ContentLength]?.toLongOrNull()
|
val declaredLength = response.headers[HttpHeaders.ContentLength]?.toLongOrNull()
|
||||||
when {
|
when {
|
||||||
|
response.status.value in 300..399 ->
|
||||||
|
fail(
|
||||||
|
invocationId,
|
||||||
|
"URL redirected (HTTP ${response.status.value}) to " +
|
||||||
|
"${response.headers[HttpHeaders.Location] ?: "an unspecified location"}. Redirects are " +
|
||||||
|
"not followed automatically — re-issue web_fetch with that URL so egress is validated " +
|
||||||
|
"for its host.",
|
||||||
|
recoverable = true,
|
||||||
|
)
|
||||||
!response.status.isSuccess() ->
|
!response.status.isSuccess() ->
|
||||||
fail(invocationId, "HTTP ${response.status.value} for $url", recoverable = true)
|
fail(invocationId, "HTTP ${response.status.value} for $url", recoverable = true)
|
||||||
declaredLength != null && declaredLength > maxBytes ->
|
declaredLength != null && declaredLength > maxBytes ->
|
||||||
@@ -123,13 +139,13 @@ class WebFetchTool(
|
|||||||
|
|
||||||
/** Reads the channel up to [maxBytes]; returns null if the body exceeds the cap. */
|
/** Reads the channel up to [maxBytes]; returns null if the body exceeds the cap. */
|
||||||
private suspend fun readBounded(channel: io.ktor.utils.io.ByteReadChannel): ByteArray? {
|
private suspend fun readBounded(channel: io.ktor.utils.io.ByteReadChannel): ByteArray? {
|
||||||
val out = ArrayList<Byte>()
|
val out = java.io.ByteArrayOutputStream()
|
||||||
val buffer = ByteArray(READ_CHUNK)
|
val buffer = ByteArray(READ_CHUNK)
|
||||||
while (true) {
|
while (true) {
|
||||||
val read = channel.readAvailable(buffer, 0, buffer.size)
|
val read = channel.readAvailable(buffer, 0, buffer.size)
|
||||||
if (read == -1) break
|
if (read == -1) break
|
||||||
if (out.size + read > maxBytes) return null
|
if (out.size() + read > maxBytes) return null
|
||||||
for (i in 0 until read) out.add(buffer[i])
|
out.write(buffer, 0, read)
|
||||||
}
|
}
|
||||||
return out.toByteArray()
|
return out.toByteArray()
|
||||||
}
|
}
|
||||||
|
|||||||
+2
-1
@@ -61,7 +61,8 @@ class ShellToolTest {
|
|||||||
// `cd dir && echo x` — cd is a builtin (implicitly allowed), echo is allowlisted, `dir`/`x`
|
// `cd dir && echo x` — cd is a builtin (implicitly allowed), echo is allowlisted, `dir`/`x`
|
||||||
// are arguments (not command positions), so the chain is valid.
|
// are arguments (not command positions), so the chain is valid.
|
||||||
val tool = ShellTool(allowedExecutables = setOf("echo"))
|
val tool = ShellTool(allowedExecutables = setOf("echo"))
|
||||||
assertEquals(ValidationResult.Valid, tool.validateRequest(createRequest(listOf("cd", "dir", "&&", "echo", "x"))))
|
val result = tool.validateRequest(createRequest(listOf("cd", "dir", "&&", "echo", "x")))
|
||||||
|
assertEquals(ValidationResult.Valid, result)
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|||||||
BIN
Binary file not shown.
Reference in New Issue
Block a user