fix(atomic-write): refuse a redirected parent for secret writes

[chenmingwei23]

What is the problem?

kiro_crew.atomic_write.atomic_write is the repo's canonical secret writer -- nine production call sites pass restrict_to_owner=True. It runs path.parent.mkdir(parents=True, exist_ok=True), then tempfile.mkstemp(dir=path.parent), then os.replace, and none of those three follow-safe the parent chain. A symlink (or Windows junction) pre-planted at the destination's parent, or at any ancestor of it, silently redirects the whole write: the secret lands under whatever the link points at and the caller sees success.

Reproduced on unmodified main before any change, in both shapes:

A parent-is-link      -> redirected: True | content: sk-live-DEADBEEF
B ancestor-is-link    -> redirected: True | content: sk-live-DEADBEEF   (mkdir walked THROUGH the link)
C leaf-is-link        -> victim overwritten: False                      (a leaf link is not a redirect)

GPT 5.6 first raised this on PR #2190 against one caller, where it was rebutted for that caller because both agent write channels to that path are already gated. The class is wider: every restrict_to_owner=True site has the same exposure and most of their parent directories are not on the sensitive-path denylist. The issue rejected per-caller patching as whack-a-mole.

Why this issue matters to the user

The 0600 bits are only half of what protects a secret file. The other half is that it lands in the directory the caller named, inside the sensitive-path fence that is the only real boundary against a same-UID reader. A redirected write puts credentials, HMAC keys, tokens and refresh-token reuse state at an attacker-chosen location outside that fence, with no error anywhere. The planting is something an attacker can set up at leisure, long before the write happens.

How our fix solves it

Symptom: a secret written through a planted parent link lands at the link's target. Root cause: the three primitives above follow every component except the final one, and the helper never checked the chain. So atomic_write now calls _refuse_linked_parent when restrict_to_owner=True, before the mkdir -- after it would be too late, since mkdir(parents=True) walks through the link and builds the missing directories under its target.

The guard splits the parent at the innermost KiroCrew-owned root containing it (config.paths.data_home / legacy_home / kiro_home), and the anchor is returned in the caller's own lexical namespace together with the names below it:

• At or above the anchor, a link is the operator's own layout and still writes: a data home relocated onto another disk, a symlinked $HOME, a symlinked config.json are all supported setups. The innermost root matters here, because the default layout nests two owned roots (~/.kiro/crew inside ~/.kiro) and anchoring on the outer one would put a relocated crew link below the anchor and refuse it.
• Below the anchor, every directory is one KiroCrew's own mkdir creates, so a link there was planted by something else.
• Outside every owned root, the walk stops at the first ancestor that already exists, because everything below that is a directory this write would create itself, while pre-existing layout above it (a symlinked /tmp on macOS) is not ours to judge.

Two checks then run, because neither is sufficient alone:

1. an lstat walk over the components below the anchor via platform_compat.is_link_or_junction -- the only thing that sees a Windows junction, which os.path.islink reports as False;
2. the parent must resolve to exactly the path rebuilt from the resolved anchor and those same names. Containment would not do: a link aimed at another directory inside the owned tree resolves to a contained path and passes any containment test, while still landing the secret in a directory the caller never named, where it can clobber a same-named file. Equality also covers a redirect the walk cannot see, such as a reparse point a platform's realpath follows but islink misses.

A first revision of this patch anchored the walk on resolved-path equality and broke out of the loop before the link check. A pre-push security review caught that a link pointing back at the anchor then satisfied the break, so it was never link-checked and the rest of the chain was never walked. That is why the anchor is now lexical and the two checks are separate; both shapes are pinned by tests.

Resolution failures fail closed rather than escaping. Path.resolve() reports a symlink loop as OSError on the versions that delegate to os.path.realpath but as RuntimeError on Python 3.10, which this repo still supports, so both are caught: a looped parent produces the refusal instead of a RuntimeError surfacing inside a caller's secret write. The same applies to root collection, where kiro_home() resolves its own override -- a resolver that cannot answer drops out of the anchor set instead of failing the write.

Only the parent chain is checked. A leaf link is not a redirect: os.replace does not follow the final component, so it swaps the link itself for the new file and the link's target keeps its old contents. The check is lstat-based and so not race-free -- a link planted between the check and the mkstemp still wins, and closing that would need an O_NOFOLLOW per-component descent that tempfile cannot be driven through. memory.py's lock-path check states the same limitation for the same reason. Refusing a link that is already there removes the pre-planting shape the report is about. Non-secret writes are untouched.

restrict_on_error="warn" cannot downgrade this: the refusal raises before mkstemp, so the two callers that pass warn (refresh_tokens.py, md_notebook/server.py) skip the write through their own except OSError rather than redirect a secret.

