fix(sandbox): scrub the daemon token file pointer (#677) - #818
Conversation
scrubSensitiveEnv removed ZERO_DAEMON_REMOTE_TOKEN but not its file counterpart. TokenFromEnv accepts either form, so a sandboxed command still inherited the path to the bridge token, and the default posture is read-all, so it could then read the token itself. Scrubbing the pointer is the whole fix rather than half of one: the path comes from this variable alone and TokenFromEnv has no default location to fall back on, so a command that never sees the variable has nothing to guess at. This mirrors GOOGLE_APPLICATION_CREDENTIALS, which is already scrubbed for the same reason. Extends the existing scrub table, which compares the surviving environment exactly, so the case fails if the entry is dropped. Verified: without the fix the assertion reports ZERO_DAEMON_REMOTE_TOKEN_FILE surviving into the scrubbed environment.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe sandbox environment scrubber now removes ChangesDaemon token environment scrubbing
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
@anandh8x @gnanam1990 putting the whole picture in one place rather than commenting on five PRs, since between you the queue clears. @anandh8x, three of these need only you. Each is at two of three with @gnanam1990 and CodeRabbit already in, so a single approval on each takes them to CLEAN:
Also #750 Both of you on #818 @gnanam1990 on #808 No rush on any of it. Flagging it because six of my eight open PRs are short on approvals rather than short on work, and I would rather ask once than nudge repeatedly. |
2 participants
Fixes #677.
scrubSensitiveEnvremovedZERO_DAEMON_REMOTE_TOKENbut notZERO_DAEMON_REMOTE_TOKEN_FILE.TokenFromEnv(internal/daemon/remote/auth.go:72-86) accepts either form, so a sandboxed command still inherited the path to the bridge token, and since the default posture is read-all it could then read the token out of that file.Why the one line is the whole fix
I considered also adding the target to the credential deny-read list, and decided against it. The path comes from this variable alone and
TokenFromEnvhas no default location to fall back on, so a command that never sees the variable has nothing to guess at. Adding a deny rule for an arbitrary env-supplied path would also collide with #816, which is currently in flight in the same function, for no extra protection.This mirrors the existing treatment of
GOOGLE_APPLICATION_CREDENTIALS, which is scrubbed for exactly this reason. The comment there already spells out the rule; this variable simply matched the description and was missed.Verification
Extended the existing scrub table rather than adding a new test, because
TestScrubSensitiveEnvcompares the surviving environment exactly, so a missed key fails the assertion by construction. Both forms of the token now go in and neither is expected out.Dropping the production line reproduces the leak:
go test ./internal/sandbox/passes in full,gofmtandgo vetclean.Scope
Two files, twelve added lines, no behaviour change beyond the scrub itself. Keeping it deliberately small: #685 covered adjacent ground and grew to fifteen files, which is a large part of why it stalled.
Summary by CodeRabbit