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 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. ChangesDaemon token protection
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. The early-out is in, and documenting the inline-token precedence in the help text answers the question I actually had: it is intentional rather than an oversight, which is all I needed.
I mutation-checked the three uncovered paths rather than taking the new tests at face value. Two are genuinely closed now: deleting the protected branch in DirExcluded fails TestProtectedCredentialDirExcluded, and removing the CanonicalizeTokenFileEnv block fails TestDaemonServeRemoteCanonicalizesTokenFileBeforeStartingWorkers.
One is still open, not blocking: the || len(protectedCredentialPaths()) > 0 disjunct at pathlists.go:478. Dropping it produces no new failures, so that path still carries behaviour nothing pins. Worth a case whenever you next touch the file.
Two notes on my own testing so you can discount them appropriately. Six sandbox tests fail for me locally, and they fail identically on clean origin/main from the same directory, so that is my environment rather than anything here. And my first attempt at the CanonicalizeTokenFileEnv mutation only removed the if line, which broke the build and looked like "no new failures" until I checked whether it had compiled.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. The early-out is in, and documenting the inline-token precedence in the help text answers the question I actually had: it is intentional rather than an oversight, which is all I needed.
I mutation-checked the three uncovered paths rather than taking the new tests at face value. Two are genuinely closed now: deleting the protected branch in DirExcluded fails TestProtectedCredentialDirExcluded, and removing the CanonicalizeTokenFileEnv block fails TestDaemonServeRemoteCanonicalizesTokenFileBeforeStartingWorkers.
One is still open, not blocking: the || len(protectedCredentialPaths()) > 0 disjunct at pathlists.go:478. Dropping it produces no new failures, so that path still carries behaviour nothing pins. Worth a case whenever you next touch the file.
Two notes on my own testing so you can discount them appropriately. Six sandbox tests fail for me locally, and they fail identically on clean origin/main from the same directory, so that is my environment rather than anything here. And my first attempt at the CanonicalizeTokenFileEnv mutation only removed the if line, which broke the build and looked like "no new failures" until I checked whether it had compiled.
|
@jatmn @gnanam1990 this one is at one of three: my approval, and CodeRabbit's was dismissed by the last push. I have re-triggered it. Since either of you last looked, @PierrunoYT closed the two coverage gaps I raised and added the empty-list early-out on the hot path. I mutation-checked rather than trusting the new tests: deleting the protected branch in The conditional protection I asked about is answered and documented: an inline token takes precedence, so a stale token-file pointer is intentionally not protected as the live credential. @kevincodex1 not ready for you yet either, same as #757. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tools/exec_command_test.go (1)
20-21: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit unrelated exec-command test changes from this security PR.
These Windows-cleanup, shell-guidance, session-lifecycle, and outcome-classification changes are outside the daemon-token protection objective, making the security change harder to validate and revert.
As per coding guidelines, “Keep each change focused on its approved or assigned scope; do not include unrelated fixes, refactors, formatting churn, generated output, or existing local changes.”
Also applies to: 79-90, 92-93, 141-141, 197-197, 206-214, 224-225, 234-234, 273-273, 304-306, 337-355, 367-367, 421-421, 547-547, 578-610, 612-688, 728-728, 766-766
🤖 Prompt for 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. In `@internal/tools/exec_command_test.go` around lines 20 - 21, Remove the unrelated exec-command test and implementation changes identified in this review, including Windows cleanup, shell guidance, session lifecycle, and outcome-classification updates around TestIndependentExecCommandConstructorsShareDefaultManager and the referenced hunks. Keep this security PR limited to daemon-token protection, preserving only changes required for that objective.Source: Coding guidelines
🧹 Nitpick comments (4)
internal/sandbox/protected_credentials_test.go (1)
315-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe case-variant expectation is derived from the same helper under test.
wantDenied := protectedPathFoldsCase()means the test passes vacuously ifprotectedPathFoldsCaseitself regresses (e.g. someone dropsdarwin) — the expectation flips along with the behavior. Probing the actual filesystem gives an independent oracle: the fixture token already exists, soos.Stat(variant)succeeding proves the volume folds case.💚 Independent expectation
- wantDenied := protectedPathFoldsCase() + // The volume itself decides: the token exists, so if its upper-case spelling + // stats, a case-variant request opens the same bearer-token file. + _, statErr := os.Stat(variant) + wantDenied := statErr == nil + if wantDenied != protectedPathFoldsCase() { + t.Fatalf("protectedPathFoldsCase() = %t, but the filesystem folds case = %t", protectedPathFoldsCase(), wantDenied) + }🤖 Prompt for 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. In `@internal/sandbox/protected_credentials_test.go` around lines 315 - 339, Update TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems to derive wantDenied from the filesystem rather than protectedPathFoldsCase: use whether os.Stat(variant) succeeds as the independent case-folding expectation, while preserving the existing validation and read-exclusion assertions.internal/sandbox/profile.go (1)
188-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
credentialDenyReadPathsInis test-only production code — move it into the test file.Its doc comment says it exists so tests can pass a synthetic home, and the advisory check flags it as unreachable. Since
credentialDenyReadPathsForEnvironmentis already the real seam, this wrapper only adds a fourth positional parameter for callers to get wrong (and it is the exact signature the sibling PR noted as a merge-conflict surface). Relocating it tointernal/sandbox/manager_test.go— or dropping it and constructingcredentialPathEnvironmentdirectly in the test — clears the finding without changing behavior.🤖 Prompt for 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. In `@internal/sandbox/profile.go` around lines 188 - 196, Remove the test-only wrapper credentialDenyReadPathsIn from production code. Update its callers in internal/sandbox/manager_test.go to invoke credentialDenyReadPathsForEnvironment with a credentialPathEnvironment directly, preserving synthetic-home test behavior and avoiding the extra positional-argument seam.Source: Linters/SAST tools
internal/cli/daemon_test.go (1)
180-206: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify PEM writing with
pem.EncodeToMemory+os.WriteFile.The create/encode/close triples double the length of the helper, and a
t.Fatalbetweenos.CreateandCloseleaves a handle open (which can maket.TempDircleanup noisy on Windows).♻️ Proposed simplification
dir := t.TempDir() certFile, keyFile := filepath.Join(dir, "cert.pem"), filepath.Join(dir, "key.pem") - certOut, err := os.Create(certFile) - if err != nil { - t.Fatal(err) - } - if err := pem.Encode(certOut, &pem.Block{Type: "CERTIFICATE", Bytes: der}); err != nil { - t.Fatal(err) - } - if err := certOut.Close(); err != nil { - t.Fatal(err) - } + if err := os.WriteFile(certFile, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}), 0o600); err != nil { + t.Fatal(err) + } keyDER, err := x509.MarshalECPrivateKey(key) if err != nil { t.Fatal(err) } - keyOut, err := os.Create(keyFile) - if err != nil { - t.Fatal(err) - } - if err := pem.Encode(keyOut, &pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}); err != nil { - t.Fatal(err) - } - if err := keyOut.Close(); err != nil { - t.Fatal(err) - } + if err := os.WriteFile(keyFile, pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}), 0o600); err != nil { + t.Fatal(err) + } return certFile, keyFile🤖 Prompt for 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. In `@internal/cli/daemon_test.go` around lines 180 - 206, Update the certificate/key-writing helper around the PEM encoding logic to use pem.EncodeToMemory followed by os.WriteFile for both certFile and keyFile. Remove the os.Create, explicit Close, and intermediate file-handle error paths while preserving the existing PEM block types, encoded contents, permissions, and t.Fatal handling.internal/sandbox/pathlists.go (1)
170-192: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftThe inode-alias check makes every walked file pay
EvalSymlinks+ twoStats.
ReadExclusions.PathExcluded/DirExcludedcall this for each entry during grep/glob/list_directorywalks, so on a large tree the fallback path costs anEvalSymlinks(onelstatper path component) plus aStatper protected entry — per file — even though the lexical check already covers the overwhelmingly common case. Two cheap mitigations: stat the protected entries once (they don't change within a walk) and skip the inode comparison when no protected entry shares the request's base name, since a hard/symlink alias to the token that resolves to the same inode still has to be reached by name.Not a blocker for correctness, but worth measuring before this lands on the search hot path.
🤖 Prompt for 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. In `@internal/sandbox/pathlists.go` around lines 170 - 192, Optimize the inode-alias fallback used by ReadExclusions.PathExcluded and DirExcluded: cache each protected entry’s os.FileInfo once per walk instead of calling os.Stat inside the per-path loop, and only perform filepath.EvalSymlinks plus inode comparison when the requested path’s base name matches a protected entry’s base name. Preserve the existing lexical exclusion and SameFile behavior.
🤖 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.
Outside diff comments:
In `@internal/tools/exec_command_test.go`:
- Around line 20-21: Remove the unrelated exec-command test and implementation
changes identified in this review, including Windows cleanup, shell guidance,
session lifecycle, and outcome-classification updates around
TestIndependentExecCommandConstructorsShareDefaultManager and the referenced
hunks. Keep this security PR limited to daemon-token protection, preserving only
changes required for that objective.
---
Nitpick comments:
In `@internal/cli/daemon_test.go`:
- Around line 180-206: Update the certificate/key-writing helper around the PEM
encoding logic to use pem.EncodeToMemory followed by os.WriteFile for both
certFile and keyFile. Remove the os.Create, explicit Close, and intermediate
file-handle error paths while preserving the existing PEM block types, encoded
contents, permissions, and t.Fatal handling.
In `@internal/sandbox/pathlists.go`:
- Around line 170-192: Optimize the inode-alias fallback used by
ReadExclusions.PathExcluded and DirExcluded: cache each protected entry’s
os.FileInfo once per walk instead of calling os.Stat inside the per-path loop,
and only perform filepath.EvalSymlinks plus inode comparison when the requested
path’s base name matches a protected entry’s base name. Preserve the existing
lexical exclusion and SameFile behavior.
In `@internal/sandbox/profile.go`:
- Around line 188-196: Remove the test-only wrapper credentialDenyReadPathsIn
from production code. Update its callers in internal/sandbox/manager_test.go to
invoke credentialDenyReadPathsForEnvironment with a credentialPathEnvironment
directly, preserving synthetic-home test behavior and avoiding the extra
positional-argument seam.
In `@internal/sandbox/protected_credentials_test.go`:
- Around line 315-339: Update
TestProtectedCredentialsMatchCaseVariantOnCaseInsensitiveFilesystems to derive
wantDenied from the filesystem rather than protectedPathFoldsCase: use whether
os.Stat(variant) succeeds as the independent case-folding expectation, while
preserving the existing validation and read-exclusion assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d394710e-2acc-461f-940e-6fc59d71af77
📒 Files selected for processing (15)
internal/cli/daemon.gointernal/cli/daemon_test.gointernal/daemon/remote/auth_test.gointernal/sandbox/engine.gointernal/sandbox/manager_test.gointernal/sandbox/pathlists.gointernal/sandbox/profile.gointernal/sandbox/protected_credentials_test.gointernal/sandbox/runner.gointernal/sandbox/runner_test.gointernal/tools/bash_auto_allow_test.gointernal/tools/daemon_token_exclusion_test.gointernal/tools/exec_command_test.gointernal/tools/list_directory.gointernal/tools/read_exclusions_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/daemon/remote/auth_test.go
- internal/cli/daemon.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] Fail closed when the automatic token restriction cannot be enforced
internal/sandbox/manager.go:231
The new token deny is added only to the derivedPermissionProfile, while this availability gate examines only user-configuredPolicy.DenyRead/DenyWrite. Consequently, if the native backend is unavailable, a remote worker withZERO_DAEMON_REMOTE_TOKEN_FILEstill receives anEnforcementDegradeddirect shell plan. The pointer is scrubbed, but a shell command can read a workspace-local token path without any OS restriction and recover the bridge bearer token. Treat a selected protected credential as an enforceable deny for this fallback decision, or reject the remote shell when no native sandbox is available. -
[P1] Do not leave the bridge token reachable through a macOS shell hard-link
internal/sandbox/pathlists.go:161
A remote worker may have both a writable workspace and a token file within it (the new fixture uses exactly that placement). Seatbelt denies the original pathname only, so the worker can create a hard link and read the new name:ln bridge-token token-alias && cat token-alias. The in-processSameFilecheck does not apply to shell commands, so this exposes the bearer token despite the new protection. The code currently documents this limitation rather than preventing it; make this placement fail closed or enforce a boundary that does not expose the token inode to sandboxed shell commands.
|
@jatmn your three P1s look resolved — they were against Rebase / conflicts. #816 git credential denies. Present and byte-identical to
#811 provider-command timeout. So the stale-branch drift is gone. Your two P2s — the macOS hard-link alias and Flagging because this is sitting on a stale blocker. I have no view yet on the P2s; I only re-checked the three that were about drift. |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
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.
The final follow-up correctly resolves the earlier Bubblewrap concern: canonicalizing the selected token file at the daemon boundary keeps a symlink pathname out of the Linux deny-mount list, so this is not a request to restore that broken approach. I also agree that ModeDisabled intentionally leaves shell commands outside the OS boundary, and do not treat the pre-existing Windows ACL limitation as a finding here. The two paths below occur with the normal enforcing policy and remain separate from those accepted boundaries.
Findings
-
[P1] Fail closed when the automatic token restriction cannot be enforced
internal/sandbox/manager.go:231
The selected daemon token is added only to the derivedPermissionProfile.FileSystem.DenyRead, while the unavailable-native-backend gate examines only the user-configuredPolicy.DenyRead/DenyWrite. With the default policy and an unavailable backend, a remote worker whose token is in its workspace receives anEnforcementDegradeddirect shell plan. The pointer is scrubbed, but that shell can read a known or discovered workspace token path and recover the bridge bearer token. This is not the intentionally openModeDisabledcase from the follow-up: the policy remains enforcing and it is the backend fallback that drops the derived deny. Treat the effective protected credential deny as enforceable for this fallback, or reject remote shell execution when native isolation is unavailable. -
[P1] Do not leave the bridge token reachable through a macOS shell hard-link
internal/sandbox/runner.go:789
Seatbelt denies the configured pathname, but a token inside a writable workspace can be linked to a new allowed pathname withln bridge-token token-aliasand then read through the alias. The new inode comparison only protects in-process tools; the shell profile is explicitly pathname-based and the broad workspace write rule allows creating the link. The follow-up accurately documents this limitation, but documenting a bearer-token disclosure does not make it safe for the remote bridge's stated protection boundary. Reject a token location under a shell-writable root, or use an enforcement boundary that does not expose the token inode to sandboxed shells.
Amp-Thread-ID: https://ampcode.com/threads/T-019fb8cf-a980-7568-80f1-fe34faaaecae Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
fefbdf8
|
PierrunoYT addressed both current P1 security findings in
Added regressions for unavailable-backend rejection, writable workspace/temp/unrestricted placement, case variants, symlink placement, safe outside-root placement, and disabled/forbid boundaries. Validation passed:
@jatmn @Vasanthdev2004 — PierrunoYT requests a fresh review of the current head. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P1] Parse quoted patch headers before enforcing token-file writes
internal/sandbox/risk.go:327
The sandbox-side parser usesstrings.Fieldsfor---/+++headers, unlikeapply_patch's parser, which preserves and unquotes the full pathname. For a protected token namedbridge token, a normal quoted Git header is reduced tobridge; the engine therefore does not identify the token, butgit applyreceives the real path and writes it. Parse the complete header remainder with the same quoted-path handling as the tool parser, and add an end-to-end patch test using a protected path containing spaces.
Amp-Thread-ID: https://ampcode.com/threads/T-019fbdfb-b1e9-765e-ac9c-240512dfcd2f Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
PierrunoYT addressed the quoted patch-header bypass in
Validation passed:
@jatmn — PierrunoYT requests a fresh review of the current head. |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
-
[P1] Parse quoted
diff --gitpaths before applying protected-token patches
internal/sandbox/risk.go:319
This only fixes quoted unified---/+++headers. Both this parser and the separate tool-side parser ininternal/tools/apply_patch.go:234still tokenizediff --gitwithstrings.Fields, which turns a valid header such asdiff --git "a/bridge token" b/exposed-tokeninto path fragments instead ofbridge tokenandexposed-token.That matters because Git accepts header-only copy/rename patches and binary patches without any
---/+++headers. A remote session can submit a 100%-similarity copy frombridge tokentoexposed-token; the sandbox'srequestPathsand the tool'svalidatePatchPaths/recheckPatchWriteTargetsboth miss the token, thengit applycopies its bytes to the ordinary workspace path. I verified thatgit apply --checkaccepts this form and that applying it creates an identical, readable copy. The remote agent can then retrieve the bearer throughread_file; rename and binary variants can also move or modify the protected file.Please replace the
strings.Fieldshandling with one shared Git-path parser used by both packages. It should consume the two path operands afterdiff --git, honor Git's C-style quoting/escapes, strip thea//b/prefixes only after unquoting, and retain both source and destination paths for copy/rename patches. Add end-to-end regressions with a spaced token filename for: (1) header-only copy, asserting the copy is denied and no exposed file is created; (2) header-only rename, asserting the token remains in place; and (3) a binary patch, asserting the token cannot be modified. Keeping those tests at theapply_patchtool boundary is important because both the engine gate and the tool's pre/post-apply checks need to agree on the same paths.
Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-bf93-702d-9a2a-35f0335e1e0c Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Addressed the latest patch-path review in ab2b7d4. Changes:
The prior Windows Smoke failure was attributable to this PR: the old Validation (Go 1.26.5):
No unrelated CI limitation remains from the observed failure. @jatmn, please re-review when you have a chance. |
Amp-Thread-ID: https://ampcode.com/threads/T-019fbec7-bf93-702d-9a2a-35f0335e1e0c Co-authored-by: Pierre Bruno <pierrebruno@hotmail.ch>
|
Follow-up: Current head: 4ebdfce (includes parser fix ab2b7d4). Post-merge validation (Go 1.26.5):
|
PatchHeaderPaths yields nothing for a `diff --git` line whose two operands both contain unquoted spaces and name different files: no split is recoverable from the line alone. That is only safe because git cannot resolve it either — real patches of that shape carry the copy/rename headers that disambiguate them, and without those git refuses the patch outright, so the token gate has nothing to miss. This asserts both halves, so a future git that accepted the form would fail here instead of silently becoming a gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Every finding across the four change requests is addressed on the current head. Rather than restate the commits, I re-verified each one adversarially and by mutation — details below, plus one small test added in [P1] Quoted Verified by mutation: restoring I also probed the parser beyond the three required regressions; all of these resolve to the protected path and are denied end-to-end: octal-escaped non-ASCII ( A residual worth naming (and the reason for the new commit): one shape stays unresolvable — [P1] Fail closed when the token restriction cannot be enforced (07-31 15:52) — [P1] macOS shell hard-link on a token in a writable root (07-31 15:52) — same commit, and it is a rejection rather than documentation: [P1] Quoted unified [P2] Verification on this head: |
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 trailing whitespace in unified-diff paths
internal/sandbox/risk.go:479
patchFileHeaderPathremoves the optional tab-delimited timestamp and then callsstrings.TrimSpaceon the entire path. That changes a valid unquoted filename ending in a space: for a configured token atbridge-token, both the sandbox request-path gate and the tool's pre/post-apply validation evaluatebridge-tokeninstead.git apply --whitespace=nowarndoes not make that transformation; it accepts--- a/bridge-token/+++ b/bridge-tokenand writes the space-suffixed file, so a remote session can patch the live bearer token despite the new protection.The root cause is that the parser is treating filename data as header formatting. Keep all bytes before the first tab as the unquoted pathname, and apply C-style unquoting only when the operand is quoted; alternatively, reject unquoted headers with ambiguous leading/trailing whitespace before passing them to Git. Please add an end-to-end protected-token regression that applies this patch form to a trailing-space filename and asserts the token is unchanged.
-
[P1] Keep the protected token path denied when the file is temporarily absent
internal/sandbox/profile.go:310
The mandatory token path is discarded whenos.Statfails, even thoughprotectedCredentialPathsdeliberately retains the configured lexical path so in-process tools cannot recreate it. This splits the security boundary: after an external secret rotation deletes the file, the next Linux shell profile contains noDenyReadentry for that workspace or temp-root pathname. A remote worker can then recreate the file with an attacker-selected bearer value; the running bridge continues using its already-loaded token, but the nextserve-remotestart reads the replacement and accepts that attacker-selected credential.The root cause is applying the credential-store existence filter to a path that must also be protected as a future write target. Preserve the selected token pathname in the effective OS deny/write set even when it is absent. The Linux builder already has an
appendUnreadableLinuxPathArgsfallback that materializes a missing deny target with atmpfsmount, so wire this mandatory path through that mechanism and give other backends equivalent fail-closed handling. Add a regression that removes a workspace-local token after bridge setup, builds the next shell profile, and proves the file cannot be recreated or selected on restart.
Summary
ZERO_DAEMON_REMOTE_TOKEN_FILEfrom inherited sandbox command environmentsAllowReadopt-out behaviorRoot cause
The sandbox scrubbed the inline
ZERO_DAEMON_REMOTE_TOKENvalue but left its file-pointer alternative in the child environment. Under the read-all workspace posture, a sandboxed command could read that pointer and then the bearer-token file outside the workspace.The pointer is now scrubbed on every platform. Its existing normalized target is protected through the same credential deny-read mechanism used for
GOOGLE_APPLICATION_CREDENTIALSon platforms where that mechanism is enabled. Windows filesystem deny-read remains subject to the existing ACL-model limitation tracked by #662.Fixes #677
Validation
go test ./internal/sandbox -count=1go vet ./...git diff --checkThe repository-pinned
golangci-lint@v2.12.2andgovulncheck@v1.3.0currently build with Go 1.25.12 and cannot load this repository's Go 1.26.5 packages.Summary by CodeRabbit