Skip to content

fix(sdk): resolve file paths before checking containment - #23

Merged
matej21 merged 3 commits into
mainfrom
fix/filesystem-path-containment
Aug 31, 2026
Merged

fix(sdk): resolve file paths before checking containment#23
matej21 merged 3 commits into
mainfrom
fix/filesystem-path-containment

Conversation

@matej21

@matej21 matej21 commented Aug 28, 2026

Copy link
Copy Markdown
Member

SessionFileStore validated 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_modules and 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 only realPath() 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_file on an image returns a file:// 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 through containedPath(), 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 remove asks about the parent, since unlink acts 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:

  • lstat added to the FileSystem platform 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 through access, which follows links, so a broken link reads as absent). Two conformance checks pin the clause the containment logic leans on — that lstat answers for a link whose target is gone, where realpath rejects — plus a violations case for the obvious wrong implementation, an lstat that is just stat.
  • Listing a root asks nothing extra: a path that is the root is inside it whatever it resolves to, so that case short-circuits before any realpath. That keeps the common directory-listing entrypoint at its previous syscall count.
  • Dropped a dead isSymbolicLink() branch in FileStore.stat() — the value came from a link-following stat, so it could never be true. A link to a file is reported as a file, which is what the tools want.
  • The plugin's directory-listing RPC methods reach the filesystem outside the store, so they got the same real-target check.

Known follow-ups, deliberately out of scope here: checkDeniedPaths is 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.ts and transport/http/path-containment.ts are now near-duplicates worth unifying, and plugins/shell/executor.ts's checkSymlinkEscape is a third copy whose catch { return false } treats an unresolvable path as contained — weaker than containmentOf, though masked in its own flow by a following existence check; hardlinks are invisible to realpath by construction, so a hardlink inside the root to a file outside defeats containment (bounded by fs.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.lstat and FileStore.containedPath are both newly required, and containedPath is async where realPath was 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

@matej21
matej21 force-pushed the fix/filesystem-path-containment branch from 7d7b411 to c30d6a9 Compare August 28, 2026 16:14
@matej21
matej21 marked this pull request as draft August 28, 2026 16:42
matej21 and others added 2 commits August 28, 2026 18:56
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
matej21 force-pushed the fix/filesystem-path-containment branch from c30d6a9 to 7880050 Compare August 28, 2026 16:57
@matej21
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
@matej21
matej21 merged commit 833f5ae into main Aug 31, 2026
1 check passed
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.

1 participant