fix(cli): enforce read permissions for file mentions#12158
fix(cli): enforce read permissions for file mentions#12158marius-kilocode wants to merge 5 commits into
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (15 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 99b33db)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 99b33db)Status: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Reviewed by gpt-5.6-sol · Input: 174.6K · Output: 27.6K · Cached: 1.6M Review guidance: REVIEW.md from base branch |
marius-kilocode
force-pushed
the
fix-read-permission-deny-bypass
branch
from
99b33db to
87b7d81
Compare
| ), | ||
| Effect.catch(() => Effect.succeed([] as string[])), | ||
| ) | ||
| const parent = yield* KiloReadObject.directory(dir).pipe(Effect.option) |
There was a problem hiding this comment.
[WARNING]: Missing-file suggestions bypass read permission
KiloReadObject.directory has already enumerated the parent before this branch, and miss never calls ctx.ask. This regresses the old ordering where the requested path was evaluated by permission.read before suggestions were generated: a denied or .kilocodeignore-covered directory can now disclose matching filenames through Did you mean. Canonicalize the parent without listing it, evaluate read permission, and only then enumerate suggestions.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| export function use<A, E, R>(requested: string, fn: (file: File) => Effect.Effect<A, E, R>) { | ||
| const acquire = Effect.tryPromise({ | ||
| try: () => open(requested, "r"), |
There was a problem hiding this comment.
[WARNING]: Opening happens before any permission check
Every caller acquires this handle before it can evaluate external_directory or read. For regular files that is mostly metadata access, but opening a FIFO for reading can block indefinitely waiting for a writer, so a denied file mention/tool call can hang a worker before authorization and cannot reach the abort-aware read loop. Reject non-regular targets using non-blocking/no-follow metadata first, or acquire them in a mode that cannot block before permission evaluation.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (opened.dev !== seen.dev || opened.ino !== seen.ino) { | ||
| throw new ChangedError(`Directory changed while opening: ${requested}`) | ||
| } | ||
| const entries = await readdir(resolved, { withFileTypes: true }) |
There was a problem hiding this comment.
[WARNING]: Bind the directory listing to the validated object
This still reads by pathname rather than through an opened directory handle. A cooperating process can rename the validated directory away, put an external-directory symlink at resolved for this readdir, and restore the original directory before the following stat/realpath; all post-checks then pass while entries contains names from the unauthorized directory. Open the directory once and enumerate through that stable handle, as the file branch does.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| filename: part.filename!, | ||
| source: part.source, | ||
| // kilocode_change start - authorize and consume direct attachments through one open object | ||
| const access = yield* KiloReadObject.use(filepath, (bound) => |
There was a problem hiding this comment.
[WARNING]: Do not hold unbounded file handles while asking
use opens the file before context.ask and keeps it open for the entire human-response wait. Prompt parts are resolved with unbounded concurrency, so one prompt containing many files under an ask rule can retain one descriptor per part and exhaust the process-wide descriptor limit before the user can reply. Bound attachment concurrency and/or close the object while waiting, then reopen and verify the same object after approval.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| yield* elog.info("cancel", { sessionID }) | ||
| yield* KiloSessionPromptQueue.cancel(sessionID) // kilocode_change - drop queued follow-up loops on abort | ||
| KiloSessionPrompt.abortPlanFollowup(sessionID) // kilocode_change - abort pending plan-followup handover work | ||
| yield* KiloSessionPrompt.abortAsyncPrompts(sessionID) // kilocode_change - interrupt attachment permission waits |
There was a problem hiding this comment.
[WARNING]: Register synchronous prompts for intake cancellation too
Only promptAsync calls startAsyncPrompt; the synchronous /prompt handler invokes promptSvc.prompt directly. If that path waits in the new attachment permission phase, SessionRunState has not started yet and this abort call finds neither an async intake nor a runner, leaving the HTTP request, pending permission, and open file handle alive after Stop. Track the pre-loop intake for both endpoints rather than only the async route.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
File mentions are converted into local
file:prompt attachments before model execution. Text attachments were expanded through a Read tool context with a no-op permission callback, while binary and media attachments read the filesystem directly. Because.kilocodeignoreis enforced by generatingpermission.readdeny rules, both paths could bypass the evaluator and persist denied file bytes into model-bound context.Route every local prompt attachment through the active session permission resolver before content is read.
allowretains the existing attachment behavior,askwaits for approval, anddenyor.kilocodeignoreproduces a failure notice without persisting file content. Directory expansion now authorizes all child files first, and direct attachments enforce external-directory policy.Canonicalize symlink targets and check both requested and resolved paths so an allowed alias cannot expose a denied target. Configured references retain their external-directory exemption only when the canonical target remains inside the configured reference root.
Fixes #12133