From 8c90b491e5fc7b88522874c80698b85fd281fa79 Mon Sep 17 00:00:00 2001 From: Ross Golder Date: Sun, 17 May 2026 10:59:49 +0700 Subject: [PATCH] fix(actions): address review issues for Actions API endpoints - Use EffectiveTaskID() for rerun jobs in DownloadActionsRunAllJobLogs - Add nil guard for deleted repos in shared.ListRuns - Make ApproveWorkflowRun idempotent (return 200 on re-approve) - Remove stale reload in ApproveWorkflowRun, use in-memory updates - Log zip errors instead of corrupting response mid-stream - Fix misleading Zip Slip comment - Revert DecodeJSON -> json.Unmarshal changes in tests - Add test coverage: cancel state assertions, zip headers, writer-but-non-admin approve, rerun job logs - Fix ToActionWorkflowRun to use repo parameter consistently Co-Authored-By: Ross Golder Co-Authored-By: OpenCode Agent --- routers/api/v1/repo/actions_run.go | 21 +++--- routers/api/v1/shared/action.go | 7 +- routers/common/actions.go | 11 +-- services/convert/convert.go | 5 +- tests/integration/api_actions_run_test.go | 81 ++++++++++++++--------- 5 files changed, 78 insertions(+), 47 deletions(-) diff --git a/routers/api/v1/repo/actions_run.go b/routers/api/v1/repo/actions_run.go index 730771c17c..2ba3a36eec 100644 --- a/routers/api/v1/repo/actions_run.go +++ b/routers/api/v1/repo/actions_run.go @@ -181,8 +181,14 @@ func ApproveWorkflowRun(ctx *context.APIContext) { return } + // GitHub-compatible: return 200 if already approved (idempotent) if !run.NeedApproval { - ctx.APIError(http.StatusBadRequest, "Run does not require approval") + convertedRun, err := convert.ToActionWorkflowRun(ctx, ctx.Repo.Repository, run, nil) + if err != nil { + ctx.APIErrorInternal(err) + return + } + ctx.JSON(http.StatusOK, convertedRun) return } @@ -195,14 +201,13 @@ func ApproveWorkflowRun(ctx *context.APIContext) { return } - // Reload run to reflect post-approval state. - updatedRun, has, err := db.GetByID[actions_model.ActionRun](ctx, runID) - if err != nil || !has { - ctx.APIErrorInternal(err) - return - } + // Update known-changed fields on the run object in memory. + // Note: the overall run status is updated asynchronously by the notifier, + // so the status field may still reflect the pre-approval state. + run.NeedApproval = false + run.ApprovedBy = ctx.Doer.ID - convertedRun, err := convert.ToActionWorkflowRun(ctx, ctx.Repo.Repository, updatedRun, nil) + convertedRun, err := convert.ToActionWorkflowRun(ctx, ctx.Repo.Repository, run, nil) if err != nil { ctx.APIErrorInternal(err) return diff --git a/routers/api/v1/shared/action.go b/routers/api/v1/shared/action.go index d2a42e191a..583ba8fbde 100644 --- a/routers/api/v1/shared/action.go +++ b/routers/api/v1/shared/action.go @@ -199,15 +199,18 @@ func ListRuns(ctx *context.APIContext, ownerID, repoID int64) { return } - res.Entries = make([]*api.ActionWorkflowRun, len(runs)) + res.Entries = make([]*api.ActionWorkflowRun, 0, len(runs)) for i := range runs { + if runs[i].Repo == nil { + continue // skip runs whose repository has been deleted + } // TODO: load run attempts in batch convertedRun, err := convert.ToActionWorkflowRun(ctx, runs[i].Repo, runs[i], nil) if err != nil { ctx.APIErrorInternal(err) return } - res.Entries[i] = convertedRun + res.Entries = append(res.Entries, convertedRun) } ctx.SetLinkHeader(total, listOptions.PageSize) ctx.SetTotalCountHeader(total) diff --git a/routers/common/actions.go b/routers/common/actions.go index 29d4b9b752..c90108966a 100644 --- a/routers/common/actions.go +++ b/routers/common/actions.go @@ -15,6 +15,7 @@ import ( repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/actions" "code.gitea.io/gitea/modules/httplib" + "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/services/context" ) @@ -61,11 +62,12 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit // Add each job's logs to the zip for _, job := range runJobs { - if job.TaskID == 0 { + taskID := job.EffectiveTaskID() + if taskID == 0 { continue // Skip jobs that haven't started } - task, err := actions_model.GetTaskByID(ctx, job.TaskID) + task, err := actions_model.GetTaskByID(ctx, taskID) if err != nil { return fmt.Errorf("GetTaskByID for job %d: %w", job.ID, err) } @@ -74,7 +76,7 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit continue } - // Create file in zip with job name and task ID; sanitize to prevent Zip Slip + // Create file in zip with job name and task ID; sanitize job names for safe zip entry paths safeJobName := strings.NewReplacer("/", "-", `\`, "-", "..", "__").Replace(job.Name) fileName := fmt.Sprintf("%s-%s-%d.log", safeWorkflowName, safeJobName, task.ID) @@ -93,7 +95,8 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit _, err = io.Copy(zipFile, reader) return err }(); err != nil { - return fmt.Errorf("job %d: %w", job.ID, err) + log.Error("Failed to add logs for job %d to zip: %v", job.ID, err) + continue } } diff --git a/services/convert/convert.go b/services/convert/convert.go index 9a93484ca7..eb5c757b14 100644 --- a/services/convert/convert.go +++ b/services/convert/convert.go @@ -258,9 +258,8 @@ func ToActionTask(ctx context.Context, t *actions_model.ActionTask) (*api.Action } func ToActionWorkflowRun(ctx context.Context, repo *repo_model.Repository, run *actions_model.ActionRun, attempt *actions_model.ActionRunAttempt) (*api.ActionWorkflowRun, error) { - if run.Repo == nil { - run.Repo = repo - } + // caller-provided repo is the single source of truth for URL construction + run.Repo = repo if err := run.LoadTriggerUser(ctx); err != nil { return nil, err } diff --git a/tests/integration/api_actions_run_test.go b/tests/integration/api_actions_run_test.go index 983c4b22ef..60df50e9ce 100644 --- a/tests/integration/api_actions_run_test.go +++ b/tests/integration/api_actions_run_test.go @@ -15,10 +15,10 @@ import ( actions_model "code.gitea.io/gitea/models/actions" auth_model "code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/db" + "code.gitea.io/gitea/models/perm" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" - "code.gitea.io/gitea/modules/json" api "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/timeutil" @@ -63,9 +63,7 @@ func TestAPIActionsGetWorkflowRun(t *testing.T) { AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) - var jobList api.ActionWorkflowJobsResponse - err = json.Unmarshal(resp.Body.Bytes(), &jobList) - require.NoError(t, err) + jobList := DecodeJSON(t, resp, &api.ActionWorkflowJobsResponse{}) job198Idx := slices.IndexFunc(jobList.Entries, func(job *api.ActionWorkflowJob) bool { return job.ID == 198 }) require.NotEqual(t, -1, job198Idx, "expected to find job 198 in run 795 jobs list") @@ -144,9 +142,7 @@ func testAPIActionsDeleteRunListArtifacts(t *testing.T, repo *repo_model.Reposit req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/artifacts", repo.FullName())). AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) - var listResp api.ActionArtifactsResponse - err := json.Unmarshal(resp.Body.Bytes(), &listResp) - assert.NoError(t, err) + listResp := DecodeJSON(t, resp, &api.ActionArtifactsResponse{}) assert.Len(t, listResp.Entries, artifacts) } @@ -154,9 +150,7 @@ func testAPIActionsDeleteRunListTasks(t *testing.T, repo *repo_model.Repository, req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/tasks", repo.FullName())). AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) - var listResp api.ActionTaskResponse - err := json.Unmarshal(resp.Body.Bytes(), &listResp) - assert.NoError(t, err) + listResp := DecodeJSON(t, resp, &api.ActionTaskResponse{}) findTask1 := false findTask2 := false for _, entry := range listResp.Entries { @@ -199,9 +193,7 @@ func TestAPIActionsRerunWorkflowRun(t *testing.T) { AddTokenAuth(writeToken) resp := MakeRequest(t, req, http.StatusCreated) - var rerunResp api.ActionWorkflowRun - err := json.Unmarshal(resp.Body.Bytes(), &rerunResp) - require.NoError(t, err) + rerunResp := DecodeJSON(t, resp, &api.ActionWorkflowRun{}) assert.Equal(t, int64(795), rerunResp.ID) assert.Equal(t, "queued", rerunResp.Status) assert.Equal(t, "c2d72f548424103f01ee1dc02889c1e2bff816b0", rerunResp.HeadSha) @@ -250,7 +242,11 @@ func TestAPIActionsCancelWorkflowRun(t *testing.T) { t.Run("Success", func(t *testing.T) { req := NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/793/cancel", repo.FullName())). AddTokenAuth(ownerToken) - MakeRequest(t, req, http.StatusOK) + resp := MakeRequest(t, req, http.StatusOK) + cancelledRun := DecodeJSON(t, resp, &api.ActionWorkflowRun{}) + assert.Equal(t, int64(793), cancelledRun.ID) + assert.Equal(t, "completed", cancelledRun.Status) + assert.Equal(t, "cancelled", cancelledRun.Conclusion) }) t.Run("NotFound", func(t *testing.T) { @@ -357,20 +353,32 @@ jobs: assert.Equal(t, actions_model.StatusWaiting, job.Status) } - // Test approve already approved run (idempotency) + // Test approve already approved run (idempotency — returns 200 like GitHub) req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/%d/approve", baseRepo.FullName(), run.ID)). AddTokenAuth(user2Token) - MakeRequest(t, req, http.StatusBadRequest) + resp = MakeRequest(t, req, http.StatusOK) + idempotentRun := DecodeJSON(t, resp, &api.ActionWorkflowRun{}) + assert.NotEqual(t, "waiting", idempotentRun.Status, "already-approved run should not be blocked") // Test approve non-existent run req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/999999/approve", baseRepo.FullName())). AddTokenAuth(user2Token) MakeRequest(t, req, http.StatusNotFound) - // Test approve by non-owner (user4 should get forbidden) + // Test approve by non-owner (user4 should get forbidden before being added as collaborator) req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/%d/approve", baseRepo.FullName(), run.ID)). AddTokenAuth(user4Token) MakeRequest(t, req, http.StatusForbidden) + + // Add user4 as a collaborator with write access + doAPIAddCollaborator(user2APICtx, user4.Name, perm.AccessModeWrite)(t) + + // Test approve by writer-but-non-admin (user4 should now succeed) + req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/%d/approve", baseRepo.FullName(), run.ID)). + AddTokenAuth(user4Token) + resp = MakeRequest(t, req, http.StatusOK) + approvedRun := DecodeJSON(t, resp, &api.ActionWorkflowRun{}) + assert.NotEqual(t, "waiting", approvedRun.Status, "approved run should not be blocked") }) } @@ -400,9 +408,7 @@ func TestAPIActionsRerunWorkflowJob(t *testing.T) { AddTokenAuth(writeToken) resp := MakeRequest(t, req, http.StatusCreated) - var rerunResp api.ActionWorkflowJob - err := json.Unmarshal(resp.Body.Bytes(), &rerunResp) - require.NoError(t, err) + rerunResp := DecodeJSON(t, resp, &api.ActionWorkflowJob{}) job199Rerun := getLatestAttemptJobByTemplateJobID(t, 795, 199) assert.Equal(t, job199Rerun.ID, rerunResp.ID) assert.Equal(t, "queued", rerunResp.Status) @@ -450,9 +456,7 @@ func TestAPIActionsListUserWorkflows(t *testing.T) { t.Run("Runs", func(t *testing.T) { req := NewRequest(t, "GET", "/api/v1/user/actions/runs").AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) - var runs api.ActionWorkflowRunsResponse - err := json.Unmarshal(resp.Body.Bytes(), &runs) - require.NoError(t, err) + runs := DecodeJSON(t, resp, &api.ActionWorkflowRunsResponse{}) assert.Positive(t, runs.TotalCount) assert.NotEmpty(t, runs.Entries) @@ -468,9 +472,7 @@ func TestAPIActionsListUserWorkflows(t *testing.T) { t.Run("Jobs", func(t *testing.T) { req := NewRequest(t, "GET", "/api/v1/user/actions/jobs").AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) - var jobs api.ActionWorkflowJobsResponse - err := json.Unmarshal(resp.Body.Bytes(), &jobs) - require.NoError(t, err) + jobs := DecodeJSON(t, resp, &api.ActionWorkflowJobsResponse{}) assert.Positive(t, jobs.TotalCount) assert.NotEmpty(t, jobs.Entries) @@ -492,9 +494,7 @@ func TestAPIActionsListRepoWorkflows(t *testing.T) { req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs", repo.FullName())).AddTokenAuth(token) resp := MakeRequest(t, req, http.StatusOK) - var runs api.ActionWorkflowRunsResponse - err := json.Unmarshal(resp.Body.Bytes(), &runs) - require.NoError(t, err) + runs := DecodeJSON(t, resp, &api.ActionWorkflowRunsResponse{}) assert.Positive(t, runs.TotalCount) assert.NotEmpty(t, runs.Entries) @@ -517,7 +517,28 @@ func TestAPIActionsGetWorkflowRunLogs(t *testing.T) { t.Run("Success", func(t *testing.T) { req := NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/logs", repo.FullName())). AddTokenAuth(token) - MakeRequest(t, req, http.StatusOK) + resp := MakeRequest(t, req, http.StatusOK) + assert.Equal(t, "application/zip", resp.Header().Get("Content-Type")) + assert.Contains(t, resp.Header().Get("Content-Disposition"), "attachment") + assert.Contains(t, resp.Header().Get("Content-Disposition"), ".zip") + body := resp.Body.Bytes() + require.NotEmpty(t, body) + assert.Equal(t, "PK", string(body[:2]), "response should be a valid zip file") + }) + + t.Run("RerunJobLogs", func(t *testing.T) { + // Rerun the workflow so latest-attempt jobs have SourceTaskID instead of TaskID + req := NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/rerun", repo.FullName())). + AddTokenAuth(token) + MakeRequest(t, req, http.StatusCreated) + + // Download logs for the latest attempt — should include rerun job logs via EffectiveTaskID + req = NewRequest(t, "GET", fmt.Sprintf("/api/v1/repos/%s/actions/runs/795/logs", repo.FullName())). + AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + body := resp.Body.Bytes() + require.NotEmpty(t, body) + assert.Equal(t, "PK", string(body[:2]), "response should be a valid zip file") }) t.Run("NotFound", func(t *testing.T) {