fix(security): pin the auto-update kiro-cli spawn off PATH

[iamwhatever]

Problem / Motivation

GatewayOrchestrator._auto_apply_update in src/kiro_crew/slack/gateway.py spawned the optional backend update as the bare name "kiro-cli", with shutil.which("kiro-cli") used only as an existence check whose resolved path was thrown away, and with no env=:

if shutil.which("kiro-cli"):
    kiro_update = await asyncio.create_subprocess_exec(
        "kiro-cli",
        "update",
        stdout=asyncio.subprocess.DEVNULL,
        stderr=asyncio.subprocess.DEVNULL,
        start_new_session=platform_compat.IS_POSIX,
    )

create_subprocess_exec re-resolves a bare argv[0] off PATH at spawn time, so the which answer decided nothing.

Why it matters

Per trusted_system_bin's own docstring a gateway's PATH can legitimately lead with agent-writable directories (a worktree venv's bin, ~/.local/bin). Anything able to drop an executable named kiro-cli ahead of the real install got arbitrary code execution as the gateway user.

This is the unattended auto-update path. stdout/stderr are DEVNULL and the handler downgrades failure to logger.debug, so a hijacked run leaves nothing behind to notice.

Two things make it pointed rather than incidental.

It completes the hardening #5387 applied to the git spawns in this same function. That PR pinned all seven git spawns here to platform_compat.trusted_git_bin() + git_command_env(), with a comment explaining exactly this hazard ("a gateway's PATH can lead with an agent-writable directory … so a bare "git" lets a planted shim run"). The guard sits roughly 430 lines above the kiro-cli spawn; that one call site was left out.

The stage became reachable in that same PR. #5387 widened the branch gate from branch != "mainline" to is_primary_branch(branch), which includes "main". This repo's default branch is main (branch mainline does not exist), so before #5387 this stage returned early on every normal checkout and never ran. It runs now.

What changed

Resolve to an absolute path and pass an explicit environment, mirroring the sibling guards rather than inventing a pattern:

• platform_compat.trusted_system_bin("kiro-cli") — probes the fixed system directories and ignores PATH entirely, so neither the spawn-time re-resolution nor a planted binary can decide what runs.
• _trusted_path_env() (kiro_crew.platform.update_provider) — the env builder this same file's wheel-update path already uses. Pinning the binary is only half of it: the child otherwise resolves its own helper words through the gateway's inherited PATH. This narrows PATH to the trusted system directories and drops the interpreter/loader variables.

Both are treated as refusals, not as things to fall back from: no binary in a trusted directory, or no trusted PATH, skips the step. A bare-name fallback would reinstate the exact hole. The step is optional and non-fatal, so the rest of the update still applies; the "resolved but no trusted PATH" case logs at WARNING so the degradation is not indistinguishable from "no kiro-cli installed".

Why not resolve_kiro_cli

The first revision of this PR used resolve_kiro_cli (the resolver the ACP launch path and cli_doctor share) on the reasoning that kiro-cli is a per-user install. That was wrong, and the GPT 5.6 review was right to block it. resolve_kiro_cli consults KIROCREW_KIRO_BIN and ~/.local/bin first — both agent-writable, and ~/.local/bin is the exact directory trusted_system_bin's docstring names as the hazard. It closes the PATH-ordering half of the hole and leaves the planted-binary half wide open, on the one code path that runs unattended with its output discarded.

The cost of the stricter resolver is real and accepted: on a host where kiro-cli lives outside the system directories, this step becomes a logged skip and the operator runs kiro-cli update themselves. That is the same trade _shell_exec_args (which refuses Windows outright) and _trusted_path_env (which returns None rather than hand over an inherited PATH) already make in this update path — an unattended exec does not get to fall back to a lookup an agent can influence. It costs an optional backend refresh, never the gateway's own update.

Tests

Three new tests in TestAutoApplyUpdateKiroCliExecPin, asserting the spawn rather than the source — a source-string check ("does the file mention trusted_system_bin") executes none of the new code and would pass on a call that still hands create_subprocess_exec a re-resolvable name:

1. test_spawn_uses_resolved_absolute_path_and_explicit_env — argv[0] is the resolver's absolute path (and os.path.isabs), and env is the trusted mapping. Asserting only the path would pass on a spawn that still inherits the environment, since env=None means "inherit".
2. test_step_skipped_when_resolution_returns_none — no spawn at all, and the update continues past it.
3. test_step_skipped_when_no_trusted_env — fails closed with a WARNING, mirroring the wheel path's _trusted_path_env() is None refusal.

The resolver stub dispatches on the requested name: trusted_git_bin() resolves git through trusted_system_bin, so a stub that answered every name would silently redirect the seven git spawns too and the tests would stop exercising the path they claim to.

Two existing tests moved their "kiro-cli is present" stub off shutil.which, which no longer gates the step; without that, test_venv_update_full_path would have depended on whether the host running it happens to have a kiro-cli installed.

Revert-verified (re-run after switching resolvers). With the fix deleted and the pre-fix spawn restored in place, all three fail on the intended assertions — AssertionError: assert 'kiro-cli' == '/usr/bin/kiro-cli' for the first, and a recorded spawn where none is permitted for the two skip cases — then all three pass with the fix restored.

black (baselined gate, scoped to origin/main...HEAD), isort, flake8, mypy, and scrub-lint all clean. Diff-scoped gates run with base refs exported (BRAND_BASE_REF and equivalents), not bare — bare runs report a false green: brand-name, focus-cue, changelog-history, harness-parity all pass. test_slack_gateway.py, test_spawn_audit.py, test_governance_updates.py, test_update_check_install_aware.py: 527 passed, 2 skipped.

Pattern harvest

Rule candidate: semgrep
Pattern: shutil.which(X) used as the condition of a spawn whose argv[0] is the bare name X — the resolved path is discarded and execvp re-resolves off PATH at spawn time, so the check authorizes an exec it does not pin

Scope

Deliberately one call site. _warn_if_kiro_cli_outdated spawns a bare "kiro-cli", "--version" with the same shape and is not touched here — it is a boot-time probe on a different reachability path, and is reported separately rather than folded in.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Real PATH-hijack on an unattended spawn, closed with the exact fail-closed pattern its sibling git and wheel paths already use; the availability trade is weighed and disclosed.

[DESIGN-REVIEWED] 10bef45

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 10bef45

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 10bef45b2af922c0893e1b3d7447b4dafb57f4c9 — 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 claims verified. I have what I need to write the review.

First-Principles-Verdict: CONCERNS

The pin is derived and reuses existing mechanisms, but the sibling count is short: the identical which-gated bare kiro-cli update spawn survives in cli_server.py:1371.

What this change ships

Intent: stop a planted kiro-cli shim from running as the gateway during unattended auto-update — a FIX.

1. Unattended update spawns the resolver's absolute path, not a PATH-resolved name — justified
2. That child gets a PATH narrowed to system dirs — justified, reuses _trusted_path_env (gateway.py:9981)
3. Hosts with kiro-cli outside /usr/bin,/bin,/sbin,/usr/sbin lose the auto backend refresh — declared trade
4. New WARNING when binary resolves but no trusted PATH exists — justified
5. Three spawn-asserting tests; two existing tests re-pinned off shutil.which — justified

Watch

• The description's scope section ("Deliberately one call site") names _warn_if_kiro_cli_outdated as the only sibling, but grepping "kiro-cli" argv[0] spawns in src/ counts 3 unfixed: gateway.py:2387 (declared), cli_server.py:1375, diagnostics.py:281. cli_server.py:1371 is the exact harvested pattern — shutil.which("kiro-cli") gating a bare ["kiro-cli", "update"] — undeclared. Its boundary is weaker (operator's interactive shell), but the author's own semgrep candidate fires on it.
• The surviving step is nearly dead: _TRUSTED_SYSTEM_BIN_DIRS excludes even /usr/local/bin, and the author states "kiro-cli is a per-user install," so on typical hosts the step is now a permanent logged skip.

