fix(agent-team): remediate C1 security-review BLOCK (2 HIGH + MED/LOW)
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.
This commit is contained in:
parent
c4bea7270b
commit
00c51192c8
11 changed files with 486 additions and 34 deletions
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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"),
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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 <token> -> Bearer <REDACTED>
|
||||
# * Authorization: <anything> -> Authorization: <REDACTED>
|
||||
# * ghp_/gho_/ghu_/ghs_/ghr_/github_pat_ tokens -> <REDACTED-TOKEN>
|
||||
# * PEM private-key bodies -> <REDACTED-PEM>
|
||||
sed -E \
|
||||
-e 's/(Bearer)[[:space:]]+[A-Za-z0-9._~+/=-]+/\1 <REDACTED>/g' \
|
||||
-e 's/([Aa]uthorization:)[[:space:]]*[^"'"'"']+/\1 <REDACTED>/g' \
|
||||
-e 's/(gh[pousr]_|github_pat_)[A-Za-z0-9_]+/<REDACTED-TOKEN>/g' \
|
||||
-e 's/-----BEGIN [A-Z ]*PRIVATE KEY-----[^-]*-----END [A-Z ]*PRIVATE KEY-----/<REDACTED-PEM>/g'
|
||||
}
|
||||
|
||||
# redacted — emit "$*" with any secret masked. Use whenever argv is echoed.
|
||||
redacted() {
|
||||
printf '%s' "$*" | redact_secrets
|
||||
}
|
||||
|
||||
# run_or_plan "<human description>" 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 <sha> -- <path>` 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"
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
|
|
|||
|
|
@ -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 @<file>`` 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 <JWT>
|
||||
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 "<REDACTED>" 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 <sha> -- 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:
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
# --------------------------------------------------------------------------- #
|
||||
|
|
|
|||
Reference in a new issue