From c4bea7270ba558abe0cc9d745b37080f0a52db0b Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Tue, 23 Jun 2026 19:18:58 -0400 Subject: [PATCH] harden(agent-team): fold C1 cross-review MEDIUMs into p3_rollback.sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GPT-4.1 cross-family review (APPROVE, no critical/high) raised two MEDIUMs on the rollback tooling; addressed both: - require_keys: each restore_* asserts its required baseline keys up front and refuses a PARTIAL (silently-weaker) restore. environment accepts ids OR logins (equivalent); a missing protection.full now REFUSES the enforce_admins-only degrade unless P3_ROLLBACK_ALLOW_PARTIAL=1 is set (loud DEGRADED warning). - out-of-band ACK: an --apply that needs a MANUAL App neutralise (no APP JWT, or action=out-of-band) refuses unless P3_ROLLBACK_OOB_ACK=1 — so the App is never left un-neutralised without a conscious operator sign-off; with the ack the other surfaces still restore. Documented both env vars in usage. +3 tests (required-key refuse, partial-protection ack, oob ack). Suite: 1362 passed, ruff clean. --- agent-team/scripts/p3_rollback.sh | 86 +++++++++++++++++++++++++++++-- agent-team/tests/test_rollback.py | 54 +++++++++++++++++-- 2 files changed, 131 insertions(+), 9 deletions(-) diff --git a/agent-team/scripts/p3_rollback.sh b/agent-team/scripts/p3_rollback.sh index c75efc1..54e5f7d 100755 --- a/agent-team/scripts/p3_rollback.sh +++ b/agent-team/scripts/p3_rollback.sh @@ -192,6 +192,16 @@ Options: --merged flip already merged (post-merge revert path) --flip-commit SHA merged flip commit to revert (post-merge path) -h, --help show this help + +Operator ACK env vars (fail-closed by default; a partial/manual step must be +acknowledged so a restore is never silently weaker than the baseline): + AGENT_APPLY_APP_JWT= App JWT for the automated App uninstall. + P3_ROLLBACK_OOB_ACK=1 acknowledge you will neutralise the App by hand + when no APP JWT is available (lets the other + surfaces restore instead of aborting). + P3_ROLLBACK_ALLOW_PARTIAL=1 accept a DEGRADED branch-protection restore + (enforce_admins only) when the baseline has no + protection.full — otherwise this is refused. USAGE } @@ -302,6 +312,27 @@ require_baseline() { fi } +# require_keys KEY... — fail HARD if any baseline key is absent (MEDIUM-1). A +# partial baseline must never yield a partial/weaker restore (e.g. restoring only +# enforce_admins while silently dropping status checks): each restore_* asserts +# the keys IT needs up front, BEFORE any mutation, so a missing key aborts the +# whole surface instead of half-restoring it. +require_keys() { + local missing="" k + for k in "$@"; do + if [ -z "$(jget "${k}")" ]; then + missing="${missing} ${k}" + fi + done + if [ -n "${missing}" ]; then + err "baseline ${BASELINE} is missing required key(s):${missing}" + err " refusing a PARTIAL restore — record a complete baseline first (--record-baseline)." + return 1 + fi +} + +warn() { printf ' \033[1;35m[WARN]\033[0m %s\n' "$*" >&2; } + # run_or_plan "" gh ... — print the plan; only execute on --apply. run_or_plan() { local desc="$1"; shift @@ -326,6 +357,7 @@ assert_equal() { # ── surface 1: workflow flip ───────────────────────────────────────────────── restore_workflow() { say "1. Restore the apply/verify workflow flip" + require_keys workflow_path workflow_baseline_sha || return 1 local wf_path baseline_sha wf_path="$(jget workflow_path)" [ -n "${wf_path}" ] || wf_path=".github/workflows/agent-team-apply-verify.yml" @@ -404,6 +436,16 @@ reviewers_put_args() { # ── surface 2: agent-apply environment ─────────────────────────────────────── restore_environment() { say "2. Restore the agent-apply environment (required reviewer ids + policy)" + require_keys environment.name || return 1 + # The reviewer can be recorded as numeric ids OR logins (the login->id resolve + # is an EQUIVALENT, not weaker, restore) — but at least one source is required, + # else there is nothing to restore the required reviewer FROM (MEDIUM-1). + if [ -z "$(jget environment.required_reviewer_ids)" ] \ + && [ -z "$(jget environment.required_reviewers)" ]; then + err "baseline has neither environment.required_reviewer_ids nor environment.required_reviewers" + err " — nothing to restore the required reviewer from; record a complete baseline." + return 1 + fi local env_name policy raw line id ids env_name="$(jget environment.name)" [ -n "${env_name}" ] || env_name="agent-apply" @@ -490,8 +532,29 @@ EOF # NOT an installation token, NOT a fine-grained PAT). Provide the JWT via # $AGENT_APPLY_APP_JWT; this surface only --apply's the uninstall when it is # set. Without it, the step degrades to an out-of-band instruction. +# Out-of-band ACK gate (MEDIUM-2): when neutralising the App needs a MANUAL +# operator action (no APP JWT for the uninstall, or action=out-of-band), --apply +# must not silently skip it. Require an explicit acknowledgement +# (P3_ROLLBACK_OOB_ACK=1) that the operator WILL perform the manual step, so the +# App is never left un-neutralised without a conscious sign-off. WITH the ack the +# surface returns 0 so the other surfaces still restore (the App step is operator- +# owed, loudly warned); WITHOUT it, --apply aborts this surface. +require_oob_ack() { + local what="$1" + if [ "${P3_ROLLBACK_OOB_ACK:-}" = "1" ]; then + warn "OUT-OF-BAND ACK accepted (P3_ROLLBACK_OOB_ACK=1): ${what}" + warn " the App is NOT neutralised by this script — you MUST do it by hand NOW." + return 0 + fi + err "${what}" + err " set P3_ROLLBACK_OOB_ACK=1 to acknowledge you will perform the manual step" + err " (lets the other surfaces restore), or set \$AGENT_APPLY_APP_JWT for the automated uninstall." + return 1 +} + restore_app() { say "3. Neutralise the GitHub App installation" + require_keys app.action || return 1 local action installation_id slug action="$(jget app.action)" [ -n "${action}" ] || action="uninstall" @@ -520,8 +583,8 @@ restore_app() { plan " remove the installation in the org UI, or run:" plan " gh api -X DELETE app/installations/${installation_id} -H 'Authorization: Bearer '" if [ "${APPLY}" = "1" ]; then - err "uninstall needs an APP JWT (\$AGENT_APPLY_APP_JWT) — operator gh cannot do this; do it out-of-band" - return 1 + require_oob_ack \ + "uninstall needs an APP JWT (\$AGENT_APPLY_APP_JWT); operator gh cannot do this" || return 1 fi fi ;; @@ -532,8 +595,8 @@ restore_app() { plan " - rotate the App private key out-of-band (App settings > Generate a new private key," plan " update the AGENT_APPLY_APP_PRIVATE_KEY Actions secret), which invalidates new-token minting." if [ "${APPLY}" = "1" ]; then - err "app.action 'out-of-band' has no API path — perform the steps above by hand, then re-run --apply with a recorded uninstall" - return 1 + require_oob_ack \ + "app.action 'out-of-band' has no API path — neutralise the App by hand" || return 1 fi ;; *) @@ -544,6 +607,21 @@ restore_app() { # ── surface 4: branch protection ───────────────────────────────────────────── restore_protection() { say "4. Restore the FULL branch protection baseline (include_administrators ON)" + require_keys protection.branch protection.include_administrators || return 1 + # protection.full is the COMPLETE payload (status checks, PR reviews, ...). + # Restoring only enforce_admins when it is absent leaves a WEAKER posture than + # baseline (MEDIUM-1). Refuse that silent degrade by default; the operator must + # explicitly accept the enforce_admins-only restore via P3_ROLLBACK_ALLOW_PARTIAL=1. + if [ -z "$(jget protection.full)" ]; then + if [ "${P3_ROLLBACK_ALLOW_PARTIAL:-}" != "1" ]; then + err "baseline has no protection.full — an enforce_admins-only restore would leave a WEAKER" + err " posture (status checks / PR reviews NOT restored). Set P3_ROLLBACK_ALLOW_PARTIAL=1 to" + err " accept the degraded restore, or record a complete baseline (--record-baseline)." + return 1 + fi + warn "DEGRADED protection restore (P3_ROLLBACK_ALLOW_PARTIAL=1): enforce_admins ONLY;" + warn " status checks / PR reviews are NOT restored — restore them by hand or re-record." + fi local branch include_admins full branch="$(jget protection.branch)" [ -n "${branch}" ] || branch="$(jget default_branch)" diff --git a/agent-team/tests/test_rollback.py b/agent-team/tests/test_rollback.py index 659ee0b..2b7d72f 100644 --- a/agent-team/tests/test_rollback.py +++ b/agent-team/tests/test_rollback.py @@ -165,11 +165,14 @@ def _run( *args: str, baseline: Path, shim: tuple[Path, Path] | None = None, + extra_env: dict[str, str] | None = None, ) -> subprocess.CompletedProcess[str]: env = dict(os.environ) if shim is not None: bin_dir, _ = shim env["PATH"] = f"{bin_dir}{os.pathsep}{env['PATH']}" + if extra_env: + env.update(extra_env) return subprocess.run( ["bash", str(_SCRIPT), *args, "--baseline", str(baseline)], capture_output=True, @@ -321,17 +324,27 @@ def test_dry_run_protection_restores_full_baseline(baseline: Path) -> None: assert "assert LIVE protection == baseline protection.full" in out -def test_dry_run_protection_without_full_falls_back_to_enforce_admins( +def test_protection_without_full_refused_unless_partial_acked( tmp_path: Path, ) -> None: + # MEDIUM-1: a baseline missing protection.full must NOT silently restore only + # enforce_admins (a weaker posture). Without the explicit ack it fails hard. data = json.loads(json.dumps(_BASELINE)) del data["protection"]["full"] p = tmp_path / "nofull.json" p.write_text(json.dumps(data), encoding="utf-8") - res = _run("protection", baseline=p) - out = res.stdout - assert "no protection.full recorded" in out - assert "branches/main/protection/enforce_admins" in out + + refused = _run("protection", baseline=p) + assert refused.returncode != 0 + assert "no protection.full" in (refused.stdout + refused.stderr) + assert "P3_ROLLBACK_ALLOW_PARTIAL" in (refused.stdout + refused.stderr) + + # With the explicit ack the degraded enforce_admins-only restore proceeds, but + # LOUDLY warns it is partial. + acked = _run("protection", baseline=p, extra_env={"P3_ROLLBACK_ALLOW_PARTIAL": "1"}) + out = acked.stdout + acked.stderr + assert "DEGRADED protection restore" in out + assert "branches/main/protection/enforce_admins" in acked.stdout def test_dry_run_incident_path_full_sequence(baseline: Path) -> None: @@ -800,3 +813,34 @@ def test_apply_protection_DETECTS_divergent_live_restore( + res.stderr ) assert "protection.full" in (res.stdout + res.stderr) + + +def test_missing_required_key_refuses_partial_restore(tmp_path: Path) -> None: + # MEDIUM-1: a baseline missing a required key for a surface aborts that + # surface up front rather than half-restoring it. + data = json.loads(json.dumps(_BASELINE)) + del data["workflow_baseline_sha"] + p = tmp_path / "no_sha.json" + p.write_text(json.dumps(data), encoding="utf-8") + res = _run("workflow", baseline=p) + assert res.returncode != 0 + out = res.stdout + res.stderr + assert "missing required key" in out + assert "workflow_baseline_sha" in out + + +def test_app_uninstall_without_jwt_requires_oob_ack(tmp_path: Path) -> None: + # MEDIUM-2: an --apply that needs a MANUAL App neutralise (no APP JWT) must + # not silently skip it — it refuses unless the operator acknowledges. + p = tmp_path / "bl.json" + p.write_text(json.dumps(_BASELINE), encoding="utf-8") + + # No JWT, no ack -> refuse. + refused = _run("app", "--apply", baseline=p) + assert refused.returncode != 0 + assert "P3_ROLLBACK_OOB_ACK" in (refused.stdout + refused.stderr) + + # No JWT, but acked -> proceeds (App neutralise is operator-owed, loudly warned). + acked = _run("app", "--apply", baseline=p, extra_env={"P3_ROLLBACK_OOB_ACK": "1"}) + assert acked.returncode == 0, acked.stdout + acked.stderr + assert "OUT-OF-BAND ACK accepted" in (acked.stdout + acked.stderr)