diff --git a/.github/workflows/e2e-fork.yaml b/.github/workflows/e2e-fork.yaml index 4c124fe1a..1d438cf79 100644 --- a/.github/workflows/e2e-fork.yaml +++ b/.github/workflows/e2e-fork.yaml @@ -81,8 +81,9 @@ jobs: head_sha: ${{ steps.r.outputs.head_sha }} run_id: ${{ steps.r.outputs.run_id }} # TIA inputs derived from TRUSTED base-repo data (the PR file list and - # labels), NOT the fork-controlled merge-ref diff — so suite selection - # keeps the same anti-forgery property as the docs-only decision (#3257). + # labels), NOT the fork-controlled merge-ref diff. That covers the INPUTS + # only — the selector and the suites themselves run from the fork tree in + # the `e2e` job; see that job's header for what actually holds. changed_b64: ${{ steps.r.outputs.changed_b64 }} full_e2e: ${{ steps.r.outputs.full_e2e }} steps: @@ -245,9 +246,10 @@ jobs: // docs-only decision above: the base-repo PR file list drives suite // selection (base64 so newlines survive the job output), and the // base-repo PR labels drive the full-e2e override. Deriving these - // from `git diff` on the fork-controlled merge ref would let a fork - // shape the diff so TIA selects nothing and e2e is skipped — the - // same class of bypass #3257 closes. Note select-e2e.sh escalates + // from `git diff` on the fork-controlled merge ref would hand a + // fork a second way to make TIA select nothing, on top of the one + // it already has — the selector runs from the fork tree, so this + // hardens the input and not the decision. Note select-e2e.sh escalates // to the full suite for an unrecognised path INSIDE packages/, but // silently selects nothing for a path it does not recognise at all // (e.g. hack/*.mk) — a pre-existing fail-open shared with the @@ -461,8 +463,18 @@ jobs: # Run the e2e suite against the pushed closure. NO OCIR push token here — the # sandbox pulls images anonymously; the fork's images/scripts run only in this # ephemeral job. Mirrors pull-requests.yaml's in-tree e2e, including Chainsaw - # Test-Impact Analysis — scoped from resolve's TRUSTED base-repo file list - # (not a fork-controlled merge-ref diff), so suite selection can only narrow. + # Test-Impact Analysis, whose INPUTS come from resolve's trusted base-repo + # file list rather than a merge-ref diff the fork could shape. + # + # What that does NOT buy: this job runs from the fork's own tree, so the + # selector (hack/select-e2e.sh), the suites under hack/e2e-chainsaw/, the + # install bats and packages/core/testing's recipes are all fork-authored. A + # fork that wants its own suite to pass can simply edit them, and no amount + # of input hardening changes that. This job is a correctness gate that stops + # an honest contributor shipping a regression, not a boundary against a + # hostile one — the boundary is the maintainer approval required before any + # fork run executes at all (see §10 of docs/agents/e2e-testing.md), plus the + # fact that nothing here holds a credential worth stealing. e2e: name: E2E (fork run) needs: [resolve, publish] @@ -553,7 +565,8 @@ jobs: # Test-Impact Analysis: select only the Chainsaw suites the PR diff # affects. The changed-file list is resolve's TRUSTED base-repo listFiles # (base64 through the job output), NOT a git diff on the fork-controlled - # merge ref — so a fork cannot shape the diff to make TIA select nothing. + # merge ref. The script below is the fork's own, so this hardens what the + # selector reads and not what it decides — see the job header. # select-e2e.sh escalates to the full suite for an unrecognised path # inside packages/; a path it does not recognise at all selects nothing # and the run below is skipped (#3392 — same in the in-tree lane). diff --git a/docs/agents/e2e-testing.md b/docs/agents/e2e-testing.md index 2e4b011f5..b8ae8c798 100644 --- a/docs/agents/e2e-testing.md +++ b/docs/agents/e2e-testing.md @@ -115,7 +115,8 @@ Consequences to keep in mind when touching either workflow: - *Not a check-run:* a check-run created with the default `GITHUB_TOKEN` floats into an arbitrary GitHub Actions check-suite (in practice the earliest-registered one, a tiny labeler — that is why `E2E Tests` once rendered under "PR size label"), and an app-token check-run would need the COZYSTACK_CI app granted `checks: write`. A commit status has neither problem: branch protection matches it by context name and it carries no check-suite grouping. It is safe to post with the default token because a fork's `pull_request` token is **read-only** and cannot post a status — so only same-repo runs (`e2e-report`, guarded `!fork`) and the privileged `workflow_run` in `e2e-fork.yaml` can set it. Each publishing job carries `permissions: statuses: write` and calls `repos.createCommitStatus`. **Consequence for branch protection:** require the `E2E Tests` **status context** (no GitHub App and no app-id pin needed). -- **Fork TIA is driven from trusted data, not a merge-ref diff.** `e2e-fork.yaml`'s `e2e` job runs the same `select-e2e.sh` TIA as in-tree (§7), but a `workflow_run` job cannot trust a `git diff` of the fork-controlled merge ref — a fork could shape that diff so TIA selects nothing and the suite is skipped. So the changed-file list and the `full-e2e` label both come from `resolve`'s base-repo API lookups (`pulls.listFiles` / the PR's labels), the same trusted source as the docs-only decision, passed to the `e2e` job as outputs (`changed_b64`, `full_e2e`). Keep it that way: never re-derive the fork selection from the checked-out tree. +- **Fork TIA reads trusted inputs, but the fork still writes the selector.** `e2e-fork.yaml`'s `e2e` job runs the same `select-e2e.sh` TIA as in-tree (§7), and its inputs are hardened: the changed-file list and the `full-e2e` label come from `resolve`'s base-repo API lookups (`pulls.listFiles` / the PR's labels), the same trusted source as the docs-only decision, passed in as outputs (`changed_b64`, `full_e2e`) rather than re-derived from a `git diff` of the fork-controlled merge ref. Keep that part as it is: re-deriving the selection from the checked-out tree would add a second bypass for no gain. +- **What the fork lane is, and is not.** The `e2e` job runs from the fork's tree, so `select-e2e.sh`, the suites under `hack/e2e-chainsaw/`, `hack/e2e-install-cozystack.bats` and `packages/core/testing`'s recipes are all fork-authored. A fork that wants its own suite to report green can edit any of them, and hardening the TIA inputs does not change that. Read the lane accordingly: it is a correctness gate that stops an honest contributor shipping a regression, not a boundary against a hostile one. What contains a hostile fork is that no fork run executes without maintainer approval (below), that the job holds no push credential, and that a diff editing the test harness is visible in review. - **A fork PR that adds a new image cannot go green until that image exists on the base branch.** `publish` derives the set of archive names it will accept from the base parent (`HEAD^1`), so that a fork cannot mint arbitrary repositories under the CI registry by naming an artifact. An image the PR itself introduces is therefore not in that set, and the job fails with `image '' is not in the base-tree allowlist`. This is fail-closed by design and there is no label override — land the image on the base branch first, or run the suite from a same-repo branch. If a fork e2e run fails on that line, the contribution is not broken; the allowlist is doing its job.