Skip to content

Conversation

@GrantBirki
Copy link
Contributor

@GrantBirkiGrantBirki commented Feb 20, 2025

Fork Operation Safety 🔒

  • If you configure skip_reviews then you can skip reviewing a PR or a PR fork when running an operation with this Action.

This avenue will no longer exist after this PR merges. Forks will be treated with highest level of restrictions when being operated on by this Action. In the past IssueOps operations did not require a review at all, but now they do (for forks).

These changes will not effect normal PRs that don't originate from forks or GitHub issues.

The reasoning behind these changes are to help prevent Actions based TOCTOU vulnerabilities. These vulns exist when a bad actor pushes a commit shortly after a legitimate actor runs an IssueOps command (like .lint). When the IssueOps command goes to operate on the ref of that PR, it checks out the malicious code. For this reason, workflows should only ever use the exact sha during an actions/checkout operation (via IssueOps) and the workflow that runs, should only ever run if the fork PR has been reviewed.

TL;DR

TL;DR: If you are using this Action on pull requests that originate from forks where the rest of your workflow checks out code, ensure that you are requiring PR reviews on your repository via branch protection settings (or rulesets). Also ensure that your workflow checks out code via the exact sha output (from this Action) instead of the ref - documentation.

fork_review_bypass

If your workflow does not checkout code in anyway and it is just doing something like adding labels to a PR, then you don't need reviews on a fork to proceed with the workflow. If this is the case, you can disable these extra safety measures with the fork_review_bypass: "true" input option enabled.

Example

If you have a workflow that you have deemed to be safe, and you want to run it on forks without requiring reviews, you can do so like this:

- uses: github/command@vX.X.X # <-- replace with the latest version of this actionid: pingwith: command: ".foo"allow_forks: true # <-- allows usage on forksskip_reviews: true # <-- allows invocations of .ping to skip the review requirementfork_review_bypass: true # <-- does not enforce the hard requirement of a review on fork PRs

Example: Calling .label if you have an IssueOps command to apply a pre-defined set of labels to a PR fork and you don't make any calls to actions/checkout.


related: github/branch-deploy#331

…all fork PRs without required approvals by default
@GrantBirkiGrantBirki added the enhancement New feature or request label Feb 20, 2025
@GrantBirkiGrantBirki self-assigned this Feb 20, 2025
CopilotAI review requested due to automatic review settings February 20, 2025 20:57
Copy link

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR improves fork operation safety by introducing the new boolean parameter forkReviewBypass to control whether operations on forked pull requests can bypass the standard review requirements.

  • Adds the forkReviewBypass input to the prechecks function and associated configuration files.
  • Updates tests and documentation to reflect the new behavior for forks.

Reviewed Changes

FileDescription
src/functions/prechecks.jsIntroduces forkReviewBypass in function parameters and modifies logic to conditionally bypass review checks on forks.
tests/schemas/action.schema.ymlAdds the fork_review_bypass input; however, the schema definition appears to be inconsistent.
action.ymlAdds the fork_review_bypass input with detailed description regarding the risks involved.
README.mdUpdates documentation with a new table entry for the fork_review_bypass input.
tests/main.test.jsAdds environment variable initialization for forkReviewBypass.
tests/functions/prechecks.test.jsEnhances tests to cover scenarios involving forkReviewBypass.

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/functions/prechecks.js:70

  • [nitpick] The variable 'isFork' is clearly named; however, consider removing explicit '=== true' comparisons in later conditions for cleaner boolean checks.
const isFork = pr?.data?.head?.repo?.fork == true 

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@GrantBirkiGrantBirki merged commit 6417c19 into mainFeb 20, 2025
4 checks passed
@GrantBirkiGrantBirki deleted the fork-safety branch February 20, 2025 21:38
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancementNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@GrantBirki