From 7668923c1f98b4430d20f049ed8a31a6f2c84f3b Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 23 Apr 2025 17:15:53 +0200 Subject: [PATCH 1/4] actions artifacts internal api list/download check status upload confirmed * fixes a fixture status to upload confirmed * add another fixture as noise to break tests as soon they are exposed to api * v4 delete test added check that artifact is no longer visible in internal api with status pending delete --- models/fixtures/action_artifact.yml | 18 ++++++++++++ routers/api/actions/artifacts.go | 11 ++++++- routers/api/actions/artifactsv4.go | 20 +++++++++---- .../api_actions_artifact_v4_test.go | 29 +++++++++++++++++++ 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/models/fixtures/action_artifact.yml b/models/fixtures/action_artifact.yml index 485474108f7ae..1b00daf19817f 100644 --- a/models/fixtures/action_artifact.yml +++ b/models/fixtures/action_artifact.yml @@ -11,6 +11,24 @@ content_encoding: "" artifact_path: "abc.txt" artifact_name: "artifact-download" + status: 2 + created_unix: 1712338649 + updated_unix: 1712338649 + expired_unix: 1720114649 + +- + id: 2 + run_id: 791 + runner_id: 1 + repo_id: 4 + owner_id: 1 + commit_sha: c2d72f548424103f01ee1dc02889c1e2bff816b0 + storage_path: "" + file_size: 1024 + file_compressed_size: 1024 + content_encoding: "30/20/1712348022422036662.chunk" + artifact_path: "abc.txt" + artifact_name: "artifact-download-incomplete" status: 1 created_unix: 1712338649 updated_unix: 1712338649 diff --git a/routers/api/actions/artifacts.go b/routers/api/actions/artifacts.go index 0832e52f5558b..573cfa1e81778 100644 --- a/routers/api/actions/artifacts.go +++ b/routers/api/actions/artifacts.go @@ -337,7 +337,10 @@ func (ar artifactRoutes) listArtifacts(ctx *ArtifactContext) { return } - artifacts, err := db.Find[actions.ActionArtifact](ctx, actions.FindArtifactsOptions{RunID: runID}) + artifacts, err := db.Find[actions.ActionArtifact](ctx, actions.FindArtifactsOptions{ + RunID: runID, + Status: int(actions.ArtifactStatusUploadConfirmed), + }) if err != nil { log.Error("Error getting artifacts: %v", err) ctx.HTTPError(http.StatusInternalServerError, err.Error()) @@ -402,6 +405,7 @@ func (ar artifactRoutes) getDownloadArtifactURL(ctx *ArtifactContext) { artifacts, err := db.Find[actions.ActionArtifact](ctx, actions.FindArtifactsOptions{ RunID: runID, ArtifactName: itemPath, + Status: int(actions.ArtifactStatusUploadConfirmed), }) if err != nil { log.Error("Error getting artifacts: %v", err) @@ -473,6 +477,11 @@ func (ar artifactRoutes) downloadArtifact(ctx *ArtifactContext) { ctx.HTTPError(http.StatusBadRequest) return } + if artifact.Status != actions.ArtifactStatusUploadConfirmed { + log.Error("Error artifact not found: unconfirmed") + ctx.HTTPError(http.StatusNotFound, "Error artifact not found") + return + } fd, err := ar.fs.Open(artifact.StoragePath) if err != nil { diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 9fb0a31549c4c..f0dbf08cada06 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -448,17 +448,15 @@ func (r *artifactV4Routes) listArtifacts(ctx *ArtifactContext) { return } - artifacts, err := db.Find[actions.ActionArtifact](ctx, actions.FindArtifactsOptions{RunID: runID}) + artifacts, err := db.Find[actions.ActionArtifact](ctx, actions.FindArtifactsOptions{ + RunID: runID, + Status: int(actions.ArtifactStatusUploadConfirmed), + }) if err != nil { log.Error("Error getting artifacts: %v", err) ctx.HTTPError(http.StatusInternalServerError, err.Error()) return } - if len(artifacts) == 0 { - log.Debug("[artifact] handleListArtifacts, no artifacts") - ctx.HTTPError(http.StatusNotFound) - return - } list := []*ListArtifactsResponse_MonolithArtifact{} @@ -510,6 +508,11 @@ func (r *artifactV4Routes) getSignedArtifactURL(ctx *ArtifactContext) { ctx.HTTPError(http.StatusNotFound, "Error artifact not found") return } + if artifact.Status != actions.ArtifactStatusUploadConfirmed { + log.Error("Error artifact not found: unconfirmed") + ctx.HTTPError(http.StatusNotFound, "Error artifact not found") + return + } respData := GetSignedArtifactURLResponse{} @@ -538,6 +541,11 @@ func (r *artifactV4Routes) downloadArtifact(ctx *ArtifactContext) { ctx.HTTPError(http.StatusNotFound, "Error artifact not found") return } + if artifact.Status != actions.ArtifactStatusUploadConfirmed { + log.Error("Error artifact not found: unconfirmed") + ctx.HTTPError(http.StatusNotFound, "Error artifact not found") + return + } file, _ := r.fs.Open(artifact.StoragePath) diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index b6dfa6e7994d5..4b7f9691f526a 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -557,6 +557,35 @@ func TestActionsArtifactV4Delete(t *testing.T) { var deleteResp actions.DeleteArtifactResponse protojson.Unmarshal(resp.Body.Bytes(), &deleteResp) assert.True(t, deleteResp.Ok) + + // confirm artifact is no longer accessible by GetSignedArtifactURL + req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/GetSignedArtifactURL", toProtoJSON(&actions.GetSignedArtifactURLRequest{ + Name: "artifact-v4-download", + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusNotFound) + + // confirm artifact is no longer accessible by GetSignedArtifactURL + req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/GetSignedArtifactURL", toProtoJSON(&actions.GetSignedArtifactURLRequest{ + Name: "artifact-v4-download", + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusNotFound) + + // confirm artifact is no longer enumerateable by ListArtifacts and returns length == 0 without error + req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/ListArtifacts", toProtoJSON(&actions.ListArtifactsRequest{ + NameFilter: wrapperspb.String("artifact-v4-download"), + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })).AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + var listResp actions.ListArtifactsResponse + protojson.Unmarshal(resp.Body.Bytes(), &listResp) + assert.Len(t, listResp.Artifacts, 0) } func TestActionsArtifactV4DeletePublicApi(t *testing.T) { From fc1e675862d1acc7c2589553a85f44ad77ccee1b Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 23 Apr 2025 17:18:21 +0200 Subject: [PATCH 2/4] remove duplicate --- tests/integration/api_actions_artifact_v4_test.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index 4b7f9691f526a..918b4909708c6 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -567,15 +567,6 @@ func TestActionsArtifactV4Delete(t *testing.T) { AddTokenAuth(token) resp = MakeRequest(t, req, http.StatusNotFound) - // confirm artifact is no longer accessible by GetSignedArtifactURL - req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/GetSignedArtifactURL", toProtoJSON(&actions.GetSignedArtifactURLRequest{ - Name: "artifact-v4-download", - WorkflowRunBackendId: "792", - WorkflowJobRunBackendId: "193", - })). - AddTokenAuth(token) - resp = MakeRequest(t, req, http.StatusNotFound) - // confirm artifact is no longer enumerateable by ListArtifacts and returns length == 0 without error req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/ListArtifacts", toProtoJSON(&actions.ListArtifactsRequest{ NameFilter: wrapperspb.String("artifact-v4-download"), From 11501c43194a9a4a6fa8f4453f81310b412e7ed8 Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Wed, 23 Apr 2025 17:28:56 +0200 Subject: [PATCH 3/4] code style fixes --- tests/integration/api_actions_artifact_v4_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index 918b4909708c6..3db8bbb82e78c 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -565,7 +565,7 @@ func TestActionsArtifactV4Delete(t *testing.T) { WorkflowJobRunBackendId: "193", })). AddTokenAuth(token) - resp = MakeRequest(t, req, http.StatusNotFound) + _ = MakeRequest(t, req, http.StatusNotFound) // confirm artifact is no longer enumerateable by ListArtifacts and returns length == 0 without error req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/ListArtifacts", toProtoJSON(&actions.ListArtifactsRequest{ @@ -576,7 +576,7 @@ func TestActionsArtifactV4Delete(t *testing.T) { resp = MakeRequest(t, req, http.StatusOK) var listResp actions.ListArtifactsResponse protojson.Unmarshal(resp.Body.Bytes(), &listResp) - assert.Len(t, listResp.Artifacts, 0) + assert.Empty(t, listResp.Artifacts) } func TestActionsArtifactV4DeletePublicApi(t *testing.T) { From bf03ae74244eebefc9603658a13d797ed63d7f2b Mon Sep 17 00:00:00 2001 From: Christopher Homberger Date: Thu, 24 Apr 2025 17:37:21 +0200 Subject: [PATCH 4/4] more detailed errors --- models/actions/artifact.go | 19 +++++++++++++++++++ routers/api/actions/artifacts.go | 2 +- routers/api/actions/artifactsv4.go | 4 ++-- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/models/actions/artifact.go b/models/actions/artifact.go index 524224f0701c5..757bd13acd7f1 100644 --- a/models/actions/artifact.go +++ b/models/actions/artifact.go @@ -30,6 +30,25 @@ const ( ArtifactStatusDeleted // 6, ArtifactStatusDeleted is the status of an artifact that is deleted ) +func (status ArtifactStatus) ToString() string { + switch status { + case ArtifactStatusUploadPending: + return "upload is not yet completed" + case ArtifactStatusUploadConfirmed: + return "upload is completed" + case ArtifactStatusUploadError: + return "upload failed" + case ArtifactStatusExpired: + return "expired" + case ArtifactStatusPendingDeletion: + return "pending deletion" + case ArtifactStatusDeleted: + return "deleted" + default: + return "unknown" + } +} + func init() { db.RegisterModel(new(ActionArtifact)) } diff --git a/routers/api/actions/artifacts.go b/routers/api/actions/artifacts.go index 573cfa1e81778..6473659e5cb30 100644 --- a/routers/api/actions/artifacts.go +++ b/routers/api/actions/artifacts.go @@ -478,7 +478,7 @@ func (ar artifactRoutes) downloadArtifact(ctx *ArtifactContext) { return } if artifact.Status != actions.ArtifactStatusUploadConfirmed { - log.Error("Error artifact not found: unconfirmed") + log.Error("Error artifact not found: %s", artifact.Status.ToString()) ctx.HTTPError(http.StatusNotFound, "Error artifact not found") return } diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index f0dbf08cada06..e9e9fc6393405 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -509,7 +509,7 @@ func (r *artifactV4Routes) getSignedArtifactURL(ctx *ArtifactContext) { return } if artifact.Status != actions.ArtifactStatusUploadConfirmed { - log.Error("Error artifact not found: unconfirmed") + log.Error("Error artifact not found: %s", artifact.Status.ToString()) ctx.HTTPError(http.StatusNotFound, "Error artifact not found") return } @@ -542,7 +542,7 @@ func (r *artifactV4Routes) downloadArtifact(ctx *ArtifactContext) { return } if artifact.Status != actions.ArtifactStatusUploadConfirmed { - log.Error("Error artifact not found: unconfirmed") + log.Error("Error artifact not found: %s", artifact.Status.ToString()) ctx.HTTPError(http.StatusNotFound, "Error artifact not found") return }