Skip to content

fix(runtime): decide workspace path containment in one realpath space - #2059

Merged
Astro-Han merged 6 commits into
mainfrom
fix/runtime-symlink-cwd-containment
Aug 4, 2026
Merged

Astro-Han merged 6 commits into
mainfrom
fix/runtime-symlink-cwd-containment

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Summary

LocalWorkspaceExecutor decided path containment by comparing a realpath'd session cwd against a candidate that was only resolved. The two sides lived in different path spaces, so any session cwd under a symlink rejected every legitimate absolute path inside the workspace: a model that wrote by absolute path could not touch its own workspace. macOS hits this always (os.tmpdir() returns /var/folders/…, whose realpath is /private/var/folders/…), and so does anyone whose workspace root is reached through a symlink; Linux CI stayed green only because it happens to have no such symlink in the path.

The fix canonicalises the candidate before the check, through its deepest existing ancestor so a not-yet-created target still resolves its parent's symlinks. Both guarantees are kept and now fall out of a single rule instead of two staged checks:

  • the writable variant still resolves the parent and still rejects a parent that resolves outside the root;
  • normalisation never legalises "follow a symlink out of the workspace" — a link inside the cwd that points out of it resolves to its outside target and fails containment.

Canonicalisation and the containment assertion are separated so the write-lock key can share the former without the latter. Without that, data.txt and /var/…/link/data.txt produced two different keys for one file, so concurrent Edits took different locks and clobbered each other.

The containment error now prints both the caller's input and what it resolved to; previously it printed the realpath'd root next to the raw input, which is what made the failure hard to read.

realpathAllowMissing moves into path-containment.ts, the owner of the containment invariant, replacing the two existing copies in sandbox-boundary-path.ts and filesystem-worker/operations.ts.

Root cause

resolveExistingInsideCwd / resolveWritableInsideCwd computed root = await fs.realpath(cwd) but left the caller's absolute path unnormalised, then compared them with isPathInside.

Audit of the same class in packages/runtime: the sandboxed filesystem-worker's read path is already single-space — builtin-tools.ts canonicalises the cwd with realpathSync and the client rewrites operation.path to the normalised enforcementPath, so operations.ts compares two canonical paths (its write path had a separate hole, below). globFiles receives the resolved (canonical) base from resolveExistingPath, and grepFiles receives an absolute canonical search path, so neither needs its own normalisation. packages/headless's isolated-workspace adapter is a deliberate lexical-only preflight and compares two lexical paths, so it is unaffected. writeLockKey was the one remaining instance in the executor and is fixed here.

Security

Three containment holes are closed on top of the path-space fix. Each was reproduced before it was fixed.

Writing through a symlink out of the workspace. The writable resolver checked the parent directory but returned the unresolved leaf, so a symlink inside the workspace pointing out of it was followed by the write. Reads already rejected this. Now rejected for writes too — the caller-visible behaviour change on this PR.

Dangling symlinks. realpath fails ENOENT on a link whose target does not exist, so canonicalisation treated such a link as a plain missing leaf and returned the link's own path — which reads as contained, while the write lands on the link's target outside the workspace. realpathAllowMissing now reads and follows the link by hand. realpath reports a cycle as ELOOP, and the kernel caps its own symlink traversal well below the helper's hop cap, so the walk always terminates by rejecting. The other two consumers of the helper gain the same accuracy: a dangling link normalises to the path a write would actually reach, which is the path the sandbox boundary should approve.

The worker authorising an unresolved write target. resolveWritableAllowed in the filesystem worker realpaths the parent of a missing target and then authorises the unresolved candidate, so a dangling link inside the root passed its boundary check while the write landed outside. Reaching it takes a request whose boundary and expectedTarget disagree, which the in-repo client never sends — but the worker is a separate process that re-checks everything precisely so it does not have to trust its caller. It now authorises the followed path, the same one assertTargetUnchanged already pins the request to.

Collapsing the two resolvers also dropped the containment check the old existing-path variant ran on its final fs.realpath result; it is restored, with a comment saying why it must not be deleted. The candidate is already canonical, so the two can only diverge when a segment that was missing during canonicalisation becomes a symlink before the realpath — narrow, and no deterministic test can drive that race, so nothing fails if it is removed.

Verification

  • packages/runtime: npm run build && node --test "dist/**/*.test.js" → 3090 pass / 0 fail / 9 skipped on macOS. Before the fix: 3076 pass / 4 fail (the four containment tests in builtin-tools.test.ts).
  • New regression tests drive the contract through an explicitly created symlink rather than relying on macOS tmpdir behaviour, so they hold on Linux CI too: file tools stay usable when the session cwd is reached through a symlink (Read/Write/Edit/Glob/Grep accepted through the link; outside paths, an escaping file symlink, an escaping directory symlink and an escaping dangling symlink still rejected) and concurrent Edits through a symlinked cwd serialize on one key.
  • realpathAllowMissing gets direct unit tests in path-containment.test.ts (symlinked ancestor, missing trailing segments, dangling link, dangling chain, ENOTDIR, pathological chain, cycle → ELOOP); it is the canonicalisation authority for three subsystems and previously had none.
  • The worker's boundary enforcement gets denies a write through a dangling symlink the boundary does not cover in filesystem-worker.test.ts.
  • Every new test was confirmed to fail on the code it guards: reverting only the writeLockKey change re-breaks the lock test, and reverting only the dangling-link hop re-breaks three tests.
  • npm run build, npm run typecheck, npm run lint, npm run format:check → clean.
  • Consumers of the moved helper: npm run test -w @maka/headless → 1339 pass / 0 fail; npm run test -w @maka/desktop → 1585 pass / 0 fail.
  • Rebased onto the latest main (ef3905a3) and re-verified end to end.
  • Not run: Playwright E2E and the packaging/release checks — no renderer or packaging surface is touched.

