test(client): Agent Skills — root-swap races the pinned handle does not close (SEC-8985 row 2) by pkaeding · Pull Request #49 · launchdarkly/js-ai-sdk

pkaeding · 2026-09-04T16:13:57Z

What

Three failing tests in a new file, packages/client/src/__tests__/skills-fs-root-swap.test.ts, stacked on #48. They state the contract the SDR response gives for SEC-8985 row 2 and show the code does not meet it. No implementation change in this PR.

The gap

resolveRoot validates the root once and returns a path. Nothing holds it open. Each write and each prune then opens <root>/<key> by path with O_NOFOLLOW | O_DIRECTORY and pins that handle. O_NOFOLLOW guards only the final component, so the root is re-resolved on every such open. Swap the root for a symlink after validation and the open follows it into the attacker's directory. assertUnswapped does not notice, because it compares the handle against an lstat of the same swapped path, so the two agree. atomicWriteIn does refuse the manifest rewrite afterwards, but by then the skill file is already outside the root.

The two <root>/<key> swap races in skills-fs.test.ts are skipped off SUPPORTS_DIR_FD because Node cannot close that window. These are not skipped. Their point is that the documented mitigation for Node's residual exposure, the README privilege-separation checklist, is not sufficient either: it denies the agent identity the root, the skill directories, the files and the manifest, and says nothing about the root's parent. In the documented layout (<app>/.claude/skills) that parent is .claude, which the agent identity typically owns, and write permission there is all this takes.

What the tests show

