diff --git a/models/git/protected_branch.go b/models/git/protected_branch.go index 1ad08736c1f..b11da1bddbc 100644 --- a/models/git/protected_branch.go +++ b/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) } diff --git a/models/git/protected_branch_list.go b/models/git/protected_branch_list.go index 9defb708401..aebc97b10d8 100644 --- a/models/git/protected_branch_list.go +++ b/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 } } diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index 0e14df432e6..9f1bdf5b27c 100644 --- a/modules/actions/workflows.go +++ b/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) +} diff --git a/modules/actions/workflows_test.go b/modules/actions/workflows_test.go index 486e734900a..8672d3205ac 100644 --- a/modules/actions/workflows_test.go +++ b/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, diff --git a/modules/glob/glob.go b/modules/glob/glob.go index edc900b72ba..1bc5a6dec62 100644 --- a/modules/glob/glob.go +++ b/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 diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 0ea44ee57d9..a7befae91d8 100644 --- a/routers/api/actions/artifactsv4.go +++ b/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) } } diff --git a/routers/api/v1/shared/action.go b/routers/api/v1/shared/action.go index 5b4ac8f4516..ac6c88bc188 100644 --- a/routers/api/v1/shared/action.go +++ b/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 diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index 6f3667ee8b7..6ce7975fd9b 100644 --- a/routers/web/repo/actions/view.go +++ b/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 } diff --git a/routers/web/repo/actions/view_test.go b/routers/web/repo/actions/view_test.go index 9e36b8a62f2..af8ccfe3fdc 100644 --- a/routers/web/repo/actions/view_test.go +++ b/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", diff --git a/services/actions/run.go b/services/actions/run.go index c2565902ce9..701ccc9983d 100644 --- a/services/actions/run.go +++ b/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) diff --git a/tests/integration/api_actions_artifact_test.go b/tests/integration/api_actions_artifact_test.go index ece8afda4c8..3e5e92ba871 100644 --- a/tests/integration/api_actions_artifact_test.go +++ b/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 diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index 5acc58669bf..ddb4515ee9a 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/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 diff --git a/tests/integration/api_actions_run_test.go b/tests/integration/api_actions_run_test.go index 6433c2789c1..d28d8e2c643 100644 --- a/tests/integration/api_actions_run_test.go +++ b/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{