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

src/kiro_crew/slack/gateway.py

@@ -9116,7 +9116,7 @@ async def _auto_apply_update(self) -> None:
         # kill-the-tree + bounded-reap contract, and there is exactly one
         # implementation of it (issue #4210 exists to close the two-conventions
         # gap, not to add a second helper).
-        from kiro_crew.platform.update_provider import _kill_and_reap
+        from kiro_crew.platform.update_provider import _kill_and_reap, _trusted_path_env
 
         try:
             # Every git call below reads a tree an agent can write, and several of
@@ -9621,13 +9621,58 @@ def _obstructions(name: str) -> bool:
                 return
             logger.info("Auto-update: reset to origin/%s, rebuilding", branch)
 
-            # Update the optional kiro-cli backend if present.
-            if shutil.which("kiro-cli"):
+            # Update the optional kiro-cli backend, resolved off `PATH` and
+            # spawned with an explicit `env` for the same reason `_git` above is.
+            # This call site was left out of the pass that hardened the seven git
+            # spawns in this function.
+            #
+            # `shutil.which` was an EXISTENCE check whose answer was discarded:
+            # the spawn passed the bare name `"kiro-cli"`, so `execvp` re-resolved
+            # it off `PATH` at spawn time. A gateway's `PATH` can lead with an
+            # agent-writable directory (a worktree venv's `bin`, `~/.local/bin`),
+            # so anything able to plant a `kiro-cli` there ahead of the real
+            # install ran as the gateway on an UNATTENDED path whose output is
+            # DEVNULL'd and whose failures log at DEBUG.
+            #
+            # `trusted_system_bin` probes the fixed system directories and
+            # nothing else — deliberately NOT `resolve_kiro_cli`, the resolver
+            # the ACP launch path uses. That one reads `KIROCREW_KIRO_BIN` and
+            # `~/.local/bin` FIRST, and both are agent-writable, so it would
+            # close only the PATH-ordering half of this hole and leave the
+            # planted-binary half wide open on the one path that runs unattended.
+            #
+            # The cost is real and accepted: kiro-cli is a per-user install, so
+            # on a host where it 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` and
+            # `_trusted_path_env` already make in the update path — an
+            # unattended exec does not get to fall back to a lookup an agent can
+            # influence — and it costs an optional backend refresh, never the
+            # gateway's own update.
+            _kiro_cli = platform_compat.trusted_system_bin("kiro-cli")
+            # Pinning the binary is only half of it, exactly as on the wheel path
+            # below: the child resolves its own helper words through the
+            # inherited `PATH`. Narrow that to the trusted system dirs and drop
+            # the interpreter/loader variables, and fail CLOSED when there is no
+            # trusted `PATH` to substitute rather than handing the child an
+            # agent-influenceable lookup.
+            _kiro_env = _trusted_path_env() if _kiro_cli else None
+            if _kiro_cli and _kiro_env is None:
+                # Skipped, not silently unpinned. This step is optional and
+                # non-fatal, so the rest of the update still applies; saying so
+                # keeps the degradation diagnosable instead of looking like "no
+                # kiro-cli installed".
+                logger.warning(
+                    "Auto-update: skipping the kiro-cli backend update — no trusted "
+                    "PATH to hand the child. Run `kiro-cli update` manually."
+                )
+            if _kiro_cli and _kiro_env is not None:
                 kiro_update: asyncio.subprocess.Process | None = None
                 try:
                     kiro_update = await asyncio.create_subprocess_exec(
-                        "kiro-cli",
+                        _kiro_cli,
                         "update",
+                        env=_kiro_env,
                         stdout=asyncio.subprocess.DEVNULL,
                         stderr=asyncio.subprocess.DEVNULL,
                         # Own process group (POSIX; no-op on Windows) so the

test/test_slack_gateway.py

@@ -16,6 +16,7 @@
 
 import pytest
 
+from kiro_crew import platform_compat
 from kiro_crew.autonudge import NudgeLoop
 from kiro_crew.config.loader import KiroCrewConfig
 from kiro_crew.slack import gateway as gw
@@ -2417,6 +2418,13 @@ async def test_enabled_creates_service(self):
 # ═══════════════════════════════════════════════════════════════════════════
 
 
+#: A stand-in for a RESOLVED kiro-cli path. Absolute and in a SYSTEM directory on
+#: purpose: the spawn under test is pinned to what `trusted_system_bin` returns,
+#: so a bare ``"kiro-cli"`` here would no longer match and the fakes below would
+#: silently stop covering the step.
+_KIRO_CLI_BIN = str(Path("/usr/bin/kiro-cli"))
+
+
 def _git_exec_fake(
     *,
     branch: bytes = b"mainline\n",
@@ -3435,7 +3443,18 @@ async def test_venv_update_full_path(self):
                         ):
                             with patch("kiro_crew.slack.gateway.build_frontend_async", new_callable=AsyncMock):
                                 with patch("os.execv", side_effect=OSError("test")):
-                                    with patch("shutil.which", return_value=None):
+                                    # No kiro-cli in a system directory: the
+                                    # optional backend step is skipped. Pinned on
+                                    # the RESOLVER, not on `shutil.which`, which
+                                    # no longer gates it. Name-dispatched so git
+                                    # still resolves (see `_resolver` above).
+                                    with patch.object(
+                                        platform_compat,
+                                        "trusted_system_bin",
+                                        side_effect=(
+                                            TestAutoApplyUpdateKiroCliExecPin._resolver(None)
+                                        ),
+                                    ):
                                         await orch._auto_apply_update()
 
         # The install runs through the shared entry point, which picks a reinstall
@@ -3471,7 +3490,7 @@ async def test_kiro_cli_update_timeout_kills_child_and_stays_nonfatal(self):
 
         async def _fake_exec(*args, **kwargs):
             argv = [a for a in args if isinstance(a, str)]
-            if argv and argv[0] == "kiro-cli":
+            if argv and argv[0] == _KIRO_CLI_BIN:
                 proc = AsyncMock()
                 proc.kill = MagicMock()
                 proc.returncode = None
@@ -3504,9 +3523,21 @@ async def _fake_kill_and_reap(proc):
                                 new_callable=AsyncMock,
                             ) as mock_build:
                                 with patch("os.execv", side_effect=OSError("test")):
