Fix allowlist rejecting paths under symlinked directories (#590)

[rahul188]

Summary

On macOS, configuring a /tmp/... path in allowedDirectories rejected every access to that directory, even for the exact configured path (#590):

Error: Path not allowed: /tmp/dc-fixture. Must be within one of these directories: /tmp/dc-fixture

Root cause

validatePath() canonicalizes the requested path with fs.realpath before checking it, so /tmp/dc-fixture becomes /private/tmp/dc-fixture (macOS symlinks /tmp → /private/tmp). isPathAllowed() however compared that against the configured allowlist entries after only normalizePath() — no symlink resolution. The comparison was therefore /private/tmp/dc-fixture vs /tmp/dc-fixture, which never matches, so the allowed directory rejected itself and all of its children.

This isn't macOS-only in principle: any allowlist entry that is, or is reached through, a symlink hits the same mismatch.

Fix

Canonicalize the configured allowlist entries the same way the requested path is canonicalized, before comparing. A small canonicalizePath() helper resolves the path via fs.realpath, falling back to the deepest existing ancestor (re-appending the remaining segments) for entries that don't exist yet — mirroring the existing behavior for requested paths. Paths outside the allowed directories are still rejected.

Testing

• test/test-allowed-directories-realpath.js — new portable regression test. It reproduces the mismatch with an explicit directory symlink instead of relying on the macOS /tmp alias, and asserts the exact directory, an existing child, and a not-yet-created child all validate, while a path outside is still rejected. Skips gracefully where directory symlinks aren't permitted (e.g. Windows without the privilege). Confirmed the test fails on main and passes with this change.
• Existing test-allowed-directories.js, test-home-directory.js, and test-directory-creation.js still pass.

Fixes #590

Summary by CodeRabbit

• Bug Fixes
  • Improved access validation for configured allowed directories when they include directory symbolic links.
  • Paths are now consistently checked against their real/canonical locations, including paths that don’t exist yet.
  • Attempts to access content outside approved directories are still correctly blocked.
• Tests
  • Added a regression test covering canonicalization vs allowlist behavior for symlinked allowed directories, with platform-safe skipping when symlinks aren’t supported.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0241d699-0f90-4eee-88a6-894df6fe2c81

📥 Commits

Reviewing files that changed from the base of the PR and between 3ae93ea and 4149669.

📒 Files selected for processing (1)

• test/test-allowed-directories-realpath.js

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

• test/test-allowed-directories-realpath.js

---

📝 Walkthrough

Walkthrough

Adds canonicalization for configured allowlisted directories, including symlink and unresolved-path handling, and adds regression coverage for allowed symlink paths, children, outside-path rejection, cleanup, and unsupported platforms.

Changes

Path canonicalization and validation

Layer / File(s)	Summary
Canonical allowlist matching src/tools/filesystem.ts	Adds canonicalizePath() and uses it to normalize expanded absolute allowedDirectories entries before containment checks.
Symlink allowlist regression coverage test/test-allowed-directories-realpath.js	Tests symlink directory and child validation, outside-path rejection, cleanup, platform skips, and pass/fail reporting.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• wonderwhy-er/DesktopCommanderMCP#76: Both changes adjust isPathAllowed() path comparison behavior.
• wonderwhy-er/DesktopCommanderMCP#321: Both changes resolve filesystem paths before allowlist validation.
• wonderwhy-er/DesktopCommanderMCP#398: Both changes address symlink and realpath handling in allowlist enforcement.

Suggested labels: size:M

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is concise and matches the main change: fixing allowlist failures for symlinked paths.
Linked Issues check	✅ Passed	The code canonicalizes allowlist entries with realpath fallback and the test covers exact, child, non-existent child, and outside-path cases.
Out of Scope Changes check	✅ Passed	The patch stays focused on allowlist canonicalization and one regression test, with no unrelated scope added.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@test/test-allowed-directories-realpath.js`:
- Around line 34-40: Update the symlink setup catch in the test to skip only
recognized platform capability errors, such as the established
unsupported-operation or permission error codes, and rethrow all other failures.
Preserve cleanup and the 'skipped' return for recognized capability errors,
while allowing setup defects to fail the test.
- Around line 68-74: Update the assertion in the not-yet-created child case to
compare the resolved parent directory directly with resolvedTarget, rather than
comparing its parent with path.dirname(resolvedTarget). Keep the existing
validatePath(newChild) flow and success message unchanged.

🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4beede26-ea9a-49d0-90cd-0f368e81e862

📥 Commits

Reviewing files that changed from the base of the PR and between 78f8f4b and 3ae93ea.

📒 Files selected for processing (2)

• src/tools/filesystem.ts
• test/test-allowed-directories-realpath.js

[rahul188]

Hi 👋 Gentle nudge on this PR — it's mergeable and the required checks are passing (one non-required/optional check shows as unstable, which shouldn't block merge). Would appreciate a review whenever you have a moment, and I'm happy to address anything needed. Thanks for your time and for maintaining this project! 🙏