fix(sandbox): scrub dynamic credential env vars#682
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change passes provider-configured API key environment names into sandbox engines, propagates them through command plans, and scrubs them alongside dynamically named ChangesSensitive environment scrubbing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as CLI sandbox setup
participant Engine as Sandbox Engine
participant Plan as Command Plan
participant Scrubber as Environment Scrubber
CLI->>Engine: Pass provider-sensitive environment keys
Engine->>Plan: Propagate normalized keys
Plan->>Scrubber: Build and scrub command environment
Scrubber-->>Plan: Return filtered environment
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 security gap in the sandbox runner by expanding secret environment-variable scrubbing to include configuration-derived provider credential env var names (apiKeyEnv) and dynamically named OAuth client-secret variables (ZERO_OAUTH_<NAME>_CLIENT_SECRET) across Linux/macOS/Windows sandbox command plans.
Changes:
- Thread config-derived sensitive env key names from the CLI’s resolved config into
sandbox.Engine, then into per-command planning. - Extend
scrubSensitiveEnvto accept additional sensitive keys and to drop dynamically patterned OAuth client-secret variables case-insensitively. - Add tests covering both direct scrubbing behavior and engine propagation of configured sensitive env keys.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/sandbox/windows_runner.go | Uses sensitive-env-aware sandbox environment construction for Windows restricted token plans. |
| internal/sandbox/runner.go | Plumbs sensitive keys through command planning and expands scrubbing to include additional keys + dynamic OAuth patterns. |
| internal/sandbox/runner_test.go | Extends scrub tests and adds an engine-level test ensuring configured sensitive keys are removed from plans. |
| internal/sandbox/engine.go | Adds SensitiveEnvKeys support to engine options and normalizes keys on engine creation. |
| internal/cli/exec.go | Passes config-derived provider apiKeyEnv names into sandbox engine construction. |
| internal/cli/app.go | Passes config-derived provider apiKeyEnv names into the interactive app’s sandbox engine creation path. |
| internal/cli/sandbox_sensitive_env_test.go | Adds a unit test for collecting provider-sensitive env keys from resolved config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, key := range keys { | ||
| key = strings.TrimSpace(key) | ||
| folded := strings.ToUpper(key) | ||
| if key == "" { | ||
| continue | ||
| } | ||
| if _, ok := seen[folded]; ok { | ||
| continue | ||
| } | ||
| seen[folded] = struct{}{} | ||
| out = append(out, key) | ||
| } |
A config value mistakenly given as a full assignment (e.g. "COMPANY_LLM_SECRET=...") would never match the real env key during scrubbing, silently re-exposing the credential to sandboxed commands. normalizeSensitiveEnvKeys now keeps only the name part before '=' and re-trims it, per PR review. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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] Scrub credentials on degraded command plans too
internal/sandbox/runner.go:169
BuildCommandPlanattaches the configured keys tospec, but the supported unavailable-backend fallback returnsdirectCommandPlan, which simply leavesEnvnil. The normal bash tool creates a spec with nilEnv, soexec.CmdinheritsCOMPANY_LLM_SECRETandZERO_OAUTH_<NAME>_CLIENT_SECRETunchanged whenever the native backend is unavailable (the intendedEnforcementDegradedpath covered byTestBuildCommandPlanDegradesUnavailableFallbackandTestUnavailableBackendsDegradeForTargetPlatforms). That leaves the reported credential-exfiltration scenario open on fallback hosts; construct a scrubbed environment for direct/degraded plans as well and add an inherited-environment regression test.
directCommandPlan (used when the native sandbox backend is unavailable, disabled, or not required, including the EnforcementDegraded fallback) left Env nil whenever spec.Env was nil, so exec.Cmd inherited the full caller environment unscrubbed. Configured apiKeyEnv credentials and dynamically named ZERO_OAUTH_<NAME>_CLIENT_SECRET variables leaked through that path even though the wrapped sandbox plans already scrubbed them. Route direct plans through scrubSensitiveEnv the same way wrapped plans do, snapshotting os.Environ() when spec.Env is nil so scrubbing still applies. Adds a regression test covering the degraded-fallback path with both a configured and a dynamically named secret. 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 7b59fc83 (head). The nil spec.Envsnapshot fix (patch 3/3) closes a real leak: direct/unwrapped command plans were inheriting the parent env unscrubbed, defeating the whole point of the scrubber — a sandboxed command could seeOPENAI_API_KEYetc. by going through the direct plan path. That was the P1 and the fix is correct (snapshotos.Environ()then runscrubSensitiveEnvbefore forking). The configurableSensitiveEnvKeysplumbing from CLI → engine is clean, and the case-insensitiveZERO_OAUTH_*_CLIENT_SECRETpattern viastrings.ToUpper` is the right shape for env vars.
LGTM. Minor follow-up (not blocking): the isDynamicSensitiveEnvKey length check rejects the empty-provider case ZERO_OAUTH__CLIENT_SECRET (double underscore) — a one-line test for that edge case would be cheap. Also worth documenting the dynamic-pattern list near the function so a future maintainer knows where to add a second pattern.
Cross-PR note: #682 sits between #681 and #685. Recommend rebasing #682 onto #681, then #685 onto #682, to resolve the credentialDenyReadPaths / scrubSensitiveEnv signature overlap cleanly.
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Local review: built and ran go test ./internal/cli ./internal/sandbox on darwin/arm64 (cli 94s); all pass. The direct/degraded-plan env scrubbing (including the spec.Env == nil -> os.Environ() snapshot) is a genuine security fix. One integration note.
| } | ||
|
|
||
| func scrubSensitiveEnv(env []string) []string { | ||
| func scrubSensitiveEnv(env []string, additionalKeys ...string) []string { |
There was a problem hiding this comment.
[P2] Sibling-PR merge conflict on scrubSensitiveEnv in runner.go
This PR changes the signature to scrubSensitiveEnv(env, additionalKeys ...string) and adds normalizeSensitiveEnvKeys. #685 adds a hardcoded ZERO_DAEMON_REMOTE_TOKEN_FILE to the same function's literal list, and both touch runner_test.go's TestScrubSensitiveEnv. Each merges into main cleanly on its own, but they conflict with each other; #681 also rewrites credentialDenyReadPathsIn in profile.go. Sequence the three sandbox credential PRs and rebase as needed.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Looks good to me. This closes a real leak: config-derived apiKeyEnv names and dynamic ZERO_OAUTH_*CLIENT_SECRET vars are now scrubbed on every plan path (linux/seatbelt/windows wrapped plus the degraded direct fallback that previously inherited the caller env unscrubbed). I checked out the branch and confirmed go build, go vet, and gofmt are clean on the touched files, and the new tests (TestScrubSensitiveEnv, TestEngineScrubsConfiguredSensitiveEnvKeys, TestBuildCommandPlanDegradedFallbackScrubsInheritedEnv, TestProviderSensitiveEnvKeys) pass. The bare ZERO_OAUTH_CLIENT_SECRET (no provider segment) the reviewer noted is preserved by the prefix/suffix length guard, but that's fine in practice: Zero's envKey always emits ZERO_OAUTH_CLIENT_SECRET with a name segment, so that exact bare name is never a credential Zero sets.
Summary
apiKeyEnvinto the sandbox engineZERO_OAUTH_<NAME>_CLIENT_SECRETvariables for dynamically named OAuth providersRoot cause
The sandbox environment scrubber knew only a static secret list and the built-in provider catalog. Custom provider profiles can name arbitrary credential variables through
apiKeyEnv, and OAuth provider names create client-secret variables dynamically, so neither category was represented in that fixed set.Sandboxed commands could therefore inherit those credentials even though built-in provider secrets were removed.
Fixes #676
Validation
go fmt ./...go vet ./...go test ./internal/sandbox ./internal/cli -count=1go test ./... -count=1completed successfully for the changed packages and all but unrelated Windows process-lifecycle testsgo run golang.org/x/vuln/cmd/govulncheck@v1.3.0 ./...: no vulnerabilities foundKnown baseline/environment limitations
TestLoadProviderCommandTimeouttakes about 10 seconds instead of its expected 5 seconds on this host.TestManagerCheckRealGoplsfails because the installedgoplsprocess exits with EOF.internal/toolsprocess-termination and temporary-directory cleanup tests fail on Windows because child processes retain file handles.make lintuses a Unix shell assignment infmt-checkthat does not run under this Windows shell.Summary by CodeRabbit
Security Improvements
ZERO_OAUTH_*_CLIENT_SECRETpattern).Bug Fixes
Tests