fix(security): reject symlinks on ~/.nemoclaw to prevent credential hijack - #1996
Closed
ericksoa wants to merge 2 commits into
Closed
fix(security): reject symlinks on ~/.nemoclaw to prevent credential hijack#1996ericksoa wants to merge 2 commits into
ericksoa wants to merge 2 commits into
Conversation
…rite hijack
mkdirSync(~/.nemoclaw, { recursive: true }) does not check whether the
path already exists as a symlink. An attacker who creates ~/.nemoclaw as
a symlink to an attacker-controlled directory before the user first runs
NemoClaw causes credentials and state to be written outside the intended
directory.
Add safeMkdirSync() utility that checks lstat() before mkdirSync() and
throws if the target is a symbolic link. Apply it to all ~/.nemoclaw
directory creation paths across both the CLI (config-io, onboard-session,
onboard, usage-notice) and plugin (state, config, runner, snapshot).
Also fix remaining process.env.HOME||"/tmp" patterns (registry.ts,
onboard-session.ts) with os.homedir().
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughReplaced direct fs.mkdirSync uses with a new safeMkdirSync helper that rejects symlinked path components before creating directories; concurrently switched several default user-path resolutions from environment-based HOME lookups to os.homedir(). 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@nemoclaw/src/onboard/config.ts`:
- Around line 76-84: Existing path checks skip symlink validation and the broad
catch hides symlink errors: ensure you validate an existing configDir with
fs.lstatSync and reject if lstat.isSymbolicLink() or if it's not a directory
(referencing configDir and safeMkdirSync). Always call safeMkdirSync when
creating the directory, and replace the broad catch with error handling that
only falls back for expected ENOENT/permission cases; rethrow or fail hard on
symlink-related errors so we don't silently switch to tmpdir. Ensure validation
happens both for the existing-path branch and after a failed mkdir attempt so
symlinks are never accepted.
In `@src/lib/safe-dir.ts`:
- Around line 16-29: The current rejectSymlink only checks the final path
component, letting writes follow symlinked ancestors; update rejectSymlink to
resolve dirPath and iterate up through each ancestor (using path.resolve and
path.dirname) calling lstatSync on each existing component and throwing if any
isSymbolicLink; on lstatSync ENOENT for a component just continue up (ancestors
above may exist) and stop when dirname equals the current path (root); keep
existing behavior of rethrowing non-ENOENT errors. This will ensure
safeMkdirSync/mkdirSync recursive creates cannot traverse into symlinked
ancestor directories.
🪄 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: 43291f7a-53b6-4c57-9331-bbb7a100564e
📒 Files selected for processing (11)
nemoclaw/src/blueprint/runner.tsnemoclaw/src/blueprint/snapshot.tsnemoclaw/src/blueprint/state.tsnemoclaw/src/lib/safe-dir.tsnemoclaw/src/onboard/config.tssrc/lib/config-io.tssrc/lib/onboard-session.tssrc/lib/onboard.tssrc/lib/registry.tssrc/lib/safe-dir.tssrc/lib/usage-notice.ts
Comment on lines
76
to
84
| if (!existsSync(configDir)) { | ||
| try { | ||
| mkdirSync(configDir, { recursive: true }); | ||
| safeMkdirSync(configDir); | ||
| } catch { | ||
| configDir = join(tmpdir(), ".nemoclaw"); | ||
| if (!existsSync(configDir)) { | ||
| mkdirSync(configDir, { recursive: true }); | ||
| safeMkdirSync(configDir); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Symlink protection is currently bypassed in the existing-path flow.
At Line 76, an existing ~/.nemoclaw path skips safeMkdirSync, so a pre-created symlink is not validated/rejected. Also, the broad catch at Line 79 masks a symlink error by falling back to /tmp, which contradicts the PR’s expected hard failure behavior.
🔧 Proposed fix
function ensureConfigDir(): void {
if (configDirCreated) return;
- if (!existsSync(configDir)) {
- try {
- safeMkdirSync(configDir);
- } catch {
- configDir = join(tmpdir(), ".nemoclaw");
- if (!existsSync(configDir)) {
- safeMkdirSync(configDir);
- }
- }
- }
+ try {
+ // Always validate/create the configured path so existing symlinks are rejected.
+ safeMkdirSync(configDir);
+ } catch (error: unknown) {
+ const err = error as NodeJS.ErrnoException;
+ const message = error instanceof Error ? error.message : "";
+ // Never downgrade symlink violations to a tmp fallback.
+ if (/symbolic link/i.test(message)) {
+ throw error;
+ }
+ // Keep legacy tmp fallback only for permission/filesystem access failures.
+ if (!["EACCES", "EPERM", "EROFS"].includes(err.code ?? "")) {
+ throw error;
+ }
+ configDir = join(tmpdir(), ".nemoclaw");
+ safeMkdirSync(configDir);
+ }
configDirCreated = true;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nemoclaw/src/onboard/config.ts` around lines 76 - 84, Existing path checks
skip symlink validation and the broad catch hides symlink errors: ensure you
validate an existing configDir with fs.lstatSync and reject if
lstat.isSymbolicLink() or if it's not a directory (referencing configDir and
safeMkdirSync). Always call safeMkdirSync when creating the directory, and
replace the broad catch with error handling that only falls back for expected
ENOENT/permission cases; rethrow or fail hard on symlink-related errors so we
don't silently switch to tmpdir. Ensure validation happens both for the
existing-path branch and after a failed mkdir attempt so symlinks are never
accepted.
…mlink errors Address CodeRabbit findings: 1. rejectSymlink() now walks from the target path upward to $HOME, checking each component with lstatSync(). If a component is a symlink, it throws. If it exists as a real directory, it stops (trusted anchor). Stops at $HOME because system-level symlinks above it (e.g. /tmp -> /private/tmp, /var -> /private/var on macOS) are trusted OS infrastructure, not attack vectors. 2. config.ts ensureConfigDir() no longer swallows symlink errors in its broad catch — symlink violations are always rethrown. Only EACCES/EPERM/EROFS trigger the tmpdir fallback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nemoclaw/src/onboard/config.ts (1)
79-85: Consider a typed symlink error contract instead of message regex matching.
/symbolic link/iworks, but it is brittle. A dedicated error class/code fromsafe-dirwould make this check more durable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nemoclaw/src/onboard/config.ts` around lines 79 - 85, Replace the brittle /symbolic link/i message check with a typed contract exported by the safe-dir package: import the specific error class or canonical error code from safe-dir (e.g., SafeDirSymlinkError or SAFE_DIR_SYMLINK_CODE) and change the conditional to throw when (error instanceof SafeDirSymlinkError) or when (error && (error as NodeJS.ErrnoException).code === SAFE_DIR_SYMLINK_CODE); keep the original regex only as a documented fallback if the safe-dir API lacks a concrete symbol, and update the comment to reference the safe-dir contract; change the conditional around the existing error variable in this block accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nemoclaw/src/onboard/config.ts`:
- Around line 79-85: Replace the brittle /symbolic link/i message check with a
typed contract exported by the safe-dir package: import the specific error class
or canonical error code from safe-dir (e.g., SafeDirSymlinkError or
SAFE_DIR_SYMLINK_CODE) and change the conditional to throw when (error
instanceof SafeDirSymlinkError) or when (error && (error as
NodeJS.ErrnoException).code === SAFE_DIR_SYMLINK_CODE); keep the original regex
only as a documented fallback if the safe-dir API lacks a concrete symbol, and
update the comment to reference the safe-dir contract; change the conditional
around the existing error variable in this block accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45a6f720-57b4-4d4b-aa8d-8bb7c0a9f0f1
📒 Files selected for processing (3)
nemoclaw/src/lib/safe-dir.tsnemoclaw/src/onboard/config.tssrc/lib/safe-dir.ts
✅ Files skipped from review due to trivial changes (1)
- nemoclaw/src/lib/safe-dir.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/safe-dir.ts
Contributor
Author
|
Superseded by #2290, which merged on 2026-04-23 and fixes the same symlink credential hijack vulnerability (NVBug 6079246). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
safeMkdirSync()utility that checkslstat()beforemkdirSync()and throws if the target path is a symbolic link~/.nemoclawdirectory creation paths across both CLI and plugin (8 vulnerable locations)process.env.HOME||"/tmp"patterns withos.homedir()Why
mkdirSync(~/.nemoclaw, { recursive: true })does not check whether the path already exists as a symlink. An attacker who creates~/.nemoclawas a symlink to an attacker-controlled directory before the user first runs NemoClaw causes credentials, state, and config to be written to the attacker's location. Combined with plaintext credential storage, API keys are directly readable.What changed
src/lib/safe-dir.tsandnemoclaw/src/lib/safe-dir.ts—safeMkdirSync()utility (lstat check + mkdirSync)config-io.ts,onboard-session.ts,onboard.ts,usage-notice.ts— replaced baremkdirSyncwithsafeMkdirSyncstate.ts,config.ts,runner.ts,snapshot.ts— replaced baremkdirSyncwithsafeMkdirSyncregistry.tsandonboard-session.ts— fixedprocess.env.HOME||"/tmp"→os.homedir()Test plan
npm run build(plugin) — compiles cleanlynpm run typecheck:cli— passesvitest run --project plugin— 334 tests passln -s /tmp/test ~/.nemoclaw && nemoclaw onboard— verify error: "path is a symbolic link"🤖 Generated with Claude Code
Summary by CodeRabbit