Subtractions

• If the per-user-install premise holds everywhere that matters, delete the optional kiro-cli update step in _auto_apply_update outright — the declared fallback ("the operator runs kiro-cli update themselves") already covers the remaining hosts, and 40 lines of rationale comment plus the WARNING branch go with it.

[FIRST-PRINCIPLES-REVIEWED] 10bef45

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 10bef45b2af922c0893e1b3d7447b4dafb57f4c9.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/slack/gateway.py:9659 -- Node injection survives the pinned environment
_kiro_env = _trusted_path_env() if _kiro_cli else None
Writable startup file exports NODE_OPTIONS=--require=<payload> -> auto-update passes it to Node-based kiro-cli -> payload runs as the gateway user
Anchor: residual/security
Fix: Remove NODE_OPTIONS from _kiro_env before spawning.
[GPT-REVIEWED] 10bef45
[BLOCK-MERGE] 10bef45
False positive or not applicable? A repository writer can comment:
/ai-review override gpt 10bef45b2af922c0893e1b3d7447b4dafb57f4c9: <one-sentence reason>

[bolichen97]

Closing — the same six lines as #7712, and this mechanism resolves to None on a normal install

Thank you for this one: the motivation is right and it is being kept. The auto-update spawn on
main really does pass a bare "kiro-cli" to create_subprocess_exec, so execvp re-resolves
argv[0] off the gateway's inherited PATH. #7712 closes that same hole in the same
six-line block, so only one of the two can land there.

