diff --git a/.gitignore b/.gitignore index 2233054..ab3375b 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,4 @@ venv/ .venv/ __pycache__/ +result.txt diff --git a/README.md b/README.md index fd79fbb..e8ee35a 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,7 @@ A GitHub Action for checking commit message formatting, branch naming, committer * [Optional Inputs](#optional-inputs) * [GitHub Action Job Summary](#github-action-job-summary) * [GitHub Pull Request Comments](#github-pull-request-comments) +* [Fork PR Comments](docs/fork-pr-comments.md) * [Badging Your Repository](#badging-your-repository) * [Versioning](#versioning) @@ -123,7 +124,12 @@ jobs: > [!IMPORTANT] > `pr-comments` is an experimental feature. By default, it's disabled. > -> PR comments are skipped for pull requests from forked repositories. For more details, refer to issue [`#143`](https://github.com/commit-check/commit-check-action/issues/143). +> PR comments are skipped for pull requests from forked repositories. See +> [docs/fork-pr-comments.md](docs/fork-pr-comments.md) for details on how to enable +> this feature for fork contributions. +> +> Note: write-access to pull-requests requires the `pull-requests: write` permission. +> See [usage example](#usage). Note: the default rule of above inputs is following [this configuration](https://github.com/commit-check/commit-check-action/blob/main/commit-check.toml). If you want to customize, just add your [`commit-check.toml`](https://commit-check.github.io/commit-check/configuration.html) config file under your repository root directory. @@ -149,6 +155,23 @@ By default, commit-check-action results are shown on the job summary page of the ![Failure pull request comment](https://github.com/commit-check/.github/blob/main/screenshot/failure-pr-comments.png) +## Fork PR Comments + +When a pull request is opened from a **forked repository**, the `GITHUB_TOKEN` used by the +`pull_request` event has **read-only** permissions by design (GitHub security policy). +This means `pr-comments: true` cannot write a comment back to the PR. + +By default, commit-check-action handles this gracefully: + +- PR comment writing is **skipped** with a `::warning::` message in the logs +- A **notice is added to the Job Summary** explaining why and how to fix it +- The commit checks themselves **still run normally** + +> **For most projects, this is sufficient** — contributors can see check results in the +> action Job Summary. But if you *must* have PR comments on fork contributions, see +> the **[Fork PR Comments](docs/fork-pr-comments.md)** documentation for +> two recommended approaches with ready-to-use workflow examples. + ## Badging Your Repository You can add a badge to your repository to show your contributors/users that you use commit-check! diff --git a/docs/fork-pr-comments.md b/docs/fork-pr-comments.md new file mode 100644 index 0000000..82ff63a --- /dev/null +++ b/docs/fork-pr-comments.md @@ -0,0 +1,166 @@ +# Fork PR Comments + +When a pull request is opened from a **forked repository**, the `GITHUB_TOKEN` used by the +`pull_request` event has **read-only** permissions by design (GitHub security policy). +This means `pr-comments: true` cannot write a comment back to the PR. + +By default, commit-check-action handles this gracefully: + +- PR comment writing is **skipped** with a `::warning::` message in the logs +- A **notice is added to the Job Summary** explaining why and how to fix it +- The commit checks themselves **still run normally** + +> **For most projects, this is sufficient** — contributors can see check results in the +> action Job Summary. But if you *must* have PR comments on fork contributions, there +> are two recommended approaches. + +--- + +## Option 1: Two-workflow pattern (recommended) + +This is the **official GitHub-recommended best practice** for writing PR comments from +fork PRs. It uses the [`workflow_run`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run) +event with **no security risks**. + +> 📁 Ready-to-use files: [`examples/commit-check-workflow-a.yml`](../examples/commit-check-workflow-a.yml) +> and [`examples/commit-check-workflow-b.yml`](../examples/commit-check-workflow-b.yml) + +**How it works:** + +``` + pull_request workflow_run + │ │ + ▼ ▼ +┌──────────────┐ ┌──────────────────┐ +│ Workflow A │ │ Workflow B │ +│ (checks) │────►│ (comment writer) │ +│ │ │ │ +│ Token: READ │ │ Token: WRITE │ +│ Saves result │ │ Reads artifact │ +│ as artifact │ │ Posts PR comment │ +└──────────────┘ └──────────────────┘ +``` + +### Workflow A + +`.github/workflows/commit-check.yml` (triggered by `pull_request`): + +```yaml +name: Commit Check + +on: + pull_request: + branches: ["main"] + +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - uses: commit-check/commit-check-action@v2 + with: + message: true + branch: true + pr-comments: false # comments handled by Workflow B + job-summary: true + - uses: actions/upload-artifact@v4 + with: + name: commit-check-result-${{ github.event.number }} + path: result.txt # saved for Workflow B +``` + +> 📄 Full file: [`examples/commit-check-workflow-a.yml`](../examples/commit-check-workflow-a.yml) + +### Workflow B + +`.github/workflows/commit-check-comment.yml` (triggered by `workflow_run`): + +```yaml +name: Commit Check Comment + +on: + workflow_run: + workflows: ["Commit Check"] # must match Workflow A's name exactly + types: [completed] + +jobs: + comment: + runs-on: ubuntu-latest + permissions: + pull-requests: write + actions: read # needed to download artifacts + steps: + - uses: actions/download-artifact@v4 + with: + name: commit-check-result-${{ github.event.workflow_run.pull_requests[0].number }} + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} + - name: Read result and post PR comment + uses: actions/github-script@v7 + with: + script: | + // See examples/commit-check-workflow-b.yml for full script + const fs = require('fs'); + const prNumber = ${{ github.event.workflow_run.pull_requests[0].number }}; + const resultText = fs.readFileSync('result.txt', 'utf8').trim(); + const body = resultText + ? '# Commit-Check ❌\n```\n' + resultText + '\n```' + : '# Commit-Check ✔️'; + // Creates or updates the matching PR comment +``` + +> 📄 Full file: [`examples/commit-check-workflow-b.yml`](../examples/commit-check-workflow-b.yml) + +### Key security benefits + +- Workflow B runs in the **base repository's context**, so `GITHUB_TOKEN` has full write + permissions (you explicitly grant `pull-requests: write`) +- Workflow B **does not checkout the PR code**, so untrusted fork code never runs + with elevated permissions +- The artifact only contains `result.txt` — no code or secrets + +--- + +## Option 2: pull_request_target (advanced, use with caution) + +If you understand the security implications, you can use +[`pull_request_target`](https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target) +which runs in the base repository's context with **write token access**. + +> **⚠️ Security warning:** Never check out (`actions/checkout`) the PR's HEAD commit +> when using `pull_request_target`. Always check out the base branch or use the +> default merge commit. Otherwise, fork code could exfiltrate your repository's secrets. + +```yaml +name: Commit Check + +on: + pull_request_target: + branches: ["main"] + +jobs: + commit-check: + runs-on: ubuntu-latest + permissions: + contents: read + pull-requests: write + steps: + # SAFE: checkout the merge commit, NOT the PR head + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - uses: commit-check/commit-check-action@v2 + with: + message: true + branch: true + pr-comments: true +``` + +> ✅ With `pull_request_target`, `pr-comments: true` **does work** on fork PRs — +> the token has the workflow's configured permissions regardless of whether the PR +> is from a fork. +> +> **When to use this:** Only if the two-workflow pattern is too complex for your setup +> and you have thoroughly reviewed the security implications. diff --git a/examples/commit-check-workflow-a.yml b/examples/commit-check-workflow-a.yml new file mode 100644 index 0000000..9d1bece --- /dev/null +++ b/examples/commit-check-workflow-a.yml @@ -0,0 +1,33 @@ +# Workflow A: Run commit checks on pull_request events. +# +# This workflow is triggered by pull_request and runs commit checks. +# It uploads the result as an artifact so Workflow B (commit-check-comment.yml) +# can read it and post a PR comment with full write permissions. +# +# See https://github.com/commit-check/commit-check-action#fork-pr-comments + +name: Commit Check + +on: + pull_request: + branches: ["main"] + +jobs: + check: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v5 + with: + fetch-depth: 0 + - uses: commit-check/commit-check-action@v2 + with: + message: true + branch: true + pr-comments: false # comments handled by Workflow B + job-summary: true + + # Save results so Workflow B can post a PR comment + - uses: actions/upload-artifact@v4 + with: + name: commit-check-result-${{ github.event.number }} + path: result.txt diff --git a/examples/commit-check-workflow-b.yml b/examples/commit-check-workflow-b.yml new file mode 100644 index 0000000..09517b8 --- /dev/null +++ b/examples/commit-check-workflow-b.yml @@ -0,0 +1,68 @@ +# Workflow B: Post PR comment after commit checks complete. +# +# This workflow is triggered by the workflow_run event from Workflow A. +# It runs in the base repository's context with full write permissions, +# making it safe for fork PRs (no checkout of fork code). +# +# Prerequisites: +# - Workflow A (commit-check.yml) must exist and upload an artifact named +# commit-check-result- containing result.txt +# +# See https://github.com/commit-check/commit-check-action#fork-pr-comments + +name: Commit Check Comment + +on: + workflow_run: + workflows: ["Commit Check"] # must match Workflow A's name exactly + types: [completed] + +jobs: + comment: + runs-on: ubuntu-latest + permissions: + pull-requests: write + actions: read # needed to download artifacts + steps: + - uses: actions/download-artifact@v4 + with: + name: commit-check-result-${{ github.event.workflow_run.pull_requests[0].number }} + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} + + - name: Read result and post PR comment + uses: actions/github-script@v7 + with: + script: | + const fs = require('fs'); + const prNumber = ${{ github.event.workflow_run.pull_requests[0].number }}; + const resultText = fs.readFileSync('result.txt', 'utf8').trim(); + + const successTitle = '# Commit-Check ✔️'; + const failureTitle = '# Commit-Check ❌'; + const body = resultText + ? `${failureTitle}\n\`\`\`\n${resultText}\n\`\`\`` + : successTitle; + + const { data: comments } = await github.rest.issues.listComments({ + ...context.repo, + issue_number: prNumber, + }); + + const existing = comments.find(c => + c.body.startsWith(successTitle) || c.body.startsWith(failureTitle) + ); + + if (existing) { + await github.rest.issues.updateComment({ + ...context.repo, + comment_id: existing.id, + body, + }); + } else { + await github.rest.issues.createComment({ + ...context.repo, + issue_number: prNumber, + body, + }); + } diff --git a/main.py b/main.py index 7f0f1e2..4814fe7 100755 --- a/main.py +++ b/main.py @@ -263,6 +263,45 @@ def is_fork_pr() -> bool: return False +def is_fork_pr_with_readonly_token() -> bool: + """Returns True when the PR is from a fork AND the event has a read-only token. + + Under the pull_request event, GITHUB_TOKEN is read-only for fork PRs. + Under pull_request_target, GITHUB_TOKEN has the workflow's configured + permissions regardless of whether the PR is from a fork. + """ + return is_fork_pr() and os.getenv("GITHUB_EVENT_NAME", "") != "pull_request_target" + + +def get_pr_number() -> int: + """Extract the pull request number from event payload or GITHUB_REF. + + For pull_request: GITHUB_REF is refs/pull//merge + For pull_request_target: GITHUB_REF is refs/heads/ (not useful), + so we fall back to the event payload. + """ + ref = os.getenv("GITHUB_REF", "") + parts = ref.split("/") + if len(parts) >= 4 and parts[1] == "pull": + return int(parts[2]) + # Fallback: read PR number from event payload + event_path = os.getenv("GITHUB_EVENT_PATH") + if event_path: + try: + with open(event_path, "r") as f: + event = json.load(f) + number = event.get("number") or (event.get("pull_request", {}) or {}).get( + "number" + ) + if number: + return int(number) + except Exception: + pass + raise ValueError( + "Unable to determine PR number from GITHUB_REF or GITHUB_EVENT_PATH" + ) + + def add_pr_comments() -> int: """Posts the commit check result as a comment on the pull request.""" if not PR_COMMENTS_ENABLED: @@ -270,14 +309,28 @@ def add_pr_comments() -> int: # Fork PRs triggered by the pull_request event receive a read-only token; # the GitHub API will always reject comment writes with 403. - if is_fork_pr(): - print( - "::warning::Skipping PR comment: pull requests from forked repositories " + # pull_request_target events always have the configured token permissions. + if is_fork_pr_with_readonly_token(): + msg = ( + "Skipping PR comment: pull requests from forked repositories " "cannot write comments via the pull_request event (GITHUB_TOKEN is " - "read-only for forks). Use the pull_request_target event or the " - "two-workflow artifact pattern instead. " - "See https://github.com/commit-check/commit-check-action/issues/77" + "read-only for forks). " + "See https://github.com/commit-check/commit-check-action/blob/main/docs/fork-pr-comments.md " + "for how to enable PR comments on fork PRs." ) + print(f"::warning::{msg}") + if JOB_SUMMARY_ENABLED: + with open(GITHUB_STEP_SUMMARY, "a") as f: + f.write( + "\n---\n" + "### \u2139\ufe0f PR Comment Skipped\n\n" + "Pull requests from forked repositories cannot write comments " + "using the `pull_request` event because `GITHUB_TOKEN` has " + "read-only permissions.\n\n" + "> **\U0001f4a1 Tip:** To enable PR comments on fork PRs, see " + "[Enabling PR Comments on Fork Pull Requests]" + "(https://github.com/commit-check/commit-check-action/blob/main/docs/fork-pr-comments.md).\n" + ) return 0 try: @@ -285,18 +338,14 @@ def add_pr_comments() -> int: token = os.getenv("GITHUB_TOKEN") repo_name = os.getenv("GITHUB_REPOSITORY") - pr_number = os.getenv("GITHUB_REF") - if pr_number is not None: - pr_number = pr_number.split("/")[-2] - else: - raise ValueError("GITHUB_REF environment variable is not set") + pr_number = get_pr_number() if not token: raise ValueError("GITHUB_TOKEN is not set") g = Github(auth=Auth.Token(token)) repo = g.get_repo(repo_name) - pull_request = repo.get_issue(int(pr_number)) + pull_request = repo.get_issue(pr_number) result_text = read_result_file() pr_comment_body = build_result_body(result_text) diff --git a/main_test.py b/main_test.py index 3a5d04c..00fc594 100644 --- a/main_test.py +++ b/main_test.py @@ -467,6 +467,79 @@ def test_failure_writes_failure_title(self): self.assertIn("bad commit message", content) +class TestAddPrComments(unittest.TestCase): + def setUp(self): + import tempfile + + self._orig_dir = os.getcwd() + self._tmpdir = tempfile.mkdtemp() + os.chdir(self._tmpdir) + with open("result.txt", "w", encoding="utf-8"): + pass + + def tearDown(self): + os.chdir(self._orig_dir) + + def test_disabled_returns_zero(self): + with patch("main.PR_COMMENTS_ENABLED", False): + rc = main.add_pr_comments() + self.assertEqual(rc, 0) + + def test_fork_pr_skips_comment_and_warns(self): + with ( + patch("main.PR_COMMENTS_ENABLED", True), + patch("main.is_fork_pr", return_value=True), + patch("main.JOB_SUMMARY_ENABLED", False), + patch("builtins.print") as mock_print, + ): + rc = main.add_pr_comments() + self.assertEqual(rc, 0) + printed = mock_print.call_args[0][0] + self.assertIn("::warning::", printed) + self.assertIn("read-only", printed) + + def test_fork_pr_writes_job_summary_hint(self): + summary_path = os.path.join(self._tmpdir, "summary.txt") + with ( + patch("main.PR_COMMENTS_ENABLED", True), + patch("main.is_fork_pr", return_value=True), + patch("main.JOB_SUMMARY_ENABLED", True), + patch("main.GITHUB_STEP_SUMMARY", summary_path), + patch("builtins.print"), + ): + rc = main.add_pr_comments() + self.assertEqual(rc, 0) + with open(summary_path, encoding="utf-8") as f: + content = f.read() + self.assertIn("PR Comment Skipped", content) + self.assertIn("read-only", content) + self.assertIn("fork-pr-comments", content) + + +class TestIsForkPrWithReadonlyToken(unittest.TestCase): + def test_fork_pr_with_pull_request_event(self): + with ( + patch("main.is_fork_pr", return_value=True), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + ): + self.assertTrue(main.is_fork_pr_with_readonly_token()) + + def test_fork_pr_with_pull_request_target_event(self): + """pull_request_target has write token — not considered read-only.""" + with ( + patch("main.is_fork_pr", return_value=True), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request_target"}), + ): + self.assertFalse(main.is_fork_pr_with_readonly_token()) + + def test_same_repo_not_fork(self): + with ( + patch("main.is_fork_pr", return_value=False), + patch.dict(os.environ, {"GITHUB_EVENT_NAME": "pull_request"}), + ): + self.assertFalse(main.is_fork_pr_with_readonly_token()) + + class TestIsForkPr(unittest.TestCase): def test_no_event_path(self): with patch.dict(os.environ, {}, clear=True):