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

[XieX]

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 — 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 test(client): Agent Skills — root-swap races the descriptor walk does not close (SEC-8985 row 2) #68 is left open deliberately — for its owner to close as superseded.

🤖 Generated with 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.

^{Reviewed by Cursor Bugbot for commit d3a8bc1. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 5040942. Configure here.}

[cursor]

Path probes authorize descriptor writes

High Severity

target.exists() and _read_regular_file still resolve the path after the root is pinned. A swap between _unsafe_path_reason and those probes — the sweep's open, which the new races already fire — can hide a real unmanaged file so the write proceeds through root_fd and overwrites it, or adopt the attacker's bytes and skip updating the real file.

Additional Locations (2)

• packages/client/src/launchdarkly_ai_server/skills_fs.py#L896-L897
• packages/client/src/launchdarkly_ai_server/skills_fs.py#L1175-L1176

 

^{Reviewed by Cursor Bugbot for commit 5040942. Configure here.}

[XieX]

Confirmed as accurate, and deliberately left in place. Tracking it with Security rather than folding it into this PR.

Both outcomes are real and I verified both in the code: exists() returning false through a swap skips the "refusing to overwrite a file this SDK did not write" guard entirely, and the adoption branch matching the attacker's bytes calls _update_entry and skips the write, so the real file is recorded as managed without ever being verified.

Two things bound it, which is why it is not blocking this change. Neither outcome escapes the root — every write still lands through root_fd inside the validated directory. And the attacker has write access to the root's parent, not inside the root, so they cannot choose what the real file contains; the adoption case leaves a stale file reported as current rather than injecting content.

The reason it is not fixed here is shape, not severity. target.exists() and _read_regular_file name <key>/SKILL.md, so covering them the way the manifest read is now covered needs a descriptor for the skill directory, not for the root — which means threading a per-key descriptor through _write_one and its probes. That is a larger change than SEC-8985 row 2 asks for, and it deserves its own review rather than riding along with this one.

Your sibling finding on the manifest read is fixed in d3a8bc1 — that one was not separable, because the manifest is the input that authorizes the destructive steps rather than an ordinary read under the root.