approve: fix silent approval bypass when PR exceeds GitHub file list API limit#707
Open
jmguzik wants to merge 1 commit intokubernetes-sigs:mainfrom
Open
approve: fix silent approval bypass when PR exceeds GitHub file list API limit#707jmguzik wants to merge 1 commit intokubernetes-sigs:mainfrom
jmguzik wants to merge 1 commit intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for k8s-prow ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
|
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jmguzik The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
area/plugins
k8s-ci-robot
added
cncf-cla: no
…API limit GitHub's "List pull request files" REST API endpoint returns at most 3000 files across all pages. When a pull request exceeds this limit, GitHub silently stops returning further pages. Prow's GetPullRequestChanges follows pagination via Link headers, so it terminates normally without any indication that the result is incomplete. This creates a security vulnerability: a contributor who has OWNERS approval rights over an alphabetically-early directory can craft a PR with more than 3000 file changes in directories they own, then append additional changes in directories they do not own. Because the approve plugin only evaluates the files returned by the API, the hidden files are never checked against OWNERS, and the PR can receive the approved label without proper review coverage. This was observed in practice against openshift/hypershift#8355. Fix: 1. Add ChangedFiles to the PullRequest struct so that the value GitHub returns on the PR object itself (which is always accurate, unlike the paginated file list) can be compared against the number of files the API actually returned. 2. Introduce a getFilenames helper that implements a layered strategy: - Fast path: use GetPullRequestChanges (works for <=3000 files). - Fallback: if the file list is shorter than PR.ChangedFiles, call ListPullRequestCommits and GetSingleCommit for each commit (up to maxCommitsForFallback=10) and union the filenames. This handles the common case of large auto-generated PRs that touch many files across a small number of commits. - Give up: if the PR has more than 10 commits, or if the per-commit file lists are also truncated, declare truncation. 3. When truncation cannot be resolved: - Post an [ApprovalNotifier] comment explaining why automated approval is blocked, with the specific reason (too many commits, or individual commits also truncated). The comment is deduplicated against the latest existing notifier comment to avoid spam. - Remove the approved label if present and it was not added manually by a human (preserving manual administrator overrides). If the WasLabelAddedByHuman API call fails, label state is preserved rather than risking removal of a legitimate manual override. - Return nil so the hook handler does not log a spurious error. For PRs within the limit the behaviour is unchanged: the fast path returns immediately after the first API call. Co-authored with Cursor. Signed-off-by: Jakub Guzik <jguzik@redhat.com>
jmguzik
force-pushed
the
global-trusted-apps
branch
from
c3553c3 to
decefa0
Compare
k8s-ci-robot
added
cncf-cla: yes
Prucek
reviewed
May 5, 2026
| seen[f] = struct{}{} | ||
| } | ||
| for _, c := range commits { | ||
| commit, err := ghc.GetSingleCommit(org, repo, c.SHA) |
Member
There was a problem hiding this comment.
Doesn't this API call have the same limitation?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GitHub's "List pull request files" REST API endpoint returns at most 3000 files across all pages. When a pull request exceeds this limit, GitHub silently stops returning further pages. Prow's GetPullRequestChanges follows pagination via Link headers, so it terminates normally without any indication that the result is incomplete.
This creates a security vulnerability: a contributor who has OWNERS approval rights over an alphabetically-early directory can craft a PR with more than 3000 file changes in directories they own, then append additional changes in directories they do not own. Because the approve plugin only evaluates the files returned by the API, the hidden files are never checked against OWNERS, and the PR can receive the approved label without proper review coverage. This was observed in practice against openshift/hypershift#8355.
Fix:
Add ChangedFiles to the PullRequest struct so that the value GitHub returns on the PR object itself (which is always accurate, unlike the paginated file list) can be compared against the number of files the API actually returned.
Introduce a getFilenames helper that implements a layered strategy:
When truncation cannot be resolved:
For PRs within the limit the behaviour is unchanged: the fast path returns immediately after the first API call.