From e80f8ff69f269bbc6dbf36dc57ddbddaea69e20a Mon Sep 17 00:00:00 2001 From: ThetaDev Date: Wed, 10 Jul 2024 05:28:01 +0000 Subject: [PATCH] fix artifact range requests (#4218) I noticed that Forgejo does not allow HTTP range requests when downloading artifacts. All other file downloads like releases and packages support them. So I looked at the code and found that the artifact download endpoint uses a simple io.Copy to serve the file contents instead of using the established `ServeContentByReadSeeker` function which does take range requests into account. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4218 Reviewed-by: Earl Warren Reviewed-by: Gusted Co-authored-by: ThetaDev Co-committed-by: ThetaDev --- models/fixtures/repo_unit.yml | 7 +++ routers/api/actions/artifactsv4.go | 10 +++- routers/web/repo/actions/view.go | 8 +-- .../api_actions_artifact_v4_test.go | 59 +++++++++++++++++-- 4 files changed, 73 insertions(+), 11 deletions(-) diff --git a/models/fixtures/repo_unit.yml b/models/fixtures/repo_unit.yml index 6dac78f588..cd49a51796 100644 --- a/models/fixtures/repo_unit.yml +++ b/models/fixtures/repo_unit.yml @@ -788,3 +788,10 @@ type: 10 config: "{}" created_unix: 946684810 + +- + id: 114 + repo_id: 4 + type: 10 + config: "{}" + created_unix: 946684810 diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 5a251e2ef9..57d7f9ad6f 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -97,6 +97,7 @@ import ( "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" + "code.gitea.io/gitea/routers/common" "code.gitea.io/gitea/services/context" "google.golang.org/protobuf/encoding/protojson" @@ -473,9 +474,14 @@ func (r *artifactV4Routes) downloadArtifact(ctx *ArtifactContext) { return } - file, _ := r.fs.Open(artifact.StoragePath) + file, err := r.fs.Open(artifact.StoragePath) + if err != nil { + log.Error("Error artifact could not be opened: %v", err) + ctx.Error(http.StatusInternalServerError, err.Error()) + return + } - _, _ = io.Copy(ctx.Resp, file) + common.ServeContentByReadSeeker(ctx.Base, artifactName, util.ToPointer(artifact.UpdatedUnix.AsTime()), file) } func (r *artifactV4Routes) deleteArtifact(ctx *ArtifactContext) { diff --git a/routers/web/repo/actions/view.go b/routers/web/repo/actions/view.go index e3e0fce3b2..e08e76b78b 100644 --- a/routers/web/repo/actions/view.go +++ b/routers/web/repo/actions/view.go @@ -28,6 +28,7 @@ import ( "code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" + "code.gitea.io/gitea/routers/common" actions_service "code.gitea.io/gitea/services/actions" context_module "code.gitea.io/gitea/services/context" @@ -662,10 +663,8 @@ func ArtifactsDownloadView(ctx *context_module.Context) { } } - ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%s.zip; filename*=UTF-8''%s.zip", url.PathEscape(artifactName), artifactName)) - // Artifacts using the v4 backend are stored as a single combined zip file per artifact on the backend - // The v4 backend enshures ContentEncoding is set to "application/zip", which is not the case for the old backend + // The v4 backend ensures ContentEncoding is set to "application/zip", which is not the case for the old backend if len(artifacts) == 1 && artifacts[0].ArtifactName+".zip" == artifacts[0].ArtifactPath && artifacts[0].ContentEncoding == "application/zip" { art := artifacts[0] if setting.Actions.ArtifactStorage.MinioConfig.ServeDirect { @@ -680,12 +679,13 @@ func ArtifactsDownloadView(ctx *context_module.Context) { ctx.Error(http.StatusInternalServerError, err.Error()) return } - _, _ = io.Copy(ctx.Resp, f) + common.ServeContentByReadSeeker(ctx.Base, artifactName, util.ToPointer(art.UpdatedUnix.AsTime()), f) return } // Artifacts using the v1-v3 backend are stored as multiple individual files per artifact on the backend // Those need to be zipped for download + ctx.Resp.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%s.zip; filename*=UTF-8''%s.zip", url.PathEscape(artifactName), artifactName)) writer := zip.NewWriter(ctx.Resp) defer writer.Close() for _, art := range artifacts { diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index f58f876849..c70e99e2eb 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -31,9 +31,7 @@ func toProtoJSON(m protoreflect.ProtoMessage) io.Reader { return &buf } -func TestActionsArtifactV4UploadSingleFile(t *testing.T) { - defer tests.PrepareTestEnv(t)() - +func uploadArtifact(t *testing.T, body string) string { token, err := actions_service.CreateAuthorizationToken(48, 792, 193) assert.NoError(t, err) @@ -55,7 +53,6 @@ func TestActionsArtifactV4UploadSingleFile(t *testing.T) { url := uploadResp.SignedUploadUrl[idx:] + "&comp=block" // upload artifact chunk - body := strings.Repeat("A", 1024) req = NewRequestWithBody(t, "PUT", url, strings.NewReader(body)) MakeRequest(t, req, http.StatusCreated) @@ -76,6 +73,13 @@ func TestActionsArtifactV4UploadSingleFile(t *testing.T) { var finalizeResp actions.FinalizeArtifactResponse protojson.Unmarshal(resp.Body.Bytes(), &finalizeResp) assert.True(t, finalizeResp.Ok) + return token +} + +func TestActionsArtifactV4UploadSingleFile(t *testing.T) { + defer tests.PrepareTestEnv(t)() + body := strings.Repeat("A", 1024) + uploadArtifact(t, body) } func TestActionsArtifactV4UploadSingleFileWrongChecksum(t *testing.T) { @@ -202,7 +206,52 @@ func TestActionsArtifactV4DownloadSingle(t *testing.T) { req = NewRequest(t, "GET", finalizeResp.SignedUrl) resp = MakeRequest(t, req, http.StatusOK) body := strings.Repeat("A", 1024) - assert.Equal(t, resp.Body.String(), body) + assert.Equal(t, "bytes", resp.Header().Get("accept-ranges")) + assert.Equal(t, body, resp.Body.String()) + + // Download artifact via user-facing URL + req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact") + resp = MakeRequest(t, req, http.StatusOK) + assert.Equal(t, "bytes", resp.Header().Get("accept-ranges")) + assert.Equal(t, body, resp.Body.String()) + + // Partial artifact download + req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact").SetHeader("range", "bytes=0-99") + resp = MakeRequest(t, req, http.StatusPartialContent) + body = strings.Repeat("A", 100) + assert.Equal(t, "bytes 0-99/1024", resp.Header().Get("content-range")) + assert.Equal(t, body, resp.Body.String()) +} + +func TestActionsArtifactV4DownloadRange(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + bstr := strings.Repeat("B", 100) + body := strings.Repeat("A", 100) + bstr + token := uploadArtifact(t, body) + + // Download (Actions API) + req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/GetSignedArtifactURL", toProtoJSON(&actions.GetSignedArtifactURLRequest{ + Name: "artifact", + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })). + AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + var finalizeResp actions.GetSignedArtifactURLResponse + protojson.Unmarshal(resp.Body.Bytes(), &finalizeResp) + assert.NotEmpty(t, finalizeResp.SignedUrl) + + req = NewRequest(t, "GET", finalizeResp.SignedUrl).SetHeader("range", "bytes=100-199") + resp = MakeRequest(t, req, http.StatusPartialContent) + assert.Equal(t, "bytes 100-199/200", resp.Header().Get("content-range")) + assert.Equal(t, bstr, resp.Body.String()) + + // Download (user-facing API) + req = NewRequest(t, "GET", "/user5/repo4/actions/runs/188/artifacts/artifact").SetHeader("range", "bytes=100-199") + resp = MakeRequest(t, req, http.StatusPartialContent) + assert.Equal(t, "bytes 100-199/200", resp.Header().Get("content-range")) + assert.Equal(t, bstr, resp.Body.String()) } func TestActionsArtifactV4Delete(t *testing.T) {