fix: block-env-files and protect-env-vars false positives on quoted content#50
fix: block-env-files and protect-env-vars false positives on quoted content#50yashexosphere wants to merge 1 commit into
Conversation
…ocks - Add stripQuotedStrings() to ignore .env/.export inside quoted args - Add negative lookahead to ENV_CMD_RE for .env.example/.env.template - Add ENV_SAFE_SUFFIX_RE for file path checks on safe template files - Add comprehensive true/false positive tests for both policies
📝 WalkthroughWalkthroughThis PR fixes false positives in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 docstrings
🧪 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hooks/builtin-policies.ts`:
- Around line 97-101: ENV_SAFE_SUFFIX_RE / ENV_FILE_PATH_RE / ENV_CMD_RE
currently allow safe substrings anywhere in the path and miss case-insensitive
matches; change the logic to extract the file basename (last path segment),
normalize it to lowercase, and compare it exactly against the whitelist of
allowed template basenames (e.g. ".env", ".env.example", ".env.template",
".env.sample", ".env.dist", ".env.test") instead of using the existing regexes
to match anywhere; update the checks that use ENV_SAFE_SUFFIX_RE,
ENV_FILE_PATH_RE and ENV_CMD_RE (including the other occurrence around the
367-373 area) to use this basename-based, case-insensitive exact comparison so
paths like "/tmp/.env.example/.env" or ".env.example.local" no longer bypass the
policy.
- Around line 114-115: stripQuotedStrings currently replaces entire quoted spans
with empty quotes which removes semantically active shell syntax and permits
real violations to bypass policy checks; change stripQuotedStrings to perform
context-aware masking (e.g., preserve or mark shell-expanding constructs like
$VAR, command substitutions $(...), backticks, and prefixes like bash -c inside
quoted strings) rather than blanking every quoted span so that downstream
matchers still see dangerous patterns, and update any other quote-stripping
logic in this file to the same approach; add regression tests that include
quoted true-positive cases such as cat ".env", source '.env', echo "$HOME", and
bash -lc "cat .env" to ensure these are detected.
🪄 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: f158ba6c-128e-4879-85dc-eb2d990fb45b
📒 Files selected for processing (2)
__tests__/hooks/builtin-policies.test.tssrc/hooks/builtin-policies.ts
| // Allow .env.example / .env.template / .env.sample / .env.dist — these are | ||
| // committed template files with no real secrets. Block everything else. | ||
| const ENV_SAFE_SUFFIX_RE = /\.(?:example|template|sample|dist|test)(?:[./\\]|$)/i; | ||
| const ENV_FILE_PATH_RE = /(?:^|[\\/])\.env(?:\.|$)/; | ||
| const ENV_CMD_RE = /\.env(?:\b|\s|$|\.)/; | ||
| const ENV_CMD_RE = /\.env(?!\.(?:example|template|sample|dist|test)\b)(?:\b|\s|$|\.)/i; |
There was a problem hiding this comment.
Match the exact safe .env basename instead of regexing the whole path.
Right now a safe-looking substring anywhere in the path suppresses the block, so /tmp/.env.example/.env is allowed, and extra-extension names like .env.example.local also slip through. ENV_FILE_PATH_RE is also case-sensitive, so .ENV/.Env bypass Read/Write on case-insensitive filesystems. Please normalize to the matched basename and compare it against the exact allowed template names.
Also applies to: 367-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/builtin-policies.ts` around lines 97 - 101, ENV_SAFE_SUFFIX_RE /
ENV_FILE_PATH_RE / ENV_CMD_RE currently allow safe substrings anywhere in the
path and miss case-insensitive matches; change the logic to extract the file
basename (last path segment), normalize it to lowercase, and compare it exactly
against the whitelist of allowed template basenames (e.g. ".env",
".env.example", ".env.template", ".env.sample", ".env.dist", ".env.test")
instead of using the existing regexes to match anywhere; update the checks that
use ENV_SAFE_SUFFIX_RE, ENV_FILE_PATH_RE and ENV_CMD_RE (including the other
occurrence around the 367-373 area) to use this basename-based, case-insensitive
exact comparison so paths like "/tmp/.env.example/.env" or ".env.example.local"
no longer bypass the policy.
| function stripQuotedStrings(cmd: string): string { | ||
| return cmd.replace(/"[^"]*"|'[^']*'|`[^`]*`/g, '""'); |
There was a problem hiding this comment.
This quote stripping creates a security bypass, not just a false-positive fix.
Because the stripped string is what both policies inspect, semantically active shell syntax disappears before matching. cat ".env", source '.env', echo "$HOME", and bash -lc "cat .env" all become invisible here, so real .env reads and env-var disclosure are now allowed with ordinary quoting. Please make the suppression context-aware instead of blanking every quoted span, and add regression tests for quoted true-positive cases.
Also applies to: 331-339, 373-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hooks/builtin-policies.ts` around lines 114 - 115, stripQuotedStrings
currently replaces entire quoted spans with empty quotes which removes
semantically active shell syntax and permits real violations to bypass policy
checks; change stripQuotedStrings to perform context-aware masking (e.g.,
preserve or mark shell-expanding constructs like $VAR, command substitutions
$(...), backticks, and prefixes like bash -c inside quoted strings) rather than
blanking every quoted span so that downstream matchers still see dangerous
patterns, and update any other quote-stripping logic in this file to the same
approach; add regression tests that include quoted true-positive cases such as
cat ".env", source '.env', echo "$HOME", and bash -lc "cat .env" to ensure these
are detected.
|
we should debate about this and figure out a better strategy for this, current strategy in the PR will allow claude to read .env in say, sub folders. |
2 participants
Summary
blockEnvFilesandprotectEnvVars.env.example,.env.template,.env.sample(committed templates, no real secrets)Closes #49
Test plan
bun run test:run— all 766 tests passgh pr create --body "don't commit .env"— no longer blockedgit commit -m "export PATH changes"— no longer blockedcat .env— still blockedexport API_KEY=abc— still blocked🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests