fix(mcp): refuse a probe temp declared inside the sealed runtime parent (#8747)

[pepmach]

What

A spec-declared MCP temp key (TMPDIR/TMP/TEMP) suppressed the probe's managed allocation entirely. When the declared path resolves inside <data home>/run — the runtime parent both sandbox backends seal read-only — the probed child got a temp dir it could not write, silently: a server that never touches temp still answers the handshake, so the probe reported green.

The declaration is now refused when it lands inside that seal, and the managed probe temp takes over (allocated before the wrap and carved out, exactly as an undeclared probe already is), with a WARNING naming the seal and the key it overrode.

Carving the declared path out of the seal was rejected as the remedy: spec env is untrusted config text and extra_writable_dirs is validated for self-derived scratch only, so a validator that merely refused guarded paths would approve any other subtree under run. This is option (a) from triage — the smaller surface, matching how the managed path works after #8746 — plus the loud diagnostic from option (b).

Details:

• sandbox.path_within_sealed_runtime_parent — the seal's own module answers the containment question, testing both the lexical and the canonical spelling (a supported symlinked data home is seen independently by path-based rules), and then asking the filesystem itself: (st_dev, st_ino) is compared against the sealed parent while climbing the declared path's ancestors, so a case, firmlink, or normalization alias of the seal cannot walk past the predicate. Resolution order is load-bearing: realpath runs on the original declaration, before any lexical pass, because normalizing first collapses .. lexically and so deletes the very symlink that .. was climbing out of — a <symlink-into-run>/../tmp declaration then read as outside the seal while the child's libc resolves symlink-first and lands back inside it. On case-insensitive APFS <data home>/RUN and <data home>/run are likewise one directory that realpath does not fold (it walks with lstat/readlink, neither of which canonicalizes case), which a purely lexical predicate answered "not sealed" for. A declared temp usually does not exist yet, so the walk climbs until a component stats — the deepest existing ancestor carries the identity, and nothing below a sealed directory can climb back out because the path arrives resolved and normalized. Only identity is compared, never a folded spelling, so a case-sensitive filesystem answers exactly as before; an unstat-able sealed parent yields the lexical answer rather than a folded guess, since the parent is primed and created a few lines above and folding there would mis-refuse a genuinely distinct RUN directory for no gain.
• Refused as a whole, not per key: tempfile consults TMPDIR before TMP, so honoring a surviving sibling key would leave writability depending on which key the spec happened to spell.
• A remote or device spelling is refused ahead of all of that, from the raw declaration — before realpath and before any stat. On Windows resolution is access: realpath opens the path (GetFinalPathNameByHandle) and stat follows it, so a declared UNC path made this local containment check open an SMB connection to a host the config author chose (stalling the caller for the SMB timeout when that host is dead), and a device spelling (\\?\, \\.\) reaches drivers and volume aliases no containment rule can reason about. Matched lexically on the two-leading-separator shape in both separator directions, so \\server\share, //server/share and the \\?\UNC\ long form all answer alike; refused on every platform deliberately, since the same config text is read on whichever OS the gateway runs. True is the honest answer as well as the safe one — containment cannot be established locally, and the caller's response to True (stop honoring the path, fall back to the managed temp) is what an unverifiable declaration warrants.
• The identity walk stats no-follow (lstat, never stat), so a planted final-component symlink cannot redirect it off-host either. Nothing is lost: symlink resolution already happened in the canonical spelling the same predicate compares, which is where a link into the seal is caught with every component resolved.
• Every temp-key comparison normalizes case. Detection is case-insensitive on purpose (Windows env keys are case-insensitive and the sanitized spec preserves the author's spelling), but the rewrite that carries the refusal out compared exact spellings — so a spec declaring tmpdir was correctly seen, correctly refused, and then left its own key in the child's env beside the managed TMPDIR, which on Windows is the same variable. The managed branch now drops every case spelling before merging its triple; the honored branch keeps the spec's exact spelling while pruning the ambient siblings, because upper-casing alone would let a declared tmpdir and an ambient TMPDIR both survive and leave writability to whichever the child's lookup reached first.
• Storage intent is preserved: a path under run named the data-home volume, and so does the managed root.
• If the managed allocation itself fails, the refusal still stands and every temp key is stripped, so the child inherits the ambient temp rather than a directory already known to be read-only.
• The backend chokepoint honors declarations the same way; this PR is scoped to the probe, as the issue is. The two runtime spawn paths carrying the identical root cause are mcp_gateway/backend.py (spawn_backend, at the "an OPERATOR-DECLARED temp wins" comment, no seal check) and the matching block in gatewayd.py — named here so the follow-up issue this defers to can point straight at them. Until that lands, a probe-green result no longer models runtime for a spec that declares a sealed temp: the probe now runs with a managed temp the runtime child will not get.

Live verification (isolated pod, not unit tests)

Isolated pod on the worktree, seed connections-two, three servers added through POST /api/mcp/custom and probed through POST /api/mcp/probe. Each server is python3 -c reporting its own TMPDIR facts as tool names, so the probe response carries the evidence.

Before (unmodified worktree):

repro-declared-sealed-existing | ok   tmpdir_<pod>/run/mcp-tmp      isdir_True  write_EROFS   parentwrite_EROFS
repro-declared-sealed-missing  | ok   tmpdir_<pod>/run/custom-tmp   isdir_False write_ENOENT  mkdir_EROFS  parentwrite_EROFS
repro-managed-control          | ok   tmpdir_<pod>/run/mcp-tmp/probe-8a10f8b3/tmp  write_ok    parentwrite_EROFS

status: ok on the first two rows is the silence in the issue title.

After (same pod, same routes, fixed code):

repro-declared-sealed-existing | ok   tmpdir_<pod>/run/mcp-tmp/probe-4a7fb45d/tmp  isdir_True  write_ok  parentwrite_EROFS
repro-declared-sealed-missing  | ok   tmpdir_<pod>/run/mcp-tmp/probe-7e78600b/tmp  isdir_True  write_ok  parentwrite_EROFS
repro-managed-control          | ok   tmpdir_<pod>/run/mcp-tmp/probe-7fef18ae/tmp  isdir_True  write_ok  parentwrite_EROFS

parentwrite_EROFS on every row shows the seal itself is unchanged — nothing was widened.

Pod journal:

WARNING kiro_crew.mcp_discovery: MCP probe [repro-declared-sealed-missing]: ignoring spec-declared
TMPDIR=<pod>/run/custom-tmp — it is inside the sandbox-sealed runtime parent, where the probed
server cannot write; probing with the managed temp instead

Pod torn down afterwards: isolated HOME nuked, pod ls clean, live gateway untouched.

Tests

Red-first: every new test fails with the corresponding source change stashed, passes with it.

• test/test_mcp_discovery.py::TestProbeTempContainment::test_sealed_spec_declared_temp_falls_back_to_managed — allocation happens, the carve-out names the managed scratch, all three canonical keys point at it, and the sealed path reaches nothing.
• …::test_sealed_declared_temp_stripped_when_allocation_fails — no temp key survives when there is no managed dir to point at.
• test/test_sandbox_argv.py::TestSealedRuntimeParentPredicate — containment inside run, non-containment outside the data home and for an empty value, and both spellings of a symlinked data home.
• Same class, alias coverage: …::test_a_symlink_climbing_back_into_the_run_parent_is_sealed builds a real out-of-run symlink pointing back inside and declares <link>/../tmp — lexically that collapses to a path outside the seal, so it fails with assert False against normalize-then-resolve and passes once realpath runs on the original spelling. …::test_a_case_alias_of_the_run_parent_is_sealed and …::test_a_missing_leaf_under_a_case_aliased_parent_is_sealed model an APFS-style case-insensitive name lookup narrowly (the fold applies to stat and lstat alike, since it lives in lookup; real calls for every path outside the fixture root; realpath deliberately left unfolded, exactly as APFS behaves) and both also fail with assert False against the lexical-only predicate. …::test_a_case_alias_outside_the_run_parent_is_still_not_sealed is the negative control — an aliased spelling that folds onto a directory outside the seal must stay honored — and it passes both before and after, so the fix is not a blanket refusal. …::test_a_real_case_alias_is_sealed_where_the_filesystem_folds_case makes the same claim against the real filesystem and skips on a case-sensitive CI filesystem.

The pre-existing test_spec_declared_temp_suppresses_probe_containment still passes unchanged: it declares a temp OUTSIDE the seal, which is still honored.

Red-first for the two security findings, both failing on 233fc5f33:

• …::TestSealedRuntimeParentPredicate::test_a_remote_or_device_declaration_is_refused_without_resolving_it — six spellings (\\host\share, //host/share, \\?\C:\tmp, \\.\PIPE\tmp, \\?\UNC\host\share, and a mixed-separator form). Each wraps realpath/stat/lstat in a recorder that logs any call made on a remote-shaped path and delegates everything else to the real function, so the test asserts both halves of the claim: the declaration is refused, and no resolution seam was reached on it. Six assert False before, and against the fix the recorder stays empty. Paired with …::test_a_local_declaration_is_still_resolved, which passes before and after so the fast rejection is not a blanket refusal of config text.
• …::test_the_identity_walk_does_not_follow_a_final_symlink — a link whose final component points at the sealed dir. Under the following stat the walk answered True (assert not True before); under lstat it sees the link's own identity and answers False. …::test_a_symlink_to_the_run_parent_is_still_sealed is the companion proving no coverage was lost — the same link is still refused by the public predicate, via the canonical spelling.
• …::TestProbeTempContainment::test_sealed_declared_temp_refused_whatever_case_the_spec_spelled[tmpdir|Temp|TmP] — the refusal bypass itself. Before the fix all three leave the sealed path in the captured child env (assert str(sealed) not in captured_env.values() fails on .../run/custom-tmp); after it the sealed path is gone in every spelling and exactly the three canonical names remain, so no lowercase twin is left to shadow the managed triple on a case-insensitive platform.
• …::test_unsealed_declared_temp_honored_in_the_spec_own_spelling — the mirror case, with ambient TMPDIR/TMP/TEMP set and a declared tmpdir outside the seal. Before: assert ['TMPDIR', 'tmpdir'] == ['tmpdir'] — the ambient key survived beside the declaration and would win tempfile's TMPDIR-before-TMP lookup. After: the declared spelling is the only temp key left, and the managed allocator is asserted never to run.

Gates: test/test_mcp_discovery.py test/test_sandbox_argv.py test/test_security_posture.py test/test_spawn_audit.py -q → 497 passed, 6 skipped, plus one pre-existing environment failure (test_importing_this_module_does_not_pull_in_the_mint_engine, which spawns a bare interpreter without the package installed and fails identically on a pristine main clone). flake8, isort --check-only, mypy src/kiro_crew/sandbox.py src/kiro_crew/mcp_discovery.py clean; black-baseline gate passes on the committed scope (no unrelated reformat churn).

Pattern harvest

Rule candidate: review-prompt

Pattern: lexical path containment (== / startswith / commonpath) used as a security or writability boundary, with no filesystem-identity check — and, its sibling, normalizing a path before resolving it, which collapses .. past the symlink it was climbing out of. Both make the comparison disagree with what the kernel will do: the first misses case, firmlink, and normalization aliases the filesystem considers the same directory; the second misses a link that climbs back into the guarded subtree. Round 1 of review found the second defect in the fix for the first, which is the argument that the class is worth a prompt rather than a one-off note. It also generalizes to an established precedent a few hundred lines above in the same module — assert_voice_runtime_outside_agent_workspace already walks (st_dev, st_ino) for exactly this reason — so a reviewer prompt that flags a new lexical boundary check near a path the sandbox seals, and asks both whether an identity walk belongs beside it and in which order the path is resolved, would have caught both before review. Deliberately not a semgrep rule: lexical containment is correct and desirable in the many places that are not a trust boundary, so the signal needs the judgment a prompt has and a pattern match does not.

Closes #8747

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Probe-only scope inverts the failure signal: a sealed-temp spec now probes green while runtime still hands the child the unwritable temp.

Watch

• Before this PR a temp-touching server with a sealed declaration failed the probe and the runtime identically; after it, the probe substitutes a managed temp the runtime child never gets (spawn_backend's "An OPERATOR-DECLARED temp wins" block has no seal check, confirmed at mcp_gateway/backend.py:4046), so for exactly the failing class the probe flips from truthful-red to false-green backed only by a journal WARNING. The PR names the two runtime sites but defers them ("this PR is scoped to the probe"), leaving the probe actively misrepresenting runtime until the follow-up lands.
  Clears when: a filed follow-up issue applies path_within_sealed_runtime_parent at spawn_backend and the gatewayd block, or the runtime fix lands in this PR.
• path_within_sealed_runtime_parent answers True for UNC/device/mapped-drive declarations that are demonstrably not inside the seal, and the caller's WARNING then tells the operator the path "is inside the sandbox-sealed runtime parent" — a false diagnostic for every remote refusal, which the runtime chokepoints will inherit when they adopt the same predicate. Same failure surface if volume_is_local misclassifies a local drive on Windows: every declared temp refused with the wrong reason.
  Clears when: the predicate distinguishes "contained" from "unverifiable/remote" (tri-state or second function) and the log message names the actual refusal cause.

Suggestions

• Since the refusal already fails closed to the managed temp, consider surfacing the override in the probe result (not only the journal), so the dashboard shows the probe ran under a different temp than runtime will use.

[DESIGN-REVIEWED] a580122

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of a58012210d2918a2a48f658b92ca6e21d53d95df — 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

~200 lines of UNC/mapped-drive/link-chain refusal guard the check's own resolution on Windows — the one platform where nothing seals <data home>/run at all.

Not justified as shipped

• Item 4 — inherited: the SMB-during-realpath hazard exists only on Windows, and sandbox.py:5112 records "Kiro Crew has no native Windows sandbox backend", so the containment question has no seal to enforce there.
• Item 5 — inherited: same premise; GetDriveTypeW probing exists to keep a no-op-on-Windows check from stalling on Windows.
• Item 6 — inherited: the 40-hop scan is inert on POSIX (realpath of a UNC-shaped target is local I/O) and only matters where the check shouldn't run.

What this change ships

Intent: FIX — a probe reported green while handing the probed server a temp dir sealed read-only, whenever the spec declared TMPDIR under <data home>/run (#8747, red-first tests, before/after repro).

1. Probe refuses a declared temp inside the sealed runtime parent, probes with the managed temp, logs a WARNING — justified
2. New predicate path_within_sealed_runtime_parent (lexical + canonical + identity) — justified
3. A case alias of the sealed parent (APFS/NTFS RUN) is recognized via (st_dev, st_ino) walk — justified
4. UNC/device spellings refused unresolved, on every platform — inherited, see above
5. Mapped/unclassifiable drive roots refused via a drive-type probe — inherited, see above
6. Symlink chains ending on a remote target refused via 40-hop no-follow scan — inherited, see above
7. Case-variant temp keys (tmpdir) now pruned/honored case-insensitively in the child env — justified
8. If managed allocation fails after a refusal, every temp key is stripped (ambient fallback) — justified

Watch

• Items 4–6 sit at symptom level: the description's "refused on every platform deliberately, since the same config text is read on whichever OS the gateway runs" treats the answer as platform-independent, but the seal is not — on Windows the predicate refuses paths a child can genuinely write, with a diagnostic naming a seal that isn't applied. The cause-level fix is a win32 gate in the predicate.
  Clears when: a Windows mechanism sealing <data home>/run is named, or the win32 gate ships and the three guards are deleted.
• The identical root cause is declared unfixed at 2 runtime spawn sites (grep declared_temp_keys / "OPERATOR-DECLARED temp wins": mcp_gateway/backend.py:4046, mcp_gateway/gatewayd.py:3257), so probe-green no longer models runtime for a sealed-temp spec.
  Clears when: the deferred follow-up issue the description promises is filed and linked.

Subtractions

• Have path_within_sealed_runtime_parent answer False on win32 (no seal exists — sandbox.py:5112), then delete _remote_or_device_spelling, _remote_or_unmountable_drive_root, _windows_drive_is_local, _remote_or_device_in_link_chain, _MAX_LINK_HOPS (~200 lines in sandbox.py) and their ~14 tests in test_sandbox_argv.py.

[FIRST-PRINCIPLES-REVIEWED] a580122

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

The sole candidate — a POSIX //-prefixed temp declaration being classified as sealed and refused — is deliberate, documented cross-platform behavior whose only consequence is that the probe substitutes a safe managed temp for the operator's chosen scratch dir (trivially avoidable with a single leading slash). No crash, no data loss, no security boundary crossed; the outcome (c) is benign and the change is by-design, well below the 80 bar. Step-2 review of the new link-chain, drive-root, and identity-walk logic surfaced nothing groundable to the same standard.

[OPUS-REVIEWED] a580122

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/mcp_discovery.py:2107 -- preserving lowercase "tmpdir" on POSIX leaves no recognized temp variable, so probes use system temp instead of the declared path -> Fix: reinsert declared temp values under canonical uppercase keys after pruning.
[GPT-REVIEWED] a580122

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

[cixuuz]

Two security issues remain:

1. path_within_sealed_runtime_parent() calls realpath()/filesystem identity probing on spec-controlled input before rejecting remote Windows spellings. A UNC/device path can therefore initiate SMB or other remote resolution during a local containment check. Reject remote/device forms before any resolution and use no-follow/local identity checks.

2. In the managed-temp rewrite, key in _CANONICAL_TEMP_KEYS is case-sensitive even though spec env keys are explicitly treated case-insensitively on Windows. A declared tmpdir/Temp can survive the rewrite. Normalize every comparison with key.upper().

[iamwhatever]

There are comments about security related, please handle it. Thanks

[pepmach]

Closing the mapped-drive residual disclosed earlier — now in eecb6f862.

The pre-resolution guard now classifies drive roots before any filesystem access: a drive-lettered spec path is checked through kiro_crew.windows_acl.volume_is_local (GetDriveTypeW on the root — sandbox.py:1547, root derivation at sandbox.py:1596), and a DRIVE_REMOTE (or unmountable) root is refused in the same seam as the UNC/device spelling check, so Z:\tmp on a mapped SMB share never reaches readlink/realpath. POSIX is a structural no-op. Tests drive the classification through a monkeypatched probe so the refusal logic runs on Linux CI, with the existing no-resolution recorder proving zero filesystem touches on the refused path.

With this, all three legs of the remote-access surface are covered pre-resolution: declared UNC/device spellings, link chains targeting them, and mapped drive roots.

[bolichen97]

@pepmach Thanks for this one. The audit found no duplicate or superseding work, so please keep the PR and finish it rather than narrowing or splitting it.

What is already on main: only the predecessor, #8746, which allocates the managed probe temp before the wrap and carves it out. It sits in your merge base and this PR builds on it (mcp_discovery.py still calls allocate_probe_tmp) instead of redoing it. Everything else here is new: main has no path_within_sealed_runtime_parent, no _sealed_declared_temp_keys, and no refusal branch in probe_server, and the residual against main is the exact reverse of your diff. Of the 14 open PRs sharing src/kiro_crew/sandbox.py, none touches your insertion point; the nearest, #8778, stops around line 1072. #9131 only edits test/test_sandbox_argv.py immediately above where you insert TestSealedRuntimeParentPredicate, which is a rebase-order detail, not a conflict of intent.

What still blocks it, not scope: the audit read 233fc5f, and at the current head @cixuuz's second point looks handled (every temp-key comparison now uses key.upper()), but the first one does not — path_within_sealed_runtime_parent still runs realpath on spec-controlled text before rejecting UNC and device spellings. @iamwhatever's CHANGES_REQUESTED rests on that, and readiness stays blocked until it clears. Please also confirm the deferred runtime half (mcp_gateway/backend.py spawn_backend and the matching block in gatewayd.py) has a follow-up issue, since until it lands the probe reports green for exactly the specs it now diverges from.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

[pepmach]

@bolichen97 Ack — matches the plan here: the PR stays whole (case-alias identity + the review-round hardenings), nothing being split out. Predecessor overlap you found is the already-merged groundwork, nothing to coordinate.

The security findings behind this request are in: remote/device path spellings, symlink chains and mapped drives are refused before any filesystem resolution, and a case-variant temp key can no longer defeat the sealed-temp refusal (details with citations: #9108 (comment), #issuecomment-5578364406, #issuecomment-5579746570). Current head a580122 is green. Dismissing the stale request so re-review can be picked up; re-requesting review.