From b8318b6f96913996bb2920937b63feb0af3210b8 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Sun, 6 Sep 2026 13:30:34 -0700 Subject: [PATCH] fix(seidroid-review): refuse a fork-originated pull request on both review paths A review clones the pull request's code into a sandbox that holds a live App credential and a shell. Where the code comes from a fork, someone outside the organisation wrote it. The guard's `Admit the request` step now compares the head and base repository ids and refuses when they differ. ai-review.yml refuses the comment path in the same words, at lines 250-257 of that file. Both paths, not one. An issue_comment run receives this workflow's secrets, so a collaborator's comment on a fork pull request drove a full review. A fork pull_request run does not receive them by default, and the machine-client check fails such a run -- but a private or internal repository can turn that withholding off, per repository or by organisation policy. Gating the automatic path on that setting would rest the posture on something no caller of this file controls. Two sources, one rule. A pull_request payload already carries both ids, so that path spends no API call. An issue_comment payload carries no head repository, so the API answers there. Both fail closed: only a definite match admits, and an absent id refuses rather than comparing equal to another absent id. The check sits before the once-per-PR gate and reads the guard's shared GATE_TOKEN, so it costs one API call on the comment path and none elsewhere. The two differ in posture on purpose: the gate fails open, where this refuses. `Require the machine-client secret` reads the verdict as well as the parse. `deny` exits 0, so every step after it still runs, and a fork pull_request run holds no secret to find. Without the verdict there, a refusal ended the guard red against a caller misconfiguration that does not exist. An admitted run still fails fast. The check exempts a close, keyed on the caller's mode rather than on the command this guard parsed. Two readers derive those two from one comment body and can disagree: the guard accepts a bare `seidroid review close`, where a caller matching the documented `@seidroid` form routes that same comment as a review. `@seidroid review close` on a fork pull request still reclaims its sandbox. The allow-tools description names this refusal in the present tense. The unrestricted shell it accepts now runs only over code from inside the organisation. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 138 ++++++++++++++++++++------ 1 file changed, 105 insertions(+), 33 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index f40cc52..e1b913f 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -13,10 +13,11 @@ run-name: UCI / seidroid review / ${{ (github.event.issue.number || github.event # TWO PATHS, one review. An AUTOMATIC review runs on `pull_request` when the caller # wires that trigger and passes `mode: review`. A MANUAL one runs when a person # comments `@seidroid review`. Both spend model quota and hold a sandbox, so both are -# gated -- see the guard below: the automatic path reviews a pull request once, -# reviews each later push only where the caller sets `re-review-on-push` or a block -# of its own stands, refuses a draft and honours the skip-review label; and the -# manual path additionally checks who is asking. +# gated -- see the guard below. Both paths refuse a fork-originated pull request. The +# automatic path reviews a pull request once. It reviews a later push only where the +# caller sets `re-review-on-push`, or where a block of its own stands. It also refuses +# a draft and honours the skip-review label. The manual path additionally checks who +# is asking. # # This file is the automation of record. It REPLACES `ai-review.yml` rather than # running beside it; a repository that wires the automatic path here should retire @@ -329,14 +330,13 @@ on: that capability. That acceptance covers code from inside the organisation. - Fork code sits outside it, and one path still reaches it. A review runs - on the repository the pull request is on, and GitHub withholds this - workflow's secrets from an automatic fork run. An explicit - `@seidroid review` arrives as an issue_comment in the base repository, - which does carry the secrets. PLT-1156 is the control that refuses such - a request on a fork-originated pull request. It is not in this file yet, - so a member who asks for one runs this shell over fork code. Weigh that - before you widen or narrow this list. + Fork code sits outside that acceptance, and the guard refuses it. An + explicit `@seidroid review` arrives as an issue_comment in the base + repository, which carries the secrets. That path reaches a fork's code + unless something stops it. The guard's fork check is what stops it, on + that path and on the automatic one. This shell therefore runs only over + code from inside the organisation. Weigh that before you widen or narrow + this list. required: false type: string default: 'Bash,Read' @@ -414,21 +414,26 @@ jobs: # Set at all because the account default is six hours. The guard only reads # API state, so a minute is generous. timeout-minutes: 5 - # Read-only, and only what the once-per-PR gate below reads. It makes two - # reads: a review, to find a standing block, on a pull-requests endpoint; and a - # comment, to find a verdict, on the ISSUE comments endpoint. GitHub documents - # that second one as taking either permission, so pull-requests alone serves - # it. issues: read is granted beside it so the read does not rest on that - # alias. A reaction is the stricter case and takes issues alone; the review job - # says so where it needs it. + # Read-only, and only what the guard's checks read. Three reads share it. The + # fork check reads the pull request. The once-per-PR gate reads a review, to find + # a standing block, and a comment, to find a verdict. # - # The gate prefers the App identity and falls back to this, so a caller that - # configures no App still gets one review per pull request rather than one per - # push. That fallback is why a refused read would cost most here: it fails - # open, so a review runs on every push, and only the run log says why. + # That second gate read goes to the ISSUE comments endpoint. GitHub documents it + # as taking either permission, so pull-requests alone serves it. issues: read is + # granted beside it so the read does not rest on that alias. A reaction is the + # stricter case and takes issues alone; the review job says so where it needs it. + # + # Every reader prefers the App identity and falls back to this, so a caller that + # configures no App still gets all three. A refused read costs them differently, + # and both costs are deliberate. The once-per-PR gate fails open: a review runs on + # every push, and only the run log says why. The fork check fails closed: the + # guard refuses the review rather than run it over code it cannot place. + # + # A caller must grant this workflow at least these two, because a reusable + # workflow may only downgrade what its caller granted. permissions: - pull-requests: read # the reviews the gate reads to find a standing block - issues: read # the comments it reads to find a verdict + pull-requests: read # the pull request the fork check reads, and the gate's reviews + issues: read # the comments the gate reads to find a verdict # Runs for an automatic pull_request review, and for any comment-triggered # dispatch, review or close. For a comment it decides whether the commenter may # command this workflow at all; for an automatic review it decides whether the @@ -438,9 +443,8 @@ jobs: # # The pull_request branch carries no author-association check, matching the path # this file replaces: the event is the push itself rather than a person's - # command, and GitHub withholds this workflow's secrets from a fork pull request - # regardless -- such a run fails the machine-client check below and reviews - # nothing, rather than running an agent over unauthorised code. + # command. The fork check in `Admit the request` refuses code from outside the + # organisation, on this path as well as the comment path. if: >- ${{ (github.event_name == 'pull_request' && inputs.mode == 'review') || (github.event_name == 'issue_comment' && @@ -578,11 +582,18 @@ jobs: IS_DRAFT: ${{ github.event.pull_request.draft }} ACTION: ${{ github.event.action }} RE_REVIEW_ON_PUSH: ${{ inputs.re-review-on-push }} + # What the CALLER routed this dispatch as. The fork check below gates on + # this rather than on COMMAND above, and states why. + MODE: ${{ inputs.mode }} + # The pull_request payload's own repository ids, which spare that path an + # API call. Empty on every other event, where the API answers instead. + HEAD_REPO_ID: ${{ github.event.pull_request.head.repo.id }} + BASE_REPO_ID: ${{ github.event.pull_request.base.repo.id }} # The App identity where a caller configured one, this workflow's own - # token where it did not. Named apart from GH_TOKEN above because the - # team check has no such fallback: reading an organisation's teams - # needs an identity that can see them, and GITHUB_TOKEN cannot, where - # reading the comments below needs no more than pull-requests: read. + # token where it did not. Named apart from GH_TOKEN above, because the + # team check has no such fallback. Reading an organisation's teams needs + # an identity that can see them, and GITHUB_TOKEN cannot. The reads below + # need no more than pull-requests: read. GATE_TOKEN: ${{ steps.identity.outputs.token || github.token }} run: | set -uo pipefail @@ -622,6 +633,62 @@ jobs: [ "$state" = "active" ] || deny "$ACTOR is not an active member of $ALLOWED_TEAM; denying" fi + # A fork pull request carries code from outside the organisation. A review + # clones that code into a sandbox. That sandbox holds a live App credential + # and a shell. Both paths refuse it. ai-review.yml refuses the comment path in + # the same words. + # + # The automatic path needs this as much as the comment path. GitHub withholds + # this workflow's secrets from a fork pull_request run by default. The + # machine-client check below then fails such a run. A private or internal + # repository can turn that withholding off, per repository or by organisation + # policy. This check does not rest on a setting nobody here controls. + # + # A pull_request payload carries both repository ids, so that path spends no + # API call. An issue_comment payload carries no head repository, so the API + # answers there. Repository ids, not names, so a rename does not read as a + # fork. A null head repository reads as a fork, which is the safe reading. + # + # This check fails closed, unlike the label check below. Only a definite "same" + # admits, so an unreadable origin refuses. A failed read costs one refused + # review a person can retry. Admitting on a signal nobody could read costs the + # sandbox above. The step runs without -e, so a failed read leaves the variable + # empty. The API's own error goes to the log, where it says why. + # + # It stops a REVIEW, not a teardown, for the reason the label check states. A + # fork pull request must still be able to reclaim its sandbox. + # + # Keyed on the caller's mode, not on the command this guard parsed. Two readers + # derive those two from one comment body, and they can disagree. This guard + # accepts a bare `seidroid review close`. A caller matching the documented + # `@seidroid` form reads that same comment as a review. Mode decides what the + # review job does, so mode is what this gates on. + if [ "$MODE" != "close" ]; then + if [ "$EVENT_NAME" = "pull_request" ]; then + # An empty BASE id means the payload did not carry the signal, which is + # the same standing as a read that failed. It must not compare equal to an + # empty HEAD id and admit. + if [ -z "$BASE_REPO_ID" ]; then + origin=unreadable + elif [ "$HEAD_REPO_ID" = "$BASE_REPO_ID" ]; then + origin=same + else + origin=fork + fi + refusal="$REPO#$PR is fork-originated; not reviewing it" + else + origin="$(GH_TOKEN="$GATE_TOKEN" gh api "repos/$REPO/pulls/$PR" \ + --jq 'if .head.repo.id != null and .head.repo.id == .base.repo.id then "same" else "fork" end' \ + || true)" + refusal="explicit re-reviews are disabled for fork-originated pull requests; not reviewing $REPO#$PR" + fi + case "$origin" in + same) ;; + fork) deny "$refusal" ;; + *) deny "could not read where $REPO#$PR comes from, so a fork cannot be ruled out; not reviewing it" ;; + esac + fi + # The label is a convenience rather than a control, so it fails open: it # stops a review someone did not want, and being unable to read it must # not stop every review when no identity is configured. @@ -740,8 +807,13 @@ jobs: # # Only ever tests emptiness -- the value is never echoed, compared against a # literal, or written to an output. + # + # Reads the verdict as well as the parse. `deny` exits 0, so every step after it + # still runs. A refused request needs no credential, and a fork pull_request run + # holds none: without the verdict here, refusing one would paint a deliberate + # refusal red and name a caller misconfiguration that does not exist. - name: Require the machine-client secret - if: steps.parse.outputs.should_run == 'true' + if: steps.parse.outputs.should_run == 'true' && steps.admit.outputs.admit == 'true' env: SECRET_PRESENT: ${{ secrets.OMNIGENT_MACHINE_CLIENT_SECRET != '' }} run: |