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.

Adjust the GitHub ok-to-test e2e to assert the untrusted comment path
without waiting for MinNumberStatus=1. That wait only revalidated the
original pull request status and did not prove that the issue_comment
event triggered anything. The updated test now verifies that an
untrusted same-repo issue_comment does not create a new PipelineRun or
Repository status and emits the expected debug log.

Fixes #2664

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

Author: chmouel

pkg/provider/github/acl.go

@@ -204,15 +204,18 @@ 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
 		}
+	} else if rev.PullRequestNumber != 0 && v.Logger != nil {
+		v.Logger.Debugf(
+			"Skipping same-repo pull request shortcut for untrusted event %T on %s/%s#%d from sender %s",
+			rev.Event, rev.Organization, rev.Repository, rev.PullRequestNumber, rev.Sender,
+		)
 	}
 
 	// If the user who has submitted the pr is a owner on the repo then allows
@@ -238,13 +241,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 {
@@ -765,3 +816,35 @@ func TestIfPullRequestIsForSameRepoWithoutFork(t *testing.T) {
 		})
 	}
 }
+
+func TestACLCheckAllIssueCommentLogsShortcutSkip(t *testing.T) {
+	fakeclient, _, _, teardown := ghtesthelper.SetupGH()
+	defer teardown()
+
+	ctx, _ := rtesting.SetupFakeContext(t)
+	repo := &v1alpha1.Repository{Spec: v1alpha1.RepositorySpec{
+		Settings: &v1alpha1.Settings{},
+	}}
+	observer, logs := zapobserver.New(zap.DebugLevel)
+
+	gprovider := Provider{
+		ghClient: fakeclient,
+		repo:     repo,
+		Logger:   zap.New(observer).Sugar(),
+	}
+
+	allowed, err := gprovider.aclCheckAll(ctx, &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{},
+	})
+	assert.NilError(t, err)
+	assert.Equal(t, false, allowed)
+	assert.Assert(t, logs.FilterMessageSnippet("Skipping same-repo pull request shortcut for untrusted event").Len() == 1)
+}

test/github_pullrequest_oktotest_test.go

@@ -7,16 +7,19 @@ import (
 	"fmt"
 	"net/http"
 	"os"
+	"regexp"
 	"strconv"
 	"testing"
+	"time"
 
 	"github.com/google/go-github/v84/github"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
+	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/cctx"
 	tgithub "github.com/openshift-pipelines/pipelines-as-code/test/pkg/github"
 	"github.com/openshift-pipelines/pipelines-as-code/test/pkg/payload"
 	twait "github.com/openshift-pipelines/pipelines-as-code/test/pkg/wait"
 	"gotest.tools/v3/assert"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -41,60 +44,80 @@ func TestGithubGHEPullRequestOkToTest(t *testing.T) {
 		Organization:  g.Options.Organization,
 		Repository:    g.Options.Repo,
 		URL:           repoinfo.GetHTMLURL(),
-		Sender:        g.Options.Organization,
 	}
 
-	installID, err := strconv.ParseInt(os.Getenv("TEST_GITHUB_SECOND_APPLICATION_ID"), 10, 64)
+	repo, err := g.Cnx.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(g.TargetNamespace).Get(ctx, g.TargetNamespace, metav1.GetOptions{})
+	assert.NilError(t, err)
+	initialStatusCount := len(repo.Status)
+
+	pruns, err := g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{
+		LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA),
+	})
+	assert.NilError(t, err)
+	initialPipelineRunCount := len(pruns.Items)
+
+	installID, err := strconv.ParseInt(os.Getenv("TEST_GITHUB_SECOND_REPO_INSTALLATION_ID"), 10, 64)
 	assert.NilError(t, err)
