fix(security): reject symlinks on config directory path by ericksoa · Pull Request #2290 · NVIDIA/NemoClaw

ericksoa · 2026-04-22T17:43:32Z

Summary

Mitigates a symlink attack on the ~/.nemoclaw credentials directory: an attacker who plants ~/.nemoclaw as a symlink to an attacker-controlled directory before the user's first run could hijack credential writes
Adds rejectSymlinksOnPath() in ensureConfigDir() that walks from the target directory up to $HOME, using lstatSync() on each component to detect symlinks without following them
Aborts with a descriptive error if any path component is a symbolic link, preventing credential write hijack

Test plan

New test: rejects a direct symlink in place of the config directory
New test: rejects a symlink in an ancestor of the config directory
New test: allows a normal (non-symlink) directory without error
All 11 config-io tests pass
Full test suite: 2062 passed, 0 regressions (5 pre-existing infra failures unrelated to this change)

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved configuration-directory creation and validation to detect and reject symlinked path components under the home directory, causing early failure when such symlinks are encountered.
Tests
- Added tests covering symlink-in-target, symlink-in-ancestor, and normal-directory success cases; verifies created config paths are not symbolic links.

…g credentials An attacker who plants ~/.nemoclaw as a symlink to an attacker-controlled directory before the user's first run could hijack credential writes. Add lstat()-based symlink detection in ensureConfigDir() that walks the path from the target up to $HOME, aborting with a descriptive error if any component is a symbolic link. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

coderabbitai · 2026-04-22T17:43:44Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 18f6c99a-11ab-4e4b-be61-a21b4892ac10

📥 Commits

Reviewing files that changed from the base of the PR and between 208f519 and a6fac4b.

📒 Files selected for processing (2)

src/lib/config-io.test.ts
src/lib/config-io.ts

🚧 Files skipped from review as they are similar to previous changes (2)

src/lib/config-io.ts
src/lib/config-io.test.ts

📝 Walkthrough

Walkthrough

Added symlink detection to config directory initialization: a helper walks resolved path components (up to HOME) and throws if any component is a symlink; ensureConfigDir calls this check before any mkdir/stat/access operations. Tests added for symlink and non-symlink scenarios.

Changes

Cohort / File(s)	Summary
Symlink Validation Implementation `src/lib/config-io.ts`	Added internal `rejectSymlinksOnPath(dirPath)` that resolves the path and walks upward (excluding HOME) using `lstatSync` to detect symbolic links, throwing errors with `readlink` target and remediation suggestion. `ensureConfigDir()` now invokes this check before creating or inspecting dirs.
Symlink Validation Tests `src/lib/config-io.test.ts`	Added three tests: (1) fail when requested config path is a symlink (asserts error contains `symbolic link` and `symlink attack`), (2) fail when an ancestor component is a symlink, (3) succeed and verify created dir is not a symlink when no symlinks exist. Only test additions; no production API changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hop the paths both near and far,

sniffing links where mischief are.
If a symlink tries to lead astray,
I shout "Nope!" and guard the way.
Configs safe — I nibble hay. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The pull request title clearly and specifically summarizes the main security fix: rejecting symlinks in the config directory path to prevent symlink attacks on credential storage.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/symlink-attack-credentials-dir

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/lib/config-io.test.ts (1)

38-65: Make symlink tests deterministic by pinning HOME

These assertions depend on HOME-bounded traversal. Set process.env.HOME to the test temp root in these cases so behavior is stable across CI/dev environments.

Suggested pattern

   it("rejects a symlink in place of the config directory", () => {
     const tmp = makeTempDir();
+    const originalHome = process.env.HOME;
+    process.env.HOME = tmp;
+    try {
     const attackerDir = path.join(tmp, "attacker");
     fs.mkdirSync(attackerDir);
     const symlinkPath = path.join(tmp, ".nemoclaw");
     fs.symlinkSync(attackerDir, symlinkPath);

     expect(() => ensureConfigDir(symlinkPath)).toThrow(/symbolic link/);
     expect(() => ensureConfigDir(symlinkPath)).toThrow(/symlink attack/);
+    } finally {
+      process.env.HOME = originalHome;
+    }
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/config-io.test.ts` around lines 38 - 65, The tests that call
ensureConfigDir rely on HOME-based traversal and should pin process.env.HOME to
the temp root to make them deterministic; in the "rejects a symlink in place of
the config directory", "rejects a symlink in an ancestor of the config
directory", and "allows a normal directory (no symlinks)" tests set
process.env.HOME = tmp (or the temp root) before calling ensureConfigDir and
restore the original HOME afterwards (or use a beforeEach/afterEach to
save/restore) so ensureConfigDir sees the expected home layout.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/config-io.ts`:
- Around line 101-105: The error message thrown in the throw new Error(...) in
src/lib/config-io.ts interpolates an unquoted ${current} into the suggested rm
command; update that string to safely quote the path (e.g., use rm --
"${current}" or rm -rf -- '${current}') so copy/paste works with spaces and
shell metacharacters, keeping the rest of the message and variables (current,
target) unchanged.
- Around line 96-113: The loop that walks up from current to check for symlinks
uses a string-length comparison (current.length > resolvedHome.length) which can
skip the walk when HOME is longer than the target; replace that length-based
stop with an ancestry check: change the while condition to walk until current
equals resolvedHome or you reach the filesystem root (e.g., while (current !==
resolvedHome && current !== path.dirname(current))) so the symlink checks in the
loop (the lstatSync/readlinkSync block) always examine all ancestor directories
up to the actual resolvedHome; ensure resolvedHome is the resolved absolute HOME
used earlier so the comparison is correct.

---

Nitpick comments:
In `@src/lib/config-io.test.ts`:
- Around line 38-65: The tests that call ensureConfigDir rely on HOME-based
traversal and should pin process.env.HOME to the temp root to make them
deterministic; in the "rejects a symlink in place of the config directory",
"rejects a symlink in an ancestor of the config directory", and "allows a normal
directory (no symlinks)" tests set process.env.HOME = tmp (or the temp root)
before calling ensureConfigDir and restore the original HOME afterwards (or use
a beforeEach/afterEach to save/restore) so ensureConfigDir sees the expected
home layout.

🪄 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: 2a9ede25-713c-463f-9ff2-e199d50ced98

📥 Commits

Reviewing files that changed from the base of the PR and between d50452a and 208f519.

📒 Files selected for processing (2)

src/lib/config-io.test.ts
src/lib/config-io.ts

…n path Address CodeRabbit review: - Replace string-length loop boundary with path.relative() ancestry check so the walk never skips when HOME is longer than the target - Only inspect path components between HOME and dirPath (the user-controllable segment); system-level symlinks above HOME (e.g. /var -> private/var on macOS) are legitimate - Shell-quote the path in the error message remediation command - Update tests to create temp dirs under HOME so the symlink check is exercised Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cjagwani · 2026-04-23T20:14:31Z

LGTM. Clean scope, three tests covering direct / ancestor / normal
cases, no regressions in the CLI vitest suite. Approved.. merge when ready!

#2436) ## Summary - Adds `lstatSync`-based symlink rejection to `createSnapshot()` and `rollbackFromSnapshot()` in `nemoclaw/src/blueprint/snapshot.ts`, closing two attack surfaces reported by an external contributor doing Yocto integration: 1. **`createSnapshot()`** calls `cpSync(OPENCLAW_DIR, dest)` with no `lstatSync` check on the source — if `~/.openclaw` is a symlink (e.g. to `/etc`), `cpSync` follows it and copies arbitrary files into the snapshot 2. **`rollbackFromSnapshot()`** calls `cpSync(source, OPENCLAW_DIR)` without verifying the destination isn't a symlink — an attacker-planted symlink could redirect restored content to an arbitrary directory - Updates `collectFiles()` to detect symlinks via `isSymbolicLink()` and record them in the snapshot manifest (previously symlinks were silently omitted) - Follows the `rejectSymlinksOnPath()` pattern established in PR #2290 for the `~/.nemoclaw` credentials directory - **Also guards the CLI code path** (`src/lib/sandbox-state.ts::backupSandboxState()`) that `nemoclaw <sandbox> snapshot create` actually uses — the blueprint helper is a separate implementation not called from the CLI - `rollbackFromSnapshot()` symlink rejection returns `false` (preserving the boolean-return contract) rather than throwing ### Severity Low-Medium. Both attacks require the attacker to already have user-level write access to `$HOME` to plant the symlink, and the sandbox's Landlock / `chattr +i` / read-only mount protections prevent in-sandbox exploitation. However, this is the same symlink attack class fixed in #2290 (different code paths: `cpSync` + directory walker vs. `mkdirSync`), and defense-in-depth warrants closing the gap. ## Test plan - [x] New test: `createSnapshot` rejects when `~/.openclaw` is a symlink - [x] New test: `createSnapshot` rejects when an ancestor of `~/.nemoclaw` is a symlink - [x] New test: `createSnapshot` records symlinks in manifest when present in tree - [x] New test: `rollbackFromSnapshot` returns false (not throws) when `~/.openclaw` is a symlink - [x] All existing snapshot tests pass (no regressions) - [x] Full plugin test suite: 362 passed - [x] CLI test suite: 1959/1961 passed (2 pre-existing flaky installer timeouts) - [x] TypeScript build clean (`tsc --noEmit`) - [x] All pre-commit and pre-push linters/formatters pass 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ## Summary by CodeRabbit * **New Features** * Snapshots now detect and record symbolic links in the snapshot manifest and refuse snapshot creation or restoration when critical config directories or their ancestor paths are symlinks. * Backup destination paths are validated to prevent writing into locations with symlinked ancestor paths. * **Tests** * Added tests covering symlink detection, manifest recording, rejection scenarios, and rollback behavior when config paths are symlinks.  --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

NVIDIA#2436) ## Summary - Adds `lstatSync`-based symlink rejection to `createSnapshot()` and `rollbackFromSnapshot()` in `nemoclaw/src/blueprint/snapshot.ts`, closing two attack surfaces reported by an external contributor doing Yocto integration: 1. **`createSnapshot()`** calls `cpSync(OPENCLAW_DIR, dest)` with no `lstatSync` check on the source — if `~/.openclaw` is a symlink (e.g. to `/etc`), `cpSync` follows it and copies arbitrary files into the snapshot 2. **`rollbackFromSnapshot()`** calls `cpSync(source, OPENCLAW_DIR)` without verifying the destination isn't a symlink — an attacker-planted symlink could redirect restored content to an arbitrary directory - Updates `collectFiles()` to detect symlinks via `isSymbolicLink()` and record them in the snapshot manifest (previously symlinks were silently omitted) - Follows the `rejectSymlinksOnPath()` pattern established in PR NVIDIA#2290 for the `~/.nemoclaw` credentials directory - **Also guards the CLI code path** (`src/lib/sandbox-state.ts::backupSandboxState()`) that `nemoclaw <sandbox> snapshot create` actually uses — the blueprint helper is a separate implementation not called from the CLI - `rollbackFromSnapshot()` symlink rejection returns `false` (preserving the boolean-return contract) rather than throwing ### Severity Low-Medium. Both attacks require the attacker to already have user-level write access to `$HOME` to plant the symlink, and the sandbox's Landlock / `chattr +i` / read-only mount protections prevent in-sandbox exploitation. However, this is the same symlink attack class fixed in NVIDIA#2290 (different code paths: `cpSync` + directory walker vs. `mkdirSync`), and defense-in-depth warrants closing the gap. ## Test plan - [x] New test: `createSnapshot` rejects when `~/.openclaw` is a symlink - [x] New test: `createSnapshot` rejects when an ancestor of `~/.nemoclaw` is a symlink - [x] New test: `createSnapshot` records symlinks in manifest when present in tree - [x] New test: `rollbackFromSnapshot` returns false (not throws) when `~/.openclaw` is a symlink - [x] All existing snapshot tests pass (no regressions) - [x] Full plugin test suite: 362 passed - [x] CLI test suite: 1959/1961 passed (2 pre-existing flaky installer timeouts) - [x] TypeScript build clean (`tsc --noEmit`) - [x] All pre-commit and pre-push linters/formatters pass 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ## Summary by CodeRabbit * **New Features** * Snapshots now detect and record symbolic links in the snapshot manifest and refuse snapshot creation or restoration when critical config directories or their ancestor paths are symlinks. * Backup destination paths are validated to prevent writing into locations with symlinked ancestor paths. * **Tests** * Added tests covering symlink detection, manifest recording, rejection scenarios, and rollback behavior when config paths are symlinks.  --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

coderabbitai Bot reviewed Apr 22, 2026

View reviewed changes

Comment thread src/lib/config-io.ts Outdated

Comment thread src/lib/config-io.ts

ericksoa self-assigned this Apr 22, 2026

ericksoa added 2 commits April 22, 2026 12:25

Merge branch 'main' into fix/symlink-attack-credentials-dir

339640f

Merge branch 'main' into fix/symlink-attack-credentials-dir

87d46bd

wscurran added priority: high and removed bug Something fails against expected or documented behavior labels Apr 22, 2026

ericksoa added 3 commits April 22, 2026 13:44

Merge branch 'main' into fix/symlink-attack-credentials-dir

c7c37dc

Merge branch 'main' into fix/symlink-attack-credentials-dir

44243b3

Merge branch 'main' into fix/symlink-attack-credentials-dir

681c491

cv approved these changes Apr 23, 2026

View reviewed changes

cv merged commit a3aafcd into main Apr 23, 2026
18 checks passed

This was referenced Apr 24, 2026

fix(security): reject symlinks on ~/.nemoclaw to prevent credential hijack #1996

Closed

fix(security): validate symlinks in snapshot create and rollback paths #2436

Merged

cjagwani mentioned this pull request May 14, 2026

feat(skills): add nemoclaw-maintainer-quick-wins skill #3519

Closed

wscurran added bug-fix PR fixes a bug or regression and removed priority: high labels Jun 3, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): reject symlinks on config directory path#2290

fix(security): reject symlinks on config directory path#2290
cv merged 7 commits into
mainfrom
fix/symlink-attack-credentials-dir

ericksoa commented Apr 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Apr 22, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

cjagwani commented Apr 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ericksoa commented Apr 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cjagwani commented Apr 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericksoa commented Apr 22, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 22, 2026 •

edited

Loading