fix(core): close env leak regressions in claude subprocess + remediation copy#1154
fix(core): close env leak regressions in claude subprocess + remediation copy#1154LaplaceYoung wants to merge 1 commit intocoleam00:devfrom
Conversation
📝 WalkthroughWalkthroughThis PR implements environment variable sanitization to prevent Claude-specific marker variables from leaking into subprocess environments and updates error remediation commands to dynamically reference actual offending keys found. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/providers/claude.test.ts (1)
449-486:⚠️ Potential issue | 🟡 MinorPut the env restore in
finally.This test mutates global
process.envand only cleans it up on the happy path. IfsendQuery()or any assertion starts failing, the leakedCLAUDE_CODE_*/NODE_OPTIONSvalues bleed into later cases and make the suite order-dependent. Wrap the mutation block intry/finallyor move restoration toafterEach.Based on learnings: Applies to **/*.test.ts : Keep tests deterministic — no flaky timing or network dependence without guardrails; ensure local validation commands (
bun run validate) map directly to CI expectations (Determinism + Reproducibility).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/providers/claude.test.ts` around lines 449 - 486, The test "subprocess env keeps user keys but strips nested Claude markers" mutates process.env but only restores it on the happy path; wrap the mutation and test execution (the portion that sets process.env values and calls client.sendQuery / consumes mockQuery) in a try/finally so the cleanup of CUSTOM_USER_KEY, CLAUDE_CODE_ENTRYPOINT, CLAUDE_CODE_OAUTH_TOKEN, and NODE_OPTIONS always runs in the finally block (or alternatively move the restoration logic to an afterEach), ensuring mockQuery and client.sendQuery behavior remains isolated and no env leaks occur across tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/providers/claude.ts`:
- Around line 64-70: sanitizeSubprocessEnv currently does case-sensitive checks
against BLOCKED_SUBPROCESS_ENV_KEYS and CLAUDE_CODE_AUTH_VARS and uses
startsWith, which allows lowercase variants on Windows to bypass filtering;
update sanitizeSubprocessEnv to normalize the environment variable name (e.g.,
const keyUpper = key.toUpperCase()) before testing with
BLOCKED_SUBPROCESS_ENV_KEYS.has(keyUpper) and
keyUpper.startsWith('CLAUDE_CODE_') so Windows-style lowercase keys are properly
filtered, but continue to set sanitized[key] = value using the original key when
not blocked; reference sanitizeSubprocessEnv, BLOCKED_SUBPROCESS_ENV_KEYS and
CLAUDE_CODE_AUTH_VARS.
In `@packages/core/src/utils/env-leak-scanner.ts`:
- Around line 126-139: The current buildRemediationCommand uses a global grep
fallback and collectOffendingKeys that can produce a generic ANTHROPIC_API_KEY
fallback; change it to build per-report.findings entries: iterate
report.findings and for each finding by its finding.file, if finding.keys
contains concrete keys (filter out keys starting with '[') build a file-specific
grep command using escapeRegexForGrep for those keys and include it in the
returned string, and if a finding has no concrete keys (only unreadable markers)
return a permission-oriented remediation message for that file (e.g.,
instructing to restrict file permissions or remove the file from the repo)
instead of a grep command; update or remove the global ANTHROPIC_API_KEY
fallback and ensure collectOffendingKeys is only used where appropriate,
returning a multi-line string that combines each per-file command/message.
---
Outside diff comments:
In `@packages/core/src/providers/claude.test.ts`:
- Around line 449-486: The test "subprocess env keeps user keys but strips
nested Claude markers" mutates process.env but only restores it on the happy
path; wrap the mutation and test execution (the portion that sets process.env
values and calls client.sendQuery / consumes mockQuery) in a try/finally so the
cleanup of CUSTOM_USER_KEY, CLAUDE_CODE_ENTRYPOINT, CLAUDE_CODE_OAUTH_TOKEN, and
NODE_OPTIONS always runs in the finally block (or alternatively move the
restoration logic to an afterEach), ensuring mockQuery and client.sendQuery
behavior remains isolated and no env leaks occur across tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 536d9a8e-982a-45dd-a59b-e0c5e28541c0
📒 Files selected for processing (4)
packages/core/src/providers/claude.test.tspackages/core/src/providers/claude.tspackages/core/src/utils/env-leak-scanner.test.tspackages/core/src/utils/env-leak-scanner.ts
|
Closing — both fixes in this PR are superseded by #1169 (merged into dev). The nested-session marker leak (#1150) is now handled structurally: |
|
Ok！Wish u all the best |
2 participants
Summary
This PR addresses two high-impact env leak issues in Claude provider paths:
CLAUDE_CODE_ENTRYPOINTstill leaks to Claude subprocess env in v0.3.6 — nested-Claude-Code hang reproduces #1150 by hardening subprocess env sanitization so nested Claude markers cannot reappear inoptions.env.EnvLeakErrorsuggested-fix command hardcoded toANTHROPIC_API_KEYregardless of actual offending key #1149 by generatingEnvLeakErrorremediation commands from the actual offending keys instead of hardcodingANTHROPIC_API_KEY.It also aligns with the zero-leak direction discussed in #1135.
Changes
packages/core/src/providers/claude.tssanitizeSubprocessEnv()defense-in-depth filtering.CLAUDECODECLAUDE_CODE_*(except auth vars:CLAUDE_CODE_OAUTH_TOKEN,CLAUDE_CODE_USE_BEDROCK,CLAUDE_CODE_USE_VERTEX)NODE_OPTIONS,VSCODE_INSPECTOR_OPTIONSprocess.envrequestOptions.env(prevents re-injection).packages/core/src/utils/env-leak-scanner.tsformatLeakError()now interpolates offending keys fromreport.findings[].keys.grep -vE '^(GEMINI_API_KEY|OPENAI_API_KEY)=' .env > .env.tmp && mv .env.tmp .envkey/keys).Added regression tests:
packages/core/src/providers/claude.test.tsrequestOptions.envcannot re-injectCLAUDECODE/CLAUDE_CODE_ENTRYPOINTpackages/core/src/utils/env-leak-scanner.test.tsValidation
bun test packages/core/src/providers/claude.test.tsbun test packages/core/src/utils/env-leak-scanner.test.tsBoth pass locally.
Closes #1149
Closes #1150
Refs #1135
Summary by CodeRabbit