fix(actions): harden log cursor, activity type and actor filter handling (#39721)

Fixes a few Actions bugs where user input or nondeterminism caused
failures:

- The job log view indexed task steps with the client-supplied cursor
step without a bounds check, so an out-of-range value panicked and
returned a 500.
- Workflow activity type filters (`types:`) were compiled with
`glob.MustCompile`, so an invalid pattern in a workflow file panicked
during event detection. Invalid patterns now simply don't match.
- Listing workflow runs with an unknown `actor` returned a 500; it now
returns 404.
- The v4 artifact `ListArtifacts` response was built by ranging over a
map, so its order changed between calls. It now follows database order.
- The error from marshalling a workflow job when inserting run jobs was
silently ignored.

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

models/git/protected_branch.go

@@ -99,8 +99,8 @@ func (protectBranch *ProtectedBranch) loadGlob() {
 	var err error
 	protectBranch.globRule, err = glob.Compile(protectBranch.RuleName, '/')
 	if err != nil {
-		log.Warn("Invalid glob rule for ProtectedBranch[%d]: %s %v", protectBranch.ID, protectBranch.RuleName, err)
-		protectBranch.globRule = glob.MustCompile(glob.QuoteMeta(protectBranch.RuleName), '/')
+		log.Debug("Invalid glob rule for ProtectedBranch[%d]: %s %v", protectBranch.ID, protectBranch.RuleName, err)
+		protectBranch.globRule, _ = glob.Compile(glob.QuoteMeta(protectBranch.RuleName), '/')
 	}
 }
 
@@ -110,7 +110,9 @@ func (protectBranch *ProtectedBranch) Match(branchName string) bool {
 	if protectBranch.isPlainName {
 		return strings.EqualFold(protectBranch.RuleName, branchName)
 	}
-
+	if protectBranch.globRule == nil {
+		return false // in case of invalid glob rule, we don't match anything
+	}
 	return protectBranch.globRule.Match(branchName)
 }
 

models/git/protected_branch_list.go

@@ -9,6 +9,7 @@ import (
 
 	"gitea.dev/models/db"
 	"gitea.dev/modules/glob"
+	"gitea.dev/modules/log"
 	"gitea.dev/modules/optional"
 )
 
@@ -55,10 +56,17 @@ func FindRepoProtectedBranchRules(ctx context.Context, repoID int64) (ProtectedB
 // FindAllMatchedBranches find all matched branches
 func FindAllMatchedBranches(ctx context.Context, repoID int64, ruleName string) ([]string, error) {
 	results := make([]string, 0, 10)
+	rule, err := glob.Compile(ruleName)
+	if err != nil {
+		log.Debug("Failed to compile rule %s: %v", ruleName, err)
+		return results, nil
+	}
+
+	const pageSize = 100
 	for page := 1; ; page++ {
-		brancheNames, err := FindBranchNames(ctx, FindBranchOptions{
+		branchNames, err := FindBranchNames(ctx, FindBranchOptions{
 			ListOptions: db.ListOptions{
-				PageSize: 100,
+				PageSize: pageSize,
 				Page:     page,
 			},
 			RepoID:          repoID,
@@ -67,14 +75,12 @@ func FindAllMatchedBranches(ctx context.Context, repoID int64, ruleName string)
 		if err != nil {
 			return nil, err
 		}
-		rule := glob.MustCompile(ruleName)
-
-		for _, branch := range brancheNames {
+		for _, branch := range branchNames {
 			if rule.Match(branch) {
 				results = append(results, branch)
 			}
 		}
-		if len(brancheNames) < 100 {
+		if len(branchNames) < pageSize {
 			break
 		}
 	}

modules/actions/workflows.go

@@ -532,7 +532,7 @@ func matchIssuesEvent(issuePayload *api.IssuePayload, evt *jobparser.Event) bool
 			}
 
 			for _, val := range vals {
-				if slices.ContainsFunc(actions, glob.MustCompile(val, '/').Match) {
+				if matchActivityType(val, actions...) {
 					matchTimes++
 					break
 				}
@@ -575,7 +575,7 @@ func matchPullRequestEvent(ctx context.Context, gitRepo *git.Repository, commit
 		}
 		log.Trace("matching pull_request %s with %v", action, vals)
 		for _, val := range vals {
-			if glob.MustCompile(val, '/').Match(string(action)) {
+			if matchActivityType(val, string(action)) {
 				activityTypeMatched = true
 				matchTimes++
 				break
@@ -678,7 +678,7 @@ func matchIssueCommentEvent(issueCommentPayload *api.IssueCommentPayload, evt *j
 			// NONE
 
 			for _, val := range vals {
-				if glob.MustCompile(val, '/').Match(string(issueCommentPayload.Action)) {
+				if matchActivityType(val, string(issueCommentPayload.Action)) {
 					matchTimes++
 					break
 				}
@@ -718,7 +718,7 @@ func matchPullRequestReviewEvent(prPayload *api.PullRequestPayload, evt *jobpars
 			}
 
 			for _, val := range vals {
-				if slices.ContainsFunc(actions, glob.MustCompile(val, '/').Match) {
+				if matchActivityType(val, actions...) {
 					matchTimes++
 					break
 				}
@@ -758,7 +758,7 @@ func matchPullRequestReviewCommentEvent(prPayload *api.PullRequestPayload, evt *
 			}
 
 			for _, val := range vals {
-				if slices.ContainsFunc(actions, glob.MustCompile(val, '/').Match) {
+				if matchActivityType(val, actions...) {
 					matchTimes++
 					break
 				}
@@ -795,7 +795,7 @@ func matchReleaseEvent(payload *api.ReleasePayload, evt *jobparser.Event) bool {
 				action = "edited"
 			}
 			for _, val := range vals {
-				if glob.MustCompile(val, '/').Match(string(action)) {
+				if matchActivityType(val, string(action)) {
 					matchTimes++
 					break
 				}
@@ -832,7 +832,7 @@ func matchPackageEvent(payload *api.PackagePayload, evt *jobparser.Event) bool {
 				action = "published"
 			}
 			for _, val := range vals {
-				if glob.MustCompile(val, '/').Match(string(action)) {
+				if matchActivityType(val, string(action)) {
 					matchTimes++
 					break
 				}
@@ -857,7 +857,7 @@ func matchWorkflowRunEvent(payload *api.WorkflowRunPayload, evt *jobparser.Event
 		case "types":
 			action := payload.Action
 			for _, val := range vals {
-				if glob.MustCompile(val, '/').Match(action) {
+				if matchActivityType(val, action) {
 					matchTimes++
 					break
 				}
@@ -893,3 +893,13 @@ func matchWorkflowRunEvent(payload *api.WorkflowRunPayload, evt *jobparser.Event
 	}
 	return matchTimes == len(evt.Acts())
 }
+
+// matchActivityType treats an invalid user-provided pattern as no match instead of panicking
+func matchActivityType(pattern string, actions ...string) bool {
+	g, err := glob.Compile(pattern, '/')
+	if err != nil {
+		log.Debug("invalid activity type pattern %q: %v", pattern, err)
+		return false
+	}
+	return slices.ContainsFunc(actions, g.Match)
+}

modules/actions/workflows_test.go

@@ -210,6 +210,13 @@ func TestDetectMatched(t *testing.T) {
 			yamlOn:       "on:\n  release:\n    types: [published]",
 			expected:     detectMatched,
 		},
+		{
+			desc:         "HookEventRelease(release) doesn't match an invalid activity type pattern",
+			triggedEvent: webhook_module.HookEventRelease,
+			payload:      &api.ReleasePayload{Action: api.HookReleasePublished},
+			yamlOn:       "on:\n  release:\n    types: [\"[\"]",
+			expected:     detectNotApplicable,
+		},
 		{
 			desc:         "HookEventPackage(package) `created` action doesn't match GithubEventRegistryPackage(registry_package) with `updated` activity type",
 			triggedEvent: webhook_module.HookEventPackage,

modules/glob/glob.go

@@ -177,7 +177,7 @@ func initGlobCompiler(g *globCompiler, pattern string, separators []rune) (Glob,
 
 	regex, err := regexp.Compile(g.regexpPattern)
 	if err != nil {
-		return nil, fmt.Errorf("failed to compile regexp: %w", err)
+		return nil, fmt.Errorf("failed to compile glob regexp: %w", err)
 	}
 
 	g.regexp = regex

routers/api/actions/artifactsv4.go

@@ -646,9 +646,10 @@ func (r *artifactV4Routes) listArtifacts(ctx *ArtifactContext) {
 			Size:                    artifact.FileSize,
 		}
 	}
-	for _, artifact := range table {
-		if artifact != nil {
-			list = append(list, artifact)
+	for _, artifact := range artifacts {
+		if item := table[artifact.ArtifactName]; item != nil {
+			list = append(list, item)
+			delete(table, artifact.ArtifactName)
 		}
 	}
 

routers/api/v1/shared/action.go

@@ -186,7 +186,7 @@ func ListRuns(ctx *context.APIContext, ownerID, repoID int64, workflowID string)
 	if actor := ctx.FormString("actor"); actor != "" {
 		user, err := user_model.GetUserByName(ctx, actor)
 		if err != nil {
-			ctx.APIErrorInternal(err)
+			ctx.APIErrorAuto(err)
 			return
 		}
 		opts.TriggerUserID = user.ID

routers/web/repo/actions/view.go

@@ -851,7 +851,7 @@ func convertToViewModel(ctx context.Context, locale translation.Locale, cursors
 	}
 
 	for _, cursor := range cursors {
-		if !cursor.Expanded {
+		if !cursor.Expanded || cursor.Step < 0 || cursor.Step >= len(steps) {
 			continue
 		}
 

routers/web/repo/actions/view_test.go

@@ -88,6 +88,13 @@ func TestConvertToViewModel(t *testing.T) {
 	viewJobSteps, _, err := convertToViewModel(t.Context(), translation.MockLocale{}, nil, task)
 	require.NoError(t, err)
 
+	t.Run("out of range cursor step", func(t *testing.T) {
+		cursors := []LogCursor{{Step: -1, Expanded: true}, {Step: 3, Expanded: true}}
+		_, logs, err := convertToViewModel(t.Context(), translation.MockLocale{}, cursors, task)
+		require.NoError(t, err)
+		assert.Empty(t, logs)
+	})
+
 	expectedViewJobs := []*ViewJobStep{
 		{
 			Summary:  "Set up job",

services/actions/run.go

@@ -190,7 +190,10 @@ func insertRunJob(ctx context.Context, run *actions_model.ActionRun, runAttempt
 	if err := workflowJob.SetJob(id, job.EraseNeeds()); err != nil {
 		return nil, nil, false, err
 	}
-	payload, _ := workflowJob.Marshal()
+	payload, err := workflowJob.Marshal()
+	if err != nil {
+		return nil, nil, false, fmt.Errorf("insertRunJob: marshal job %q: %w", id, err)
+	}
 
 	isReusableWorkflowCaller := job.Uses != ""
 	status := util.Iif(runAttempt.Status == actions_model.StatusBlocked || run.NeedApproval, actions_model.StatusBlocked, actions_model.StatusWaiting)

tests/integration/api_actions_artifact_test.go

@@ -682,7 +682,7 @@ jobs:
 	taskToken3 := task3.Context.GetFields()["gitea_runtime_token"].GetStringValue()
 
 	// the new attempt inherits what the previous attempt uploaded
-	assert.ElementsMatch(t, []string{"job1-only", "job1-shared"}, listArtifactNamesForRunV4(t, run.ID, job3.ID, taskToken3))
+	assert.Equal(t, []string{"job1-only", "job1-shared"}, listArtifactNamesForRunV4(t, run.ID, job3.ID, taskToken3))
 	assert.Equal(t, strings.Repeat("A", 32), downloadArtifactContentV4ByTask(t, run.ID, job3.ID, taskToken3, "job1-only"))
 	assert.Contains(t, listArtifactNamesForRun(t, run.ID, taskToken3), "job1-v3")
 
@@ -699,7 +699,7 @@ jobs:
 	inheritedSharedID := listArtifactIDForRunV4(t, run.ID, job3.ID, taskToken3, "job1-shared")
 	require.Len(t, listArtifactsByIDV4(t, run.ID, job3.ID, inheritedSharedID, taskToken3), 1)
 	uploadTestArtifactFileV4(t, run.ID, job3.ID, taskToken3, "job1-shared", strings.Repeat("C", 32))
-	assert.ElementsMatch(t, []string{"job1-only", "job1-shared"}, listArtifactNamesForRunV4(t, run.ID, job3.ID, taskToken3))
+	assert.Equal(t, []string{"job1-only", "job1-shared"}, listArtifactNamesForRunV4(t, run.ID, job3.ID, taskToken3))
 	assert.Equal(t, strings.Repeat("C", 32), downloadArtifactContentV4ByTask(t, run.ID, job3.ID, taskToken3, "job1-shared"))
 
 	// a shadowed artifact is not listed by id either: a download resolves by name

tests/integration/api_actions_artifact_v4_test.go

@@ -899,7 +899,7 @@ func testActionRunAttemptArtifactV4(t *testing.T, repo *repo_model.Repository, s
 	uploadTestArtifactFileV4(t, run.ID, job1.ID, taskToken1, "artifact-attempt-1", strings.Repeat("A", 32))
 	uploadTestArtifactFileV4(t, run.ID, job1.ID, taskToken1, "artifact-shared", strings.Repeat("C", 32))
 	attempt1Names := listArtifactNamesForRunV4(t, run.ID, job1.ID, taskToken1)
-	assert.ElementsMatch(t, []string{"artifact-attempt-1", "artifact-shared"}, attempt1Names)
+	assert.Equal(t, []string{"artifact-attempt-1", "artifact-shared"}, attempt1Names)
 
 	runner.execTask(t, task1, &mockTaskOutcome{result: runnerv1.Result_RESULT_SUCCESS})
 
@@ -915,7 +915,7 @@ func testActionRunAttemptArtifactV4(t *testing.T, repo *repo_model.Repository, s
 	uploadTestArtifactFileV4(t, run.ID, job2.ID, taskToken2, "artifact-attempt-2", strings.Repeat("B", 32))
 	uploadTestArtifactFileV4(t, run.ID, job2.ID, taskToken2, "artifact-shared", strings.Repeat("D", 32))
 	attempt2Names := listArtifactNamesForRunV4(t, run.ID, job2.ID, taskToken2)
-	assert.ElementsMatch(t, []string{"artifact-attempt-2", "artifact-shared"}, attempt2Names)
+	assert.Equal(t, []string{"artifact-attempt-2", "artifact-shared"}, attempt2Names)
 	assert.NotContains(t, attempt2Names, "artifact-attempt-1")
 
 	// "artifact-attempt-1" belongs to the first attempt, so the rerun token cannot access it

tests/integration/api_actions_run_test.go

@@ -69,6 +69,12 @@ func testAPIActionsGetWorkflowRun(t *testing.T) {
 		MakeRequest(t, req, http.StatusOK)
 	})
 
+	t.Run("ListRunsUnknownActor", func(t *testing.T) {
+		req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs?actor=no-such-user", repo.FullName())).
+			AddTokenAuth(token)
+		MakeRequest(t, req, http.StatusNotFound)
+	})
+
 	t.Run("GetJobSteps", func(t *testing.T) {
 		// Insert task steps for task_id 53 (job 198) so the API can return them once the backend loads them
 		_, err := db.GetEngine(t.Context()).Insert(&actions_model.ActionTaskStep{