fix(sandbox): protect daemon token file - #685
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe daemon token file is canonicalized before remote serving, added to mandatory sandbox protections, excluded from search and file tools, and removed from spawned command environments. Patch parsing now fails closed for ambiguous paths. Tests cover platform enforcement and path edge cases. ChangesDaemon token protection
Estimated code review effort: 5 (Critical) | ~100 minutes Suggested reviewers: Merge Risk: 🟠 High · up to This PR strengthens daemon-token protection across child processes and file tools, but concurrent filesystem changes can still expose or overwrite the token during protected reads and writes. The security boundary is therefore not safe to merge until those race conditions are addressed. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR closes a sandbox escape where ZERO_DAEMON_REMOTE_TOKEN_FILE could be inherited by sandboxed commands (allowing them to locate and read the daemon bearer token file under the read-all posture). It scrubs the pointer env var across platforms and extends the existing “credential deny-read” profile logic to also deny reads of the referenced token file where deny-read enforcement is supported.
Changes:
- Scrub
ZERO_DAEMON_REMOTE_TOKEN_FILEfrom sandbox command environments (in addition to the inline token env var). - Extend
credentialDenyReadPathsto include the path named byZERO_DAEMON_REMOTE_TOKEN_FILE(alongsideGOOGLE_APPLICATION_CREDENTIALS) and plumb this through the pure helper. - Add/extend regression tests covering env scrubbing and permission-profile deny-read construction (skipping the deny-read assertion on Windows per existing platform limitations).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/sandbox/runner.go | Adds ZERO_DAEMON_REMOTE_TOKEN_FILE to the sandbox env scrub list. |
| internal/sandbox/runner_test.go | Extends env scrubbing regression test to ensure the pointer env var is removed. |
| internal/sandbox/profile.go | Adds ZERO_DAEMON_REMOTE_TOKEN_FILE to default credential deny-read path construction and updates helper signature/docs. |
| internal/sandbox/manager_test.go | Updates credential deny-read tests for the new parameter and adds a profile-level regression test for daemon token file denial (non-Windows). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Deny writes to the daemon token file on macOS as well
internal/sandbox/profile.go:176
The new target entersDenyRead, but the Seatbelt backend translates that only intofile-read*and unlink denials. Its broadfile-write*allowance still covers every workspace root and the default temporary roots. Therefore, whenZERO_DAEMON_REMOTE_TOKEN_FILEnames a file under/tmpor another writable root, a sandboxed command can discover the filename from its parent directory and overwrite or truncate the bearer-token file. This makes the remote bridge unavailable and can replace its credential on a restart/reload. Add a write denial for credentialDenyReadfiles in the Seatbelt profile (and a macOS regression case for a token under a writable temporary root).
Address code review on PR Gitlawb#685: the Seatbelt profile only translated DenyRead entries into file-read* and file-write-unlink denials. The broad file-write* allowance for workspace/temp write roots still covered a DenyRead file (e.g. the file ZERO_DAEMON_REMOTE_TOKEN_FILE names) if it happened to sit under one of them, so a sandboxed command could discover and overwrite/truncate the daemon bearer-token file even though it couldn't read or delete it. A file a sandboxed command must not read has no legitimate reason to be written either, so seatbeltProfileFromPermissionProfile now also emits a full file-write* deny for every DenyRead path, placed after the broad write allow (deny rules that follow an allow win, matching the existing DenyWrite/metadata-carveout ordering). Adds a regression test with a DenyRead file under a writable /tmp root, and extends the existing deny-ordering test to assert the new file-write* rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Re-reviewing against commit 2248aca8 (head). The macOS Seatbelt fix (patch 2/2) is the right primitive: a DenyRead file that's also under a writable root was overwritable/truncatable because the prior profile only emitted file-read* and file-write-unlink, not file-write*. Denying the full write direction for every DenyRead path is correct, the ordering (deny after the broad allow) is correct, and TestSeatbeltProfileDeniesWritesToDenyReadUnderWritableRoot covers both the rule presence and the ordering. The TestSeatbeltProfileProtectsMetadataAndDenyOrdering extension covers the general case.
LGTM.
Cross-PR note: #685 depends on the credentialDenyReadPathsIn signature change from #681 (daemon token file as a parameter) and the scrubSensitiveEnv plumbed sensitiveEnvKeys from #682. Recommend rebasing #685 onto #681 + #682 in that order.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Local review: built and ran go test ./internal/sandbox on darwin/arm64; all pass. The deny-write-for-DenyRead fix is a genuine security improvement (closes the truncate/overwrite bypass under a writable root). One integration note.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Protect the configured symlink pathname as well as its target
internal/sandbox/profile.go:200
normalizeProfilePathsresolvesZERO_DAEMON_REMOTE_TOKEN_FILEthrough symlinks before it is added toDenyRead. If the configured pathname is a symlink under a writable root such as/tmp, the new deny rules protect only its current referent; a sandboxed command can unlink the writable symlink and recreate a regular file at the configured pathname. On the next remote-daemon start,TokenFromEnvreads that replacement pathname and accepts the attacker-chosen bearer token (or fails, causing a denial of service). Preserve and deny the lexical configured path in addition to its resolved target, and add a symlink-replacement regression test.
There was a problem hiding this comment.
Approving clean security hardening. Scrubbing ZERO_DAEMON_REMOTE_TOKEN_FILE from child envs and adding its target to the credential deny-read set closes a real hole (a sandboxed command could otherwise resolve the pointer and read the daemon bearer-token file under the read-all posture), and extending the macOS seatbelt profile to file-write*-deny every DenyRead path is the right fix: denyReadRules only blocked read and unlink, leaving a credential file under a writable root overwritable/truncatable. I checked the Linux bubblewrap path and it already bind-mounts DenyRead targets read-only, so this just brings macOS to parity. One thing to be aware of: the write-deny now covers all DenyRead paths (~/.aws, ~/.azure, etc.), so no sandboxed command can update cloud creds consistent with the existing unlink-deny and fine under the current threat model, just calling it out.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/profile.go`:
- Around line 320-328: Keep normalizeProfilePath purely lexical by removing its
filepath.EvalSymlinks resolution and returning the result of
normalizeProfilePathLexical unchanged. Resolve symlinks only within
normalizeProfilePathVariants while retaining both the configured lexical path
and resolved target for deny-policy expansion, and add a regression test
covering a writable denied symlink.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 974aa02c-d6a1-45e8-ae0b-c2df72771e98
📒 Files selected for processing (4)
internal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/runner.gointernal/sandbox/runner_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Do not pass a lexical symlink to Bubblewrap's deny mount
internal/sandbox/profile.go:200
For an existingZERO_DAEMON_REMOTE_TOKEN_FILEsymlink, the new variant list includes the symlink pathname as well as its target. The Linux backend then emits--ro-bind /dev/null <symlink>for that pathname; Bubblewrap rejects a symlink mount destination before the command starts (Can't create file at .../daemon-token: No such file or directory). Thus configuring the supported token-file option through a symlink makes every Linux sandboxed command fail to launch. Materialize/protect that pathname with a Bubblewrap-safe mechanism (or avoid adding it to the Linux deny-mount list) and add a Linux regression test. -
[P1] Resolve the token-file path in the daemon's context, not each worker's
internal/sandbox/profile.go:195
TokenFromEnvaccepts relative token paths, andserve-remotereads one before it starts workers. The daemon then preservesZERO_DAEMON_REMOTE_TOKEN_FILEfor workers whosecmd.Diris the per-sessionspec.Cwd;normalizeProfilePathLexicalconsequently turnstokeninto a path beneath that session instead of the daemon startup directory that contains the actual bearer-token file. The real file is left outsideDenyReadunder the read-all posture, so a sandboxed command that can infer its location can read it. Normalize the value at the daemon boundary (or pass an already-absolute protected path) and cover a remote worker whose session CWD differs from the daemon CWD.
|
Following up on my earlier approve, which I am pulling back from for now. jatmn's latest P1 is a real one: the symlink-protection commit adds the ZERO_DAEMON_REMOTE_TOKEN_FILE symlink pathname itself, not just its resolved target, to the Linux deny-mount list, and Bubblewrap rejects a symlink as a mount destination, so every sandboxed command on Linux fails to launch when that option points at a symlink. I am on Windows and cannot reproduce the bwrap behavior here, but jatmn tested it on Linux with the exact "Can't create file ... daemon-token" error and the mechanism is sound. The target protection and the macOS write-deny are still the right hardening. This just needs the Linux side to protect that pathname without ro-binding the symlink itself (materialize it, or keep the symlink pathname off the Linux deny-mount list). Not re-approving until that is closed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/linux_helper.go`:
- Around line 319-324: Add the same lexical-symlink guard used in the DenyRead
path to appendReadOnlyLinuxPathArgs, checking the mount path with os.Lstat and
returning the existing args unchanged when it is a symlink. Keep the current
handling for non-symlink paths unchanged.
In `@internal/sandbox/profile.go`:
- Line 325: The FileSystemPolicy initializers in PermissionProfileFromPolicy and
seatbeltCompatibilityPermissionProfile must preserve both lexical and resolved
paths for user deny policies. Replace single-path normalization for
policy.DenyRead and policy.DenyWrite with normalizeProfilePathVariants, while
leaving normalizeProfilePath unchanged for other uses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cbb0b9b3-3559-4c77-bb5a-2c1692650e7a
📒 Files selected for processing (7)
internal/cli/daemon.gointernal/cli/daemon_test.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/runner_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/sandbox/runner_test.go
- internal/sandbox/manager_test.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Preserve the resolved target for user-configured
DenyReadsymlinks
internal/sandbox/profile.go:104
normalizeProfilePathis now lexical-only, while this initializer still usesnormalizeProfilePathsfor policy entries. On Linux,appendUnreadableLinuxPathArgsthen skips that symlink mount destination and no resolved target is present (unlike the credential-path branch). Thus a policy such asdenyRead: [link], wherelinkpoints to a secret, produces no deny mount under the read-all profile and the sandboxed command can read the target. Keep both variants for deny paths (and update the macOS compatibility initializer) so the Bubblewrap-safe target is actually denied. -
[P1] Do not use lexical paths for ordinary sandbox roots
internal/sandbox/profile.go:324
This changed the shared normalizer used forworkspaceRoot,AllowWrite, andDenyWrite, not just the new credential deny variant. A workspace opened through a symlink now reaches Linux Bubblewrap as--bind <link> <link>; Bubblewrap rejects a symlink mount destination, so every sandboxed command fails before it starts. I reproduced the failure with a symlinked workspace. Restore resolved normalization for ordinary roots and keep lexical-plus-resolved handling scoped to deny-path expansion. -
[P1] Do not leave a writable token-file symlink unprotected on Linux
internal/sandbox/linux_helper.go:319
Skipping the lexical symlink avoids Bubblewrap's invalid mount destination, but only its original target is masked. IfZERO_DAEMON_REMOTE_TOKEN_FILEis a symlink under a writable root such as/tmp, a sandboxed command can replace it with a link to another host-readable file and read through the replacement; it can also corrupt the daemon's token path. The test currently asserts the unsafe omission. Protect or materialize the lexical pathname with a Bubblewrap-safe mechanism rather than simply dropping its deny rule. -
[P1] Handle symlinked parent directories before emitting a deny mount
internal/sandbox/linux_helper.go:319
TheLstatcheck catches only a final-component symlink. For a supported token path such as/tmp/linkdir/token, wherelinkdiris a symlink,Lstat(token)reports a regular file and the helper emits a deny mount through the symlinked parent. Bubblewrap rejects that destination and every Linux sandbox launch fails. Detect path traversal through a symlink (or omit the lexical variant after retaining the resolved target) and add a regression case for this layout.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/linux_helper.go`:
- Around line 323-349: The Linux path argument helpers currently abort on
lexical symlinks instead of skipping them when their resolved target is also
protected. Update the profile-processing flow around appendReadOnlyLinuxPathArgs
and appendUnreadableLinuxPathArgs to recognize lexical symlink entries whose
resolved targets exist in the same deny set, skip those entries, and continue
enforcing the target; retain the existing error behavior when no enforceable
target is present. Update the related test to assert successful sandbox startup
and target enforcement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baa5d0ce-25e0-42a5-8752-15ae141e7d1d
📒 Files selected for processing (7)
internal/cli/daemon.gointernal/cli/daemon_test.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/sandbox/manager_test.gointernal/sandbox/profile.gointernal/sandbox/runner.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/daemon.go
- internal/sandbox/runner.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Keep the remote token excluded from in-process file tools
internal/sandbox/profile.go:104
The new daemon-token path is added only toPermissionProfile.FileSystem.DenyRead, which protects wrapped shell commands. Built-in tools do not consume that profile:read_filereads scoped files directly, and grep/glob exclusions are built fromPolicy.DenyRead. If the token file is inside a remote session workspace (for example, a daemon started with a relative token-file path from that workspace), a remote-controlled agent can useread_fileto exfiltrate the bridge bearer token. Apply the automatic credential exclusion to the in-process read/search tool boundary as well, and cover this with an end-to-end tool test. -
[P1] Preserve inline-token precedence when a token-file variable is stale
internal/cli/daemon.go:480
TokenFromEnvintentionally returns a nonemptyZERO_DAEMON_REMOTE_TOKENbefore consultingZERO_DAEMON_REMOTE_TOKEN_FILE, but this new preflight resolves the file first. Consequently, a valid inline token plus an inherited missing or dangling token-file variable now makesdaemon serve-remoteexit instead of starting. Only canonicalize the file when it is the selected source (or otherwise leave an ignored file pointer from changing the result), and add the both-variables regression case. -
[P1] Do not make symlink-backed credential paths disable every Linux sandbox command
internal/sandbox/linux_helper.go:344
The profile now deliberately retains both lexical and resolved forms of every credential/deny path, but the Linux argument builder aborts whenever either form has a symlink component. This makes common configurations such asGOOGLE_APPLICATION_CREDENTIALS=/var/run/...(where/var/runis commonly a symlink to/run) fail plan construction for every sandboxed command; the pre-PR profile kept only the resolved target. Preserve the denial of the resolved target while using a Bubblewrap-safe treatment for the lexical path instead of turning a valid credential configuration into a global sandbox-startup failure.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/engine.go`:
- Around line 57-75: Update withAutomaticDenyRead to recompute automaticDenyRead
from the current effective policy before merging it with policy.DenyRead, rather
than reusing the constructor-time list. Ensure credential paths allowed through
session or turn permission profiles are removed from the automatic deny set
while preserving deduplication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 059619b5-dbbc-4812-a361-6fad61cca69c
📒 Files selected for processing (6)
internal/cli/daemon.gointernal/cli/daemon_test.gointernal/sandbox/engine.gointernal/sandbox/linux_helper.gointernal/sandbox/linux_helper_test.gointernal/tools/read_exclusions_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/cli/daemon.go
- internal/sandbox/linux_helper.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
@Vasanthdev2004 lgtm off to you
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make the unified-diff executor use the exact parser used for authorization
internal/tools/apply_patch.go:162-176,249-258
apply_patchfirst obtains target paths fromsandbox.PatchHeaderPathsand rejects a protected target before any mutation. That parser intentionally treats every unquoted byte after---or+++as pathname data (apart from a tab-delimited timestamp), including leading and trailing spaces. The executor then reparses those same headers withpatchFileHeaderPath, which callsstrings.TrimSpace; its rename/copy handling ininternal/tools/unified_patch.go:219-228does the same. Consequently a patch whose authorization headers name the unprotected siblingbridge-tokencan pass validation, while the executor trims the name tobridge-tokenand mutates the selected bearer-token file. The inverse mismatch can also make a valid whitespace-bearing filename execute against a different file.Please remove this second, byte-changing interpretation of patch paths rather than adding another targeted deny. Make the parser that defines the authorization target also provide the executor's source/destination paths (including
diff --git,---/+++, copy, and rename forms), or consolidate both consumers behind one parser with an explicit byte-preservation contract. Add end-to-end regressions for unquoted and C-quoted leading/trailing-space names, covering ordinary update, copy, and rename paths, and assert both that an unprotected control patch operates on its literal filename and that a protected token remains unchanged. Preserve the existing rooted/no-follow mutation flow,/dev/nullsemantics, tab-separated timestamps, and Git quoting support.
Implementation guidance
This PR has accumulated security fixes across token selection, path normalization, profile generation, OS backends, MCP, direct tools, and patch execution. The recurring review pattern is not simply missing checks; it is multiple layers independently interpreting the same security-sensitive pathname. A check is only load-bearing when the next layer consumes the same identity and bytes.
For the remaining work, please treat the token pathname and patch target as explicit cross-layer contracts. Define one authoritative representation for each supported patch header form, carry that representation from authorization through operation planning and rooted file mutation, and make each downstream consumer use it rather than reparsing raw input. Test the full lifecycle—not only parser output—using whitespace, quotes, symlink/canonical aliases, copy/rename, and failure paths. For every regression, include an unprotected control that proves the patch format is executable, then verify the protected-token variant is rejected before any read, rename, or write. This will address the root cause (parser/consumer divergence) without broadening the PR into unrelated sandbox redesign.
apply_patch authorized a unified diff with sandbox.PatchHeaderPaths, whose contract is that every unquoted byte after "--- ", "+++ ", "rename from " and friends is pathname data. The executor then re-read the same headers through its own parser, which trimmed surrounding whitespace and unquoted differently. The two layers could therefore name different files: a patch whose authorization headers say `bridge-token ` cleared the gate as an unprotected sibling, while the executor resolved the trimmed `bridge-token` — the selected remote bridge token beside it. Remove the second interpretation instead of adding another targeted deny. The parser that defines the authorization target now also supplies the executor's source and destination for every supported header form — `diff --git`, `---`/`+++`, copy and rename — through an exported surface carrying one byte-preservation contract, and the executor's own trimming parsers are gone. A header form the parser cannot interpret exactly is a patch refusal, matching the gate's fail-closed behavior. The end-to-end regressions cover unquoted and C-quoted leading- and trailing-space names across update, copy and rename. Each proves both halves: the whitespace-bearing name is a real, patchable file whose control effect lands byte for byte, and the protected token one byte away is unchanged. The inverse suite makes the same names the token and asserts refusal before any read, rename or write. Restoring this fidelity also fixes the leading-space copy that previously trimmed itself into a name that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RT1ZzPYPy1jzKXSMPYTKaZ
|
Addressed the P1 on The mismatch. Authorization used The fix, per the guidance: remove the second interpretation rather than add another deny. The parser that defines the authorization target now also supplies the executor's source and destination for every supported header form. One deliberate byte-level detail: Regressions (
Verified they fail on the pre-fix tree: the four whitespace cases were refused with "holds the remote bridge token and is never writable", i.e. the executor really was resolving the sibling onto the token. Restoring this fidelity also fixed a real behavior gap, so one existing expectation changed: the
|
|
My original blocker is closed, and I verified it the way I asked you to: mutating I cannot clear the verdict though, because this head introduces a sandbox write bypass that main does not have. A unified diff with mismatched The evasive patch is just It is a regression, not something you inherited. Identical probe on Main denies both, because main applied the per-header strip on the gate side too, so the two layers agreed on what path a header names. The mechanism is two spellings of one path with no contract between them. The gate now normalises as a pair: func hasDefaultGitPrefixes(source, destination string) bool {
return len(source) > 2 && len(destination) > 2 &&
strings.HasPrefix(source, "a/") && strings.HasPrefix(destination, "b/")
}so func stripPatchPrefix(path string) string {
if strings.HasPrefix(path, "a/") || strings.HasPrefix(path, "b/") { path = path[2:] }
...
}called separately at Everything the gate derives from those paths is affected the same way, not just DenyWrite: out-of-workspace containment, the permission preview the user actually approves, and the changed-file list. The daemon token specifically survives, but only because The fix is to make one function answer "what path does this header name" for both layers. Whichever rule you pick is fine as long as the gate and the executor cannot disagree; the pair-matching rule is defensible on its own, it just has to be the executor's rule too. Worth a regression that drives a mismatched-prefix patch through |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes, and I need to own the position I am reversing: I approved b9b6de87, and the defect below arrived in that head. My approval missed it, so this is not a new ask appearing late so much as one I should have made two rounds ago.
@jatmn's grep P1 is closed, and closed well. scanGrepFile re-asks after its own open, before any byte is read, and readExcluder.handle is wired at both constructors so the engine-less path is covered too. Deleting the four-line check kills exactly one test, and that test runs rather than skipping on Windows.
The byte-fidelity half of his second P1 is closed too. strconv, TrimSpace and unquoteGitPath are gone and both files call the sandbox parser. Re-inserting a single strings.TrimSpace kills two committed tests.
The gate and the executor still choose different files
What that P1 asked for was one parse and one answer. That is still violated, in two spellings. I drove both myself on this head and on the merge base rather than reasoning from the diff.
A git-generated delete or create. flushSection takes the unified branch only when both operands are non-/dev/null, so every create and delete falls through to case section.diffParsed, which uses the diff --git operands and never looks at the surviving --- or +++ header. That branch has no cross-check. Same patch, both trees:
patch: diff --git a/decoy.txt b/decoy.txt
deleted file mode 100644
--- a/secret.txt
+++ /dev/null
HEAD gate sees: [decoy.txt decoy.txt]
BASE gate sees: [decoy.txt decoy.txt secret.txt /dev/null]
And the executor on this head, from parseUnifiedPatch on the same bytes:
op[0] = {kind:delete path:secret.txt verifyDelete:true}
So the gate evaluates decoy.txt, and the tool deletes secret.txt. A DenyWrite entry on secret.txt is never consulted, and because MutationTargets is built from the same gate paths, /rewind never snapshots the file the patch destroys.
The a/ b/ prefix. The gate strips it only as a matched pair, the executor strips each header independently and unconditionally:
patch: diff --git a/secret.txt secret.txt
--- a/secret.txt
+++ secret.txt
HEAD gate sees: [a/secret.txt secret.txt]
BASE gate sees: [secret.txt secret.txt secret.txt secret.txt]
Base was a deliberate over-collection: it gathered operands from the diff --git line and the unified headers and evaluated the union. Narrowing that to one parse is the right direction, and it is what the P1 asked for, but the parse it kept is not the one the executor uses. The patch text is entirely model-controlled, which is the threat model this PR exists for.
The token itself is not reachable this way. resolveStructuredPatchTarget calls protectedMutationDenied on every executor-side target, so the same shape aimed at the bridge token is refused for move, delete and copy. The headline protection holds. What is bypassed is the user-configured DenyWrite boundary and the rewind snapshot, which is why I am blocking on it rather than treating it as a hardening nit.
Nothing pins the agreement, either. The eight TestPatchHeaderPaths* cases all assert what the sandbox parser returns; none compares that set against the operations parseUnifiedPatch actually produces. That comparison is the test I would want here.
One more I verified directly
Setting ZERO_DAEMON_REMOTE_TOKEN_FILE disables format-on-write and inline diagnostics for the whole workspace, including files with no relation to the token, with nothing said about why. credentialsActive := protectedCredentialsActive(tool.workspaceRoot) is a workspace-level boolean, and it gates maybeFormatWrittenFile and inlineDiagnostics at write_file.go:121/:146 and edit_file.go:180/:215. The decision belongs on the target, which protectedMutationDenied already answers per path. The diagnostics half is the one that matters: the agent stops being told about errors it just introduced.
Reported by my verification, not driven by me
Flagging these as second-hand so you can weigh them accordingly. Moving write_file and edit_file onto writeRootedFile (temp plus rename) is said to carry three side effects onto those two tools that structured_patch.go already had: an explicitly hardened ACL is replaced by the inherited one on rewrite, a write now fails while another handle holds the target open, and hard links break silently. The first is the one worth checking on a credential-protection PR. I did not reproduce any of the three.
Amp-Thread-ID: https://ampcode.com/threads/T-01a063ab-ba5f-7319-bbb6-3dfca232439e Co-authored-by: Amp <amp@ampcode.com>
|
PierrunoYT addressed these findings in 71e95173:
Validation completed:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
PR Review: #685
PR: #685
Head reviewed: 71e951733287ffa6169949b81db615fc269e391e
Base used for attribution: merge base 27b319ca88a3180bed5183f0c599e9307f3ece12
Live target observed: 1b5db1765672820caac1684b168c9898b5ba3593
Summary
This is intended as a consolidated final-round review, not another drip pass. I rechecked the complete 55-file diff, every prior accepted/rejected class that remains relevant, both directions of base attribution, and the live target. Two blocking security defects remain in the daemon-token protection boundary; both are instances of the same root cause that produced the earlier rounds. No additional candidates survived the reconciliation.
The branch is also two commits behind the live target and must be brought onto current main under the repository's fresh-base rule. All current hosted checks are green, the synthetic merge is conflict-free, and focused local race tests, go vet ./..., go build ./..., and git diff --check passed.
Merge readiness
-
[P1] Bring the branch onto current
mainbefore merge.The reviewed head is two commits behind live
main(1b5db1765672820caac1684b168c9898b5ba3593), including the mainline MCP OAuth protected-resource work. The synthetic merge is conflict-free and no semantic overlap with the token-resource changes was found, but the repository guidance makes a fresh base a hard merge gate. Rebase or merge currentmain, retain those target-only commits, and rerun the checks.
Findings
-
[P1] Keep post-write helpers inside the daemon-token boundary —
internal/tools/write_file.go:116The direct write itself is now bound correctly:
writeRootedFileopens the target underos.Root, compares the opened object with the protected credential set, and writes through that same handle. The authorization boundary ends when that call returns, however.write_fileimmediately passes the original pathname tomaybeFormatWrittenFileand later toinlineDiagnostics;edit_filedoes the same atinternal/tools/edit_file.go:177,210.Both downstream consumers select the security-relevant object again:
maybeFormatWrittenFilelaunches an in-place formatter againstabsolutePath, then callsos.ReadFile(absolutePath)atinternal/tools/format_on_write.go:87-98. Neither operation is tied to the handle that passed the write check.- The production diagnostics adapter calls
os.ReadFile(absPath)atinternal/agent/file_diagnostics.go:32before sending those bytes to the language server. That read is likewise outside the protected-open primitive.
A concurrent workspace writer can therefore replace an ordinary target after
writeRootedFilewith a symlink or hard link to the daemon-token object. The formatter can modify the credential in place; the formatter reread or diagnostics read can then put the bearer bytes into tracker/preview state, tool output, or an LSP request. This is the same check/use shape the PR already fixed in grep: the earlier decision described one object, while the lateropenselected another. The blind searches independently reproduced the essential filesystem behavior by showinggofmt -wmodify a target reached through a swapped symlink.There is also a race-independent disclosure path.
exec.CommandContextleavesCmd.Envnil, so the formatter inheritsZERO_DAEMON_REMOTE_TOKEN,ZERO_DAEMON_REMOTE_TOKEN_FILE, andZERO_INTERNAL_DAEMON_REMOTE_TOKEN_FILE_RESOLVED. A repository-selected/configured formatter or plugin therefore receives the bridge credential variables directly. That contradicts this PR's stated env-scrub contract that the pointer must not reach child processes.Root cause: the code treats the protected write syscall as the complete security operation, although the logical tool operation continues through formatter execution, a second file read, diagnostics, tracking, and preview construction. Child-process scrubbing is also applied at the sandbox runner rather than at every process-launch boundary that can execute repository-influenced code. The last commit makes this explicit: it removes the earlier
credentialsActiveguard around formatting and diagnostics to restore those features for ordinary files, but it does not replace that coarse guard with target-bound protection.Bounded fix guidance: preserve formatting and diagnostics for ordinary files, but make all bytes and side effects after the write derive from protected, verified objects. Diagnostics should read through the same rooted, opened-handle credential check used elsewhere. For formatting, do not rely on a pathname precheck followed by an in-place external open; that recreates the race. One valid shape is to format detached/staged content with a scrubbed environment, verify the formatter result, and publish it through the existing rooted protected-write primitive. A descriptor-bound or otherwise race-free equivalent is also fine. Use the repository's central sensitive-environment scrub for the formatter child instead of maintaining another variable list. If the post-write target can no longer be proven to be the authorized ordinary object, fail/skip that post-processing for that invocation without globally disabling the features whenever a token is configured.
Add deterministic regressions that pause between the rooted write and each downstream consumer, swap the ordinary target to both a symlink and a hard-link alias, and assert that the token is neither changed nor returned to the tracker, preview, diagnostics output, or LSP. Add a formatter-helper regression that records its environment and proves all three token variables are absent. Keep positive controls proving ordinary files still format and receive diagnostics when a token is active.
-
[P1] Preserve the startup token object's identity across path rotation —
internal/sandbox/pathlists.go:255The new source model correctly distinguishes the operator-configured spelling from the symlink-resolved pathname selected at daemon startup. It does not, however, retain the selected object.
PersistSourcestores only those two path strings in environment variables (internal/remotetoken/source.go:84-89),FileSource.Pathsreturns only strings, andprotectedInfoDeniedcallsos.Stat(entry)again for every later access. By contrast,TokenAuthenticatorretains the bytes read at startup for its lifetime (internal/daemon/remote/auth.go:47-68).Consider a regular token file at pathname T with inode A and a hard-link alias H to A. The daemon starts, reads A's bytes, and continues accepting them. An operator or credential manager then performs the usual atomic replacement of T: create inode B and rename it over T. The configured and resolved strings still name T, so every later
os.Stat(T)identifies B. An in-process tool opening H identifies A;os.SameFile(A, B)is false, so the new handle-bound predicate permits the read even though A contains the bearer the running authenticator still accepts.read_file, grep, and MCP resource reads all ultimately rely on this predicate and can return the live credential through H.This is not a request for general cross-process locking or a new rotation feature. The PR already states that the resolved startup object remains protected for the run, calls
ReadPaththe object "pinned at startup," and tests symlink retargeting as an until-restart invariant. The implementation currently pins a pathname while the authenticator pins bytes; those lifetimes diverge when the pathname's object is replaced.Root cause:
FileSourceuses a path as if it were durable object identity, and authentication state is created separately from protection state. Re-resolving or re-statting a saved name answers "what is at this path now," not "which object supplied the credential still accepted by this daemon."Bounded fix guidance: create one immutable protection snapshot from the same opened object that supplies the startup token bytes, and keep that snapshot paired with the authenticator generation. Continue reserving the configured pathname so replacement cannot become an unprotected future authority, but compare tool/MCP handles against a durable identity for the original startup object rather than re-statting only its old name. That identity may be represented by a retained handle, a platform-specific stable file identifier carried to workers, or another abstraction that remains valid after rename/unlink. An equally valid alternative is supported rotation that atomically replaces both the authenticator and protection snapshot, retiring the old token before its identity is dropped. The required invariant is simply: if a token's bytes are still accepted, the object that supplied them is still denied.
Add a lifecycle regression using a regular file—not only a retargeted symlink—that captures inode A, retains alias H, atomically replaces pathname T with inode B, and proves the old token still authenticates while H remains denied by
read_file, grep, and MCP resources. Then simulate restart/reload and prove the new object becomes authoritative and the old identity may be released only after the old token is no longer accepted. Include ordinary-file and no-rotation controls.
Overall guidance: close the class, not another instance
The review history is long because the original one-variable leak expanded into a security contract spanning token selection, daemon startup, worker inheritance, direct tools, MCP entrypoints, formatters/diagnostics, patch planning, and three OS backends. Those additions are within the guarantees this PR now advertises, but they created several independently implemented interpretations of the same authority.
Most prior findings—including lexical versus resolved paths, policy/profile versus engine-less tools, case and alias handling, grep's walk-time check versus its later open, and the patch authorization parser versus its executor—share one pattern: layer A authorizes a convenient representation, then layer B reparses a string, re-resolves a pathname, reopens a file, or launches a process that consumes something different. Fixing only the reported call site closes one reproduction while leaving the next consumer free to diverge. The two remaining findings are the last observed versions of that pattern.
Please use these finite invariants as the completion boundary for this PR:
- Authenticator/protection lifetime: every object containing bytes still accepted by the running authenticator remains protected until those bytes are retired.
- Configured-name reservation: the exact configured spelling remains reserved independently of whichever object currently occupies or is reached through it.
- Use-bound authorization: a decision to read or mutate is made against the object actually consumed. No later raw-path reopen, reparse, or child process may silently select a different object.
- Child environment: every repository-influenced child process crosses one centralized sensitive-environment scrub; the inline token, file pointer, and internal resolved marker are absent unless a narrowly trusted daemon handoff explicitly requires them.
- One derived representation: startup and request parsing may happen once, but downstream consumers must receive the resulting typed/snapshotted identity or prepared operation rather than reconstructing it from strings and ambient filesystem state.
- Fail closed locally: if a particular helper invocation cannot establish these facts, refuse or omit that helper invocation. Do not disable unrelated ordinary-file functionality globally, and do not silently continue through a weaker pathname-only check.
A bounded implementation could pair TokenAuthenticator with a TokenProtectionSnapshot captured from the same startup open and expose shared operations such as "deny this opened object" and "scrub this child environment." Tool operations can then carry verified handles/bytes through their complete read or mutation lifecycle. This is guidance, not a mandated architecture: any implementation satisfying the six invariants is acceptable. It does not require redesigning general sandbox policy, fixing the pre-existing MCP containment race, adding global filesystem locks, or building a new credential store.
For verification, build one shared contract harness rather than another collection of isolated helper tests. Reuse it across the already-claimed entrypoints with a compact matrix:
- source lifecycle: startup, pathname replacement, symlink retarget, restart/reload;
- identity: exact path, configured symlink, hard-link alias, case variant where applicable;
- consumer: direct read/write/edit, grep, MCP resource read, formatter, diagnostics;
- outcome: no token bytes in file output, diagnostics/LSP, tracker/preview, or child environment; no token mutation;
- controls: ordinary files continue to work, inline-token precedence remains intact, and the new token becomes authoritative only with its matching protection snapshot.
The matrix need not multiply every combination on every platform. Put the source/identity invariant in a shared platform-neutral harness, then add backend-specific cases only where filesystem identity or process inheritance differs. Completing that contract should prevent another consumer-by-consumer review round without broadening the PR beyond the daemon-token boundary.
Validation
go test -race ./internal/toolswith focused changed-surface tests: passed.go test -race ./internal/sandboxwith focused protected-path and sandbox tests: passed.go test -race ./internal/mcpfor token-resource coverage: passed.go test -race ./internal/daemon/remotefor token/source coverage: passed.go vet ./...: passed.go build ./...: passed.git diff --check 27b319ca88a3180bed5183f0c599e9307f3ece12..71e951733287ffa6169949b81db615fc269e391e: passed.- Broad package race invocation was additionally attempted; tests requiring loopback listeners were blocked by the execution sandbox, and one CLI test could not access its default config path. The equivalent focused changed-surface tests above passed, and all current hosted Linux/macOS/Windows smoke, security, performance, and automated-review checks are green.
Disposition
Request changes. Resolve both security findings, update the branch to current main, and rerun the full checks.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed at 71e9517. The direction is right and the single-parse cleanup is the correct shape, but it lands the same class of problem this PR has hit before: the gate was narrowed and one of its two producers was not brought along.
Every apply_patch preflight through the agent loop denies
applyPatchPathBlock refuses outright when request.PatchPaths is nil (internal/sandbox/risk.go:313), and Engine.Evaluate runs it before every policy short-circuit, deliberately (internal/sandbox/engine.go:344-348).
There are two producers of a sandbox.Request. Registry.RunWithOptions sets the field (internal/tools/registry.go:252). sandboxRequest in the agent loop does not: internal/agent/loop.go:2184-2196 builds the Request with no PatchPaths at all, and grep -c PatchPaths internal/agent/loop.go is 0. That request is what the pre-execution Evaluate is given.
I ran it against the real engine at this head rather than reasoning about it:
AGENT-SHAPED notes.txt action=deny "patch paths were not supplied by the apply_patch executor"
AGENT-SHAPED .agents/notes.md action=deny "patch paths were not supplied by the apply_patch executor"
REGISTRY-SHAPED notes.txt action=allow "workspace write is allowed"
REGISTRY-SHAPED .agents/notes.md action=prompt "tool requires approval before execution"
The user-visible consequence is worse than a denial, because a deny is not a prompt. shouldRequestPermission returns false for a deny, so no approval is raised; execution reaches the registry, which does supply the paths and correctly asks for approval, but PermissionGranted is false because nobody was ever asked, and that becomes a hard error. So an apply_patch to a protected path fails with "approval required" and the user is never given the chance to approve, while write_file and edit_file to the same path still prompt and still work, because their preflight carries the path argument the gate reads.
The same nil field also reaches sandbox.Classify on the executed-risk path, so the risk recorded for a patch loses every path-derived category.
Why CI is green
The apply_patch cases in internal/sandbox/engine_test.go were edited to hand-write PatchPaths into the fixture, including TestEngineDoesNotAutoAllowProtectedMetadataWrites, which is the scenario that breaks. Five added lines do this. Every test therefore supplies what the second producer does not, and nothing anywhere asserts that a Request built outside the registry carries the field. That is the test moving with the code rather than holding it still.
What I would do
Populate PatchPaths in sandboxRequest from the same helper the registry uses, rather than adding a fallback parse in the engine, since removing the second parse is the point of the change. Then add a producer-side test that builds the request the way the agent loop does and asserts the decision is prompt rather than deny, so a future third producer fails loudly instead of silently denying.
Worth checking the merge base while you are in there: before this commit the same preflight returned prompt for .agents/notes.md, so this is a behaviour change on the user-facing path, not only an internal one.
Smaller
internal/tools/apply_patch_paths_test.go:91: the old parser cross-checked the diff --git header against the rename/unified headers and refused a patch whose headers disagreed. That check went with it, so a self-contradictory patch is now applied according to its rename headers. Low severity because the paths are still bounded to the workspace, but it was a real refusal and it is gone silently.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed at 71e9517. The direction is right and the single-parse cleanup is the correct shape, but it lands the same class of problem this PR has hit before: the gate was narrowed and one of its two producers was not brought along.
Every apply_patch preflight through the agent loop denies
applyPatchPathBlock refuses outright when request.PatchPaths is nil (internal/sandbox/risk.go:313), and Engine.Evaluate runs it before every policy short-circuit, deliberately (internal/sandbox/engine.go:344-348).
There are two producers of a sandbox.Request. Registry.RunWithOptions sets the field (internal/tools/registry.go:252). sandboxRequest in the agent loop does not: internal/agent/loop.go:2184-2196 builds the Request with no PatchPaths at all, and grep -c PatchPaths internal/agent/loop.go is 0. That request is what the pre-execution Evaluate is given.
I ran it against the real engine at this head rather than reasoning about it:
AGENT-SHAPED notes.txt action=deny "patch paths were not supplied by the apply_patch executor"
AGENT-SHAPED .agents/notes.md action=deny "patch paths were not supplied by the apply_patch executor"
REGISTRY-SHAPED notes.txt action=allow "workspace write is allowed"
REGISTRY-SHAPED .agents/notes.md action=prompt "tool requires approval before execution"
The user-visible consequence is worse than a denial, because a deny is not a prompt. shouldRequestPermission returns false for a deny, so no approval is raised; execution reaches the registry, which does supply the paths and correctly asks for approval, but PermissionGranted is false because nobody was ever asked, and that becomes a hard error. So an apply_patch to a protected path fails with "approval required" and the user is never given the chance to approve, while write_file and edit_file to the same path still prompt and still work, because their preflight carries the path argument the gate reads.
The same nil field also reaches sandbox.Classify on the executed-risk path, so the risk recorded for a patch loses every path-derived category.
Why CI is green
The apply_patch cases in internal/sandbox/engine_test.go were edited to hand-write PatchPaths into the fixture, including TestEngineDoesNotAutoAllowProtectedMetadataWrites, which is the scenario that breaks. Every test therefore supplies what the second producer does not, and nothing anywhere asserts that a Request built outside the registry carries the field. That is the test moving with the code rather than holding it still.
What I would do
Populate PatchPaths in sandboxRequest from the same helper the registry uses, rather than adding a fallback parse in the engine, since removing the second parse is the point of the change. Then add a producer-side test that builds the request the way the agent loop does and asserts the decision is prompt rather than deny, so a future third producer fails loudly instead of silently denying.
Worth noting the merge base: before this commit the same preflight returned prompt for .agents/notes.md, so this is a behaviour change on the user-facing path, not only an internal one.
Smaller
internal/tools/apply_patch_paths_test.go:91: the old parser cross-checked the diff --git header against the rename and unified headers and refused a patch whose headers disagreed. That check went with it, so a self-contradictory patch is now applied according to its rename headers. Low severity because the paths are still bounded to the workspace, but it was a real refusal and it is gone silently.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed at 71e9517. The direction is right and the single-parse cleanup is the correct shape, but it lands the same class of problem this PR has hit before: the gate was narrowed and one of its two producers was not brought along.
Every apply_patch preflight through the agent loop denies
applyPatchPathBlock refuses outright when request.PatchPaths is nil (internal/sandbox/risk.go:313), and Engine.Evaluate runs it before every policy short-circuit, deliberately (internal/sandbox/engine.go:344-348).
There are two producers of a sandbox.Request. Registry.RunWithOptions sets the field (internal/tools/registry.go:252). sandboxRequest in the agent loop does not: internal/agent/loop.go:2184-2196 builds the Request with no PatchPaths at all, and grep -c PatchPaths internal/agent/loop.go is 0. That request is what the pre-execution Evaluate is given.
I ran it against the real engine at this head rather than reasoning about it:
AGENT-SHAPED notes.txt action=deny "patch paths were not supplied by the apply_patch executor"
AGENT-SHAPED .agents/notes.md action=deny "patch paths were not supplied by the apply_patch executor"
REGISTRY-SHAPED notes.txt action=allow "workspace write is allowed"
REGISTRY-SHAPED .agents/notes.md action=prompt "tool requires approval before execution"
The user-visible consequence is worse than a denial, because a deny is not a prompt. shouldRequestPermission returns false for a deny, so no approval is raised; execution reaches the registry, which does supply the paths and correctly asks for approval, but PermissionGranted is false because nobody was ever asked, and that becomes a hard error. So an apply_patch to a protected path fails with "approval required" and the user is never given the chance to approve, while write_file and edit_file to the same path still prompt and still work, because their preflight carries the path argument the gate reads.
The same nil field also reaches sandbox.Classify on the executed-risk path, so the risk recorded for a patch loses every path-derived category.
Why CI is green
The apply_patch cases in internal/sandbox/engine_test.go were edited to hand-write PatchPaths into the fixture, including TestEngineDoesNotAutoAllowProtectedMetadataWrites, which is the scenario that breaks. Every test therefore supplies what the second producer does not, and nothing anywhere asserts that a Request built outside the registry carries the field. That is the test moving with the code rather than holding it still.
What I would do
Populate PatchPaths in sandboxRequest from the same helper the registry uses, rather than adding a fallback parse in the engine, since removing the second parse is the point of the change. Then add a producer-side test that builds the request the way the agent loop does and asserts the decision is prompt rather than deny, so a future third producer fails loudly instead of silently denying.
Worth noting the merge base: before this commit the same preflight returned prompt for .agents/notes.md, so this is a behaviour change on the user-facing path, not only an internal one.
Smaller
internal/tools/apply_patch_paths_test.go:91: the old parser cross-checked the diff --git header against the rename and unified headers and refused a patch whose headers disagreed. That check went with it, so a self-contradictory patch is now applied according to its rename headers. Low severity because the paths are still bounded to the workspace, but it was a real refusal and it is gone silently.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed at 71e9517. The direction is right and the single-parse cleanup is the correct shape, but it lands the same class of problem this PR has hit before: the gate was narrowed and one of its two producers was not brought along.
Every apply_patch preflight through the agent loop denies
applyPatchPathBlock refuses outright when request.PatchPaths is nil (internal/sandbox/risk.go:313), and Engine.Evaluate runs it before every policy short-circuit, deliberately (internal/sandbox/engine.go:344-348).
There are two producers of a sandbox.Request. Registry.RunWithOptions sets the field (internal/tools/registry.go:252). sandboxRequest in the agent loop does not: internal/agent/loop.go:2184-2196 builds the Request with no PatchPaths at all, and grep -c PatchPaths internal/agent/loop.go is 0. That request is what the pre-execution Evaluate is given.
I ran it against the real engine at this head rather than reasoning about it:
AGENT-SHAPED notes.txt action=deny "patch paths were not supplied by the apply_patch executor"
AGENT-SHAPED .agents/notes.md action=deny "patch paths were not supplied by the apply_patch executor"
REGISTRY-SHAPED notes.txt action=allow "workspace write is allowed"
REGISTRY-SHAPED .agents/notes.md action=prompt "tool requires approval before execution"
The user-visible consequence is worse than a denial, because a deny is not a prompt. shouldRequestPermission returns false for a deny, so no approval is raised; execution reaches the registry, which does supply the paths and correctly asks for approval, but PermissionGranted is false because nobody was ever asked, and that becomes a hard error. So an apply_patch to a protected path fails with "approval required" and the user is never given the chance to approve, while write_file and edit_file to the same path still prompt and still work, because their preflight carries the path argument the gate reads.
The same nil field also reaches sandbox.Classify on the executed-risk path, so the risk recorded for a patch loses every path-derived category.
Why CI is green
The apply_patch cases in internal/sandbox/engine_test.go were edited to hand-write PatchPaths into the fixture, including TestEngineDoesNotAutoAllowProtectedMetadataWrites, which is the scenario that breaks. Every test therefore supplies what the second producer does not, and nothing anywhere asserts that a Request built outside the registry carries the field. That is the test moving with the code rather than holding it still.
What I would do
Populate PatchPaths in sandboxRequest from the same helper the registry uses, rather than adding a fallback parse in the engine, since removing the second parse is the point of the change. Then add a producer-side test that builds the request the way the agent loop does and asserts the decision is prompt rather than deny, so a future third producer fails loudly instead of silently denying.
Worth noting the merge base: before this commit the same preflight returned prompt for .agents/notes.md, so this is a behaviour change on the user-facing path, not only an internal one.
Smaller
internal/tools/apply_patch_paths_test.go:91: the old parser cross-checked the diff --git header against the rename and unified headers and refused a patch whose headers disagreed. That check went with it, so a self-contradictory patch is now applied according to its rename headers. Low severity because the paths are still bounded to the workspace, but it was a real refusal and it is gone silently.
Amp-Thread-ID: https://ampcode.com/threads/T-01a07cd4-e4b2-73ad-b785-5a58906a3b0a Co-authored-by: Amp <amp@ampcode.com>
|
PierrunoYT pushed the review fixes in 682e23ff. The branch also includes the current upstream main fetched for this update. Review findings addressed
Regression evidence
ValidationPassed: make fmt-check; go vet ./...; go test ./...; affected-package race tests for tools, agent, sandbox, MCP, and remote authentication; release build and smoke; make lint-static (0 issues); make vulncheck (no vulnerabilities); git diff HEAD --check. Windows/macOS amd64 cross-compilation passed; native platform execution remains for CI. POSIX external-formatter fixtures skip on Windows, while pure-Go alias tests are not blanket-skipped there. Behavior notePrivate staging preserves the original formatter working directory, but formatters that discover settings solely from the input file’s ancestor directories may resolve configuration differently. This tradeoff is documented in the formatter implementation; formatting and diagnostics are not globally disabled when a token is configured. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewed at 682e23ff. My three findings are closed, and I drove each one. One thing new in this head needs to change before it goes in.
Closed. sandboxRequest now derives PatchPaths with the executor's own parser, and TestSandboxRequestApplyPatchPreflight is the producer-side test I asked for: an agent-shaped request through the real engine allows notes.txt, prompts for .agents/notes.md, and the prompt is offered for approval. Dropping the field population fails it. The header cross-check is back in parseUnifiedPatch, stricter than before, with the --no-prefix ambiguity resolved through the extended headers; I ran prefixed, no-prefix, spaced and plain forms and they all parse, and the three contradiction tests refuse what they should. Forcing the ---/+++ comparison to match fails the git-header and executor-path ones. The identity pinning for the token file reads correctly to me, including the Windows handle path, and the read-side consumers all go through the handle check now.
Format-on-write no longer formats to the project's style. The hardening added since my review stages the formatter's input in a temp directory outside the workspace and hands the formatter that path. Most formatters in formatterCommands resolve their configuration from the input file's location upward: prettier, ruff, rustfmt, clang-format, shfmt, ktlint, swiftformat. Staged under the temp directory, none of them can see .prettierrc, pyproject.toml, rustfmt.toml, .clang-format or .editorconfig, so they format with their built-in defaults. I drove it: a workspace with .clang-format setting IndentWidth: 8, then write_file of a .c file with ZERO_FORMAT_ON_WRITE=1:
through write_file at this head two-space indent clang-format's default LLVM style
clang-format on the real path eight-space indent what the project asks for
The feature's stated purpose is that output lands in project-canonical style and never fails a CI format check it cannot see. At this head it does the opposite for those projects: it rewrites bytes the model wrote into a style the project's CI will reject, and nothing tells anyone. The doc comment on maybeFormatWrittenFile already says settings from the file's ancestors may differ; that sentence is the finding.
The fix keeps the security property and drops the staging file entirely: feed the content on stdin and read stdout, passing the destination path only as the filename hint each formatter provides for exactly this purpose (prettier --stdin-filepath, ruff format --stdin-filename, clang-format --assume-filename, shfmt --filename, stylua --stdin-filepath, swiftformat --stdinpath, dart format --stdin-name, ktlint --stdin; gofmt, rustfmt, zig fmt --stdin, gleam format --stdin and terraform fmt - read stdin natively). The formatter then never opens the destination pathname at all, which is stronger than the temp copy, and configuration resolves from the real location. Publish the stdout through writeRootedFile as now, and pin it with a test like the one above: a project config the default style would not produce.
CI 6 of 6 at head. Locally the packages pass apart from two stale-base cases: the serve symlink test that #1042 fixed on main after this branch last merged it, and the eager schema budget test, whose accounting main changed in #1017 (the schemas here total the same 3653 tokens main reports). The branch conflicts with main as well.
Resolve the PR conflicts while keeping formatter input detached from the workspace and restoring project configuration lookup through filename hints. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Keep the worktree-pointer regression test aligned with the planner API so vet and test builds exercise the hardened filesystem plan. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Use ktlint formatting mode with the real stdin path and suppress stdout logging. Let Dart select stdin by omitting positional paths. Add deterministic production-argv contract tests. Both cases fail before the fix: Kotlin publishes empty stdout and Dart leaves source unformatted. Amp-Thread-ID: https://ampcode.com/threads/T-01a097c8-4873-7527-baa8-4cc463d3d65f Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
Compare the formatter filename hint against the physical destination rather than a symlinked temporary-directory spelling. Reproduced the macOS failure with a symlinked TMPDIR on Linux and verified the regression and focused race suite pass with that layout. Amp-Thread-ID: https://ampcode.com/threads/T-01a097c8-4873-7527-baa8-4cc463d3d65f Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
Wait for local Serve cleanup and final logging after shutdown before releasing caller-owned writers and runtime fixtures. Preserve the original TLS bind error and unregister signal delivery on return. Add a synchronized regression that fails without the join: serve-remote returned 1 while local Serve was still logging. Restore the token identity environment in the startup fixture. Validation: full tests, vet, formatting, release build/smoke, govulncheck, and focused CLI/security race suites passed. Advisory static lint retains four unrelated upstream findings. Amp-Thread-ID: https://ampcode.com/threads/T-01a097c8-4873-7527-baa8-4cc463d3d65f Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found four issues that need to be addressed before this is ready.
Reviewed head: 8c01bb9b35c252ad89add0ba74a9293836e34e31. The branch contains the captured current main (c1937dfac72e6ad0e5ade6e48e2d9c17d9c3e5d6), is conflict-free, and has passing hosted checks. The findings below concern this head.
Findings
-
[P1] Preserve the live startup credential in Linux shell admission
internal/sandbox/manager.go:362The startup-identity request is not fully addressed for shell execution. This preflight checks the link count and filesystem of the file currently at the configured/resolved pathname, but never checks that it is the object identified by
ZERO_INTERNAL_DAEMON_REMOTE_TOKEN_FILE_IDENTITY.Start the daemon with a single-link token on a dedicated filesystem, outside every shell-writable root and broader credential-directory deny. Then rename
tokentotoken.oldand create a new regulartoken. Both files have one link, so the Linux separate-filesystem exception permits the next shell. The read-all root exposestoken.old, while the exact/dev/nullbind masks only the replacementtoken.NewAuthenticatorFromEnvkeeps accepting the original bytes, as its replacement test explicitly verifies. The shell can therefore read a bearer that still controls the running daemon.This is a sequential rotation case, not a demand for cross-process locking. Please keep shell execution fail-closed when the startup object is no longer covered—for example, when the current pathname no longer matches its captured identity—while preserving the safe unrotated placement. Add the rotation lifecycle to the Linux admission coverage. A native fixture should use a separate mount outside
/dev; the synthetic/devmakes/dev/shmunsuitable for proving the read-back behavior. -
[P1] Keep pre-mutation checkpoints inside the token boundary
internal/tools/mutation_targets.go:25The new protection-aware target filter is only in the
apply_patchbranch;write_fileandedit_filestill return an in-workspace token here.agent/loop.goinvokesOnToolCallbeforeexecuteToolCall; the CLI recorder and TUI callback then call the session checkpoint code.SnapshotForCheckpointreads the target withos.ReadFileand stores its bytes in a new content-addressed blob before the new tool gate can refuse the mutation. Remote daemon workers use the stream-JSON exec path that enables this recording.With an active token, I verified that direct token reads and both subsequent mutations are denied, yet both checkpoint paths store the bearer. A later
read_filewith access to the session-data directory returns it from the blob, whose pathname and inode are outside the protected set. That read-back requires access to the session directory; it does not require permission to read the token itself. Forking a session also copies its checkpoint blobs.The checkpoint reader predates this PR, so this is an incomplete file-tool protection fix, not a newly introduced storage bug. Please prevent credential bytes from entering checkpoints, binding the decision to the object actually read so an alias swap cannot defeat an early pathname filter. Cover both writer callbacks, a denied mutation, and subsequent blob visibility, while retaining ordinary checkpoints and rewind behavior.
-
[P2] Retain the disambiguated rename/copy paths for hunk validation
internal/tools/unified_patch.go:287For an unquoted
--no-prefixrename such asold name.txttonew name.txt,DiffGitPathsreturns an ambiguous result. The extended headers correctly resolve it here by checking their exact concatenation, butdiffOldPathanddiffNewPathremain empty. When the patch also changes content, the following---/+++pair reachesstartFile, which compares those paths against the empty pair and rejects the valid patch.A real
git diff --cached --no-prefix -Mfixture with that rename and one changed line fails with---/+++ paths disagree with diff --git paths from line 1; the same fixture is accepted at the base. Header-only rename tests miss this case, and copies use the same branch. Since preparation feeds the agent gate and executor, this prevents the actual tool call. Please carry the validated pair into the following consistency check, preserving rejection of contradictory headers, and add rename/copy cases with hunks. -
[P2] Isolate the whitespace planner test from developer configuration
internal/sandbox/protected_credentials_test.go:333TestProtectedCredentialFilenameWhitespaceReachesOSSandboxcreates its token undert.TempDir, but builds the rest of the profile from the real process environment. Its call tomustBuildLinuxBwrapFilesystemPlanreachesensureLinuxDenyReadDirs, which creates the process's Zero config directory when absent. The test never redirects or cleans that directory. It runs on both Linux and macOS.Running only this test with a previously absent, isolated
XDG_CONFIG_HOMEleaves itszerodirectory behind after the test passes. Without that external isolation, the same effect targets the developer's configuration. This violates the explicit hermetic-test requirement inAGENTS.md. Please isolate all environment-derived roots the planner can materialize, or construct an isolated profile, while retaining the exact-whitespace assertion.
The environment-pointer fix in merged #818 does not supersede this PR’s broader file/object protections. Open #1011 overlaps the Linux masking implementation but addresses SSH/GPG discovery and does not close these daemon-token paths.
Focused race tests, formatting, vet, and builds passed, including Windows/macOS cross-builds. The broader local socket-dependent suite and native platform enforcement were not fully exercised; passing hosted checks do not cover the failure paths above.
Summary
Protect the remote daemon's file-backed bearer credential from sandboxed commands, in-process file tools, and pre-mutation session checkpoints.
Fixes #677.
The configured filename is treated as literal pathname data, including whitespace. Both its configured spelling and startup-resolved target remain protected, and the daemon passes the opened file's stable identity to workers. File tools check the actual opened object, so permission grants, hard-link aliases, and rotation do not bypass the credential boundary. Reads and mutations use rooted filesystem operations.
Shell behavior
Linux permits the existing safe placement on a filesystem separate from shell-writable roots, provided the credential has no additional hard links and its protected pathname still names the startup object. Rotation that renames the live token and replaces or removes its pathname now fails admission; restoring the original object restores admission. This sequential lifecycle check does not introduce cross-process locking around rotation and sandbox startup.
macOS and Windows require the inline
ZERO_DAEMON_REMOTE_TOKENfor sandboxed shell execution with remote authentication. In-process file tools retain their file-token protections. The existing CLI help documents the Linux and macOS restrictions.Latest review fixes
write_fileandedit_filecheckpoint targets. The checkpoint reader independently opens through the workspace root and checks the same handle it reads, preventing a rotated alias from being copied into an unprotected checkpoint blob.The branch contains current main (
c1937dfa).Regression evidence
New tests were run with the fixes and against the original implementation in an isolated Linux copy. Without the fixes:
---/+++ paths disagree with diff --git paths from line 1..config/zero; the fixed test left it absent.Coverage includes missing/restored token paths, rotated hard-link aliases, both mutation callbacks, reading persisted session files through
read_file, and ordinary checkpoint/rewind tests in the existing suite.Validation
make fmt-check(Linux) andgit diff HEAD --check: pass; new callback test files also checked directly with gofmt.go vet ./...: pass.go test ./...on Linux andgo test ./... -count=1on Windows: pass. Windows validation clears the inheritedZERO_PROVIDERoverride for the test process.-raceregression tests on Linux: pass, including both checkpoint callbacks. The broader sandbox and sessions race suites also pass.go run ./cmd/zero-release buildandsmokeon Linux and Windows: pass.make vulncheckon Linux and Windows: no vulnerabilities found.make lint-static: four existing Linux findings and seven existing Windows findings, all in unchanged files.Additional validation limits: the broader race suite exposes a race in
TestWebFetchUnicodeProxyHostnameIsStillTheProxy(web_fetch_proxy_test.go:126/133), reproduced on the unchanged PR head. Windows-racecould not build with the installed cgo toolchain. Linux rotation coverage exercises admission; native Bubblewrap execution was unavailable on this host. No native macOS enforcement test was run locally.