Fix claude code entrypoint leak#1165
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a4e017907
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const grepCommand = keysArray | ||
| .map(key => `grep -v '^${key}='`) | ||
| .join(' | ') + ' .env > .env.tmp && mv .env.tmp .env'; |
There was a problem hiding this comment.
When more than one sensitive key is detected, this formatter emits a command like grep -v '^KEY1=' | grep -v '^KEY2=' .env > .env.tmp ...; in that form, the first grep has no file/stdin source and waits on terminal input, so the recommended remediation hangs instead of editing .env. This only appears in multi-key findings (for example .env containing both OPENAI_API_KEY and GEMINI_API_KEY), which is a common case for this gate.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughThis PR adds Windows symlink setup documentation, enhances environment leak detection by dynamically aggregating sensitive keys into shell remediation commands, and filters Claude-related environment variables to prevent unintended propagation during subprocess execution. Changes
Possibly related issues
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 1
🤖 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/utils/env-leak-scanner.ts`:
- Around line 135-146: The generated grep pipeline is invalid for multiple keys
and may include unsafe patterns; update the code that builds grepCommand (using
keysArray/allKeys/report.findings) to first filter keys to only valid env
variable names (e.g. /^[A-Za-z_][A-Za-z0-9_]*$/), skip if none remain, escape
regex metacharacters for each key (add a small escapeRegex helper), and then
build a safe pipeline that starts with "cat .env" and pipes through each grep -v
like: "cat .env | grep -v '^ESCAPED_KEY=' | grep -v '^OTHER_KEY=' > .env.tmp &&
mv .env.tmp .env" so each grep operates on the stream and the command is valid
for multiple keys.
🪄 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: 25c4fd09-78cd-418b-a229-03df79c376cc
📒 Files selected for processing (3)
README.mdpackages/core/src/utils/env-leak-scanner.tspackages/providers/src/claude/provider.ts
| // Collect all unique keys from findings | ||
| const allKeys = new Set<string>(); | ||
| report.findings.forEach(finding => { | ||
| finding.keys.forEach(key => allKeys.add(key)); | ||
| }); | ||
| const keysArray = Array.from(allKeys); | ||
|
|
||
| // Generate grep command that excludes all detected keys | ||
| const grepCommand = keysArray | ||
| .map(key => `grep -v '^${key}='`) | ||
| .join(' | ') + ' .env > .env.tmp && mv .env.tmp .env'; | ||
|
|
There was a problem hiding this comment.
Fix generated remediation command for multiple keys.
Line 143-145 builds an invalid pipeline for >1 key (only the last grep gets .env). It also includes non-env placeholders (e.g. unreadable markers), which can generate broken regex patterns.
💡 Proposed fix
- // Collect all unique keys from findings
- const allKeys = new Set<string>();
- report.findings.forEach(finding => {
- finding.keys.forEach(key => allKeys.add(key));
- });
- const keysArray = Array.from(allKeys);
-
- // Generate grep command that excludes all detected keys
- const grepCommand = keysArray
- .map(key => `grep -v '^${key}='`)
- .join(' | ') + ' .env > .env.tmp && mv .env.tmp .env';
+ // Collect unique concrete env var keys only (skip diagnostic placeholders)
+ const keysArray = Array.from(new Set(report.findings.flatMap(f => f.keys))).filter(key =>
+ /^[A-Z_][A-Z0-9_]*$/.test(key)
+ );
+
+ const escapedKeys = keysArray.map(key => key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'));
+ const grepCommand =
+ escapedKeys.length > 0
+ ? `grep -v -E '^(${escapedKeys.join('|')})=' .env > .env.tmp && mv .env.tmp .env`
+ : '# No concrete key names could be derived; remove keys manually from .env';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Collect all unique keys from findings | |
| const allKeys = new Set<string>(); | |
| report.findings.forEach(finding => { | |
| finding.keys.forEach(key => allKeys.add(key)); | |
| }); | |
| const keysArray = Array.from(allKeys); | |
| // Generate grep command that excludes all detected keys | |
| const grepCommand = keysArray | |
| .map(key => `grep -v '^${key}='`) | |
| .join(' | ') + ' .env > .env.tmp && mv .env.tmp .env'; | |
| // Collect unique concrete env var keys only (skip diagnostic placeholders) | |
| const keysArray = Array.from(new Set(report.findings.flatMap(f => f.keys))).filter(key => | |
| /^[A-Z_][A-Z0-9_]*$/.test(key) | |
| ); | |
| const escapedKeys = keysArray.map(key => key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')); | |
| const grepCommand = | |
| escapedKeys.length > 0 | |
| ? `grep -v -E '^(${escapedKeys.join('|')})=' .env > .env.tmp && mv .env.tmp .env` | |
| : '# No concrete key names could be derived; remove keys manually from .env'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/utils/env-leak-scanner.ts` around lines 135 - 146, The
generated grep pipeline is invalid for multiple keys and may include unsafe
patterns; update the code that builds grepCommand (using
keysArray/allKeys/report.findings) to first filter keys to only valid env
variable names (e.g. /^[A-Za-z_][A-Za-z0-9_]*$/), skip if none remain, escape
regex metacharacters for each key (add a small escapeRegex helper), and then
build a safe pipeline that starts with "cat .env" and pipes through each grep -v
like: "cat .env | grep -v '^ESCAPED_KEY=' | grep -v '^OTHER_KEY=' > .env.tmp &&
mv .env.tmp .env" so each grep operates on the stream and the command is valid
for multiple keys.
|
Closing — #1150 (the underlying issue) is now fixed structurally in #1169 (merged into dev) via |
3 participants
Summary
Describe this PR in 2-5 bullets:
CLAUDE_CODE_ENTRYPOINTstill leaks to Claude subprocess env in v0.3.6 — nested-Claude-Code hang reproduces #1150 and provides an extra layer of protection even if stripCwdEnv() is bypassed or platform-specific env inheritance adds markers back## SummaryDescribe this PR in 2-5 bullets:
CLAUDE_CODE_ENTRYPOINTstill leaks to Claude subprocess env in v0.3.6 — nested-Claude-Code hang reproduces #1150 and provides an extra layer of protection even if stripCwdEnv() is bypassed or platform-specific env inheritance adds markers back## SummaryDescribe this PR in 2-5 bullets:
UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory (list every module-to-module edge, mark changes):
Label Snapshot
risk: low|medium|highsize: XS|S|M|L|XLcore|workflows|isolation|git|adapters|server|web|cli|paths|config|docs|dependencies|ci|tests|skills<scope>:<component>(e.g.workflows:executor,adapters:slack,core:orchestrator)Change Metadata
bug|feature|refactor|docs|security|chorecore|workflows|isolation|git|adapters|server|web|cli|paths|multiLinked Issue
Validation Evidence (required)
Commands and result summary:
Security Impact (required)
Yes/No)Yes/No)Yes/No)Yes/No)Yes, describe risk and mitigation:Compatibility / Migration
Yes/No)Yes/No)Yes/No)Human Verification (required)
What was personally validated beyond CI:
Side Effects / Blast Radius (required)
Rollback Plan (required)
Risks and Mitigations
List real risks in this PR (or write
None).Summary by CodeRabbit
Documentation
Bug Fixes