Skip to content

fix(privacy): resolve PR identity from full objects in workflow_run gate#3454

Merged
marcusrbrown merged 2 commits into
mainfrom
fix/private-leak-pr-resolution
Jun 5, 2026
Merged

fix(privacy): resolve PR identity from full objects in workflow_run gate#3454
marcusrbrown merged 2 commits into
mainfrom
fix/private-leak-pr-resolution

Conversation

@marcusrbrown

Copy link
Copy Markdown
Collaborator

The per-PR private-leak gate failed closed on every legitimate PR. The PR objects inside the workflow_run.pull_requests[] payload are abbreviated — they carry base.repo.name but not base.repo.full_name, which the identity validator requires. Every candidate was rejected, so the gate blocked all PRs.

The fix treats pull_requests[] entries as hints: it extracts candidate PR numbers, fetches each full PR object by number, and validates that complete object (which carries the base repo identity and head SHA). The head-SHA fallback already used full objects; this aligns the fast path with it. Validation still requires exactly one matching PR and fails closed otherwise.

Found by a live test PR that exercised the merged gate end-to-end.

…workflow_run entries

The workflow_run.pull_requests[] entries are abbreviated and omit base.repo.full_name,
so validating them directly rejected every legitimate PR and the gate failed closed on
all of them. Extract candidate PR numbers from those entries as hints, then fetch the
full PR object by number and validate that — the shape that actually carries the base
repo identity. The head-SHA fallback already used full objects; this aligns the fast path.
fro-bot
fro-bot previously approved these changes Jun 5, 2026

@fro-bot fro-bot left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Verdict: PASS

The root cause analysis is accurate. GitHub's workflow_run.pull_requests[] entries are documented to be abbreviated — they carry base.repo.name / id / url but omit full_name. The old code called validatePrIdentity (which requires base.repo.full_name) directly on those abbreviated objects, producing 0 valid candidates on every invocation and failing closed. The fix correctly decouples number extraction from validation.

Blocking issues

None.

Non-blocking concerns

scripts/check-private-leak.ts:332 — misleading error message in head-SHA fallback

The throw inside the fallback reads:

if (nums.length === 0) {
  throw new Error(
    `check-private-leak: head-SHA fallback returned ${nums.length} valid PR(s), expected exactly 1 — fail-closed`,
  )
}

Two problems: (1) nums.length is always 0 in that branch — the interpolation adds no information. (2) "valid PR(s)" is wrong; these aren't validated yet, only numbered. A tighter message:

throw new Error('check-private-leak: head-SHA fallback found no PR numbers — fail-closed')

scripts/check-private-leak.ts:361 — double-fetch of the validated PR

The full PR object is fetched once per candidate in Step 3 and then fetched a second time unconditionally for title/author extraction. The comment acknowledges this. For a script that fires in CI this is fine — one extra API call per run — but if fetchPrByNumber ever grows caching or side-effects the duplication could matter. Not blocking; call it out for the next refactor pass.

Missing tests

None. The regression lock suite covers:

  • abbreviated objects resolving successfully (the live bug)
  • mutation check (verifies old behavior would have failed)
  • wrong base repo via full-object validation
  • wrong branch via full-object validation
  • SHA mismatch via full-object validation
  • multiple candidates (both pull_requests[] and head-SHA fallback paths)

The happy-path test and all error paths are updated to use the abbreviated shape that GitHub actually sends. Coverage is solid.

Risk assessment: LOW

The gate remains fail-closed: any result set other than exactly one validated PR throws and exits 1. The new code adds API calls (one fetchPrByNumber per candidate number) but abbreviated payloads in practice contain one entry. The double-fetch is a minor inefficiency, not a correctness or security issue. The fix closes a real production bug that silently blocked every legitimate PR.


Run Summary
Field Value
Event pull_request
Repository fro-bot/.github
Run ID 27004466004
Cache hit
Session ses_169167625ffeTSfFtSzIN1Bk1W

@fro-bot fro-bot left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Second commit addresses the path-specific error message concern from the prior review. The usedHeadShaFallback flag routes the exactly-one guard to the correct message depending on which resolution path fired.

The remaining non-blocking item — the early-exit message at line ~334 still interpolates ${nums.length} which is always 0 in that branch, and labels them "valid PR(s)" when they haven't been validated yet — is a cosmetic issue. The message is factually accurate (zero entries were found), just redundant. Not blocking.

Double-fetch stands as documented. Also not blocking.

Verdict: PASS

Blocking issues

None.

Non-blocking concerns

scripts/check-private-leak.ts:334 — early-exit error message still has redundant interpolation

throw new Error(
  `check-private-leak: head-SHA fallback returned ${nums.length} valid PR(s), expected exactly 1 — fail-closed`,
)

nums.length is always 0 here (that's the condition). The phrase "valid PR(s)" is inaccurate (no validation has occurred). Could be 'check-private-leak: head-SHA fallback found no PR numbers — fail-closed'. Cosmetic; the gate behavior is correct.

scripts/check-private-leak.ts:361 — double-fetch

Acknowledged in the prior review and in the code comment. Still present, still not blocking.

Missing tests

None.

Risk assessment: LOW

Gate is fail-closed on all paths. The second commit correctly differentiates error messages by resolution path. Two cosmetic items remain open for a future pass.


Run Summary
Field Value
Event pull_request
Repository fro-bot/.github
Run ID 27004659515
Cache hit
Session ses_169167625ffeTSfFtSzIN1Bk1W

@marcusrbrown marcusrbrown merged commit d816631 into main Jun 5, 2026
13 of 14 checks passed
@marcusrbrown marcusrbrown deleted the fix/private-leak-pr-resolution branch June 5, 2026 15:18
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.

2 participants