What tests we did

test/test_atomic_write_parent_link.py (new, 21 tests), one behaviour each:

• the reported shape (linked parent) and a link above a not-yet-created parent, both refused with nothing created under the target;
• an in-tree alias pointing back at the anchor, and an alias shadowing a deeper out-of-tree link, both refused (the shapes the first revision let through);
• an in-tree redirect to a sibling directory refused with link detection stubbed out, pinning equality rather than containment;
• a redirect refused when is_link_or_junction is stubbed to miss it, pinning the resolved-path check on its own;
• a symlink loop refused, the resolver wrapper turning a RuntimeError into "cannot prove", and a root resolver that raises dropping out of the anchor set rather than failing the write;
• an unresolvable chain refused, so the guard fails closed;
• a relocated data home under the kiro home still writes, pinning the innermost-root anchor;
• a symlinked data home and real subdirectories below it still write;
• a leaf link replaced with its target untouched; the guard applied to bytes payloads; restrict_on_error="warn" still refusing; non-secret writes unaffected; the ordinary write unaffected.

Mutation-verified: 12 mutants, all caught -- guard not called, guard moved after the mkdir, walk removed, equality check disabled, equality relaxed to containment, candidate order swapped, outermost root chosen as anchor, fail-closed branch removed, no-anchor walk stripped of its link check, both RuntimeError catches narrowed back to OSError, and the message dropping the component name. An A/B against the first revision confirms it fails the two alias tests that the current one passes.

Caller-side suites (browser_cli/token, refresh_tokens, secrets_vault, webhooks_store, mcp_gateway_prewarm, md_notebook) and the atomic-write suites pass -- 401 tests over the run before the last two review rounds. black (repo baselined gate), isort, flake8, the brand-name gate and mypy are clean on the touched files. Symlink-planting tests skip on Windows, where creating a directory symlink needs a privilege CI lacks.

Any other suggestions on the work

Scope correction, since the issue's framing invites over-reading this: the refusal closes the exposure for every writer that goes THROUGH atomic_write, which is the nine restrict_to_owner=True sites. It does not close the class. At least seven hand-rolled secret writers still do their own mkdir plus mkstemp and never enter the helper, so they keep the exact exposure -- sel.py (the HMAC key), dashboard/token_auth.py, dashboard/token_secret.py, beacon.py, apps/install_receipt.py, mcp_gateway/rewriter.py (the env sidecar holding API keys), dashboard/handlers/messaging.py, dashboard/handlers_system.py. Migrating them onto the chokepoint is a larger change than this one and belongs in its own issue; naming them here so the gap is on the record rather than implied to be fixed.

One behaviour change worth stating plainly: an operator who relocates a single directory below the data home onto another disk with a symlink (say a large state directory) will now get a hard OSError on secret writes into that subtree. That is the line the issue asked for, and the message names the offending component so it is actionable, but it is a real change and not only a hardening.

_owned_roots also lists legacy_home and kiro_home even though the nine current callers all land under the data home. That breadth only ever tightens the check below those roots, and it is what makes the innermost-root rule meaningful on a legacy-layout install, so it stays.

Closing the remaining race would need atomic_write to stop using tempfile.mkstemp and descend with O_NOFOLLOW directory handles instead. That is a bigger change to the writer's shape than this fix, and worth its own issue if the pre-planting refusal proves insufficient.

Closes #4381

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design review complete. The diff is a single guard in atomic_write (the chokepoint nine restrict_to_owner=True secret writers pass through) that refuses a pre-planted symlink/junction in the parent chain below a KiroCrew-owned trust anchor, plus a 21-test pinning suite. The threat is real and named (the agent is untrusted w.r.t. the sensitive-path fence; a redirected write lands secrets outside it), the chokepoint is the right layer, alternatives (per-caller patching, O_NOFOLLOW descent) were weighed explicitly, and the TOCTOU residue is documented rather than hidden. The remaining signals are scoping, not shape.

Design-Verdict: CONCERNS

Sound chokepoint fix, but "Closes #4381" retires the class report while seven named hand-rolled secret writers still carry the exact exposure.

Watch

• The description itself states the class stays open — "sel.py (the HMAC key), dashboard/token_auth.py, … keep the exact exposure" — yet the PR says "Closes atomic_write: secret writes (restrict_to_owner=True) follow a pre-planted parent symlink/junction — add refusal in the shared helper #4381", the class report. Once merged, the only record of the gap is a PR description nobody re-reads. File the follow-up issue (migrate the seven writers onto atomic_write) before this lands, or don't auto-close atomic_write: secret writes (restrict_to_owner=True) follow a pre-planted parent symlink/junction — add refusal in the shared helper #4381.
• The acknowledged behavior change — an operator symlinking a subtree below the data home now gets hard OSError on every secret write, and the two restrict_on_error="warn" callers silently skip persisting instead — ships with no escape hatch beyond "replace the link". Fine as the chosen line, but worth a release-note/docs mention since token refresh failing on a previously-working layout will read as a regression.

