Skip to content

fix: resolve filesystem tool paths symlink-aware before read/write/list - #2881

Open
dungdong-aws wants to merge 3 commits into
Amazon-Q-Developer:feature/mcp-security-enchancefrom
dungdong-aws:fix/filesystem-tool-canonical-path
Open

dungdong-aws wants to merge 3 commits into
Amazon-Q-Developer:feature/mcp-security-enchancefrom
dungdong-aws:fix/filesystem-tool-canonical-path

Conversation

@dungdong-aws

Copy link
Copy Markdown
Contributor

Problem

The filesystem tools resolve the path they operate on differently from the path the workspace-containment check evaluates.

  • ChecktoolShared.ts's requiresPathAcceptance() calls resolveSymlinkAwarePath(), which follows symlinks at every segment (including a link whose target does not exist yet) and tests the resolved target against the workspace boundary.
  • OperationfsWrite, fsReplace, fsRead, fileSearch, and listDirectory called sanitize() 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 dereferenced the link when it opened the file. The tool therefore did not operate on the target its own containment check had evaluated.

A path whose name sits inside the workspace can point anywhere, so this affects all five tools: fsWrite/fsReplace write through the link, fsRead returns the linked file's contents to the model, and fileSearch/listDirectory report names from the linked location.

Change

Add resolveCanonicalPath() to toolShared.tssanitize() (expand ~, make absolute) followed by resolveSymlinkAwarePath() (follow symlinks at every segment) — and use it in requiresPathAcceptance() and at every I/O site in the five tools.

sanitize() is composed with the resolver rather than replaced by it. resolveSymlinkAwarePath starts at path.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.

File Before After
toolShared.ts resolveSymlinkAwarePath(inputPath) resolveCanonicalPath(inputPath)
fsWrite.ts sanitize(params.path) in validate/invoke await resolveCanonicalPath(params.path)
fsReplace.ts sanitize(params.path) in validate/invoke await resolveCanonicalPath(params.path)
fsRead.ts sanitize(path) before readFile await resolveCanonicalPath(path)
fileSearch.ts sanitize(params.path) before the walk await resolveCanonicalPath(params.path)
listDirectory.ts sanitize(params.path) before the listing await 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-aware resolveCanonicalPath() at both its validation and its I/O site.

Every call site was already async, so awaiting the resolver required no signature changes.

Behavior after this change

  • The containment check and the operation derive the path the same way, through one shared helper.
  • Each tool operates on a concrete resolved target it computed, rather than handing a path to the OS to dereference later.
  • Paths containing ~ resolve consistently in the check and in the operation.

Testing

  • Build passes on Linux and Windows.
  • Existing unit suites cover all five tools (fsWrite.test.ts, fsReplace.test.ts, fsRead.test.ts, fileSearch.test.ts, listDirectory.test.ts, toolShared.test.ts).
  • fsRead.test.ts's after/afterEach hooks did not await tempFolder.delete()/tempFolder.clear(), so cleanup from one test could still be removing files while the next test wrote and read its fixtures. That surfaced as an ENOENT in reads multiple files once the tool gained additional await points before its read. The hooks now await, matching the four sibling suites which already did.
  • These tools now report and act on the resolved target rather than the link path. Assertions comparing a path string where the workspace or temp root is itself a symlink (for example macOS /tmp -> /private/tmp) will see the resolved value; content assertions are unaffected.

Base branch: feature/mcp-security-enchance.

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 dereferenced 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 `requiresPathAcceptance` and all five tools, so the
containment check and the operation derive the path the same way and each
tool acts on a concrete resolved target it computed.

`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.
The `after`/`afterEach` hooks called `tempFolder.delete()` and
`tempFolder.clear()` without awaiting them, so cleanup from one test could
still be removing files while the next test wrote its fixtures and read
them back. `reads multiple files` failed with ENOENT on a file it had just
written once the tool gained additional await points before its read.

The four sibling suites (fsWrite, fsReplace, listDirectory, fileSearch)
already await these calls; this makes fsRead consistent with them.
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 96.66667% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...src/language-server/agenticChat/tools/fsReplace.ts 88.88% 1 Missing ⚠️
...r/src/language-server/agenticChat/tools/fsWrite.ts 88.88% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

`handleCreate` reports the path it wrote to, which is now the resolved
path, so the assertion must compare against the canonical form rather than
the path the test passed in. On Windows `os.tmpdir()` yields an 8.3 short
name (RUNNER~1) that resolution expands to its long form (runneradmin),
which failed the deep-equal on that platform only.

Derives the expected value with `fs.realpath` rather than the tool's own
helper, so the assertion stays an independent check of the reported path.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants