diff --git a/CLAUDE.md b/CLAUDE.md index c73bf271..fb5bf8e0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -97,8 +97,9 @@ and [example/module/ci.yml](example/module/ci.yml)): secrets), gated on an `allowed-for-ci` label that only a CODEOWNER can apply. - `fork_ci_label_guard.yml` **strips** that label on `synchronize`/`closed`, so every new commit re-requires a fresh CODEOWNER review and re-label. The caller - intentionally omits `synchronize` from its acceptance trigger to avoid racing - the guard. + runs it as a job in `ci.yml`, and gates fork acceptance on the `allowed-for-ci` + labeling event itself (never on the label's presence), so a `synchronize` + payload that still carries the label can't start acceptance. When touching anything in this area, preserve the property that **untrusted code from a fork can never run with secrets without a CODEOWNER re-approving the diff --git a/docs/how-to/fork-pr-ci.md b/docs/how-to/fork-pr-ci.md index 79e211ca..eb2c38e9 100644 --- a/docs/how-to/fork-pr-ci.md +++ b/docs/how-to/fork-pr-ci.md @@ -54,7 +54,7 @@ The acceptance job uses the environment only on fork PRs (see `module_acceptance ### 5. Update the caller workflows -Replace `.github/workflows/ci.yml` with the two-track pattern (see [example/module/ci.yml](../../example/module/ci.yml)). Add a new file `.github/workflows/fork_ci_label_guard.yml` that strips the label on each new commit (see [example/module/fork_ci_label_guard.yml](../../example/module/fork_ci_label_guard.yml)). +Replace `.github/workflows/ci.yml` with the two-track pattern (see [example/module/ci.yml](../../example/module/ci.yml)). Its `Fork_CI_Label_Guard` job strips the label on each new commit and on close. Modules that render `ci.yml` from pdk-templates get the same job. Only a caller whose `ci.yml` doesn't include that job needs the separate `.github/workflows/fork_ci_label_guard.yml` (see [example/module/fork_ci_label_guard.yml](../../example/module/fork_ci_label_guard.yml)); don't use both. Keep "Require approval for first-time contributors" enabled at the org Actions setting — it's an additional pre-execution gate independent of this design. @@ -75,8 +75,8 @@ The Spec track has already run by the time the reviewer sees the PR; review its - A fork contributor opens a PR. `Spec` runs immediately (public track, no secrets). `Acceptance` does not run. - A CODEOWNER reviews the diff, focusing on the files above, and applies `allowed-for-ci`. The `labeled` event triggers the workflow on `pull_request_target`. A single `approval_gate` job (non-matrix) targets the `puppetcore-fork` environment and pauses for environment approval — the approval prompt shows the head SHA being deployed, which the reviewer should compare against the SHA they reviewed before approving. A `@puppetlabs/modules` member approves; `approval_gate` completes; the matrix `acceptance` jobs then run with `PUPPET_FORGE_TOKEN` in scope. -- If the contributor pushes a new commit, `fork_ci_label_guard.yml` fires on `synchronize` and removes the label. The main caller workflow does **not** include `synchronize` in its `pull_request_target` trigger, so no acceptance run is created for the new commit. To run acceptance against the new code, a CODEOWNER must review again and re-apply the label. -- If the PR is closed, `fork_ci_label_guard.yml` also fires on `closed` and removes the label. This prevents an attack where a contributor closes the PR, pushes new commits (no events fire on a closed PR), then reopens — without close-strip the label would persist across the reopen and the new commits would inherit the prior authorisation. Side effect: closing a merged PR also strips the label, which is cosmetic (the label is only meaningful for open PRs). +- If the contributor pushes a new commit, the guard job fires on `synchronize` and removes the label. Acceptance does not run on that event: for fork PRs it requires the `labeled` event for `allowed-for-ci` itself, not just the label's presence (a `synchronize` payload still carries the label until the guard removes it). The `synchronize` run also cancels that PR's in-flight acceptance through the concurrency group. To run acceptance against the new code, a CODEOWNER must review again and re-apply the label. +- If the PR is closed, the guard job also fires on `closed` and removes the label. This prevents an attack where a contributor closes the PR, pushes new commits (no events fire on a closed PR), then reopens — without close-strip the label would persist across the reopen and the new commits would inherit the prior authorisation. Side effect: closing a merged PR also strips the label, which is cosmetic (the label is only meaningful for open PRs). ## Verification @@ -104,7 +104,8 @@ This is tracked as a follow-up to the initial pilot. The approval gate on the `p ## Security considerations -- Do **not** add `synchronize` to the `pull_request_target` types in the caller's main `ci.yml`. Doing so re-races the label-strip workflow and can allow a malicious commit to inherit a prior label's authorization. +- Gate fork-PR acceptance on the labeling event (`github.event.action == 'labeled' && github.event.label.name == 'allowed-for-ci'`), **never** on `contains(github.event.pull_request.labels.*.name, 'allowed-for-ci')`. A `synchronize` or `reopened` payload still carries a label applied to an earlier commit, so a presence check lets a new, unreviewed commit inherit the prior authorisation. With the event gate, `synchronize` and `closed` can safely trigger `ci.yml` to run the label guard. +- Key the caller's concurrency group on the PR number, not `github.ref`: on `pull_request_target`, `github.ref` is the base branch, so a shared group lets one PR's events cancel another PR's acceptance. - Do **not** pass `secrets: inherit` to the `Spec` track. It runs on `pull_request` (untrusted) and must have no secrets in scope. - Do **not** widen the trigger from `pull_request_target` to include user-controlled refs other than the PR head SHA pinned by the reusable workflow. - The label name `allowed-for-ci` is a contract; changing it requires updating the reusable workflow's `if:` expressions and the label-guard's input. diff --git a/example/module/ci.yml b/example/module/ci.yml index b77f9d72..ea6ca99f 100644 --- a/example/module/ci.yml +++ b/example/module/ci.yml @@ -5,12 +5,12 @@ on: pull_request: branches: - "main" - # Trusted private track: acceptance runs with secrets, gated by label for - # fork PRs. `synchronize` is intentionally omitted to avoid racing with - # fork_ci_label_guard.yml; new commits must be re-labelled by a CODEOWNER - # to re-trigger acceptance. + # Trusted private track: acceptance runs with secrets, gated for fork PRs on + # the `allowed-for-ci` labeling event itself. `synchronize` and `closed` only + # run the label guard below, which strips the label so new commits must be + # re-labelled by a CODEOWNER to re-trigger acceptance. pull_request_target: - types: [labeled, reopened] + types: [labeled, reopened, synchronize, closed] branches: - "main" workflow_dispatch: @@ -19,7 +19,11 @@ permissions: contents: read concurrency: - group: ${{ github.workflow }}-${{ github.ref }} + # On pull_request_target, github.ref is the base branch, so key on the PR number + # (and the event) to keep runs of different PRs from cancelling each other. A new + # commit or a close cancels that PR's running acceptance; adding any other label + # gets its own group so it doesn't. + group: ${{ github.workflow }}-${{ github.event_name }}-${{ github.event.pull_request.number || github.ref }}-${{ github.event.action == 'labeled' && github.event.label.name != 'allowed-for-ci' && github.run_id || 'ci' }} cancel-in-progress: true jobs: @@ -28,11 +32,26 @@ jobs: uses: "puppetlabs/cat-github-actions/.github/workflows/module_ci.yml@main" # No `secrets: inherit` — public track must not see private secrets. + # Fork PRs need the `allowed-for-ci` labeling event, not just the label: a + # `synchronize` payload still carries the label until the guard strips it. Acceptance: if: >- github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request_target' && - (github.event.pull_request.head.repo.fork == false || - contains(github.event.pull_request.labels.*.name, 'allowed-for-ci'))) + ((github.event.pull_request.head.repo.fork == false && + (github.event.action == 'labeled' || github.event.action == 'reopened')) || + (github.event.pull_request.head.repo.fork == true && + github.event.action == 'labeled' && + github.event.label.name == 'allowed-for-ci'))) uses: "puppetlabs/cat-github-actions/.github/workflows/module_acceptance.yml@main" secrets: "inherit" + + # Strips allowed-for-ci on new commits and closes, so each fork-PR acceptance run + # needs a fresh label from a CODEOWNER. + Fork_CI_Label_Guard: + if: >- + github.event_name == 'pull_request_target' && + (github.event.action == 'synchronize' || github.event.action == 'closed') + uses: "puppetlabs/cat-github-actions/.github/workflows/fork_ci_label_guard.yml@main" + permissions: + pull-requests: write diff --git a/example/module/fork_ci_label_guard.yml b/example/module/fork_ci_label_guard.yml index c8a51247..207a2a66 100644 --- a/example/module/fork_ci_label_guard.yml +++ b/example/module/fork_ci_label_guard.yml @@ -1,5 +1,9 @@ name: "Fork CI Label Guard" +# Only needed when the caller's ci.yml doesn't run the guard itself. The +# example ci.yml (and pdk-templates' ci.yml) include it as the +# Fork_CI_Label_Guard job, so a module using either needs no separate file. + # Strips the `allowed-for-ci` label from a fork PR whenever it is updated in # a way that could invalidate the prior CODEOWNER review: # - `synchronize`: a new commit is pushed.