[DESIGN-REVIEWED] 89cef67

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 89cef67e3c9aae12e81d7b534ed0c17a9866a062 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/atomic_write.py:272 -- function-local "from kiro_crew.config import paths as config_paths" violates top-level-imports -> Fix: move the import to module scope.
[GPT-REVIEWED] 89cef67

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 89cef67e3c9aae12e81d7b534ed0c17a9866a062 — 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.

First-Principles-Verdict: CONCERNS

The fix is real and class-wide for atomic_write, but ~5 secret writers never go through that helper, so the "one helper every secret write already goes through" framing overclaims.

What this change ships

Intent: stop a pre-planted parent symlink/junction from silently redirecting secret writes — a FIX.

1. Secret write through a planted parent link below the owned root now refuses — justified (issue atomic_write: secret writes (restrict_to_owner=True) follow a pre-planted parent symlink/junction — add refusal in the shared helper #4381, mechanism level)
2. Relocated data home / symlinked $HOME / innermost-root anchoring keeps writing — justified
3. Unresolvable parent chain (loop) now refuses instead of crashing or writing — justified
4. Best-effort link walk for destinations outside every owned root — justified (also the degraded path when root resolvers fail)
5. Operator's own symlinked subdirectory inside the data home now refuses secret writes — declared trade-off, premise unevidenced
6. First secret write can trigger data-home first-resolution maintenance (breadcrumb write) — undeclared in description, declared in-diff
7. 21 tests pinning both refusal and allowed layouts — justified

Watch

• Unfixed siblings of the same root cause (mkdir(parents=True) + create/replace of a secret, bypassing atomic_write): I grepped platform_compat.restrict_to_owner( and counted 5 — dashboard/server.py:1491 (_write_secret_file), dashboard/token_auth.py:178 (revoked-nonce store) and :992 (app secret), dashboard/token_secret.py:144 (signing key), secrets/vault.py:158 (vault key). Each follows a plantable parent chain; O_EXCL protects only the leaf. The new docstring's "the refusal lives in the one helper every secret write already goes through" is contradicted by these. Routing them through atomic_write (or this guard) is the larger general fix — accepted-and-deferred, but the docstring claim should not outlive it.
• Item 5's premise — "below the anchor every directory is one KiroCrew's own mkdir creates" — is asserted, not evidenced; confirm no supported layout symlinks a subtree (e.g. sessions on another disk) before this refusal ships to them.

[FIRST-PRINCIPLES-REVIEWED] 89cef67

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 89cef67e3c9aae12e81d7b534ed0c17a9866a062 — this comment is updated in place on each push.

Review details

Both candidates hinge on the same reachability question, so I traced the arithmetic and every production caller.

Candidate 1 (IndexError at line 328): The logic error is real. If _link_trust_anchor reaches the second loop iteration (candidate = resolved) and matches there, best is computed from the deep resolved path while names/anchor are sliced from the shallow lexical path — lexical.parents[best-1] can index past the tuple. I confirmed: /link -> /data/a/b with KIROCREW_HOME=/data yields best=2, lexical=/link (parents = [/], one element), parents[1] → IndexError.

But the second iteration only runs when lexical matches NO owned root. Every restrict_to_owner=True caller (vault, refresh_tokens, mcp handler, browser token, prewarm, discord resume, md_notebook, webhooks) builds its path from config_dir()/kiro_home(), which are lexically identical to the strings _owned_roots() produces (override → both .resolve()d; default → both unresolved Path.home()/.kiro/crew). So the constructed parent is always lexically under an owned root, the first iteration always matches and returns, and the resolved branch is never reached. Triggering it requires a caller passing a top-level symlink aliasing into the data home — "if a caller were to," which no caller does. (a) fails.

Candidate 2 (outside-roots walk): Same reachability wall — no production caller writes a secret outside the owned roots, so the split is None branch is dead for real callers. The candidate itself only alleges a code/comment disagreement ("whichever is wrong, one is a defect"), which is not a grounded observable wrong outcome, and refusing a link at the first existing ancestor is a defensible security choice, not a proven defect. (a) and (c) fail.

Neither survives falsification. The normal in-tree path — which is what the diff actually exercises — computes a valid best, names, and anchor and passes the equality check, as the added tests confirm.

No findings.

[OPUS-REVIEWED] 89cef67

Verdict parsed from the review's SHA-scoped output markers for commit 89cef67e3c9aae12e81d7b534ed0c17a9866a062.

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