The VFS canonicalizes paths, so containment stops being each caller's job - #434
Merged
Conversation
readlink -f and realpath each walked a path component by component, calling ctx.backend.lstat/read_link and building the next hop from the raw symlink target, then re-dispatching the accumulated VFS-absolute path through the mount table on every hop. A relative symlink target with enough ../ hops to walk above its own mount's root got re-routed to whatever mount covered the folded path instead of being refused - not a host escape when the mount stack was simple, but a real cross-mount leak once a second mount (most commonly the / mount every embedder has) covered the escaped path. kaish already ships kaish_vfs::resolve_beneath with containment built in; the bug was that the readlink walk never reached it. Added Filesystem::canonicalize (kaish-vfs) and KernelBackend::- canonicalize (kaish-tool-api) as defaulted trait methods, following the path_access precedent: the default is the generic per-component walk (moved, not copied, from readlink.rs), correct for a backend with no root to enforce. LocalFs overrides it with one resolve_beneath call; resolve_beneath's Follow::Final does not itself distinguish a missing final component from a missing intermediate one, so the override checks the resolved answer's parent separately to keep that distinction. VfsRouter overrides it to delegate to the single mount that owns the path instead of re-entering the mount table per hop - the actual fix for the escape, since the old bug was router-level re-routing, not a missing check inside any one backend. VirtualOverlayBackend (the wrapper Kernel::with_backend puts around every embedder backend) needed the same override. Without it, a kernel built with with_backend - which is how router_mount_ancestor_- tests.rs's own fixture, and every real embedder, constructs a kernel - still walked hop by hop through lstat/read_link and reproduced the exact bug canonicalize was built to close. A regression test mounts a rooted LocalFs alongside a / mount holding a marker file at the path 20 levels of ../ would land on if a hop ever re-routed; before this override it printed the marker's path, after it refuses with "path escapes root" or "No such file or directory". readlink -f and realpath are now thin callers of ctx.backend.canonicalize (true and false respectively for allow_missing_final - realpath needed no separate stat call to enforce full existence, canonicalize's own contract covers it). readlink.rs dropped from 451 to 258 lines; realpath.rs from 189 to 164. The conformance suite (crates/kaish-vfs/src/conformance.rs) gained two cases, run against LocalFs, MemoryFs, and OverlayFs: an escaping symlink either refuses or, for an unrooted backend with no boundary to enforce, resolves to an ordinary in-namespace path with no residual ..; and a missing path component is tolerated only when it is the final one and the caller asked for that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ainment case The escaping-symlink case accepted any error as a pass, on the reasoning that an error cannot leak a path. That reasoning is sound and the case was still too permissive: a backend that never implemented `canonicalize` returns Unsupported, which is an error, so the case passed without the backend deciding anything about containment. A conformance case exists to make a backend prove a guarantee. One that a backend can satisfy by not having the feature proves the opposite. Unsupported now fails the case and says why. Every other error still passes, because refusing is the correct answer for a rooted backend and no answer can leak.
…uite These eight tests came from the first fix for the kaibo report, which patched readlink's own path walk. That fix is gone: the ancestor half became the router's, the containment half became the VFS's. The tests outlived it. They are worth keeping because of where they sit. The conformance cases ask a backend whether it contains an escaping symlink, and the router tests ask whether an ancestor resolves -- both at a trait boundary, with a fixture built for the question. These run `readlink -f` and `realpath` through a real kernel in the exact shape a user reported: LocalFs read-only at a deep mount path mirroring its own host path, MemoryFs at `/`, cwd inside the mount. A guarantee that holds at the trait and not through the kernel is not a guarantee anyone can use. All eight pass with no readlink-side resolution code present, which is the claim worth pinning: the fixes are structural, and the builtin inherits them. The escaping-link fixture is named `outward.txt` rather than `escape.txt`. readlink formats a failure as `readlink: <operand>: <message>`, so an operand containing "escape" would put that word in every possible failure, including ones that mean the opposite of containment working.
tobert
force-pushed
the
feat/vfs-canonicalize
branch
from
September 2, 2026 13:26
210028e to
164f49e
Compare
Merged
tobert
added a commit
that referenced
this pull request
Sep 2, 2026
Version bump and changelog stamp for v0.17.1, a patch release covering six PRs merged since v0.17.0: help/kaish-tools nested-subcommand recursion (#430), the wrapped-command allow_external_commands framing correction (#431), a changelog correction plus a new zero-padded date/time migration note (#432), nested verb groups for wrapped commands (#433), mount-point ancestor navigation when a backend also covers `/` (#435), and VfsRouter-shared path canonicalization closing a containment leak in `readlink -f`/`realpath` (#434). This bump also carries two documentation fixes surfaced by the release-gate review below rather than opening a separate PR for text-only changes: `docs/EMBEDDING.md` claimed `realpath` passes `allow_missing_final: true` and rechecks existence, when it actually passes `false` directly; and the canonicalize changelog entry overstated the default implementation as containment-checked, when containment is a property of `LocalFs`'s and `VfsRouter`'s overrides, not the shared default. Reviewed with kaibo (`consult`, cast `deepseek`) against the full `v0.17.0..HEAD` diff. Verdict: no undocumented semver breaks — the two new `canonicalize` trait methods are defaulted and every changed public type is either `#[non_exhaustive]` or privately fielded, so the patch framing holds. Two smaller findings from that review are real but scoped as code changes rather than release-blocking text, so they're queued as follow-up work rather than folded into this bump: a wrapped-command node can silently accept a no-op `json_output` declaration instead of being refused, and the new `canonicalize` default's symlink-hop cap has thin test coverage. Gates: `cargo test --all` (2231 passed), `cargo clippy --all --all-targets -- -D warnings` (clean), `cargo insta test --check` (no pending snapshots).
tobert
added a commit
to tobert/kaibo
that referenced
this pull request
Sep 2, 2026
0.17.1 carries the fix that has been blocking kaibo v0.4.0: `readlink -f` and `realpath` failed on **every** operand on a rooted mount, with a message naming neither the operand nor its target (`readlink: o-exists: No such file or directory: /tmp`). Both resolve now, and an escape refuses by name. We reported that shape upstream during the 0.17.0 bump; it became two structural kaish PRs rather than a patch — [#435](tobert/kaish#435) (the router's mount-ancestor synthesis was unreachable whenever `/` is mounted) and [#434](tobert/kaish#434) (`Filesystem::canonicalize` as a defaulted trait method, moving containment out of the builtin into the VFS). ## The compiler found nothing, so the shell was the check No API break reached kaibo — `Filesystem::canonicalize` is defaulted and kaibo implements no `Filesystem` or `KernelBackend`. The composed tool contract is byte-identical under both pins (throwaway crate calling `compose(&Recipe::tool_description(), …)`), and so are all eleven `help` surfaces kaibo renders, 43941 bytes each side. So the check was the shell, per the rule the 0.17.0 bump wrote into AGENTS.md. Every battery in `docs/sandbox-probes.md` was run against **both** a 0.17.0 and a 0.17.1 binary and diffed. A, B, D, E, F and G came back identical. Two changes are the whole delta a model can see. **1. The release blocker is fixed.** `readlink -f` / `realpath` resolve an in-tree path (exit 0) and refuse an escape by name. Battery G3 re-run on the new canonicalize path: existing, missing, and unreadable targets still refuse byte-identically, so the new code introduces no existence oracle. **2. The directories above the mount list again — new, and accepted.** 0.17.0 answered "not found" for every directory above the project; 0.17.1 restores them as synthesized directories. It is synthesis from kaibo's own mount table, not a host read: each level names only the next component down to the project, so a model walking up recovers the root path string the caller already handed it and nothing else. Counted rather than argued — `ls /tmp` returns one entry where the host `/tmp` holds 3575. Adjacent secrets, real siblings, and the state db and media CAS all stay invisible (E2/F2 re-run against a non-empty store: 4 KiB db, 210 CAS objects, both `not found`). That observable is pinned by a new containment test with a **recorded positive control** — move the mount up and the leak assertion fires — because a battery where everything comes back empty proves nothing otherwise. Battery C's claim that `/home` is "an inert stub that cannot be walked" is now false and is corrected in place, the same way the 0.17.0 bump corrected three other criteria. ## The probe caught itself once E1 run without `--root` created a state db, because the fixture was then outside every allowed tree and the guard correctly did not fire. Re-run with the root pointing at the fixture, both E1 and F1 refuse loudly and create nothing. The §0 question — *would this read differently if the probe were broken, versus if the thing it audits were broken?* — is what found it. ## Gates - `cargo clippy --all-targets`: clean. - `cargo test`: 1147 passed. The lone failure is the known `tests/credentials.rs` ETXTBSY exec race under parallelism (green serially, reproduces on unmodified code). - containment: 25 passed, one new. - `cargo tree -i` empty for `aws-lc-rs`, `mimalloc`, `openssl-sys`. Cross-family review posted as a comment below. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
1 participant
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.
readlink -fwalked a path itself —lstatandread_linkper component, following each hop and handing the next constructed path back to the router. That walk never reached the containment the owning backend would have applied, so a symlink pointing outside a rooted mount resolved and printed the outside path.realpathshared the walk and the hole.The escape is the symptom. The defect is a builtin owning a second implementation of a guarantee the VFS already ships as
resolve_beneath.Filesystem::canonicalize(path, allow_missing_final)is now a defaulted trait method, the same shapepath_accesstook in 0.17.0, so existing backends keep compiling.LocalFsoverrides it with oneresolve_beneathcall and inherits containment rather than restating it.VfsRouterdelegates to the mount that owns the path. The default is the generic per-hop walk, moved out ofreadlink.rs, for a backend with no root to enforce.readlink.rsandrealpath.rslost 230 lines and gained 12. They are callers now.Two things this turned up that reasoning did not.
VirtualOverlayBackendhad no override, andKernel::with_backendis the constructor every real embedder uses — without it the call fell through to the per-hop default and re-created the routing bug the change exists to close. Separately, a single-backend escape was already contained, becauseLocalFs::lstatandread_linkeach callresolve_beneathindependently; the override buys efficiency there, and the correctness bug was specifically router and overlay re-routing across mounts.The conformance suite goes from 20 cases to 22, so LocalFs, MemoryFs, and OverlayFs each prove containment and the missing-component contract against themselves. A guarantee proved per backend outlives a guarantee proved per caller.
Stacked on
fix/router-synthesizes-mount-ancestors. The eight tests from the original kaibo report ride along as an end-to-end suite and pass with no readlink-side resolution code present.Gates:
clippy -D warnings,cargo test --all, conformance across three backends,insta --check,no-default-features, rustdoc-D warnings. All clean.