Skip to content

Commit eba5597

Browse files
fix(ci): decide fork docs-only from the PR file list and harden publish (#3257)
resolve inferred "docs-only" from the absence of a pr-patch artifact, but artifact presence is decided by pull-requests.yaml running from the fork's merge commit — a fork could suppress it (code=false, or if:false on finalize) and conclude the required E2E Tests check green with e2e never run, re-opening the very bypass this feature closes. Derive docs-only from trusted base-repo data (pulls.listFiles): green only when every changed file is under docs/; any non-docs file makes e2e required, and a missing pr-patch then fails the check closed. Resolve the PR open-only (no closed/merged fallback). Also harden the privileged jobs: - drop `packages: write` from publish (it authenticates to OCIR, not GHCR); - assert at least one OCI archive before pushing, and validate each archive basename against a strict charset before using it as a registry repo; - install flux from a downloaded script rather than piping curl to sudo bash; - assert `git rev-parse HEAD^2` equals the resolved head SHA in publish and e2e, so a moved merge ref cannot operate on a different tree than the check. Refs: #3257 Assisted-By: Claude <[email protected]> Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent 68cdeb8 commit eba5597

1 file changed

Lines changed: 64 additions & 11 deletions

File tree

‎.github/workflows/e2e-fork.yaml‎

Lines changed: 64 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,12 @@ permissions:
4242
jobs:
4343
# Decide whether this is a fork PR that needs e2e, resolve its PR number and
4444
# head SHA, and open the pending "E2E Tests" check-run so the PR is not
45-
# mergeable until we conclude it. Fail-closed if the fork build failed;
46-
# green-and-stop for docs-only fork PRs (no pr-patch artifact = nothing built).
45+
# mergeable until we conclude it. Every decision is derived from data the fork
46+
# cannot forge — the workflow_run conclusion, the base-repo PR list, and the
47+
# base-repo PR file list — NOT from the presence/absence of an artifact the
48+
# fork's own build controls. Fail-closed if the fork build failed or the
49+
# pr-patch artifact is missing; green-and-stop only for a PR whose file list is
50+
# entirely under docs/ (verified against the base-repo API).
4751
resolve:
4852
name: Resolve fork PR
4953
# Only fork PRs: head_repository differs from the base repository. Same-repo
@@ -56,6 +60,7 @@ jobs:
5660
contents: read
5761
checks: write
5862
actions: read
63+
pull-requests: read
5964
outputs:
6065
should_run: ${{ steps.r.outputs.should_run }}
6166
pr_number: ${{ steps.r.outputs.pr_number }}
@@ -99,28 +104,46 @@ jobs:
99104
}
100105
101106
// Resolve the PR number for this commit (workflow_run.pull_requests
102-
// is empty for forks). Validate it is a bare integer before it flows
103-
// into a checkout ref.
107+
// is empty for forks). Select ONLY an OPEN PR — never fall back to a
108+
// closed/merged association, which a fork could point at a stale
109+
// commit to smuggle a green through. Validate it is a bare integer
110+
// before it flows into a checkout ref.
104111
const assoc = await github.rest.repos.listPullRequestsAssociatedWithCommit({
105112
owner, repo, commit_sha: HEAD_SHA,
106113
});
107-
const pr = assoc.data.find(p => p.state === 'open') || assoc.data[0];
114+
const pr = assoc.data.find(p => p.state === 'open');
108115
const prNumber = pr ? String(pr.number) : '';
109116
if (!/^[0-9]+$/.test(prNumber)) {
110-
await conclude('failure', `could not resolve an open PR for ${HEAD_SHA}`);
117+
await conclude('failure', `no open PR for ${HEAD_SHA}`);
111118
core.setOutput('should_run', 'false');
112119
return;
113120
}
114121
core.setOutput('pr_number', prNumber);
115122
116-
// Docs-only fork PR: the build/finalize chain was skipped, so no
117-
// pr-patch artifact exists — nothing to test, conclude success.
123+
// Docs-only decision from TRUSTED data: the base-repo PR file list,
124+
// NOT the presence of a fork-build artifact. A fork controls whether
125+
// pull-requests.yaml emits pr-patch (its plan step runs from the fork
126+
// merge commit), so inferring docs-only from a missing artifact would
127+
// let a fork skip e2e and conclude green with nothing tested (#3257).
128+
const files = await github.paginate(github.rest.pulls.listFiles, {
129+
owner, repo, pull_number: Number(prNumber), per_page: 100,
130+
});
131+
const hasNonDocs = files.some(f => !f.filename.startsWith('docs/'));
132+
if (!hasNonDocs) {
133+
await conclude('success', 'docs-only PR (verified via PR file list) — e2e not required');
134+
core.setOutput('should_run', 'false');
135+
return;
136+
}
137+
138+
// Non-docs change → e2e is REQUIRED. The fork build must have produced
139+
// the pr-patch artifact; a missing one means the build did not
140+
// complete, so fail closed (never conclude green without running e2e).
118141
const arts = await github.paginate(
119142
github.rest.actions.listWorkflowRunArtifacts,
120143
{ owner, repo, run_id: RUN_ID, per_page: 100 },
121144
);
122145
if (!arts.some(a => a.name === 'pr-patch')) {
123-
await conclude('success', 'docs-only PR — e2e not required');
146+
await conclude('failure', 'expected pr-patch artifact is missing — the fork build did not complete');
124147
core.setOutput('should_run', 'false');
125148
return;
126149
}
@@ -143,17 +166,18 @@ jobs:
143166
if: ${{ needs.resolve.outputs.should_run == 'true' }}
144167
runs-on: oracle-vm-4cpu-16gb-x86-64
145168
timeout-minutes: 40
169+
# No `packages: write`: this job authenticates to OCIR via OCIR_USER/
170+
# OCIR_TOKEN (not the GHCR GITHUB_TOKEN), so it needs no GHCR push scope.
146171
permissions:
147172
contents: read
148-
packages: write
149173
actions: read
150174
outputs:
151175
packages_digest: ${{ steps.push.outputs.packages_digest }}
152176
steps:
153177
- name: Set up toolchain (skopeo, flux)
154178
run: |
155179
command -v skopeo >/dev/null || { sudo apt-get update -qq && sudo apt-get install -y -qq skopeo; }
156-
command -v flux >/dev/null || curl -fsSL https://fluxcd.io/install.sh | sudo bash
180+
if ! command -v flux >/dev/null; then s="$(mktemp)"; curl -fsSL https://fluxcd.io/install.sh -o "$s"; sudo bash "$s"; rm -f "$s"; fi
157181
# The base repo's own merge ref — safe to fetch, not fork-controlled code
158182
# execution. pr.patch is applied as DATA below; nothing from the tree runs.
159183
- name: Checkout PR merge ref
@@ -162,6 +186,16 @@ jobs:
162186
ref: refs/pull/${{ needs.resolve.outputs.pr_number }}/merge
163187
fetch-depth: 0
164188
persist-credentials: false
189+
# Guard against a TOCTOU race: the check-run is stamped on the SHA `resolve`
190+
# saw, but refs/pull/N/merge tracks the live PR head. If the fork pushed
191+
# again between resolve and here, HEAD^2 (the merge ref's PR-head parent)
192+
# no longer matches — abort rather than push/test unresolved code.
193+
- name: Assert checkout matches resolved head SHA
194+
env:
195+
EXPECTED_SHA: ${{ needs.resolve.outputs.head_sha }}
196+
run: |
197+
got="$(git rev-parse HEAD^2)"
198+
[ "$got" = "$EXPECTED_SHA" ] || { echo "::error::merge ref moved: HEAD^2=$got != resolved $EXPECTED_SHA"; exit 1; }
165199
- name: Download pr.patch
166200
uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4
167201
with:
@@ -199,8 +233,19 @@ jobs:
199233
run: |
200234
set -eu
201235
shopt -s nullglob
236+
# A non-docs fork PR must have exported at least one image; an empty
237+
# download means the build silently produced nothing — fail rather than
238+
# push an empty closure and let e2e install a stale/mismatched tree.
239+
count=$(ls _out/oci/*.oci.tar 2>/dev/null | wc -l)
240+
[ "$count" -ge 1 ] || { echo "::error::no OCI image archives to push"; exit 1; }
202241
for f in _out/oci/*.oci.tar; do
203242
name="$(basename "$f" .oci.tar)"
243+
# The archive basename becomes a registry repo path — the fork picked
244+
# the filename, so validate it against a strict charset before it
245+
# flows into a docker:// ref (defence against a crafted artifact name).
246+
case "$name" in
247+
*[!a-z0-9._-]*|""|-*) echo "::error::unexpected image archive name: $name"; exit 1;;
248+
esac
204249
echo "Pushing $name → ${REGISTRY}/${name}:${IMAGE_TAG}"
205250
# skopeo preserves the manifest digest bit-for-bit, so the
206251
# ${REGISTRY}/${name}@sha256:… refs in pr.patch resolve.
@@ -244,6 +289,14 @@ jobs:
244289
ref: refs/pull/${{ needs.resolve.outputs.pr_number }}/merge
245290
fetch-depth: 0
246291
persist-credentials: false
292+
# Same TOCTOU guard as `publish`: refuse to test a merge ref that moved off
293+
# the SHA the check-run is stamped on (see the publish job for the rationale).
294+
- name: Assert checkout matches resolved head SHA
295+
env:
296+
EXPECTED_SHA: ${{ needs.resolve.outputs.head_sha }}
297+
run: |
298+
got="$(git rev-parse HEAD^2)"
299+
[ "$got" = "$EXPECTED_SHA" ] || { echo "::error::merge ref moved: HEAD^2=$got != resolved $EXPECTED_SHA"; exit 1; }
247300
- name: Set up toolchain (yq)
248301
run: |
249302
command -v yq >/dev/null || { sudo curl -sSL -o /usr/local/bin/yq https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64 && sudo chmod +x /usr/local/bin/yq; }

0 commit comments

Comments
 (0)