-	event := github.IssueCommentEvent{
-		Comment: &github.IssueComment{
-			Body: github.Ptr(`/ok-to-test`),
-		},
-		Installation: &github.Installation{
-			ID: &installID,
-		},
-		Action: github.Ptr("created"),
-		Issue: &github.Issue{
-			State: github.Ptr("open"),
-			PullRequestLinks: &github.PullRequestLinks{
-				HTMLURL: github.Ptr(fmt.Sprintf("%s/pull/%d", runevent.URL, g.PRNumber)),
+
+	sendIssueComment := func(t *testing.T, sender string) {
+		t.Helper()
+
+		event := github.IssueCommentEvent{
+			Comment: &github.IssueComment{
+				Body: github.Ptr(`/ok-to-test`),
+			},
+			Installation: &github.Installation{
+				ID: &installID,
+			},
+			Action: github.Ptr("created"),
+			Issue: &github.Issue{
+				State: github.Ptr("open"),
+				PullRequestLinks: &github.PullRequestLinks{
+					HTMLURL: github.Ptr(fmt.Sprintf("%s/pull/%d", runevent.URL, g.PRNumber)),
+				},
 			},
-		},
-		Repo: &github.Repository{
-			DefaultBranch: &runevent.DefaultBranch,
-			HTMLURL:       &runevent.URL,
-			Name:          &runevent.Repository,
-			Owner:         &github.User{Login: &runevent.Organization},
-		},
-		Sender: &github.User{
-			Login: &runevent.Sender,
-		},
+			Repo: &github.Repository{
+				DefaultBranch: &runevent.DefaultBranch,
+				HTMLURL:       &runevent.URL,
+				Name:          &runevent.Repository,
+				Owner:         &github.User{Login: &runevent.Organization},
+			},
+			Sender: &github.User{
+				Login: github.Ptr(sender),
+			},
+		}
+
+		err = payload.Send(ctx,
+			g.Cnx,
+			os.Getenv("TEST_GITHUB_SECOND_EL_URL"),
+			os.Getenv("TEST_GITHUB_SECOND_WEBHOOK_SECRET"),
+			os.Getenv("TEST_GITHUB_SECOND_API_URL"),
+			os.Getenv("TEST_GITHUB_SECOND_REPO_INSTALLATION_ID"),
+			event,
+			"issue_comment",
+		)
+		assert.NilError(t, err)
 	}
 
-	err = payload.Send(ctx,
-		g.Cnx,
-		os.Getenv("TEST_GITHUB_SECOND_EL_URL"),
-		os.Getenv("TEST_GITHUB_SECOND_WEBHOOK_SECRET"),
-		os.Getenv("TEST_GITHUB_SECOND_API_URL"),
-		os.Getenv("TEST_GITHUB_SECOND_APPLICATION_ID"),
-		event,
-		"issue_comment",
-	)
+	g.Cnx.Clients.Log.Infof("Sending /ok-to-test from untrusted sender on same-repo pull request")
+	sendIssueComment(t, "nonowner")
+
+	time.Sleep(10 * time.Second)
+
+	pruns, err = g.Cnx.Clients.Tekton.TektonV1().PipelineRuns(g.TargetNamespace).List(ctx, metav1.ListOptions{
+		LabelSelector: fmt.Sprintf("%s=%s", keys.SHA, g.SHA),
+	})
 	assert.NilError(t, err)
+	assert.Equal(t, initialPipelineRunCount, len(pruns.Items), "untrusted issue_comment must not create a new PipelineRun")
 
-	g.Cnx.Clients.Log.Infof("Wait for the second repository update to be updated")
-	waitOpts := twait.Opts{
-		RepoName:        g.TargetNamespace,
-		Namespace:       g.TargetNamespace,
-		MinNumberStatus: 1,
-		PollTimeout:     twait.DefaultTimeout,
-		TargetSHA:       g.SHA,
-	}
-	_, err = twait.UntilRepositoryUpdated(ctx, g.Cnx.Clients, waitOpts)
+	repo, err = g.Cnx.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(g.TargetNamespace).Get(ctx, g.TargetNamespace, metav1.GetOptions{})
 	assert.NilError(t, err)
+	assert.Equal(t, initialStatusCount, len(repo.Status), "untrusted issue_comment must not add a new Repository status")
 
-	g.Cnx.Clients.Log.Infof("Check if we have the repository set as succeeded")
-	repo, err := g.Cnx.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(g.TargetNamespace).Get(ctx, g.TargetNamespace, metav1.GetOptions{})
+	ctx, err = cctx.GetControllerCtxInfo(ctx, g.Cnx)
+	assert.NilError(t, err)
+	numLines := int64(1000)
+	logRegex := regexp.MustCompile(`Skipping same-repo pull request shortcut for untrusted event \*github\.IssueCommentEvent`)
+	err = twait.RegexpMatchingInControllerLog(ctx, g.Cnx, *logRegex, 10, "ghe-controller", &numLines, nil)
 	assert.NilError(t, err)
-	assert.Assert(t, repo.Status[len(repo.Status)-1].Conditions[0].Status == corev1.ConditionTrue)
 }