diff --git a/agent-team/agent_team/ci_fetcher.py b/agent-team/agent_team/ci_fetcher.py index b2bbdc7..049a0c5 100644 --- a/agent-team/agent_team/ci_fetcher.py +++ b/agent-team/agent_team/ci_fetcher.py @@ -101,10 +101,23 @@ _ALLOWED_CONCLUSIONS: frozenset[str] = frozenset( } ) -# The dedicated read-only token env var (preferred), falling back to the generic -# GITHUB_TOKEN (the idiom github_live uses). The token MUST be read-only — the -# fetcher only ever GETs; a write-scoped token here would be unnecessary blast -# radius (provisioning issues a read-only fine-grained PAT / read-only var). +# The dedicated read-only token env var (PREFERRED on the live box), falling back +# to the generic GITHUB_TOKEN (the idiom github_live uses). The token MUST be +# read-only — the fetcher only ever GETs; a write-scoped token here would be +# unnecessary blast radius (provisioning issues a read-only fine-grained PAT / +# read-only var). +# +# SEC-04 (CWE-269): the GITHUB_TOKEN fallback is a *documented, explicitly- +# narrowed convenience* for the github_live idiom — on the live box GITHUB_TOKEN +# MUST be read-only (contents:read). Provisioning issues the dedicated +# AGENT_TEAM_CI_READ_TOKEN for this read path and that name is the preferred +# source; GITHUB_TOKEN is only the fallback. Because the no-standing-write-token +# audit (scripts/assert_no_write_token.py) name-exempts GITHUB_TOKEN from the +# write-token *name* heuristic, it deliberately does NOT exempt GITHUB_TOKEN from +# the write-token *value* scan: a write-scoped PAT/installation token +# (ghp_/ghs_/...) parked in GITHUB_TOKEN on the box is still flagged there. This +# fetcher itself is unconditionally read-only (single GET, no write verbs), so +# even a mistakenly write-scoped token is never exercised for a write here. CI_READ_TOKEN_ENV = "AGENT_TEAM_CI_READ_TOKEN" _FALLBACK_TOKEN_ENV = "GITHUB_TOKEN" @@ -118,9 +131,13 @@ _DEFAULT_TIMEOUT_S = 15.0 def _resolve_read_token() -> str | None: """Resolve the read-only GitHub token at call time, or ``None``. - Prefers :data:`CI_READ_TOKEN_ENV`, falls back to ``GITHUB_TOKEN``. Returns - ``None`` when neither is set so the fetcher fails closed (the caller maps a - missing token to a ``None`` result -> gate BLOCK), rather than raising. + PREFERS the dedicated read-only :data:`CI_READ_TOKEN_ENV` + (``AGENT_TEAM_CI_READ_TOKEN``); falls back to ``GITHUB_TOKEN`` only as the + documented, explicitly-narrowed convenience described above — on the live box + ``GITHUB_TOKEN`` MUST be read-only (the no-standing-write-token audit + value-scans it, and this fetcher only ever issues a single read-only GET). + Returns ``None`` when neither is set so the fetcher fails closed (the caller + maps a missing token to a ``None`` result -> gate BLOCK), rather than raising. """ return os.environ.get(CI_READ_TOKEN_ENV) or os.environ.get(_FALLBACK_TOKEN_ENV) diff --git a/agent-team/agent_team/nodes/verifier.py b/agent-team/agent_team/nodes/verifier.py index 069f5c2..e318753 100644 --- a/agent-team/agent_team/nodes/verifier.py +++ b/agent-team/agent_team/nodes/verifier.py @@ -15,10 +15,12 @@ fix hint for the builders. The LLM is never asked whether the task passed. State contract (mirrors :class:`agent_team.task_model.PipelineState`): -* reads ``candidate_diff``, ``diff_hash`` (ledger hash), ``ci_results``; +* reads ``candidate_diff``, ``diff_hash`` (ledger hash), ``ci_results``, + ``build_loops`` (the durable per-task build<->verify count); * writes ``status``, ``current_phase``, ``review_verdicts`` (appends the gate - verdict), and ``ci_results`` (annotated with the gate decision for - provenance). + verdict), ``ci_results`` (annotated with the gate decision for provenance), + and — on a recoverable gate FAIL — the incremented ``build_loops`` so the + budget advances across the BUILD->DISPATCH->VERIFY loop (LOGIC-RACE-01). Transitions (the §3.3 "Stability + autonomy bounds" — the verifier must pass or the task loops/holds, never ships): @@ -73,8 +75,18 @@ class VerifierConfig: ``None`` effective run id is a BLOCK (never a vacuous pass). ``allowed_scope`` is the task's declared-scope path prefixes for the denylist boundary. ``max_build_loops`` caps build<->verify retries before - the task parks. ``build_loops`` is the loops already consumed for this task - (the coordinator threads it through state). + the task parks. + + ``build_loops`` is an INITIAL FALLBACK ONLY. The loop count that actually + bounds the build<->verify cycle is DURABLE per-task state read from + ``state["build_loops"]`` at node-run time, because a single ``VerifierConfig`` + is shared across every task at wiring time and never advances (LOGIC-RACE-01: + reading the count from this shared config meant the park guard never fired and + a perpetually-FAILing task looped BUILD->DISPATCH->VERIFY forever). The + verifier writes the incremented count back into the returned partial state so + the checkpointer carries it to the NEXT VERIFY. This field is consulted only + when state carries no ``build_loops`` (e.g. a unit harness driving the node + directly). """ expected_run_id: str | None = None @@ -183,6 +195,17 @@ def verifier_node( else config.expected_run_id ) + # Per-task build-loop budget: the count that bounds the build<->verify cycle + # is DURABLE per-task state (``state["build_loops"]``), NOT the shared + # wiring-time config. Reading it from state is the LOGIC-RACE-01 fix: the + # shared ``VerifierConfig.build_loops`` never advanced, so the park guard + # never fired and a perpetually-FAILing task looped forever. ``config`` is + # only a fallback for a harness that drives the node without per-task state. + state_build_loops = state.get("build_loops") + current_build_loops = ( + state_build_loops if isinstance(state_build_loops, int) else config.build_loops + ) + if not isinstance(candidate_diff, str): # No diff to verify is itself a refuse-to-proceed: park for a human # rather than declaring anything. (A builder must have produced a diff @@ -209,7 +232,7 @@ def verifier_node( if gate_result.decision is GateDecision.PASS: verdict = _verdict( - gate_result, next_phase=Phase.DONE, build_loops=config.build_loops + gate_result, next_phase=Phase.DONE, build_loops=current_build_loops ) return { "status": TaskStatus.DONE.value, @@ -224,7 +247,8 @@ def verifier_node( fix_hint = _fix_advisor(gate_result, state) if gate_result.decision is GateDecision.FAIL: - next_loops = config.build_loops + 1 + # Count this failed loop against the DURABLE per-task budget read above. + next_loops = current_build_loops + 1 if next_loops >= config.max_build_loops: # Exhausted the build-loop budget: hold rather than spin (§3.3 #6). verdict = _verdict( @@ -241,6 +265,7 @@ def verifier_node( "current_phase": Phase.PARKED.value, "review_verdicts": [verdict], "ci_results": annotated_ci, + "build_loops": next_loops, "updated_at": verdict["at"], } verdict = _verdict( @@ -249,11 +274,15 @@ def verifier_node( build_loops=next_loops, fix_hint=fix_hint, ) + # Persist the incremented count into DURABLE state so the NEXT VERIFY + # (after BUILD->DISPATCH) sees it and the budget actually advances. The + # checkpointer carries it because it is a PipelineState key. return { "status": TaskStatus.ACTIVE.value, "current_phase": Phase.BUILD.value, "review_verdicts": [verdict], "ci_results": annotated_ci, + "build_loops": next_loops, "updated_at": verdict["at"], } @@ -262,7 +291,7 @@ def verifier_node( verdict = _verdict( gate_result, next_phase=Phase.PARKED, - build_loops=config.build_loops, + build_loops=current_build_loops, fix_hint=fix_hint, ) return { diff --git a/agent-team/agent_team/task_model.py b/agent-team/agent_team/task_model.py index c1efadd..5391f95 100644 --- a/agent-team/agent_team/task_model.py +++ b/agent-team/agent_team/task_model.py @@ -108,6 +108,12 @@ class TaskRecord: run_id: str | None = None ci_correlation_tag: str | None = None dispatched_at: str | None = None + # Count of build<->verify loops already consumed for THIS task (§3.3 #6). + # Durable per-task state (NOT the shared wiring-time VerifierConfig): the + # verifier reads it from state, increments on a recoverable gate FAIL, and + # parks once it reaches VerifierConfig.max_build_loops so a perpetually- + # failing task can never loop BUILD->DISPATCH->VERIFY forever (LOGIC-RACE-01). + build_loops: int = 0 transport: str = "" created_at: str | None = None updated_at: str | None = None @@ -150,6 +156,12 @@ class PipelineState(TypedDict, total=False): run_id: str | None ci_correlation_tag: str | None dispatched_at: str | None + # Build<->verify loops already consumed for THIS task (mirrors TaskRecord). + # The verifier threads it through DURABLE state — increments on a recoverable + # gate FAIL, parks at VerifierConfig.max_build_loops — so the build-loop + # budget is real (LOGIC-RACE-01: it was previously read from the shared + # wiring-time config and never advanced). + build_loops: int transport: str created_at: str | None updated_at: str | None @@ -185,6 +197,7 @@ def task_from_dict(data: dict[str, Any]) -> TaskRecord: run_id=data.get("run_id"), ci_correlation_tag=data.get("ci_correlation_tag"), dispatched_at=data.get("dispatched_at"), + build_loops=data.get("build_loops", 0), transport=data.get("transport", ""), created_at=data.get("created_at"), updated_at=data.get("updated_at"), diff --git a/agent-team/scripts/assert_no_write_token.py b/agent-team/scripts/assert_no_write_token.py index a561772..91ddea5 100644 --- a/agent-team/scripts/assert_no_write_token.py +++ b/agent-team/scripts/assert_no_write_token.py @@ -106,15 +106,41 @@ _DEFAULT_ENV_FILES: tuple[str, ...] = ("~/secrev.env", "~/orchestrator/.env") # --------------------------------------------------------------------------- # -def scan_environ(environ: Mapping[str, str]) -> list[str]: +def scan_environ(environ: Mapping[str, str], *, app_id: str | None = None) -> list[str]: """Scan a process environment for write-shaped GitHub token material. Flags (a) the explicit App-credential env vars, (b) any var whose *name* - matches the GitHub-write heuristic, and (c) any var whose *value* carries a - write-capable GitHub token prefix. Known read-only tokens are exempt. + matches the GitHub-write heuristic, (c) any var whose *value* carries a + write-capable GitHub token prefix, (d) any var whose *value* is a PEM + private-key block (the App key exported under a benign name), and (e) any var + whose *value* contains the configured App id. Known read-only tokens are + exempt from the *name* and write-token *value* heuristics, but the PEM and + App-id value checks below apply to EVERY var (including the allowlisted ones + and ``GITHUB_TOKEN``) — a private key or the App id can never legitimately + sit in any env var, so those checks are never name-exempted. This mirrors + :func:`scan_env_file`, which greps file contents for the same App-id / + private-key material regardless of the var name carrying it. """ findings: list[str] = [] for name, value in environ.items(): + # The PEM private-key VALUE check and the configured App-id VALUE check + # are NOT name-exempt: the App's private key exported under a benign name + # (e.g. GH_APP_KEY=-----BEGIN RSA PRIVATE KEY-----...) and the configured + # App id parked in any var are both forbidden material, even in an + # otherwise read-only/allowlisted var. Check them up front, before the + # allowlist short-circuits the name/token-prefix heuristics. + if value and _PRIVATE_KEY_RE.search(value): + findings.append( + f"environment variable {name!r} holds a PEM private-key block " + "(-----BEGIN ... PRIVATE KEY-----); no private key may live on " + "the box (the App key must live ONLY as an Actions secret)" + ) + if app_id and value and app_id in value: + findings.append( + f"environment variable {name!r} contains the configured App id " + "value; the App id must not be present on the box" + ) + if name in _ALLOWED_TOKEN_ENV_VARS: continue if name in _FORBIDDEN_ENV_VARS: @@ -238,10 +264,10 @@ def audit( ) -> list[str]: """Run every scanner and return the combined list of findings (empty == clean).""" environ = os.environ if environ is None else environ - findings: list[str] = [] - findings += scan_environ(environ) - findings += scan_config(config) app_id = environ.get(APP_ID_ENV) or None + findings: list[str] = [] + findings += scan_environ(environ, app_id=app_id) + findings += scan_config(config) for path in _resolve_env_files(env_files, environ): findings += scan_env_file(path, app_id=app_id) return findings diff --git a/agent-team/scripts/p3_rollback.sh b/agent-team/scripts/p3_rollback.sh index 54e5f7d..3aad155 100755 --- a/agent-team/scripts/p3_rollback.sh +++ b/agent-team/scripts/p3_rollback.sh @@ -333,14 +333,41 @@ require_keys() { warn() { printf ' \033[1;35m[WARN]\033[0m %s\n' "$*" >&2; } +# redact_secrets — read text on stdin and mask anything that looks like a secret +# (App JWTs, GitHub tokens, Authorization headers, PEM blocks) to a placeholder. +# SEC-01 (CWE-532): the DEFAULT --dry-run mode echoes the gh/git argv into the +# plan output, and restore_app passes `-H "Authorization: Bearer ${JWT}"`, so the +# real App JWT would otherwise reach stdout/CI logs. This is applied CENTRALLY in +# run_or_plan (and anywhere else that echoes argv) so no secret can be printed. +redact_secrets() { + # sed: each pattern collapses the secret to a stable placeholder. Order matters + # (Authorization/Bearer first so the bare-token rules don't double-process it). + # * Bearer -> Bearer + # * Authorization: -> Authorization: + # * ghp_/gho_/ghu_/ghs_/ghr_/github_pat_ tokens -> + # * PEM private-key bodies -> + sed -E \ + -e 's/(Bearer)[[:space:]]+[A-Za-z0-9._~+/=-]+/\1 /g' \ + -e 's/([Aa]uthorization:)[[:space:]]*[^"'"'"']+/\1 /g' \ + -e 's/(gh[pousr]_|github_pat_)[A-Za-z0-9_]+//g' \ + -e 's/-----BEGIN [A-Z ]*PRIVATE KEY-----[^-]*-----END [A-Z ]*PRIVATE KEY-----//g' +} + +# redacted — emit "$*" with any secret masked. Use whenever argv is echoed. +redacted() { + printf '%s' "$*" | redact_secrets +} + # run_or_plan "" gh ... — print the plan; only execute on --apply. +# The plan branch echoes the FULL argv, so it is passed through redact_secrets +# first (SEC-01) — a Bearer JWT or token in "$@" never reaches stdout. run_or_plan() { local desc="$1"; shift if [ "${APPLY}" = "1" ]; then - do_ "${desc}" + do_ "$(redacted "${desc}")" "$@" else - plan "${desc}: $*" + plan "$(redacted "${desc}: $*")" fi } @@ -393,6 +420,13 @@ restore_workflow() { if [ -n "${baseline_sha}" ]; then run_or_plan "revert ${wf_path} to baseline SHA ${baseline_sha}" \ git checkout "${baseline_sha}" -- "${wf_path}" + # P3-IAC-08 (CWE-665): `git checkout -- ` only STAGES the revert + # in the local worktree/index — unlike the post-merge path it does NOT commit + # or push, so the remote live YAML is unchanged until a human lands it. Warn + # loudly so the restore is never assumed complete on the remote. + warn "LIVE-YAML revert of ${wf_path} is STAGED-ONLY (local worktree/index)." + warn " It is NOT committed or pushed — the remote workflow is UNCHANGED until you" + warn " manually commit + push (e.g. git commit -m 'revert P3 flip workflow' && git push)." else plan "(no workflow_baseline_sha recorded — skipping live YAML revert)" fi @@ -552,6 +586,36 @@ require_oob_ack() { return 1 } +# uninstall_app_via_curl SLUG INSTALLATION_ID — issue the App-JWT-authenticated +# DELETE /app/installations/{id} WITHOUT the JWT ever appearing on a command line +# (SEC-02 / CWE-214). The Authorization header is written to a 0600 temp file and +# passed to `curl -H @file`; argv carries only the filename. The file is shredded +# (or rm'd) on return via a trap. Honours --apply (dry-run only prints the plan, +# with the JWT redacted by run_or_plan-style masking). +uninstall_app_via_curl() { + local slug="$1" inst="$2" + if [ "${APPLY}" != "1" ]; then + # Dry-run: never write the header file; describe the action with NO secret. + plan "$(redacted "UNINSTALL App '${slug}' installation ${inst} via curl -H @<0600 hdrfile> (Authorization: Bearer ${AGENT_APPLY_APP_JWT})")" + plan " (JWT is written to a 0600 temp header file, never to the process argv)" + return 0 + fi + do_ "UNINSTALL App '${slug}' installation ${inst} (App JWT via 0600 header file; not on argv)" + local hdr_file rc=0 + hdr_file="$(mktemp "${TMPDIR:-/tmp}/p3-app-hdr.XXXXXX")" + # Tighten perms BEFORE writing the secret. + chmod 600 "${hdr_file}" + printf 'Authorization: Bearer %s\n' "${AGENT_APPLY_APP_JWT}" > "${hdr_file}" + curl -fsS -X DELETE \ + -H @"${hdr_file}" \ + -H "Accept: application/vnd.github+json" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + "https://api.github.com/app/installations/${inst}" || rc=$? + # Always shred/remove the header file so the JWT does not linger on disk. + shred -u "${hdr_file}" 2>/dev/null || rm -f "${hdr_file}" + return "${rc}" +} + restore_app() { say "3. Neutralise the GitHub App installation" require_keys app.action || return 1 @@ -573,9 +637,13 @@ restore_app() { fi # DELETE /app/installations/{id} requires an APP JWT — NOT operator gh. if [ -n "${AGENT_APPLY_APP_JWT:-}" ]; then - run_or_plan "UNINSTALL App '${slug}' installation ${installation_id} (requires APP JWT)" \ - gh api -X DELETE "app/installations/${installation_id}" \ - -H "Authorization: Bearer ${AGENT_APPLY_APP_JWT}" + # SEC-02 (CWE-214): keep the JWT OUT of the process argv (visible in + # ps/proc to any local user). `gh api` has no header-from-file mechanism + # and -H puts the value on the command line, so we issue the DELETE with + # `curl -H @headerfile` instead — the Authorization header (with the JWT) + # lives only in a 0600 temp file that is shredded on return. The argv + # carries only the file reference, never the secret. + uninstall_app_via_curl "${slug}" "${installation_id}" else # No JWT available: never pretend operator gh can do this. Plan only. plan "UNINSTALL App '${slug}' installation ${installation_id} requires an APP JWT" diff --git a/agent-team/tests/test_build_verify_subgraph.py b/agent-team/tests/test_build_verify_subgraph.py index a2fa548..c354afb 100644 --- a/agent-team/tests/test_build_verify_subgraph.py +++ b/agent-team/tests/test_build_verify_subgraph.py @@ -237,6 +237,47 @@ def test_verify_failing_ci_result_loops_back_to_build() -> None: assert out["current_phase"] == Phase.BUILD.value assert out["ci_results"]["gate_decision"] == "fail" assert route_after_verify(out) == BUILD_ROUTE + # The recoverable loop persists the incremented DURABLE count into state. + assert out["build_loops"] == 1 + + +def test_repeated_fail_through_wrapper_parks_after_max_build_loops() -> None: + """A perpetually-FAILing task PARKS after exactly max_build_loops loops. + + Drives the durable BUILD->DISPATCH->VERIFY budget through the subgraph + wrapper with ONE shared VerifierConfig (the wiring-time reality). The count + that bounds the loop is the per-task ``state["build_loops"]`` the node writes + back each round (LOGIC-RACE-01) — the shared config's build_loops stays 0, so + if the node read the count from config the loop would never terminate. + """ + diff = _diff_for("src/foo.py") + + def fail_fetcher(state): + return {"run_id": "r1", "conclusion": "failure", "diff_hash": _hash(diff)} + + shared_cfg = VerifierConfig( + expected_run_id="r1", + allowed_scope=["src"], + max_build_loops=3, + build_loops=0, + ) + node = make_verify_node(shared_cfg, ci_result_fetcher=fail_fetcher) + + state = _verify_state(diff) + state["build_loops"] = 0 + + routes: list[str] = [] + for _ in range(10): # bound the harness; a regression must not hang + out = node(state) + routes.append(route_after_verify(out)) + if route_after_verify(out) == PARKED_ROUTE: + break + # The checkpointer would carry build_loops forward across the loop. + state["build_loops"] = out["build_loops"] + + assert routes == [BUILD_ROUTE, BUILD_ROUTE, PARKED_ROUTE] + assert out["status"] == TaskStatus.PARKED.value + assert out["current_phase"] == Phase.PARKED.value def test_verify_malformed_fetcher_result_fails_safe_to_parked() -> None: diff --git a/agent-team/tests/test_ci_fetcher.py b/agent-team/tests/test_ci_fetcher.py index 31edc68..328ce10 100644 --- a/agent-team/tests/test_ci_fetcher.py +++ b/agent-team/tests/test_ci_fetcher.py @@ -174,6 +174,29 @@ def test_missing_token_fails_closed(monkeypatch: pytest.MonkeyPatch) -> None: assert fetch_ci_result(_state(), owner="o", repo="r") is None +# --------------------------------------------------------------------------- # +# SEC-04: token resolution prefers the dedicated read-only var over GITHUB_TOKEN +# --------------------------------------------------------------------------- # + + +def test_resolve_prefers_dedicated_read_token(monkeypatch: pytest.MonkeyPatch) -> None: + from agent_team.ci_fetcher import _resolve_read_token + + monkeypatch.setenv(CI_READ_TOKEN_ENV, "dedicated-read-only") + monkeypatch.setenv("GITHUB_TOKEN", "fallback-token") + # The dedicated read-only var wins on the live box read path. + assert _resolve_read_token() == "dedicated-read-only" + + +def test_resolve_falls_back_to_github_token(monkeypatch: pytest.MonkeyPatch) -> None: + from agent_team.ci_fetcher import _resolve_read_token + + monkeypatch.delenv(CI_READ_TOKEN_ENV, raising=False) + monkeypatch.setenv("GITHUB_TOKEN", "fallback-token") + # Documented, explicitly-narrowed convenience fallback (must be read-only). + assert _resolve_read_token() == "fallback-token" + + # --------------------------------------------------------------------------- # # Read-only contract: never derives a verdict, never writes # --------------------------------------------------------------------------- # diff --git a/agent-team/tests/test_no_write_token.py b/agent-team/tests/test_no_write_token.py index 675e8ab..a4b0862 100644 --- a/agent-team/tests/test_no_write_token.py +++ b/agent-team/tests/test_no_write_token.py @@ -70,9 +70,12 @@ def test_forbidden_app_id_env_var_is_flagged(mod: ModuleType) -> None: def test_forbidden_app_private_key_env_var_is_flagged(mod: ModuleType) -> None: + # The var trips both the forbidden-name check AND the PEM-value check (SEC-03) + # — both are legitimate findings; assert it is flagged (not an exact count). findings = mod.scan_environ({"AGENT_APPLY_APP_PRIVATE_KEY": _RSA_KEY}) - assert len(findings) == 1 - assert "AGENT_APPLY_APP_PRIVATE_KEY" in findings[0] + assert findings + assert all("AGENT_APPLY_APP_PRIVATE_KEY" in fn for fn in findings) + assert any("PRIVATE KEY" in fn for fn in findings) def test_write_token_shaped_name_is_flagged(mod: ModuleType) -> None: @@ -128,6 +131,65 @@ def test_plain_github_token_fallback_is_not_flagged_by_name(mod: ModuleType) -> assert mod.scan_environ({"GITHUB_TOKEN": "a-non-write-shaped-value"}) == [] +def test_github_token_value_is_still_write_value_scanned(mod: ModuleType) -> None: + # SEC-04: GITHUB_TOKEN is name-exempt from the write *name* heuristic, but a + # write-capable token VALUE parked in it must still be flagged. + findings = mod.scan_environ({"GITHUB_TOKEN": "ghp_" + "a" * 36}) + assert len(findings) == 1 + assert "GITHUB_TOKEN" in findings[0] + + +# SEC-03: a PEM private key exported under a *benign* env name must be flagged +# (the App key value check is NOT name-exempt). +def test_pem_private_key_under_benign_env_name_is_flagged(mod: ModuleType) -> None: + findings = mod.scan_environ({"GH_APP_KEY": _RSA_KEY}) + assert any("PRIVATE KEY" in fn for fn in findings) + assert any("GH_APP_KEY" in fn for fn in findings) + + +@pytest.mark.parametrize("key", [_RSA_KEY, _EC_KEY, _OPENSSH_KEY, _PKCS8_KEY]) +def test_pem_private_key_value_is_flagged_for_each_algo( + mod: ModuleType, key: str +) -> None: + # Even a totally unremarkable var name carrying any PEM algorithm is flagged. + findings = mod.scan_environ({"HARMLESS": key}) + assert any("PRIVATE KEY" in fn for fn in findings) + + +def test_pem_private_key_in_allowlisted_env_var_is_still_flagged( + mod: ModuleType, +) -> None: + # The value-level PEM check is not exempted by the read-only allowlist. + findings = mod.scan_environ({"AGENT_TEAM_CI_READ_TOKEN": _PKCS8_KEY}) + assert any("PRIVATE KEY" in fn for fn in findings) + + +# SEC-03: the configured App-id appearing as an env VALUE (under any name) must +# be flagged — mirroring scan_env_file's App-id grep. +def test_configured_app_id_value_in_env_is_flagged(mod: ModuleType) -> None: + findings = mod.scan_environ({"SOME_VAR": "installed-987654-here"}, app_id="987654") + assert any("App id value" in fn for fn in findings) + assert any("SOME_VAR" in fn for fn in findings) + + +def test_app_id_value_check_is_skipped_without_app_id(mod: ModuleType) -> None: + # No configured app_id -> the value-substring check does not fire. + assert mod.scan_environ({"SOME_VAR": "987654"}) == [] + + +def test_audit_flags_app_id_value_in_environ(mod: ModuleType, tmp_path: Path) -> None: + # End-to-end: AGENT_APPLY_APP_ID in environ both flags itself and is matched + # against other env VALUES (a benign var carrying the same id is flagged). + clean = tmp_path / "secrev.env" + clean.write_text("ok\n") + findings = mod.audit( + environ={"AGENT_APPLY_APP_ID": "424242", "BENIGN": "id-is-424242"}, + env_files=[str(clean)], + ) + assert any("AGENT_APPLY_APP_ID" in fn for fn in findings) + assert any("BENIGN" in fn and "App id value" in fn for fn in findings) + + # --------------------------------------------------------------------------- # # scan_config # --------------------------------------------------------------------------- # diff --git a/agent-team/tests/test_rollback.py b/agent-team/tests/test_rollback.py index 2b7d72f..0ffe07e 100644 --- a/agent-team/tests/test_rollback.py +++ b/agent-team/tests/test_rollback.py @@ -294,13 +294,49 @@ def test_apply_app_uninstall_without_jwt_fails_closed( def test_apply_app_uninstall_with_jwt_invokes_delete( - baseline: Path, shim_bin: tuple[Path, Path] + baseline: Path, tmp_path: Path ) -> None: - """With an App JWT present, --apply uninstall issues the DELETE to the - (shimmed) gh with a Bearer Authorization header.""" + """With an App JWT present, --apply uninstall issues the DELETE via curl with + a 0600 header FILE (SEC-02 / CWE-214) — the JWT is NEVER on the process argv. + + The shimmed ``curl`` records its argv AND dumps the contents of the + ``-H @`` header file, so we can assert: + * the DELETE hits app/installations/424242, + * the JWT lives only inside the header file (not in argv), + * the header file referenced on argv carries the Bearer line. + """ + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + calls_log = tmp_path / "calls.log" + + gh = bin_dir / "gh" + gh.write_text( + f'#!/usr/bin/env bash\nprintf "gh %s\\n" "$*" >> "{calls_log}"\nexit 0\n', + encoding="utf-8", + ) + # curl shim: record argv, and resolve any `-H @file` to dump the file body so + # the test can confirm the secret was passed by FILE, not on the command line. + curl = bin_dir / "curl" + curl.write_text( + "#!/usr/bin/env bash\n" + f'printf "curl %s\\n" "$*" >> "{calls_log}"\n' + "prev=''\n" + 'for a in "$@"; do\n' + ' if [ "$prev" = "-H" ]; then\n' + ' case "$a" in\n' + f' @*) printf "HDRFILE %s\\n" "$(cat "${{a#@}}")" >> "{calls_log}" ;;\n' + " esac\n" + " fi\n" + ' prev="$a"\n' + "done\n" + "exit 0\n", + encoding="utf-8", + ) + for f in (gh, curl): + f.chmod(f.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) + env = dict(os.environ) env["AGENT_APPLY_APP_JWT"] = "jwt-token-abc" - bin_dir, calls_log = shim_bin env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}" res = subprocess.run( ["bash", str(_SCRIPT), "app", "--apply", "--baseline", str(baseline)], @@ -310,8 +346,66 @@ def test_apply_app_uninstall_with_jwt_invokes_delete( ) assert res.returncode == 0, res.stderr + res.stdout calls = calls_log.read_text(encoding="utf-8") - assert "DELETE app/installations/424242" in calls - assert "Authorization: Bearer jwt-token-abc" in calls + # The DELETE went out via curl to the App installations endpoint. + assert "app/installations/424242" in calls + assert "-X DELETE" in calls + # SEC-02: the JWT is NEVER on the process argv (curl line), only in the file. + assert "jwt-token-abc" not in "".join( + line for line in calls.splitlines() if line.startswith("curl ") + ) + # The header file carried the Bearer line. + assert "HDRFILE Authorization: Bearer jwt-token-abc" in calls + # And the JWT never reached stdout (it is masked everywhere it is echoed). + assert "jwt-token-abc" not in res.stdout + + +def test_dry_run_app_redacts_jwt_from_plan_output(baseline: Path) -> None: + """SEC-01 (CWE-532): in the DEFAULT --dry-run mode the App-uninstall plan + echoes the gh/curl argv (which includes the Authorization: Bearer + header). The real App JWT must NEVER appear on stdout — it is masked to a + placeholder. Regression for the secret-in-CI-log leak.""" + secret = "supersecretjwtvalue1234567890" + res = _run("app", baseline=baseline, extra_env={"AGENT_APPLY_APP_JWT": secret}) + assert res.returncode == 0, res.stderr + blob = res.stdout + res.stderr + # The secret value never leaks; the masked placeholder is what is printed. + assert secret not in blob + assert "" in blob + + +def test_dry_run_app_redacts_jwt_in_all_surface(baseline: Path) -> None: + """SEC-01: the redaction is central (run_or_plan), so it holds on the `all` + and `incident` paths too — any surface that echoes the App-uninstall argv.""" + secret = "anotherjwtsecretZZZ999" + res = _run( + "all", + "--flip-pr", + "7", + baseline=baseline, + extra_env={"AGENT_APPLY_APP_JWT": secret}, + ) + assert res.returncode == 0, res.stderr + assert secret not in (res.stdout + res.stderr) + + +def test_workflow_live_revert_warns_staged_only(baseline: Path) -> None: + """P3-IAC-08 (CWE-665): the LIVE-YAML revert uses `git checkout -- path`, + which only STAGES locally (no commit/push). The script must WARN that the + revert is staged-only and needs a manual commit+push, so the restore is never + assumed complete on the remote.""" + res = _run( + "workflow", + "--flip-pr", + "7", + "--flip-branch", + "agent-team/apply/t1", + baseline=baseline, + ) + assert res.returncode == 0, res.stderr + blob = res.stdout + res.stderr + assert "git checkout 0123abc --" in res.stdout + assert "STAGED-ONLY" in blob + assert "commit + push" in blob or "commit+push" in blob def test_dry_run_protection_restores_full_baseline(baseline: Path) -> None: diff --git a/agent-team/tests/test_task_model.py b/agent-team/tests/test_task_model.py index 2611a0a..dbcb626 100644 --- a/agent-team/tests/test_task_model.py +++ b/agent-team/tests/test_task_model.py @@ -76,6 +76,7 @@ def test_roundtrip_dict() -> None: run_id="27990718108", ci_correlation_tag="t1-1a2b3c", dispatched_at="2026-06-17T00:30:00Z", + build_loops=2, transport="slack", created_at="2026-06-17T00:00:00Z", updated_at="2026-06-17T01:00:00Z", @@ -86,6 +87,21 @@ def test_roundtrip_dict() -> None: assert restored.run_id == "27990718108" assert restored.ci_correlation_tag == "t1-1a2b3c" assert restored.dispatched_at == "2026-06-17T00:30:00Z" + # The durable build<->verify loop count round-trips (LOGIC-RACE-01). + assert restored.build_loops == 2 + + +def test_build_loops_defaults_zero_and_roundtrips() -> None: + rec = TaskRecord( + thread_id="t1", + status=TaskStatus.ACTIVE, + current_phase=Phase.BUILD, + ) + assert rec.build_loops == 0 + # A dict missing build_loops (older record) defaults to 0, not a crash. + legacy = task_to_dict(rec) + del legacy["build_loops"] + assert task_from_dict(legacy).build_loops == 0 def test_roundtrip_json() -> None: diff --git a/agent-team/tests/test_verifier.py b/agent-team/tests/test_verifier.py index 0fcf3ee..60a869f 100644 --- a/agent-team/tests/test_verifier.py +++ b/agent-team/tests/test_verifier.py @@ -123,6 +123,69 @@ def test_fail_increments_build_loops_in_verdict() -> None: assert out["review_verdicts"][0]["build_loops"] == 2 +# --------------------------------------------------------------------------- # +# Durable per-task build-loop budget (LOGIC-RACE-01) +# --------------------------------------------------------------------------- # + + +def test_fail_reads_loop_count_from_state_not_config() -> None: + # The count that bounds the loop is DURABLE per-task state. A shared config + # with build_loops=0 must NOT mask a per-task state["build_loops"] of 2. + diff = _diff_for("src/foo.py") + ci = {"run_id": "r1", "conclusion": "failure", "diff_hash": _hash(diff)} + state = _state(diff, ci) + state["build_loops"] = 2 + out = verifier_node(state, VerifierConfig(expected_run_id="r1", build_loops=0)) + # next = state(2) + 1 = 3 == DEFAULT_MAX_BUILD_LOOPS -> park (not a vacuous + # loop driven by the always-zero shared config). + assert out["status"] == TaskStatus.PARKED.value + assert out["review_verdicts"][0]["build_loops"] == 3 + + +def test_fail_writes_incremented_loop_count_into_state() -> None: + # On a recoverable FAIL the node persists the incremented count into the + # returned partial state so the NEXT VERIFY (after BUILD->DISPATCH) sees it. + diff = _diff_for("src/foo.py") + ci = {"run_id": "r1", "conclusion": "failure", "diff_hash": _hash(diff)} + state = _state(diff, ci) + state["build_loops"] = 0 + out = verifier_node(state, VerifierConfig(expected_run_id="r1")) + assert out["current_phase"] == Phase.BUILD.value + assert out["build_loops"] == 1 + + +def test_repeated_fail_parks_after_exactly_max_build_loops_via_state() -> None: + # Simulate the durable BUILD->DISPATCH->VERIFY loop: the incremented + # build_loops the node returns is fed back into the next call's state (the + # checkpointer carries it). A perpetually-FAILing task must reach PARKED + # after EXACTLY max_build_loops iterations, never loop unbounded. + diff = _diff_for("src/foo.py") + ci = {"run_id": "r1", "conclusion": "failure", "diff_hash": _hash(diff)} + # A single SHARED config (build_loops stays 0) — exactly the wiring-time + # reality that made the budget dead before the fix. + shared_cfg = VerifierConfig(expected_run_id="r1", max_build_loops=3, build_loops=0) + + state = _state(diff, ci) + state["build_loops"] = 0 + + statuses: list[str] = [] + for _ in range(10): # bound the harness so a regression can't hang the test + out = verifier_node(state, shared_cfg) + statuses.append(out["status"]) + if out["status"] == TaskStatus.PARKED.value: + break + # Carry the durable count forward, as the checkpointer would. + state["build_loops"] = out["build_loops"] + + # FAILs at counts 1, 2 loop back to BUILD; the 3rd (next==3==max) parks. + assert statuses == [ + TaskStatus.ACTIVE.value, + TaskStatus.ACTIVE.value, + TaskStatus.PARKED.value, + ] + assert out["current_phase"] == Phase.PARKED.value + + # --------------------------------------------------------------------------- # # BLOCK path — park for human + GPT cross-review # --------------------------------------------------------------------------- #