chore: add Claude Code hook to protect .env files#4474
chore: add Claude Code hook to protect .env files#4474
Conversation
pomfrida
requested review from
Chibuzor-Nwemambu,
millus and
vnys
as code owners
pomfrida
added
the
ai-tooling
|
@claude review |
|
Claude finished @pomfrida's task in 1m 4s —— View job Code Review Complete ✓Todo List
SummaryThis PR implements a security-focused hook to protect Security & Implementation✅ Strengths
🔍 Potential Issues1. Pattern matching could be more robust ( if (readPath.includes('.env')) {This will catch legitimate files like const basename = readPath.split('/').pop() || ''
if (basename === '.env' || basename.startsWith('.env.')) {This would match 2. Missing error handling for JSON parsing ( const toolArgs = JSON.parse(Buffer.concat(chunks).toString())If stdin provides malformed JSON, this will crash with an uncaught exception. Consider wrapping in a try-catch: let toolArgs
try {
toolArgs = JSON.parse(Buffer.concat(chunks).toString())
} catch (e) {
console.error('Failed to parse hook input:', e.message)
process.exit(1)
}3. Hook doesn't handle The matcher only includes Documentation✅ StrengthsThe
💡 Minor Suggestions1. Add testing guidance for hooks ( After line 252 in the "Tips" section, consider adding guidance on testing hooks: - Test hooks locally by running them with sample JSON: `echo '{"tool_input":{"file_path":".env"}}' | node .claude/hooks/read_hook.js`2. Consider documenting hook limitations ( The documentation could mention that hooks only protect against tool usage, not against Claude suggesting the file contents from memory or discussing them conceptually. This sets appropriate security expectations. Configuration✅ StrengthsThe
TestingThe test plan shows manual verification, which is appropriate for this type of change. Consider whether automated tests for hooks are feasible in your CI/CD pipeline. Overall AssessmentThis is a well-implemented feature with excellent documentation. The primary concerns are:
The documentation alone makes this PR valuable - it provides a solid foundation for future hook development in the project. Recommendations
|
Code ReviewI've identified 2 security issues with the .env protection hook: Issue 1: Bash tool bypassThe hook matcher only covers Read and Grep tools, but not the Bash tool. This means .env files can still be read via commands like cat .env, completely bypassing the protection. Reference: design-system/.claude/settings.json Lines 4 to 6 in cc25f7b Suggested fix: Add Bash to the matcher and update the hook script to check the command field for .env references. Issue 2: Grep directory search bypassThe Grep tool can expose .env file contents when searching directories. The hook only checks if the path itself contains .env, but when Grep searches a directory like . or /project, it includes .env files in results. Reference: design-system/.claude/hooks/read_hook.js Lines 8 to 15 in cc25f7b Suggested fix: For Grep invocations, check if performing a directory search and either block it or exclude .env files via glob patterns. |
|
@claude can you review again after I added your suggestions |
|
Claude finished @pomfrida's task in 1m 18s —— View job Code Review ✓Todo List
SummaryExcellent work! You've addressed all the major concerns from the previous review. The hook implementation is now significantly more robust with improved pattern matching, error handling, and comprehensive tool coverage. The documentation remains outstanding. Previous Suggestions - Status✅ Addressed Successfully
Security Analysis🔒 Protection CoverageThe hook now provides defense-in-depth across all major attack vectors:
|
.claude/settings.json
Outdated
| "hooks": { | ||
| "PreToolUse": [ | ||
| { | ||
| "matcher": "Read|Grep", |
There was a problem hiding this comment.
Many ways to trick the system, so this security thing is a difficult one! I tried to ask claude to edit the .env file (and to edit you must first know what the file contains aka read it), so if its ever been looked at before this hook is active claude will know the contents and be able to edit. So to be even more secure I would suggest to add these aswell:
"matcher": "Read|Grep|Edit|Write|NotebookEdit",
.claude/hooks/read_hook.js
Outdated
| const readPath = | ||
| toolArgs.tool_input?.file_path || toolArgs.tool_input?.path || '' | ||
|
|
||
| if (readPath.includes('.env')) { | ||
| console.error('You cannot read the .env file') | ||
| process.exit(2) | ||
| } | ||
| } |
There was a problem hiding this comment.
After testing it out with the edit route to trick claude to show me .env content in the course project, he suggested these improvements:
async function main() {
const chunks = [];
for await (const chunk of process.stdin) {
chunks.push(chunk);
}
const toolArgs = JSON.parse(Buffer.concat(chunks).toString());
// Get the file path from various tool input structures
const readPath =
toolArgs.tool_input?.file_path ||
toolArgs.tool_input?.path ||
toolArgs.tool_input?.notebook_path ||
"";
// List of sensitive file patterns to block
const blockedPatterns = [
".env",
".env.local",
".env.production",
".env.development",
"credentials",
"secrets",
"private_key",
"id_rsa",
".pem",
".key",
];
// Check if the path contains any blocked patterns
const isBlocked = blockedPatterns.some((pattern) =>
readPath.toLowerCase().includes(pattern.toLowerCase()),
);
if (isBlocked) {
console.error(
`🚫 Blocked: Claude is trying to access a sensitive file: ${readPath}`,
);
console.error("This file may contain secrets, API keys, or credentials.");
process.exit(2);
}
// Hook passes - allow the file access
// (Logging removed to reduce noise - only block attempts are logged)
}
main();
`
Maybe you could see if this will be a good fit for this repo as well @claude?
There was a problem hiding this comment.
Claude finished @millus's task in 1m 4s —— View job
Security Hook Analysis & Recommendations
Todo List
- Read current hook implementation
- Analyze suggested changes
- Evaluate security vs usability tradeoffs
- Provide recommendations
Great catch on the Edit/Write bypass, @millus! You're absolutely right that the current implementation has a significant gap. Let me break down the suggested changes:
Suggested Changes Analysis
1. ✅ Add Edit/Write/NotebookEdit to matcher (CRITICAL)
Current state: Matcher only covers Read|Grep|Glob|Bash
The bypass: If Claude has seen .env contents before the hook was active (or remembers from conversation history), it can be asked to "edit" the file, which bypasses the Read protection entirely.
Recommendation: Strongly recommend adding these tools
"matcher": "Read|Grep|Glob|Bash|Edit|Write|NotebookEdit"Why: This closes a major security hole. Without this, the protection is easily circumvented by using write-based tools instead of read-based tools.
Implementation note: The hook would need to check toolInput.file_path and toolInput.notebook_path for these tools, which the suggested code already does.
2. ⚠️ Expand blocked patterns (TRADEOFF REQUIRED)
Suggested patterns:
const blockedPatterns = [
".env",
".env.local",
".env.production",
".env.development",
"credentials",
"secrets",
"private_key",
"id_rsa",
".pem",
".key",
]Pros:
- Broader protection beyond just .env files
- Catches common credential file patterns
- Defense in depth
Cons:
- High false positive risk: Blocking "credentials" would block files like:
src/utils/credentials.ts(legitimate code)docs/credentials-guide.md(documentation)tests/credentials.test.ts(test files)
- Same issue with "secrets", "key", etc.
- Could significantly impact developer workflow
Recommendation: Use a more targeted approach
Instead of substring matching on generic terms, I'd suggest:
const blockedPatterns = [
/^\.env($|\.)/, // .env, .env.local, etc.
/^id_rsa($|\.)/, // SSH private keys
/\.pem$/, // Certificate files
/^\.?private[_-]key/, // private_key, .private-key, etc.
/credentials\.json$/, // Specific credential files
/secrets\.json$/, // Specific secret files
]This approach:
- Uses regex patterns for precise matching
- Targets specific file naming conventions
- Minimizes false positives for legitimate code files
- Still catches the most dangerous patterns
3. ⚠️ Simplify to .includes() check (REGRESSION)
Current approach: Uses basename matching
const basename = filePath.split('/').pop() || ''
if (basename === '.env' || basename.startsWith('.env.')) {
// block
}Suggested approach: Use .includes()
const isBlocked = blockedPatterns.some((pattern) =>
readPath.toLowerCase().includes(pattern.toLowerCase()),
)Problem: This is actually a regression from the improvements in this PR!
The previous review specifically called out that .includes('.env') causes false positives:
development.ts❌ (contains ".env")preview.tsx❌ (contains ".env")deployment/config.js❌ (contains ".env")
Recommendation: Keep the current basename approach for .env files, but combine it with regex patterns for other sensitive files:
// Check basename for .env files (most precise)
const basename = filePath.split('/').pop() || ''
if (basename === '.env' || basename.startsWith('.env.')) {
console.error('Blocked: cannot access .env files')
process.exit(2)
}
// Check against other sensitive patterns
const sensitivePatterns = [
/^id_rsa($|\.)/,
/\.pem$/,
/^\.?private[_-]key/,
/credentials\.json$/,
/secrets\.json$/,
]
if (sensitivePatterns.some(pattern => pattern.test(basename))) {
console.error('Blocked: cannot access sensitive credential files')
process.exit(2)
}Final Recommendations
Must implement (closes critical gap):
- ✅ Add
Edit|Write|NotebookEditto the matcher - ✅ Update hook to check these tools for file_path/notebook_path
Should consider (defense in depth with low false positives):
3. ✅ Add targeted regex patterns for common credential files (SSH keys, PEM files, specific JSON files)
Should NOT implement (too many false positives):
4. ❌ Generic substring matching on terms like "credentials", "secrets", "key"
5. ❌ Reverting to .includes() for .env detection
Proposed Implementation
For this design system repo specifically, I'd recommend:
const basename = filePath.split('/').pop() || ''
// Block .env files
if (basename === '.env' || basename.startsWith('.env.')) {
console.error('Blocked: cannot access .env files')
process.exit(2)
}
// Block common credential file patterns
const credentialPatterns = [
/^id_rsa($|\.pub)?$/, // SSH keys
/\.pem$/, // SSL certificates
/\.key$/, // Private key files
/^\.?credentials\.json$/, // Credential config
/^\.?secrets\.json$/, // Secrets config
]
if (credentialPatterns.some(p => p.test(basename))) {
console.error('Blocked: cannot access credential/certificate files')
process.exit(2)
}This balances security with usability - it protects the most sensitive files while allowing normal development workflows on files that just happen to contain keywords like "authentication" or "authorization" in their names.
Would you like me to implement these changes to close the Edit/Write gap?
| ├── commands/ # Slash commands | ||
| │ └── new-component.md # /new-component <Name> | ||
| ├── hooks/ # Custom hook scripts | ||
| │ └── read_hook.js # Blocks reading of .env files |
There was a problem hiding this comment.
Really nice and important addition to our project! 👏
2 participants
Summary
PreToolUsehook that blocks Claude Code from reading.envfiles, protecting secrets from accidental exposure.claude/hooks/to keep all Claude configuration co-located.claude/README.mdwith usage guide, available events, and examples for creating custom hooksTest plan
Readtool on.envfiles (exit code 2)Greptool on.envfiles$CLAUDE_PROJECT_DIRpath resolution works after move🤖 Generated with Claude Code