-                                    # Truthy: the optional kiro-cli step runs.
-                                    with patch(
-                                        "shutil.which", return_value="/usr/bin/kiro-cli"
+                                    # Resolved + a trusted env: the optional
+                                    # kiro-cli step runs. Both are now required
+                                    # (the spawn is pinned), and `shutil.which`
+                                    # no longer gates it.
+                                    with patch.object(
+                                        platform_compat,
+                                        "trusted_system_bin",
+                                        side_effect=(
+                                            TestAutoApplyUpdateKiroCliExecPin._resolver(
+                                                _KIRO_CLI_BIN
+                                            )
+                                        ),
+                                    ), patch(
+                                        "kiro_crew.platform.update_provider._trusted_path_env",
+                                        return_value={"PATH": "/usr/bin:/bin"},
                                     ):
                                         # The gateway resolves _kill_and_reap
                                         # function-locally on every call, so
@@ -3526,6 +3557,184 @@ async def _fake_kill_and_reap(proc):
         assert mock_install.call_count == 1
 
 
+# ═══════════════════════════════════════════════════════════════════════════
+# Tests: _auto_apply_update kiro-cli exec pin
+# ═══════════════════════════════════════════════════════════════════════════
+
+
+class TestAutoApplyUpdateKiroCliExecPin:
+    """The optional `kiro-cli update` step resolves off `PATH` and pins its env.
+
+    The seven git spawns in `_auto_apply_update` were pinned to
+    ``trusted_git_bin()`` + ``git_command_env()``; this one was left passing the
+    BARE NAME ``"kiro-cli"`` with ``shutil.which`` used only as an existence
+    check, so ``execvp`` re-resolved it off ``PATH`` at spawn time and no ``env``
+    was pinned. A gateway's ``PATH`` can lead with an agent-writable directory,
+    and this runs unattended with output DEVNULL'd — so a planted shim was
+    arbitrary code execution as the gateway, silently.
+
+    These assert the SPAWN, not the source. A source-string check ("does the file
+    say `resolve_kiro_cli`") executes none of the new code and passes on a call
+    that still hands `create_subprocess_exec` a re-resolvable name.
+    """
+
+    @pytest.fixture(autouse=True)
+    def _permit_update_preconditions(self):
+        """Neutralize the seam preconditions so these tests keep their subject.
+
+        Same reason as ``TestAutoApplyUpdateVenvPath``: the refusals read the
+        REAL git metadata of ``KIROCREW_PROJECT_DIR``, which is not a repo here,
+        so without this every test below would pass vacuously by refusing before
+        it ever reached the reset — let alone the kiro-cli step after it.
+        """
+        with patch(
+            "kiro_crew.slack.gateway.hidden_worktree_edits", return_value=[]
+        ), patch(
+            "kiro_crew.slack.gateway.repo_exec_config_reason",
+            return_value="",
+        ), patch(
+            "kiro_crew.slack.gateway.tracks_upstream", return_value=True
+        ), patch(
+            "kiro_crew.slack.gateway.commits_ahead", return_value=0
+        ):
+            yield
+
+    @staticmethod
+    def _resolver(resolved):
+        """A `trusted_system_bin` stub that answers only for ``kiro-cli``.
+
+        Name-dispatched, not a blanket return: `trusted_git_bin()` resolves git
+        THROUGH `trusted_system_bin`, so a stub that answered every name would
+        also redirect the seven git spawns and the test would stop exercising
+        the path it claims to.
+        """
+        real = platform_compat.trusted_system_bin
+
+        def _stub(name):
+            return resolved if name == "kiro-cli" else real(name)
+
+        return _stub
+
+    @staticmethod
+    async def _drive(*, resolved, trusted_env, spawns, caplog=None):
+        """Run `_auto_apply_update` to completion, recording every spawn.
+
+        *resolved* is what the kiro-cli resolver returns, *trusted_env* what the
+        trusted-PATH builder returns. Everything past the reset (frontend build,
+        dependency install, re-exec) is stubbed so the run reaches — and gets
+        past — the optional backend step under test.
+        """
+        orch = _make_orchestrator()
+        orch.dashboard_state = _mock_dashboard_state()
+        orch.sessions = _mock_sessions()
+        _fake_exec = _git_exec_fake(record=spawns)
+
+        with patch("kiro_crew.env.is_toolbox_install", return_value=False), patch.dict(
+            "os.environ", {"KIROCREW_PROJECT_DIR": "/tmp/proj"}
+        ), patch(
+            "asyncio.create_subprocess_exec", side_effect=_fake_exec
+        ) as mock_spawn, patch(
+            "kiro_crew.dep_sync.sync_or_reinstall", return_value=0
+        ), patch.object(
+            GatewayOrchestrator, "_is_brazil_install", return_value=False
+        ), patch(
+            "kiro_crew.slack.gateway.build_frontend_async", new_callable=AsyncMock
+        ) as mock_build, patch(
+            "os.execv", side_effect=OSError("test")
+        ), patch.object(
+            platform_compat,
+            "trusted_system_bin",
+            side_effect=TestAutoApplyUpdateKiroCliExecPin._resolver(resolved),
+        ), patch(
+            "kiro_crew.platform.update_provider._trusted_path_env",
+            return_value=trusted_env,
+        ):
+            await orch._auto_apply_update()
+        return mock_spawn, mock_build
+
+    @staticmethod
+    def _kiro_calls(mock_spawn, *, needle="kiro-cli"):
+        """Every recorded spawn whose argv[0] names the kiro-cli binary."""
+        return [
+            call
+            for call in mock_spawn.call_args_list
+            if call.args and isinstance(call.args[0], str) and needle in call.args[0]
+        ]
+
+    @pytest.mark.asyncio
+    async def test_spawn_uses_resolved_absolute_path_and_explicit_env(self):
+        """argv[0] is the resolver's ABSOLUTE path and an `env` is passed.
+
+        Both halves are the fix. An absolute argv[0] stops `execvp` re-resolving
+        the name off `PATH` at spawn time; the explicit `env` stops the child
+        resolving its own helper words through the gateway's inherited `PATH`.
+        Asserting only the first would pass on a spawn that still inherits it.
+        """
+        resolved = _KIRO_CLI_BIN
+        pinned_env = {"PATH": "/usr/bin:/bin"}
+        spawns: list = []
+
+        mock_spawn, _ = await self._drive(
+            resolved=resolved, trusted_env=pinned_env, spawns=spawns
+        )
+
+        calls = self._kiro_calls(mock_spawn)
+        assert len(calls) == 1, f"expected one kiro-cli spawn, got {calls}"
+        (call,) = calls
+        # The resolved path, not the bare name: `os.path.isabs` is the property
+        # that matters, and the exact value pins that the RESOLVER's answer is
+        # what is spawned rather than a coincidentally-absolute constant.
+        assert call.args[0] == resolved
+        assert os.path.isabs(call.args[0])
+        assert call.args[1] == "update"
+        # `env` present AND the trusted one. `env=None` means "inherit", which is
+        # the pre-fix behaviour and would satisfy a bare `"env" in kwargs`.
+        assert call.kwargs.get("env") == pinned_env
+
+    @pytest.mark.asyncio
+    async def test_step_skipped_when_resolution_returns_none(self):
+        """No kiro-cli in a trusted directory: skipped, never spawned by name.
+
+        The pre-fix code reached the spawn on a truthy `shutil.which` and handed
+        over a bare name. A resolver returning None must produce NO spawn at all
+        — falling back to `"kiro-cli"` reinstates the hazard — and must leave the
+        rest of the update running, since this step is optional.
+        """
+        spawns: list = []
+
+        mock_spawn, mock_build = await self._drive(
+            resolved=None, trusted_env={"PATH": "/usr/bin:/bin"}, spawns=spawns
+        )
+
+        assert self._kiro_calls(mock_spawn) == []
+        # Skipping is non-fatal: the frontend build after it still ran.
+        mock_build.assert_awaited_once()
+
+    @pytest.mark.asyncio
+    async def test_step_skipped_when_no_trusted_env(self, caplog):
+        """A resolved binary but no trusted PATH fails CLOSED, and says so.
+
+        Mirrors the wheel path's `_trusted_path_env() is None` refusal in this
+        same file. Handing the inherited PATH over instead would reopen the
+        second hop the pin exists to close. Logged at WARNING rather than DEBUG
+        so the degradation is not indistinguishable from "no kiro-cli installed".
+        """
+        spawns: list = []
+
+        with caplog.at_level(logging.WARNING, logger="kiro_crew.slack.gateway"):
+            mock_spawn, mock_build = await self._drive(
+                resolved=_KIRO_CLI_BIN,
+                trusted_env=None,
+                spawns=spawns,
+            )
+
+        assert self._kiro_calls(mock_spawn) == []
+        mock_build.assert_awaited_once()
+        assert any(
+            "kiro-cli backend update" in r.getMessage() for r in caplog.records
+        ), f"no refusal logged: {[r.getMessage() for r in caplog.records]}"
+
+
 # ═══════════════════════════════════════════════════════════════════════════
 # Tests: Subagent Slack injection timeout
 # ═══════════════════════════════════════════════════════════════════════════