Harden CLI credential and login secret writes#537
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThree files modified to enhance filesystem security. New module Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 9261003aca
ℹ️ 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".
src/login.ts
Outdated
| await sendEvent(apikey, { | ||
| channel: 'user-login', | ||
| event: 'User CLI login', | ||
| icon: '✅', | ||
| user_id: userId, | ||
| notify: false, | ||
| }).catch() |
There was a problem hiding this comment.
loginInternal now calls sendEvent before validating --local repository constraints and before writing .capgo, so failed logins still produce a "User CLI login" event. For example, capgo login --local outside a git repo throws Not in a git repository but this path already reported a successful login, which can skew analytics and trigger downstream workflows on failed attempts.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/utils/safeWrites.ts (3)
1-3: Minor: Fix import ordering per linter rules.ESLint flags import ordering issues. The
appendFileshould come beforewriteFile, andnode:cryptoshould come beforenode:path.🧹 Proposed fix for import ordering
-import { chmod, lstat, mkdir, readFile, rename, rm, writeFile, appendFile } from 'node:fs/promises' -import { dirname, join } from 'node:path' -import { randomBytes } from 'node:crypto' +import { appendFile, chmod, lstat, mkdir, readFile, rename, rm, writeFile } from 'node:fs/promises' +import { randomBytes } from 'node:crypto' +import { dirname, join } from 'node:path'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/safeWrites.ts` around lines 1 - 3, The import statements are out of order per ESLint rules: place node:crypto imports before node:path and ensure appendFile is imported before writeFile; update the import line to import randomBytes from 'node:crypto' before importing dirname and join from 'node:path', and reorder the fs/promises named imports so appendFile appears before writeFile (keeping other names—chmod, lstat, mkdir, readFile, rename, rm—in their existing group). Locate the imports at the top of src/utils/safeWrites.ts (references: appendFile, writeFile, randomBytes, dirname, join) and reorder them accordingly to satisfy the linter.
37-50: Consider adding symlink check for temp file path.The temp file path itself isn't checked for symlinks before writing. While the random 8-byte suffix makes collision extremely unlikely (~2^64 possibilities), for maximum defense-in-depth, consider guarding the temp path as well.
Additionally,
dirname(filePath)could resolve through a symlink. If strict symlink avoidance is required for the entire path, consider validating the parent directory too.🛡️ Optional: Add temp path symlink check
const tempPath = join(dirname(filePath), `.capgo-tmp-${randomBytes(8).toString('hex')}`) try { + await ensureNotSymlink(tempPath) await writeFile(tempPath, content, { encoding: options.encoding ?? 'utf-8', mode }) await rename(tempPath, filePath) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/safeWrites.ts` around lines 37 - 50, The writeFileAtomic function currently calls ensureNotSymlink(filePath) but doesn't validate the temp file or parent dir; update writeFileAtomic to also validate the target directory and the generated tempPath by calling ensureNotSymlink on dirname(filePath) (to avoid a symlinked parent) and on tempPath before writeFile is invoked, so both the parent directory and the temporary file path are checked for symlinks prior to writing/renaming; keep the existing cleanup/rename logic and error handling as-is.
10-21: Known limitation: TOCTOU race between symlink check and file operation.There's a time-of-check to time-of-use (TOCTOU) window between
ensureNotSymlink()and the subsequent file operation. An attacker could theoretically create a symlink after the check passes but before the write occurs.For this CLI's threat model (local user credentials), the risk is low since:
- The attacker would need local access during the brief window
writeFileAtomicuses a random temp filename, making the temp path unpredictableConsider documenting this limitation. For stricter hardening, using
O_NOFOLLOWflags viafs.open()would eliminate the race, but Node.js's high-level APIs don't expose this easily.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/safeWrites.ts` around lines 10 - 21, Add a short comment above ensureNotSymlink (and/or at top of src/utils/safeWrites.ts) documenting the TOCTOU race: explain that a symlink could be created after the lstat check and before a subsequent write, note why the risk is low for this CLI (local attacker, random temp filenames used by writeFileAtomic), and mention that full hardening would require using O_NOFOLLOW via fs.open or similar lower-level APIs; optionally add a TODO linking to a ticket to implement O_NOFOLLOW-based writes for stricter security.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/safeWrites.ts`:
- Around line 1-3: The import statements are out of order per ESLint rules:
place node:crypto imports before node:path and ensure appendFile is imported
before writeFile; update the import line to import randomBytes from
'node:crypto' before importing dirname and join from 'node:path', and reorder
the fs/promises named imports so appendFile appears before writeFile (keeping
other names—chmod, lstat, mkdir, readFile, rename, rm—in their existing group).
Locate the imports at the top of src/utils/safeWrites.ts (references:
appendFile, writeFile, randomBytes, dirname, join) and reorder them accordingly
to satisfy the linter.
- Around line 37-50: The writeFileAtomic function currently calls
ensureNotSymlink(filePath) but doesn't validate the temp file or parent dir;
update writeFileAtomic to also validate the target directory and the generated
tempPath by calling ensureNotSymlink on dirname(filePath) (to avoid a symlinked
parent) and on tempPath before writeFile is invoked, so both the parent
directory and the temporary file path are checked for symlinks prior to
writing/renaming; keep the existing cleanup/rename logic and error handling
as-is.
- Around line 10-21: Add a short comment above ensureNotSymlink (and/or at top
of src/utils/safeWrites.ts) documenting the TOCTOU race: explain that a symlink
could be created after the lstat check and before a subsequent write, note why
the risk is low for this CLI (local attacker, random temp filenames used by
writeFileAtomic), and mention that full hardening would require using O_NOFOLLOW
via fs.open or similar lower-level APIs; optionally add a TODO linking to a
ticket to implement O_NOFOLLOW-based writes for stricter security.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3ce58f3-132c-441f-8e6b-032681e92ead
📒 Files selected for processing (3)
src/build/credentials.tssrc/login.tssrc/utils/safeWrites.ts
|
1 participant
This PR hardens Capgo CLI secret persistence against symlink-based overwrite attacks. It introduces shared safe file helpers for symbolic-link checks, atomic writes, and restricted permissions. Login now validates API keys before writing .capgo and persists keys only after successful verification. Credential storage now refuses symlink targets, writes atomically, and enforces 0700 directory plus 0600 file permissions for local credentials.
Summary by CodeRabbit