Fix GitOps Command Issue on Pushed Commit by Unautorized User

Issue: when an unautorized user sends GitOps comment on a pushed commit, PAC is triggering CI since access check is done only for pull_request event in verifyRepoAndUser func of controller. Solution: added a check for push event and Ops comment event type in verifyRepoAndUser func. https://issues.redhat.com/browse/SRVKP-7110 Signed-off-by: Zaki Shaikh <[email protected]>
openshift-pipelines · Feb 12, 2025 · ee59992 · ee59992
1 parent c1ef894
commit ee59992
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 12 deletions.
diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go
@@ -132,11 +132,31 @@ is that what you want? make sure you use -n when generating the secret, eg: echo
 		return repo, err
 	}
 
+	// Verify whether the sender of the GitOps command (e.g., /test) has the appropriate permissions to
+	// trigger CI on the repository, as any user is able to comment on a pushed commit in open-source repositories.
+	if p.event.TriggerTarget == triggertype.Push && opscomments.IsAnyOpsEventType(p.event.EventType) {
+		status := provider.StatusOpts{
+			Status:     CompletedStatus,
+			Title:      "Permission denied",
+			Conclusion: failureConclusion,
+			DetailsURL: p.event.URL,
+		}
+		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
+			return nil, err
+		}
+	}
+
 	// Check if the submitter is allowed to run this.
 	// on push we don't need to check the policy since the user has pushed to the repo so it has access to it.
 	// on comment we skip it for now, we are going to check later on
 	if p.event.TriggerTarget != triggertype.Push && p.event.EventType != opscomments.NoOpsCommentEventType.String() {
-		if allowed, err := p.checkAccessOrErrror(ctx, repo, "via "+p.event.TriggerTarget.String()); !allowed {
+		status := provider.StatusOpts{
+			Status:     queuedStatus,
+			Title:      "Pending approval, waiting for an /ok-to-test",
+			Conclusion: pendingConclusion,
+			DetailsURL: p.event.URL,
+		}
+		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "via "+p.event.TriggerTarget.String()); !allowed {
 			return nil, err
 		}
 	}
@@ -245,7 +265,13 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep
 	// if the event is a comment event, but we don't have any match from the keys.OnComment then do the ACL checks again
 	// we skipped previously so we can get the match from the event to the pipelineruns
 	if p.event.EventType == opscomments.NoOpsCommentEventType.String() || p.event.EventType == opscomments.OnCommentEventType.String() {
-		if allowed, err := p.checkAccessOrErrror(ctx, repo, "by gitops comment"); !allowed {
+		status := provider.StatusOpts{
+			Status:     queuedStatus,
+			Title:      "Pending approval, waiting for an /ok-to-test",
+			Conclusion: pendingConclusion,
+			DetailsURL: p.event.URL,
+		}
+		if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed {
 			return nil, err
 		}
 	}
@@ -370,26 +396,20 @@ func (p *PacRun) checkNeedUpdate(_ string) (string, bool) {
 	return "", false
 }
 
-func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, viamsg string) (bool, error) {
+func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, status provider.StatusOpts, viamsg string) (bool, error) {
 	allowed, err := p.vcx.IsAllowed(ctx, p.event)
 	if err != nil {
 		return false, err
 	}
 	if allowed {
 		return true, nil
 	}
-	msg := fmt.Sprintf("User %s is not allowed to trigger CI %s on this repo.", p.event.Sender, viamsg)
+	msg := fmt.Sprintf("User %s is not allowed to trigger CI %s in this repo.", p.event.Sender, viamsg)
 	if p.event.AccountID != "" {
-		msg = fmt.Sprintf("User: %s AccountID: %s is not allowed to trigger CI %s on this repo.", p.event.Sender, p.event.AccountID, viamsg)
+		msg = fmt.Sprintf("User: %s AccountID: %s is not allowed to trigger CI %s in this repo.", p.event.Sender, p.event.AccountID, viamsg)
 	}
 	p.eventEmitter.EmitMessage(repo, zap.InfoLevel, "RepositoryPermissionDenied", msg)
-	status := provider.StatusOpts{
-		Status:     queuedStatus,
-		Title:      "Pending approval, needs /ok-to-test",
-		Conclusion: pendingConclusion,
-		Text:       msg,
-		DetailsURL: p.event.URL,
-	}
+	status.Text = msg
 	if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil {
 		return false, fmt.Errorf("failed to run create status, user is not allowed to run the CI:: %w", err)
 	}

diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go
@@ -465,6 +465,24 @@ func TestRun(t *testing.T) {
 			finalStatusText:          "PipelineRun has no taskruns",
 			expectedNumberofCleanups: 10,
 		},
+		{
+			name: "Do not allow unauthorized user to run CI on pushed commit",
+			runevent: info.Event{
+				SHA:           "principale",
+				Organization:  "organizationes",
+				Repository:    "lagaffe",
+				URL:           "https://service/documentation",
+				HeadBranch:    "main",
+				BaseBranch:    "main",
+				Sender:        "fantasio",
+				EventType:     "test-all-comment",
+				TriggerTarget: "push",
+			},
+			tektondir:                    "testdata/push_branch",
+			finalStatus:                  "failure",
+			finalStatusText:              "User fantasio is not allowed to trigger CI by GitOps comment on push commit in this repo.",
+			skipReplyingOrgPublicMembers: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {