fix(sandbox): protect daemon token file

[PierrunoYT]

Summary

Protect the remote bridge's bearer token from the agent it authorizes.

ZERO_DAEMON_REMOTE_TOKEN_FILE names a file that grants control of the daemon. Issue #677 is narrow — the sandbox scrubbed the inline ZERO_DAEMON_REMOTE_TOKEN value but left the file pointer in the child environment, so a sandboxed command could read the pointer and then the file it names. Closing that leak turned out to require agreement across every layer that interprets the pathname, which is what this branch grew into and why it took several review rounds.

Fixes #677

The pathname contract

Each review round found a different layer disagreeing about what the token pathname is. The four rules every consumer must share are now written down in one place (internal/sandbox/pathlists.go), so a new consumer lands on an existing rule instead of inventing a fifth:

1. The env value is pathname data, not a word. Only an all-whitespace value counts as unset. Never trimmed, never shell-split, ~ never expanded — os.ReadFile, the daemon's own reader, treats it literally, so anything else protects a file the daemon does not read.
2. Both the selected spelling and its current resolved target are protected. serve-remote canonicalizes what it selects, but an inherited symlinked value must not leave the link replaceable.
3. Tool arguments are compared as exact bytes, because that is what the tool opens.
4. Protection is not re-includable. AllowRead, a permission grant, and a session profile all leave it in place, on every platform.

What each layer does and does not cover

Layer	Covers	Does not
Env scrub (scrubSensitiveEnv)	The pointer never reaches a child process, every platform	Nothing — a child that already knows the path is layer 2's problem
In-process tool gate (protectedCredentialPaths)	read_file, write_file, edit_file, apply_patch, grep, glob, list_directory; pathname and inode, so symlink and hard-link aliases are caught	Wrapped shell commands — a shell request carries a command line, not a path
OS profile (Seatbelt / bwrap deny-read)	Wrapped shell commands, by pathname	Hard-link aliases: a path-based rule cannot cover a second name for the same inode
Shell preflight (BuildCommandPlan)	Fails closed rather than hand a shell an un-maskable token — see below	—
Windows filesystem deny-read	—	Still the ACL-model limitation in #662; the in-process gate applies on Windows regardless

File-based token + sandboxed shell — current behavior, decision pending

With ZERO_DAEMON_REMOTE_TOKEN_FILE set under the default read-all policy, BuildCommandPlan refuses on Linux and macOS, directing operators to the inline ZERO_DAEMON_REMOTE_TOKEN or a token on a separate filesystem. In-process tools (read_file, grep, …) work normally; sandboxed bash does not on default Unix layouts.

This is deliberate — a pathname-based OS rule cannot stop a sandboxed shell from ln <token> alias && cat alias — but it is a product boundary, not just an implementation detail, and it was not in the original #677 scope. This is the open maintainer decision on the PR (review): keep the fail-closed posture and document it in serve-remote help, or narrow the preflight to nlink > 1 plus same-filesystem so an ordinary in-workspace token can run a shell, accepting documented hard-link TOCTOU the way user DenyRead already does. Nothing below depends on which way it goes.

Capture atomicity — explicitly not in this PR

SecureProviderProfile-style capture is unrelated here, but the analogous caveat is worth stating: this branch does not introduce cross-process locking over the token lifecycle. The mandatory-symlink path fails closed rather than racing a rotation.

Reconciled with main

Merged main (d065467c) after #681 (credential deny-read refactor), #682 (dynamic env scrub), and #774 (daemon child cleanup) landed on the same files.

Only one content conflict, in internal/cli/daemon_test.go: both sides appended test functions and imports, resolved as a union — this branch's TestDaemonServeRemoteCanonicalizesTokenFileBeforeStartingWorkers and writeDaemonTestCertificate alongside main's daemon lifecycle / terminate-and-reap coverage. No test dropped or rewritten; the two sets share no helper names.

internal/cli/daemon.go merged cleanly, keeping both this branch's CanonicalizeTokenFileEnv() and main's terminateAndReapDaemonProcess / background.TerminateCommand. The sandbox files merged without conflict: this branch was already written against #681's credentialPathOptions shape, so no flat credentialDenyReadPathsIn signature is reintroduced.

The whitespace bypass (P1)

requestPaths ran every path-carrying tool argument through argString, which TrimSpaces, while the tools resolve the same arguments with aliasedStringArg, which does not. A credential whose filename carries meaningful whitespace was protected under its real spelling while the gate inspected a different one.

Reproduced end to end before fixing — with the token named " bridge-token", read_file {"path": " bridge-token"} cleared a gate that checked "bridge-token" and returned the bearer:

read_file served the protected token under its exact spelling:
output="File:  bridge-token (1 lines)\n\n1 | bridge-secret"

The gate now reads the exact bytes the tool will open. The trimmed spelling is still emitted when it differs, so the gate never inspects less than it did before.

One subtlety worth recording: the whitespace must sit at the boundary of the argument string for TrimSpace to reach it, so the exploit needs the relative spelling. In an absolute path the space is mid-string (after the separator) and the old gate incidentally behaved — which is why the existing absolute-path coverage never caught this.

Engine-less list_directory (P3)

list_directory disclosed the token filename when reached without a sandbox engine. Registry.Run funnels into RunWithOptions with empty options, so that is the MCP / legacy production path, not a test shape — patching Run() alone would have been dead code.

The protected-credential set is derived from this process's environment rather than from a policy, so there is no engine to consult for it and no reason for that path to be less protected. sandboxReadExcluderWithin applies it with or without an engine; policy DenyRead still requires one.

Tests

• TestDaemonTokenProtectionMatrix — read_file, write_file, list_directory, grep, and apply_patch crossed with exact, trailing-space, relative, dot-segment, and parent-traversal spellings, every cell through registry.RunWithOptions with the engine, so a future gap fails as a matrix cell rather than arriving as a new report.
• TestEngineDeniesReadFileWithExactSpacedTokenPath — the P1 regression, verified to fail against the unfixed gate.
• TestListDirectoryWithoutEngineStillHidesProtectedToken — the engine-less path, verified to fail before the fix.
• Fixtures skip rather than fail where the host filesystem will not store the name, checked against the real directory entry: Windows silently strips trailing spaces from both the create and the lookup, so an Lstat guard would have asserted against a file that never existed.

Validation

• go build ./...
• go vet ./...
• gofmt -l . (clean)
• go test ./internal/... (green)

Still open

• The shell-vs-file-token decision above.
• [P3] completeCreatedPatchTargets still uses the local header parser. PatchHeaderPaths returns a flat path list, so it cannot directly replace a function that needs /dev/null creation pairs; unifying them means adding a pairs-returning API with PatchHeaderPaths as a flattener over it. Security gate is already on the shared parser — this is integrity-adjacent bookkeeping.
• [P3] Mandatory token paths are still subject to the pathsOutsideRoots optimization against a user DenyRead parent, which can drop the OS write-deny while the in-process gate still blocks it.

Summary by CodeRabbit

• Security
  • Strengthened protections for the daemon remote-token file: it’s now treated as a protected credential target with fail-closed behavior, broader sandbox/seatbelt deny rules, and stronger path handling. Inline tokens take precedence over token-file values.
  • Improved enforcement prevents token-file replacement via policy, and tools won’t surface the token via filesystem aliases.
• Bug Fixes
  • Refined read/write exclusion behavior so allow/deny changes can’t accidentally re-enable token access; improved credential-path deny coverage for environment/config overrides.
  • Updated directory listing to respect per-run read exclusions.
• Tests
  • Added/expanded coverage for token-file canonicalization, nested allow/deny listing behavior, alias handling, and permission-profile denial cases.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The daemon token file is canonicalized before remote serving, added to sandbox credential protections, excluded from search and file tools, and removed from spawned command environments. Tests cover path resolution, policy enforcement, seatbelt rules, deny-read profiles, and edge cases.

Changes

Daemon token protection

