From ba2ac1dbe4e38395b1d1342f474534726e5dfe8c Mon Sep 17 00:00:00 2001 From: Binay Date: Tue, 18 Aug 2026 16:49:01 -0400 Subject: [PATCH] fix(ci): advisory reviewer could never comment on a fork PR (403) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rolls out Signetry/core#92, verified there end to end before propagating. reviewer.yml runs on `pull_request` and requested `pull-requests: write`. On a PR from a fork GitHub downgrades GITHUB_TOKEN to read-only regardless of the `permissions:` block, so the grant was silently dropped and issues.createComment failed with "Resource not accessible by integration" (403). Every outside contribution showed a red `review` check and never got the advisory comment. Split into the standard trusted/untrusted pair: * reviewer.yml (untrusted, runs PR code) now holds no write permission and uploads the rendered comment + PR number as an artifact. * reviewer-comment.yml (trusted) runs on `workflow_run` from the default branch, where the token is writable, and posts it. It never checks out, builds, or executes PR code. Deliberately not `pull_request_target`: that grants a writable token in the base repo's context, and checking out PR head code under it is an RCE footgun. Also fixes a second bug found while testing the first: the advisory step's trailing `exit 0` had never run, because GitHub invokes `run:` steps as `bash -e`, so any non-zero reviewer exit (a Block verdict) aborted the step first and turned the check red — contradicting the workflow's own "never blocks the PR" contract. Bumps the signetry-reviewer pin to v0.2.0, which carries the fix for CI rules firing on YAML comments (a workflow that documented a risk tripped the rule meant to catch it). --- .github/workflows/reviewer-comment.yml | 96 ++++++++++++++++++++++++++ .github/workflows/reviewer.yml | 65 +++++++++++------ 2 files changed, 140 insertions(+), 21 deletions(-) create mode 100644 .github/workflows/reviewer-comment.yml diff --git a/.github/workflows/reviewer-comment.yml b/.github/workflows/reviewer-comment.yml new file mode 100644 index 0000000..5512b77 --- /dev/null +++ b/.github/workflows/reviewer-comment.yml @@ -0,0 +1,96 @@ +name: Reviewer comment + +# Trusted half of the advisory reviewer (see reviewer.yml). +# +# The `Reviewer` workflow runs against untrusted PR code with a read-only token +# and cannot post its own comment — on a fork PR GitHub downgrades +# GITHUB_TOKEN to read-only, so `pull-requests: write` there is silently +# dropped and the API call 403s. This workflow runs from the default branch in +# the base repo's context, where the token *is* writable, and posts the comment +# the other workflow staged as an artifact. +# +# Security properties that must be preserved: +# * It never checks out, builds, or executes PR code — it only reads two +# files out of an artifact. This is what makes holding a writable token +# here safe, unlike `pull_request_target` + checkout of PR head. +# * The PR number is filtered to digits on both sides before use. +# * The comment body is passed to the API as data (never through a shell or +# `eval`), so a crafted diff cannot escalate beyond comment text. +on: + workflow_run: + workflows: ["Reviewer"] + types: [completed] + +permissions: + contents: read + actions: read # download the artifact from the triggering run + pull-requests: write # post/update the advisory review comment + +jobs: + comment: + runs-on: ubuntu-latest + # Only for PR-triggered runs. Note we do not gate on the triggering run's + # conclusion: the reviewer job exits 0 by design (advisory), and we still + # want a comment when it produced a partial result. + if: github.event.workflow_run.event == 'pull_request' + steps: + - name: Download the staged review comment + id: dl + continue-on-error: true + uses: actions/download-artifact@v4 + with: + name: signetry-reviewer-comment + path: staged + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} + + - name: Post / update the review comment + if: steps.dl.outcome == 'success' + uses: actions/github-script@v9 + with: + script: | + const fs = require('fs'); + + // PR number: digits only. workflow_run.pull_requests is empty for + // forks, so the number is carried in the artifact instead. + let raw = ''; + try { raw = fs.readFileSync('staged/pr-number', 'utf8'); } catch (e) {} + const issue_number = parseInt(String(raw).replace(/\D/g, ''), 10); + if (!Number.isInteger(issue_number) || issue_number <= 0) { + core.info('No usable PR number in the artifact; nothing to post.'); + return; + } + + let body = ''; + try { body = fs.readFileSync('staged/comment.md', 'utf8'); } catch (e) {} + if (!body.trim()) body = 'Signetry Reviewer: no review was produced.'; + + const marker = ''; + // GitHub rejects bodies over 65536 chars. + const LIMIT = 65000; + if (body.length > LIMIT) { + body = body.slice(0, LIMIT) + '\n\n_(truncated)_'; + } + body = marker + '\n' + body; + + const { owner, repo } = context.repo; + const comments = await github.paginate( + github.rest.issues.listComments, + { owner, repo, issue_number, per_page: 100 }, + ); + // Only ever update a comment we authored, so a contributor cannot + // get the bot to overwrite someone else's comment by planting the + // marker in a PR description or comment. + const mine = comments.find( + c => c.body + && c.body.includes(marker) + && c.user + && c.user.type === 'Bot', + ); + if (mine) { + await github.rest.issues.updateComment({ owner, repo, comment_id: mine.id, body }); + core.info(`Updated comment ${mine.id} on #${issue_number}.`); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number, body }); + core.info(`Created a comment on #${issue_number}.`); + } diff --git a/.github/workflows/reviewer.yml b/.github/workflows/reviewer.yml index f781b21..e6d5866 100644 --- a/.github/workflows/reviewer.yml +++ b/.github/workflows/reviewer.yml @@ -4,12 +4,23 @@ name: Reviewer # and posts one recommendation comment. Advisory only — it never merges and never # fails the PR. Changes to security-sensitive surfaces (workflows, packaging) are # escalated to a human. See https://github.com/Signetry/reviewer +# +# This job runs against untrusted PR code, so it deliberately holds NO write +# permission. On a fork PR, GitHub downgrades GITHUB_TOKEN to read-only +# regardless of the `permissions:` block, which is why requesting +# `pull-requests: write` here failed with "Resource not accessible by +# integration" (403) on every outside contribution. The rendered comment is +# handed to the trusted `Reviewer comment` workflow (reviewer-comment.yml) via +# an artifact and posted from there. +# +# We deliberately do NOT use `pull_request_target`: that runs with a writable +# token in the base repo's context, and checking out PR head code under it is a +# remote-code-execution footgun. on: pull_request: permissions: contents: read - pull-requests: write checks: read jobs: @@ -24,7 +35,7 @@ jobs: python-version: "3.12" - name: Install signetry-reviewer # source-available (All Rights Reserved); install from source, not PyPI. - run: pip install "signetry-reviewer @ git+https://github.com/Signetry/reviewer@v0.1.2" + run: pip install "signetry-reviewer @ git+https://github.com/Signetry/reviewer@v0.2.0" - name: Compute the PR diff env: BASE_SHA: ${{ github.event.pull_request.base.sha }} @@ -39,6 +50,9 @@ jobs: env: PR: ${{ github.event.pull_request.number }} run: | + # GitHub runs `run:` steps as `bash -e`, so a non-zero exit from the + # reviewer aborted the step before the trailing `exit 0` could run. + set +e set -uo pipefail signetry-reviewer review \ --diff "$OUT/pr.diff" \ @@ -47,25 +61,34 @@ jobs: --required-check unknown \ --protected ".github/workflows/*,.github/actions/**,action.yml,pyproject.toml" \ --comment-out "$OUT/comment.md" \ - --json-out "$OUT/review.json" + --json-out "$OUT/review.json" || true exit 0 - - name: Post / update the review comment + + # Hand the rendered comment to the trusted workflow. This job cannot post + # it itself: it has no write permission, and on fork PRs it could not be + # granted one. The PR number travels in the artifact because + # `workflow_run.pull_requests` is empty for forks. + - name: Stage the review comment for the trusted poster + if: always() + env: + PR: ${{ github.event.pull_request.number }} + run: | + set -uo pipefail + mkdir -p "${RUNNER_TEMP:-/tmp}/rev-out" + if [ -f "$OUT/comment.md" ]; then + cp "$OUT/comment.md" "${RUNNER_TEMP:-/tmp}/rev-out/comment.md" + else + printf 'Signetry Reviewer: no review was produced.\n' \ + > "${RUNNER_TEMP:-/tmp}/rev-out/comment.md" + fi + # Digits only — this value is read back by the trusted workflow. + printf '%s' "$PR" | tr -cd '0-9' > "${RUNNER_TEMP:-/tmp}/rev-out/pr-number" + + - name: Upload the review comment artifact if: always() - uses: actions/github-script@v9 + uses: actions/upload-artifact@v4 with: - script: | - const fs = require('fs'); const path = require('path'); - const out = process.env.OUT || ''; - let body = 'Signetry Reviewer: no review was produced.'; - try { body = fs.readFileSync(path.join(out, 'comment.md'), 'utf8'); } catch (e) {} - const marker = ''; - body = marker + '\n' + body; - const { owner, repo } = context.repo; - const issue_number = context.issue.number; - const comments = await github.rest.issues.listComments({ owner, repo, issue_number }); - const mine = comments.data.find(c => c.body && c.body.includes(marker)); - if (mine) { - await github.rest.issues.updateComment({ owner, repo, comment_id: mine.id, body }); - } else { - await github.rest.issues.createComment({ owner, repo, issue_number, body }); - } + name: signetry-reviewer-comment + path: ${{ runner.temp }}/rev-out + retention-days: 3 + if-no-files-found: warn