fix(security): reject unsafe HOME fallback in credential storage#864
fix(security): reject unsafe HOME fallback in credential storage#864fdzdev wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCredential path resolution now validates and normalizes the user's home directory, rejects unsafe world-readable locations, and computes credential directory and file paths lazily. IO functions use the resolved paths; saving enforces directory (0700) and file (0600) permissions. Exports changed from constants to getter properties. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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 `@bin/lib/credentials.js`:
- Around line 48-53: The write path in saveCredential (using getCredsDir,
getCredsFile, loadCredentials) relies on fs.writeFileSync's mode which only
applies on creation; ensure permissions are enforced on every save by calling
fs.chmodSync(file, 0o600) immediately after writing the JSON, so the credentials
file always ends up with strict 0600 permissions (keep the existing
fs.mkdirSync(dir, { mode: 0o700 }) behavior for the directory).
- Around line 10-22: Normalize and canonicalize the resolved HOME before
checking against UNSAFE_HOME_PATHS: inside resolveHomeDir(), take the computed
home (from process.env.HOME || os.homedir()), normalize it (e.g., remove
trailing slashes using path.resolve or path.normalize) and resolve symlinks to a
real path using fs.realpathSync (with a safe try/catch fallback to the
normalized path if realpath fails), then run the UNSAFE_HOME_PATHS.has() check
against that canonical path (and optional lowercase on case-insensitive
platforms) so values like "/tmp/" or symlinked aliases correctly match the
blocklist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 274cf4d0-5e61-4e13-90b1-a42a6e656ab0
📒 Files selected for processing (1)
bin/lib/credentials.js
When HOME is unset, credentials.js fell back to /tmp, making credentials.json world-readable (CWE-522). Replace eager module-level resolution with lazy getters that validate HOME at access time and reject /tmp, /var/tmp, /dev/shm, and /. Reads degrade gracefully (return empty); writes throw a clear error. Module import never crashes. NVBUG 6002816 Made-with: Cursor
- Canonicalize HOME via path.resolve + fs.realpathSync before blocklist check so /tmp/ (trailing slash) and symlinks are caught - Add explicit fs.chmodSync after write to enforce 0o600/0o700 permissions on existing files (writeFileSync mode only applies on creation) Made-with: Cursor
fdzdev
force-pushed
the
fix/credentials-home-fallback
branch
from
b5bb2ca to
8844c96
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/credentials.js`:
- Around line 279-282: saveCredential currently calls getCredsDir() and
getCredsFile() directly and can throw when resolveHomeDir() deems HOME unsafe;
wrap those calls in a try/catch inside saveCredential (catch the error, add
context like "saving credential failed: unsafe HOME" and rethrow or return a
rejected Promise consistently with saveCredential's API) and then update each
call site (onboard.js caller and the two callers in credentials.js) to handle
the thrown/rejected error (try/catch or .catch) and surface a user-friendly
message instead of assuming success.
🪄 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
Run ID: 171eee63-2630-4d22-89e4-5538d01df797
📒 Files selected for processing (1)
bin/lib/credentials.js
| Object.defineProperty(exports_, "CREDS_DIR", { get: getCredsDir, enumerable: true }); | ||
| Object.defineProperty(exports_, "CREDS_FILE", { get: getCredsFile, enumerable: true }); | ||
|
|
||
| module.exports = exports_; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all callers of saveCredential and check if they have error handling
# Find saveCredential calls and show surrounding context for error handling
rg -n -B3 -A3 'saveCredential\s*\(' --type=jsRepository: NVIDIA/NemoClaw
Length of output: 1536
🏁 Script executed:
# Check if CREDS_DIR or CREDS_FILE are destructured anywhere
rg -n "{\s*CREDS_DIR|{\s*CREDS_FILE|require.*CREDS" --type=jsRepository: NVIDIA/NemoClaw
Length of output: 177
🏁 Script executed:
# Find all getCredential calls and their context to check for error handling
rg -n -B2 -A2 "getCredential\s*\(" --type=js | head -80Repository: NVIDIA/NemoClaw
Length of output: 4920
🏁 Script executed:
# Verify the actual implementation of saveCredential to confirm it can throw
cat -n bin/lib/credentials.js | sed -n '64,75p'Repository: NVIDIA/NemoClaw
Length of output: 517
🏁 Script executed:
# Search for CREDS_DIR/CREDS_FILE destructuring with corrected regex
rg -n "CREDS_DIR|CREDS_FILE" --type=js | grep -E "require|import|const.*="Repository: NVIDIA/NemoClaw
Length of output: 41
🏁 Script executed:
# Get the full implementation of getCredsDir, getCredsFile, and getCredential
cat -n bin/lib/credentials.js | sed -n '1,80p'Repository: NVIDIA/NemoClaw
Length of output: 2891
🏁 Script executed:
# Check if resolveHomeDir can throw
cat -n bin/lib/credentials.js | sed -n '1,50p'Repository: NVIDIA/NemoClaw
Length of output: 1958
🏁 Script executed:
# Get the exports section to verify what's exported
cat -n bin/lib/credentials.js | sed -n '260,282p'Repository: NVIDIA/NemoClaw
Length of output: 760
🏁 Script executed:
# Check all places where credentials module is imported to see if anyone destructures CREDS_DIR/CREDS_FILE
rg -n "require.*credentials|from.*credentials" --type=js -A 2Repository: NVIDIA/NemoClaw
Length of output: 4528
🏁 Script executed:
# Get full context of bin/nemoclaw.js import
cat -n bin/nemoclaw.js | sed -n '25,35p'Repository: NVIDIA/NemoClaw
Length of output: 501
🏁 Script executed:
# Get full context of bin/lib/onboard.js import
cat -n bin/lib/onboard.js | sed -n '30,40p'Repository: NVIDIA/NemoClaw
Length of output: 660
saveCredential() can throw on unsafe HOME and lacks error handling at call sites.
The getter-based exports for CREDS_DIR and CREDS_FILE work correctly for lazy resolution. However, saveCredential() calls getCredsDir() and getCredsFile() directly (lines 65-66) without try/catch protection. These functions can throw from resolveHomeDir() if HOME is unsafe. The three call sites—onboard.js:1133, credentials.js:215, and credentials.js:262—have no error handling for this condition, though they do validate input before calling saveCredential().
Note: getCredential() is safe; it wraps getCredsFile() in a try/catch via loadCredentials(), so it will not throw even if HOME is unsafe.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/credentials.js` around lines 279 - 282, saveCredential currently
calls getCredsDir() and getCredsFile() directly and can throw when
resolveHomeDir() deems HOME unsafe; wrap those calls in a try/catch inside
saveCredential (catch the error, add context like "saving credential failed:
unsafe HOME" and rethrow or return a rejected Promise consistently with
saveCredential's API) and then update each call site (onboard.js caller and the
two callers in credentials.js) to handle the thrown/rejected error (try/catch or
.catch) and surface a user-friendly message instead of assuming success.
2 participants
Summary
HOMEis unset,credentials.jsfalls back to/tmp, makingcredentials.jsonworld-readable (CWE-522, NVBUG 6002816)const CREDS_DIR = path.join(process.env.HOME || "/tmp", ...)with lazy getters that validate at access time/tmp,/var/tmp,/dev/shm, and/as credential storage locations{}); writes throw a clear error; module import never crashesTest plan
NVIDIA_API_KEYin env +HOMEunset →getCredential("NVIDIA_API_KEY")returns the env value (no file access)HOMEunset, no env var →getCredential()returnsnull(no crash)HOME=/tmp→saveCredential()throws with actionable error messageHOME=/Users/x(normal) → credentials saved/loaded as before, mode 0600CREDS_DIR/CREDS_FILEgets lazily resolved values via property gettersSummary by CodeRabbit