fix(security): gate linked ancestors before fs probes (#5962)

[CrysisDeu]

Summary

PR #5943 added first_linked_ancestor and wired it in the themes handler: on Windows, an ancestor symlink/junction whose target is a UNC share launders the lexical UNC screen, because the first filesystem call (even an lstat, which resolves every ancestor) becomes the outbound SMB credential probe. This PR replicates that Windows-gated, guard-precedes-every-resolving-call pattern at the remaining is_unc_shape call sites, plus one sibling consumer review surfaced.

Closes #5962

What changed

• src/kiro_crew/acp/prompt_blocks.py — image-path loop: ancestor walk + junction-aware leaf check after the lexical suffix screen, before is_file(); a refused candidate is skipped like the UNC case (path stays in the text).
• src/kiro_crew/hooks.py validate_file_path — inside a Windows-only branch: lexical abspath anchoring (so the walk covers what realpath resolves), a UNC re-screen of the anchored form (~ on a roaming profile or a UNC CWD can surface a UNC shape the raw text lacked; abspath never strips UNC-ness so one screen covers both forms), the ancestor walk, and the leaf check — then realpath consumes the same string the walk inspected. POSIX keeps byte-identical resolve-through-every-symlink semantics, pinned by a dot-dot-through-symlink test; the messaging.md spec paragraph is updated in the same commit.
• src/kiro_crew/messaging/outbound_files.py _inspect — ancestor walk + leaf check FIRST inside the try, before is_sensitive_path (whose candidate forms are built with realpath/resolve, so it was the first chain-resolving call) and before the junction-blind is_symlink() lstat. Same reply as the leaf case, per the reference. local_destination additionally screens the expanded form — still lexical, its documented contract.
• src/kiro_crew/image_artifacts.py _local_file — the registration-side consumer of the same lexical screen gains the same ancestor + leaf guards before its is_file() probe.
• src/kiro_crew/memory.py _read_root_guard — Windows ancestor walk on the workspace before the leaf reparse checks, with an audit line (workspace_linked_ancestor); the old "linked ancestors deliberately not rejected" comment is rewritten to keep only the POSIX half, and the INVARIANT docstrings are qualified to match (write path inherits via _require_link_free_roots).

No user-facing message names the offending ancestor (which ancestor is a link is filesystem layout the caller supplied a path to guess at).

Pre-push adversarial review

3 rounds of dual blind model-pinned review (GPT + Opus lanes, mirroring the CI charters). 14 findings total: 8 fixed (ordering vs is_sensitive_path, expanded/anchored-form screens, the unguarded _local_file sibling consumer, the leaf-link class, POSIX abspath semantics regression, test attributability), 4 advisory adoptions (docstring accuracy, stub completeness, audit-count bound), 2 documented trade-offs below. Round 3: GPT PASS zero findings; Opus findings were test-quality only, fixed as prescribed.

Documented trade-offs (deliberate, matching the merged reference semantics)

1. Refuse-all-links predicate. The walk refuses ANY linked ancestor on Windows, including locally-targeted junctions (C:\dev -> D:\dev). Narrowing to UNC-targeted links only would require reading each link's target and re-validating the target's own chain recursively; the merged themes wiring (fix(themes): enable theme pack routes on Windows #5943) chose refuse-all, and this PR replicates those semantics as the issue mandates. At the memory site a refusal surfaces as empty memory plus a logger.warning + SEL audit entry — same surface the existing UNC/leaf refusals already use.
2. TOCTOU inside first_linked_ancestor itself. An ancestor could be swapped to a link between the walk and the probe. Closing this needs handle-relative (openat-style) traversal with no Windows equivalent — a cross-platform redesign of the merged helper, out of scope for replicating its accepted pattern.

Residuals noted in-code, follow-up planned: is_link_or_junction fails open on an unreadable component (pre-existing shared machinery); a mapped network drive / subst target is outside the reparse-point class entirely.

Testing

• 22 new Windows-marked tests across 5 test modules, mirroring the themes tests: ordering pinned by wiring the FIRST downstream probe to explode (realpath, is_file, is_sensitive_path), mutation-checked (refusal disappears when the walk reports no link), POSIX behavior pinned unchanged (including the dot-dot-through-symlink canonicalization pin).
• Local gates: isort / flake8 / mypy / diff-scoped black all green; full pytest 70806 passed, 86 failures byte-identical to the pre-change host-env baseline (proven in the unmodified-base log), zero in touched modules.

no linked issue in title: issue reference carried in the trailing "(#5962)" of the conventional-commit subject and "Closes #5962" above.

No screenshots: backend-only security hardening, no UI surface.

Pattern harvest

Rule candidate: any call site that lexically screens a user- or agent-supplied path for UNC shape must place the screen (and a Windows linked-ancestor/leaf-link check) BEFORE the first resolving filesystem call — including is_sensitive_path, whose candidate forms are built with realpath()/resolve(). When a lexical screen lives in a shared normalizer (local_destination), every consumer that later probes the path needs its own guard; grep for sibling consumers before declaring the class closed.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 75c36950f684695ea2908b5487f553d3ac02f6fc — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound, fail-closed replication of a merged pattern — but the class is closed by grep, not by structure, so the next consumer silently reopens it.

Watch

• The identical ancestor-walk + leaf-check preamble is now hand-copied at four consumer sites, and the PR's own "pattern harvest" concedes closure depends on "grep for sibling consumers": a future is_file()/realpath consumer of local_destination or any lexical screen reintroduces the SMB-probe laundering with nothing going red. No CI rule or shared enforcement ships with the harvested rule candidate.
• The ~100-line hop-by-hop leaf-chain resolver in validate_file_path re-implements Windows link-resolution semantics (long-path prefix folding, drive-relative shape allowlist) in Python solely to preserve that one site's resolve-benign-leaf contract — a bespoke security state machine that must track OS behavior forever, versus the refuse-leaf-links semantics every sibling site accepted.
• Refuse-all-links means a Windows workspace under any junctioned ancestor (mklink /J C:\dev D:\dev, OneDrive reparse points) now yields empty memory and refused uploads, surfaced only in logs/SEL — a deliberate, documented trade-off, but a support-ticket shape humans should expect.

Suggestions

• Extract the shared first_linked_ancestor + is_link_or_junction refuse-pair into one helper (e.g., in hooks.py beside is_unc_shape) and land the harvested rule as an AUTOSDE.yaml review rule, so the invariant is a function plus a gate rather than five commented copies.

[DESIGN-REVIEWED] 75c3695

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 75c36950f684695ea2908b5487f553d3ac02f6fc — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification is done. The is_unc_shape sibling class is genuinely closed by this PR, the leaf-chain walk is derived from a test-pinned contract, and the one real finding is the undeclared depth/hop ceilings that exist only at the hooks.py site. Final review:

First-Principles-Verdict: CONCERNS

Two undeclared DoS ceilings ride along in hooks.py, and the depth ceiling is a point patch — 10 of 12 first_linked_ancestor sites stay unbounded.

What this change ships

Intent: stop a Windows ancestor/leaf link from laundering an SMB credential probe past the lexical UNC screens at every remaining site (#5962) — a FIX.

1. Dashboard file paths refuse a linked ancestor before any resolve — justified
2. Dashboard leaf links screened hop-by-hop; benign local links still resolve — justified (contract pinned by test_allows_benign_symlink)
3. Windows paths deeper than 255 components now refused — undeclared, symptom-level point patch
4. Leaf chains longer than 40 hops now refused — undeclared, justified (ELOOP analog)
5. Chat image candidates under a linked ancestor/leaf link skipped — justified
6. Outbound uploads refuse linked ancestor/leaf before is_sensitive_path — justified
7. local_destination screens the ~-expanded form — justified (mirrors themes reference)
8. Image-artifact registration gains the same guards — justified (counted sibling consumer)
9. Memory reads refuse a linked workspace ancestor, new audit rule — justified
10. messaging.md paragraph updated same-commit — mandated by AGENTS.md

The core class is genuinely closed: grepped is_unc_shape — 6 consuming sites in src/, all guarded after this PR, plus both local_destination consumers (_inspect, image_artifacts._local_file).

Watch

• _MAX_SCREENED_PATH_DEPTH's own rationale (one lstat per component on untrusted input stalls the loop) indicts the siblings it skips: grepped first_linked_ancestor( — 12 call sites in src/, only hooks.py:2102/2178 bounded; prompt_blocks.py:191, outbound_files.py:454, image_artifacts.py:128, themes.py:242 walk the same agent/user-supplied paths unbounded. The general fix is one ceiling inside platform_compat.first_linked_ancestor, which would let both hooks-local constants and their per-site checks be deleted.
• Neither ceiling (255 components, 40 hops) appears in the description's "What changed" — both are observable Windows refusals a human should see declared.

Subtractions

• Replace the call-site _MAX_SCREENED_PATH_DEPTH screens (hooks.py:2137, 2220) with a ceiling inside platform_compat.first_linked_ancestor — deletes the constant and two per-site checks, and covers the 10 unbounded callers.

[FIRST-PRINCIPLES-REVIEWED] 75c3695

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 75c36950f684695ea2908b5487f553d3ac02f6fc and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 75c3695

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 75c36950f684695ea2908b5487f553d3ac02f6fc: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 75c36950f684695ea2908b5487f553d3ac02f6fc — this comment is updated in place on each push.

Review details

The candidate list contains no candidates — the discovery pass found nothing to falsify. I independently reviewed the diff's load-bearing logic: the Windows-gated ancestor/leaf-link screens in validate_file_path (UNC fold case-insensitivity, depth bound before the walk, hop-by-hop shape allowlist, fail-closed on unreadable/over-long chains, realpath consuming the same anchored string the walk inspected), the mirrored leaf/ancestor gates in prompt_blocks.py, image_artifacts.py, outbound_files.py, and memory._read_root_guard, and confirmed the POSIX path is unchanged and os is imported everywhere the new os.name checks live. Nothing grounds a concrete input → call path → wrong outcome at the required bar.

No findings.

[OPUS-REVIEWED] 75c3695

Verdict parsed from the review's SHA-scoped output markers for commit 75c36950f684695ea2908b5487f553d3ac02f6fc.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 75c36950f684695ea2908b5487f553d3ac02f6fc: <one-sentence reason>

[CrysisDeu]

/ai-review override gpt 33147ef: The event-loop finding targets a bound this lane itself requested (round 3 demanded a depth ceiling as the event-loop remedy; round 6 flags that same <=255-lstat Windows-only bound as the stall). validate_file_path is a synchronous guard whose pre-existing realpath and is_sensitive_path already do filesystem I/O on the caller's thread; moving the whole guard off-loop is an architectural change out of scope for this hardening PR, per the maintainer escalation on issue #5962.

[github-actions]

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 33147efc235637f4bd61c44ad8cdf3d613455622.

The event-loop finding targets a bound this lane itself requested (round 3 demanded a depth ceiling as the event-loop remedy; round 6 flags that same <=255-lstat Windows-only bound as the stall). validate_file_path is a synchronous guard whose pre-existing realpath and is_sensitive_path already do filesystem I/O on the caller's thread; moving the whole guard off-loop is an architectural change out of scope for this hardening PR, per the maintainer escalation on issue #5962.

This decision applies only to this commit. A new push requires a new judgment.

[CrysisDeu]

/ai-review override gpt 75c3695: The raceable-ancestor finding (walk-then-probe TOCTOU) is documented trade-off #2 in the PR body: closing it requires handle-retained no-reparse traversal with no Windows-portable equivalent across these five call sites, which the maintainer escalation on issue #5962 ruled an architectural change out of scope for this hardening PR. The walk still eliminates the static laundering class the issue names; the race window is recorded as an accepted residual. The sibling extended-namespace finding from this round is FIXED in this head (bare extended-length prefixes now require a drive-absolute remainder, else refused fail-closed, with parametrized regression tests).

[github-actions]

AI-review override not recorded: keep the reason to 500 characters or fewer.

[bolichen97]

Correct replication of the merged #5943 pattern at the remaining is_unc_shape sites, and the load-bearing insight is the ordering one: in outbound_files._inspect the guard is placed before is_sensitive_path, not merely before the is_symlink() lstat, because is_sensitive_path builds its candidate forms with realpath/resolve and was therefore already the first chain-resolving call — putting the walk after it would have left the hole open. The validate_file_path leaf-chain walk is the most delicate part and it fails closed everywhere it should: readlink is local reparse metadata rather than a traversal, the \\?\UNC\ fold is case-insensitive so a lowercase \\?\unc\ spelling cannot fall into the local branch and launder a share, non-drive extended namespaces (GLOBALROOT, Volume{guid}) are refused instead of prefix-stripped into relative-looking strings, root-relative and drive-relative targets are refused because the string screened and the string realpath resolves could diverge by drive, and the for/else bounds the chain at an ELOOP-equivalent 40. Screening the anchored form and bounding depth before starting the walk is right too — the walk is one lstat per component, so on an untrusted UNC path or an adversarially deep one the guard itself would have been the probe. os.path.abspath is correctly scoped to the os.name == "nt" branch since it collapses .. lexically and would otherwise change what realpath returns across a symlinked component; POSIX stays byte-identical and is pinned by the dot-dot-through-symlink test. _local_file was the real gap worth catching — the sibling consumer of the same lexical local_destination screen — and refusal messages deliberately do not name the offending ancestor, which is the right call since that is filesystem layout the caller supplied a path to guess at.