From 6f8c4f9db54b666e714761cff4e7e0b8f5881dfa Mon Sep 17 00:00:00 2001 From: Binay Date: Tue, 18 Aug 2026 14:47:01 -0400 Subject: [PATCH 1/2] 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 The Reviewer workflow runs on `pull_request` and requested `pull-requests: write` to post its advisory comment. On a PR from a fork GitHub downgrades GITHUB_TOKEN to read-only regardless of the `permissions:` block, so the write 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 received the advisory comment it was supposed to get. Split into the standard trusted/untrusted pair: * reviewer.yml (untrusted, runs PR code) now holds no write permission at all. It stages the rendered comment plus the PR number and uploads them as an artifact. * reviewer-comment.yml (trusted) runs on `workflow_run` from the default branch, where the token is writable, and posts the comment. 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 — not a trade this repo should make. The PR number travels inside the artifact because `workflow_run.pull_requests` is empty for fork PRs, and is filtered to digits on both write and read. The poster only updates a comment whose marker it finds on a Bot-authored comment, so a contributor cannot get the bot to overwrite a human's comment by planting the marker. --- .github/workflows/reviewer-comment.yml | 96 ++++++++++++++++++++++++++ .github/workflows/reviewer.yml | 57 ++++++++++----- 2 files changed, 134 insertions(+), 19 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 d57f7bf..1aa70a9 100644 --- a/.github/workflows/reviewer.yml +++ b/.github/workflows/reviewer.yml @@ -5,12 +5,23 @@ name: Reviewer # recommendation. Advisory only — it never merges and does not block the PR # (the self-admission + test checks remain the gates). It escalates changes to # security-sensitive surfaces (workflows, release, packaging) to a human. +# +# 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 # post/update the advisory review comment checks: read # cross-verify the self-admission check status jobs: @@ -76,23 +87,31 @@ jobs: --json-out "$OUT/review.json" exit 0 # advisory: never fail the PR from this job - - 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() - uses: actions/github-script@v9 + 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/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 From 3d779835f1c4be2e6705f53d7ceddb2188c07cfc Mon Sep 17 00:00:00 2001 From: Binay Date: Tue, 18 Aug 2026 14:54:12 -0400 Subject: [PATCH 2/2] fix(ci): advisory reviewer step could fail the PR despite 'exit 0' GitHub runs `run:` steps as `bash -e`, so `set -uo pipefail` never disabled errexit. A non-zero exit from signetry-reviewer (any Block verdict) aborted the step immediately and the trailing `exit 0 # advisory: never fail the PR from this job` was unreachable. The workflow header promises the job 'never merges and does not block the PR (the self-admission + test checks remain the gates)', but a Block verdict turned the check red. Add `set +e` and an explicit `|| true` so the verdict lands in the advisory comment, not in the check status. Found while testing the fork-PR comment fix: this PR touches .github/workflows/**, the reviewer returned Block, and the step died at the reviewer invocation rather than reaching its own exit 0. --- .github/workflows/reviewer.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/reviewer.yml b/.github/workflows/reviewer.yml index 1aa70a9..aa01756 100644 --- a/.github/workflows/reviewer.yml +++ b/.github/workflows/reviewer.yml @@ -76,6 +76,12 @@ jobs: REQ: ${{ steps.check.outputs.status }} 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 — + # which made this "advisory" job fail the PR on any Block verdict, + # exactly what the header says it must never do. `set +e` plus an + # explicit `|| true` keeps the verdict in the comment, not the check. + set +e set -uo pipefail signetry-reviewer review \ --diff "$OUT/pr.diff" \ @@ -84,7 +90,7 @@ jobs: --required-check "$REQ" \ --protected ".github/workflows/*,.github/actions/**,pyproject.toml,signetry_core/pipeline/receipt.py,signetry_core/pipeline/gates.py,signetry_core/pipeline/verifier.py,signetry_core/pipeline/contract.py,signetry_core/pipeline/trust_boundary.py" \ --comment-out "$OUT/comment.md" \ - --json-out "$OUT/review.json" + --json-out "$OUT/review.json" || true exit 0 # advisory: never fail the PR from this job # Hand the rendered comment to the trusted workflow. This job cannot post