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.

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 — 1489 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. Reads under the root (_load_manifest, and target.exists() / _read_regular_file in _write_one) remain path-based and can still be redirected by a post-pin swap; neither yields an escape, since the manifest is already treated as untrusted and every write and delete behind those reads resolves through the root descriptor. Called out in case Security wants it tracked separately.
• 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

[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]

Manifest loaded by path after pin

High Severity

_load_manifest still reads through the root path after root_fd is held. A swap in that window loads an empty or attacker-controlled manifest, then _rewrite_manifest writes it into the real root via root_fd, wiping ownership records. The old atomic_write_in path refused a swapped-symlink root and left the real manifest untouched.

Additional Locations (2)

• packages/client/src/launchdarkly_ai_server/skills_fs.py#L261-L263
• packages/client/src/launchdarkly_ai_server/skills_fs.py#L630-L638

 

^{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.}