feat(cli): accept mobile file attachments in remote sessions - #12394
Conversation
The mobile client uploads each attachment to R2 and sends a first-class FilePartInput with a server-issued <uuid>.<ext> basename. The CLI fetches the file over HTTPS, re-emits it as a data: URL for text / image / PDF, or writes it to a per-session scratch directory for generic binaries so the agent's tools can read it. - Fetches are HTTPS-only, reject redirects, never forward credentials, are bounded to 5 MB + 1 byte (partial deleted on overflow), and time out. - Any per-attachment failure becomes an explanatory text part so the rest of the prompt still runs; the send_message ACK is unaffected because materialization happens inside the long-running dispatch before prompt(). - Scratch directory (0700 / files 0600) lives under Global.Path.tmp and is removed on session deletion and sender dispose. Basenames derive from the attachment id + validated extension, never the client-supplied filename. - The relay heartbeat now advertises capabilities.attachments so the mobile app only enables attachments for CLIs that support them.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All previously-flagged issues (CRITICAL path traversal, WARNING disposed-once-checked, WARNING deletion/materialization race, and the SUGGESTION items including the unbounded Files Reviewed (4 files)
Previous Review Summaries (2 snapshots, latest commit 7b9a787)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7b9a787)Status: 1 Issue Found | Recommendation: Address before merge All 8 previously-flagged issues (CRITICAL path traversal, WARNING disposal race, WARNING sender/deletion race, and the SUGGESTION items) were verified fixed in the latest commits: scratch filenames now use random UUIDs independent of the untrusted Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous review (commit 4f1734e)Status: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (9 files)
Reviewed by claude-sonnet-5 · Input: 46 · Output: 12.8K · Cached: 1.1M Review guidance: REVIEW.md from base branch |
…chments # Conflicts: # packages/opencode/src/kilo-sessions/remote-ws.ts
|
(bot) @kilocode-bot please review the latest head 08c0ef2. |
Code Review — head
|
| # | Severity | Issue | Status |
|---|---|---|---|
| 1 | CRITICAL | part.id used unsanitized to build the on-disk scratch filename → path traversal / arbitrary write |
✅ Fixed |
| 2 | WARNING | disposed flag checked once, not per part → races dispose()'s scratch-dir removal |
✅ Fixed |
| 3 | WARNING | Session-delete dispose() can race an in-flight materialize() with no coordination |
✅ Fixed |
| 4 | SUGGESTION | Traversal only tested via malicious filename, not id |
✅ Fixed |
1 — Path traversal (CRITICAL): Fixed. The on-disk name no longer derives from part.id. The scratch dir is path.join(root, SCRATCH_DIRNAME, Buffer.from(sessionID).toString("base64url")) (remote-attachments.ts:269) and the basename is `${crypto.randomUUID()}.${extension}` (:300), where extension is run through safeExtension() (^[A-Za-z0-9]{1,16}$, else "bin"). part.id is now used only as the emitted part's id field (:299, :309, :343) and never touches a filesystem path. Confirmed by the new test at remote-attachments.test.ts:402, which feeds id: prt_../../escaped, sessionID: ses_../../escaped, filename: ../../../etc/passwd and asserts the resolved dir does not escape the scratch root, only a single <uuid>.bin entry is created, and the traversal token never appears in output.
2 — Per-part disposal race (WARNING): Fixed. Rather than re-checking a flag per part, create() now tracks in-flight jobs in an active set (:274, :356-372) and dispose() does Promise.allSettled([...active]).then(cleanup) (:374-381). Scratch removal is deferred until the running run() settles, so writes can no longer be clobbered mid-materialize. Covered by remote-attachments.test.ts:542 ("dispose waits for an already-started materialize before removing scratch").
3 — Delete-vs-materialize race (WARNING): Fixed. The Session.Event.Deleted listener in remote-sender.ts (~:304-314) removes the cached materializer and calls result.dispose(), which uses the same active/allSettled coordination as above; the deleted/closed guards in attachmentFor (:316-323) stop a resumed send from re-creating ownership. Covered by sender tests :346 and :409.
New / remaining observations (all minor)
| Severity | Location | Finding |
|---|---|---|
| SUGGESTION | remote-attachments.ts:390-408 |
cleanupSession() is exported and unit-tested (:582) but not wired into production — the deletion path uses result.dispose() instead. It's effectively dead prod code, and its doc comment ("The RemoteSender registers a single Session.Event.Deleted listener that dispatches here") is now stale/inaccurate. Remove it or wire it in, and fix the comment. |
| SUGGESTION | remote-sender.ts (deleted set, ~:299) |
deleted grows unbounded over the lifetime of a long-lived sender that sees many session deletions. Minor memory growth; consider pruning alongside cache eviction. |
| SUGGESTION | remote-attachments.ts:374-381 + prompt path |
On deletion during an in-flight materialize that already wrote a scratch file, dispose() cleanup can remove the scratch dir concurrently with prompt() consuming the emitted "read the file at <path>" text part. Acceptable since the session is being torn down, but the emitted path can dangle. |
| NIT | remote-attachments.ts:355 |
After dispose()/closed, materialize() returns the parts unchanged, so a file part with an http(s) R2 URL passes through to resolvePart, which cannot fetch it. Teardown-only graceful degradation; noting for completeness. |
Verdict
All four previously identified issues — including the CRITICAL path traversal and the id-based test-coverage gap — are resolved. The SSRF/safety net in fetchOne (HTTPS-only, R2 host allowlist, redirect: "error", credentials: "omit", bounded body + timeout) looks solid. Remaining items are minor code-quality/doc cleanups and do not block.
|
The review has already been posted. All 4 previously flagged issues are confirmed fixed at head |
|
(bot) Latest-head re-review acknowledged. All eight original inline findings are fixed, replied to, and resolved. The new observations are non-blocking teardown/code-cleanup notes: the deleted-session tombstones intentionally last for the sender lifetime so delayed work cannot recreate deleted materializers, and the remaining race/passthrough cases occur only during teardown. Per the review verdict, no additional behavioral change is needed in this PR. |
|
(bot) @kilocode-bot please verify latest head 7b9a787 after removing redundant kilocode_change markers from Kilo-owned paths. |
Review — head
|
|
Review already posted at head |
|
(bot) @kilocode-bot please re-review latest head 6750e1c. The four minor observations from your prior review are now addressed, with additional race/fail-closed tests. |
Review — head
|
|
Review already posted at head |
|
(bot) Final local iOS E2E passed on exact head |
| protocolVersion: InstallationVersion, | ||
| capabilities: { attachments: true }, |
There was a problem hiding this comment.
I think we can also gate it on protocol version without introducing "capabilities"
Integrate #12394 (mobile file attachments). Resolved additive heartbeat-wire conflicts in remote-protocol.ts / remote-ws.ts by keeping both the K1 instance-advertisement field and the #12394 capabilities.attachments field; combined the round-trip tests. Typecheck + focused kilo-sessions suites green.
| // canonicalize to `text/plain` at re-entry (per text caveat in the | ||
| // design). The binary fallback is `application/octet-stream` and is | ||
| // applied to any extension not present here AND to extensionless inputs. | ||
| export const EXTENSION_MIME: Record<string, string> = { |
There was a problem hiding this comment.
EXTENSION_MIMElooks pretty hardcoded to me and incomplete
There was a problem hiding this comment.
I think it's fine, we can never have an exhaustive list anyway and application/octet-stream covers the fallback.
| try { | ||
| reader.releaseLock() | ||
| } catch { | ||
| // reader already detached; nothing to do |
There was a problem hiding this comment.
The code has a few catch statements without logging, please consider doing so.
There was a problem hiding this comment.
These are in finally, so logging has already happened.
…g#12394) * feat(cli): accept mobile file attachments in remote sessions The mobile client uploads each attachment to R2 and sends a first-class FilePartInput with a server-issued <uuid>.<ext> basename. The CLI fetches the file over HTTPS, re-emits it as a data: URL for text / image / PDF, or writes it to a per-session scratch directory for generic binaries so the agent's tools can read it. - Fetches are HTTPS-only, reject redirects, never forward credentials, are bounded to 5 MB + 1 byte (partial deleted on overflow), and time out. - Any per-attachment failure becomes an explanatory text part so the rest of the prompt still runs; the send_message ACK is unaffected because materialization happens inside the long-running dispatch before prompt(). - Scratch directory (0700 / files 0600) lives under Global.Path.tmp and is removed on session deletion and sender dispose. Basenames derive from the attachment id + validated extension, never the client-supplied filename. - The relay heartbeat now advertises capabilities.attachments so the mobile app only enables attachments for CLIs that support them. * fix(cli): secure remote attachment materialization * chore(cli): remove redundant change markers * fix(cli): coordinate remote attachment lifetime * fix(cli): fail closed during attachment cleanup * fix(cli): track idle attachment cleanup
3 participants
What & why
Remote CLI sessions can now receive file attachments sent from the Kilo mobile app. The mobile client uploads each file to R2 and sends a first-class
FilePartInputwhoseurlis an HTTPS presigned GET and whosefilenameis the server-issued<uuid>.<ext>basename. This change makes the CLI fetch and materialize those attachments for the agent, and advertise a capability so the mobile app only enables attachments for CLIs that support them.Companion cloud change (upload/download presign, capability plumbing, mobile UX): Kilo-Org/cloud#4628.
How
src/kilocode/remote-attachments.ts, invoked fromremote-sender.tsinside thedispatchLongRunningcallback immediately beforeprompt(...)— parsing, synchronous normalization, and schema validation stay before thesend_messageACK, so an unresolved attachment fetch can never delay the ACK.filenameextension via a canonical table: text/* is canonicalized totext/plainfor re-entry into the existingresolvePartbranches, PDF staysapplication/pdf, images pass through, and generic binaries are written to a per-session scratch dir and surfaced as a text part naming the absolute path/filename/mime/size.Global.Path.tmp/remote-attachments/<sessionID>(dirs0700, files0600); basenames derive from the attachment id + validated extension (never the client filename); the dir is removed onSession.Event.Deletedand on sender dispose.capabilities.attachments. Builds without this change simply never send it.Confined to Kilo-owned dirs (
src/kilocode/,src/kilo-sessions/); no shared upstream files touched. Includes a minor changeset.