From 47ffd078dd769ec97df7a9efdb80410b8b2fb953 Mon Sep 17 00:00:00 2001 From: Ross Golder Date: Sun, 3 May 2026 09:50:24 +0700 Subject: [PATCH] Fix review issues: auth on log routes, LoadRepos removal, test coverage - Add reqToken/reqRepoReader to GET log download endpoints for consistency with the POST streaming endpoint - Remove spurious LoadRepos call in DownloadActionsRunAllJobLogs; jobs are already scoped to the repo by the query and Repo is never read - Refactor reader.Close() in zip loop to use a closure with defer - Update copyright year to 2026 on new services/actions/{cancel,log}.go - Add TestAPIActionsListUserWorkflows and TestAPIActionsListRepoWorkflows as standalone top-level tests (were dropped when breaking up the orchestrator) - Add idempotency assertion to TestAPIActionsApproveWorkflowRun: approving an already-approved run returns 400 Co-Authored-By: Claude Sonnet --- routers/api/v1/api.go | 4 +- routers/common/actions.go | 35 +++++------ services/actions/cancel.go | 2 +- services/actions/log.go | 2 +- tests/integration/api_actions_run_test.go | 71 +++++++++++++++++++++++ 5 files changed, 91 insertions(+), 23 deletions(-) diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 846195893d..ede987cd66 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -1265,11 +1265,11 @@ func Routes() *web.Router { m.Post("/approve", reqToken(), reqRepoWriter(unit.TypeActions), repo.ApproveWorkflowRun) m.Group("/jobs", func() { m.Get("", repo.ListWorkflowRunJobs) - m.Get("/{job_id}/logs", repo.GetWorkflowJobLogs) + m.Get("/{job_id}/logs", reqToken(), reqRepoReader(unit.TypeActions), repo.GetWorkflowJobLogs) m.Post("/{job_id}/rerun", reqToken(), reqRepoWriter(unit.TypeActions), repo.RerunWorkflowJob) }) m.Group("/logs", func() { - m.Get("", repo.GetWorkflowRunLogs) + m.Get("", reqToken(), reqRepoReader(unit.TypeActions), repo.GetWorkflowRunLogs) m.Post("", reqToken(), reqRepoReader(unit.TypeActions), repo.GetWorkflowRunLogsStream) }) m.Get("/artifacts", repo.GetArtifactsOfRun) diff --git a/routers/common/actions.go b/routers/common/actions.go index bf514c81da..29d4b9b752 100644 --- a/routers/common/actions.go +++ b/routers/common/actions.go @@ -35,9 +35,6 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit if err != nil { return fmt.Errorf("GetLatestAttemptJobsByRepoAndRunID: %w", err) } - if err = runJobs.LoadRepos(ctx); err != nil { - return fmt.Errorf("LoadRepos: %w", err) - } if len(runJobs) == 0 { return util.NewNotExistErrorf("no jobs found for run %d", runID) @@ -77,27 +74,27 @@ func DownloadActionsRunAllJobLogs(ctx *context.Base, ctxRepo *repo_model.Reposit continue } - reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename) - if err != nil { - return fmt.Errorf("OpenLogs for job %d: %w", job.ID, err) - } - // Create file in zip with job name and task ID; sanitize to prevent Zip Slip safeJobName := strings.NewReplacer("/", "-", `\`, "-", "..", "__").Replace(job.Name) fileName := fmt.Sprintf("%s-%s-%d.log", safeWorkflowName, safeJobName, task.ID) - zipFile, err := zipWriter.Create(fileName) - if err != nil { - reader.Close() - return fmt.Errorf("Create zip file %s: %w", fileName, err) - } - // Copy log content to zip file - if _, err := io.Copy(zipFile, reader); err != nil { - reader.Close() - return fmt.Errorf("Copy logs for job %d: %w", job.ID, err) - } + if err := func() error { + reader, err := actions.OpenLogs(ctx, task.LogInStorage, task.LogFilename) + if err != nil { + return err + } + defer reader.Close() - reader.Close() + zipFile, err := zipWriter.Create(fileName) + if err != nil { + return err + } + + _, err = io.Copy(zipFile, reader) + return err + }(); err != nil { + return fmt.Errorf("job %d: %w", job.ID, err) + } } return nil diff --git a/services/actions/cancel.go b/services/actions/cancel.go index 9289acde4a..be6f29249d 100644 --- a/services/actions/cancel.go +++ b/services/actions/cancel.go @@ -1,4 +1,4 @@ -// Copyright 2025 The Gitea Authors. All rights reserved. +// Copyright 2026 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package actions diff --git a/services/actions/log.go b/services/actions/log.go index 088a1f512b..e5206eabe8 100644 --- a/services/actions/log.go +++ b/services/actions/log.go @@ -1,4 +1,4 @@ -// Copyright 2025 The Gitea Authors. All rights reserved. +// Copyright 2026 The Gitea Authors. All rights reserved. // SPDX-License-Identifier: MIT package actions diff --git a/tests/integration/api_actions_run_test.go b/tests/integration/api_actions_run_test.go index 1efb18c41c..32b5410fd4 100644 --- a/tests/integration/api_actions_run_test.go +++ b/tests/integration/api_actions_run_test.go @@ -358,6 +358,11 @@ jobs: assert.Equal(t, actions_model.StatusWaiting, job.Status) } + // Test approve already approved run (idempotency) + 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) + // Test approve non-existent run req = NewRequest(t, "POST", fmt.Sprintf("/api/v1/repos/%s/actions/runs/999999/approve", baseRepo.FullName())). AddTokenAuth(user2Token) @@ -436,6 +441,72 @@ func TestAPIActionsRerunWorkflowJob(t *testing.T) { }) } +func TestAPIActionsListUserWorkflows(t *testing.T) { + defer prepareTestEnvActionsArtifacts(t)() + + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + session := loginUser(t, user.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadUser) + + 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) + + assert.Positive(t, runs.TotalCount) + assert.NotEmpty(t, runs.Entries) + + for _, run := range runs.Entries { + assert.NotEmpty(t, run.DisplayTitle, "display_title should be populated") + assert.NotNil(t, run.Repository, "repository should be populated via batch loading") + assert.NotEmpty(t, run.Repository.FullName, "repository full_name should be populated") + assert.NotNil(t, run.TriggerActor, "trigger_actor should be populated via batch loading") + } + }) + + 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) + + assert.Positive(t, jobs.TotalCount) + assert.NotEmpty(t, jobs.Entries) + + for _, job := range jobs.Entries { + assert.NotEmpty(t, job.Name, "job name should be populated") + assert.NotEmpty(t, job.HTMLURL, "html_url should be populated via batch-loaded repo") + } + }) +} + +func TestAPIActionsListRepoWorkflows(t *testing.T) { + defer prepareTestEnvActionsArtifacts(t)() + + repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: repo.OwnerID}) + session := loginUser(t, user.Name) + token := getTokenForLoggedInUser(t, session, auth_model.AccessTokenScopeReadRepository) + + 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) + + assert.Positive(t, runs.TotalCount) + assert.NotEmpty(t, runs.Entries) + + for _, run := range runs.Entries { + assert.NotNil(t, run.Repository, "repository should be populated from ctx.Repo") + assert.Equal(t, repo.FullName(), run.Repository.FullName, "repository full_name should match") + assert.NotNil(t, run.TriggerActor, "trigger_actor should be populated") + } +} + func TestAPIActionsGetWorkflowRunLogs(t *testing.T) { defer prepareTestEnvActionsArtifacts(t)()