High-recall /sh-security-review fan-out + proof-or-kill verifier found two
confirmed HIGH; both now closed (verified empirically against the working tree):
- LOGIC-RACE-01 (HIGH, CWE-835): the build-loop budget was structurally dead
(verifier read a shared wiring-time VerifierConfig.build_loops, always 0, so
the max_build_loops park never fired -> a perpetually-failing task looped
BUILD->DISPATCH->VERIFY forever, force-pushing + firing a CI run each round).
Threaded build_loops through durable PipelineState/TaskRecord; verifier reads
state.get('build_loops',0), writes the incremented count back on each FAIL, and
PARKS at max_build_loops. Parks after exactly N failures, never unbounded.
- SEC-01 (HIGH, CWE-532) + SEC-02 (MED, CWE-214): p3_rollback.sh echoed the live
App JWT to stdout in default dry-run and passed it as a gh argv literal. Added
redact_secrets (Bearer/Authorization/ghX_/PEM masking) through run_or_plan; the
App uninstall now uses curl -H @<0600 tempfile> (JWT never on argv), shredded
after. Empirical: app/incident/all dry-runs leak 0 JWT occurrences.
- SEC-03 (MED, CWE-798): assert_no_write_token now applies the PEM regex + the
configured App-ID to env/config VALUES (not just files) — an App private key
under a benign env name is caught.
- SEC-04 (LOW) + P3-IAC-08 (LOW): tightened the box GITHUB_TOKEN fallback /
value-scan; staged-only WARN on the live workflow revert.
Suite: 1382 passed, ruff clean. Branch only; not merged/deployed.
NOTE: re-verifier flagged SEC-01 as open by grepping COMMITTED blobs (the fix was
uncommitted working-tree state); independently confirmed closed empirically.
380 lines
14 KiB
Python
380 lines
14 KiB
Python
"""Unit tests for agent_team.ci_fetcher — the read-only, fail-closed CI fetcher.
|
|
|
|
The fetcher is a pure DATA seam: on a clean authenticated run it returns the
|
|
``{run_id, conclusion, diff_hash}`` mapping the pure-code gate consumes; on ANY
|
|
error (404 / auth fail / malformed body / timeout / missing run_id / missing
|
|
token) it returns ``None`` so the gate BLOCKs (fail-closed). It NEVER derives a
|
|
verdict and NEVER writes. Everything is mocked with a fake HTTP client; no
|
|
network, no real token.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import pytest
|
|
|
|
from agent_team.ci_fetcher import (
|
|
CI_READ_TOKEN_ENV,
|
|
build_ci_result_fetcher,
|
|
fetch_ci_result,
|
|
)
|
|
|
|
|
|
class _FakeResponse:
|
|
def __init__(self, status: int, body):
|
|
self.status_code = status
|
|
self._body = body
|
|
|
|
def json(self):
|
|
if isinstance(self._body, Exception):
|
|
raise self._body
|
|
return self._body
|
|
|
|
|
|
class _FakeClient:
|
|
"""A requests-like client recording calls; raises only write verbs if asked."""
|
|
|
|
def __init__(self, response=None, raise_exc=None):
|
|
self._response = response
|
|
self._raise = raise_exc
|
|
self.calls: list[tuple[str, dict]] = []
|
|
|
|
def get(self, url, *, timeout=None):
|
|
self.calls.append((url, {"timeout": timeout}))
|
|
if self._raise is not None:
|
|
raise self._raise
|
|
return self._response
|
|
|
|
# If the fetcher ever tried to write, these would record it — they must not
|
|
# be called (the fetcher is read-only).
|
|
def post(self, *a, **k): # pragma: no cover - must never be called
|
|
raise AssertionError("ci_fetcher must never POST")
|
|
|
|
def patch(self, *a, **k): # pragma: no cover - must never be called
|
|
raise AssertionError("ci_fetcher must never PATCH")
|
|
|
|
|
|
def _state(run_id="123", diff_hash="abc123"):
|
|
return {"run_id": run_id, "diff_hash": diff_hash}
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Success path
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_success_returns_correct_mapping() -> None:
|
|
client = _FakeClient(
|
|
_FakeResponse(200, {"id": 123, "conclusion": "success", "status": "completed"})
|
|
)
|
|
out = fetch_ci_result(_state(), owner="o", repo="r", client=client)
|
|
assert out == {"run_id": "123", "conclusion": "success", "diff_hash": "abc123"}
|
|
# It hit the runs endpoint for the right run, read-only.
|
|
assert client.calls[0][0].endswith("/repos/o/r/actions/runs/123")
|
|
|
|
|
|
def test_failure_conclusion_is_echoed_not_judged() -> None:
|
|
# The fetcher returns the raw conclusion; it does NOT decide pass/fail.
|
|
client = _FakeClient(_FakeResponse(200, {"id": 5, "conclusion": "failure"}))
|
|
out = fetch_ci_result(_state(run_id="5"), owner="o", repo="r", client=client)
|
|
assert out is not None
|
|
assert out["conclusion"] == "failure" # echoed verbatim, no verdict derived
|
|
|
|
|
|
def test_diff_hash_echoed_from_state() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 9, "conclusion": "success"}))
|
|
out = fetch_ci_result(
|
|
{"run_id": "9", "diff_hash": "DEADBEEF"}, owner="o", repo="r", client=client
|
|
)
|
|
assert out["diff_hash"] == "DEADBEEF"
|
|
|
|
|
|
def test_run_id_read_from_state_drives_url() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 777, "conclusion": "success"}))
|
|
fetch_ci_result(_state(run_id="777"), owner="acme", repo="svc", client=client)
|
|
assert client.calls[0][0].endswith("/repos/acme/svc/actions/runs/777")
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Fail-closed paths (every error -> None)
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_404_fails_closed() -> None:
|
|
client = _FakeClient(_FakeResponse(404, {"message": "Not Found"}))
|
|
assert fetch_ci_result(_state(), owner="o", repo="r", client=client) is None
|
|
|
|
|
|
def test_auth_fail_401_fails_closed() -> None:
|
|
client = _FakeClient(_FakeResponse(401, {"message": "Bad credentials"}))
|
|
assert fetch_ci_result(_state(), owner="o", repo="r", client=client) is None
|
|
|
|
|
|
def test_forbidden_403_fails_closed() -> None:
|
|
client = _FakeClient(_FakeResponse(403, {"message": "forbidden"}))
|
|
assert fetch_ci_result(_state(), owner="o", repo="r", client=client) is None
|
|
|
|
|
|
def test_malformed_body_fails_closed() -> None:
|
|
client = _FakeClient(_FakeResponse(200, ValueError("not json")))
|
|
assert fetch_ci_result(_state(), owner="o", repo="r", client=client) is None
|
|
|
|
|
|
def test_non_object_body_fails_closed() -> None:
|
|
client = _FakeClient(_FakeResponse(200, ["not", "a", "dict"]))
|
|
assert fetch_ci_result(_state(), owner="o", repo="r", client=client) is None
|
|
|
|
|
|
def test_missing_conclusion_fails_closed() -> None:
|
|
# In-progress run: conclusion is None -> not an authenticated verdict.
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": None}))
|
|
assert (
|
|
fetch_ci_result(_state(run_id="1"), owner="o", repo="r", client=client) is None
|
|
)
|
|
|
|
|
|
def test_timeout_fails_closed() -> None:
|
|
client = _FakeClient(raise_exc=TimeoutError("timed out"))
|
|
assert fetch_ci_result(_state(), owner="o", repo="r", client=client) is None
|
|
|
|
|
|
def test_connection_error_fails_closed() -> None:
|
|
client = _FakeClient(raise_exc=ConnectionError("refused"))
|
|
assert fetch_ci_result(_state(), owner="o", repo="r", client=client) is None
|
|
|
|
|
|
def test_missing_run_id_fails_closed() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": "success"}))
|
|
assert (
|
|
fetch_ci_result({"diff_hash": "x"}, owner="o", repo="r", client=client) is None
|
|
)
|
|
# And it never even made a request.
|
|
assert client.calls == []
|
|
|
|
|
|
def test_empty_run_id_fails_closed() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": "success"}))
|
|
assert (
|
|
fetch_ci_result(
|
|
{"run_id": "", "diff_hash": "x"}, owner="o", repo="r", client=client
|
|
)
|
|
is None
|
|
)
|
|
assert client.calls == []
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# Missing token (no injected client) -> fails closed (does NOT raise)
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_missing_token_fails_closed(monkeypatch: pytest.MonkeyPatch) -> None:
|
|
monkeypatch.delenv(CI_READ_TOKEN_ENV, raising=False)
|
|
monkeypatch.delenv("GITHUB_TOKEN", raising=False)
|
|
# No client injected -> it must resolve a token; none set -> None (no raise).
|
|
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
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_fetcher_never_returns_a_verdict_field() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 2, "conclusion": "failure"}))
|
|
out = fetch_ci_result(_state(run_id="2"), owner="o", repo="r", client=client)
|
|
assert out is not None
|
|
# The mapping is DATA only — there is no gate_decision / passed / verdict.
|
|
assert set(out.keys()) == {"run_id", "conclusion", "diff_hash"}
|
|
assert "passed" not in out
|
|
assert "gate_decision" not in out
|
|
|
|
|
|
def test_build_ci_result_fetcher_returns_state_callable() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 3, "conclusion": "success"}))
|
|
fetcher = build_ci_result_fetcher(owner="o", repo="r", client=client)
|
|
out = fetcher({"run_id": "3", "diff_hash": "h"})
|
|
assert out == {"run_id": "3", "conclusion": "success", "diff_hash": "h"}
|
|
|
|
|
|
def test_build_ci_result_fetcher_fails_closed_on_error() -> None:
|
|
client = _FakeClient(_FakeResponse(500, {"message": "boom"}))
|
|
fetcher = build_ci_result_fetcher(owner="o", repo="r", client=client)
|
|
assert fetcher({"run_id": "3", "diff_hash": "h"}) is None
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# BLOCK-2: run_id validation (fail-closed; never reaches the URL)
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"bad_run_id",
|
|
[
|
|
"abc", # non-numeric
|
|
"12a", # mixed
|
|
"../runs/999", # path traversal
|
|
"1 2", # whitespace
|
|
"1" * 21, # too long (> 20 digits)
|
|
"-1", # sign
|
|
"0x10", # hex
|
|
],
|
|
)
|
|
def test_invalid_run_id_fails_closed(bad_run_id: str) -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": "success"}))
|
|
assert (
|
|
fetch_ci_result(
|
|
{"run_id": bad_run_id, "diff_hash": "x"},
|
|
owner="o",
|
|
repo="r",
|
|
client=client,
|
|
)
|
|
is None
|
|
)
|
|
# It never made a request: rejected before URL construction.
|
|
assert client.calls == []
|
|
|
|
|
|
def test_valid_numeric_run_id_is_accepted() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 42, "conclusion": "success"}))
|
|
out = fetch_ci_result(_state(run_id="42"), owner="o", repo="r", client=client)
|
|
assert out is not None and out["run_id"] == "42"
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# BLOCK-3: owner/repo validation (fail-closed; never reaches the URL)
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"owner,repo",
|
|
[
|
|
("o/../x", "r"), # traversal in owner
|
|
("o", "r/runs/9"), # extra segments in repo
|
|
("o ", "r"), # whitespace
|
|
("o", ""), # empty repo
|
|
("", "r"), # empty owner
|
|
("o", "r#frag"), # url metachar
|
|
("o?", "r"), # query metachar
|
|
],
|
|
)
|
|
def test_invalid_owner_repo_fails_closed(owner: str, repo: str) -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": "success"}))
|
|
assert fetch_ci_result(_state(), owner=owner, repo=repo, client=client) is None
|
|
assert client.calls == []
|
|
|
|
|
|
def test_valid_owner_repo_with_dots_dashes_underscores() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": "success"}))
|
|
out = fetch_ci_result(
|
|
_state(run_id="1"), owner="my-org.x", repo="repo_name.v2", client=client
|
|
)
|
|
assert out is not None
|
|
assert client.calls[0][0].endswith("/repos/my-org.x/repo_name.v2/actions/runs/1")
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# FIX-1: conclusion allowlist (unrecognised -> fail-closed)
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"conclusion",
|
|
[
|
|
"success",
|
|
"failure",
|
|
"cancelled",
|
|
"skipped",
|
|
"timed_out",
|
|
"action_required",
|
|
"neutral",
|
|
"stale",
|
|
"startup_failure",
|
|
],
|
|
)
|
|
def test_allowlisted_conclusions_pass(conclusion: str) -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": conclusion}))
|
|
out = fetch_ci_result(_state(run_id="1"), owner="o", repo="r", client=client)
|
|
assert out is not None and out["conclusion"] == conclusion
|
|
|
|
|
|
def test_conclusion_is_normalized_lowercase() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": "SUCCESS"}))
|
|
out = fetch_ci_result(_state(run_id="1"), owner="o", repo="r", client=client)
|
|
assert out is not None and out["conclusion"] == "success"
|
|
|
|
|
|
@pytest.mark.parametrize("bad", ["bogus", "passed", "won", "", "in_progress"])
|
|
def test_unrecognised_conclusion_fails_closed(bad: str) -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 1, "conclusion": bad}))
|
|
assert (
|
|
fetch_ci_result(_state(run_id="1"), owner="o", repo="r", client=client) is None
|
|
)
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# FIX-5: fetched_id validation (no arbitrary-string laundering)
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_integer_fetched_id_is_used() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": 999, "conclusion": "success"}))
|
|
out = fetch_ci_result(_state(run_id="1"), owner="o", repo="r", client=client)
|
|
assert out is not None and out["run_id"] == "999"
|
|
|
|
|
|
def test_numeric_string_fetched_id_is_used() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": "888", "conclusion": "success"}))
|
|
out = fetch_ci_result(_state(run_id="1"), owner="o", repo="r", client=client)
|
|
assert out is not None and out["run_id"] == "888"
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"bad_id",
|
|
["../evil", "abc", "9a", True, {"x": 1}, ["1"], 1.5],
|
|
)
|
|
def test_non_integer_fetched_id_falls_back_to_validated_run_id(bad_id) -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"id": bad_id, "conclusion": "success"}))
|
|
out = fetch_ci_result(_state(run_id="7"), owner="o", repo="r", client=client)
|
|
# Falls back to the already-validated requested run_id, never laundering the id.
|
|
assert out is not None and out["run_id"] == "7"
|
|
|
|
|
|
def test_missing_fetched_id_falls_back_to_run_id() -> None:
|
|
client = _FakeClient(_FakeResponse(200, {"conclusion": "success"}))
|
|
out = fetch_ci_result(_state(run_id="55"), owner="o", repo="r", client=client)
|
|
assert out is not None and out["run_id"] == "55"
|
|
|
|
|
|
# --------------------------------------------------------------------------- #
|
|
# FIX-3: production builder does not accept a caller-overridable api_root
|
|
# --------------------------------------------------------------------------- #
|
|
|
|
|
|
def test_build_ci_result_fetcher_rejects_api_root_kwarg() -> None:
|
|
# The production builder must NOT expose api_root (so the host cannot be
|
|
# redirected from the coordinator/production wiring).
|
|
with pytest.raises(TypeError):
|
|
build_ci_result_fetcher(owner="o", repo="r", api_root="https://evil.example")
|