diff --git a/src/kiro_crew/apps/builtins/auto_improvement/backend/clone_setup.py b/src/kiro_crew/apps/builtins/auto_improvement/backend/clone_setup.py index 19f0ba3ad88..a6cde138f65 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/backend/clone_setup.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/backend/clone_setup.py @@ -26,6 +26,7 @@ from pathlib import Path from urllib.parse import urlparse +from kiro_crew.platform.context import redact_via_context from kiro_crew.platform_compat import ( first_linked_ancestor, is_link_or_junction, @@ -491,7 +492,10 @@ def setup_safe_clone(url: str, scratch_root: Path, *, timeout_s: int = 300) -> t if proc.returncode != 0: tail = (proc.stderr or "").strip().splitlines()[-1:] or [""] rmtree_force(dest) - return {}, f"git clone failed: {tail[0][:200]}" + # Redact BEFORE the bound (here and at every sibling site below): the slice + # can cut a credential in the echoed remote URL mid-match, leaving a fragment + # no downstream redaction pass recognises. + return {}, f"git clone failed: {redact_via_context(tail[0])[:200]}" if not _repository_is_safe(dest): rmtree_force(dest) return {}, "cloned repository failed Git metadata safety verification" @@ -552,7 +556,7 @@ def list_clone_branches(clone: Path, *, timeout_s: int = 30) -> tuple[list[str], ) if proc.returncode != 0: tail = (proc.stderr or "").strip().splitlines()[-1:] or [""] - return [], f"could not list branches: {tail[0][:160]}" + return [], f"could not list branches: {redact_via_context(tail[0])[:160]}" names: list[str] = [] seen: set[str] = set() for raw in (proc.stdout or "").splitlines(): @@ -845,7 +849,7 @@ def _run(*args: str, tmo: int = timeout_s) -> subprocess.CompletedProcess: if co.returncode == 0: return True, f"checked out {bare} @ origin/{bare}" err = (co.stderr or "").strip().splitlines()[-1:] or [""] - return False, f"could not check out {bare}: {err[0][:160]}" + return False, f"could not check out {bare}: {redact_via_context(err[0])[:160]}" # The fetch failed. That is the NORMAL case here, not an edge case: this clone's # origin is neutralized to DISABLED_NO_PUSH (both urls — see `_disable_push`), so # `git fetch origin ` always exits 128. Measured against a local bare repo. @@ -874,7 +878,7 @@ def _run(*args: str, tmo: int = timeout_s) -> subprocess.CompletedProcess: if co.returncode == 0: return True, f"checked out local {bare} (fetch failed — offline?)" err = (fetched.stderr or "").strip().splitlines()[-1:] or [""] - return False, f"could not fetch {bare}: {err[0][:160]}" + return False, f"could not fetch {bare}: {redact_via_context(err[0])[:160]}" def _ok(spec: CloneSpec, dest: Path, *, reused: bool) -> dict: diff --git a/src/kiro_crew/apps/builtins/auto_improvement/backend/commit.py b/src/kiro_crew/apps/builtins/auto_improvement/backend/commit.py index 908ce2bcb7d..0fa77703bb1 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/backend/commit.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/backend/commit.py @@ -20,6 +20,7 @@ import threading from pathlib import Path +from kiro_crew.platform.context import redact_via_context from kiro_crew.security import redact from ..profiles.github_repo.pr_recipe import _prefer_authenticated_remote @@ -134,7 +135,13 @@ def materialize_queued_diff( if fetch.returncode != 0: return { "ok": False, - "error": f"could not fetch {branch}: {(fetch.stderr or '')[:160]}", + # Redact BEFORE the bound: git echoes the authenticated remote URL — + # userinfo and all — on an auth failure, and a slice can cut the + # credential mid-match into a fragment the downstream serving route's + # redaction pass no longer recognises. + "error": ( + f"could not fetch {branch}: " f"{redact_via_context(fetch.stderr or '')[:160]}" + ), } else: # No configured url: the push cannot succeed either, so this degrades to @@ -154,7 +161,10 @@ def materialize_queued_diff( if checkout.returncode != 0: return { "ok": False, - "error": f"could not check out {branch}: {(checkout.stderr or '')[:160]}", + "error": ( + f"could not check out {branch}: " + f"{redact_via_context(checkout.stderr or '')[:160]}" + ), } apply_proc = subprocess.run( @@ -173,7 +183,10 @@ def materialize_queued_diff( _git(clone, "reset", "--hard", base_ref_local) return { "ok": False, - "error": f"the queued diff did not apply: {(apply_proc.stderr or '')[:160]}", + "error": ( + f"the queued diff did not apply: " + f"{redact_via_context(apply_proc.stderr or '')[:160]}" + ), } return {"ok": True, "base": base_ref_local} @@ -197,7 +210,10 @@ def commit_staged_for_draft(*, clone: Path, body_path: Path, fp: str) -> dict[st if commit.returncode != 0: return { "ok": False, - "error": f"could not commit the staged diff: {(commit.stderr or '')[:160]}", + "error": ( + f"could not commit the staged diff: " + f"{redact_via_context(commit.stderr or '')[:160]}" + ), } return {"ok": True, "sha": (_git(clone, "rev-parse", "HEAD").stdout or "").strip()} @@ -266,7 +282,10 @@ def _commit_finding_locked(fp: str) -> dict[str, object]: commit = _git(clone, "-c", "commit.gpgsign=false", "commit", "-m", message) if commit.returncode != 0: _git(clone, "reset", "--hard", base_ref_local) - return {"ok": False, "error": f"commit failed: {(commit.stderr or '')[:160]}"} + return { + "ok": False, + "error": f"commit failed: {redact_via_context(commit.stderr or '')[:160]}", + } sha = (_git(clone, "rev-parse", "HEAD").stdout or "").strip() # Scan the CONTENT before it leaves the host. `_commit_message` is already redacted; @@ -327,7 +346,10 @@ def _commit_finding_locked(fp: str) -> dict[str, object]: push = _git(clone, "push", url, f"HEAD:refs/heads/{branch}", timeout=_PUSH_TIMEOUT_S) if push.returncode != 0: _git(clone, "reset", "--hard", base_ref_local) - return {"ok": False, "error": f"push failed: {(push.stderr or '')[:200]}"} + return { + "ok": False, + "error": f"push failed: {redact_via_context(push.stderr or '')[:200]}", + } return {"ok": True, "fp": fp, "branch": branch, "sha": sha} diff --git a/src/kiro_crew/apps/builtins/auto_improvement/backend/pr_watchers.py b/src/kiro_crew/apps/builtins/auto_improvement/backend/pr_watchers.py index d639a9561c1..1b792243847 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/backend/pr_watchers.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/backend/pr_watchers.py @@ -60,6 +60,7 @@ from pathlib import Path from typing import Any, Callable +from kiro_crew.platform.context import redact_via_context from kiro_crew.subprocess_utf8 import UTF8_TEXT from ..spine.git_safety import GIT_SAFE_CONFIG, require_pinned @@ -229,7 +230,12 @@ def setup_isolated_clone( # --local hardlinks the object store: no network, near-instant, cheap on disk. proc = _git("clone", "--local", shared_clone, dest, timeout=300) if proc.returncode != 0: - return "", f"git clone --local failed: {(proc.stderr or '').strip()[:200]}" + # Redact BEFORE the bound: a slice can cut a credential in the URL git + # echoes mid-match, leaving a fragment no downstream pass recognises. + return "", ( + f"git clone --local failed: " + f"{redact_via_context((proc.stderr or '').strip())[:200]}" + ) if branch: checkout = _git("-C", dest, "checkout", branch, timeout=60) if checkout.returncode != 0: @@ -244,7 +250,7 @@ def setup_isolated_clone( shutil.rmtree(dest, ignore_errors=True) return "", ( f"could not check out the pull request head {branch!r}: " - f"{(checkout.stderr or '').strip()[:160]}" + f"{redact_via_context((checkout.stderr or '').strip())[:160]}" ) _fetch_base_ref(dest, base_ref) neutralize_origin(dest) @@ -1331,7 +1337,7 @@ def publish_if_authorized(pr: str, status: dict[str, Any]) -> tuple[bool, str]: proc = _gh("pr", "ready", pr) if proc.returncode != 0: tail = (proc.stderr or proc.stdout or "").strip().splitlines()[-1:] or [""] - return False, f"gh pr ready failed: {tail[0][:160]}" + return False, f"gh pr ready failed: {redact_via_context(tail[0])[:160]}" logger.info("watchers: marked %s ready for review (%s)", pr, reason) return True, reason diff --git a/src/kiro_crew/apps/builtins/auto_improvement/backend/routes.py b/src/kiro_crew/apps/builtins/auto_improvement/backend/routes.py index 74f79c2e7ba..13521a5b305 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/backend/routes.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/backend/routes.py @@ -1309,11 +1309,12 @@ def _commit() -> dict[str, Any] | object: sha=str(result.get("sha") or ""), ) return web.json_response(result, status=200) - # Redacted, like every sibling error response here. `commit.py` builds its `error` from - # `(proc.stderr or '')[:160]` — raw git stderr, which quotes the ref, the path, and - # anything a repository's own hook printed. Latent while nothing rendered it; D-97 started - # showing it at the finding row, which made it a live egress path to the browser. - # Raised by the GPT review. + # Redacted, like every sibling error response here. `commit.py` builds its `error` + # from git stderr — which quotes the ref, the path, and anything a repository's own + # hook printed — scrubbed at the source with `redact_via_context` so its bound can + # never cut a credential mid-match. Latent while nothing rendered it; D-97 started + # showing it at the finding row, which made it a live egress path to the browser, so + # this pass stays as the output-boundary backstop. Raised by the GPT review. return web.json_response( {"code": "request_failed", "error": _redact_for_display(str(result.get("error") or ""))}, status=400, diff --git a/src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/pr_recipe.py b/src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/pr_recipe.py index 27b02e39cef..ca7e9585d21 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/pr_recipe.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/pr_recipe.py @@ -43,6 +43,7 @@ import subprocess from pathlib import Path +from kiro_crew.platform.context import redact_log_via_context from kiro_crew.subprocess_utf8 import UTF8_TEXT from ...spine.git_safety import GIT_SAFE_CONFIG, require_pinned @@ -417,7 +418,7 @@ def _push_fix_branch(self, *, branch: str) -> tuple[bool, str]: "push failed for %s (git exit %s): %s", branch, proc.returncode, - (proc.stderr or "").strip()[:200], + redact_log_via_context((proc.stderr or "").strip())[:200], ) return False, "push failed" return True, branch @@ -504,7 +505,9 @@ def draft( return f"QUEUED:{fingerprint}" if proc.returncode != 0: logger.warning( - "gh pr create failed for %s: %s", fingerprint, (proc.stderr or "").strip()[:200] + "gh pr create failed for %s: %s", + fingerprint, + redact_log_via_context((proc.stderr or "").strip())[:200], ) return f"QUEUED:{fingerprint}" return extract_pr_url(proc.stdout or "") or f"QUEUED:{fingerprint}" diff --git a/src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/profile.py b/src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/profile.py index d3c503d7615..1b80a414052 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/profile.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/profile.py @@ -81,6 +81,7 @@ from typing import Callable from kiro_crew import platform_compat +from kiro_crew.platform.context import redact_via_context from kiro_crew.sandbox import run_limited, sandboxed_spawn_argv from ...spine import agent_discovery @@ -971,7 +972,10 @@ def build_and_test(self, *, worktree: Path, src: Path) -> GateResult: tail = (proc.stdout or proc.stderr or "").strip().splitlines()[-1:] or [""] return GateResult( passed=False, - detail=f"suite red (exit {proc.returncode}): {tail[0][:160]}", + # Redact BEFORE the bound: a candidate's test run can echo a credential, + # and the slice can cut it mid-match into a fragment no downstream + # redaction pass recognises. + detail=f"suite red (exit {proc.returncode}): {redact_via_context(tail[0])[:160]}", failing_tests=failing, ) diff --git a/src/kiro_crew/apps/builtins/auto_improvement/spine/agent_runner.py b/src/kiro_crew/apps/builtins/auto_improvement/spine/agent_runner.py index c2654dd0a2d..8ff5aefc0e3 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/spine/agent_runner.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/spine/agent_runner.py @@ -41,6 +41,7 @@ from kiro_crew.config import KiroCrewConfig from kiro_crew.hooks import TOOL_DENY, HookManager, hooks_config_from_config_dict +from kiro_crew.platform.context import redact_via_context from kiro_crew.platform_compat import SIGKILL, kill_process_tree from kiro_crew.sandbox import popen_limited, sandboxed_spawn_argv from kiro_crew.subprocess_utf8 import UTF8_TEXT @@ -893,8 +894,14 @@ def run( dur = time.monotonic() - t0 if proc.returncode != 0: + # Redact BEFORE the tail cut: a credential straddling the bound keeps its + # right half otherwise, a fragment no downstream pass can match. Tail (not + # a head bound) because the END of stderr carries the + # actionable error; slicing redacted text can at worst split a marker. return AgentResult( - ok=False, error=f"exit {proc.returncode}: {proc.stderr[-400:]}", duration_s=dur + ok=False, + error=f"exit {proc.returncode}: {redact_via_context(proc.stderr or '')[-400:]}", + duration_s=dur, ) try: envelope = json.loads(proc.stdout) @@ -1012,7 +1019,8 @@ def _drain_stderr() -> None: except Exception: # noqa: BLE001 self._terminate_group(popen) stderr_thread.join(timeout=2.0) # let the drain finish; tail comes from its buffer - stderr_tail = ("".join(stderr_chunks))[-400:] + # Redact BEFORE the tail cut, same reason as the non-streaming path above. + stderr_tail = redact_via_context("".join(stderr_chunks))[-400:] dur = time.monotonic() - t0 with self._cost_lock: diff --git a/src/kiro_crew/apps/builtins/auto_improvement/spine/driver.py b/src/kiro_crew/apps/builtins/auto_improvement/spine/driver.py index 058046e178b..c0eec0c7b22 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/spine/driver.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/spine/driver.py @@ -38,6 +38,7 @@ from dataclasses import dataclass from pathlib import Path +from kiro_crew.platform.context import redact_log_via_context, redact_via_context from kiro_crew.subprocess_utf8 import UTF8_TEXT from . import ledger as L @@ -1480,14 +1481,23 @@ def _direct_push(self, *, fp: str, kind: str, target: str, sha: str) -> bool | N head_after = _git(["rev-parse", "HEAD"], self.clone) self.pushed_sha = (head_after.stdout or "").strip() or sha if push.returncode != 0: - self.log.error("direct-push FAILED for %s: %s", target, (push.stderr or "")[:300]) + # Redact BEFORE the bound (here and at every stderr slice below): git + # echoes the authenticated remote URL on an auth failure, and slicing + # first can cut the credential into a fragment no later pass matches. + # Log lines use the companion-aware log redactor; the persisted ledger + # note keeps the baseline redact-then-bound helper. + self.log.error( + "direct-push FAILED for %s: %s", + target, + redact_log_via_context(push.stderr or "")[:300], + ) self.ledger.record( L.LedgerEntry( fp=fp, kind=kind, target=target, status=L.STATUS_ERROR, - note=f"direct-push failed: {(push.stderr or '')[:150]}", + note=f"direct-push failed: {redact_via_context(push.stderr or '')[:150]}", ) ) return False @@ -1520,7 +1530,7 @@ def _discard_staged(self, why: str) -> None: self.log.error( "could not discard the staged diff after %s: %s", why, - (reset.stderr or "")[:200], + redact_log_via_context(reset.stderr or "")[:200], ) for rel in paths: try: @@ -1559,7 +1569,9 @@ def _stage_winner(self, winner: Proposal) -> bool: errors="surrogateescape", ) if ap.returncode != 0: - self.log.error("winner diff did not apply: %s", ap.stderr[:200]) + self.log.error( + "winner diff did not apply: %s", redact_log_via_context(ap.stderr or "")[:200] + ) return False _git(["add", "-A"], self.clone) return True @@ -1600,7 +1612,7 @@ def _commit_winner_provisional(self, winner: Proposal) -> bool: self.log.error( "provisional commit failed for %s: %s", winner.cand_id, - (commit.stderr or "")[:200], + redact_log_via_context(commit.stderr or "")[:200], ) self._discard_staged(f"a failed provisional commit for {winner.cand_id}") return False @@ -1619,7 +1631,7 @@ def _reset_provisional(self, pre_sha: str) -> None: self.log.error( "could not roll back the provisional commit to %s: %s", pre_sha[:10], - (res.stderr or "").strip()[:160], + redact_log_via_context((res.stderr or "").strip())[:160], ) def _finalize_winner_commit( @@ -1857,11 +1869,14 @@ def _apply(extra: list[str]): if ap.returncode != 0: self.log.info( "bug fix plain-apply failed (%s) — retrying with --3way", - (ap.stderr or "").strip()[:120], + redact_log_via_context((ap.stderr or "").strip())[:120], ) ap = _apply(["--3way"]) if ap.returncode != 0: - self.log.error("bug fix diff did not apply (even --3way): %s", ap.stderr[:200]) + self.log.error( + "bug fix diff did not apply (even --3way): %s", + redact_log_via_context(ap.stderr or "")[:200], + ) return False _git(["add", "-A"], self.clone) return True @@ -1892,7 +1907,7 @@ def _commit_bug_winner_provisional(self, winner: Proposal) -> bool: self.log.error( "provisional bug commit failed for %s: %s", winner.cand_id, - (commit.stderr or "")[:200], + redact_log_via_context(commit.stderr or "")[:200], ) self._discard_staged(f"a failed provisional bug commit for {winner.cand_id}") return False diff --git a/src/kiro_crew/apps/builtins/auto_improvement/spine/gate.py b/src/kiro_crew/apps/builtins/auto_improvement/spine/gate.py index fa7faba6cf9..96c3da2e349 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/spine/gate.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/spine/gate.py @@ -33,6 +33,7 @@ import subprocess from pathlib import Path +from kiro_crew.platform.context import redact_via_context from kiro_crew.subprocess_utf8 import UTF8_TEXT from .bug_gate import BugGate @@ -102,7 +103,7 @@ def _changed_status_paths(worktree: Path, base_sha: str) -> list[tuple[str, str] # the error so the caller rejects the candidate instead of admitting it. raise RuntimeError( f"git diff failed (rc={r.returncode}) for base {base_sha!r}: " - f"{(r.stderr or '').strip()[:200]}" + f"{redact_via_context((r.stderr or '').strip())[:200]}" ) out: list[tuple[str, str]] = [] for line in r.stdout.splitlines(): diff --git a/src/kiro_crew/apps/builtins/auto_improvement/tests/test_dogfood_learnings.py b/src/kiro_crew/apps/builtins/auto_improvement/tests/test_dogfood_learnings.py index b22b6d4ed30..df72498536b 100644 --- a/src/kiro_crew/apps/builtins/auto_improvement/tests/test_dogfood_learnings.py +++ b/src/kiro_crew/apps/builtins/auto_improvement/tests/test_dogfood_learnings.py @@ -6519,9 +6519,11 @@ def test_the_profile_passes_its_track_to_the_fence(self) -> None: class TestCommitErrorsReachTheBrowserRedacted: - """The commit route returned `str(result.get("error"))` verbatim, and `commit.py` builds - that value from `(proc.stderr or '')[:160]` — RAW GIT STDERR, which quotes the ref, the - path, and whatever a repository's own hooks printed. + """The commit route returned `str(result.get("error"))` verbatim, and `commit.py` built + that value from a raw fixed-bound slice of git stderr — which quotes the ref, the + path, and whatever a repository's own hooks printed. (`commit.py` now scrubs its + stderr at the source with `redact_via_context`; this route-level pass stays as the + output-boundary backstop.) This was latent while nothing rendered it. D-97 (surfacing a refused commit at the finding row, so the operator learns WHY) turned it into a live egress path: a failing pre-commit diff --git a/src/kiro_crew/apps/builtins/auto_improvement/tests/test_stderr_redact_before_bound.py b/src/kiro_crew/apps/builtins/auto_improvement/tests/test_stderr_redact_before_bound.py new file mode 100644 index 00000000000..0fa573e1c0c --- /dev/null +++ b/src/kiro_crew/apps/builtins/auto_improvement/tests/test_stderr_redact_before_bound.py @@ -0,0 +1,166 @@ +"""Redact-before-bound on subprocess stderr across the auto-improvement app. + +Several error payloads and log lines quote git stderr bounded to a fixed +character count. ``commit.py`` reaches git with an AUTHENTICATED remote URL +(``materialize_queued_diff`` passes ``_prefer_authenticated_remote``'s result as +argv), and on an auth failure git echoes that URL — userinfo and all — to +stderr. Bounding BEFORE redaction can cut the credential mid-match, leaving a +prefix that no longer matches any credential regex, so the downstream serving +route's own redaction pass (``routes.py``'s ``_redact_for_display``) cannot +recognise it either. The fix is redact-then-bound through the companion-aware +context shims (``redact_via_context`` for payloads, ``redact_log_via_context`` +for log lines; ``security.redact_and_truncate`` remains the baseline spelling +elsewhere in the tree), which scrub +the full text first and bounds after. + +The behavioral tests here pin the highest-reachability site (the fetch failure +in ``materialize_queued_diff``) with the straddle layout: the secret is placed +so the bound falls INSIDE it, so a raw slice AND a slice-then-redact reorder +both go red. The structural sweep then pins the whole class across every +non-test module of the app — head slices, tail slices (a tail cut keeps the +credential's RIGHT half, which equally matches nothing), and the +``tail[0][:N]`` / ``err[0][:N]`` last-line alias forms — so re-introducing a +raw bounded stderr slice at any sibling site fails without needing a per-site +behavioral test. +""" + +from __future__ import annotations + +import re +import subprocess +from pathlib import Path + +import pytest + +from kiro_crew.apps.builtins.auto_improvement.backend import commit as commit_mod + +_APP_ROOT = Path(__file__).resolve().parents[1] + +# The bound applied at the fetch-failure site in materialize_queued_diff. +_FETCH_BOUND = 160 + + +def _proc(rc: int, stderr: str = "") -> subprocess.CompletedProcess: + return subprocess.CompletedProcess(args=["git"], returncode=rc, stdout="", stderr=stderr) + + +class TestFetchFailureStderrIsRedactedBeforeTheBound: + """The commit path's fetch failure must never serve a raw credential fragment.""" + + def _materialize_with_fetch_stderr( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, stderr: str + ) -> dict[str, object]: + # Route the flow down the remote-url branch without touching a network + # or a real repository: the config resolves to a URL, the authenticated + # form is a fixed fake, and the ONLY git call made is the failing fetch. + monkeypatch.setattr(commit_mod, "resolve_origin_url", lambda cfg: "https://x.test/r.git") + monkeypatch.setattr( + commit_mod, "_prefer_authenticated_remote", lambda url: "https://u:t@x.test/r.git" + ) + calls: list[tuple] = [] + + def _git(clone, *args, **kw): + calls.append(args) + assert args[0] == "fetch", f"unexpected git call before the fetch failed: {args}" + return _proc(1, stderr=stderr) + + monkeypatch.setattr(commit_mod, "_git", _git) + out = commit_mod.materialize_queued_diff( + clone=tmp_path, branch="main", config={}, diff_text="--- a\n+++ b\n" + ) + assert calls, "the fetch was never attempted, so this test exercised nothing" + return out + + def test_a_fetch_failure_does_not_leak_the_remote_credential( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + token = "s3cr3t-remote-token" # a fake secret, only asserted absent + stderr = f"fatal: could not read from 'https://ci-bot:{token}@github.example/r.git'\n" + out = self._materialize_with_fetch_stderr(monkeypatch, tmp_path, stderr) + assert out["ok"] is False + assert token not in str(out["error"]) + assert "[REDACTED" in str(out["error"]) + + def test_the_credential_is_redacted_before_it_is_bounded( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + """Straddle layout: the 160-char bound falls INSIDE the token. + + A raw slice keeps the token's left half verbatim. A slice-then-redact + reorder keeps it too, because the cut drops the ``@host`` tail the + userinfo regex needs to match. Only redact-then-bound scrubs it, so + this test goes red if the two steps are ever reordered. + """ + token = "TOKENedge9876543210" # a fake secret, only asserted absent + userinfo = "https://ci-bot:" + # Lay the token out to START ten characters shy of the bound, so the + # bound cuts into it and the '@host' tail lands beyond the bound. + pad = "x" * (_FETCH_BOUND - 10 - len("fatal: ") - len(userinfo)) + line = f"fatal: {pad}{userinfo}{token}@github.example/r.git" + # Premise guards: the layout must actually straddle, or this test + # silently stops pinning the invariant. + start = line.index(token) + assert start < _FETCH_BOUND < start + len(token) + assert line.index("@") > _FETCH_BOUND + + out = self._materialize_with_fetch_stderr(monkeypatch, tmp_path, line + "\n") + assert out["ok"] is False + err = str(out["error"]) + assert token not in err + # The exact fragment a bound-before-redact implementation would leak — + # everything of the token left of the bound — must be absent too. + leaked_prefix = token[: _FETCH_BOUND - start] + assert leaked_prefix and leaked_prefix not in err + + +class TestNoRawBoundedStderrSliceAnywhereInTheApp: + """Structural class pin: the whole app is swept, not just the fixed sites. + + The defect recurred file by file (commit.py, pr_watchers.py, gate.py, + driver.py, pr_recipe.py all carried it), so a per-site behavioral test + cannot keep the class closed. Any non-test module that slices stderr to a + bound must redact-then-bound through a redactor shim instead. + """ + + # The shapes redaction can no longer see through once the slice has run: + # - a stderr expression head-sliced to a bound, with or without an interposed + # `or ''` default or `.strip()` -> stderr...[:N] + # - a stderr expression (or a var named like one) TAIL-sliced to a bound + # -> stderr...[-400:]; the multi-digit floor keeps the legitimate + # `splitlines()[-1:]` last-LINE idiom out (a whole line cuts no secret; + # it is the CHAR slice applied to it afterwards that does) + # - that idiom's conventional aliases char-sliced -> tail[0][:N] / err[0][:N] + # Known limits, accepted: the scan is per-line (a wrapped site whose stderr + # expression and slice land on different lines evades it), and an alias + # renamed away from tail/err evades the third alternative. proposer.py's + # unbounded `r.stderr.strip()` RuntimeError is deliberately out of scope: + # with no slice, a credential in it keeps its full shape, which downstream + # pattern-based redaction can still match. + _RAW_SLICE = re.compile( + r"stderr\b[^\n]*\[:\d+\]" r"|stderr\w*\b[^\n]*\[-\d{2,}:\]" r"|\b(?:tail|err)\[0\]\[:\d+\]" + ) + # The sanctioned forms: redact the FULL text, then cut the redactor's RESULT. + # A slice of already-redacted text can at worst split a redaction marker, + # never a secret. Covers redact(x)[-N:], redact_and_truncate's callers, and + # the companion-aware log spelling redact_log_via_context(x)[:N]. The + # trailing (?!\)) keeps the mirror-image defect flagged: in + # redactor((stderr)[:N]) the slice sits INSIDE the call, so a ')' follows + # the bracket and the line stays an offender. + _SANCTIONED_TAIL = re.compile(r"redact\w*\([^\n]*\)\s*\[(?:-\d+:|:\d+)\](?!\))") + + def test_no_module_slices_stderr_before_redaction(self) -> None: + offenders: list[str] = [] + for path in sorted(_APP_ROOT.rglob("*.py")): + rel = path.relative_to(_APP_ROOT).as_posix() + if rel.startswith("tests/"): + continue + for lineno, text in enumerate( + path.read_text(encoding="utf-8", errors="replace").splitlines(), start=1 + ): + if self._RAW_SLICE.search(text) and not self._SANCTIONED_TAIL.search(text): + offenders.append(f"{rel}:{lineno}: {text.strip()}") + assert offenders == [], ( + "raw bounded stderr slice (redaction cannot match a cut credential); " + "use redact_via_context(text)[:bound] (or the log spelling) instead: " + + "; ".join(offenders) + ) diff --git a/src/kiro_crew/security_posture.py b/src/kiro_crew/security_posture.py index c3b745b1070..7f52fcfec42 100644 --- a/src/kiro_crew/security_posture.py +++ b/src/kiro_crew/security_posture.py @@ -442,8 +442,10 @@ class PostureControl: "scanned): it reached the browser verbatim. The session " "records go through it too, because `save_session` merges the caller's patch and the " "stored `title` is built from a finding's target. So do the route ERROR bodies: " - "`commit.py` builds its `error` from `(proc.stderr or '')[:160]` — raw git stderr, " - "which quotes refs, paths and whatever a repository's own hooks printed. That was " + "`commit.py` builds its `error` from git stderr — which quotes refs, paths and " + "whatever a repository's own hooks printed — scrubbed at that source " + "(redact-then-bound) so its character bound can never cut a credential mid-match. " + "That was " "latent while nothing rendered it; surfacing a refused commit at the finding row made " 'it a live path to the browser, so all five `result.get("error")` responses plus the ' "PR-status and draft bodies are scanned. " @@ -1376,6 +1378,22 @@ class PostureControl: # through ``_handle_deps_install`` in routes.py, the registered sink # for this app. "apps/builtins/auto_improvement/backend/deps.py", + # Same source-side pre-pass shape, for the app's git/gh subprocess stderr: + # git echoes the remote's userinfo URL on an auth failure, and the fixed + # character bound applied to each error string can cut a credential + # mid-match, so the companion-aware redact_via_context runs where the + # string is BUILT, before the bound. + # None of these owns an output boundary: + # - gate.py's scrubbed RuntimeError becomes the gate verdict detail, which + # travels to the ledger and the candidate detail — surfaces owned by the + # spine driver and the backend routes, this app's registered sinks. + # - clone_setup.py's error strings reach the dashboard only through the + # backend routes (the registered sink for this app). + # - profile.py's GateResult.detail follows the same ledger/candidate path as + # gate.py's verdicts. + "apps/builtins/auto_improvement/backend/clone_setup.py", + "apps/builtins/auto_improvement/profiles/github_repo/profile.py", + "apps/builtins/auto_improvement/spine/gate.py", # Inbound: the crew worker's slot title is derived from an issue title, # which is untrusted text anyone who can open an issue wrote. It is # scrubbed before it becomes a slot title (and fails CLOSED to the slot