Skip to content

Commit 673cbf4

Browse files
basvandijkclaude
andauthored
fix(CI): verify that commit-sha is the head of pr-number in ci-kickoff-manual (#11174)
The `ci-kickoff-manual` workflow takes a `commit-sha` and a `pr-number` input, with the intention that the commit belongs to that PR. Nothing checked this. A mismatch would run CI on unrelated code while reporting commit statuses and PR comments against the PR, and — because the auto-fix jobs in `ci-pr-only.yml` do `git push origin HEAD:'${{ inputs.head-ref }}'` — would let did-check / buf-format / cargo-lock fixups derived from a foreign tree be pushed onto the PR's head branch. ## Changes - `get-pr-refs` already fetches the PR, so it is extended to verify that `commit-sha` is the **head commit** of the PR, and renamed to `verify-inputs-and-get-pr-refs`. - It also rejects a `pr-number` that isn't a positive decimal integer or a `commit-sha` that isn't a full 40-hex SHA, and refuses non-open PRs. - Neither `github-script` step interpolates the inputs into its JS body any more — both the verification job and `post-comment-with-ci-link` read them from `env:`, removing a script-injection surface. The remaining `${{ inputs.* }}` uses are in `with:` / `ref:` positions, where the gate above guarantees the values are a 40-hex SHA and a decimal integer. - The `confirm` checkbox is now enforced: for boolean `workflow_dispatch` inputs `required: true` only means the field is present, not that it is ticked, so the acknowledgement that the PR was reviewed at this commit was optional in practice. - Every job now depends on the verification. Previously only `ci-pr-only` did, so a verification failure did not stop `ci-main`, the PR comment, or the Slack notification — a failed job does not cancel independent jobs. ## Don't push autofixes to a branch named after a fork's branch Follow-up on a review comment, in the same area: the pushing jobs in `ci-pr-only.yml` (`autofix`, `lock-generate`, `generate-config-fixtures`) do `git push origin HEAD:<head-ref>`. `actions/checkout` defaults to `repository: github.repository`, so `origin` is always this repository, while `head-ref` is `pr.head.ref` — an unqualified branch name. On the `pull_request` path that is fine: `ci-kickoff.yml` only calls `ci-pr-only` when `head.repo.full_name == github.repository`. On the `ci-kickoff-manual` path — which exists for external contributions — `head-ref` is the *fork's* branch name, so the push creates a branch of that name **here** instead of updating the PR. It uses the `PR_CREATION_BOT` app token, so the job's `contents: read` does not restrain it. Two notable cases: a fork branch called `main`/`master` is rejected by the branch ruleset (so the job just fails), and a fork branch called `dev-gh-*` would land on a name that is a `push` trigger for Kickoff, running CI Main with secrets on unreviewed code. The fix gates the push rather than the jobs: - `ci-pr-only.yml` takes a new `head-repo` input, defaulting to `github.event.pull_request.head.repo.full_name` and supplied on the manual path from the `pulls.get` response the verification job already has (empty if the fork was deleted, which fails closed). - The three jobs commit and push only when it equals `github.repository`; otherwise they print the staged diff and still exit non-zero. The commit is created only on the pushing path, so the printed diff is the patch the contributor actually has to apply (a `git diff --cached` after committing would print nothing). Guarding the jobs with `if: github.event_name == 'pull_request' && …` instead would work, but `autofix` is the only place the `//pre-commit:*` checks run (`shfmt`, `ruff`, `buf`, `buf-breaking`, `do-not-merge`, `buildifier`, `gazelle`, `rustfmt`), so external PRs would get a green `ci-pr-only-status` with no lint gate at all. Gating the push keeps those checks gating exactly the PRs that need them most, and leaves the contributor an actionable diff in the log. `head-ref` and `head-repo` are also passed to the scripts through the environment now instead of being interpolated: git allows single quotes in branch names, so `HEAD:'${{ inputs.head-ref }}'` was itself a fork-controlled shell-injection surface. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2c31ebb commit 673cbf4

2 files changed

Lines changed: 122 additions & 30 deletions

File tree

.github/workflows/ci-kickoff-manual.yml

Lines changed: 67 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ on:
44
workflow_dispatch:
55
inputs:
66
commit-sha:
7-
description: 'The commit SHA to run the workflow on'
7+
description: 'The commit SHA to run the workflow on (must be the head commit of the PR)'
88
required: true
99
default: ''
1010
pr-number:
@@ -17,37 +17,79 @@ on:
1717
type: boolean
1818

1919
jobs:
20-
get-pr-refs:
21-
name: Get PR Refs
20+
verify-inputs-and-get-pr-refs:
21+
name: Verify Inputs and Get PR Refs
2222
runs-on: ubuntu-latest
2323
permissions:
2424
contents: read
2525
pull-requests: read
2626
outputs:
2727
base-ref: ${{ steps.fetch.outputs.base_ref }}
2828
head-ref: ${{ steps.fetch.outputs.head_ref }}
29+
head-repo: ${{ steps.fetch.outputs.head_repo }}
2930
steps:
30-
- name: Fetch PR Base Ref
31+
- name: Verify Commit Is PR Head and Fetch PR Refs
3132
id: fetch
3233
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
34+
env:
35+
PR_NUMBER: ${{ inputs.pr-number }}
36+
COMMIT_SHA: ${{ inputs.commit-sha }}
37+
CONFIRM: ${{ inputs.confirm }}
3338
with:
3439
script: |
35-
const prNumber = parseInt('${{ inputs.pr-number }}', 10);
36-
if (isNaN(prNumber)) {
37-
core.setFailed('Invalid pr-number input');
40+
// For boolean workflow_dispatch inputs, 'required: true' does not
41+
// enforce that the box is ticked, so check it here.
42+
if (process.env.CONFIRM !== 'true') {
43+
core.setFailed('confirm must be checked: it acknowledges that you reviewed ' +
44+
'the PR at the specified commit');
45+
return;
46+
}
47+
48+
// Match on the raw string: Number() would also accept forms like
49+
// '0x10', '1e3' or ' 12 ', which would target a different PR.
50+
if (!/^[1-9][0-9]*$/.test(process.env.PR_NUMBER || '')) {
51+
core.setFailed(`pr-number must be a positive decimal integer, got: ${process.env.PR_NUMBER}`);
3852
return;
3953
}
54+
const prNumber = Number(process.env.PR_NUMBER);
55+
const sha = (process.env.COMMIT_SHA || '').toLowerCase();
56+
if (!/^[0-9a-f]{40}$/.test(sha)) {
57+
core.setFailed('commit-sha must be a full 40-character commit SHA');
58+
return;
59+
}
60+
4061
const { data: pr } = await github.rest.pulls.get({
4162
owner: context.repo.owner,
4263
repo: context.repo.repo,
4364
pull_number: prNumber
4465
});
45-
core.info(`PR #${prNumber} base ref: ${pr.base.ref}, head ref: ${pr.head.ref}`);
66+
if (pr.state !== 'open') {
67+
core.setFailed(`PR #${prNumber} is ${pr.state}, refusing to run CI on it`);
68+
return;
69+
}
70+
71+
// Require the head commit: the confirm checkbox states the PR was
72+
// reviewed at this commit, and an older commit of the PR would run
73+
// CI on code that is no longer what the PR proposes to merge.
74+
if (sha !== pr.head.sha.toLowerCase()) {
75+
core.setFailed(`Commit ${sha} is not the head of PR #${prNumber} ` +
76+
`(head is ${pr.head.sha}). Re-run with the head commit.`);
77+
return;
78+
}
79+
80+
// Empty if the fork was deleted after the PR was opened. Jobs that
81+
// push to the head branch compare this against github.repository and
82+
// skip the push unless the branch lives in this repository.
83+
const headRepo = pr.head.repo ? pr.head.repo.full_name : '';
84+
core.info(`PR #${prNumber} base ref: ${pr.base.ref}, ` +
85+
`head ref: ${pr.head.ref}, head repo: ${headRepo}`);
4686
core.setOutput('base_ref', pr.base.ref);
4787
core.setOutput('head_ref', pr.head.ref);
88+
core.setOutput('head_repo', headRepo);
4889
4990
ci-main:
5091
name: CI Main
92+
needs: [verify-inputs-and-get-pr-refs]
5193
uses: ./.github/workflows/ci-main.yml
5294
secrets: inherit
5395
with:
@@ -61,13 +103,14 @@ jobs:
61103

62104
ci-pr-only:
63105
name: CI PR Only
64-
needs: [get-pr-refs]
106+
needs: [verify-inputs-and-get-pr-refs]
65107
uses: ./.github/workflows/ci-pr-only.yml
66108
secrets: inherit
67109
with:
68110
commit-sha: ${{ inputs.commit-sha }}
69-
base-ref: ${{ needs.get-pr-refs.outputs.base-ref }}
70-
head-ref: ${{ needs.get-pr-refs.outputs.head-ref }}
111+
base-ref: ${{ needs.verify-inputs-and-get-pr-refs.outputs.base-ref }}
112+
head-ref: ${{ needs.verify-inputs-and-get-pr-refs.outputs.head-ref }}
113+
head-repo: ${{ needs.verify-inputs-and-get-pr-refs.outputs.head-repo }}
71114
pr-number: ${{ inputs.pr-number }}
72115
permissions:
73116
contents: read
@@ -114,26 +157,35 @@ jobs:
114157
post-comment-with-ci-link:
115158
name: Post Comment with CI Link
116159
runs-on: ubuntu-latest
160+
needs: [verify-inputs-and-get-pr-refs]
117161
permissions:
118162
contents: read
119163
pull-requests: write
120164
steps:
121165
- name: Add PR Comment
122166
uses: actions/github-script@60a0d83039c74a4aee543508d2ffcb1c3799cdea # v7.0.1
167+
env:
168+
PR_NUMBER: ${{ inputs.pr-number }}
169+
COMMIT_SHA: ${{ inputs.commit-sha }}
123170
with:
124171
script: |
125-
let message = 'Run on ${{ inputs.commit-sha }} URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}\n\n'
172+
// Both inputs were validated by verify-inputs-and-get-pr-refs, which
173+
// this job depends on.
174+
const runUrl = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}` +
175+
`/actions/runs/${context.runId}`;
176+
const message = `Run on ${process.env.COMMIT_SHA} URL: ${runUrl}\n\n`;
126177
127-
github.rest.issues.createComment({
128-
issue_number: ${{ inputs.pr-number }},
178+
await github.rest.issues.createComment({
179+
issue_number: Number(process.env.PR_NUMBER),
129180
owner: context.repo.owner,
130181
repo: context.repo.repo,
131182
body: message
132-
})
183+
});
133184
134185
notify-slack-new-workflow-run:
135186
name: Notify Slack for new External Contributor Workflow Run
136187
runs-on: ubuntu-latest
188+
needs: [verify-inputs-and-get-pr-refs]
137189
steps:
138190
- name: Post to a Slack channel
139191
id: slack

.github/workflows/ci-pr-only.yml

Lines changed: 55 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,11 @@ on:
1818
required: false
1919
default: ${{ github.event.pull_request.head.ref }}
2020
type: string
21+
head-repo:
22+
description: The repository the head branch of the PR lives in ("owner/repo")
23+
required: false
24+
default: ${{ github.event.pull_request.head.repo.full_name }}
25+
type: string
2126
pr-number:
2227
description: 'The PR number'
2328
required: false
@@ -63,6 +68,9 @@ jobs:
6368
runs-on: *dind-small-setup
6469
container: *container-setup
6570
timeout-minutes: 30
71+
env: &pr-branch-env
72+
HEAD_REF: ${{ inputs.head-ref }}
73+
HEAD_REPO: ${{ inputs.head-repo }}
6674
steps:
6775
- name: Create GitHub App Token
6876
uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0
@@ -116,11 +124,21 @@ jobs:
116124
git add --all
117125
git status
118126
if ! git diff --cached --quiet; then
119-
# There are some changes staged
120-
git config --global user.email "infra+github-automation@dfinity.org"
121-
git config --global user.name "IDX GitHub Automation"
122-
git commit -m "Automatically fixing code for linting and formatting issues"
123-
git push origin HEAD:'${{ inputs.head-ref }}'
127+
# There are some changes staged. Only commit and push them when the PR's
128+
# head branch lives in this repository: on the ci-kickoff-manual path the
129+
# PR can come from a fork, in which case "origin" is still this repository
130+
# and the push would create a branch named after the fork's branch here
131+
# instead of updating the PR. Show the diff for the contributor instead.
132+
if [ "$HEAD_REPO" = "$GITHUB_REPOSITORY" ]; then
133+
git config --global user.email "infra+github-automation@dfinity.org"
134+
git config --global user.name "IDX GitHub Automation"
135+
git commit -m "Automatically fixing code for linting and formatting issues"
136+
git push origin HEAD:"$HEAD_REF"
137+
else
138+
echo "Head branch '$HEAD_REF' lives in '$HEAD_REPO', not in" \
139+
"'$GITHUB_REPOSITORY': not pushing. Apply the diff below instead:"
140+
git --no-pager diff --cached
141+
fi
124142
125143
# Because the buildifier made changes, fail the PR
126144
EXIT_STATUS=1
@@ -163,6 +181,7 @@ jobs:
163181
name: Lock Generate
164182
runs-on: ubuntu-latest
165183
timeout-minutes: 30
184+
env: *pr-branch-env
166185
steps:
167186
- name: Create GitHub App Token
168187
uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0
@@ -205,11 +224,21 @@ jobs:
205224
git add Cargo.lock Cargo.Bazel.*.lock
206225
git status
207226
if ! git diff --cached --quiet; then
208-
# There are some changes staged
209-
git config --global user.email "infra+github-automation@dfinity.org"
210-
git config --global user.name "IDX GitHub Automation"
211-
git commit -m "Automatically updated Cargo*.lock"
212-
git push origin HEAD:'${{ inputs.head-ref }}'
227+
# There are some changes staged. Only commit and push them when the PR's
228+
# head branch lives in this repository: on the ci-kickoff-manual path the
229+
# PR can come from a fork, in which case "origin" is still this repository
230+
# and the push would create a branch named after the fork's branch here
231+
# instead of updating the PR. Show the diff for the contributor instead.
232+
if [ "$HEAD_REPO" = "$GITHUB_REPOSITORY" ]; then
233+
git config --global user.email "infra+github-automation@dfinity.org"
234+
git config --global user.name "IDX GitHub Automation"
235+
git commit -m "Automatically updated Cargo*.lock"
236+
git push origin HEAD:"$HEAD_REF"
237+
else
238+
echo "Head branch '$HEAD_REF' lives in '$HEAD_REPO', not in" \
239+
"'$GITHUB_REPOSITORY': not pushing. Apply the diff below instead:"
240+
git --no-pager diff --cached
241+
fi
213242
214243
# Because the lockfiles need updating, fail the PR
215244
EXIT_STATUS=1
@@ -222,6 +251,7 @@ jobs:
222251
runs-on: *dind-small-setup
223252
container: *container-setup
224253
timeout-minutes: 10
254+
env: *pr-branch-env
225255
steps:
226256
- name: Create GitHub App Token
227257
uses: actions/create-github-app-token@f8d387b68d61c58ab83c6c016672934102569859 # v3.0.0
@@ -255,11 +285,21 @@ jobs:
255285
git add rs/ic_os/config/types/compatibility_tests/fixtures
256286
git status
257287
if ! git diff --cached --quiet; then
258-
# There are some changes staged
259-
git config --global user.email "infra+github-automation@dfinity.org"
260-
git config --global user.name "IDX GitHub Automation"
261-
git commit -m "Automatically updated config type fixtures"
262-
git push origin HEAD:'${{ inputs.head-ref }}'
288+
# There are some changes staged. Only commit and push them when the PR's
289+
# head branch lives in this repository: on the ci-kickoff-manual path the
290+
# PR can come from a fork, in which case "origin" is still this repository
291+
# and the push would create a branch named after the fork's branch here
292+
# instead of updating the PR. Show the diff for the contributor instead.
293+
if [ "$HEAD_REPO" = "$GITHUB_REPOSITORY" ]; then
294+
git config --global user.email "infra+github-automation@dfinity.org"
295+
git config --global user.name "IDX GitHub Automation"
296+
git commit -m "Automatically updated config type fixtures"
297+
git push origin HEAD:"$HEAD_REF"
298+
else
299+
echo "Head branch '$HEAD_REF' lives in '$HEAD_REPO', not in" \
300+
"'$GITHUB_REPOSITORY': not pushing. Apply the diff below instead:"
301+
git --no-pager diff --cached
302+
fi
263303
264304
# Because the fixtures need updating, fail the PR
265305
EXIT_STATUS=1

0 commit comments

Comments
 (0)