feat(security): add private key detection hook #45
Conversation
Add pre-commit hook to detect potential Ethereum and Solana private keys
in staged files before they can be committed.
- Ethereum pattern: (0x)?[A-Fa-f0-9]{64}
- Solana pattern: ^[1-9A-HJ-NP-Za-km-z]{88}$
- Scans .js, .ts, .sol, .py, .sh, .txt, .json files
Closes blockmatic#3
|
@jlaran is attempting to deploy a commit to the Gabo Esquivel Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe PR introduces a new Git hook script to scan staged files for private keys during the pre-commit phase. A new 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
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: 3
🤖 Fix all issues with AI agents
In `@scripts/scan-private-keys.mjs`:
- Around line 14-27: The catch block in getStagedFiles currently swallows
failures and returns an empty array, allowing the hook to silently pass; change
the catch to capture the error and fail the process instead of returning [];
e.g., in getStagedFiles log the caught error and call process.exit(1) (or
rethrow) so any git diff --cached failure causes the script to exit non‑zero and
prevents scanning from being skipped; reference getStagedFiles and EXTENSIONS to
update the error handling accordingly.
- Line 8: The ETH_PATTERN regex is too permissive and matches any 64-hex
sequence inside longer hex strings; update the ETH_PATTERN to require non-hex
boundaries so it only matches standalone 64-character hex sequences (with
optional 0x prefix) and not when preceded or followed by other hex characters.
Specifically, modify the ETH_PATTERN definition to use negative
lookbehind/negative lookahead (or explicit non-hex boundary checks) around the
(0x)?[A-Fa-f0-9]{64} portion so matches are not embedded in longer hex strings;
keep the optional 0x handling and preserve case-insensitivity.
- Around line 3-4: The hook currently reads file contents with readFileSync (and
imports execSync), which inspects the working tree and may miss
staged-but-modified files, and its catch returns false allowing commits on read
errors; replace the readFileSync usage with execFileSync('git', ['show',
`:${filePath}`]) from node:child_process to read the staged blob from the index,
update imports to include execFileSync (remove or stop using execSync if
unused), and change the error handling in the try/catch that reads file contents
to log the error and exit with process.exit(1) (fail closed) instead of
returning false so that any read failure blocks the commit.
🧹 Nitpick comments (1)
scripts/scan-private-keys.mjs (1)
74-79: Use repo logger instead of console in hooks.Line 74-79 uses
console.error, which violates the repo’s logging rules. Replace with@repo/utils/logger.♻️ Suggested change (within this block)
- console.error('\n❌ Error: Detected potential private keys in staged files\n') + logger.error('\n❌ Error: Detected potential private keys in staged files\n') for (const { file, line, type } of detectedFiles) { - console.error(` - ${file}:${line} (${type} private key pattern)`) + logger.error(` - ${file}:${line} (${type} private key pattern)`) } - console.error('\nPlease remove private keys before committing.') - console.error('If this is a false positive, consider adding an allowlist.\n') + logger.error('\nPlease remove private keys before committing.') + logger.error('If this is a false positive, consider adding an allowlist.\n')Add the logger import near the top:
import { logger } from '@repo/utils/logger'As per coding guidelines, "never use console; use
@repo/utils/logger; use@repo/errorfor error handling".
| import { execSync } from 'node:child_process' | ||
| import { readFileSync } from 'node:fs' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/scan-private-keys.mjsRepository: blockmatic/basilic
Length of output: 2768
Read staged content from git index, not the working tree, and fail closed on read errors.
Line 32 reads the working tree using readFileSync, allowing a developer to stage a file containing a private key, then remove it from the working tree before the hook runs—the key remains staged but won't be detected. Similarly, the catch block at lines 53-55 fails open by silently returning false on any read error, allowing commits to proceed when files cannot be checked.
Use execFileSync('git', ['show', :${filePath}]) to read the staged version from the git index instead, and exit with code 1 on read errors to fail closed.
🔧 Suggested fix
-import { execSync } from 'node:child_process'
-import { readFileSync } from 'node:fs'
+import { execFileSync, execSync } from 'node:child_process'
function checkFileForPrivateKeys(filePath) {
try {
- const content = readFileSync(filePath, 'utf-8')
+ const content = execFileSync('git', ['show', `:${filePath}`], {
+ encoding: 'utf-8',
+ stdio: ['pipe', 'pipe', 'pipe'],
+ })
const lines = content.split('\n')
@@
- } catch {
- return { found: false }
- }
+ } catch {
+ exit(1)
+ }
}🤖 Prompt for AI Agents
In `@scripts/scan-private-keys.mjs` around lines 3 - 4, The hook currently reads
file contents with readFileSync (and imports execSync), which inspects the
working tree and may miss staged-but-modified files, and its catch returns false
allowing commits on read errors; replace the readFileSync usage with
execFileSync('git', ['show', `:${filePath}`]) from node:child_process to read
the staged blob from the index, update imports to include execFileSync (remove
or stop using execSync if unused), and change the error handling in the
try/catch that reads file contents to log the error and exit with
process.exit(1) (fail closed) instead of returning false so that any read
failure blocks the commit.
| import { exit } from 'node:process' | ||
|
|
||
| // Define patterns for Ethereum and Solana private keys | ||
| const ETH_PATTERN = /(0x)?[A-Fa-f0-9]{64}/ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/scan-private-keys.mjsRepository: blockmatic/basilic
Length of output: 2768
Tighten the Ethereum regex to reduce false positives with legitimate hex data.
The pattern matches any 64-character hexadecimal sequence, including transaction hashes and other valid 256-bit hex values commonly found in code. This causes false positives that can block legitimate commits. Add negative lookahead and lookbehind to ensure the pattern only matches standalone 64-hex sequences, not those embedded in longer hex strings.
🔧 Suggested fix
-const ETH_PATTERN = /(0x)?[A-Fa-f0-9]{64}/
+const ETH_PATTERN = /(?<![A-Fa-f0-9])(0x)?[A-Fa-f0-9]{64}(?![A-Fa-f0-9])/🤖 Prompt for AI Agents
In `@scripts/scan-private-keys.mjs` at line 8, The ETH_PATTERN regex is too
permissive and matches any 64-hex sequence inside longer hex strings; update the
ETH_PATTERN to require non-hex boundaries so it only matches standalone
64-character hex sequences (with optional 0x prefix) and not when preceded or
followed by other hex characters. Specifically, modify the ETH_PATTERN
definition to use negative lookbehind/negative lookahead (or explicit non-hex
boundary checks) around the (0x)?[A-Fa-f0-9]{64} portion so matches are not
embedded in longer hex strings; keep the optional 0x handling and preserve
case-insensitivity.
| function getStagedFiles() { | ||
| try { | ||
| const output = execSync('git diff --cached --name-only --diff-filter=ACM', { | ||
| encoding: 'utf-8', | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }) | ||
| return output | ||
| .split('\n') | ||
| .map(line => line.trim()) | ||
| .filter(line => line.length > 0) | ||
| .filter(line => EXTENSIONS.test(line)) | ||
| } catch { | ||
| return [] | ||
| } |
There was a problem hiding this comment.
Fail closed when staged-file listing fails.
On Line 25-26, returning an empty list lets the hook exit 0 and skip scanning if git diff fails. For a security gate, prefer exiting non‑zero to avoid a silent bypass.
🔧 Suggested fix
- } catch {
- return []
- }
+ } catch {
+ exit(1)
+ }🤖 Prompt for AI Agents
In `@scripts/scan-private-keys.mjs` around lines 14 - 27, The catch block in
getStagedFiles currently swallows failures and returns an empty array, allowing
the hook to silently pass; change the catch to capture the error and fail the
process instead of returning []; e.g., in getStagedFiles log the caught error
and call process.exit(1) (or rethrow) so any git diff --cached failure causes
the script to exit non‑zero and prevents scanning from being skipped; reference
getStagedFiles and EXTENSIONS to update the error handling accordingly.
1 participant
Summary
Adds a pre-commit hook to detect potential Ethereum and Solana private keys in staged files before they can be committed.
(0x)?[A-Fa-f0-9]{64}(64 hex chars with optional 0x prefix)^[1-9A-HJ-NP-Za-km-z]{88}$(88 base58 chars).js,.ts,.sol,.py,.sh,.txt,.jsonChanges
scripts/scan-private-keys.mjs- Node.js script that scans staged fileshooks:securitypipeline inpackage.jsonsecurity:private-keysscript for manual runsTest plan
Closes #3
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.