The local workspace executor compared a realpath'd session cwd against a
merely resolved candidate path. Any session cwd under a symlink — macOS
tmpdirs (`/var` → `/private/var`), or a workspace root organised with
symlinks — therefore rejected every legitimate absolute path inside the
workspace, so a model writing by absolute path could not touch its own
workspace.

Canonicalise the candidate through its deepest existing ancestor before
the containment check, so both sides live in the realpath space and the
target may still be missing. Following the symlinks does not weaken
containment: a link inside the cwd pointing out of it now resolves to its
outside target and is rejected, including for Write. The write-lock key
gets the same canonicalisation, so relative and symlinked-absolute
spellings of one file no longer take two different locks.

`realpathAllowMissing` moves to path-containment.ts, the owner of the
containment invariant, and replaces the two copies in
sandbox-boundary-path.ts and filesystem-worker/operations.ts.
Collapsing the two resolvers dropped the check the old existing-path
variant ran on its final `fs.realpath` result. The candidate is already
canonical, so the two can only diverge when a segment that was missing
during canonicalisation becomes a symlink before the realpath — a narrow
race, but one the old code caught and the new code returned unchecked to
Read, Edit, FormatJson, Glob, and Grep.

Restore it by making the containment assertion a named helper both
call sites share.
`realpath` fails ENOENT on a symlink whose target does not exist, so
`realpathAllowMissing` treated such a link as a plain missing leaf and
returned the link's own path. A link inside the workspace pointing at a
not-yet-created path outside it therefore read as contained, and a Write
through it created the file outside the workspace — verified against the
local executor before the fix.

Read the link and follow it by hand instead. `realpath` reports a cycle
as ELOOP, so each hop consumes one existing link and the walk terminates;
the hop cap is a backstop, not the cycle guard. The two other consumers
of the helper gain the same accuracy: a dangling link now normalises to
the path a write would actually reach, which is the path the sandbox
boundary should approve.

Covers the helper with direct unit tests — it is now the canonicalisation
authority for three subsystems and had none of its own.
The filesystem worker resolves a missing write target by realpathing its
parent and then authorising the unresolved candidate. A dangling symlink
inside the root therefore passed its boundary check while the write
landed on the link's target outside the root — reproduced by handing the
worker a request whose boundary names only the workspace-internal link.

Reaching it takes a request whose boundary and expectedTarget disagree,
which the in-repo client never sends, but the worker is a separate
process that re-checks everything precisely so it does not have to trust
its caller. Authorise the followed path, the same one
`assertTargetUnchanged` already pins the request to.
…hains

Neither branch had a test. The chain case documents where the bound
actually comes from: the kernel caps its own symlink traversal well below
the helper's hop cap, so it answers ELOOP first and the hop cap is a
backstop for a filesystem that does not. The contract worth pinning is
that the walk terminates by rejecting, not which layer rejects.

Also corrects the module docstring: the leaf has not imported only
`node:path` for some time.
`writeLockKey` repeated the resolver's root/resolve/canonicalise sequence
verbatim. Identical today, so the keys match the resolved paths, but the
two spaces drifting apart is exactly what breaks write serialization
silently. Split the canonicalisation from the containment assertion so
the lock key uses the former and the resolvers add the latter.

Also records why the post-realpath assertion in the existing-path
resolver must not be deleted: it guards a race no deterministic test can
drive, so removing it breaks nothing visible.
@Astro-Han
Astro-Han force-pushed the fix/runtime-symlink-cwd-containment branch from 6fc1a7b to c2b4468 Compare August 4, 2026 02:19
@Astro-Han
Astro-Han marked this pull request as ready for review August 4, 2026 02:22
@Astro-Han
Astro-Han merged commit fab1245 into main Aug 4, 2026
10 checks passed
sunheyi6 added a commit to sunheyi6/maka-agent that referenced this pull request Aug 4, 2026
) conflicts

- sandbox-boundary-path: adopt main realpath-space model (no followFinalSymlink)
- builtin-tools ApplyPatch adapter: writes follow the canonical target (apache#2059),
  lstat/delete keep directory-entry semantics via realpathAllowMissing parents
- filesystem worker client: entry-mode normalizer for delete/lstat/create;
  writable roots back to main narrow dirname form
- operations: worker write resolves through followed path; delete/lstat operands
  stay entry-scoped with containment
- storage: drop FileSessionStore per main apache#2040; restore editingProtocol
  persistence in buildSessionHeader + normalizeSessionHeader
- tests: merge symlink suites (replace-mode now follow-final), keep PR lstat/
  delete entry tests, add editingProtocol persistence regression
sunheyi6 added a commit to sunheyi6/maka-agent that referenced this pull request Aug 4, 2026
…tingProtocol persistence

- filesystem-worker: create-mode parents test uses mode create (plain writes
  now require the parent in realpath space per apache#2059); replace-mode symlink
  test asserts the canonical target is written
- filesystem-worker-client: writable-roots test back to main narrow dirname
  form; entry normalizer resolves auto scope; drop symlink pin comparisons
- storage: buildSessionHeader/normalizeSessionHeader persist and validate
  editingProtocol; SQLite session-store regression test
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