Test	Swap fires at	Result on this branch
create	`mkdir(<root>/a)`	`SKILL.md` written into `<outside>/a/`
clobber	first `O_NOFOLLOW` open of `<root>/a`	`<outside>/a/SKILL.md`, never in the manifest, replaced with served content
prune	second `O_NOFOLLOW` open of `<root>/a` (the first is the orphan sweep's, and `pruneOne` re-runs the realpath check after it)	`<outside>/a/SKILL.md` unlinked, then `<outside>/a` removed by the `rmdir`

vitest run src/__tests__/skills-fs-root-swap.test.ts in packages/client:

× a root swapped at the skill directory create cannot redirect the write
    AssertionError: expected [ 'a' ] to deeply equal []
× a root swapped at the skill directory open cannot clobber an outside file
    AssertionError: expected 'served update\n' to be 'precious\n'
× a root swapped at the prune cannot redirect the unlink
    AssertionError: expected false to be true      // exists(victim)

The swap has to land before the per-skill directory is opened, which is earlier than the fsOps rename/unlink hook the other race tests use, so mkdir and open from node:fs/promises are wrapped with a file-wide vi.mock (pass-through until a test arms one of them for one exact path). That is why this is its own file. Nothing in the implementation is touched.

Why the build is red

Deliberately. The tests assert the contract, the code violates it, and the failing run is the demonstration. They go green when the fix below lands, with no wrapper to remove. Rest of the client suite on this branch: 646 passed, 3 failed (these three), 2 skipped (the existing SUPPORTS_DIR_FD races); biome check clean.

The fix (not in this PR)

Node has no *at() family, but Linux has a standard workaround: hold the root FileHandle and address children through /proc/self/fd/<handle.fd>/<name>. The kernel resolves that prefix to the pinned inode, so a mkdir, open, rename or unlink spelled that way cannot be redirected by a swap of the root's name. It does not exist on macOS or Windows, so it would be a Linux fast path behind a probe with the current assertUnswapped floor elsewhere; server-side agents run on Linux, which covers the deployments that matter. If the fix is not taken, the minimum is adding the root's parent to the README verification checklist and changing the "held for the whole reconcile" sentence in the response and the design-doc comment to "the skill directory is pinned per operation".

Companion PR with the same three tests for the Python SDK: launchdarkly/python-ai-sdk#68.

🤖 Generated with Claude Code

via LD Research 🤖

SEC-8985 row 2. `resolveRoot` validated the managed root and returned a path; nothing held it open. Every write and prune then opened `<root>/<key>` by path with `O_NOFOLLOW`, which guards only the final component, so the root was re-resolved on each one and a root swapped for a symlink after validation redirected the open into the attacker's directory. `assertUnswapped` agreed with itself, because it lstat'd the same swapped path the handle came from. Only the manifest write pinned the root, and by then the skill files were already outside it. The precondition is write access to the root's *parent* — `.claude` for a root of `.claude/skills`, which the agent identity typically owns. The root is now opened `O_RDONLY|O_DIRECTORY|O_NOFOLLOW` once, fstat'd, and held for the whole reconcile, and every child is addressed as `/proc/self/fd/<fd>/<name>`. The kernel resolves that prefix to the pinned inode rather than to the name, which gives it the property an `*at()` call would have; the per-skill directory is pinned in turn for the files inside it. Gated on a new `SUPPORTS_PROC_FD` probe (Linux plus a usable `/proc/self/fd`, verified at load by resolving a live descriptor back to its own inode), so macOS and Windows keep the per-component lstat floor unchanged — `assertUnswapped` is skipped only for a directory the caller addressed through a descriptor, which is why the floor's own tests still exercise it on Linux. Containment checks in `unsafePathReason` stay as defense in depth; they are no longer the boundary here. A swap landing before the pin fails the open and is reported as a run-level error rather than thrown, since the root was usable when the caller named it. `atomicWriteIn` is gone: the root is always pinned by the caller now, so a primitive whose job was to re-open a directory by path is exactly the footgun this bug was made of. Descriptor addresses are stripped out of `errno` messages, which leaves the offending file named relative to whichever directory was pinned. Tests: the two `<root>/<key>` races in skills-fs.test.ts now run wherever either capability probe is true, so they are live on Linux. The root-swap file from Security's PR #49 keeps its assertions; its trigger matches the final component plus the shape of its parent rather than one exact path, so the same bodies fire the swap against both the old and new addressing — pinning the trigger to `path.join(root, key)` would have made them pass vacuously the moment the fix landed. Verified in a Linux container: all four fail against the pre-fix code with real assertion failures (the write escapes, the outside victim is clobbered, the outside victim is unlinked) and pass against this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…es not close Three expected-failure tests (skills-fs-root-swap.test.ts) for SEC-8985 row 2. resolveRoot validates the root once and returns a path; nothing holds it open. Each write and prune re-opens <root>/<key> by path with O_NOFOLLOW, which guards only the final component, so a root swapped for a symlink after validation redirects the open into the attacker's directory. assertUnswapped does not notice, because it lstat's the same swapped path the handle came from. Precondition is write permission on the root's parent, which the README privilege-separation checklist does not mention, so the documented mitigation for Node's residual exposure does not cover this. The tests state the contract (nothing lands outside the root, no outside file is overwritten, no outside file is removed) under a knownGap() wrapper: pytest's strict xfail narrowed to assertion errors, which it.fails is not, so a harness breakage is a real failure and the wrapper fails the run the day the body passes. Separate file because the swap must land before the per-skill directory is opened, which means intercepting mkdir/open from node:fs/promises with a file-wide vi.mock. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

The expected-failure marker made the suite green while the contract was violated. A red run is the demonstration: the tests assert the contract, the code does not meet it, and they go green when the root is pinned for the reconcile, with nothing to remove. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

SEC-8985 row 2. `resolveRoot` validated the managed root and returned a path; nothing held it open. Every write and prune then opened `<root>/<key>` by path with `O_NOFOLLOW`, which guards only the final component, so the root was re-resolved on each one and a root swapped for a symlink after validation redirected the open into the attacker's directory. `assertUnswapped` agreed with itself, because it lstat'd the same swapped path the handle came from. Only the manifest write pinned the root, and by then the skill files were already outside it. The precondition is write access to the root's *parent* — `.claude` for a root of `.claude/skills`, which the agent identity typically owns. The root is now opened `O_RDONLY|O_DIRECTORY|O_NOFOLLOW` once, fstat'd, and held for the whole reconcile, and every child is addressed as `/proc/self/fd/<fd>/<name>`. The kernel resolves that prefix to the pinned inode rather than to the name, which gives it the property an `*at()` call would have; the per-skill directory is pinned in turn for the files inside it. Gated on a new `SUPPORTS_PROC_FD` probe (Linux plus a usable `/proc/self/fd`, verified at load by resolving a live descriptor back to its own inode), so macOS and Windows keep the per-component lstat floor unchanged — `assertUnswapped` is skipped only for a directory the caller addressed through a descriptor, which is why the floor's own tests still exercise it on Linux. Containment checks in `unsafePathReason` stay as defense in depth; they are no longer the boundary here. A swap landing before the pin fails the open and is reported as a run-level error rather than thrown, since the root was usable when the caller named it. `atomicWriteIn` is gone: the root is always pinned by the caller now, so a primitive whose job was to re-open a directory by path is exactly the footgun this bug was made of. Descriptor addresses are stripped out of `errno` messages, which leaves the offending file named relative to whichever directory was pinned. Tests: the two `<root>/<key>` races in skills-fs.test.ts now run wherever either capability probe is true, so they are live on Linux. The root-swap file from Security's PR #49 keeps its assertions; its trigger matches the final component plus the shape of its parent rather than one exact path, so the same bodies fire the swap against both the old and new addressing — pinning the trigger to `path.join(root, key)` would have made them pass vacuously the moment the fix landed. Verified in a Linux container: all four fail against the pre-fix code with real assertion failures (the write escapes, the outside victim is clobbered, the outside victim is unlinked) and pass against this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…8985 row 2) (#51) Closes the root-swap window in the Agent Skills filesystem layer. - **SEC-8985 row 2** - Supersedes #49 (Security's expected-failure tests, cherry-picked here with `-x` so authorship is preserved). **Please close #49 as superseded** — I have deliberately left it open for you. - Security review response: https://launchdarkly.atlassian.net/wiki/spaces/PD/pages/5293965360 **The first commit is Security's tests and CI was red on it** ([run](https://github.com/launchdarkly/js-ai-sdk/actions/runs/33909786540) — three failures, real assertion failures rather than harness errors). The head of this branch is green. ## The gap `resolveRoot` validated the managed root and returned a path; nothing held it open. `writeThroughPinnedDirectory`, `pruneOne` and `sweepSkillDirectory` then opened `<root>/<key>` by absolute path with `O_NOFOLLOW`, which guards only the **final** component — so the root was re-resolved on every one of those opens, and a root swapped for a symlink after validation redirected the open into the attacker's directory. On the create path `mkdir(<root>/<key>)` followed it too. `assertUnswapped` did not notice, because it compared the handle against an `lstat` of the *same swapped path* the handle came from, so the two agreed. Only the manifest write pinned the root, and by then the skill files were already outside it. The attacker precondition is write access to the root's **parent** — `.claude` for a root of `.claude/skills`, which the agent identity typically owns. The README's privilege-separation checklist did not mention ancestors, so the documented mitigation for Node's residual exposure did not cover this. ## The fix 1. New `SUPPORTS_PROC_FD` probe in `safe-fs.ts`: true on Linux when `/proc/self/fd` is usable, verified at module load by opening a descriptor and confirming the magic symlink resolves back to that descriptor's own inode. A genuine probe, not a platform string, for the same reason `SUPPORTS_DIR_FD` is one — the swap-race tests are gated on it. 2. `writeSkills` opens the root `O_RDONLY|O_DIRECTORY|O_NOFOLLOW` immediately after `resolveRoot`, `fstat`s it, and holds it for the whole reconcile (closed in `finally`). Threaded through `writeAll`, `pruneEntries`, `rewriteManifest`, `sweepOrphanTemps`, `loadManifest` and the per-skill helpers as a `PinnedDirectory`, which carries both the real path (for reports, manifest keys and containment checks) and the address (for filesystem calls). 3. On the fast path every child is addressed as `/proc/self/fd/<fd>/<name>` for `mkdir`, `open`, `rename`, `unlink`, `rmdir`, `lstat`, `readdir` and the manifest write. The kernel resolves that prefix to the pinned inode, so a swap of the root's *name* cannot redirect the operation. The per-skill directory is pinned in turn for the files inside it. 4. Off the fast path, path-based operations and the `assertUnswapped` floor are unchanged. `assertUnswapped` is skipped only for a directory the caller addressed through a descriptor — driven by how the caller addressed it rather than by a global flag, which is what keeps the floor's own tests in `safe-fs.test.ts` exercising it on Linux too. 5. `unsafePathReason` containment checks stay as defense in depth. `realpath` collapses a descriptor address to the real location, so the comparison reads identically on both paths. A swap that lands *before* the pin fails the open and is reported as a run-level `error` action rather than thrown: `resolveRoot` throws because an unusable root is a caller mistake, whereas a root that was a real directory an instant ago is an attack in flight. ## Beyond the brief Two things I did not find in the ticket but that follow from it: - **`atomicWriteIn` removed.** It existed to re-open the root by path for the manifest write; with the root pinned for the whole reconcile that is both redundant and exactly the footgun this bug was made of. `rewriteManifest` now writes through the held handle. Its one direct test went with it — the property it covered (a symlinked root is refused) is now covered by the new root-pin refusal test. - **Descriptor addresses stripped from `errno` messages.** Otherwise an operator reads `open '/proc/self/fd/23/a/SKILL.md'`. Stripping the prefix leaves the file named relative to whichever directory was pinned, which is the form the rest of the messages already use. ## Tests Security's assertions are unchanged. Their trigger was not, and this matters: the hook fired the swap only when the intercepted `mkdir`/`open` argument *strictly equalled* `path.join(root, 'a')`. After the fix the code passes `/proc/self/fd/N/a`, so the trigger would never have matched and all three tests would have passed vacuously. The trigger now matches on the final component plus the shape of its parent — the root's own path **or** a descriptor prefix — so one test body fires the swap against either implementation, which is the only thing that makes the red-to-green transition evidence of anything. - Gated `describe.skipIf(!SUPPORTS_PROC_FD)`: runs on Linux CI, skipped on macOS dev machines. - Added a fourth case: a root swapped before the pin is refused with a run-level error. - The two existing `<root>/<key>` races in `skills-fs.test.ts` now run when either capability probe is true, so they are live on Linux for the first time. - Added a probe test asserting `SUPPORTS_PROC_FD` reflects the real property (never true off Linux; on Linux only when the descriptor resolves to its own inode). - `interceptRename`/`interceptUnlink` resolve descriptor addresses back to real paths at interception time, while the descriptor is still open — four existing tests assert on where an operation landed. **Verified against the pre-fix code in a Linux container** (`docker run --rm -v "$PWD":/w node:24`), with both source files reverted to the base and only the probe shimmed in: all four fail with real assertion failures — the write escapes into `outside`, the outside `SKILL.md` is clobbered with served content, the outside victim is unlinked. All four pass against this branch. Local: `yarn test` green (652 client tests; 646 + 6 skipped on macOS, all 652 on Linux), `biome check` clean, `yarn typecheck` clean, `sherif` clean. ## Docs `packages/client/README.md`: the privilege-separation checklist and its shell snippet now cover the root's parent and every ancestor (the snippet walks up to `/`), and the *Security posture* paragraph no longer claims the Python SDK is alone in holding a descriptor for the whole reconcile or that Node's floor runs on every platform. It now splits by platform: Linux holds the root handle and addresses children through `/proc/self/fd`; macOS and Windows keep the per-component lstat floor, where write permission on the root **and now its ancestors** is the security boundary. ## Not in this PR The companion Python fix is being done separately in `python-ai-sdk`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Closes a **TOCTOU hole** where swapping the managed skills root for a symlink after `resolveRoot` could redirect `writeSkills` writes and prunes outside the intended directory (attackers need write access on the root’s **parent**, e.g. `.claude` for `.claude/skills`). > > **Linux:** `writeSkills` now opens and holds the managed root for the entire reconcile and routes child operations through **`/proc/self/fd/<fd>/<name>`** via a new `SUPPORTS_PROC_FD` probe and `directoryAddress` / `PinnedDirectory` (real paths for reports, descriptor addresses for syscalls). **`atomicWriteIn` is removed**; the manifest write uses the pinned root. **`assertUnswapped`** is skipped when paths are already descriptor-relative. > > **macOS / Windows:** Behavior stays on the per-component `lstat` floor; **README** now documents that the security boundary includes **ancestor directories**, with an expanded privilege-separation checklist and shell snippet. > > **Tests:** New Linux-gated root-swap race suite; existing swap-race and intercept tests updated so triggers match both path and proc-fd spellings and assertions still resolve real paths. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 35f3779. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>

XieX · 2026-09-16T15:40:29Z

Adopted

SEC-8985 row 2. `resolveRoot` validated the managed root and returned a path; nothing held it open. Every write and prune then opened `<root>/<key>` by path with `O_NOFOLLOW`, which guards only the final component, so the root was re-resolved on each one and a root swapped for a symlink after validation redirected the open into the attacker's directory. `assertUnswapped` agreed with itself, because it lstat'd the same swapped path the handle came from. Only the manifest write pinned the root, and by then the skill files were already outside it. The precondition is write access to the root's *parent* — `.claude` for a root of `.claude/skills`, which the agent identity typically owns. The root is now opened `O_RDONLY|O_DIRECTORY|O_NOFOLLOW` once, fstat'd, and held for the whole reconcile, and every child is addressed as `/proc/self/fd/<fd>/<name>`. The kernel resolves that prefix to the pinned inode rather than to the name, which gives it the property an `*at()` call would have; the per-skill directory is pinned in turn for the files inside it. Gated on a new `SUPPORTS_PROC_FD` probe (Linux plus a usable `/proc/self/fd`, verified at load by resolving a live descriptor back to its own inode), so macOS and Windows keep the per-component lstat floor unchanged — `assertUnswapped` is skipped only for a directory the caller addressed through a descriptor, which is why the floor's own tests still exercise it on Linux. Containment checks in `unsafePathReason` stay as defense in depth; they are no longer the boundary here. A swap landing before the pin fails the open and is reported as a run-level error rather than thrown, since the root was usable when the caller named it. `atomicWriteIn` is gone: the root is always pinned by the caller now, so a primitive whose job was to re-open a directory by path is exactly the footgun this bug was made of. Descriptor addresses are stripped out of `errno` messages, which leaves the offending file named relative to whichever directory was pinned. Tests: the two `<root>/<key>` races in skills-fs.test.ts now run wherever either capability probe is true, so they are live on Linux. The root-swap file from Security's PR #49 keeps its assertions; its trigger matches the final component plus the shape of its parent rather than one exact path, so the same bodies fire the swap against both the old and new addressing — pinning the trigger to `path.join(root, key)` would have made them pass vacuously the moment the fix landed. Verified in a Linux container: all four fail against the pre-fix code with real assertion failures (the write escapes, the outside victim is clobbered, the outside victim is unlinked) and pass against this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

pkaeding requested review from XieX and andrewklatzke September 4, 2026 16:13

pkaeding mentioned this pull request Sep 4, 2026

test(client): Agent Skills — root-swap races the descriptor walk does not close (SEC-8985 row 2) launchdarkly/python-ai-sdk#68

Closed

XieX mentioned this pull request Sep 4, 2026

fix(client): pin the skills root through /proc/self/fd on Linux (SEC-8985 row 2) #51

Merged

pkaeding and others added 2 commits September 14, 2026 16:08

XieX force-pushed the xie/skills-08-review-closeout branch from 90b45a5 to 888a616 Compare September 14, 2026 20:13

XieX force-pushed the sec/skills-root-swap-race-tests branch from 31d47db to 037da03 Compare September 14, 2026 20:13

XieX closed this Sep 16, 2026

Navigation Menu

test(client): Agent Skills — root-swap races the pinned handle does not close (SEC-8985 row 2) - #49

test(client): Agent Skills — root-swap races the pinned handle does not close (SEC-8985 row 2)#49
pkaeding wants to merge 2 commits into
xie/skills-08-review-closeoutfrom
sec/skills-root-swap-race-tests

pkaeding commented Sep 4, 2026 •

edited by atlassian Bot

Loading

Uh oh!

XieX commented Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

pkaeding commented Sep 4, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

The gap

What the tests show

Why the build is red

The fix (not in this PR)

Uh oh!

XieX commented Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pkaeding commented Sep 4, 2026 •

edited by atlassian Bot

Loading