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

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

What

Three failing tests, TestRootSwapRaces in packages/client/tests/test_skills_fs.py, stacked on #66. They state the contract the SDR response gives for SEC-8985 row 2 (every destructive operation runs relative to a descriptor "held for the duration of the reconcile") and show the code does not meet it yet. No implementation change in this PR.

The gap

_resolve_root 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 directory. O_NOFOLLOW guards only the final component, so the root and every ancestor are re-resolved on every such open. Swap the root for a symlink after validation and the open follows it: the descriptor is pinned to <outside>/<key>, and every descriptor-relative step behind it runs against the wrong directory. atomic_write_in does refuse the manifest rewrite afterwards (the run reports an error), but by then the skill file is already outside the root.

The existing TestSymlinkAttacks race tests swap <root>/<key> and prove that window is closed. These swap <root> itself.

Precondition: write permission on the root's parent, not on the root. The README privilege-separation checklist denies the agent identity the root, the skill directories, the files and the manifest, and says nothing about the parent. In the documented layout (<app>/.claude/skills) the parent is .claude, which the agent identity typically owns.

What the tests show

Test	Swap fires at	Result on this branch
create	`os.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	first `O_NOFOLLOW` open of `<root>/a`	`<outside>/a/SKILL.md` unlinked, then `<outside>/a` removed by the `rmdir`

uv run pytest packages/client/tests/test_skills_fs.py -k TestRootSwapRaces:

FAILED TestRootSwapRaces::test_root_swapped_at_the_skill_directory_create_cannot_redirect_the_write
  AssertionError: assert [PosixPath('/.../outside/a')] == []
FAILED TestRootSwapRaces::test_root_swapped_at_the_skill_directory_open_cannot_clobber_an_outside_file
  AssertionError: assert 'served update\n' == 'precious\n'
FAILED TestRootSwapRaces::test_root_swapped_at_the_prune_cannot_redirect_the_unlink
  AssertionError: assert (False)        # victim.exists()

The swap is fired from an os.mkdir / os.open interceptor (_SwapRootDuring, the sibling of the existing _SwapDirectoryDuring), so it lands at the narrowest possible instant rather than by timing. 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 marker to remove. Rest of the suite on this branch: 727 passed, 3 failed (these three); ruff check and ruff format --check clean.

The fix (not in this PR)

Pin the root once at the top of write_skills and walk from it: root_fd = os.open(root, O_RDONLY | O_DIRECTORY | O_NOFOLLOW), then os.mkdir(key, dir_fd=root_fd) and os.open(key, O_DIRECTORY | O_NOFOLLOW, dir_fd=root_fd) for each skill directory, and atomic_write(..., dir_fd=root_fd) for the manifest. os.open, os.mkdir, os.stat, os.unlink and os.replace all take dir_fd on POSIX and safe_fs.SUPPORTS_DIR_FD already probes for them. 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 TypeScript SDK: launchdarkly/js-ai-sdk#49.

🤖 Generated with Claude Code

via LD Research 🤖

…does not close Three expected-failure tests (TestRootSwapRaces) for SEC-8985 row 2. The SDR response says every destructive operation runs relative to a descriptor held for the duration of the reconcile. The code pins <root>/<key> per operation and never holds the root: _resolve_root validates it once and returns a path, and each write and prune re-opens <root>/<key> by path with O_NOFOLLOW, which guards only the final component. A root swapped for a symlink after validation redirects the open, and every descriptor-relative step behind it, into the attacker's directory. Precondition is write permission on the root's parent, which the README checklist does not mention. The tests state the contract (nothing lands outside the root, no outside file is overwritten, no outside file is removed) and are marked xfail(strict=True, raises=AssertionError) so the suite stays green, the gap is recorded next to the other race tests, and the fix cannot land without removing the marker. Run with --runxfail to see the three escapes. 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>

XieX · 2026-09-15T14:57:52Z

Adopted by #70

…w 2) (#70) Fixes **SEC-8985 row 2**. Supersedes #68, whose two test commits are the first two commits here (cherry-picked with `-x`, authorship preserved). SDR response: https://launchdarkly.atlassian.net/wiki/spaces/PD/pages/5293965360 **The first commit is Security's tests and CI ran red on it** ([run 33909812977](https://github.com/launchdarkly/python-ai-sdk/actions/runs/33909812977) — 3 failed, 1484 passed, with lint, format and type check green). The head of this branch is green. ## The gap The SDR response says every destructive operation runs relative to a descriptor held for the duration of the reconcile. It did not. `_resolve_root` validated the managed root and returned a plain `Path` — nothing held it open. Each write and each prune then opened `<root>/<key>` *by path* with `O_NOFOLLOW|O_DIRECTORY` and pinned that. `O_NOFOLLOW` guards only the final component, so the root and every ancestor were re-resolved on every such open: a root renamed aside and replaced with a symlink after validation redirected the open, and with it every descriptor-relative step behind it, into the attacker's directory. On the create path `os.mkdir(<root>/<key>)` followed the link too — `mkdir` follows a symlink at its parent. `_sweep_orphan_temp_files` and `_prune_one`'s `rmdir` had the same shape. The manifest write was the only operation that pinned the root, and it ran last, by which time the skill files were already outside it. Attacker precondition: **write access to the root's parent** (`.claude`, for a root of `.claude/skills`) — which the README checklist did not mention. Reproduced: the prune case deleted a file outside the root outright. ## The fix `write_skills` opens the root once, immediately after `_resolve_root`, with `O_RDONLY|O_DIRECTORY|O_NOFOLLOW`, confirms `S_ISDIR` on the descriptor, and holds it until the call returns (closed in `finally`). The descriptor is threaded through `_write_all`, `_prune`, `_rewrite_manifest`, the orphan sweep and the per-skill helpers, and every destructive step names a bare component against it: | Was | Is | |---|---| | `os.mkdir(root / key)` | `os.mkdir(key, dir_fd=root_fd)` | | `os.open(root / key, ...)` | `os.open(key, ..., dir_fd=root_fd)` | | `skill_dir.rmdir()` | `os.rmdir(key, dir_fd=root_fd)` | | `atomic_write_in(root, MANIFEST, ...)` | `atomic_write(root, MANIFEST, ..., dir_fd=root_fd)` | A root swapped in the one interval left — after validation, before the open — fails `O_NOFOLLOW` and is reported as a **run-level error with nothing touched**, rather than as the `ValueError` an unusable root raises. `safe_fs`'s three openers (`pinned_directory` / `open_or_create_directory` / `open_directory_nofollow`) take a `dir_fd` for the *parent* rather than growing a parallel API, and `SUPPORTS_DIR_FD` now probes `os.mkdir` and `os.rmdir` alongside the four it already named. Where the `*at()` family is absent the per-component `lstat` floor runs exactly as before — the root open returns `None` there and every call site keeps its full-path branch, which `TestWithoutDirFd` covers. `_unsafe_path_reason` stays and still runs, now documented as defense in depth rather than the boundary: every check in it inspects a path, so each is a check-then-use against anything that can rename a component of that path. ## On the tests Security's `_SwapRootDuring` fired only when the intercepted `os.mkdir`/`os.open` was handed the absolute `<root>/<key>`. The fix stops passing that, so **the swap would never have fired and all three tests would have passed vacuously.** The trigger now matches the bare key as well, so it fires in both worlds — verified by reverting the source fix locally with the new test code in place, where all five tests fail. Two tests added: - a root swapped *before* the pin is refused at the run level (the `O_NOFOLLOW` open fails `ELOOP`); - an audit that across a reconcile which creates, writes, renames, unlinks and removes, no destructive call names an absolute path — the property the individual race tests are each one instance of, so a new path-based call site is caught even though no existing swap test aims at it. Also checked by hand: no descriptor leak across 80 reconciles including prunes. ## Follow-up: the manifest read (Cursor Bugbot, finding 1) Bugbot flagged that `_load_manifest` still read by path after the pin, and it was right. Fixed in the last commit, because the manifest is not an ordinary read under the root — it is the input that *authorizes* every destructive step the pin was added to protect. The damage is delayed, which is what makes it worth fixing rather than noting. During the swapped run nothing is destroyed: `_unsafe_path_reason` compares against the root path resolved before the swap, sees the write and the prune land outside it, and refuses both, so the report shows only refusals. What it cannot refuse is the record. `_rewrite_manifest` commits the entries that were read back over the *real* manifest through the held descriptor, so the run ends with the real manifest claiming a file the SDK never wrote — and the **next** reconcile, an ordinary one with no attacker present, every path check passing, the key well formed and the path genuinely inside the root, removes the customer's file. Verified end to end: the second run reports `action=removed` for a file the SDK did not write. This is also the one respect in which pinning the root made things *worse*, and the reason the original note below understated it. Before the pin, the manifest write was the only operation that took its own `O_NOFOLLOW` descriptor, so a swapped root made that write fail and left the real manifest intact. `_load_manifest` now reads through the held descriptor, reusing `_read_regular_file` rather than hand-rolling a second open, so the manifest inherits the protections the skill files already had: | Was | Is | |---|---| | `path.exists()` then `path.read_text()` | one `os.open(MANIFEST, ..., dir_fd=root_fd)` | | a symlink wearing the manifest's name is followed | `O_NOFOLLOW` refuses it as corruption | | a FIFO there blocks the event loop forever | `O_NONBLOCK` + `S_ISREG` on the descriptor refuses it | `_read_regular_file` gained `dir_fd`, and `max_bytes=None` for a read to EOF — the manifest is parsed rather than compared against a known length, so it has no bound the caller can predict. Two tests, both verified to fail with the source change reverted: - a root swapped between the pin and the manifest read cannot supply the entries — the real manifest still records what it owned; - the same swap cannot poison the ownership record — asserted across *both* phases, since the deletion lands on the following clean run. (Checked that the phase-two assertion fails on its own too, with the phase-one guard removed, so the test proves its whole claim rather than just its first line.) ## Docs The privilege-separation checklist and its shell snippet now walk every ancestor of the root up to `/`, since renaming any ancestor is what enables the swap. The platform-bound paragraph claimed a descriptor held for the whole reconcile before that was true of the root; it now describes what is actually held, for how long, and names the ancestors alongside the root as the boundary on every platform. ## Verification - `uv run pytest` — 1491 passed, 11 skipped - `uv run ruff check .` — clean - `uv run ruff format --check .` — clean - `uv run mypy packages/*/src` — clean ## Notes - Scoped to destructive operations, per row 2 — plus the manifest read, which the section above explains is not separable from them. **This supersedes the original note here**, which claimed the manifest read yielded no escape because the manifest is already treated as untrusted. That is true about escape and was the wrong test to apply: the manifest read by path let a post-pin swap poison the ownership record inside the real root, and the deletion then happened on a later clean run. - Still path-based, and still out of scope: `target.exists()` and `_read_regular_file` in `_write_one`. Bugbot's finding 2 covers these — a swap between `_unsafe_path_reason` and those probes can hide a real unmanaged file so the write proceeds through `root_fd`, or match the attacker's bytes so the write is skipped and the file is recorded as managed anyway. Neither escapes the root, and the attacker cannot choose the real file's contents. Covering them properly needs a descriptor for the *skill directory* rather than for the root, since what they name is `<key>/SKILL.md` — a larger change than row 2 asks for, so it is flagged here for Security to track rather than folded in. - The companion JS fix is being done separately in `js-ai-sdk`. - **PR #68 is left open deliberately** — for its owner to close as superseded. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  --- > [!NOTE] > **Overview** > Closes a **TOCTOU gap** where an attacker with write access to the managed root’s **parent** could rename the root and leave a symlink, redirecting writes, prunes, and `mkdir` outside the intended tree even after `_resolve_root` validated the path. > > **`write_skills`** now opens and holds a pinned root descriptor for the entire reconcile. Destructive steps (`mkdir`/`open`/`unlink`/`rmdir` for skill dirs, manifest atomic write, orphan temp sweep) pass bare path components with `dir_fd=root_fd` instead of reopening `<root>/…` by absolute path. A swap between validation and pin fails with a **run-level error** and no filesystem changes. **`_load_manifest`** reads `.launchdarkly-skills.json` through the same descriptor so a swapped root cannot supply fake ownership entries. > > **`safe_fs`** gains optional parent `dir_fd` on directory open/create helpers, `_at()` for basename vs full path, and expanded `SUPPORTS_DIR_FD` probing for `mkdir`/`rmdir`. > > **README** updates the POSIX guarantee text and extends the deployment checklist (plus shell loop) to deny write on **every ancestor** of the skills root, not just the root and contents. > > **Tests** add root-swap race coverage (`_SwapRootDuring` matching both absolute and bare key paths), pre-pin refusal, manifest-poisoning scenarios, and an audit that destructive syscalls never use absolute paths without `dir_fd`. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit d3a8bc1. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>

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 pinned handle does not close (SEC-8985 row 2) launchdarkly/js-ai-sdk#49

Closed

XieX mentioned this pull request Sep 4, 2026

fix(client): pin the skills root for the whole reconcile (SEC-8985 row 2) #70

Merged

Base automatically changed from split/skills-review-closeout to split/skills-typed-outcome September 15, 2026 14:34

Base automatically changed from split/skills-typed-outcome to split/skills-self-healing September 15, 2026 14:34

Base automatically changed from split/skills-self-healing to split/skills-fs-hardening September 15, 2026 14:35

Base automatically changed from split/skills-fs-hardening to split/skills-materialization September 15, 2026 14:35

Base automatically changed from split/skills-materialization to split/skills-safe-fs September 15, 2026 14:35

Base automatically changed from split/skills-safe-fs to split/skills-retrieval September 15, 2026 14:35

Base automatically changed from split/skills-retrieval to split/skills-references September 15, 2026 14:36

XieX closed this Sep 15, 2026

XieX deleted the sec/skills-root-swap-race-tests branch September 15, 2026 14:57

Navigation Menu

Uh oh!

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

test(client): Agent Skills — root-swap races the descriptor walk does not close (SEC-8985 row 2)#68
pkaeding wants to merge 2 commits into
split/skills-referencesfrom
sec/skills-root-swap-race-tests

pkaeding commented Sep 4, 2026 •

edited by atlassian Bot

Loading

Uh oh!

XieX commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

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 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pkaeding commented Sep 4, 2026 •

edited by atlassian Bot

Loading