diff --git a/docs/fork-pr-comments.md b/docs/fork-pr-comments.md index b288173..204fa8a 100644 --- a/docs/fork-pr-comments.md +++ b/docs/fork-pr-comments.md @@ -19,12 +19,15 @@ Everything except the comment. The action does not degrade on a fork PR: So a contributor pushing to a fork already gets the red check, the per-finding annotations on their diff, and the whole report in the job summary. The action says so in the log: -``` +```text ::warning::Skipping PR comment: pull requests from forked repositories cannot write comments via the pull_request event (GITHUB_TOKEN is read-only for forks). The findings are in this job's summary and in the annotations on the Files changed tab. ``` +(with `job-summary: false` the message names the annotations alone, since there is no +summary to read.) + The run is **not** failed by this: `pr-comments: true` on a fork PR is a no-op, not an error. ## If you want feedback on the pull request itself @@ -73,6 +76,10 @@ jobs: with: ref: refs/pull/${{ github.event.number }}/merge # the PR's commits fetch-depth: 0 + # Required since checkout v7: without it the step refuses to place + # fork code in a pull_request_target job, and this workflow never + # reaches the action below. + allow-unsafe-pr-checkout: true - uses: commit-check/commit-check-action@v2 with: message: true @@ -81,11 +88,15 @@ jobs: ``` > [!WARNING] -> `pull_request_target` grants a writable token to a workflow whose checkout contains the -> fork's code. commit-check only *reads* commit metadata and never executes the checked-out -> tree, but any other step you add to this job runs with that token. Keep the job to the -> checkout and this action, never cache or build from it, and never expose secrets to it. -> See [GitHub's guidance on `pull_request_target`](https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/). +> This is the "pwn request" shape, and `allow-unsafe-pr-checkout: true` is the switch +> `actions/checkout` makes you flip to acknowledge it. The job runs with the base +> repository's `GITHUB_TOKEN`, its secrets, its cache scope and its runner, and the +> checkout puts the fork's code on that runner. commit-check only *reads* commit metadata +> and never executes the checked-out tree, but any other step you add to this job would. +> Keep the job to exactly these two steps, never build, install or cache from the tree, +> and never expose secrets to it. Read +> [Securely using `pull_request_target`](https://gh.io/securely-using-pull_request_target) +> before turning this on. ## What this page used to describe diff --git a/main.py b/main.py index 23dd4b0..2d6095c 100755 --- a/main.py +++ b/main.py @@ -1477,11 +1477,15 @@ def add_pr_comments(results: list[ScopeResult]) -> int: # the GitHub API will always reject comment writes with 403. # pull_request_target events always have the configured token permissions. if is_fork_pr_with_readonly_token(): + # Name only the surfaces this run actually produced: with + # job-summary: false there is no summary to send the reader to. + where = "in the annotations on the Files changed tab" + if JOB_SUMMARY_ENABLED and GITHUB_STEP_SUMMARY: + where = f"in this job's summary and {where}" msg = ( "Skipping PR comment: pull requests from forked repositories " "cannot write comments via the pull_request event (GITHUB_TOKEN is " - "read-only for forks). The findings are in this job's summary and " - "in the annotations on the Files changed tab. " + f"read-only for forks). The findings are {where}. " "See https://github.com/commit-check/commit-check-action/blob/main/docs/fork-pr-comments.md" ) print(f"::warning::{msg}") diff --git a/main_test.py b/main_test.py index a5a106e..a078b0c 100644 --- a/main_test.py +++ b/main_test.py @@ -1900,6 +1900,37 @@ def test_fork_pr_skips_comment_and_warns(self): self.assertIn("::warning::", printed) self.assertIn("read-only", printed) + def test_fork_pr_warning_names_only_the_surfaces_that_exist(self): + """With job-summary: false the warning must not send the reader to a + summary this run never wrote; the annotations are still named.""" + summary_path = os.path.join(tempfile.mkdtemp(), "summary.txt") + with ( + patch("main.PR_COMMENTS_ENABLED", True), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.is_fork_pr", return_value=True), + patch("main.JOB_SUMMARY_ENABLED", False), + patch("main.GITHUB_STEP_SUMMARY", summary_path), + patch("builtins.print") as mock_print, + ): + rc = main.add_pr_comments([pass_scope()]) + self.assertEqual(rc, 0) + printed = mock_print.call_args[0][0] + self.assertIn("annotations on the Files changed tab", printed) + self.assertNotIn("job's summary", printed) + self.assertFalse(os.path.exists(summary_path)) + + with ( + patch("main.PR_COMMENTS_ENABLED", True), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + patch("main.is_fork_pr", return_value=True), + patch("main.JOB_SUMMARY_ENABLED", True), + patch("main.GITHUB_STEP_SUMMARY", summary_path), + patch("builtins.print") as mock_print, + ): + main.add_pr_comments([pass_scope()]) + printed = mock_print.call_args[0][0] + self.assertIn("in this job's summary and in the annotations", printed) + def test_fork_pr_writes_job_summary_hint(self): summary_path = os.path.join(tempfile.mkdtemp(), "summary.txt") with (