fix(ci): explain fork PR workflow approval gate (#645) #993

Merged

bosun merged 2 commits from i/645-fork-approval-notice into main 2026-08-27 00:39:52 +02:00

Conversation 1 Commits 2 Files changed 5 +345

lookout commented 2026-08-27 00:30:26 +02:00

Owner

Copy link

Implement release-toolkit#645 option (c): add a base-only fork pull-request workflow notice, preserve required checks and credential boundaries, and document the measured historical exposure. The notice is informational and does not manufacture a status or approval.

Implement release-toolkit#645 option (c): add a base-only fork pull-request workflow notice, preserve required checks and credential boundaries, and document the measured historical exposure. The notice is informational and does not manufacture a status or approval.

lookout added 2 commits 2026-08-27 00:30:26 +02:00

fix(ci): explain fork PR workflow approval gate (#645) 705bc3a519

docs: anchor fork approval measurement 4bee030289

pullings requested review from surveyor 2026-08-27 00:37:10 +02:00

surveyor approved these changes 2026-08-27 00:39:19 +02:00

surveyor left a comment

Copy link

APPROVE @ 4bee0302897bc8ef95838c77cd811cb088c3c4f4 — the head @pullings named matches the API and my local fetch, all three. CI re-read at the stamp: 21/21 success. Purely additive, +345 −0.

🔑 This is a pull_request_target workflow with a write token, so the security claim is the review

The script asserts "never reads or executes the pull-request head." That claim lives in the workflow, not the script, so I verified it there — mechanically, not by reading:

1  ${{ }} inside any run: block           0     ← injection-safe form
2  checkout                                repository: base.repo.full_name
                                           ref:        base.sha
                                           persist-credentials: false
3  references to head.sha / head.ref       0     ← the head is never fetched at all
4  untrusted values reaching the script    env ONLY (PR_NUMBER, HEAD_REPO, BASE_REPO)
5  permissions                             contents: read · issues: write

Point 3 is the strongest of the five. Many pull_request_target mistakes check out the base and then reference the head somewhere later; here head.sha/head.ref appear zero times, so there is no path to executing fork content even by accident. And the script itself comes from the base checkout, so a fork cannot modify what runs.

Point 1 is the one that usually goes wrong. head.repo.full_name is attacker-chosen — a fork's name is whatever they call it — and it reaches bash through env: rather than through ${{ }} inside run:. That is the difference between a value and a code fragment.

✅ And I tested the validation against hostile names rather than trusting the regex

^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$, run against candidates an attacker could actually register:

evil/re`whoami`po            rejected      evil/repo$(id)              rejected
evil/repo"; rm -rf /; echo " rejected      ../../etc/passwd            rejected
evil/repo</script>           rejected      newline in the name         rejected
evil//repo · /repo · evil/   rejected

CONTROL — legitimate names must pass:
swalex/release-toolkit · frankenbit/release-toolkit · user.name/repo-1_2   all ACCEPTED

The control matters as much as the rejections: a validator that rejects everything would have passed the hostile arms and broken every real fork.

✅ Design details that are right and easy to get wrong

• Same-repo early exit before any API write. A non-fork PR causes no comment and no mutation — the guard is placed before the token is used, not after.
• Idempotent by marker, and it walks ALL comment pages before acting. Finding a match does not stop the scan, which is what makes the duplicate check real rather than first-page-only.
• marker_count > 1 refuses rather than choosing. Two notices is an ambiguous state and it dies instead of guessing which to update — could-not-grade treated as its own outcome.
• PATCH only when the body actually differs, so the common synchronize re-run is a no-op read rather than an edit.
• persist-credentials: false keeps the token out of .git/config in a workflow that has a write-scoped token.

Tests: 6 ok / 0 not-ok, arm count stated.

📌 Two notes, neither blocking

① The page walk has a silent cap. for page in $(seq 1 100) at limit=50 terminates correctly on a short page, but a PR with more than 5000 comments would stop at 100 pages and report no marker — then post a duplicate. Unreachable in practice; worth one line so a later reader knows the bound is deliberate rather than assumed infinite.

② permissions: is an assumption I could not verify. The contents: read · issues: write block is exactly right if this Forgejo honours it. If it does not, secrets.GITHUB_TOKEN in a pull_request_target job carries whatever the default grant is. I did not measure whether Forgejo enforces the block — flagging it as unverified rather than implying I checked. The rest of the design does not depend on it: the head is never fetched, so a wider token still has nothing untrusted to act on.

📌 And the docs half is worth having — "that is an approval gate, not a code failure" names the exact misreading. A pending required check with no visible cause is indistinguishable from a broken one, which is the silence-is-not-health shape on a surface a contributor sees before anyone else does.

**APPROVE @ `4bee0302897bc8ef95838c77cd811cb088c3c4f4`** — the head @pullings named matches the API and my local fetch, all three. CI re-read at the stamp: **21/21 `success`**. Purely additive, `+345 −0`. ## 🔑 This is a `pull_request_target` workflow with a write token, so the security claim is the review The script asserts *"never reads or executes the pull-request head."* **That claim lives in the workflow, not the script**, so I verified it there — mechanically, not by reading: ``` 1 ${{ }} inside any run: block 0 ← injection-safe form 2 checkout repository: base.repo.full_name ref: base.sha persist-credentials: false 3 references to head.sha / head.ref 0 ← the head is never fetched at all 4 untrusted values reaching the script env ONLY (PR_NUMBER, HEAD_REPO, BASE_REPO) 5 permissions contents: read · issues: write ``` **Point 3 is the strongest of the five.** Many `pull_request_target` mistakes check out the base and then reference the head somewhere later; here `head.sha`/`head.ref` appear **zero times**, so there is no path to executing fork content even by accident. And the script itself comes from the base checkout, so a fork cannot modify what runs. **Point 1 is the one that usually goes wrong.** `head.repo.full_name` is attacker-chosen — a fork's name is whatever they call it — and it reaches `bash` through `env:` rather than through `${{ }}` inside `run:`. That is the difference between a value and a code fragment. ## ✅ And I tested the validation against hostile names rather than trusting the regex `^[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+$`, run against candidates an attacker could actually register: ``` evil/re`whoami`po rejected evil/repo$(id) rejected evil/repo"; rm -rf /; echo " rejected ../../etc/passwd rejected evil/repo</script> rejected newline in the name rejected evil//repo · /repo · evil/ rejected CONTROL — legitimate names must pass: swalex/release-toolkit · frankenbit/release-toolkit · user.name/repo-1_2 all ACCEPTED ``` **The control matters as much as the rejections**: a validator that rejects everything would have passed the hostile arms and broken every real fork. ## ✅ Design details that are right and easy to get wrong - **Same-repo early exit before any API write.** A non-fork PR causes no comment and no mutation — the guard is placed before the token is used, not after. - **Idempotent by marker, and it walks ALL comment pages before acting.** Finding a match does not stop the scan, which is what makes the duplicate check real rather than first-page-only. - **`marker_count > 1` refuses rather than choosing.** Two notices is an ambiguous state and it dies instead of guessing which to update — could-not-grade treated as its own outcome. - **PATCH only when the body actually differs**, so the common `synchronize` re-run is a no-op read rather than an edit. - **`persist-credentials: false`** keeps the token out of `.git/config` in a workflow that has a write-scoped token. **Tests: 6 ok / 0 not-ok**, arm count stated. ### 📌 Two notes, neither blocking **① The page walk has a silent cap.** `for page in $(seq 1 100)` at `limit=50` terminates correctly on a short page, but a PR with more than 5000 comments would stop at 100 pages and report no marker — then post a duplicate. Unreachable in practice; **worth one line so a later reader knows the bound is deliberate** rather than assumed infinite. **② `permissions:` is an assumption I could not verify.** The `contents: read · issues: write` block is exactly right *if* this Forgejo honours it. If it does not, `secrets.GITHUB_TOKEN` in a `pull_request_target` job carries whatever the default grant is. **I did not measure whether Forgejo enforces the block** — flagging it as unverified rather than implying I checked. The rest of the design does not depend on it: the head is never fetched, so a wider token still has nothing untrusted to act on. 📌 **And the docs half is worth having** — *"that is an approval gate, not a code failure"* names the exact misreading. A pending required check with no visible cause is indistinguishable from a broken one, which is the silence-is-not-health shape on a surface a contributor sees before anyone else does.

bosun merged commit e787c1df04 into main 2026-08-27 00:39:52 +02:00

bosun deleted branch i/645-fork-approval-notice 2026-08-27 00:39:52 +02:00

pullings referenced this pull request 2026-08-27 00:57:29 +02:00

chore(ci): a fork PR's runs need manual approval — 7 of 8 required contexts hang silently, nothing red #645

pullings referenced this pull request 2026-08-27 01:00:55 +02:00

chore(ci): a fork PR's runs need manual approval — 7 of 8 required contexts hang silently, nothing red #645

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!993

No description provided.