From ebedabbe11a4a010aad0f103fefe242dc3fe3403 Mon Sep 17 00:00:00 2001 From: Babissimo Date: Wed, 23 Sep 2026 17:15:36 +0100 Subject: [PATCH] Run production's E2E rollback in its own job, away from npm e2e-prod runs npm ci, and the same job handed SERVER_SSH_KEY (root on production) to its rollback step. Any install script in the npm tree ran first in the environment the key was later given to. The rollback is now rollback-production-on-e2e-failure: a job of its own, needing e2e-prod, and keyed on the test step's outcome exported as a job output rather than on the job's failure(), so a failed checkout, npm ci or Chrome report still leaves production alone. It is gated on !cancelled() rather than always(), so cancelling the run still stops it, as it stopped the in-job step. It takes production-deploy like the deploy-failure rollback it is modelled on, so a following run's deploy cannot be in the deploy directory while rollback.sh is. Its ssh-action is pinned to the same commit as every other use of that action, and it keeps the workflow's read-only token. test_deploy_workflows.py now holds every workflow to the rule that no job that installs packages, runs in a container or starts service containers can read a droplet key, and pins the new job's condition across refs, events and test outcomes. Checked against main's ci.yml, where the rule fails on e2e-prod. ClickUp 123zgec4mze. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 60 ++++++--- backend/tests/test_deploy_workflows.py | 176 +++++++++++++++++++++++-- 2 files changed, 204 insertions(+), 32 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ac86d8858..d3ce4751b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -391,11 +391,17 @@ jobs: name: Playwright E2E (production) runs-on: ubuntu-latest # A backstop above the test step's own cap, not the primary bound: a job - # timeout is a cancellation and would skip the rollback below, whereas the - # step cap marks the step failed and keeps it reachable. + # timeout cancels the test step, and a cancelled step is not a verdict the + # rollback job acts on, whereas the step cap marks it failed. timeout-minutes: 30 needs: [production-smoke-tests] if: github.ref == 'refs/heads/main' && github.event_name == 'push' + # The test step's own outcome, for rollback-production-on-e2e-failure. Not + # the job's result: a failed checkout, npm ci or Chrome report fails the job + # too, and none of them says anything about whether production is healthy. + # Only tests that ran and failed roll production back. + outputs: + tests: ${{ steps.e2e.outcome }} # The browser is the Google Chrome the runner image ships # (e2e/playwright.config.ts) rather than one installed here: `npx # playwright install --with-deps` puts an apt update and Playwright's CDN in @@ -418,9 +424,9 @@ jobs: - name: Report the runner's Chrome run: node e2e/report-chrome.js # Bounded at the step rather than relying on the job cap: a step timeout - # marks the step failed, which still trips the rollback below, whereas a - # job timeout is a cancellation and would skip it. A production wedged - # badly enough to hang Playwright is exactly when the rollback is wanted. + # marks the step failed, which still trips the rollback job, whereas a + # job timeout cancels it and would not. A production wedged badly enough + # to hang Playwright is exactly when the rollback is wanted. - name: Run E2E tests (production) id: e2e # See the staging job: the step cap has to clear nodes.spec.ts's retried @@ -430,13 +436,29 @@ jobs: env: CI: "true" RADAR_API_KEY: ${{ secrets.RADAR_API_KEY }} - # Scoped to the test step's own outcome, as the production smoke job is. - # A bare failure() also fires when checkout, npm ci or the Chrome report - # fails, and anything that breaks before the tests run is a statement - # about the runner, not about production. Those leave the job red for a - # human to read; only tests that ran and failed roll production back. - - name: Rollback production on E2E failure - if: failure() && steps.e2e.outcome == 'failure' + + # A job of its own, so that production's root key is never handed to a job + # that has just run npm's install scripts. test_deploy_workflows.py holds + # every workflow to that. + rollback-production-on-e2e-failure: + name: Roll back production if its E2E tests failed + runs-on: ubuntu-latest + timeout-minutes: 20 + needs: [e2e-prod] + # See rollback-production-on-deploy-failure: a following push's deploy must + # not be inside the deploy directory while rollback.sh is. + concurrency: + group: production-deploy + cancel-in-progress: false + # !cancelled() rather than always(): e2e-prod failing is when this has to + # run, but an operator cancelling the run, say over a known flake, must + # still be able to stop it. The main-push gate is repeated so no other event + # can open production SSH. + if: >- + !cancelled() && github.ref == 'refs/heads/main' && github.event_name == 'push' && + needs.e2e-prod.outputs.tests == 'failure' + steps: + - name: Roll back production uses: appleboy/ssh-action@0ff4204d59e8e51228ff73bce53f80d53301dee2 # v1.2.5 with: host: ${{ secrets.SERVER_HOST }} @@ -934,8 +956,8 @@ jobs: # Reached only once the app answered /api/health. The box is no # longer mid-deploy, so drop the marker; any failure after this - # point is a smoke/E2E failure, which the rollback steps on those - # jobs already cover. + # point is a smoke/E2E failure, which the smoke job's rollback step + # and rollback-production-on-e2e-failure already cover. rm -f ${{ env.APP_DIR }}/.deploy-in-progress rollback-production-on-deploy-failure: @@ -959,9 +981,9 @@ jobs: # would then SSH into production to consider rolling back a deploy that # never ran. Keyed to the deploy job's own result, it cannot. # - # This is the gap the two existing rollbacks do not cover. Both hang off - # jobs DOWNSTREAM of the deploy (production-smoke-tests, e2e-prod), and a - # failed deploy skips those jobs and their rollback steps with them. + # This is the gap the two other rollbacks do not cover. Both hang off jobs + # DOWNSTREAM of the deploy (production-smoke-tests, e2e-prod), and a failed + # deploy skips those jobs, and so the rollbacks keyed to them. # 'cancelled' as well as 'failure'. Cancelling mid-deploy leaves production # exactly as half-deployed as a crash does, and deploy-production sets # cancel-in-progress: false, so this can only be a human pressing cancel — @@ -1368,7 +1390,7 @@ jobs: echo "" smoke_summary [ "$FAIL" -eq 0 ] || exit 1 - # Auto-rollback on smoke failure, mirroring the prod E2E job. A failed + # Auto-rollback on smoke failure, as on an E2E failure. A failed # smoke test means a bad build is live serving prod; revert to # last-known-good rather than leaving it up for a human to catch. The # condition is scoped to the smoke step's own outcome, so a failure in the @@ -1383,7 +1405,7 @@ jobs: key: ${{ secrets.SERVER_SSH_KEY }} envs: APP_DIR script: | - # See the identical assertion in e2e-prod's rollback step. + # See the identical assertion in rollback-production-on-e2e-failure. [ "$(hostname)" = "retina-prod" ] || { echo "::error::Refusing to roll back on '$(hostname)' — expected retina-prod."; exit 1; } echo "Production smoke tests failed — rolling back production..." cd ${{ env.APP_DIR }} && bash deploy/rollback.sh diff --git a/backend/tests/test_deploy_workflows.py b/backend/tests/test_deploy_workflows.py index bae525009..2ea43cfd2 100644 --- a/backend/tests/test_deploy_workflows.py +++ b/backend/tests/test_deploy_workflows.py @@ -4,6 +4,7 @@ and pin what must hold in every copy. """ +import functools import re from pathlib import Path @@ -12,6 +13,12 @@ WORKFLOWS = Path(__file__).resolve().parents[2] / ".github" / "workflows" + +@functools.cache +def _workflow(workflow: str) -> dict: + return yaml.safe_load((WORKFLOWS / workflow).read_text()) + + DEPLOYS = [ pytest.param("ci.yml", "deploy-production", id="production"), pytest.param("staging-deploy-verify.yml", "deploy", id="staging"), @@ -26,7 +33,7 @@ def _job(workflow: str, job: str) -> dict: - jobs = yaml.safe_load((WORKFLOWS / workflow).read_text())["jobs"] + jobs = _workflow(workflow)["jobs"] assert job in jobs, f"{workflow} has no job {job!r}" return jobs[job] @@ -205,22 +212,165 @@ def test_rollback_job_reads_back_what_the_deploy_wrote(workflow, deploy, rollbac assert "always()" in condition +def _allowed(workflow: str, job: str, values: dict[str, str], *, cancelled: bool = False) -> bool: + """A job's `if:`, evaluated with each context field replaced by a value.""" + condition = " ".join(_job(workflow, job)["if"].split()) + for field, value in values.items(): + # Whole fields only: github.ref must not also rewrite github.ref_name. + condition = re.sub(rf"(? list[Path]: + return sorted([*WORKFLOWS.glob("*.yml"), *WORKFLOWS.glob("*.yaml")]) + + +def _workflow_jobs() -> list: + return [ + pytest.param(path.name, name, job, id=f"{path.name}:{name}") + for path in _workflow_files() + for name, job in _workflow(path.name)["jobs"].items() + ] + + +def _runs_third_party_code(job: dict) -> bool: + steps = job.get("steps", []) + return ( + "container" in job + or "services" in job + # Another repository's reusable workflow. + or ("uses" in job and not str(job["uses"]).startswith("./")) + or any("uses" in step and not str(step["uses"]).startswith(KEY_JOB_ACTIONS) for step in steps) + or any(THIRD_PARTY_CODE.search(step.get("run", "")) for step in steps) + ) + + +def test_the_guard_recognises_what_the_jobs_run(): + # Guards the guard: patterns that matched nothing would pass every job. + assert _runs_third_party_code(_job("ci.yml", "web-build")) + assert _runs_third_party_code(_job("ci.yml", "backend-tests")) + assert _runs_third_party_code(_job("ci.yml", "lint")) + assert _runs_third_party_code(_job("ci.yml", "e2e-prod")) + assert _runs_third_party_code({"services": {"db": {"image": "postgres"}}, "steps": []}) + assert _runs_third_party_code({"steps": [{"uses": "docker://alpine:3"}]}) + assert _runs_third_party_code({"steps": [{"uses": "./.github/actions/setup-uv"}]}) + assert _runs_third_party_code({"steps": [{"run": "uv run python scripts/check.py"}]}) + assert _runs_third_party_code({"steps": [{"run": "docker run --rm some/image"}]}) + assert _runs_third_party_code({"steps": [{"run": "npm --prefix frontend ci"}]}) + assert _runs_third_party_code({"steps": [{"run": "sudo apt-get -y install jq"}]}) + assert _runs_third_party_code({"uses": "someorg/repo/.github/workflows/x.yml@main"}) + assert not _runs_third_party_code(_job("ci.yml", "staging")) + assert not _runs_third_party_code(_job("ci.yml", "deploy-production")) + assert not _runs_third_party_code(_job("ci.yml", "production-smoke-tests")) + assert SSH_KEY.search(yaml.safe_dump(_job("ci.yml", "deploy-production"))) + assert SSH_KEY.search("key: ${{ secrets['STAGING_SSH_KEY'] }}") + assert SSH_KEY.search("key: ${{ secrets[format('{0}_SSH_KEY', inputs.env)] }}") + assert SSH_KEY.search("ALL: ${{ toJson(secrets) }}") + assert SSH_KEY.search("key: ${{ secrets.server_ssh_key }}") + assert SSH_KEY.search("key: ${{ secrets.DEPLOY_SSH_PRIVATE_KEY }}") + assert SSH_KEY.search(yaml.safe_dump({"uses": "someorg/repo/.github/workflows/x.yml@main", "secrets": "inherit"})) + assert not SSH_KEY.search("RADAR_API_KEY: ${{ secrets.RADAR_API_KEY }}") + + +@pytest.mark.parametrize(("workflow", "name", "job"), _workflow_jobs()) +def test_no_job_that_runs_third_party_code_can_read_a_droplet_key(workflow, name, job): + if _runs_third_party_code(job): + assert not SSH_KEY.search(yaml.safe_dump(job)), f"{workflow}:{name} runs third-party code" + + +@pytest.mark.parametrize("path", _workflow_files(), ids=lambda path: path.name) +def test_no_droplet_key_is_handed_to_every_job(path): + # Workflow-level env reaches every job, the npm ones included. + assert not SSH_KEY.search(yaml.safe_dump(_workflow(path.name).get("env", {}))) + + +# ── Production's E2E rollback ──────────────────────────────────────────────── + + +def test_the_e2e_rollback_acts_on_the_test_step_alone(): + # A failed pull, checkout or npm ci fails e2e-prod as well, and says + # nothing about production. Only the test step's own verdict may count. + e2e = _job("ci.yml", "e2e-prod") + assert [step for step in e2e["steps"] if step.get("id") == "e2e" and "test:e2e:prod" in step["run"]] + assert e2e["outputs"]["tests"] == "${{ steps.e2e.outcome }}" + + +def test_the_e2e_rollback_holds_the_production_lock_on_the_runner(): + rollback = _job("ci.yml", "rollback-production-on-e2e-failure") + assert "container" not in rollback + assert rollback["needs"] == ["e2e-prod"] + assert rollback["concurrency"] == {"group": "production-deploy", "cancel-in-progress": False} + script = _script("ci.yml", "rollback-production-on-e2e-failure") + identity = _index(script, re.escape('[ "$(hostname)" = "retina-prod" ]')) + assert identity < _index(script, r"\bbash deploy/rollback\.sh$") + + +@pytest.mark.parametrize("ref", ["refs/heads/main", "refs/heads/topic", "refs/pull/417/merge"]) +@pytest.mark.parametrize("event", ["push", "pull_request", "workflow_dispatch"]) +@pytest.mark.parametrize("tests", ["failure", "cancelled", "success", "skipped", ""]) +@pytest.mark.parametrize("cancelled", [False, True]) +def test_the_e2e_rollback_requires_failed_tests_on_a_main_push(ref, event, tests, cancelled): + # "" is what the output reads when e2e-prod never reached the test step, + # or was skipped outright. Cancelling the run stops it: an operator cancels + # to keep a known flake from reverting a healthy production. + allowed = _allowed( + "ci.yml", + "rollback-production-on-e2e-failure", + {"github.ref": ref, "github.event_name": event, "needs.e2e-prod.outputs.tests": tests}, + cancelled=cancelled, + ) + assert allowed is (ref == "refs/heads/main" and event == "push" and tests == "failure" and not cancelled)