fix: resolve filesystem tool paths symlink-aware before read/write/list - #2880
Closed
dungdong-aws wants to merge 1 commit into
Closed
dungdong-aws wants to merge 1 commit into
dungdong-aws wants to merge 1 commit into
Conversation
The filesystem tools derived the path they operate on differently from the path the workspace-containment check evaluates. `requiresPathAcceptance` resolves symlinks at every segment via `resolveSymlinkAwarePath`, while `fsWrite`, `fsReplace`, `fsRead`, `fileSearch`, and `listDirectory` called `sanitize()`, which only expands `~` and makes the path absolute. The unresolved path was then handed to `writeFile`/`readFile`/the directory walk, so the operating system followed the link when it opened the file rather than the tool operating on the target it had evaluated. Add `resolveCanonicalPath()` to `toolShared` (sanitize, then resolve symlinks) and use it in both `requiresPathAcceptance` and all five tools, so the boundary decision and the operation act on the same resolved path and a link replaced after the path is resolved no longer changes where the operation lands. `sanitize()` is composed with the resolver rather than replaced by it: `resolveSymlinkAwarePath` does not expand `~`, so calling it alone would drop tilde expansion at these call sites.
Contributor
Author
|
Closed and superseded by #2881. |
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
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.
Problem
The filesystem tools resolve the path they operate on differently from the path the workspace-containment check evaluates.
toolShared.ts'srequiresPathAcceptance()callsresolveSymlinkAwarePath(), which follows symlinks at every segment (including a link whose target does not exist yet) and tests the resolved target against the workspace boundary. If the resolved target is inside the workspace it returnsrequiresAcceptance: falseand no prompt is shown.fsWrite,fsReplace,fsRead,fileSearch, andlistDirectorycalledsanitize()instead, which only expands~and makes the path absolute. It does not resolve symlinks.The unresolved path was then passed to
writeFile/readFile/ the directory walk, so the operating system followed the link when it opened the file. The tool therefore did not operate on the target its own boundary check had evaluated: if the link is replaced while the operation is in flight, the check evaluated one target and the operation landed on another.A path whose name sits inside the workspace can point anywhere, so this affects all five tools: an out-of-workspace write for
fsWrite/fsReplace, out-of-workspace file contents returned to the model forfsRead, and out-of-workspace names disclosed byfileSearch/listDirectory.Change
Add
resolveCanonicalPath()totoolShared.ts—sanitize()(expand~, make absolute) followed byresolveSymlinkAwarePath()(follow symlinks at every segment) — and use it inrequiresPathAcceptance()and at every I/O site in the five tools.sanitize()is composed with the resolver rather than replaced by it.resolveSymlinkAwarePathstarts atpath.resolve()and does not expand~, so calling it alone at these call sites would drop tilde expansion; composing keeps that behavior and adds symlink resolution.toolShared.tsresolveSymlinkAwarePath(inputPath)resolveCanonicalPath(inputPath)fsWrite.tssanitize(params.path)invalidate/invokeawait resolveCanonicalPath(params.path)fsReplace.tssanitize(params.path)invalidate/invokeawait resolveCanonicalPath(params.path)fsRead.tssanitize(path)beforereadFileawait resolveCanonicalPath(path)fileSearch.tssanitize(params.path)before the walkawait resolveCanonicalPath(params.path)listDirectory.tssanitize(params.path)before the listingawait resolveCanonicalPath(params.path)Text alternative for the table: each of the five tools plus the shared helper moves from a non-symlink-aware
sanitize()call to the symlink-awareresolveCanonicalPath()at both its validation and its I/O site.Every call site was already
async, so awaiting the resolver required no signature changes.Testing
pretty-quick,git-secrets,commitlint).fsWrite.test.ts,fsReplace.test.ts,fsRead.test.ts,fileSearch.test.ts,listDirectory.test.ts,toolShared.test.ts)./tmp->/private/tmp) will see the resolved value; content assertions are unaffected.Base branch:
feature/mcp-security-enchance.