The reason this one is the one to close is not preference, it is a measurement. Two reviewers
ran both resolvers on a host with kiro-cli actually installed, independently of each other:

trusted_system_bin("kiro-cli")   -> None
resolve_kiro_cli()               -> /home/<user>/.local/bin/kiro-cli
trusted_system_bin("git")        -> /usr/bin/git      # control: the mechanism itself works

_TRUSTED_SYSTEM_BIN_DIRS is ("/usr/bin", "/bin", "/usr/sbin", "/sbin", "/run/current-system/sw/bin"), and kiro-cli is not in any of them. That is not a quirk of one
host: intersecting kiro_cli.known_kiro_cli_dirs() — the repository's own model of where the
binary lives — with the trusted set is empty on linux, darwin and win32.
docs/reference/kiro-cli/installation.md says the installer puts it in ~/.local/bin; macOS
uses the app bundles, /opt/homebrew/bin or /usr/local/bin; Windows uses %LOCALAPPDATA% /
%ProgramFiles%. None of those is a trusted dir.

So on every documented default install this PR's guard resolves None and the update step is
skipped rather than hardened. The three-way runtime behaviour was confirmed on that host: main
runs the update with a re-resolvable bare name, #7712 runs it with a pinned absolute path, this
PR does not run it at all.

The precedent cited — the trusted_git_bin pin on the sibling git spawns — is sound, and it
works because git lives in /usr/bin. The idiom does not carry over to a per-user binary
that by design never lands in a system directory.

The security thesis was tested, not waved away

Both reviewers checked the underlying claim that resolving through an agent-writable
~/.local/bin is itself the hole. It is a real concern, and it is not closed by either PR:
acp/client.py::_resolve_kiro_bin already resolves that identical candidate order — with
~/.local/bin first — and execs it at every session start, which this PR leaves untouched. And
since ~/.local/bin is normally on PATH, the pre-fix bare name already execs the same file. So
the marginal exposure #7712 adds is nil, and the residual belongs to a write fence rather than to
this call site.

What is being carried over from here — the valuable part

env=_trusted_path_env(). #7712 pins argv[0] but leaves the child inheriting the gateway's
whole environment; this PR narrows PATH to the trusted dirs and drops the interpreter/loader
variables (PYTHONPATH, PYTHONHOME, LD_PRELOAD, LD_LIBRARY_PATH, DYLD_*). That is
orthogonal to which resolver picks argv[0], it is the idiom the wheel-update path in the same
file already uses, and it is worth having. It is being filed as a follow-up onto #7712's absolute
path — explicitly not onto trusted_system_bin, which is what would kill the step.

Two things that follow-up has to settle with a real run rather than an assumption: kiro-cli 2.15+
is a multi-call binary that execs siblings relative to its own path, and kiro-cli update is a
downloader that needs the proxy/TLS variables (_trusted_path_env() does preserve those). Your
fail-closed refusal when _trusted_path_env() returns None should come with it.

Also carried: the test shape. TestAutoApplyUpdateKiroCliExecPin asserts the spawn itself —
argv[0] equals the resolver's answer, os.path.isabs, env equals the trusted mapping, and no
spawn at all on either refusal path. That is a sharper assertion than pinning the surrounding
behaviour, and it should survive onto the follow-up.

---

_{From a repository-wide duplicate/overlap audit of every pull request open against main (origin/main 680baf9448dc). Both PRs were read as their full merge-base diffs plus every comment and review. Because the surviving PR is the repository owner's and this one is not, the survivor question was deliberately re-decided by two further reviewers who were told the conflict of interest, were not shown the earlier answer, and were asked to settle the mechanism question by running the resolvers rather than by reading them. All three agreed, independently. If this is wrong, reopening costs nothing — say so, and treat the reasoning rather than the outcome as the thing to correct.}