fix: env_clear() — make [agent].env a true whitelist#670
Conversation
Agent subprocess inherited all OAB env vars (including DISCORD_BOT_TOKEN, SLACK_BOT_TOKEN, etc.) because spawn() never called env_clear(). Now the child process starts with a clean environment and only receives: - HOME and PATH as baseline - Explicit [agent].env entries from config Fixes #669
|
All PRs must reference a prior Discord discussion to ensure community alignment before implementation. Please edit the PR description to include a link like: This PR will be automatically closed in 3 days if the link is not added. |
All supported backends use OAuth/file-based auth and do not need env var injection. Removing [agent].env eliminates the risk of accidentally passing sensitive credentials to the agent subprocess.
…CLI" This reverts commit 276cecb.
Log a warning at startup listing the env var keys being passed to the agent. Also add a security warning in config.toml.example advising users to prefer OAuth login over env var API keys.
- Add USER baseline env var (many Unix tools and git need it) - PATH falls back to /usr/local/bin:/usr/bin:/bin instead of empty string - Remove emoji from tracing::warn! for log aggregation compatibility
Co-authored-by: 普渡法師 <pudo-agent@users.noreply.github.com>
env_clear() on Windows removes SystemRoot which breaks DLL loading. Add platform-specific baseline vars: - Unix: USER - Windows: USERPROFILE, USERNAME, SystemRoot, SystemDrive Co-authored-by: 覺渡法師 <juedo-agent@users.noreply.github.com>
chaodu-agent
left a comment
There was a problem hiding this comment.
Blocking finding: cmd.env("HOME", working_dir) in src/acp/connection.rs introduces a compatibility regression for configurations where working_dir is only a workspace path, not the agent home directory. On main, working_dir is documented as the agent working directory in README.md, and its default remains /tmp in src/config.rs. Before this PR, child processes inherited the real HOME, so locally authenticated CLIs could still find ~/.codex, ~/.claude, ~/.config/gh, etc. After this change, any config that leaves working_dir = "/tmp" (or points it somewhere other than the auth home) will lose access to those existing credentials and can fail even though no secrets are being leaked anymore. env_clear() itself looks right; the risky part is repurposing working_dir as HOME.
chaodu-agent
left a comment
There was a problem hiding this comment.
Re-review on latest head (a6ceeee, de41c50): the Windows baseline env additions look good, but my earlier blocking concern is still unresolved because the runtime behavior did not change. HOME is still forcibly set to working_dir; the new comment clarifies that this is intentional, but it does not remove the compatibility regression for configs where working_dir is only a workspace path or is left at the default /tmp. So my verdict is unchanged: env_clear() is good, Windows baseline vars are good, but repurposing working_dir as HOME still breaks previously working auth setups unless users also discover and override it via [agent].env.
working_dir defaults to /tmp and is the agent's cwd, not its home. Setting HOME=working_dir breaks OAuth auth file lookup (~/.codex, ~/.claude, ~/.config/gh). Preserve the real HOME from the parent process, falling back to working_dir only if HOME is unset. Same fix for USERPROFILE on Windows. Co-authored-by: 擺渡法師 <baidu-agent@users.noreply.github.com>
chaodu-agent
left a comment
There was a problem hiding this comment.
Re-review on latest head (51bc256): this resolves my previous blocker. The child process now preserves the real HOME/USERPROFILE from the parent environment, falling back to working_dir only when those vars are unset, so existing OAuth/auth file lookup paths (~/.codex, ~/.claude, ~/.config/gh, etc.) keep working. env_clear() still provides the intended credential-leakage fix, and the Windows baseline env additions remain correct. LGTM from my side.
Summary
Fixes #669
Agent subprocess inherited all OAB process env vars because
spawn()never calledenv_clear(). This meansDISCORD_BOT_TOKEN,SLACK_BOT_TOKEN, and other OAB credentials were visible to the agent — a user could ask the agent toecho $DISCORD_BOT_TOKENand get the real value back.Before vs After
DISCORD_BOT_TOKEN, etc.)env_clear()[agent].envvaluesHOME,PATH[agent].envis setChanges
cmd.env_clear()inAcpConnection::spawn()— agent starts with a clean environmentHOMEandPATHare passed as baseline[agent].envbecomes a true whitelist — only explicitly listed env vars are passed through[agent].envis configured, listing the key names being exposedconfig.toml.exampleadvising OAuth login over env var API keysImpact per backend
All supported backends use OAuth/file-based auth and work without
[agent].env:~/.kiro/credentialsclaude login(OAuth)codex login(OAuth)gemini login(OAuth)gh auth login(OAuth)opencode auth login[agent].envremains available for users who need to pass custom env vars, with a startup warning about the exfiltration risk.What was tested
std::process::CommandAPI