Просмотр исходного кода

fix(testing): check out PR head in fork e2e instead of main (#6647)

* fix(testing): check out PR head in fork e2e instead of main

The fork e2e path (repository_dispatch: ok-to-test-command) checked out
`${{ env.TARGET_SHA || github.sha }}` where TARGET_SHA came only from the
`/ok-to-test sha=<sha>` argument. On a repository_dispatch, github.sha is the
default branch head, so a bare `/ok-to-test` with no sha= built and tested main
and then reported the result on the pull request, a false green. Prefer an
explicit sha= when provided, otherwise use client_payload.pull_request.head.sha
(the PR head captured when the comment was made), matching e2e-managed.yml. Also
drop the unused GHCR_USERNAME env; the reusable pipeline only loads images into
kind and never pushes them.

Signed-off-by: Alexander Chernov <alexander@chernov.it>

* fix(testing): resolve fork e2e target from explicit sha or review commit

A bare `/ok-to-test` comment left TARGET_SHA empty and the checkout fell
back to github.sha (the default branch on repository_dispatch), so the
fork PR was never tested and report-fork posted a false pass/fail. Using
the live PR head is no better: it is not the commit the maintainer
reviewed and it moves on every later push.

Require an explicit, pinned source for the tested commit via two paths:

- Comment: accept `/ok-to-test sha=<sha>` only. A new guard-fork job
  rejects a bare comment (it posts a short PR comment and fails) before
  any checkout, with no fallback to the PR head or main.
- Review: a new ok-to-test-review.yml runs e2e against
  github.event.review.commit_id when a maintainer submits an Approve or
  Comment review whose body contains /ok-to-test. commit_id is the exact
  reviewed commit and does not move on later pushes. The workflow checks
  the reviewer's write access (pull_request_review is not maintainer-only)
  and emits the same ok-to-test-command payload the comment path uses.

report-fork now skips cleanly when integration-fork was skipped, so a
rejected comment does not post an empty-sha result. Also drop the unused
GHCR_USERNAME env; the reusable pipeline only loads images into kind.

Signed-off-by: Alexander Chernov <alexander@chernov.it>

* fix(testing): scope create-github-app-token to least privilege

zizmor's github-app audit (high) flagged the fork-e2e app tokens as
inheriting blanket installation access for the whole owner. Scope each
token to the current repository and only the permissions it uses:
issues + pull-requests write for the PR-comment tokens (guard-fork and
report-fork), and contents write plus metadata read for the review
dispatcher (repository_dispatch, plus reading the reviewer's collaborator
permission level).

Signed-off-by: Alexander Chernov <alexander@chernov.it>

* fix(testing): harden review-triggered ok-to-test

Address findings from an independent review of the review-trigger path:

- Match /ok-to-test only when it is the command on the first line of the
  review body, mirroring slash-command-dispatch. A mention in prose no
  longer fires a secret-bearing run, and the untrusted body is no longer
  echoed to the log (workflow-command injection).
- guard-fork now requires a full 40-char commit SHA, so a bare or
  malformed sha= (branch name, short SHA, flag) is rejected instead of
  reaching the checkout ref.
- Drop the unused pull_request.head.sha from the review dispatch payload;
  only the fields e2e.yml actually reads remain.
- Note that the collaborator .permission field is already rolled up
  (Maintain surfaces as write), so the check accepts admin/write.

Signed-off-by: Alexander Chernov <alexander@chernov.it>

---------

Signed-off-by: Alexander Chernov <alexander@chernov.it>
Alexander Chernov 1 неделя назад
Родитель
Сommit
81c44958ce

+ 5 - 0
.github/workflows/e2e-reusable.yml

@@ -93,6 +93,11 @@ env:
   KIND_VERSION: 'v0.30.0'
   KIND_IMAGE: 'kindest/node:v1.33.4'
   AWS_REGION: "eu-central-1"
+  # SHA under test on the fork path. Populated by a dispatcher: an explicit
+  # `/ok-to-test sha=<sha>` comment (ok-to-test.yml), or the reviewed commit_id
+  # of a PR review carrying /ok-to-test (ok-to-test-review.yml). Empty on the
+  # trusted pull_request path, where the checkout falls back to github.sha (the
+  # PR merge ref).
   TARGET_SHA: ${{ github.event.client_payload.slash_command.args.named.sha }}
   # Ephemeral tag: images are only ever loaded into kind, never pushed, so a
   # fixed tag keeps the build and test jobs in sync without passing a version.

+ 71 - 3
.github/workflows/e2e.yml

@@ -12,8 +12,13 @@ env:
   # Common versions
   KIND_VERSION: 'v0.30.0'
   KIND_IMAGE: 'kindest/node:v1.33.4'
+  # SHA under test on the fork path. Two dispatchers populate it: an explicit
+  # `/ok-to-test sha=<sha>` comment (ok-to-test.yml) or a PR review whose body
+  # carries /ok-to-test (ok-to-test-review.yml, pinned to the reviewed
+  # commit_id). A bare comment leaves it empty and guard-fork rejects the run
+  # rather than testing the PR head or main. Also empty on the trusted
+  # pull_request path, where the checkout falls back to github.sha.
   TARGET_SHA: ${{ github.event.client_payload.slash_command.args.named.sha }}
-  GHCR_USERNAME: ${{ github.actor }}
   AWS_REGION: "eu-central-1"
 
 jobs:
@@ -66,11 +71,71 @@ jobs:
       ORACLE_FINGERPRINT: ${{ secrets.ORACLE_FINGERPRINT }}
       ORACLE_KEY: ${{ secrets.ORACLE_KEY }}
 
-  # Repo owner has commented /ok-to-test on a (fork-based) pull request.
+  # A /ok-to-test *comment* must carry an explicit sha=. A bare comment has no
+  # trustworthy pinned commit (the dispatch would otherwise read the live PR
+  # head, which a later push can move), so reject it here instead of testing the
+  # wrong code. Reviews carry commit_id and arrive already pinned via
+  # ok-to-test-review.yml, so this guard never fires for them.
+  guard-fork:
+    if: github.event_name == 'repository_dispatch'
+    runs-on: ubuntu-latest
+    permissions:
+      pull-requests: write # to explain a rejected /ok-to-test on the PR
+      contents: read
+    steps:
+    - uses: step-security/harden-runner@9af89fc71515a100421586dfdb3dc9c984fbf411 # v2.19.4
+      with:
+        egress-policy: audit
+
+    # Require a full 40-char commit SHA. This rejects a bare /ok-to-test (empty
+    # TARGET_SHA) and a malformed sha= (e.g. a branch name like `main`, a short
+    # SHA, or a flag), so the checkout ref is always a pinned commit. The review
+    # path supplies review.commit_id, which is always a full SHA.
+    - name: Validate the target sha
+      id: check
+      run: |
+        if printf '%s' "$TARGET_SHA" | grep -qE '^[0-9a-fA-F]{40}$'; then
+          echo "valid=true" >> "$GITHUB_OUTPUT"
+          echo "target sha $TARGET_SHA"
+        else
+          echo "valid=false" >> "$GITHUB_OUTPUT"
+        fi
+
+    - name: Generate token
+      id: create_token
+      if: steps.check.outputs.valid == 'false'
+      uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0
+      env:
+        APP_ID: ${{ secrets.APP_ID }}
+      with:
+        app-id: ${{ env.APP_ID }}
+        private-key: ${{ secrets.PRIVATE_KEY }}
+        owner: ${{ github.repository_owner }}
+        repositories: ${{ github.event.repository.name }}
+        permission-issues: write
+        permission-pull-requests: write
+
+    - name: Explain the rejection on the PR
+      if: steps.check.outputs.valid == 'false'
+      uses: peter-evans/create-or-update-comment@e8674b075228eee787fea43ef493e45ece1004c9 # v5.0.0
+      with:
+        token: ${{ steps.create_token.outputs.token }}
+        issue-number: ${{ github.event.client_payload.pull_request.number }}
+        body: |
+            [Bot] - :warning: A `/ok-to-test` comment needs an explicit full commit SHA: `/ok-to-test sha=<40-char-sha>`. Alternatively submit a PR review (Approve or Comment) whose body contains `/ok-to-test` to test the exact commit you reviewed.
+
+    - name: Fail without a valid target sha
+      if: steps.check.outputs.valid == 'false'
+      run: |
+        echo "::error::/ok-to-test needs sha=<full 40-char commit SHA>; refusing to fall back to the PR head or main"
+        exit 1
+
+  # Repo owner approved a fork PR via /ok-to-test (comment with sha= or review).
   integration-fork:
     permissions:
       id-token: write # for oidc auth with aws/gcp/azure
       contents: read  # for checkout
+    needs: guard-fork
     if: github.event_name == 'repository_dispatch'
     uses: ./.github/workflows/e2e-reusable.yml
     secrets:
@@ -116,7 +181,7 @@ jobs:
 
   # Report the fork run result back onto the originating pull request.
   report-fork:
-    if: github.event_name == 'repository_dispatch' && always()
+    if: ${{ always() && github.event_name == 'repository_dispatch' && needs.integration-fork.result != 'skipped' }}
     needs: integration-fork
     runs-on: ubuntu-latest
     permissions:
@@ -135,6 +200,9 @@ jobs:
         app-id: ${{ env.APP_ID }}
         private-key: ${{ secrets.PRIVATE_KEY }}
         owner: ${{ github.repository_owner }}
+        repositories: ${{ github.event.repository.name }}
+        permission-issues: write
+        permission-pull-requests: write
 
     - name: Update on Success
       if: needs.integration-fork.result == 'success'

+ 105 - 0
.github/workflows/ok-to-test-review.yml

@@ -0,0 +1,105 @@
+# If someone with write access submits a PR review (Approve or Comment) whose
+# body contains "/ok-to-test", emit a repository_dispatch pinned to the exact
+# commit that was reviewed (github.event.review.commit_id). A review records the
+# head the reviewer actually saw, so the tested SHA cannot be moved by a later
+# push, and no PR-head lookup or sha= argument is needed.
+name: Ok To Test (review)
+
+on:
+  pull_request_review:
+    types: [submitted]
+
+permissions:
+  contents: read
+
+jobs:
+  ok-to-test-review:
+    # Only Approve or Comment reviews (never Request changes), and only when the
+    # review body mentions the command. contains() is a coarse pre-filter; the
+    # step below enforces that /ok-to-test is the command on the first line.
+    if: >-
+      (github.event.review.state == 'approved' ||
+       github.event.review.state == 'commented') &&
+      contains(github.event.review.body, '/ok-to-test')
+    runs-on: ubuntu-latest
+    steps:
+    - uses: step-security/harden-runner@9af89fc71515a100421586dfdb3dc9c984fbf411 # v2.19.4
+      with:
+        egress-policy: audit
+
+    # Mirror how slash-command-dispatch parses the comment path: the command
+    # must be on the FIRST line and start with the slash command. This rejects a
+    # /ok-to-test mentioned in prose elsewhere in a longer review, and the
+    # /ok-to-test-managed prefix. Do not echo the body back: it is attacker
+    # controlled and could inject `::workflow commands::` into the log.
+    - name: Match the command in the review body
+      id: cmd
+      env:
+        REVIEW_BODY: ${{ github.event.review.body }}
+      run: |
+        first=$(printf '%s' "$REVIEW_BODY" | head -n1 | tr -d '\r' \
+          | sed -E 's/^[[:space:]]+//; s/[[:space:]]+$//')
+        if printf '%s' "$first" | grep -qE '^/ok-to-test([[:space:]]|$)'; then
+          echo "match=true" >> "$GITHUB_OUTPUT"
+        else
+          echo "match=false" >> "$GITHUB_OUTPUT"
+          echo "first line is not an /ok-to-test command; no e2e triggered"
+        fi
+
+    # App installation token, reused for the permission check and the dispatch.
+    - name: Generate token
+      id: generate_token
+      if: steps.cmd.outputs.match == 'true'
+      uses: actions/create-github-app-token@bcd2ba49218906704ab6c1aa796996da409d3eb1 # v3.2.0
+      env:
+        APP_ID: ${{ secrets.APP_ID }}
+      with:
+        app-id: ${{ env.APP_ID }}
+        private-key: ${{ secrets.PRIVATE_KEY }}
+        owner: ${{ github.repository_owner }}
+        repositories: ${{ github.event.repository.name }}
+        # contents:write to POST the repository_dispatch; metadata:read (implicit
+        # for any app token, stated for clarity) to read the reviewer's
+        # collaborator permission level.
+        permission-contents: write
+        permission-metadata: read
+
+    # pull_request_review does not restrict to maintainers, and
+    # author_association is not authoritative for repo write access, so verify it
+    # explicitly. This is the equivalent of slash-command-dispatch's
+    # `permission: maintain` on the comment path.
+    - name: Verify the reviewer has write access
+      if: steps.cmd.outputs.match == 'true'
+      env:
+        GH_TOKEN: ${{ steps.generate_token.outputs.token }}
+        REPO: ${{ github.repository }}
+        REVIEWER: ${{ github.event.review.user.login }}
+      run: |
+        perm=$(gh api "repos/${REPO}/collaborators/${REVIEWER}/permission" \
+          --jq '.permission')
+        echo "reviewer ${REVIEWER} permission: ${perm}"
+        # .permission is rolled up to admin/write/read/none (Maintain surfaces
+        # as write, Triage as read); accept write and above.
+        case "${perm}" in
+          admin|write) : ;;
+          *) echo "::error::${REVIEWER} lacks write access (${perm})"; exit 1 ;;
+        esac
+
+    # Emit the client_payload fields e2e.yml consumes on the comment path:
+    # slash_command.args.named.sha (the target SHA) and pull_request.number
+    # (used by report-fork to comment the result). jq quotes both safely.
+    - name: Dispatch ok-to-test-command
+      if: steps.cmd.outputs.match == 'true'
+      env:
+        GH_TOKEN: ${{ steps.generate_token.outputs.token }}
+        REPO: ${{ github.repository }}
+        SHA: ${{ github.event.review.commit_id }}
+        PR_NUMBER: ${{ github.event.pull_request.number }}
+      run: |
+        jq -cn --arg sha "$SHA" --argjson num "$PR_NUMBER" '{
+          event_type: "ok-to-test-command",
+          client_payload: {
+            slash_command: { args: { named: { sha: $sha } } },
+            pull_request: { number: $num }
+          }
+        }' | gh api --method POST "repos/${REPO}/dispatches" --input -