🛡️ fix: Deny a Non-Lane Root's Own Git Metadata in the Native SRT Sandbox - #276
Conversation
…dbox A trusted-vm command in a native (non-lane) SRT workspace could write the registered root's own `.git/hooks/*` and `.git/config`, planting a hook or a `core.fsmonitor` / `filter.*` entry that then runs unsandboxed the next time the user — or trusted worker code — invokes Git in that checkout. The non-lane config relied on SRT's mandatory `.git/hooks` and `.git/config` denies, which on Linux are computed from the worker *process's* current directory (see `sandbox/linux-sandbox-utils.js`). The worker's cwd is its home (the systemd user unit sets no WorkingDirectory), never the registered root, so those denies landed elsewhere and the root's `.git` was writable. macOS was unaffected because it applies global `**/.git/hooks/**` and `**/.git/config` Seatbelt patterns. `NativeSrtWorkspaceCommandSandbox` now adds explicit `denyWrite` entries for the registered root's own Git metadata when `<root>/.git` is a directory (lanes are untouched — they already keep the whole common Git directory read-only): - `<root>/.git/hooks` and `<root>/.git/config` unconditionally; - `<root>/.git/config.worktree` and `<root>/.git/commondir` where they exist; - the same files for every submodule Git directory under `.git/modules/**` (including nested submodules) and every `.git/worktrees/*`. `commondir` and `config.worktree` are denied only where present: Git reads them strictly and SRT would otherwise mask an absent target with an empty `/dev/null` bind Git cannot parse, breaking ordinary commands. Denied files that exist are re-bound read-only, so `.git/config` stays readable (remotes keep working) while writes fail; a benign `git commit` in the workspace is unaffected. Tests: a live SRT test gated on `LIBRECHAT_CODE_LIVE_SRT_TESTS=1` runs the sandbox with `process.cwd()` outside the root and asserts hooks, config, an existing per-worktree config, a submodule config, and a linked-worktree commondir cannot be tampered while a normal commit still succeeds; wired into the Linux native-sandbox CI job. A fast unit test asserts the computed `denyWrite` set. `packages/code/README.md` documents the guarantee and the residual writable-`.git` vectors (whole-`.git` replacement, a fresh top-level commondir, a nested repo) that the personal-machine SRT trust model accepts.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3df2c8771
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| async function collectGitMetadataDenies(gitDir: string): Promise<string[]> { | ||
| const denies = [join(gitDir, 'hooks'), join(gitDir, 'config')]; | ||
| await pushExistingDeny(denies, join(gitDir, 'config.worktree')); |
There was a problem hiding this comment.
When .git/config already contains extensions.worktreeConfig=true but .git/config.worktree does not yet exist, this existence check omits the path from denyWrite. A sandboxed command can then run git config --worktree core.fsmonitor <command>—git config -h defines --worktree as using the per-worktree config file—which creates the omitted file, and the next unsandboxed Git operation executes that command. This occurs in the live test's own repository setup, where the extension is enabled for a linked worktree while the main worktree has no per-worktree config; an empty /dev/null-backed config is valid to Git, so config.worktree should be denied even when absent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not taking the proposed fix: denying an absent config.worktree breaks Git in exactly the repositories this targets. Corrected the misleading comment and documented the case in cdfcce0.
Evidence (Linux, real SRT 0.0.75, bwrap): a repo with extensions.worktreeConfig=true and no main-worktree config.worktree, git status inside the sandbox:
- current code →
exit=0 config.worktreedenied even when absent →exit=128,warning: unable to access '.git/config.worktree': Permission denied…fatal: unknown error occurred while reading the configuration files
SRT blocks creation of a missing path with a /dev/null stub that Git cannot open, and Git treats that as fatal for a config file it is required to read. So "an empty /dev/null-backed config is valid to Git" does not hold here. The same constraint applies to commondir, which Git reads at every startup.
Why this is documented rather than closed: a missing file that Git reads strictly can't be creation-blocked without breaking Git. A new top-level commondir is in the same class, doesn't depend on the extension, and redirects the common directory just as effectively. Closing only config.worktree would not change what a sandboxed command can do. packages/code/README.md now lists "creating a missing config.worktree in a repository that already enables worktreeConfig" next to the replaced .git, new commondir, and nested-repo residuals. The guarantee this PR makes is that existing executable Git metadata can't be modified, which covers the hooks/config vector from the issue and matches macOS. macOS's Seatbelt patterns don't cover config.worktree creation either.
The code comment you anchored on was wrong to imply the extension could only be enabled through the denied .git/config; it now states the residual explicitly.
Also in cdfcce0, from self-review: the deny set is recomputed for every root command, since the worker outlives its initialization snapshot. Metadata the worker cannot inspect now fails the command closed instead of dropping a deny. Both new tests were checked by mutation: each fails with its behavior removed.
The root's Git metadata denies were a snapshot taken at sandbox initialization, but the worker is long-lived: a repository, submodule, or linked worktree created on the host afterwards stayed writable until restart. Ordinary root commands now recompute the set and pass it as a per-command `denyWrite` override (every other filesystem field is the session policy), the way SRT recomputes its own mandatory denies on every wrap. Native Windows keeps the initialization snapshot because srt-win rejects per-command allowRead/allowWrite. Inspecting the metadata now treats only ENOENT/ENOTDIR as absence; any other failure propagates, so an unreadable `.git` fails the command closed (COMMAND_UNAVAILABLE) instead of silently dropping a deny. Corrects the `config.worktree` comment: in a repository that already enables `worktreeConfig`, a missing per-worktree config stays creatable, because denying an absent file makes every Git command fail. The README lists it with the other writable-workspace residuals.
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
What breaks
In a native (non-lane) SRT workspace on Linux, a
trusted-vmcommand couldwrite the registered root's own
.git/hooks/*and.git/config. Thatlets a sandboxed command plant a hook, or a
core.fsmonitor/filter.*/diff.*config entry, that then runs unsandboxed the next time the user — orany trusted worker code — invokes Git in that checkout. Verified live against a
trusted-vmsandbox: from a worker cwd outside the workspace, bothprintf … > .git/hooks/post-checkoutand appending[core] fsmonitor=…to.git/configsucceeded on the host.What triggers it
The non-lane config relied on SRT's mandatory
.git/hooksand.git/configdenies. On Linux those are computed from the worker process's current
directory (
node_modules/@anthropic-ai/sandbox-runtime/dist/sandbox/linux-sandbox-utils.js,const cwd = process.cwd()), and the per-callcwdpassed towrapWithSandboxArgvis documented as unused there. The worker's cwd is its home(
librechat-code@.servicesets noWorkingDirectory), never the registeredroot, so the mandatory denies landed elsewhere and the root's
.gitstayedwritable. macOS was unaffected: it applies global
**/.git/hooks/**and**/.git/configSeatbelt patterns.Behavior after the change
NativeSrtWorkspaceCommandSandboxnow adds explicitdenyWriteentries for theregistered root's own Git metadata when
<root>/.gitis a directory. Laneworkspaces are untouched — they already keep the entire common Git directory
read-only. The denies cover:
<root>/.git/hooksand<root>/.git/config— unconditionally;<root>/.git/config.worktreeand<root>/.git/commondir— where they exist;.git/modules/**(including recursively nested submodules) and every
.git/worktrees/*.commondirandconfig.worktreeare denied only where present, because Gitreads them strictly (commondir at every startup; config.worktree whenever the
worktreeConfigextension is on) and SRT masks a non-existent deny target withan empty
/dev/nullbind that Git then fails to parse — which would breakordinary commands. Existing denied files are re-bound read-only, so
.git/configstays readable (remotes keep working) while writes fail; a benign
git commit,git worktree add,git gc, etc. in the workspace are unaffected..git/infois intentionally not denied: its attributes only referencefilter/diff drivers by name, and those drivers' commands live in the already
read-only config.
Residual, documented limitation
Because the workspace itself stays writable, a command can still stage config
Git will honor later by restructuring the repo — replacing the whole
.gitdirectory, writing a fresh top-level
.git/commondirthat redirects the commondirectory, or initializing a nested repository. Closing those would require
making the workspace's Git storage structurally read-only, which the
personal-machine SRT trust model does not (and which macOS's patterns also do not
cover).
packages/code/README.mddocuments this; use the Docker/NsJail backendor a dedicated VM when a workspace command must be treated as adversarial.
Mechanism
On Linux, an existing deny becomes SRT's
--ro-bind <file> <file>(readable,EROFSon write); a non-existent one becomes--ro-bind /dev/null <leaf>(blocks creation) — the reason the strictly-read
commondir/config.worktreeare gated on existence.
Tests
packages/code/src/root-git-denies-live.test.ts(new, gated onLIBRECHAT_CODE_LIVE_SRT_TESTS=1): runs the real sandbox withprocess.cwd()set outside the root and asserts hooks, config, an existing per-worktree
config, a submodule config, and a linked-worktree commondir cannot be tampered
— and that planting a fresh
.git/hooks/pre-commitis blocked — while a normalcommit still succeeds and no planted marker runs on the host. Wired into the
linux-native-sandbox-testsCI job.packages/code/src/native-sandbox.test.ts(new case): asserts thecomputed
denyWriteset for a non-lane root, including recursive submodule andlinked-worktree metadata, and that an absent top-level
commondir/submodule
config.worktreeis left alone.npm test(Node 20/22/24) andnpx tsc --noEmitpass inpackages/code.Related to the linked-worktree lane work (#270, #272, #274), which already
handled the lane variant of this surface.