From c1a10be07e1af2d1af6dfe9dc470a5f2da5a0eae Mon Sep 17 00:00:00 2001
From: Zettat123 <zettat123@gmail.com>
Date: Tue, 11 Jul 2023 14:42:07 +0800
Subject: [PATCH] Fix activity type match in `matchPullRequestEvent` (#25746)
 (#25796)

Backport #25746

Fix #25736
Caused by #24048

Right now we only check the activity type for `pull_request` event when
`types` is specified or there are no `types` and filter. If a workflow
only specifies filters but no `types` like this:
```
on:
  pull_request:
    branches: [main]
```
the workflow will be triggered even if the activity type is not one of
`[opened, reopened, sync]`. We need to check the activity type in this
case.
---
 modules/actions/workflows.go      | 69 ++++++++++++++++---------------
 modules/actions/workflows_test.go | 19 +++++++++
 2 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go
index 723dc38c50..3592d4447e 100644
--- a/modules/actions/workflows.go
+++ b/modules/actions/workflows.go
@@ -321,44 +321,47 @@ func matchIssuesEvent(commit *git.Commit, issuePayload *api.IssuePayload, evt *j
 }
 
 func matchPullRequestEvent(commit *git.Commit, prPayload *api.PullRequestPayload, evt *jobparser.Event) bool {
-	// with no special filter parameters
-	if len(evt.Acts()) == 0 {
+	acts := evt.Acts()
+	activityTypeMatched := false
+	matchTimes := 0
+
+	if vals, ok := acts["types"]; !ok {
 		// defaultly, only pull request `opened`, `reopened` and `synchronized` will trigger workflow
 		// See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request
-		return prPayload.Action == api.HookIssueSynchronized || prPayload.Action == api.HookIssueOpened || prPayload.Action == api.HookIssueReOpened
+		activityTypeMatched = prPayload.Action == api.HookIssueSynchronized || prPayload.Action == api.HookIssueOpened || prPayload.Action == api.HookIssueReOpened
+	} else {
+		// See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request
+		// Actions with the same name:
+		// opened, edited, closed, reopened, assigned, unassigned
+		// Actions need to be converted:
+		// synchronized -> synchronize
+		// label_updated -> labeled
+		// label_cleared -> unlabeled
+		// Unsupported activity types:
+		// converted_to_draft, ready_for_review, locked, unlocked, review_requested, review_request_removed, auto_merge_enabled, auto_merge_disabled
+
+		action := prPayload.Action
+		switch action {
+		case api.HookIssueSynchronized:
+			action = "synchronize"
+		case api.HookIssueLabelUpdated:
+			action = "labeled"
+		case api.HookIssueLabelCleared:
+			action = "unlabeled"
+		}
+		log.Trace("matching pull_request %s with %v", action, vals)
+		for _, val := range vals {
+			if glob.MustCompile(val, '/').Match(string(action)) {
+				activityTypeMatched = true
+				matchTimes++
+				break
+			}
+		}
 	}
 
-	matchTimes := 0
 	// all acts conditions should be satisfied
-	for cond, vals := range evt.Acts() {
+	for cond, vals := range acts {
 		switch cond {
-		case "types":
-			// See https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request
-			// Actions with the same name:
-			// opened, edited, closed, reopened, assigned, unassigned
-			// Actions need to be converted:
-			// synchronized -> synchronize
-			// label_updated -> labeled
-			// label_cleared -> unlabeled
-			// Unsupported activity types:
-			// converted_to_draft, ready_for_review, locked, unlocked, review_requested, review_request_removed, auto_merge_enabled, auto_merge_disabled
-
-			action := prPayload.Action
-			switch action {
-			case api.HookIssueSynchronized:
-				action = "synchronize"
-			case api.HookIssueLabelUpdated:
-				action = "labeled"
-			case api.HookIssueLabelCleared:
-				action = "unlabeled"
-			}
-			log.Trace("matching pull_request %s with %v", action, vals)
-			for _, val := range vals {
-				if glob.MustCompile(val, '/').Match(string(action)) {
-					matchTimes++
-					break
-				}
-			}
 		case "branches":
 			refName := git.RefName(prPayload.PullRequest.Base.Ref)
 			patterns, err := workflowpattern.CompilePatterns(vals...)
@@ -407,7 +410,7 @@ func matchPullRequestEvent(commit *git.Commit, prPayload *api.PullRequestPayload
 			log.Warn("pull request event unsupported condition %q", cond)
 		}
 	}
-	return matchTimes == len(evt.Acts())
+	return activityTypeMatched && matchTimes == len(evt.Acts())
 }
 
 func matchIssueCommentEvent(commit *git.Commit, issueCommentPayload *api.IssueCommentPayload, evt *jobparser.Event) bool {
diff --git a/modules/actions/workflows_test.go b/modules/actions/workflows_test.go
index 6ef5d59942..44281b4161 100644
--- a/modules/actions/workflows_test.go
+++ b/modules/actions/workflows_test.go
@@ -57,6 +57,25 @@ func TestDetectMatched(t *testing.T) {
 			yamlOn:       "on: pull_request",
 			expected:     false,
 		},
+		{
+			desc:         "HookEventPullRequest(pull_request) `closed` action doesn't match GithubEventPullRequest(pull_request) with no activity type",
+			triggedEvent: webhook_module.HookEventPullRequest,
+			payload:      &api.PullRequestPayload{Action: api.HookIssueClosed},
+			yamlOn:       "on: pull_request",
+			expected:     false,
+		},
+		{
+			desc:         "HookEventPullRequest(pull_request) `closed` action doesn't match GithubEventPullRequest(pull_request) with branches",
+			triggedEvent: webhook_module.HookEventPullRequest,
+			payload: &api.PullRequestPayload{
+				Action: api.HookIssueClosed,
+				PullRequest: &api.PullRequest{
+					Base: &api.PRBranchInfo{},
+				},
+			},
+			yamlOn:   "on:\n  pull_request:\n    branches: [main]",
+			expected: false,
+		},
 		{
 			desc:         "HookEventPullRequest(pull_request) `label_updated` action matches githubEventPullRequest(pull_request) with `label` activity type",
 			triggedEvent: webhook_module.HookEventPullRequest,