fix: detect fork-sourced workflow_run events before allowing Filecoin upload#588
Conversation
There was a problem hiding this comment.
In case it wasn't clear, below response was generated with AI as I was reviewing the code and asking questions.
LGTM — fix is correct, the centralized evaluateUploadProvenance() cleanly covers both the original pull_request path and the missing workflow_run path from the advisory, fails closed on incomplete provenance, and the reorder of parseInputs('upload') after the blocked-status early-exit in upload.js is a nice defense-in-depth bonus (blocked runs never even read walletPrivateKey).
Approving so we don't widen the exposure window. A few non-blocking notes that can land here or in an immediate follow-up:
1. Load-bearing comment on the single-process invariant (worth capturing before this falls off the radar). The PR description correctly notes "no separate re-check needed in upload.js" because run.mjs runs build → upload in one process and the context is passed in-memory. That's true today, but it's the kind of invariant that would silently break under a future refactor — e.g., splitting build and upload into separate jobs where the uploadStatus signal has to round-trip through an artifact or output (an attacker-influenced channel). Suggest a short comment near the fork-pr-blocked early-exit in upload.js saying explicitly: "this gate is only safe because we share a process with runBuild; do not rehydrate uploadStatus from an external source." Cheap insurance against the next refactor.
2. Add a pull_request_target test case. evaluateUploadProvenance correctly handles pull_request_target (treated the same as pull_request), and that's the right call even though README discourages the trigger — if anyone ignores the advice, the fix still applies. Worth one test asserting pull_request_target from a fork returns { trusted: false } so the contract is locked in.
3. README "Current Limitations" clarity. The new bullets are good. One small thing: a reader who lands on the README without knowing the advisory history won't immediately connect "Fork-sourced workflow_run artifacts are rejected" to the fact that the previously documented two-workflow pattern is a workflow_run-triggered flow. Consider a sentence like "Fork PRs that flow through the two-workflow workflow_run pattern are now also blocked, not just direct pull_request triggers" so it's unambiguous.
Happy to file (2) and (3) as a follow-up post-release if it's easier. (1) I'd lean toward landing here — it's a 2-line code comment and it's preventing the next person from removing the safety net by accident.
Closes FilOzone/security-triage#6
What
Extend upload-action fork detection to cover privileged
workflow_runevents by comparing the workflow run’s head repository with its base repository. Incomplete repository provenance now fails closed.Move upload input parsing after the
fork-pr-blockedcheck so blocked runs cannot validate or use wallet credentials, fund Filecoin Pay, or upload fork-controlled content.Add regression coverage for direct fork PRs, fork-originated workflow runs, trusted same-repository runs, incomplete provenance, build-phase blocking, and early upload exit.
Verification
Notes
build.jsandupload.jsrun in the same process (run.mjs), so the provenance decision isn't re-derived or rehydrated between phases, no separate re-check needed inupload.js.