fix(sdk): resolve file paths before checking containment - #23
Merged
Conversation
matej21
force-pushed
the
fix/filesystem-path-containment
branch
from
August 28, 2026 16:14
7d7b411 to
c30d6a9
Compare
matej21
marked this pull request as draft
August 28, 2026 16:42
The adapter had `stat` and `realpath` but no way to ask about a path without following the link it may be. Both the Bun adapter and the test-only Node adapter map it straight onto `node:fs/promises`. Required rather than optional, like `stat`: it is a primitive any host with a filesystem can answer, and a caller that had to work without it would lose the one clause it is here for — a link whose target is gone answers `lstat` where it rejects `realpath`, which is how "not there" is told apart from "there but unresolvable". Being required, it is a compile break for any platform adapter outside this repo, which has to add the one line. The conformance suite gets that clause as two checks, one of them gated on `fs.symlinks`, plus a platform that breaks it by letting `lstat` follow the link — caught by the check that names it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbVWntVv8xSdtn6DC6ZApM
SessionFileStore validated paths lexically and left the real check to its callers. The HTTP routes did it; nothing else did, so a path that passed the lexical check could still land outside the configured roots. The check now lives in the store, where every operation goes through it: resolve lexically as before, then confirm the canonical target is still inside the area the store may touch. Links are not banned — one whose target stays inside keeps working, which pnpm stores, node_modules and jj all depend on. A path that does not exist yet is judged by its deepest existing ancestor, so writing a new file only needs its parent to be contained. That area is session and workspace together, not whichever root the path happened to match lexically: uploads land in the session directory and linking one into the workspace is an ordinary thing to do. What an unresolvable link means is the operation's to say. `write` refuses one, because it would follow the link and create the target outside; the operations that only follow it are left alone and fail on their own, so a broken link is absent to `exists` rather than an error. `remove` unlinks the entry rather than its target, so containment there is the parent's to answer and cleaning up a dangling link still works. Callers that reach the filesystem themselves get `containedPath()`, which applies the same check. The image processor needed it most: it re-resolves a `file://` URL out of conversation history on every later inference, and what that path names can have changed since the tool first read it. `realPath()` stays lexical, now documented as being for paths that never reach the filesystem — the two HTTP routes, which follow it with their own canonical check, are the only callers left. Also drops a dead `isSymbolicLink()` branch in `stat()` — the result came from a link-following `stat`, so it could never be true — and extends the check to the plugin's directory-listing RPC methods, which reach the filesystem outside the store. Listing a root asks nothing extra: a path that is its own root is inside it whatever it resolves to. Below the root the check canonicalizes both ends, which the walk/loop parity test now states as the cost it is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbVWntVv8xSdtn6DC6ZApM
matej21
force-pushed
the
fix/filesystem-path-containment
branch
from
August 28, 2026 16:57
c30d6a9 to
7880050
Compare
matej21
marked this pull request as ready for review
August 28, 2026 16:58
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CbVWntVv8xSdtn6DC6ZApM
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.
SessionFileStorevalidated paths lexically and left the real check to its callers. The HTTP file routes did it; the filesystem plugin's tools did not, so a path that passed the lexical check could still resolve outside the configured root.The check now lives in the store, where every operation goes through it: resolve lexically as before, then confirm the canonical target is still inside the canonical root. Both ends are canonicalized, so a root that is itself reached through a link keeps working.
Symlinks are not banned. A link whose target stays inside the root keeps working — pnpm stores,
node_modulesand jj all rely on that. A path that does not exist yet is judged by its deepest existing ancestor, so writing a new file only requires its parent to be contained. A component that exists but cannot be resolved (dangling link, link loop) is rejected.A caller outside the store now has an API for the same guarantee:
FileStore.containedPath()applies the store's own check and returns the resolved path. That was the structural gap —realPath()was the only resolver such a caller could reach, so "do the check yourself" was advice with nothing behind it. Every path-resolving caller was audited and converted; the onlyrealPath()uses left are two HTTP routes whose paths the server builds and which canonicalize themselves.realPath()'s doc now says it is for paths that never reach the filesystem.The most consequential conversion is the image path.
read_fileon an image returns afile://URL carrying the caller's path, which is re-resolved on every later inference and its bytes handed to the model — so a lexical resolve there let content from outside the root reach the prompt. It now resolves throughcontainedPath(), with a regression test that swaps a real image for a link out of the session and asserts the outside bytes never appear.Containment accepts the full allowed area rather than whichever root matched lexically: a link from the workspace to a file in the session, or the reverse, keeps working — uploads land in the session directory and linking one into the workspace is an ordinary move.
scoped()inherits its parent's set, so a sub-store cannot escape the root it was carved out of.Resolution takes an explicit mode, so an unresolvable link is judged by what the operation will do with it: reads and stats pass it through (the operation follows the same link and fails the same way), writes refuse it (following it would create the target outside), and
removeasks about the parent, sinceunlinkacts on the link itself. That keeps upload cleanup able to delete a dangling link, which the first version of this change broke.Also in this PR:
lstatadded to theFileSystemplatform interface, implemented in the Bun adapter and the test-only Node adapter. It is required rather than an optional verb: a host that omitted it would need a fallback, and the only available fallback is the dangling-link hole this guards (exists()goes throughaccess, which follows links, so a broken link reads as absent). Two conformance checks pin the clause the containment logic leans on — thatlstatanswers for a link whose target is gone, whererealpathrejects — plus a violations case for the obvious wrong implementation, anlstatthat is juststat.realpath. That keeps the common directory-listing entrypoint at its previous syscall count.isSymbolicLink()branch inFileStore.stat()— the value came from a link-followingstat, so it could never be true. A link to a file is reported as a file, which is what the tools want.Known follow-ups, deliberately out of scope here:
checkDeniedPathsis still a lexical test on the supplied path (containment holds regardless, but the deny-list itself can be dodged by a link);core/file-store/containment.tsandtransport/http/path-containment.tsare now near-duplicates worth unifying, andplugins/shell/executor.ts'scheckSymlinkEscapeis a third copy whosecatch { return false }treats an unresolvable path as contained — weaker thancontainmentOf, though masked in its own flow by a following existence check; hardlinks are invisible torealpathby construction, so a hardlink inside the root to a file outside defeats containment (bounded byfs.protected_hardlinks); the canonical roots are re-realpath'd on every call and could be cached per store instance; and check-then-use remains two syscalls, the same window the existing HTTP-route check has.Breaking for external implementors:
FileSystem.lstatandFileStore.containedPathare both newly required, andcontainedPathis async whererealPathwas sync — an implementor cannot alias it. Acceptable at 0.1.x, but it belongs in the release notes.Tests: unit coverage in
core/file-store/file-store.test.ts(link inside the root allowed, link out rejected for read/write/list, new nested file allowed, scoped sub-store confined) and tool-level coverage through the filesystem plugin's integration test.🤖 Generated with Claude Code
https://claude.ai/code/session_01CbVWntVv8xSdtn6DC6ZApM