fix: the application stores api keys in plaintext js... in cli.js#214
fix: the application stores api keys in plaintext js... in cli.js#214anupamme wants to merge 1 commit into
Conversation
Automated security fix generated by OrbisAI Security
📝 WalkthroughWalkthroughAuth file handling now encrypts stored values with an AES-256-CBC key derived from the auth file path, decrypts existing values when updating, and writes the resulting JSON with restrictive permissions. ChangesAuth file encryption
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli.js`:
- Around line 906-908: Update the auth-data parsing and decryption flow around
the catch block to validate that the parsed value is an object and every auth
value is a decryptable string, rejecting malformed or non-string values instead
of preserving them. On any parse or decryption failure, abort before the later
write that replaces the file, leaving existing auth data unchanged.
- Line 913: Update the auth-file write flow around AUTH_FILE so existing files
are also enforced as mode 0600. Either atomically replace the file using a newly
created 0600 file or explicitly chmod the existing file after writing, while
preserving the current JSON output behavior.
- Around line 895-897: Replace the path-derived _authKey encryption in
encryptAuthValue and decryptAuthValue with a real secret sourced from the
established secure key store, or remove this custom credential encryption in
favor of secure storage. Use authenticated encryption such as AES-GCM, persist
the nonce and authentication tag with ciphertext, and make decryptAuthValue
reject authentication failures instead of returning malformed ciphertext as
plaintext.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a0a3ee3d-4923-4f17-af33-93d9d53ae8ea
📒 Files selected for processing (1)
cli.js
📜 Review details
🧰 Additional context used
🪛 ast-grep (0.44.1)
cli.js
[warning] 902-902: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(AUTH_FILE, 'utf-8')
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
[warning] 912-912: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.writeFileSync(AUTH_FILE, JSON.stringify(out, null, 2), { encoding: 'utf-8', mode: 0o600 })
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename)
| const _authKey = crypto.createHash('sha256').update(AUTH_FILE).digest(); | ||
| function encryptAuthValue(v) { const iv = crypto.randomBytes(16); const c = crypto.createCipheriv('aes-256-cbc', _authKey, iv); return 'enc:' + iv.toString('hex') + ':' + Buffer.concat([c.update(v, 'utf-8'), c.final()]).toString('hex'); } | ||
| function decryptAuthValue(v) { if (!v || !v.startsWith('enc:')) return v; try { const [, h, d] = v.split(':'); const dc = crypto.createDecipheriv('aes-256-cbc', _authKey, Buffer.from(h, 'hex')); return Buffer.concat([dc.update(Buffer.from(d, 'hex')), dc.final()]).toString('utf-8'); } catch (e) { return v; } } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,80p'
echo "== locate cli.js =="
fd -a '^cli\.js$' . || true
echo "== relevant cli.js lines =="
if [ -f cli.js ]; then
sed -n '840,930p' cli.js | cat -n
fi
echo "== AUTH_FILE references =="
rg -n "AUTH_FILE|auth.json|authJson|encryptAuthValue|decryptAuthValue|createCipheriv|createDecipheriv|crypto" cli.js || true
echo "== git diff stat/name =="
git diff --stat || true
git diff -- cli.js | sed -n '1,220p' || trueRepository: SakuraByteCore/codexmate
Length of output: 9346
Do not derive the encryption key from AUTH_FILE.
sha256(AUTH_FILE) is predictable; anyone who can read the known ~/.codex/auth.json path can derive the same key and decrypt the stored API key. Use a real secret/key store for this protection, or avoid encrypting credentials with path-derived material. Also prefer authenticated encryption (for example AES-GCM) and reject failed authentication tags rather than falling back to treating malformed ciphertext as plaintext.
[high_effort_and_high_review]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli.js` around lines 895 - 897, Replace the path-derived _authKey encryption
in encryptAuthValue and decryptAuthValue with a real secret sourced from the
established secure key store, or remove this custom credential encryption in
favor of secure storage. Use authenticated encryption such as AES-GCM, persist
the nonce and authentication tag with ciphertext, and make decryptAuthValue
reject authentication failures instead of returning malformed ciphertext as
plaintext.
| for (const k of Object.keys(raw)) authData[k] = decryptAuthValue(raw[k]); | ||
| } | ||
| } catch (e) { } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed instead of overwriting unreadable auth data.
If parsing or decrypting any value fails—including a valid JSON value that is not a string—the empty catch leaves authData empty, and lines 910-912 silently replace the file with only the new API key. Validate the object/value schema and abort without writing on failure; malformed enc: values should be rejected, not preserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli.js` around lines 906 - 908, Update the auth-data parsing and decryption
flow around the catch block to validate that the parsed value is an object and
every auth value is a decryptable string, rejecting malformed or non-string
values instead of preserving them. On any parse or decryption failure, abort
before the later write that replaces the file, leaving existing auth data
unchanged.
| fs.writeFileSync(AUTH_FILE, JSON.stringify(authData, null, 2), { encoding: 'utf-8', mode: 0o600 }); | ||
| const out = {}; | ||
| for (const k of Object.keys(authData)) out[k] = encryptAuthValue(authData[k]); | ||
| fs.writeFileSync(AUTH_FILE, JSON.stringify(out, null, 2), { encoding: 'utf-8', mode: 0o600 }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce 0600 for existing auth files too.
fs.writeFileSync(..., { mode: 0o600 }) only applies when creating a file; it does not change the mode of an existing file. A pre-existing permissive auth file remains readable after this update. Use an atomic replacement that writes into a newly created 0600 file, or explicitly chmod the existing file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cli.js` at line 913, Update the auth-file write flow around AUTH_FILE so
existing files are also enforced as mode 0600. Either atomically replace the
file using a newly created 0600 file or explicitly chmod the existing file after
writing, while preserving the current JSON output behavior.
Summary
Fix critical severity security issue in
cli.js.Vulnerability
V-001cli.js:895Description: The application stores API keys in plaintext JSON files in the user's home directory. While file permissions are restricted, the keys are not encrypted at rest, making them vulnerable to extraction.
Evidence
Exploitation scenario: An attacker with read access to the user's home directory (via malware, shared environment, or backup exposure) can read ~/.codex/auth.json and extract plaintext API keys.
Scanner confirmation: multi_agent_ai rule
V-001flagged this pattern.Production code: This file is in the production codebase, not test-only code.
Threat Model Context
This is a Node.js library - vulnerabilities affect downstream consumers who use this package.
Changes
cli.jsBehavior Preservation
The change is scoped to 1 file on the vulnerable path, and the project's existing tests still pass, so intended behavior is unchanged.
Verification
Security Invariant
Regression test
This test guards against regressions — it's useful independent of the code change above.
Automated security fix by OrbisAI Security
Summary by CodeRabbit