Skip to content

Fix claude-code-review.yml trust gate: check PR author, not head repo owner - #82

Open
jnasbyupgrade wants to merge 1 commit into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix-claude-review-trust-gate
Open

Fix claude-code-review.yml trust gate: check PR author, not head repo owner#82
jnasbyupgrade wants to merge 1 commit into
Postgres-Extensions:masterfrom
jnasbyupgrade:fix-claude-review-trust-gate

Conversation

@jnasbyupgrade

Copy link
Copy Markdown
Contributor

The claude-review job's trust gate checked:

github.event.pull_request.head.repo.owner.login == 'jnasbyupgrade'

head.repo.owner.login only identifies "who owns the fork" for fork-headed PRs. For an upstream-branch-headed PR (base and head both in this repo -- required by gh stack, and also just what you get from gh pr create without a fork), head.repo.owner.login is always the repo's own org, never the actual PR author -- so the gate silently skipped review on every such PR regardless of who opened it.

Fix: check the PR author instead:

github.event.pull_request.user.login == 'jnasbyupgrade'

PR author can't be spoofed by a third party any more than head repo owner can, and it's the more direct question for this gate's actual purpose (trusting the PERSON asking for review, not the repository their branch happens to live in). Works for both fork-headed and upstream-branch-headed PRs.

Also updated the SECURITY-CRITICAL comment above the condition to describe the author check instead of the old, now-wrong head-owner field.

… owner

head.repo.owner.login only identifies who owns the fork on fork-headed PRs.
For an upstream-branch-headed PR (base and head both in this repo -- e.g.
from gh stack, or gh pr create without a fork), it's always this repo's own
org, never the actual author, so the gate silently skipped review on every
such PR regardless of who opened it. Check pull_request.user.login instead,
which identifies the actual PR author in both cases.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a3f0bf86-07dc-427f-b92b-659ef9ac6d15

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 1m 49s —— View job


Code Review

  • Gather context (PR diff, CLAUDE.md)
  • Review the trust-gate fix in .github/workflows/claude-code-review.yml
  • Check comment accuracy and security implications
  • Post final review

Verdict: correct, minimal, safe to merge.

The fix itself is sound. github.event.pull_request.user.login is the actual PR author and, per the GitHub Actions webhook payload docs, it's set by GitHub from the account that created the PR — not attacker-controllable, and unlike head.repo.owner.login it's populated correctly for both fork-headed and same-repo/upstream-branch-headed PRs (gh stack, gh pr create without a fork). That's exactly the bug described in the PR body: the old check compared the fork owner field, which for a same-repo PR is always this org, never the actual opener — so review silently never ran on same-repo PRs regardless of author. Good catch.

A couple of observations, neither blocking:

  1. .github/workflows/CLAUDE.md calls out that this exact file can't be verified by its own PR ("GitHub always executes the workflow FILE for a pull_request_target event from the BASE branch... a PR that changes that file cannot prove the change took effect by watching that PR's own claude-review check"). So this PR's own claude-review run (if any) is still exercising the old master-branch logic, not this diff. Worth confirming this fix actually took effect by watching the claude-review check on the next same-repo-branch PR after this merges — not something to fix now, just a heads-up for verification.

  2. Author-gate vs. push-gate is unchanged (not a regression here): the check gates on who opened the PR, not who can push commits to it afterward. For a same-repo-headed PR, anyone with write access to the repo could push a synchronize commit under the trusted author's PR and still trigger the secret-bearing job. This is pre-existing behavior (the old fork-owner check had the same class of gap for fork collaborators) and not introduced by this change — flagging only for awareness, since it's implicit in the "the more direct question for this gate's actual purpose" framing in the PR description. Given write access already implies broad trust in this repo, this is likely fine as-is.

The updated SECURITY-CRITICAL comment block accurately reflects the new check, and no other file references the old head.repo.owner.login/owner-based check (claude.yml's github.actor == 'jnasbyupgrade' gate is for a different trigger type — comment/issue events — and correctly uses the actor field already).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant