feat(cli): enforce /add-dir via sandbox writable roots (⚠ security — please review) - #166
feat(cli): enforce /add-dir via sandbox writable roots (⚠ security — please review)#166oratis wants to merge 1 commit into
Conversation
…itive) /add-dir only printed a message; the settings.permissions.additionalDirectories field was declared but consumed nowhere. Now: - /add-dir validates the path is an existing directory and PERSISTS it to permissions.additionalDirectories (dedup) in the user settings; with no args it lists the current set. - New core helper withAdditionalWritableDirs(sandbox, dirs) folds those dirs into the sandbox's filesystem.allowWrite. The REPL + headless build their sandboxConfig through it, so the sandboxed Bash tool can write to added dirs (beyond cwd). No-op when the sandbox is off; never mutates input. The file tools (Read/Write/Edit/Glob/Grep) already accept any absolute path, so this only changes the SANDBOX boundary for Bash — which is the security-relevant surface. Holding this PR for review rather than auto-merging. Tests: withAdditionalWritableDirs (add/dedup/undefined-safe) + /add-dir (persist / reject-missing / list). core 646 · cli 143. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review: reworked as #214 — approach was right, coverage is no longer sufficientThe diagnosis in this PR is correct and worth restating, because it's the non-obvious part: the file tools (Read/Write/Edit/Glob/Grep) already accept any absolute path, so there is no cwd containment for Two things have changed since June that make this specific branch unmergeable: 1. Coverage is now incomplete, in a way that matters for a security setting. This PR wires the CLI REPL and headless paths. It predates the app-server — since #192/#195/#196, desktop, VS Code, and LSP all run through #214 routes all 7 sandbox assembly sites across 4 files through one helper, including the plugin capability bridges — a plugin's 2. The docs change would regress. This branch rewrites the whole #214 also hardens the helper a bit beyond this version: it's a no-op when the sandbox is disabled (it must never enable a sandbox as a side effect, nor appear to grant access that isn't mediated), it returns the input unchanged when nothing new is added, and it drops non-absolute entries rather than handing them to profile writers that require absolute paths. Thanks for flagging this as security-sensitive and leaving it for review rather than self-merging — that was the right call, and the extra scrutiny is what surfaced the app-server gap. Closed in favour of #214. |
|
Reworked as #214 — same approach, extended to the app-server (desktop/VS Code/LSP) and without the stale BEHAVIOR_PARITY rewrite. |
…#214) Reworks #166, which was correct in approach but is now incomplete and carries stale docs. `permissions.additionalDirectories` has been declared in the settings schema and read by /permissions for display, but consumed for enforcement nowhere — /add-dir only printed "recorded ... (effective in M3)". Users could reasonably believe it did something. Why the sandbox is the right boundary: the file tools (Read/Write/Edit/ Glob/Grep) already accept any absolute path, so there is no cwd containment to widen. The only thing that actually restricts writes is the OS sandbox wrapping Bash. Enforcing /add-dir therefore means adding those directories to `filesystem.allowWrite`. The part #166 could not have covered: it predates the app-server, and wired only CLI REPL + headless. Since #192/#195/#196, desktop, VS Code and LSP all run through apps/server, so that wiring would have enforced the setting in the CLI and silently ignored it everywhere else — for a security-relevant setting, partial enforcement is worse than none, because it is indistinguishable from full enforcement at the UI. All 7 sandbox assembly sites across 4 files now route through the helper. - core `withAdditionalWritableDirs(sandbox, dirs, cwd?)`: pure, never mutates input, no-op when the sandbox is disabled (never silently enables it), dedupes, and drops non-absolute entries rather than handing them to profile writers that require absolute paths - /add-dir validates the path is an existing directory, stores it absolute (resolved against cwd), refuses duplicates, and lists the current set with no args - BEHAVIOR_PARITY: only the /add-dir row changes. #166 rewrote the whole table with June-era content that would have regressed /btw, /voice, /tasks and /background back to unshipped. (Prettier reflows the table columns; `git diff -w` shows the 2 real lines.) tsc -b --force, lint (--max-warnings=0), format:check, docs:check clean; 1048 tests pass (9 new helper + 6 new /add-dir); build clean. Co-authored-by: t <t@t> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Security-sensitive — opening for review, not auto-merging.
/add-dironly printed a message;settings.permissions.additionalDirectorieswas declared but consumed nowhere. Now:/add-dir <path>validates the path is an existing directory and persists it topermissions.additionalDirectories(deduped) in the user settings; with no args it lists the current set.withAdditionalWritableDirs(sandbox, dirs)folds those dirs into the sandbox'sfilesystem.allowWrite. The REPL + headless build theirsandboxConfigthrough it, so the sandboxed Bash tool can write to added dirs (beyond cwd). No-op when the sandbox is off; never mutates input.Why this is the right boundary
The file tools (Read/Write/Edit/Glob/Grep) already accept any absolute path () — there's no cwd containment to "enforce". The only thing that restricts writes is the sandbox (for Bash). So
/add-direnforcement = expanding the sandbox's writable roots, which is exactly the user's intent. That's the security-relevant change — hence review.Tests
withAdditionalWritableDirs(add / dedup / undefined-safe / seeds empty) +/add-dir(persist validated dir / reject missing / list). core 646 · cli 143, typecheck +format:checkclean.🤖 Generated with Claude Code