ci: fix CI on fork PRs by moving off pull_request_target - #2754
Conversation
actions/checkout now refuses an explicit fork-PR ref inside a pull_request_target workflow, which broke every fork PR at the checkout step in under 10s. None of the three active jobs consume a secret, so the trusted context bought them nothing. Drops the authorize gate and the ref: override, pins least-privilege permissions, stops persisting git credentials, and replaces the broken cancel-workflow-action with native concurrency. Refs #2385
Same checkout-guard fix as test.yml. Fork and Dependabot PRs run with a read-only GITHUB_TOKEN, so the sticky comment is skipped for them and findings surface via inline annotations plus a new job summary step. Annotations are emitted as workflow log commands rather than API calls, so they keep working without a writable token. Refs #2385
…ead_ref Fork PRs get no repository secrets, so CLOUDFLARE_API_TOKEN was empty and the deploy always failed. The docs build still runs for them, which is the part that can actually regress. Also routes github.head_ref through an env var instead of interpolating it into the shell command, matching the convention react-doctor.yml already uses for BASE_REF.
CI verification status on this PRAll four runs green — the duplicate Proven by this PR (from the
NOT proven by this PR: the React Doctor comment-gating and job-summary steps. This PR only touches Those Risk if the gating expression is wrong: bounded. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe pull request hardens three GitHub Actions workflows by switching PR-triggered jobs to safer execution, disabling persisted checkout credentials, adding concurrency controls, gating fork deployments and comments, and updating related action versions. ChangesWorkflow hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant ActionsCheckout
participant ReactDoctor
participant GitHubReporting
PullRequest->>ActionsCheckout: checkout merge ref without persisted credentials
ActionsCheckout->>ReactDoctor: provide reattached named branch
ReactDoctor->>GitHubReporting: post same-repository comment or append findings to job summary
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docs-deploy.yml:
- Around line 42-45: Update the “Install Wrangler” step in the docs deployment
workflow to use a pinned Wrangler version or install it through the repository’s
lockfile, rather than resolving the latest global release. Preserve the existing
conditional execution and ensure the deploy step invokes the pinned installation
while using the Cloudflare credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f971ee66-220f-4a21-8567-191c40e64c97
📒 Files selected for processing (3)
.github/workflows/docs-deploy.yml.github/workflows/react-doctor.yml.github/workflows/test.yml
The install step hands CLOUDFLARE_API_TOKEN to whatever it pulls, so resolving "latest" at runtime would let a compromised release reach the credential. Pinned to the major rather than an exact version so patch and minor fixes still land — an exact pin goes stale and breaks the deploy when Cloudflare moves their API forward. Kept as a global install on purpose (acb25d0).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/docs-deploy.yml:
- Around line 45-51: Update the Wrangler installation in the deploy step to use
an immutable exact version instead of the floating wrangler@4 range. Preserve
the global npm installation and add the repository’s established automated
dependency-update mechanism or committed lockfile so the pinned version remains
maintainable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f848ed9a-a10e-416f-8564-13679838bb6e
📒 Files selected for processing (1)
.github/workflows/docs-deploy.yml
wrangler@4 still resolved the newest 4.x at runtime, and that package receives CLOUDFLARE_API_TOKEN. Wrangler pins all its own non-optional deps exactly, so an exact pin here yields a deterministic tree. Bumping is manual: Dependabot parses `uses:` refs, not npm names inside `run:` scripts. Accepted trade — a stale wrangler fails loudly at deploy time, a compromised one fails silently. Not made a devDependency of apps/docs: wrangler pulls workerd (~127MB) and CI has no pnpm caching, so all 5 jobs would download it on every PR.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/docs-deploy.yml (1)
38-44: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable checkout credentials before building fork PRs.
In fork-controlled
pull_requestruns,pnpm installandpnpm docs:buildexecute before the deploy guard, while the unconfiguredactions/checkout@v4persistsGITHUB_TOKENin local Git config by default. Setpersist-credentials: falseon checkout to remove token access from these steps.Proposed fix
- name: ⬇️ Checkout repo uses: actions/checkout@v4 + with: + persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/docs-deploy.yml around lines 38 - 44, Update the workflow’s actions/checkout@v4 step to set persist-credentials to false, ensuring fork pull-request build steps do not retain GITHUB_TOKEN in local Git configuration while leaving the existing build and deployment conditions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/docs-deploy.yml:
- Around line 38-44: Update the workflow’s actions/checkout@v4 step to set
persist-credentials to false, ensuring fork pull-request build steps do not
retain GITHUB_TOKEN in local Git configuration while leaving the existing build
and deployment conditions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f7037fe2-e2cc-4849-8d34-7dc9f3854206
📒 Files selected for processing (1)
.github/workflows/docs-deploy.yml
pnpm install and pnpm docs:build run fork-authored code (lifecycle scripts, vitepress config) before the deploy guard, and checkout writes GITHUB_TOKEN into .git/config by default. Nothing here needs git auth after checkout. Completes the sweep — test.yml and react-doctor.yml already had this; docs-deploy.yml was the third fork-reachable workflow and was missed.
Addressed:
|
| Workflow | Checkouts | persist-credentials: false |
Fork-reachable? |
|---|---|---|---|
test.yml |
3 active | ✅ | Yes |
react-doctor.yml |
1 | ✅ | Yes |
docs-deploy.yml |
1 | ✅ (this commit) | Yes |
build.yml |
1 | ❌ | No — push on main/dev only |
deploy.yml |
2 | ❌ | No — push on main/dev only |
All three fork-reachable workflows are covered. build.yml and deploy.yml are deliberately left alone: they only run on push to main/dev, so no untrusted code ever executes alongside that token. (test.yml:137 also reads as uncovered — that's inside the commented-out Playwright block, inert.)
For calibration: on a fork pull_request the token is read-only and this is a public repo, so the exposure here was low-value. The reason to fix it isn't blast radius — it's that hardening two of three fork-reachable workflows and missing the third is exactly the inconsistency that becomes a real hole the next time someone raises those job permissions.
Closes #2385
Problem
Every PR opened from a fork currently fails all five CI checks. Jobs die at the checkout step in 4–9s, before installing anything:
Example: run 30000187031 (fork
SRF-Consulting-Group-Inc/scim) —authorizepassed after manual approval, then ESLint / TypeScript / Vitest each failed at⬇️ Checkout repo. React Doctor's two matrix legs failed identically. Three fork PRs are blocked on this right now (#2744, #2737, #2717).Root cause
actions/checkoutadded a "pwn request" guard — v7 GA in June 2026, backported to all supported majors (including thev4tag we pin) on 2026-07-20. That date matches our first failures.The guard is narrow. From
src/input-helper.ts:It throws only when the event is
pull_request_target/workflow_runand the head repo is a fork and the resolved ref points at the fork PR head. A default checkout (noref:) is exempt.So our explicit
ref: ${{ github.event.pull_request.head.sha }}is precisely what trips it, and removing it is what makes the config legal again.What changed
test.ymlpull_request_target→pull_request; deletedauthorizejob and 3ref:overrides; replaced 3 brokencancel-workflow-actionsteps with nativeconcurrency; addedpermissions: contents: read,persist-credentials: false, and a SECURITY INVARIANT header. Renamed to🧪 Test.react-doctor.ymlauthorize/ref:/cancel removal; sticky comment gated to writable-token PRs; new job-summary step for every PR.docs-deploy.ymlCLOUDFLARE_API_TOKENso the deploy always failed. Guarded the deploy step (not the job, so forks still build the docs). Also fixed agithub.head_refshell interpolation.workflow_calland itssecrets:block stay intest.yml—deploy.ymlpasses all six secrets, so removing it would breakdeploy.ymlimmediately. E2E gets its own gated workflow when it returns.Security reasoning — please read this part
This removes risk rather than adding it. The instinct is that dropping a gate must be less safe, so it's worth being explicit about why the opposite is true.
Under
on: pull_requestfrom a fork, secrets are not passed to the runner at all — absent, not redacted. Per GitHub's docs: "With the exception ofGITHUB_TOKEN, secrets are not passed to the runner when a workflow is triggered from a forked repository."A contributor adding
console.log(process.env.DATABASE_URL)printsundefined. And sincepull_requestreads the workflow YAML from the merge commit, a fork can edit the workflow to addenv: LEAK: ${{ secrets.DATABASE_URL }}— it evaluates to the empty string. Secrets aren't withheld from the code; they're never handed to the run.We already have proof of this in our own repo.
docs-deploy.ymlruns on plainpull_requestand referencesCLOUDFLARE_API_TOKEN. On fork run 30000188946 the log readsCLOUDFLARE_API_TOKEN:— empty. That job's failure is the protection working.What we have today is the risky configuration. Under
pull_request_target, approved fork code runs in the trusted context, andactions/checkoutdefaultspersist-credentials: true— so the base repo'sGITHUB_TOKENis sitting in.git/configwhile fork code executes in the same job. Any fork-controlled code running afterward (apnpm installlifecycle script, a modified vitest config) can read it. Inreact-doctor.ymlthat token carriespull-requests: write.The structural problem is worse than the current concrete exposure: we're one careless PR away from a real leak. Someone adds
env: DATABASE_URL: ${{ secrets.DATABASE_URL }}to the vitest job to make an integration test pass, and every fork PR silently exfiltrates the production database URL — no error, no review signal. After this change that same mistake is inert.This also drops fork PRs from the default-branch cache scope to a PR-scoped one. Nothing here uses
actions/cachetoday, so there's no active poisoning vector — but the trusted-context exposure goes away regardless.Hardening included
persist-credentials: falseon all four PR checkouts — the token stops being written to disk, closing that path independently of which trigger is in use.permissions: contents: readintest.yml— the repo default is alreadyread, but pinning it means a future org-level default change can't silently escalate these jobs.test.ymlstating the rule: any job consuming a secret must never check out fork code — it belongs in a separate workflow behind theexternalenvironment.Residual risk, stated honestly
A fork PR can still execute arbitrary code on an ephemeral runner and consume Actions minutes — bounded by our
first_time_contributorsapproval policy — and run lifecycle scripts via a modified lockfile, identical to the risk of any dependency and equally true today. It cannot read a secret, write to the repo, poison the default-branch cache, or publish anything.Side effects worth knowing
authorizecheck disappears. Verified safe: branch protection has no required status checks (contexts: [],checks: []) and the only ruleset is Copilot review.first_time_contributors, so only genuinely new contributors need an approval click — down from every fork run today. That's the original ask in [Feature request]: Run CI validations without authorization gate for external PRs #2385.Expect duplicate checks on this PR only
mainstill haspull_request_targetregistered, so its old definition fires alongside this branch's newpull_requestone — two⬣ ESLint, twoʦ TypeScript, etc., with identical names. Both should pass (same-repo PRs are exempt from the guard). This disappears on merge and is not a defect.Verification
actionlintclean on all three changed files. Repo-wide findings went 5 → 4; the 4 remaining are pre-existing outdated action versions indeploy.yml(push-only, not fork-reachable), left alone to keep this PR scoped.deploy.ymlstill resolves against the rewritten reusable workflow (noworkflow_callerrors).waitingruns, then close/reopen fix: allow reducing an oversubscribed QUANTITY_TRACKED booking #2744 and confirm all five checks go green with zero approval clicks.No contributor needs to rebase —
pull_requestreads its workflow from the merge commit, so this applies to already-open fork PRs on their next event.On #2385
The direction there was right, but the snippet couldn't be merged as written:
pull_request_targetregistered with theref:override, so the checkout guard still fires. The issue predates the guard (filed 2026-03-02; guard landed 2026-07-20), so it addressed the approval friction, not the break.needs:is not an expression context.needs: ${{ ... && 'authorize' || '' }}is resolved when the dependency graph is built, before expressions evaluate.if:guard.react-doctor.ymldidn't exist yet (added 2026-04-28), so half the affected surface wasn't covered.Thanks @iuryeng for filing it — your #2744 is one of the PRs this unblocks.
Summary by CodeRabbit
pull_requesttriggers.