Skip to content

(sandbox): opt-in writable skills and agents - #483

Merged
devsuitup merged 2 commits into
devsuitup:mainfrom
abate:feat/sandbox-rw-skills-agents
Oct 11, 2026
Merged

devsuitup merged 2 commits into
devsuitup:mainfrom
abate:feat/sandbox-rw-skills-agents

Conversation

@abate

@abate abate commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Now: the last review round only

#548 merged an older head of this PR (93672b5), not the approved 8aa60be. This branch is now main plus what 93672b5 lacked:

  • A private ~/.claude entry (shell-snapshots, session-env, backups, state) or ~/.claude.json that is a link into, or contains, an opted-in skills or agents directory refuses the launch (status 125, opt_in_refuse_private).
  • A repository inside an opted-in directory gets a read-only .claude; a read-only directory bound under another spelling is compared by its resolved path.
  • The real-sandbox test covers a project .claude/skills opt-in and the private-entry refusal.
  • The changelog entry moves from v0.0.90 to Unreleased; docs/sandbox.md lists the refusal.

Tests first: cdc61d0 is red on the three ubuntu test jobs, on exactly the four new or extended sandbox-wrapper tests (run 38142875733; the Windows job's one failure, setScheduleEnabled: replaces the file rather than writing into it, is unrelated). The fix 060d0a0 is green on every job (run 38143391048).


Based on #538, which fixes on its own the protection below a symlinked Additional Directory that this PR first carried; its two commits are at the bottom of this branch until #538 is merged.

What

Two opt-in flags for the Linux sandbox, set in the session's Pre-launch Command:

  • SWITCHBOARD_SANDBOX_RW_SKILLS=1 — skills becomes read-write session state in ~/.claude and in every bound directory's .claude.
  • SWITCHBOARD_SANDBOX_RW_AGENTS=1 — the same for agents.

Both are off by default; without them nothing changes.

Why

#372 made skills and agents read-only inside the sandbox, because an unsandboxed claude started later loads them. That blocks workflows where a sandboxed session is meant to author or refine skills and agents. This keeps #372's default and lets a user give up that part of the protection knowingly, the same way SWITCHBOARD_SANDBOX_PODMAN=1 does for the podman socket.

Behaviour with a flag set

  • The entry is added to the user and project state lists, so it is bound read-write on top of the read-only .claude / the ~/.claude tmpfs.
  • ~/.claude/skills / ~/.claude/agents is created before launch when missing, so a first skill is not dropped with the tmpfs. A missing one in a project's .claude cannot be created from inside (that .claude stays read-only) — documented.
  • A symlinked ~/.claude/skills / agents follows the existing state-link rule (same-name directory target, never $HOME or a parent).
  • Links inside the writable entry are no longer followed and protected.

Docs

docs/sandbox.md: table row, new section "Writable skills and agents" stating what protection is given up, and an entry under "What is not isolated". Changelog entry under Unreleased.

Tests

New test in test/sandbox-wrapper.test.js: read-only by default, each flag covers only its own directory, both together, missing ~/.claude/agents created. Red without the script change, green with it.

🤖 Generated with Claude Code

@abate
abate force-pushed the feat/sandbox-rw-skills-agents branch from d5352f2 to d4188a8 Compare October 5, 2026 15:10

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at d4188a8. This weakens the sandbox on request, so I read it as a security change. The default is off and the opt-in stays confined to skills and agents, but one hole blocks.

Blocking

  1. A symlinked skills or agents makes an external repository writable without its protections (scripts/claude-sandbox.sh, the USER_STATE_ENTRIES / PROJECT_STATE_ENTRIES additions and the symlink branch of the mount loop). When ~/.claude/skills (or a project .claude/skills, or agents) is a symlink to another directory, the opt-in adds a writable mount of the target. The existing protection for a linked target (its Git config and hooks, settings linked into it) is not applied to it, so the sandbox can write .git/hooks or a settings file there, and that runs outside the sandbox later. That goes beyond the documented exception ("what is written in skills and agents is loaded by an unsandboxed claude"). Fix: treat the opted-in targets like the other writable targets, protecting Git config/hooks and linked settings inside them. Tests: recording-only tests for an external linked target, and for the enabled case that settings and hooks stay read-only.

Non-blocking

  • With either flag on, the Sandbox tooltip (public/index.html) still promises protection of later executable content and does not state the exception.
  • The new explanatory comments in claude-sandbox.sh exceed the one-line pointer rule; the rationale belongs in docs/sandbox.md.

12 UI tests pass; the 46 wrapper tests are skipped on Windows, so the mount arguments were reasoned from the code, not run. Not verified: a real sandbox, Node 20/22 with c8.

@abate
abate requested a review from devsuitup October 7, 2026 14:04
abate added a commit to abate/switchboard that referenced this pull request Oct 7, 2026
Review of devsuitup#483: with SWITCHBOARD_SANDBOX_RW_SKILLS/AGENTS, a skills or
agents directory linked to a repository elsewhere was bound writable
without the protections other writable roots get, so the session could
write that repository's hooks or a settings file linked into it.

- An opted-in link target is a writable root, so files linked into it
  from ~/.claude are mounted read-only.
- The tree under an opted-in entry, real or linked, gets the same .git
  and .claude protection as a bound directory, without creating .claude.
- The Sandbox tooltip names the opt-in among what is not isolated.
- The test rig drops inherited SWITCHBOARD_SANDBOX_* variables, which
  leaked into the wrapper when the suite ran from a sandboxed session.
@abate

abate commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks. Rebased on current main (d165362) and addressed in 376ada7.

Blocking — linked target writable without its protections

  • An opted-in ~/.claude/skills / agents that is a link now adds its resolved target to WRITABLE_ROOTS, so sandbox_view sees it and a file linked into it from ~/.claude (a settings.json kept in the same dotfiles repo) is mounted read-only like any other link target.
  • The tree under an opted-in entry, linked or real, now goes through the same .git / .claude protection as a bound directory (bind_project_dir's loop, extracted as protect_tree): config, hooks, core.hooksPath and nested .claude read-only. Unlike a bound directory it does not create a missing .claude there.
  • This also closes the same hole for a skill kept as a git repository inside a real ~/.claude/skills (no link involved), which the same loop did not reach before.
  • Project-level: a .claude/skills link is still left as is (not followed), so a target outside what the sandbox sees stays invisible; a test pins that.

Tests (recording-only): an external linked target that is a repository with a .claude and a linked settings.json — target writable, .git/config, .git/hooks, the linked settings and its .claude/settings.json read-only; a repository inside a real opted-in agents — config and hooks read-only, no .claude created; a project link out of the sandbox — no mount. Removing the writable-root registration, either protect_tree call, or switching to create each turns one of them red.

Non-blocking

  • Tooltip (public/index.html, and its copy in docs/sandbox.md): skills and agents under either flag are now listed in "NOT isolated".
  • Comments in claude-sandbox.sh cut to one-line pointers; the rationale and the protection inside opted-in directories are in docs/sandbox.md, "Writable skills and agents".

Also: cleanEnv in test/sandbox-wrapper.test.js now drops inherited SWITCHBOARD_SANDBOX_*. Running the suite from a sandboxed session leaked SWITCHBOARD_SANDBOX_PROJECT_FOLDER (and the opt-in flags) into the wrapper under test, which made four existing tests fail locally for reasons unrelated to the code. All 47 wrapper tests pass on Linux, including the real-bwrap persistence test.

@abate
abate force-pushed the feat/sandbox-rw-skills-agents branch from d4188a8 to 376ada7 Compare October 7, 2026 14:12

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 376ada7. This weakens the sandbox on request, so I read it as a security change. The default is off, the branch is rebased on main, and the commit "protect git and .claude inside opted-in skills and agents" closes the blocker of d4188a8: an opted-in skills or agents that is a symlink to an external repository now gets its Git config and hooks and its .claude protected (protect_tree "$STATE_LINK_TARGET" nocreate). One bypass remains, found by tracing the argument vector and checked in the mount ordering.

Blocking

  1. A shared or nested target lets the writable bind override a read-only protection (scripts/claude-sandbox.sh, bind_user_claude_dir ~487 and the ordering in build_bwrap_args ~759). The mounts are sorted by the depth of the destination and then by insertion order. If ~/.claude/hooks and the opted-in ~/.claude/skills are symlinks to the same directory (or skills points to a directory that contains, or sits under, a protected path such as Git hooks), the read-only mount made for hooks and the writable bind made for skills land on the same destination or on nested ones at the same depth. The loop visits hooks before skills, so the writable bind comes later and wins: the hook scripts become writable and run outside the sandbox on the next session, which is exactly what the read-only .claude is there to prevent. The same happens for a target beneath protected Git hooks. Fix: resolve every opted-in target and refuse (or leave read-only) a target equal to, an ancestor of, or under any protected path, and apply the protections after all the writable binds; add the overlap check to the argument builder rather than relying on the loop order. Tests: recording-only fixtures for an equal target and for an ancestor/descendant overlap, asserting that the protected path ends up read-only in the final vector.

Non-blocking

  • test/sandbox-wrapper.test.js (~517): the new fixtures cover an external linked repository but not equal-target or ancestor overlaps, so they would pass with the bypass present.

Four existing test files pass (37 passed, 49 skipped, 0 failed) on Node 24; all wrapper tests are skipped on Windows and WSL was denied, so Linux mount enforcement is verified only by reading the argument vectors. Not run: Node 20/22 with c8, fresh CI. CI on this head is not approved yet.

@jbr-sekoia

Copy link
Copy Markdown
Collaborator

Tested on Linux (Ubuntu, GNOME on Wayland), PR head 376ada7.

How it was run. scripts/claude-sandbox.sh was run directly under env -i, with a throwaway HOME and a project that is a git repository, and no UI session. Switchboard prepends the Pre-launch Command to bash claude-sandbox.sh … as raw shell, and validatePreLaunchCmd accepts SWITCHBOARD_SANDBOX_RW_SKILLS=1 SWITCHBOARD_SANDBOX_RW_AGENTS=1, so the result is the same. Inside the sandbox, a probe tries to create a file in each directory, or to append to each file. Where a real claude (2.1.292) was used, it is named.

Flags off and on: works

  • Flags off. These are all read-only:
    • ~/.claude/{skills,agents,commands,hooks,settings.json};
    • the project's .claude/{skills,agents,commands,settings.json};
    • .git/config and .git/hooks.
  • RW_SKILLS=1. ~/.claude/skills and the project's .claude/skills are writable. Everything else stays read-only.
  • RW_AGENTS=1. Same, for agents. With both flags on, both directories are writable and nothing else is.
  • Missing agents directory. A missing ~/.claude/agents is created only when its flag is set.
  • Real claude.
    • Flags off: both writes fail with EROFS.
    • RW_SKILLS=1: skills/<name>/SKILL.md is created on the host, and agents/<name>.md fails with EROFS.
    • With RW_SKILLS=1, the CLI also wrote ~/.claude/skills/synced/<org-id>/… on its own: the organisation's synced skills now land in the writable directory.

~/.claude/skills as a symlink to an external repository: works

  • The target holds a .git, a .claude and a nested repository, and ~/.claude/settings.json is a symlink to a file inside it.
  • With RW_SKILLS=1:
    • the target is writable;
    • its .git/config and .git/hooks, and the nested repository's, are read-only;
    • its .claude and the linked settings.json are read-only.
  • With the flags off, the linked skills are not visible, as today for a symlink out of ~/.claude.

The remaining blocking point (a shared or nested target) reproduces live

  • Same target. ~/.claude/hooks and ~/.claude/skills both link to X/skills, with RW_SKILLS=1.
    • Debug output shows ro-bind X/skills, then rw-bind X/skills on the same destination. The later bind wins.
    • echo "echo pwned" >> ~/.claude/hooks/h.sh inside the sandbox changed the hook script on the host.
  • Any read-only entry that shares the target and sorts before skills. With agents and commands linked to the skills target and only RW_SKILLS=1 set, ~/.claude/agents and ~/.claude/commands/cmd.md are writable.
  • A protected path containing the target. With hooks → X/h and skills → X/h/skills, ~/.claude/hooks/skills/inner.sh is writable from inside, and the write reaches the host.
  • Holds: a target containing the protected path. With skills → X/skills and hooks → X/skills/hk, hooks stays read-only, because the deeper read-only mount wins.
  • Holds: core.hooksPath. A project's core.hooksPath pointing at the skills target stays read-only, because the git protections are added later.

Not run: a session started from the Switchboard UI, and the PR's own test file.

abate added a commit to abate/switchboard that referenced this pull request Oct 8, 2026
Review of devsuitup#483: with SWITCHBOARD_SANDBOX_RW_SKILLS/AGENTS, a skills or
agents directory linked to a repository elsewhere was bound writable
without the protections other writable roots get, so the session could
write that repository's hooks or a settings file linked into it.

- An opted-in link target is a writable root, so files linked into it
  from ~/.claude are mounted read-only.
- The tree under an opted-in entry, real or linked, gets the same .git
  and .claude protection as a bound directory, without creating .claude.
- The Sandbox tooltip names the opt-in among what is not isolated.
- The test rig drops inherited SWITCHBOARD_SANDBOX_* variables, which
  leaked into the wrapper when the suite ran from a sandboxed session.
@abate

abate commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks, and thanks @jbr-sekoia for the live reproduction. Rebased on current main (eaf59ff) and addressed in 55ad25a.

Blocking — a shared or nested target overrides a read-only protection

The finished mount list is now checked before launch (check_opt_in_binds, after pin_protected_paths). Each opted-in writable bind is recorded, from ~/.claude (link or real directory) and from a project's .claude. The launch is refused when that bind is also, or lies inside, something the sandbox keeps read-only:

  • Any protected path, visible or not: every path protect_resolved was asked to protect. That covers:
    • link targets of other ~/.claude entries, including hooks → X/h with skills → X/h/skills, where X/h is not otherwise visible and so never got a mount;
    • git config and hooks, including core.hooksPath;
    • targets of links in a project's .claude, such as .claude/hooks → skills.
  • For a ~/.claude entry, a read-only covering mount: the deepest mount at or above the bind, its own excluded, is read-only (e.g. skills → ~/.claude/plugins/skills). This rule is not applied to a project's .claude/skills, which sits inside the read-only .claude by design.

Since the check runs on the whole list, the order the entries are bound in no longer matters. A read-only path inside the writable directory is still allowed: the deeper mount wins, as @jbr-sekoia observed for hooks → X/skills/hk. ~/.claude/skills linked to the project's own .claude/skills is also allowed: both binds there are writable.

I chose to refuse rather than silently leave the directory read-only, so that the flag never quietly does nothing. One consequence: core.hooksPath pointing at the skills target, which held before, is now refused too.

Non-blocking — tests for overlaps

Two new recording-only tests:

  • Refused: hooks, agents, commands or statusline linked to the same directory as skills; commands linked to a real ~/.claude/skills; skills inside the unseen target of hooks; inside ~/.claude/plugins; inside the project's .git/hooks; the project's .claude/hooks linked to its .claude/skills.
  • Allowed: a read-only path inside the target, which stays read-only; ~/.claude/skills linked to the project's .claude/skills; the flag off.

Each of the three rules was removed in turn, and each removal turns a case red. Applying the covering-mount rule to project entries as well turns the existing opt-in test red.

docs/sandbox.md, "Writable skills and agents", describes the refusal.

test/sandbox-wrapper.test.js: 49 passed, 2 skipped (real bwrap is not usable in the session I ran it from). A dry run against a real ~/.claude with both flags launched as before.

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 55ad25a. This weakens the sandbox on request, so I read it as a security change. The default is off. The equal-target and descendant collisions of my last review are fixed with their fixtures (check_opt_in_binds refuses a target equal to, or under, a protected path), the tooltip now states the exception, and the shell comments are fixed. One bypass remains, found by tracing the final mount vector.

Blocking

  1. The same repository can be mounted at two destinations, and only the first is protected (scripts/claude-sandbox.sh, check_opt_in_binds ~703 and the protection pass). The check and the Git config/hooks protection work on destination paths. A symlinked Additional Directory and an opted-in skills target that resolve to the same repository are mounted at two different destinations; the protections (read-only .git/hooks, config, linked settings) are applied to the first one only, and the second stays writable, so the sandbox can write a hook there and it runs outside the sandbox later. Protection across aliases and writable ancestors is still incomplete. Fix: resolve every writable target with realpath, key the protections by the resolved path, and apply them to every destination that maps to a protected real path (or refuse the opt-in when two destinations resolve to the same real path). Tests: recording-only fixtures for an alias of the same repository through an Additional Directory, and through a writable ancestor, asserting that the final vector has the protected paths read-only at every destination.

Non-blocking

  • test/sandbox-wrapper.test.js (~22): the new rationale comment goes beyond the one-line pointer allowed in code.

Four related test files give 46 passed and 51 skipped; every wrapper test is skipped on Windows and WSL was denied, so the mount behaviour is verified only by reading the argument vectors. Not run: a real sandbox, Node 20/22 with c8, fresh CI. CI on this head is not approved yet.

@jbr-sekoia jbr-sekoia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 55ad25a. This weakens the sandbox on request, so I read it as a security change. The default is off, and check_opt_in_binds closes the blocker of 376ada7 for ~/.claude. I ran scripts/claude-sandbox.sh at this head under env -i, with a throwaway HOME and a stand-in claude that tries the writes. Results for the cases in the Linux comment, all with RW_SKILLS=1:

  • hooks and skills linked to the same X/skills: refused.
  • agents and commands, or agents alone, linked to the skills target: refused.
  • hooks → X/h, skills → X/h/skills: refused.
  • skills → X/skills, hooks → X/skills/hk: launches, and hooks/h.sh stays read-only.
  • A project's core.hooksPath at the skills target, linked or a real ~/.claude/skills: refused.

The project-level rule still has a bypass.

Blocking

  1. The project-level overlap check compares two spellings of a path (scripts/claude-sandbox.sh, check_opt_in_binds ~715). For a project's .claude/skills, only the PROTECTED_SEEN rule applies. PROTECTED_SEEN holds resolved host paths. t is the mount destination: $PWD/.claude/skills, or the Additional Directory, spelled as it was reached.
    • How it fails. When the project is reached through a symbolic link, t never matches. The read-only bind made for .claude/hooks -> skills and the writable bind of skills then share a destination, and the later one, skills, wins.
    • When. Switchboard runs cd <spawnCwd> && … claude-sandbox.sh, so $PWD keeps the stored spelling. The same happens with a symlinked $HOME, or an Additional Directory typed through a link.
    • Reproduced. Project .claude/hooks, agents or commands → skills, with RW_SKILLS=1:
      • working directory as the real path: refused;
      • the same project reached through a symlinked parent: launches, and echo >> .claude/hooks/h.sh from inside changed the file on the host;
      • an Additional Directory through a link, and a $HOME that is itself a link: same result.
    • Fix. Compare in one space, e.g. tr="$(readlink -f -- "${M_SRC[k]}")" and case "$tr/" in "$r"/*). With that change, all four reproductions are refused and test/sandbox-wrapper.test.js stays green (50 passed).
    • Test. A recording-only fixture whose working directory goes through a symlink.

Non-blocking

  • An opted-in link is resolved without check_chain (bind_user_claude_dir ~486, bind_state_link_target). If ~/.claude/skills resolves through a link inside the writable project (skills → proj/lnk/skills), the session can re-point that link. The next launch then binds another directory named skills read-write. Reproduced: launch 1 re-pointed proj/lnk to another, unbound project's .claude, and launch 2 wrote that project's .claude/skills/SKILL.md. The docs promise only ~/.claude and bound directories. The read-only links already run check_chain on the link text (protect_resolved_link); the opted-in link could do the same. The precondition is uncommon, and the same gap exists for the other state links, where it is harmless.
  • A mutant survives. -ge → -gt in the covering-mount tie-break (~710) leaves the suite green. I found no layout where PROTECTED_SEEN does not also catch it, so this is coverage only.

Tests: test/sandbox-wrapper.test.js at this head, Node with SWITCHBOARD_TEST_CONCURRENCY=1, GIT_/HUSKY unset: 50 passed, 1 skipped, 0 failed.

Mutations, one at a time; each turned at least one test red:

  • dropping check_opt_in_binds, either of its two rules, or the project registration;
  • not skipping the bind's own index;
  • t/ prefix → exact match;
  • dropping either protect_tree, or the writable-root registration;
  • nocreate → create;
  • the covering rule applied to projects;
  • dropping the pre-creation or the project state entry;
  • ignoring RW_AGENTS.

Not run: a session started from the Switchboard UI, Node 20/22 with c8, CI on this head.

abate added a commit to abate/switchboard that referenced this pull request Oct 9, 2026
Review of devsuitup#483: with SWITCHBOARD_SANDBOX_RW_SKILLS/AGENTS, a skills or
agents directory linked to a repository elsewhere was bound writable
without the protections other writable roots get, so the session could
write that repository's hooks or a settings file linked into it.

- An opted-in link target is a writable root, so files linked into it
  from ~/.claude are mounted read-only.
- The tree under an opted-in entry, real or linked, gets the same .git
  and .claude protection as a bound directory, without creating .claude.
- The Sandbox tooltip names the opt-in among what is not isolated.
- The test rig drops inherited SWITCHBOARD_SANDBOX_* variables, which
  leaked into the wrapper when the suite ran from a sandboxed session.
@abate
abate force-pushed the feat/sandbox-rw-skills-agents branch from 55ad25a to aef998e Compare October 9, 2026 14:00
@abate

abate commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Thanks to both of you. The fixes are in aef998e. The branch is still on eaf59ff: the token I push with lacks the workflow scope that a rebase over the new build.yml needs. It merges cleanly with current main (75ab0ad), and the full suite was run on a rebase onto it.

Blocking

  1. A repository seen at two paths (@devsuitup).
    • protect_resolved now mounts a protected path read-only under every writable root that contains it, not only under the first one, and pins the way to each. A repository reached through a symlinked Additional Directory and through an opted-in skills target keeps .git/config and .git/hooks read-only at both paths. So does one reached through a symlinked ancestor.
    • The same applies to the linked settings and every other path protected through PROTECTED_SEEN.
    • Tracing that case turned up a related gap, also on main: find "$d" does not descend into a starting point that is a symbolic link. A repository or .claude nested below a symlinked Additional Directory was therefore not found. The search now starts at "$d/".
    • Tests:
      • "a repository the sandbox sees at two paths…" covers the alias through an Additional Directory and through a writable ancestor, asserting config and hooks read-only at both destinations.
      • "a repository and a .claude below an Additional Directory that is a link…" covers the find change.
  2. The project-level check compared two spellings (@jbr-sekoia).
    • check_opt_in_binds now resolves the source of every mount (readlink -m) and works in that space:
      • The opted-in directory is refused when it equals, or lies under, a PROTECTED_SEEN path.
      • It is also refused when it is read-only at any place the sandbox sees it, through any mount of a directory that contains it. Access at each such place is decided as before: deepest mount, the later one on the same path. The opt-in's own mount is left out, and so is its own destination for a project's .claude.
    • This also covers the user-level counterpart, e.g. ~/.claude/skills into a project's .claude/plugins while the project is reached through a link.
    • Test: "refuses an opted-in skills that overlaps a read-only path when the project is reached through a symbolic link". It covers hooks, agents and commands → skills, with the working directory:
      • spelled as its real path;
      • reached through a symlinked parent (with PWD passed so bash keeps the logical spelling);
      • reached through an Additional Directory typed through a link;
      • reached through a $HOME that is itself a link.

Non-blocking

  • check_chain on the opted-in link: the text of an opted-in ~/.claude/skills or agents link now goes through check_chain, so skills → proj/lnk/skills is refused. Test: "refuses an opted-in skills linked through a link the session could re-point".
  • The -ge → -gt mutant: it is now killed by a fixture where the tie matters. NVM_DIR is mounted read-only, the same directory is an Additional Directory mounted read-write after it, and skills links inside it. The later mount wins, so this launches; with -gt it is refused.
  • The test comment: the cleanEnv comment is back to main's wording. Only the regular expression changes.
  • docs/sandbox.md covers the resolved-path comparison, the chain check on the opted-in link, and protection at every path a repository is seen at.

Mutations, one at a time. Each turned at least one test red:

  • protecting at the first view only;
  • views and overlap compared in destination space;
  • the PROTECTED_SEEN rule on the destination spelling;
  • dropping the per-view access check;
  • dropping the project's own-view skip;
  • dropping the chain check;
  • find "$d" without the slash;
  • -ge → -gt.

Tests: test/sandbox-wrapper.test.js, 54 passed, 2 skipped (the real-bwrap tests, skipped in my sandboxed session). Full suite: 0 lint errors, and the only failures are the CLI-state canary and test-pr ones that fail the same way on main here.

Not run:

  • a real bwrap launch, and the write-from-inside reproductions;
  • a session started from the Switchboard UI;
  • Node 20 with c8;
  • CI on this head.

@abate
abate requested a review from devsuitup October 9, 2026 14:00

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at aef998e. This weakens the sandbox on request, so I read it as a security change. Thanks for the careful iterations: the previous blockers are closed in the code, and CI (run 37940930328, which tests the merge with current main) is green, including the Linux wrapper tests and the real-bwrap persistence test.

Previous findings

  • d4188a8, blocking (symlinked target left unprotected): resolved, protect_tree "$STATE_LINK_TARGET" nocreate / protect_tree "$e" nocreate (scripts/claude-sandbox.sh:504-516), test test/sandbox-wrapper.test.js:515.
  • d4188a8, non-blocking: both resolved (tooltip public/index.html:109; one-line pointers :107, :715).
  • 376ada7, blocking (shared or nested target): resolved, check_opt_in_binds (:716-739) runs on the finished list after pin_protected_paths (:817-818), fixtures test:587.
  • 55ad25a, blocking (one repository seen at two paths): resolved, protect_resolved mounts at every writable root containing the path and pins each view (:387-405), test test:695.
  • 55ad25a, blocking (two spellings in the project-level check): resolved, sources compared after readlink -m (:720-737), test test:646.
  • 55ad25a, non-blocking (check_chain on the opted-in link): resolved (:498-502), test test:757. Test comment: resolved. -ge → -gt mutant: a fixture now targets it (test:775); not re-run here.

Also tried, with no way out found: a target equal to, containing or under the project; aliases through a symlinked Additional Directory or $HOME; a trailing slash in an Additional Directory (// spellings from find "$d/" stay consistent and fail safe); .claude / .git seen from two roots; dangling and file-typed skills entries; glob characters in paths (every case pattern is quoted).

Major

  1. After merge, the changelog entry lands in a released section. In GitHub's test merge (a098511) and a local merge with 75ab0ad, the (#483) line ends up under ## v0.0.90 — 2026-10-09; the changelog check passes anyway and What's new would credit the feature to v0.0.90. It belongs under ## Unreleased → ### New. Since rebasing onto the new build.yml needs the workflow scope, we can rebase this branch ourselves (maintainer edits are allowed) or move the entry at merge — tell us which you prefer.
  2. This PR also fixes a security bug on main, with no CI run showing it failing first (scripts/claude-sandbox.sh:656, find "$d/"). On main, find "$d" does not descend into a starting point that is a symbolic link, so a repository or .claude below a symlinked Additional Directory gets no protection (GNU find: find link -mindepth 1 prints nothing, find link/ lists the contents). The fix and its test (test:734) arrive in one commit, and no earlier commit of the branch has a CI run. Please split it into its own small PR — test pushed alone so CI shows it red, then the one-character fix, with a Fixed changelog entry — or add a test-only commit before the fix here.
  3. The opt-in has no real-sandbox test, although CI can run one (test/sandbox-wrapper.test.js:1569). The new tests check the recorded bwrap argument vector through the test's own accessAt model (test:136); the bypasses of the last two rounds were only proven by writing from inside a live sandbox, and the real-bwrap persistence test runs on ubuntu-latest (650 ms). Please add an opted-in variant (SWITCHBOARD_SANDBOX_RW_SKILLS=1) asserting that a skill write reaches the host; that inside a skills linked to a repository .git/hooks/pre-commit, .git/config and .claude/settings.json are unchanged; and that writes to hooks, commands and agents (flag off) are still refused.

Minor

  1. :615-637 (nocreate): a repository at the root of, or inside, an opted-in target can be given a new .claude. With ~/.claude/skills -> ~/src/skills (a git repository without .claude) and RW_SKILLS=1, the session can create ~/src/skills/.claude/settings.json with a SessionStart hook that the next unsandboxed claude started there runs. List this in the "Writable skills and agents" exception, or create the placeholder at repository roots inside the target.
  2. The repair of main's symlinked-Additional-Directory protection is not in the changelog, although it changes behaviour for every sandboxed user (see Major 2).

Suggestions

  • check_opt_in_binds (:720-724) forks one readlink -m per mount on each of up to four passes; one readlink -m -z -- "${srcs[@]}" would do, as protect_links_below does with realpath -z.
  • docs/sandbox.md: say that a settings.json hook pointing at a script under skills/ or agents/ runs whatever the session wrote there.
  • ro-bind entries not tracked in PROTECTED_SEEN (RO_DIRS, plain read-only ~/.claude entries) are protected only at their own destination; if one lies inside an opted-in target reached under another spelling it is writable there. A one-line refusal when a RO_DIRS entry or the real ~/.claude lies under an opted-in target would close it.

Not run: a live bwrap launch or write-from-inside reproduction (Windows host); mount reasoning is from reading the script and the recorded argument vectors.

Changes requested: 0 blocking, 3 major, 2 minor, 3 suggestions. Two related issues that already exist on main are filed separately.

@abate

abate commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased on current main (75ab0ad), now that my token can push a branch that contains the workflow change of #499. New head 93672b5; the patch is the same as aef998e, with no conflicts. Full suite and lint at the new head: only the failures this sandboxed environment has on main.

@abate
abate force-pushed the feat/sandbox-rw-skills-agents branch from aef998e to 93672b5 Compare October 9, 2026 14:36
abate added a commit to abate/switchboard that referenced this pull request Oct 9, 2026
Review of devsuitup#483: with SWITCHBOARD_SANDBOX_RW_SKILLS/AGENTS, a skills or
agents directory linked to a repository elsewhere was bound writable
without the protections other writable roots get, so the session could
write that repository's hooks or a settings file linked into it.

- An opted-in link target is a writable root, so files linked into it
  from ~/.claude are mounted read-only.
- The tree under an opted-in entry, real or linked, gets the same .git
  and .claude protection as a bound directory, without creating .claude.
- The Sandbox tooltip names the opt-in among what is not isolated.
- The test rig drops inherited SWITCHBOARD_SANDBOX_* variables, which
  leaked into the wrapper when the suite ran from a sandboxed session.
@abate
abate requested a review from devsuitup October 9, 2026 14:36
@devsuitup

Copy link
Copy Markdown
Owner

Thanks for the rebase. git range-diff of aef998e against 93672b5 shows the same patch (only context lines move), so the findings of the review at aef998e stand.

One thing the rebase did not fix: the (#483) changelog line is still under ## v0.0.90 — 2026-10-09, a released section, in CHANGELOG.md at 93672b5. Now that you can push on top of current main, please move it under ## Unreleased (same subsection).

abate added a commit to abate/switchboard that referenced this pull request Oct 10, 2026
Review of devsuitup#483: with SWITCHBOARD_SANDBOX_RW_SKILLS/AGENTS, a skills or
agents directory linked to a repository elsewhere was bound writable
without the protections other writable roots get, so the session could
write that repository's hooks or a settings file linked into it.

- An opted-in link target is a writable root, so files linked into it
  from ~/.claude are mounted read-only.
- The tree under an opted-in entry, real or linked, gets the same .git
  and .claude protection as a bound directory, without creating .claude.
- The Sandbox tooltip names the opt-in among what is not isolated.
- The test rig drops inherited SWITCHBOARD_SANDBOX_* variables, which
  leaked into the wrapper when the suite ran from a sandboxed session.
@abate
abate force-pushed the feat/sandbox-rw-skills-agents branch from 93672b5 to 85fe1a3 Compare October 10, 2026 09:10
@abate

abate commented Oct 10, 2026 •

Copy link
Copy Markdown
Collaborator Author

Thanks. Rebased on current main (a02f96c, v0.0.92). The changes of this round are in 85fe1a3; the branch now sits on #538.

Major

  1. Changelog. The (#483) line is under ## Unreleased → ### New.
  2. The fix to main's symlinked Additional Directory protection is its own PR, (sandbox): protect repositories below a symlinked Additional Directory #538.
  3. The opt-in now has a real-sandbox test: "from inside a real sandbox, an opted-in skills linked to a repository is writable, and its git and .claude are not".
    • It sets SWITCHBOARD_SANDBOX_RW_SKILLS=1 with ~/.claude/skills linked to a git repository that has a .claude. A new skill written from inside reaches the host.
    • .git/hooks/pre-commit, .git/config and .claude/settings.json of that repository are unchanged after writes from inside.
    • A nested repository gets no .claude/settings.json (see Minor 1).
    • Writes to hooks, commands and agents (its flag off) do not reach the host.
    • It ran, not skipped, on both Linux jobs of CI run 38040430867 on this head, which is green. I also ran it locally against a real bwrap: all 56 wrapper tests pass and 2 are skipped: the check that CI gets the real-sandbox tests (it runs only when SWITCHBOARD_REQUIRE_REAL_BWRAP is set), and the resolv.conf one, which does not apply to this host.

Minor

  1. A repository root inside an opted-in target now gets an empty, read-only .claude before launch, like a repository in a bound directory. The session can no longer give it a settings.json. A folder there that is not a repository gets none; docs/sandbox.md lists that as given up, since a claude started in such a folder later would load what the session wrote. The existing recorded test for a repository inside ~/.claude/agents now asserts the read-only .claude at its root and none in the non-repository parent.
  2. The changelog entry for the main fix is (sandbox): protect repositories below a symlinked Additional Directory #538's ### Fixed entry.

Suggestions

  • readlink: check_opt_in_binds resolves every mount source with one readlink -m -z call.
  • Docs: docs/sandbox.md says that a hook in a read-only settings.json that runs a script under skills/ or agents/ runs whatever the session wrote there.
  • Read-only directories under an opted-in target:
    • A read-only bind whose real path lies inside an opted-in target refuses the launch when the sandbox would see it writable through the target. For example, $NVM_DIR inside the skills target, spelled through a link.
    • New recorded test: "refuses an opted-in skills that contains a read-only directory bound under another spelling". With the flag off, the same layout launches.
    • Plain read-only ~/.claude entries are covered by the same rule, since they are read-only binds.

Checks

  • Mutations: each change below was removed in turn, and each removal fails a test:
    • the placeholder for the link target;
    • the placeholder for a real directory;
    • creating at every level instead of repository roots only;
    • dropping protect_tree on the link target;
    • readlink without -m;
    • the new refusal.
  • Full suite in my sandboxed session: the only failures are the CLI-state canary and the test-pr isolated tests, which fail the same way on main here. Lint: 0 errors.

Not run: a session started from the Switchboard UI.

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 85fe1a3 — request changes (1 major, 2 suggestions). Every previous finding is closed.

Previous findings

Finding Status
Major 1, changelog under v0.0.90 closed — (#483) under Unreleased / New
Major 2, find "$d/" main fix without red CI closed — split into #538: run 38039845996 on 81cf86b fails both ubuntu test jobs on /link/sub/.git/config must stay read-only, run 38040407529 on 4fc768f green
Major 3, no real-sandbox test closed — test/sandbox-wrapper.test.js:1696 runs (not skipped) in job 114179410612 of run 38040430867
Minor 1, repo inside an opted-in dir gets a .claude closed — repo mode at claude-sandbox.sh:624-627; vendored/.claude/settings.json read-only at 85fe1a3, writable at 93672b5
Minor 2, changelog for the main fix closed — #538 has its Fixed entry
Suggestions (single readlink -m -z, hook-under-skills doc, refuse read-only binds under the target) done; the last one does not cover the case below

Earlier blockers (d4188a8, 376ada7, 55ad25a) unchanged; hooks and skills linked to the same directory is still refused (exit 125).

Major — two protections without a source escape check_opt_in_binds (scripts/claude-sandbox.sh:719-749, with :491-492 and :827)
The check only compares the sources of --bind/--ro-bind mounts (:723-727). The private tmpfs entries (shell-snapshots, session-env, backups, state, :491-492) and the private copy of ~/.claude.json (--file 9, :827) have none, so they are never compared with the opted-in directory. If one of them is a symlink into the opted-in skills/agents, the launch proceeds and the host file is writable from inside; unsandboxed sessions later source snapshot/session-env scripts and load MCP commands from ~/.claude.json, i.e. code that runs outside the sandbox. Emulated with the PR's script under Git Bash and a recording fake bwrap (no real bwrap here): shell-snapshots -> skills/snaps with RW_SKILLS=1 exits 0 and skills/snaps/snap.sh is writable (read-only with the flag off); skills -> ../dot/skills, session-env -> ../dot/skills/se, ~/.claude.json -> dot/skills/claude.json exits 0 with both targets writable. docs/sandbox.md:213-217 says the launch is refused for "the target of another ~/.claude entry", which does not hold here. Uncommon layout and an explicit opt-in, hence major rather than blocking. Fix: resolve each private entry that is a link, and $CLAUDE_JSON when it is a link, and refuse when the result equals, lies under or contains the opted-in directory; add recorded fixtures and a line to the real-sandbox test.

Suggestions

  1. Merge #538 first: this branch carries 81cf86b and 4fc768f, and with squash merges #538 would otherwise land empty, detached from its red-then-green record.
  2. The real-sandbox test covers only the user-level skills link; a project .claude/skills opt-in and the refusal paths are checked only through recorded argument vectors.

CI green on 85fe1a3 (runs 38040430867 / 38040430889), mergeable on a02f96c, comments within the rule. Not run here: real bwrap (Windows), any live write inside a sandbox.

abate added a commit to abate/switchboard that referenced this pull request Oct 10, 2026
Review of devsuitup#483: with SWITCHBOARD_SANDBOX_RW_SKILLS/AGENTS, a skills or
agents directory linked to a repository elsewhere was bound writable
without the protections other writable roots get, so the session could
write that repository's hooks or a settings file linked into it.

- An opted-in link target is a writable root, so files linked into it
  from ~/.claude are mounted read-only.
- The tree under an opted-in entry, real or linked, gets the same .git
  and .claude protection as a bound directory, without creating .claude.
- The Sandbox tooltip names the opt-in among what is not isolated.
- The test rig drops inherited SWITCHBOARD_SANDBOX_* variables, which
  leaked into the wrapper when the suite ran from a sandboxed session.
@abate
abate force-pushed the feat/sandbox-rw-skills-agents branch from 85fe1a3 to 8aa60be Compare October 10, 2026 16:35
@abate

abate commented Oct 10, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed at 8aa60be, rebased on #538's new head 5680fee. This should merge after #538.

Major (private entries without a source). bind_user_claude_dir now records each private entry (shell-snapshots, session-env, backups, state) that is a link, and the --file 9 site records ~/.claude.json when it is a link. check_opt_in_binds resolves them (readlink -m) and refuses with status 125 when the result equals, lies under or contains an opted-in directory (opt_in_refuse_private).

  • Recorded test refuses an opted-in skills or agents that overlaps the target of a private ~/.claude entry or of ~/.claude.json: shell-snapshots -> skills/snaps with RW_SKILLS=1 (refused; flag off launches); skills -> ../dot/skills + session-env -> ../dot/skills/se; ~/.claude.json -> dot/skills/claude.json; state pointing at a parent of the linked skills (contains); and a private link beside skills that still launches.
  • Real-sandbox test (an opted-in skills linked to a repository is writable…): a second launch with shell-snapshots linked into the opted-in repository exits 125, and the snapshot is unchanged.
  • Mutation: removing either comparison fails the recorded test; removing both also fails the real-sandbox test.
  • docs/sandbox.md, "Writable skills and agents": a bullet for this refusal and why.

Suggestion 1. #538 first, as above.

Suggestion 2. The real-sandbox opt-in test now also has a project .claude/skills: a skill written there reaches the host, and the project's .claude/settings.json stays read-only. The refusal paths remain checked through recorded argument vectors, plus the one real launch above.

test/sandbox-wrapper.test.js locally: 57/57, real bwrap included.

@abate
abate requested a review from devsuitup October 10, 2026 16:35

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 8aa60be — approve (1 minor, 1 nit).

Previous point Status
Major: private entries and the ~/.claude.json copy not compared in check_opt_in_binds closed — scripts/claude-sandbox.sh:493, :841 record each such entry that is a link, :729-730 resolve it, :744-748 refuse both directions. Replayed under Git Bash with a recording fake bwrap: shell-snapshots -> skills/snaps with RW_SKILLS=1 exits 125 (0 with the flag off); session-env -> ../dot/skills/se and ~/.claude.json -> dot/skills/claude.json, alone and together, exit 125. Same cases against 85fe1a3 exit 0
docs/sandbox.md:213-217 overclaimed closed — docs/sandbox.md:228-235
Merge #538 first still applies (81cf86b, 4fc768f, 5680fee are in this branch with the same SHAs)
Real-sandbox coverage partly — the real test now writes a project skill, checks project settings.json stays read-only and the shell-snapshots refusal; ran as ok 3125 in job 114260269785

Variants refused (125): relative links, chains of 2–3 links, dangling target, trailing slash, x/../x/./, agents with RW_AGENTS=1, ~/.claude.json as a relative link or a chain into agents, a private target containing skills, project .claude/skills as the target of shell-snapshots or ~/.claude.json, ~/.claude itself inside the project's opted-in .claude/skills (via the read-only ide entry). A sibling skills2 is correctly not refused.

Minor — scripts/claude-sandbox.sh:491-493, docs/sandbox.md:228-231: only private entries that are themselves links are checked. ~/.claude/skills -> session-env/skills with session-env a plain directory launches (0): bwrap gets --tmpfs ~/.claude/session-env then a read-write --bind ~/.claude/session-env/skills, so part of the host's session-env is writable. Low impact (the CLI most likely reads only session-env/<session-id>, not verified), but the doc's "a target that contains it is refused" does not hold. Resolve every private entry and $CLAUDE_JSON whether or not it is a link, or narrow the doc.

Nit — a link inside a plain private directory is not checked (shell-snapshots/snapshot-bash-1.sh -> skills/s.sh launches); needs a pre-planted link.

The fix and its tests are one commit (no red CI run); it fixes this PR's own unmerged code, so I am not holding it on that. CI green on 8aa60be (runs 38068273353 / 38068273355), mergeable on a02f96c, changelog under Unreleased, no comments added. Not run: real bwrap, a write from inside a real sandbox.

@devsuitup
devsuitup self-requested a review October 10, 2026 17:17
devsuitup added a commit that referenced this pull request Oct 11, 2026
abate pushed a commit to abate/switchboard that referenced this pull request Oct 11, 2026
abate added 2 commits October 11, 2026 15:02
…, a linked dir, or a repository's .claude

Tests for the last review round of devsuitup#483, which devsuitup#548 did not merge: a
repository inside an opted-in directory keeps a read-only .claude, a
read-only directory bound under another spelling is refused, a private
~/.claude entry or ~/.claude.json linked into an opted-in directory is
refused, and the real-sandbox test covers a project .claude/skills opt-in.
…try, and protect a repository's .claude inside it

A private ~/.claude entry (shell-snapshots, session-env, backups, state)
or ~/.claude.json that links into an opted-in skills or agents would be
writable from inside the sandbox; the launch is refused instead. A
repository inside an opted-in directory gets a read-only .claude, and a
read-only directory bound under another spelling is compared by its
resolved path. The changelog entry moves to Unreleased.
@abate
abate force-pushed the feat/sandbox-rw-skills-agents branch from 8aa60be to cdc61d0 Compare October 11, 2026 13:21
@devsuitup

Copy link
Copy Markdown
Owner

Review of 060d0a0: approved.

  • Same code as reviewed. Apart from the CHANGELOG, the non-test code is identical to what the reviewed head 8aa60be gives on 8b12045. That covers docs/sandbox.md, scripts/claude-sandbox.sh and public/index.html. The CHANGELOG entry is now under ## Unreleased.
  • Red, then green. The tests commit cdc61d0 is red in run 38142875733, on the 4 sandbox tests, on assertions. The fix 060d0a0 is green: Test run 38143391048, 4 test jobs, e2e, lint and changelog.
  • No new defect in the fix commit.
  • Minor, not reproduced. check_opt_in_binds uses mapfile -d '' (bash 4.4 or later) and readlink -m -z (GNU). That is fine on the Linux bwrap path, but would break on a busybox host.

Merging into v0.0.93 at 060d0a0.

@devsuitup devsuitup left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at 060d0a0 (see comment above).

@devsuitup
devsuitup dismissed jbr-sekoia’s stale review October 11, 2026 13:55

Superseded: its three items were verified closed at 060d0a0. The blocking one, compare resolved paths rather than the mount destination, was replayed at 55ad25a then at 060d0a0 through a symlinked cwd, a symlinked Additional Directory and a symlinked HOME: exit 0 then 125. It is covered by test/sandbox-wrapper.test.js:695, which ran in a required job of run 38143391048. The other two items, check_chain on an opted-in link and the tie-break mutant, are closed too: test:803 and test:821.

@devsuitup
devsuitup merged commit 882c0ed into devsuitup:main Oct 11, 2026
12 checks passed
@abate

abate commented Oct 11, 2026

Copy link
Copy Markdown
Collaborator Author

#548 merged 93672b5, an older head of this PR, not the approved 8aa60be. The branch is now main plus that missing last round only (private-entry refusal, read-only .claude for a repository in an opted-in directory, resolved-path comparison, real-sandbox coverage, changelog moved to Unreleased): tests cdc61d0 red (run 38142875733), fix 060d0a0 green (run 38143391048). The body lists it.

@abate
abate requested a review from devsuitup October 11, 2026 13:56
@abate
abate deleted the feat/sandbox-rw-skills-agents branch October 15, 2026 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants