fix(mcp): reject symlink path escapes#93
Merged
sampleXbro merged 5 commits intoJul 8, 2026
Merged
Conversation
Owner
|
Thanks for this, @fengjikui. It's a genuinely good catch. A committed symlink under .agentsmesh survives a Git clone, so anyone running their agent on a shared or cloned repository could have configuration or skill files read from - or written to or deleted - outside the project. Great work with the failing tests first and the changeset. 🙏 While reviewing the merge, we found that the fix needs to be expanded to fully close the class.
I've put together a completion that builds directly on your commit with the same approach plus the above, including tests and an end-to-end check. I'm happy to push it to your branch so it lands under your name or leave the details here for you to apply—your call. Either way, the credit stays yours. |
…ete surfaces Builds on @fengjikui's PR sampleXbro#93. Anchors containment at the project root — a `.agentsmesh` boundary canonicalizes through a symlinked `.agentsmesh` and cancels, so a symlinked parent still escaped — and extends the guard to every remaining MCP filesystem surface: - settings config reads AND writes (get/update config, permissions, hooks, ignore, mcp servers) via the readYaml/atomicWrite/safeConfigWrite choke points - canonical list(); create() and delete() on canonical + skills - read-before-write pre-reads (updatePermissions/updateIgnore append) Hardening: fail-closed canonicalize (only ENOENT reconstructs), assert-before existence probe (removes an out-of-project filename oracle), dangling-symlink doc note, and it.skipIf(win) on all symlink tests (unprivileged Windows symlink() throws EPERM on the windows-latest CI matrix). Also drops the unrelated lessons.json trigger churn that rode in the original PR. Co-authored-by: sampleXbro <55853222+sampleXbro@users.noreply.github.com>
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
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.
What does this PR do?
Reject MCP read/write/delete paths that escape canonical directories through symlinks by canonicalizing the allowed root and target before touching files.
This covers:
safeReadsafeWriteget/updatereadsSKILL.mddirect reads and supporting-file writes/deletesType of change
Checklist
pnpm test,pnpm lint,pnpm typecheck)pnpm changeset) for user-visible changesValidation
Failing-first checks before the fix:
node_modules/.bin/vitest run tests/unit/mcp/writers/safe-read.test.ts tests/unit/mcp/writers/safe-write.test.tsfailed because symlinked paths resolved/read/wrote outside instead of rejecting.node_modules/.bin/vitest run tests/unit/mcp/handlers/canonical-factory-edges.test.ts tests/unit/mcp/handlers/skills.test.tsfailed for direct handler reads/writes through symlinked paths.Passing checks after the fix:
node_modules/.bin/vitest run tests/unit/mcp/writers/safe-read.test.ts tests/unit/mcp/writers/safe-write.test.ts tests/unit/mcp/handlers/canonical-factory-edges.test.ts tests/unit/mcp/handlers/skills.test.ts— 45 passednode_modules/.bin/vitest run tests/unit/mcp— 34 files / 360 tests passednode_modules/.bin/tsc --noEmitnode_modules/.bin/eslint src testsnode_modules/.bin/prettier --check src/mcp/handlers/canonical-factory.ts src/mcp/handlers/skills.ts src/mcp/writers/path-containment.ts src/mcp/writers/safe-read.ts src/mcp/writers/safe-write.ts tests/unit/mcp/handlers/canonical-factory-edges.test.ts tests/unit/mcp/handlers/skills.test.ts tests/unit/mcp/writers/safe-read.test.ts tests/unit/mcp/writers/safe-write.test.tsgit diff --checknode_modules/.bin/tsx src/cli/index.ts lessons validate— exit 0, graph ok with existing warningsNote: I used direct
node_modules/.bin/*commands instead of pnpm scripts because the repo's local lessons warn that pnpm 11 can mutate the pnpm 10 lockfile shape in this checkout.