fix restrict same repo ACL perm to trusted context

Avoid granting issue_comment senders trust based solely on same-repo PR shape.

Keep the same-repo fast path for pull_request events and PR rerequest
flows, while forcing comment senders through collaborator,
org-membership, or OWNERS checks.

Add regression coverage to ensure issue_comment events do not bypass ACL
and same-repo check_run/check_suite rerequests continue to work.

Fixes #2664

Co-authored-by: Cursor <cursor@users.noreply.github.com>
Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>

Author: 2 people

pkg/provider/github/acl.go

@@ -204,11 +204,9 @@ func (v *Provider) aclCheckAll(ctx context.Context, rev *info.Event) (bool, erro
 		return true, nil
 	}
 
-	// If the user who has submitted the PR is not a owner or public member or Collaborator or not there in OWNERS file
-	// but has permission to push to branches then allow the CI to be run.
-	// This can only happen with GithubApp and Bots.
-	// Ex: dependabot, bots
-	if rev.PullRequestNumber != 0 {
+	// Allow same-repo pull requests from bots or other non-members when the PR
+	// branch lives in this repository instead of a fork.
+	if v.canUseSameRepoPullRequestShortcut(rev) {
 		isFromSameRepo := v.checkPullRequestForSameURL(ctx, rev)
 		if isFromSameRepo {
 			return true, nil
@@ -238,13 +236,30 @@ func (v *Provider) aclCheckAll(ctx context.Context, rev *info.Event) (bool, erro
 	return v.IsAllowedOwnersFile(ctx, rev)
 }
 
-// checkPullRequestForSameURL checks if a pull request's head and base branches are from the same repository.
-// means if the user has access to create a branch in the repository without forking or having any
-// permissions then PAC should allow to run CI.
+// canUseSameRepoPullRequestShortcut returns true only for event types where
+// the sender is expected to match the pull request author. That keeps the
+// same-repo shortcut available for PR and rerequest flows, but not comments.
+func (v *Provider) canUseSameRepoPullRequestShortcut(rev *info.Event) bool {
+	if rev.PullRequestNumber == 0 {
+		return false
+	}
+
+	switch rev.Event.(type) {
+	case *github.PullRequestEvent, *github.CheckRunEvent, *github.CheckSuiteEvent:
+		return true
+	default:
+		return false
+	}
+}
+
+// checkPullRequestForSameURL returns true when the PR comes from another branch
+// in the same repository instead of from a fork.
 //
-//	ex: dependabot, *[bot] etc...
+// This is the fast path that lets bot-authored PRs such as Dependabot run
+// without needing collaborator, org-member, or OWNERS access.
 //
-// HeadURL is set by getPullRequest() before aclCheckAll; if missing, fall through to other ACL checks.
+// HeadURL is filled by getPullRequest() before aclCheckAll. If it is missing,
+// ACL falls back to the regular membership checks.
 func (v *Provider) checkPullRequestForSameURL(_ context.Context, runevent *info.Event) bool {
 	if runevent.HeadURL == "" {
 		return false

pkg/provider/github/acl_test.go

@@ -708,6 +708,7 @@ func TestIfPullRequestIsForSameRepoWithoutFork(t *testing.T) {
 				BaseURL:           "http://org.com/owner/repo",
 				HeadBranch:        "main",
 				BaseBranch:        "dependabot",
+				Event:             &github.PullRequestEvent{},
 			},
 			pullRequestNumber: 1,
 			allowed:           true,
@@ -719,6 +720,7 @@ func TestIfPullRequestIsForSameRepoWithoutFork(t *testing.T) {
 				Sender:            "nonowner",
 				Repository:        "repo",
 				PullRequestNumber: 1,
+				Event:             &github.PullRequestEvent{},
 			},
 			pullRequestNumber: 1,
 			allowed:           false,
@@ -734,10 +736,59 @@ func TestIfPullRequestIsForSameRepoWithoutFork(t *testing.T) {
 				BaseURL:           "http://org.com/owner/repo1",
 				HeadBranch:        "main",
 				BaseBranch:        "dependabot",
+				Event:             &github.PullRequestEvent{},
 			},
 			pullRequestNumber: 1,
 			allowed:           false,
 			wantError:         false,
+		}, {
+			name: "when issue comment sender is not trusted, same repo shortcut is not applied",
+			event: &info.Event{
+				Organization:      "owner",
+				Sender:            "nonowner",
+				Repository:        "repo",
+				PullRequestNumber: 1,
+				HeadURL:           "http://org.com/owner/repo",
+				BaseURL:           "http://org.com/owner/repo",
+				HeadBranch:        "main",
+				BaseBranch:        "dependabot",
+				Event:             &github.IssueCommentEvent{},
+			},
+			pullRequestNumber: 1,
+			allowed:           false,
+			wantError:         false,
+		}, {
+			name: "when check run rerequest resolves to same repo pull request the shortcut is applied",
+			event: &info.Event{
+				Organization:      "owner",
+				Sender:            "dependabot[bot]",
+				Repository:        "repo",
+				PullRequestNumber: 1,
+				HeadURL:           "http://org.com/owner/repo",
+				BaseURL:           "http://org.com/owner/repo",
+				HeadBranch:        "dependabot/npm-foo",
+				BaseBranch:        "main",
+				Event:             &github.CheckRunEvent{},
+			},
+			pullRequestNumber: 1,
+			allowed:           true,
+			wantError:         false,
+		}, {
+			name: "when check suite rerequest resolves to same repo pull request the shortcut is applied",
+			event: &info.Event{
+				Organization:      "owner",
+				Sender:            "dependabot[bot]",
+				Repository:        "repo",
+				PullRequestNumber: 1,
+				HeadURL:           "http://org.com/owner/repo",
+				BaseURL:           "http://org.com/owner/repo",
+				HeadBranch:        "dependabot/npm-foo",
+				BaseBranch:        "main",
+				Event:             &github.CheckSuiteEvent{},
+			},
+			pullRequestNumber: 1,
+			allowed:           true,
+			wantError:         false,
 		},
 	}
 	for _, tt := range tests {