Layer / File(s)	Summary
Token file canonicalization internal/daemon/remote/auth.go, internal/daemon/remote/auth_test.go, internal/cli/daemon.go, internal/cli/daemon_test.go	Relative and symlinked token-file paths are resolved before token loading; unresolved paths fail closed, while inline tokens take precedence.
Sandbox credential protection internal/sandbox/pathlists.go, internal/sandbox/engine.go, internal/sandbox/profile.go, internal/sandbox/protected_credentials_test.go, internal/sandbox/manager_test.go	The configured daemon token path is denied from reads and writes despite allow policies, including alias paths and disabled-policy modes; credential overrides and permission profiles are covered.
Sandbox runtime and tool hardening internal/sandbox/runner.go, internal/sandbox/runner_test.go, internal/tools/*	The token-file environment variable is scrubbed, seatbelt rules add targeted write-unlink protection, and sandbox-aware listing/search/file tools honor read exclusions while preserving nested allowed reads.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

• Sandbox can read SSH private keys, the GPG keyring, and git credential stores #815 — Both changes expand credentialDenyReadPathsForEnvironment and its credential deny-path candidates.

Possibly related PRs

• Gitlawb/zero#681 — Modifies the shared credential deny-read path computation used here.

Suggested reviewers: jatmn, gnanam1990, anandh8x

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR adds broad sandbox and tool hardening beyond #677, including alias protection, disabled-policy behavior, and tool-specific exclusions.	If these protections are intentional, track them in a broader issue or separate PR; otherwise trim this patch to env scrubbing, token-path deny-read, and the related tests.
Docstring Coverage	⚠️ Warning	Docstring coverage is 41.18% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and accurately describes the main change: protecting the daemon token file.
Linked Issues check	✅ Passed	The PR removes ZERO_DAEMON_REMOTE_TOKEN_FILE from sandboxed envs and denies the token path, with tests covering both.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

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_FILE from sandbox command environments (in addition to the inline token env var).
• Extend credentialDenyReadPaths to include the path named by ZERO_DAEMON_REMOTE_TOKEN_FILE (alongside GOOGLE_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]

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 enters DenyRead, but the Seatbelt backend translates that only into file-read* and unlink denials. Its broad file-write* allowance still covers every workspace root and the default temporary roots. Therefore, when ZERO_DAEMON_REMOTE_TOKEN_FILE names a file under /tmp or 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 credential DenyRead files in the Seatbelt profile (and a macOS regression case for a token under a writable temporary root).

[anandh8x]

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]

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]

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
normalizeProfilePaths resolves ZERO_DAEMON_REMOTE_TOKEN_FILE through symlinks before it is added to DenyRead. 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, TokenFromEnv reads 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.

[Vasanthdev2004]

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.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8533492 and 5619a29.

📒 Files selected for processing (4)

• internal/sandbox/manager_test.go
• internal/sandbox/profile.go
• internal/sandbox/runner.go
• internal/sandbox/runner_test.go

[jatmn]

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 existing ZERO_DAEMON_REMOTE_TOKEN_FILE symlink, 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
TokenFromEnv accepts relative token paths, and serve-remote reads one before it starts workers. The daemon then preserves ZERO_DAEMON_REMOTE_TOKEN_FILE for workers whose cmd.Dir is the per-session spec.Cwd; normalizeProfilePathLexical consequently turns token into a path beneath that session instead of the daemon startup directory that contains the actual bearer-token file. The real file is left outside DenyRead under 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.

[Vasanthdev2004]

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.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5619a29 and 5cd8009.

📒 Files selected for processing (7)

• internal/cli/daemon.go
• internal/cli/daemon_test.go
• internal/sandbox/linux_helper.go
• internal/sandbox/linux_helper_test.go
• internal/sandbox/manager_test.go
• internal/sandbox/profile.go
• internal/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]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Preserve the resolved target for user-configured DenyRead symlinks
  internal/sandbox/profile.go:104
normalizeProfilePath is now lexical-only, while this initializer still uses normalizeProfilePaths for policy entries. On Linux, appendUnreadableLinuxPathArgs then skips that symlink mount destination and no resolved target is present (unlike the credential-path branch). Thus a policy such as denyRead: [link], where link points 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 for workspaceRoot, AllowWrite, and DenyWrite, 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. If ZERO_DAEMON_REMOTE_TOKEN_FILE is 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
  The Lstat check catches only a final-component symlink. For a supported token path such as /tmp/linkdir/token, where linkdir is 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.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5cd8009 and a9da4ff.

📒 Files selected for processing (7)

• internal/cli/daemon.go
• internal/cli/daemon_test.go
• internal/sandbox/linux_helper.go
• internal/sandbox/linux_helper_test.go
• internal/sandbox/manager_test.go
• internal/sandbox/profile.go
• internal/sandbox/runner.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/cli/daemon.go
• internal/sandbox/runner.go

[jatmn]

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 to PermissionProfile.FileSystem.DenyRead, which protects wrapped shell commands. Built-in tools do not consume that profile: read_file reads scoped files directly, and grep/glob exclusions are built from Policy.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 use read_file to 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
TokenFromEnv intentionally returns a nonempty ZERO_DAEMON_REMOTE_TOKEN before consulting ZERO_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 makes daemon serve-remote exit 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 as GOOGLE_APPLICATION_CREDENTIALS=/var/run/... (where /var/run is 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.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 2a0e63e and 4db4c6f.

📒 Files selected for processing (6)

• internal/cli/daemon.go
• internal/cli/daemon_test.go
• internal/sandbox/engine.go
• internal/sandbox/linux_helper.go
• internal/sandbox/linux_helper_test.go
• internal/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

[PierrunoYT]

Addressed the latest macOS hard-link finding in 41433640.

Changes:

• fail closed when the selected daemon-token file shares a filesystem with any Seatbelt shell-writable root, including when the token is outside that root
• retain the existing direct containment check for missing/in-root paths
• give operators actionable alternatives: inline token, separate filesystem, or no shell write access
• add a Darwin regression that proves the outside token is hard-linkable into the workspace and verifies command-plan construction rejects it
• update the Seatbelt comment to distinguish user DenyRead pathname semantics from the fail-closed automatic token guard

Verification:

• go test ./internal/sandbox -run "TestSandboxManagerRejectsMacOSTokenInsideWritableWorkspace|TestProtectedCredentialLinkableIntoWritableMacOSRoot" -count=1
• Darwin cross-compile: GOOS=darwin GOARCH=amd64 go test -c ./internal/sandbox
• go vet ./...
• go test ./... (84 packages passed, 5 had no tests)
• go run ./cmd/zero-release build
• go run ./cmd/zero-release smoke
• go run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./... (no vulnerabilities)
• git diff HEAD --check

Environment-only validation notes:

• make fmt-check could not run because make is not installed on this Windows workstation. The equivalent tracked-file gofmt -l check reports existing files outside this commit; all changed files were formatted.
• advisory static lint still reports the unrelated existing ST1005 finding at internal/peermsg/private_dir_windows.go:107.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Close the Linux token-rotation check/use gap
  internal/sandbox/linux_helper.go:268
  The mandatory-path validation (validateLinuxMandatoryDenyReadPaths) establishes that the selected pathname is a regular file, but the plan is assembled later by buildLinuxBwrapFilesystemPlan. If an external token rotation replaces that file with a symlink in between, appendMandatoryUnreadableLinuxPathArgs observes the new symlink and deliberately emits no Bubblewrap mask for its lexical pathname. MandatoryDenyReadPaths still contains only the target resolved when the profile was created, not the symlink's new target. The resulting shell therefore sees the rotated bearer through ZERO_DAEMON_REMOTE_TOKEN_FILE's configured path, despite the PR's mandatory-token and fail-closed claims.

  Address the root cause by making the path identity used for validation and the identity used to construct the Bubblewrap plan one atomic/consistent operation; a preliminary Lstat cannot establish that property. In practice, retain an immutable selected target established by the daemon and mask that target, or re-resolve the mandatory pathname immediately while assembling the plan and reject the launch whenever it is no longer the same non-link object/target that the profile protects. Do not silently skip a newly symlinked mandatory path. Add a regression that changes a regular mandatory token file into a symlink after validation but before mount construction, and assert that plan construction fails closed (or that every reachable replacement target is masked).

[PierrunoYT]

Pushed cb263546 addressing today's finding.

Fixed

• [P1] Close the Linux token-rotation check/use gap (linux_helper.go)
  validateLinuxMandatoryDenyReadPaths and buildLinuxBwrapFilesystemPlan each Lstat'd a mandatory path separately, so a rotation that replaced a validated regular file with a symlink to an unmasked target in the gap between those two calls slipped through unmasked entirely.

  Closed by making the symlink-target check part of the exact Lstat that decides whether to emit a mask, instead of trusting an earlier syscall's result:

  • mandatoryDenyReadSymlinkTarget centralizes "does this symlink resolve to another protected entry," called from both the early best-effort validation pass and the late, authoritative one.
  • appendUnreadableLinuxPathArgsForPath now takes the mandatory-path set and re-runs that check against its own fresh Lstat immediately before deciding to skip a symlink — no window between check and decision.
  • The early validateLinuxMandatoryDenyReadPaths pass is now explicitly documented as advisory/fail-fast only; the late check is what actually enforces the guarantee.

  Added TestLinuxBwrapMandatoryPathRotationToUnprotectedSymlinkFailsClosed, which rotates a validated regular file into a symlink pointing outside the mandatory set between validation and plan construction and asserts the plan now fails closed, plus TestLinuxBwrapMandatoryPathSymlinkToAnotherMandatoryPathIsMasked as a control confirming the accepted shape (symlink to another mandatory path) still masks correctly through its resolved target.

Re-verified as already closed

Went through the full outstanding list before touching anything, since several rounds have landed since the last time I looked. All confirmed fixed by the intervening commits, no regressions:

• 08-10 P1 — test assertion checked DenyRead instead of DenyReadIfExists/MandatoryDenyReadPaths → now asserts the right fields, Windows branch expresses the deferred-ACL behavior explicitly.
• 08-10 P1 — absent mandatory token silently unmasked → buildLinuxBwrapFilesystemPlan now fails closed with "bubblewrap cannot enforce missing mandatory credential path" rather than materializing or skipping.
• 08-10 P1 — binary-patch header trailing-space trimming → PatchHeaderPaths now preserves a trailing-space filename through a GIT binary patch header (verified directly against the bridge-token repro in the finding).
• 08-11 P1 — macOS hard-link aliasing → pathsShareFilesystem (device-equality check) now catches a token linkable into a writable root even when it isn't lexically inside one; not just the review-comment's own fix, verified the actual check.

Validation

• go build ./..., go vet ./..., gofmt -l on changed files — clean
• go test ./... (whole module) — pass
• git diff HEAD --check — clean

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Fix the failing cross-platform smoke test
  internal/sandbox/linux_helper_test.go:401
  The accepted-symlink fixture puts the raw t.TempDir() spelling into MandatoryDenyReadPaths, then mandatoryDenyReadSymlinkTarget looks up the result of filepath.EvalSymlinks. Those are not the same spelling on macOS (/var is resolved as /private/var) or on Windows (the runner's short temp-directory name resolves to its long name), so the membership check rejects a target that the fixture intentionally made mandatory. This is the exact failure in both required smoke jobs, and it leaves the PR unmergeable even though Ubuntu happens to preserve the same path spelling. Establish one canonical identity for every mandatory path when constructing the profile, and use that same identity for both the set lookup and the mount target; do not make path-string equality depend on a platform's temporary-directory presentation. Add cross-platform coverage for a lexical path whose EvalSymlinks result differs, rather than only exercising the current runner's spelling.

• [P1] Close the remaining symlink check-to-use gap in the Linux mount plan
  internal/sandbox/linux_helper.go:508
  The head change closes the earlier regular-file-to-symlink gap only at the point it notices a symlink: appendUnreadableLinuxPathArgsForPath first Lstats the mandatory pathname, then mandatoryDenyReadSymlinkTarget resolves it with a separate EvalSymlinks call and returns without placing a mask on the lexical path. A concurrent token rotator can repoint that symlink after the resolution check but before Bubblewrap creates the namespace. The pathname is then unmasked in the read-all root and resolves to a non-mandatory live bearer target, allowing the sandboxed shell to read it. The current regression changes a regular file before plan construction; it does not exercise the symlink-repoint interval left by these two syscalls.

  Address the root cause by making the path identity used for validation the same identity that is bound or masked at launch. A preliminary pathname check cannot provide that guarantee for a mutable symlink. Prefer a descriptor/handle-relative approach that pins the selected object through namespace construction; if Bubblewrap cannot express that safely, reject a selected symlink token path (or fail the launch whenever it changes). Add a synchronized regression that repoints a previously accepted mandatory symlink after validation/resolution and proves the command either fails closed or cannot read either reachable target.

[PierrunoYT]

Addressed jatmn's latest review in 980f2f42 and pushed it to this PR branch.

• Mandatory Linux credential symlinks now fail closed even when their current target also appears in the mandatory set; there is no longer a skipped lexical path that can be repointed before Bubblewrap starts.
• Mandatory/baseline membership uses canonical physical identity, while enforcement still emits the regular lexical pathname so Bubblewrap rejects a post-plan symlink swap at namespace construction.
• Added regressions for regular-file-to-symlink rotation, an initially symlinked mandatory path whose target is also mandatory, and canonical-equivalent membership.

Validation:

• changed files are gofmt-clean
• go vet ./...
• go test ./... (84 packages passed)
• go run ./cmd/zero-release build
• go run ./cmd/zero-release smoke
• Linux cross-compile: GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go test -c ./internal/sandbox
• govulncheck reports no vulnerabilities
• git diff HEAD --check

Environment notes: make is unavailable on this Windows workstation. Advisory static lint still reports only the unrelated pre-existing ST1005 finding at internal/peermsg/private_dir_windows.go:107. Race validation is unavailable because this Go environment has CGO disabled (-race requires cgo).

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Make the mandatory-symlink regression agree with the new fail-closed contract
  internal/sandbox/protected_credentials_test.go:302
  Head 980f2f4 deliberately changes the mandatory-token contract: validateLinuxMandatoryDenyReadPaths now rejects a configured token pathname when Lstat reports a symlink, because Bubblewrap cannot safely bind-mask a mutable symlink destination. That is the correct fail-closed direction for the prior check/use issue. However, TestUnreadableLinuxPathSkipsProtectedSymlinkDestination still configures ZERO_DAEMON_REMOTE_TOKEN_FILE to a symlink, calls mustBuildLinuxBwrapFilesystemPlan, and expects a successful plan that masks the resolved target. The current package test deterministically fails at that call with bubblewrap cannot enforce mandatory credential symlink; this is also consistent with the failed required smoke test steps.

  Please make the test suite describe the same public contract as the planner. Replace this test's success-path assertion with an assertion that a selected symlink token fails before command launch, and retain any generic "do not bind over an optional symlink" coverage in a non-mandatory credential case. Add a command-plan-level regression that sets the token-file environment variable to a symlink and verifies that the shell is refused, so a future change cannot restore the unsafe accepted-symlink behavior while leaving only a helper-level test green.

• [P1] Close the Linux hard-link alias path for shell commands
  internal/sandbox/linux_helper.go:374
protectedCredentialPaths and the in-process file-tool gate close aliases with os.SameFile, but the Linux Bubblewrap plan only emits a --ro-bind /dev/null over each lexical entry in MandatoryDenyReadPaths. A hard link is another directory entry for the same inode, so an alias that already exists in a workspace or another shell-readable root is not affected by the bind over the configured pathname. A remote command can therefore run cat token-alias, receive the bearer token, and use it to authenticate to the bridge independently. The macOS path has a preflight for a token linkable into a writable root; Linux has no equivalent check, and the comment in pathlists.go explicitly acknowledges that pathname-only OS rules do not close this class.

  Address the identity boundary rather than adding more pathname masks. Before building a Linux shell plan, reject a file-backed token whenever it can be hard-linked into a root that the shell can read or write, unless the token is held on an isolated filesystem or the design can bind the selected object by a stable descriptor/handle through namespace construction. Do not rely on the token-file variable having been scrubbed: a remote caller may know or guess the token path, and a pre-existing alias needs neither variable nor a new link operation. Add an end-to-end Bubblewrap regression that creates a token and hard-link alias before plan construction, runs a shell under the plan, and proves the alias cannot reveal the token (or that plan construction fails closed).

[jatmn]

I found issues that need to be addressed before this is ready.

This is meant to be a single consolidated pass, not another drip round. The recurring review churn on this PR is not because the original #677 fix was wrong — it is because each follow-up commit hardened one layer of a multi-layer problem without locking a single pathname contract and one end-to-end test matrix across every entrypoint. The guidance below is ordered so you can close the class of issues, not just the latest instance.

---

Why this PR keeps getting more findings

1. The fix grew from one leak into a security surface, without a written boundary contract.

Issue #677 is narrow: scrub ZERO_DAEMON_REMOTE_TOKEN_FILE from child env and deny-read the file it points at. The branch now also covers: mandatory Linux bwrap paths, macOS hard-link preflight, inode-level in-process closure, patch header parsing, list_directory exclusions, ModeDisabled behavior, whitespace filename semantics, canonicalization at serve-remote, and shell vs file-tool divergence. Each layer is correct to question, but reviewers only discover the next gap when a different layer is exercised (shell vs read_file, Linux bwrap vs Seatbelt profile test, Evaluate vs validatePathWithPolicy).

Root cause: No upfront doc (even a short comment block or test table) stating: which layers protect the token, which layers deliberately do not, and what operators must configure (inline vs file, separate FS, etc.). Every round fills that doc one bullet at a time.

2. Pathname authority is duplicated, not shared.

daemonTokenDenyPaths is described as the shared authority, but paths flow through several normalizers:

Consumer	Normalization
remote.TokenFilePathFromEnv / TokenFromEnv	verbatim bytes; trim only all-whitespace
daemonTokenDenyPaths / protectedCredentialPaths	filepath.Abs + optional EvalSymlinks
requestPaths → Engine.Evaluate	argString → strings.TrimSpace on path args
read_file / write_file tools	aliasedStringArg → no trim
PatchHeaderPaths	exact header bytes
completeCreatedPatchTargets	legacy local parser

When you fix whitespace or symlink behavior in one function, another path still disagrees. That is exactly how the firstExactStringArg patch fix landed while path args still trim.

3. Tests prove subsystems, not the attack path.

Many tests call validatePathWithPolicy, PermissionProfileFromPolicy, or buildLinuxBwrapFilesystemPlan directly with hand-set paths. They do not run:

registry.RunWithOptions(..., read_file, {path: exact token spelling}, {Sandbox: engine})

So the gate that actually runs in production (Evaluate ← requestPaths) can drift from what profile tests prove.

4. Stacked sandbox PRs on the same files.

#681 (merged) and #682 (merged) rewrote credentialDenyReadPaths, scrubSensitiveEnv, and large parts of manager_test.go. This branch diverged at dc15e822 and still touches the same surfaces. GitHub shows mergeable: false / dirty — rebasing is not cosmetic; it is where hidden conflicts and double-fixes come from.

5. Fail-closed shell preflight vs “protect file token” product story.

New protectedCredentialLinkableIntoLinuxShellRoot / macOS equivalent intentionally refuse BuildCommandPlan when the token shares a filesystem with shell-visible roots (including the default read-all / layout). Error text tells operators to use inline ZERO_DAEMON_REMOTE_TOKEN. That may be the right tradeoff, but it was not in the original #677 summary — so each reviewer pass re-asks whether file-pointer auth + sandboxed bash is supposed to work.

---

Holistic guidance (do this before the next review request)

Step A — Rebase and reconcile (blocker)

1. Rebase onto current main (0eab63c9 at review time).
2. Resolve internal/cli/daemon.go: keep both CanonicalizeTokenFileEnv() (your bridge pinning) and main's terminateAndReapDaemonProcess / background.TerminateCommand (#774).
3. Reconcile profile.go, runner.go, manager_test.go with #681's credentialPathOptions shape — do not reintroduce the flat credentialDenyReadPathsIn signature gnanam1990 flagged.
4. Re-run the full sandbox test packages after rebase; fix any test drift from merged #681/#682 helpers.

Step B — One pathname contract (stops the whitespace / symlink drip)

Pick one rule and enforce it everywhere the token path is interpreted:

• Env pointer bytes are pathname data (only all-whitespace = unset).
• Daemon reader, protectedCredentialPaths, OS profile, and tool args must compare the same spelling after the same transforms (Abs, EvalSymlinks, canonicalize-at-serve-remote).

Concrete actions:

1. Add firstExactStringArg (or a shared exactPathArgs helper) for all path-carrying keys in requestPaths, matching the alias list already documented in the comment above requestPaths in risk.go. Do not use argString for paths that tools open via aliasedStringArg.
2. Add one regression test: token file named with trailing space (or leading space in a relative segment), read_file with that exact path arg through registry.RunWithOptions + Sandbox engine — must deny before I/O.
3. Document in pathlists.go (or auth.go) that CanonicalizeTokenFileEnv is required for any long-lived process that sets ZERO_DAEMON_REMOTE_TOKEN_FILE, not only serve-remote, or pin processCredentialBaseDir for the token pointer the same way OAuth paths are pinned.

Step C — One end-to-end test matrix (stops layer-by-layer drip)

Add a table-driven test file (or extend daemon_token_exclusion_test.go) that runs production paths, not just profile builders:

Tool / path	Spaced filename	Symlink alias	Hard-link alias	AllowRead covers workspace
read_file	via RunWithOptions + engine
write_file
apply_patch (header)
grep / glob / list_directory
bash / exec_command	expect deny or documented fail-closed error

Mark each row with expected outcome per layer (in-process deny, OS profile deny, plan-build refuse). When the table is green, you have a defensible “done” line.

Step D — Shell vs file-token product decision (write it down)

Choose one and document in PR body + daemon serve-remote help:

• Option 1 (current code bias): File-pointer auth is supported for in-process tools only; sandboxed shell on Linux/macOS requires inline ZERO_DAEMON_REMOTE_TOKEN or token on a separate mount. serve-remote should fail fast or warn when file pointer + enforcing sandbox + shell are combined.
• Option 2: Narrow protectedCredentialLinkableIntoLinuxShellRoot so a normal in-workspace token (no pre-existing hard link, nlink == 1) can run shell with pathname /dev/null bind, accepting documented hard-link TOCTOU like user DenyRead.

Do not leave this implicit in error strings only — that is what drives the next review round.

Step E — Finish parser migration (small, closes rewind class)

Route completeCreatedPatchTargets through sandbox.PatchHeaderPaths (or delete the duplicate patchFileHeaderPath in apply_patch.go). One parser, one test suite (apply_patch_paths_test.go).

Step F — Optional hardening (P3, only if you want zero drip from MCP)

Wire list_directory.Run() through the same sandboxReadExcluder when a global/engine is available, or document that MCP is out of scope for #677.

---

Findings

[P2] Rebase onto current main before merge

Where: branch agent/protect-daemon-token-file vs main (mergeable: false, mergeable_state: dirty)

What: Merge-base with head is dc15e822 (27 files in the real PR diff). main has moved forward with #774 (daemon child cleanup), #681 (credential deny-read refactor), and #682 (dynamic env scrub). Merge-tree shows conflicts in internal/cli/daemon.go and overlapping edits in sandbox profile/runner tests.

Why it matters: Merging without rebase risks dropping main fixes or duplicating logic. Review comments about #681 signature conflicts remain valid until the rebase lands.

Author guidance: Rebase, run go test ./internal/sandbox/... ./internal/daemon/remote/... ./internal/cli/... ./internal/tools/..., push, and note conflict resolutions in the PR description so reviewers do not re-audit phantom diffs against stale main.

---

[P1] requestPaths trims path args — bypasses whitespace filename protection

Where: internal/sandbox/risk.go — argString (lines ~211–227), requestPaths (lines ~248–258)

Failure path:

1. Operator sets ZERO_DAEMON_REMOTE_TOKEN_FILE to a pathname with meaningful whitespace (e.g. bridge-token — valid on Unix; your tests already use this spelling).
2. protectedCredentialPaths() correctly protects /…/bridge-token (with space).
3. Remote agent calls read_file with {"path": "bridge-token "} (exact spelling).
4. Engine.Evaluate calls requestPaths → argString → strings.TrimSpace → gate checks bridge-token (no space).
5. Gate allows; read_file uses aliasedStringArg → opens bridge-token → bearer secret returned.

Root cause: This PR added firstExactStringArg for patch/diff and preserved whitespace in TokenFilePathFromEnv, daemonTokenDenyPaths, and PatchHeaderPaths, but left path-carrying tool args on the old argString path. The comment above requestPaths already says to stay aligned with aliasedStringArg — trimming violates that.

Author guidance:

• Replace argString in requestPaths with firstExactStringArg for path keys (path, file, file_path, filepath, filename, cwd, workdir, dir, directory), or introduce firstExactPathArg that mirrors aliasedStringArg's key list and exact-byte rule.
• Keep argString only where trimming is intentional (non-path metadata).
• Add test: TestEngineDeniesReadFileWithExactSpacedTokenPath via registry.RunWithOptions + daemonTokenFixture engine — must deny and must not contain secret in output.
• Grep for other requestPaths callers / duplicate path extraction in risk.go after the change.

---

Needs maintainer decision

File-based token + sandboxed shell on Linux/macOS

Where: internal/sandbox/manager.go — protectedCredentialLinkableIntoLinuxShellRoot, protectedCredentialLinkableIntoWritableMacOSRoot, BuildExecutionRequest (~232–245)

Behavior (new in this PR, not at merge-base dc15e822):

• With ZERO_DAEMON_REMOTE_TOKEN_FILE set and default policy, read roots include /.
• protectedCredentialLinkableIntoLinuxShellRoot returns true when pathWithinRoot("/", credential) — true for any absolute token path on Linux.
• BuildCommandPlan fails with "bubblewrap cannot protect the remote token file … hard-link aliases".
• macOS fails similarly when token shares a filesystem with workspace write roots (TestSandboxManagerRejectsMacOSTokenHardLinkableIntoWritableWorkspace).

Not drift: Code and tests intentionally fail-closed; error text directs operators to inline token or separate filesystem.

Tension: PR title/body emphasize protecting the file token; remote sessions commonly place the token in the session workspace and rely on sandboxed bash. In-process tools (grep, read_file) work; shell does not on default Unix layouts.

Author guidance:

• If Option 1 (inline token for shell): state explicitly in PR description and serve-remote help; consider detecting file-pointer + enforcing sandbox at daemon start and printing a one-line operational warning.
• If Option 2 (shell should work with file token in workspace): narrow preflight to pathHardLinkCount > 1 and same-filesystem checks without treating every path under / as linkable; add TestBuildCommandPlanSucceedsWithFileTokenInWorkspace on Linux CI.

---

Lower priority (real but narrow — fix only if you want zero follow-up)

[P3] User DenyRead parent can drop OS write-deny for the token file

Where: internal/sandbox/profile.go finalizeCredentialDenyPaths (~791–792); internal/sandbox/runner.go credentialDenyWriteRules (~921–935)

When: Custom policy with denyRead: ["/parent"] and token at /parent/bridge-token. pathsOutsideRoots removes token from MandatoryPaths / DenyReadIfExists. Parent gets read-deny via policy; credentialDenyWriteRules only write-denies paths with an exact entry in denied, not children covered by a parent deny. Sandboxed shell may still truncate the token; in-process tools remain blocked via protectedCredentialPaths().

Root cause: Mandatory token was folded into the same “user deny subsumes automatic mask” optimization used for optional credential stores.

Author guidance: Exempt MandatoryPaths from pathsOutsideRoots against user DenyRead, or always append mandatory token paths to credentialDenyWriteRules regardless of exact denied membership. Add test with custom denyRead parent + shell write attempt.

---

[P3] completeCreatedPatchTargets still uses legacy header parser

Where: internal/tools/apply_patch.go (~173+) vs internal/sandbox/risk.go PatchHeaderPaths

Issue: Validation uses strict shared parser; post-apply FileTracker bookkeeping still walks headers with local patchFileHeaderPath. Quoted operands and extended headers can diverge → rewind/observation credit wrong. Security gate is fixed; this is integrity adjacent.

Author guidance: Call PatchHeaderPaths once in completeCreatedPatchTargets or delete duplicate parser; extend tests if created-file detection depends on new header forms.

---

[P3] list_directory.Run() skips protected-credential exclusions

Where: internal/tools/list_directory.go — Run() passes empty readExcluder{}; RunWithOptions is correct.

Issue: MCP / legacy registry.Run without Sandbox can list the token filename. Pre-existing pattern; this PR fixed the agent path.

Author guidance: Only if #677 scope includes MCP: wire exclusions or document exclusion. Otherwise one sentence in PR: “MCP list_directory out of scope.”

---

Suggested “done” checklist for the author

Before requesting another review, confirm:

• Rebased on main; CI green; conflict resolutions described
• requestPaths uses exact path bytes; spaced-filename read_file e2e test passes
• End-to-end matrix (Step C) green or explicitly scoped out with comments
• Shell + file-token decision documented (Step D); help text matches behavior
• PatchHeaderPaths is the only patch path parser in production code
• No new findings from self-review by running: spaced path via Evaluate, file token + BuildCommandPlan on Linux, rebase diff limited to intended 27-file surface

If that checklist is green, the remaining questions are product boundaries (Windows #662, ModeDisabled shell, inline-token stale file), not another layer of implementation drip.

[PierrunoYT]

Took the consolidated pass as one round rather than another drip. 947584ae, pushed as two reviewable commits — a pure merge, then the fixes. PR description rewritten to match what actually ships.

Step A — rebase (blocker)

Merged main (d065467c). One correction to the finding: it reported conflicts in internal/cli/daemon.go plus overlapping sandbox edits, but on current head there was exactly one content conflict, in internal/cli/daemon_test.go — both sides appended test functions and imports, resolved as a union with nothing dropped or rewritten.

daemon.go merged cleanly, keeping both CanonicalizeTokenFileEnv() and terminateAndReapDaemonProcess / background.TerminateCommand. The sandbox files merged without conflict because this branch was already written against #681's credentialPathOptions shape — there is no flat credentialDenyReadPathsIn signature to reintroduce. mergeable is now MERGEABLE.

[P1] requestPaths — confirmed exploitable, fixed

Real, not theoretical. I reproduced it end to end before touching the gate: token named " bridge-token", read_file with the exact spelling returned the bearer.

read_file served the protected token under its exact spelling:
output="File:  bridge-token (1 lines)\n\n1 | bridge-secret"

The gate now reads the exact bytes the tool opens. I kept emitting the trimmed spelling when it differs, so the gate is a strict superset of its old coverage rather than a swap — a whitespace-padded argument is checked both ways instead of only the way the tool does not use.

One detail your write-up did not have, and it matters for anyone re-deriving this: the whitespace has to sit at the boundary of the argument string for TrimSpace to reach it, so the exploit needs the relative spelling. In an absolute path the space is mid-string (after the separator) and the old gate incidentally behaved. My first attempt at the regression used the absolute path, passed against the unfixed code, and would have shipped as a test that proved nothing — I caught it by reverting the fix and confirming the test actually fails. Worth knowing before writing the next one of these.

[P3] list_directory — your suggested fix would not have worked

The finding points at Run() passing an empty readExcluder{}. Patching Run() is dead code: Registry.Run funnels into RunWithOptions with empty options, so the MCP / legacy path never reaches Run() at all. I confirmed this the hard way — fixed Run(), test still failed.

Fixed at the shared helper instead. sandboxReadExcluderWithin applies the protected-credential set with or without an engine; policy DenyRead still requires one. The reasoning is that the protected set is derived from this process's environment rather than from a policy, so there is no engine to consult for it and no principled reason the engine-less path should disclose more. That also answers the "or document that MCP is out of scope" alternative — it turned out cheaper to just cover it.

Step B — pathname contract

Written into internal/sandbox/pathlists.go as four rules (data not a word; selected and resolved spellings; exact bytes at tool arguments; never re-includable), with the note that a consumer needing a fifth rule means the contract changes, not just that call site. The PR description carries the same table plus a layer-by-layer covers/does-not-cover matrix.

Step C — end-to-end matrix

TestDaemonTokenProtectionMatrix: read_file × write_file × list_directory × grep × apply_patch, crossed with exact, trailing-space, relative, dot-segment, and parent-traversal spellings, every cell through registry.RunWithOptions with the engine rather than a profile builder.

Fixture note: it verifies the token name against the real directory entry rather than Lstat, because Windows strips trailing spaces from the create and the lookup — an Lstat guard silently "passes" while asserting against a file that never existed. Rows skip rather than fail where the filesystem will not store the name.

Validation

go build ./..., go vet ./..., gofmt -l . clean, go test ./internal/... green.

Still open — deliberately not folded in

Step D (shell vs file token) is with the maintainer. The description now states the current behavior plainly — file-pointer auth works for in-process tools, sandboxed shell fails closed on default Unix layouts — and lays out both options without picking one, since it is a product boundary rather than an implementation detail.

Step E (patch parser) is not a one-line call as scoped. PatchHeaderPaths returns a flat path list, while completeCreatedPatchTargets needs /dev/null creation pairs to decide whole-file observation credit. Calling it directly would lose that and silently mis-credit rewinds. Unifying them properly means adding a pairs-returning API with PatchHeaderPaths as a flattener over it — happy to do that, but it is a real refactor, and the security gate is already on the shared parser.

[P3] mandatory-path write-deny (the pathsOutsideRoots / credentialDenyWriteRules exemption) is also still open.

I stopped rather than start the parser refactor half-finished and hand you a fourth partial layer — which is the pattern the review was calling out. Say the word on Step D and whether you want E and the write-deny in this PR or a follow-up, and I will close them out.

[jatmn]

I found issues that need to be addressed before this is ready.

Findings

• [P1] Complete the token exclusion for the engine-less MCP path
  internal/mcp/server.go:218
  The MCP dispatcher predates this PR, but its zero-options call bypasses Engine.Evaluate and therefore the new protectedCredentialPathBlock. This change explicitly identifies that engine-less route as the MCP/legacy production path and adds a fallback only to list_directory; read_file, read_minified_file, grep, glob, write_file, edit_file, and apply_patch remain outside the new boundary. For example, with the token at <workspace>/bridge-token, a default MCP tools/call can invoke read_file on that path, while grep and glob can expose its content or name; unsafe-tool MCP mode can overwrite it. Independently, resources/list advertises the file and resources/read returns its bytes without a tool call.

  Fix the boundary rather than each symptom: give the MCP server a credential-aware sandbox/guard for every tool invocation, and apply the same automatic exclusion to MCP resource enumeration and reads. If the intended design is tool-local enforcement, make every engine-less read, search, and mutation entry point consult the same protected-path predicate/exclusion. Add end-to-end MCP regressions covering tools/call read and write, resources/list, and resources/read with a token inside the served workspace.

• [P1] Preserve cwd bytes when deriving patch targets for the sandbox gate
  internal/sandbox/risk.go:290
  The tool resolves cwd verbatim, while this helper obtains it via firstArgString, which trims whitespace. With a workspace directory named " token-dir" containing the selected token, an apply_patch request using cwd: " token-dir" and a relative token header is checked as token-dir/token but git apply writes token-dir/token. The generic cwd request-path check does not save this case: it checks the directory itself, not the derived patch target. The token exclusion therefore misses and the patch can overwrite the bearer.

  Derive patch paths from the exact, type-checked cwd value that apply_patch consumes—do not use the display-oriented trimming helper for a pathname. Ideally centralize patch-root resolution so the tool, sandbox gate, mutation tracker, and any future caller cannot normalize it differently. Add regressions for leading and trailing whitespace in cwd, with relative patch headers targeting a protected token.

• [P3] Keep the mandatory token write denial when a user deny-read parent contains it
  internal/sandbox/profile.go:792
pathsOutsideRoots removes a mandatory daemon-token entry when the user has DenyRead on its parent. On macOS that leaves the parent with a read-only Seatbelt denial, while credentialDenyWriteRules derives its automatic write deny from DenyReadIfExists, where the token no longer appears. A broad writable-root rule can then let a sandboxed shell replace or unlink the bearer; the in-process check does not protect that shell path. That permits denial of service and can make the next daemon start accept an attacker-chosen token.

  Treat mandatory bridge-token paths as a separate invariant, not as optional credential-store candidates that may be eliminated by parent-coverage optimization. Keep them in the backend's write-deny source set even if a parent user rule subsumes their read deny, or make credentialDenyWriteRules always add the mandatory paths independently. Add a macOS profile regression with DenyRead: [token parent], a writable workspace parent, and assertions for both the exact token read and write denials.

Holistic guidance

The recurring findings have the same underlying cause: the PR is securing a credential whose pathname is interpreted by several independent layers, but each layer currently owns part of the contract. The daemon selects and canonicalizes the file; the engine evaluates tool requests; individual tools resolve aliases and working directories; the registry may invoke tools without an engine; MCP exposes both tools and resources; profile generation feeds OS sandboxes; and the Linux/macOS backends enforce different subsets of the profile. A local fix can therefore be correct in its own layer while another entry point still uses a different path transform, omits the automatic exclusion, or loses the write direction.

The desired invariant should be stated and implemented once: when the daemon selects ZERO_DAEMON_REMOTE_TOKEN_FILE, an untrusted client must not learn the selected file's name or bytes, nor alter the selected file or its future replacement, through any exposed Zero entry point. That invariant needs to hold independently of user AllowRead/permission grants, exact pathname spelling, aliases, whether a request originates through the agent, legacy registry, MCP tools, MCP resources, or an OS-wrapped shell.

I recommend treating the next revision as a boundary-completion pass rather than another collection of local fixes:

1. Create one credential-access authority. Expose a small shared API that answers both “is this path protected?” and “which paths may a walk/list expose?” using the same exact-byte anchoring and filesystem-identity rules. Do not make callers reconstruct that logic from environment variables or policy lists.

2. Enforce it at every dispatch boundary. The agent registry with an engine is only one boundary. Cover zero-options Registry.Run, MCP tools/call, and MCP resources/list / resources/read; then retain tool-local exclusions only where they are needed for recursive walks. This avoids having each of read_file, grep, glob, list_directory, and mutation tools independently remember a nil-engine special case.

3. Use one path-resolution pipeline for each operation. For apply_patch, resolve the effective root and parsed target paths once from exact, type-checked arguments, then share those resolved targets among the sandbox gate, tool validation, mutation tracking, and file-tracker bookkeeping. Avoid TrimSpace or other presentation coercions anywhere after an argument becomes pathname data.

4. Keep mandatory-token handling separate from optional credential discovery. Optional stores can be removed because a broader policy rule already masks them; the selected bridge token cannot, because it also needs write protection and fail-closed lifecycle behavior. Model its read and write requirements explicitly through profile generation and each native backend.

5. Test an entry-point matrix, not just helpers. Build one table-driven end-to-end suite that creates a token in the workspace and exercises: agent registry with an engine; registry with no engine; MCP tools/call; MCP resources; file reads; searches/lists; writes/edits/patches; exact, leading/trailing-space, relative, dot-segment, symlink, and hard-link spellings; and a user-deny parent. Each cell should assert that neither token bytes nor its name are exposed and that the file cannot be modified. Keep focused backend-profile tests for Linux/macOS/Windows semantics alongside that matrix.

6. Resolve the shell product decision before final review. The current same-filesystem hard-link preflight intentionally refuses normal file-token shell sessions on default Unix layouts. Decide whether that fail-closed behavior is the supported product contract or whether ordinary nlink == 1 file tokens must support shell use, then align help text, startup behavior, tests, and error guidance to that decision. Do not leave it as an implementation-side effect.

This approach makes a future review about one credential boundary and its matrix, rather than repeatedly discovering the next bypass in a sibling dispatcher or pathname parser.

Maintainer decision

The branch intentionally refuses ordinary file-token sessions from running sandboxed shell commands on Linux and macOS under the default read-all policy. The PR describes this as an open decision; it needs explicit maintainer acceptance before merge, since a normal local file token shares a filesystem with / and therefore causes command-plan construction to fail even with no existing hard-link alias.

[PierrunoYT]

PierrunoYT updated this branch to f5adef9a22cf0ae9a6baa714eafba68beb72c2d4, rebased onto upstream main (d66ad715). The update completes daemon-token exclusion across engine-less MCP file/search/mutation tools and resources/list/resources/read, preserves exact apply_patch cwd bytes, and retains mandatory exact token read/write denial when a parent DenyRead contains it. End-to-end regressions cover the affected MCP, patch, and Seatbelt paths.

Validation passed: focused regressions; affected package tests; make fmt-check; go vet ./...; go test ./...; release build; smoke; make lint-static (0 issues); and diff hygiene. make vulncheck still reports only the known GO-2026-6115 finding in github.com/ledongthuc/pdf, for which no fixed version is available; govulncheck found no other vulnerabilities.

Remaining maintainer decision: whether ordinary file-backed tokens should continue to fail closed for sandboxed shell commands on Linux/macOS because of hard-link alias risk, or whether normal nlink == 1 file-token configurations should permit shell use.