From 8f0a05a7e47a1ed333bfdde01ee2e4d8580d39d0 Mon Sep 17 00:00:00 2001 From: ChristopherHX Date: Sun, 22 Sep 2024 13:01:09 +0200 Subject: [PATCH 01/10] Fix artifact v4 upload above 8MB (#31664) Multiple chunks are uploaded with type "block" without using "appendBlock" and eventually out of order for bigger uploads. 8MB seems to be the chunk size This change parses the blockList uploaded after all blocks to get the final artifact size and order them correctly before calculating the sha256 checksum over all blocks Fixes #31354 (cherry picked from commit b594cec2bda6f861effedb2e8e0a7ebba191c0e9) Conflicts: routers/api/actions/artifactsv4.go conflict because of Refactor AppURL usage (#30885) 67c1a07285008cc00036a87cef966c3bd519a50c that was not cherry-picked in Forgejo the resolution consist of removing the extra ctx argument --- routers/api/actions/artifacts_chunks.go | 50 +++++- routers/api/actions/artifactsv4.go | 144 +++++++++++++----- .../api_actions_artifact_v4_test.go | 130 ++++++++++++++++ 3 files changed, 285 insertions(+), 39 deletions(-) diff --git a/routers/api/actions/artifacts_chunks.go b/routers/api/actions/artifacts_chunks.go index b0c96585cb..cdb56584b8 100644 --- a/routers/api/actions/artifacts_chunks.go +++ b/routers/api/actions/artifacts_chunks.go @@ -123,6 +123,54 @@ func listChunksByRunID(st storage.ObjectStorage, runID int64) (map[int64][]*chun return chunksMap, nil } +func listChunksByRunIDV4(st storage.ObjectStorage, runID, artifactID int64, blist *BlockList) ([]*chunkFileItem, error) { + storageDir := fmt.Sprintf("tmpv4%d", runID) + var chunks []*chunkFileItem + chunkMap := map[string]*chunkFileItem{} + dummy := &chunkFileItem{} + for _, name := range blist.Latest { + chunkMap[name] = dummy + } + if err := st.IterateObjects(storageDir, func(fpath string, obj storage.Object) error { + baseName := filepath.Base(fpath) + if !strings.HasPrefix(baseName, "block-") { + return nil + } + // when read chunks from storage, it only contains storage dir and basename, + // no matter the subdirectory setting in storage config + item := chunkFileItem{Path: storageDir + "/" + baseName, ArtifactID: artifactID} + var size int64 + var b64chunkName string + if _, err := fmt.Sscanf(baseName, "block-%d-%d-%s", &item.RunID, &size, &b64chunkName); err != nil { + return fmt.Errorf("parse content range error: %v", err) + } + rchunkName, err := base64.URLEncoding.DecodeString(b64chunkName) + if err != nil { + return fmt.Errorf("failed to parse chunkName: %v", err) + } + chunkName := string(rchunkName) + item.End = item.Start + size - 1 + if _, ok := chunkMap[chunkName]; ok { + chunkMap[chunkName] = &item + } + return nil + }); err != nil { + return nil, err + } + for i, name := range blist.Latest { + chunk, ok := chunkMap[name] + if !ok || chunk.Path == "" { + return nil, fmt.Errorf("missing Chunk (%d/%d): %s", i, len(blist.Latest), name) + } + chunks = append(chunks, chunk) + if i > 0 { + chunk.Start = chunkMap[blist.Latest[i-1]].End + 1 + chunk.End += chunk.Start + } + } + return chunks, nil +} + func mergeChunksForRun(ctx *ArtifactContext, st storage.ObjectStorage, runID int64, artifactName string) error { // read all db artifacts by name artifacts, err := db.Find[actions.ActionArtifact](ctx, actions.FindArtifactsOptions{ @@ -230,7 +278,7 @@ func mergeChunksForArtifact(ctx *ArtifactContext, chunks []*chunkFileItem, st st rawChecksum := hash.Sum(nil) actualChecksum := hex.EncodeToString(rawChecksum) if !strings.HasSuffix(checksum, actualChecksum) { - return fmt.Errorf("update artifact error checksum is invalid") + return fmt.Errorf("update artifact error checksum is invalid %v vs %v", checksum, actualChecksum) } } diff --git a/routers/api/actions/artifactsv4.go b/routers/api/actions/artifactsv4.go index 7b2f9c4360..677e89da2f 100644 --- a/routers/api/actions/artifactsv4.go +++ b/routers/api/actions/artifactsv4.go @@ -24,8 +24,15 @@ package actions // PUT: http://localhost:3000/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact?sig=mO7y35r4GyjN7fwg0DTv3-Fv1NDXD84KLEgLpoPOtDI=&expires=2024-01-23+21%3A48%3A37.20833956+%2B0100+CET&artifactName=test&taskID=75&comp=block // 1.3. Continue Upload Zip Content to Blobstorage (unauthenticated request), repeat until everything is uploaded // PUT: http://localhost:3000/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact?sig=mO7y35r4GyjN7fwg0DTv3-Fv1NDXD84KLEgLpoPOtDI=&expires=2024-01-23+21%3A48%3A37.20833956+%2B0100+CET&artifactName=test&taskID=75&comp=appendBlock -// 1.4. Unknown xml payload to Blobstorage (unauthenticated request), ignored for now +// 1.4. BlockList xml payload to Blobstorage (unauthenticated request) +// Files of about 800MB are parallel in parallel and / or out of order, this file is needed to enshure the correct order // PUT: http://localhost:3000/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact?sig=mO7y35r4GyjN7fwg0DTv3-Fv1NDXD84KLEgLpoPOtDI=&expires=2024-01-23+21%3A48%3A37.20833956+%2B0100+CET&artifactName=test&taskID=75&comp=blockList +// Request +// +// +// blockId1 +// blockId2 +// // 1.5. FinalizeArtifact // Post: /twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact // Request @@ -82,6 +89,7 @@ import ( "crypto/hmac" "crypto/sha256" "encoding/base64" + "encoding/xml" "fmt" "io" "net/http" @@ -153,31 +161,34 @@ func ArtifactsV4Routes(prefix string) *web.Route { return m } -func (r artifactV4Routes) buildSignature(endp, expires, artifactName string, taskID int64) []byte { +func (r artifactV4Routes) buildSignature(endp, expires, artifactName string, taskID, artifactID int64) []byte { mac := hmac.New(sha256.New, setting.GetGeneralTokenSigningSecret()) mac.Write([]byte(endp)) mac.Write([]byte(expires)) mac.Write([]byte(artifactName)) mac.Write([]byte(fmt.Sprint(taskID))) + mac.Write([]byte(fmt.Sprint(artifactID))) return mac.Sum(nil) } -func (r artifactV4Routes) buildArtifactURL(endp, artifactName string, taskID int64) string { +func (r artifactV4Routes) buildArtifactURL(endp, artifactName string, taskID, artifactID int64) string { expires := time.Now().Add(60 * time.Minute).Format("2006-01-02 15:04:05.999999999 -0700 MST") uploadURL := strings.TrimSuffix(setting.AppURL, "/") + strings.TrimSuffix(r.prefix, "/") + - "/" + endp + "?sig=" + base64.URLEncoding.EncodeToString(r.buildSignature(endp, expires, artifactName, taskID)) + "&expires=" + url.QueryEscape(expires) + "&artifactName=" + url.QueryEscape(artifactName) + "&taskID=" + fmt.Sprint(taskID) + "/" + endp + "?sig=" + base64.URLEncoding.EncodeToString(r.buildSignature(endp, expires, artifactName, taskID, artifactID)) + "&expires=" + url.QueryEscape(expires) + "&artifactName=" + url.QueryEscape(artifactName) + "&taskID=" + fmt.Sprint(taskID) + "&artifactID=" + fmt.Sprint(artifactID) return uploadURL } func (r artifactV4Routes) verifySignature(ctx *ArtifactContext, endp string) (*actions.ActionTask, string, bool) { rawTaskID := ctx.Req.URL.Query().Get("taskID") + rawArtifactID := ctx.Req.URL.Query().Get("artifactID") sig := ctx.Req.URL.Query().Get("sig") expires := ctx.Req.URL.Query().Get("expires") artifactName := ctx.Req.URL.Query().Get("artifactName") dsig, _ := base64.URLEncoding.DecodeString(sig) taskID, _ := strconv.ParseInt(rawTaskID, 10, 64) + artifactID, _ := strconv.ParseInt(rawArtifactID, 10, 64) - expecedsig := r.buildSignature(endp, expires, artifactName, taskID) + expecedsig := r.buildSignature(endp, expires, artifactName, taskID, artifactID) if !hmac.Equal(dsig, expecedsig) { log.Error("Error unauthorized") ctx.Error(http.StatusUnauthorized, "Error unauthorized") @@ -272,6 +283,8 @@ func (r *artifactV4Routes) createArtifact(ctx *ArtifactContext) { return } artifact.ContentEncoding = ArtifactV4ContentEncoding + artifact.FileSize = 0 + artifact.FileCompressedSize = 0 if err := actions.UpdateArtifactByID(ctx, artifact.ID, artifact); err != nil { log.Error("Error UpdateArtifactByID: %v", err) ctx.Error(http.StatusInternalServerError, "Error UpdateArtifactByID") @@ -280,7 +293,7 @@ func (r *artifactV4Routes) createArtifact(ctx *ArtifactContext) { respData := CreateArtifactResponse{ Ok: true, - SignedUploadUrl: r.buildArtifactURL("UploadArtifact", artifactName, ctx.ActionTask.ID), + SignedUploadUrl: r.buildArtifactURL("UploadArtifact", artifactName, ctx.ActionTask.ID, artifact.ID), } r.sendProtbufBody(ctx, &respData) } @@ -306,38 +319,77 @@ func (r *artifactV4Routes) uploadArtifact(ctx *ArtifactContext) { comp := ctx.Req.URL.Query().Get("comp") switch comp { case "block", "appendBlock": - // get artifact by name - artifact, err := r.getArtifactByName(ctx, task.Job.RunID, artifactName) - if err != nil { - log.Error("Error artifact not found: %v", err) - ctx.Error(http.StatusNotFound, "Error artifact not found") - return - } + blockid := ctx.Req.URL.Query().Get("blockid") + if blockid == "" { + // get artifact by name + artifact, err := r.getArtifactByName(ctx, task.Job.RunID, artifactName) + if err != nil { + log.Error("Error artifact not found: %v", err) + ctx.Error(http.StatusNotFound, "Error artifact not found") + return + } - if comp == "block" { - artifact.FileSize = 0 - artifact.FileCompressedSize = 0 + _, err = appendUploadChunk(r.fs, ctx, artifact, artifact.FileSize, ctx.Req.ContentLength, artifact.RunID) + if err != nil { + log.Error("Error runner api getting task: task is not running") + ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") + return + } + artifact.FileCompressedSize += ctx.Req.ContentLength + artifact.FileSize += ctx.Req.ContentLength + if err := actions.UpdateArtifactByID(ctx, artifact.ID, artifact); err != nil { + log.Error("Error UpdateArtifactByID: %v", err) + ctx.Error(http.StatusInternalServerError, "Error UpdateArtifactByID") + return + } + } else { + _, err := r.fs.Save(fmt.Sprintf("tmpv4%d/block-%d-%d-%s", task.Job.RunID, task.Job.RunID, ctx.Req.ContentLength, base64.URLEncoding.EncodeToString([]byte(blockid))), ctx.Req.Body, -1) + if err != nil { + log.Error("Error runner api getting task: task is not running") + ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") + return + } } - - _, err = appendUploadChunk(r.fs, ctx, artifact, artifact.FileSize, ctx.Req.ContentLength, artifact.RunID) + ctx.JSON(http.StatusCreated, "appended") + case "blocklist": + rawArtifactID := ctx.Req.URL.Query().Get("artifactID") + artifactID, _ := strconv.ParseInt(rawArtifactID, 10, 64) + _, err := r.fs.Save(fmt.Sprintf("tmpv4%d/%d-%d-blocklist", task.Job.RunID, task.Job.RunID, artifactID), ctx.Req.Body, -1) if err != nil { log.Error("Error runner api getting task: task is not running") ctx.Error(http.StatusInternalServerError, "Error runner api getting task: task is not running") return } - artifact.FileCompressedSize += ctx.Req.ContentLength - artifact.FileSize += ctx.Req.ContentLength - if err := actions.UpdateArtifactByID(ctx, artifact.ID, artifact); err != nil { - log.Error("Error UpdateArtifactByID: %v", err) - ctx.Error(http.StatusInternalServerError, "Error UpdateArtifactByID") - return - } - ctx.JSON(http.StatusCreated, "appended") - case "blocklist": ctx.JSON(http.StatusCreated, "created") } } +type BlockList struct { + Latest []string `xml:"Latest"` +} + +type Latest struct { + Value string `xml:",chardata"` +} + +func (r *artifactV4Routes) readBlockList(runID, artifactID int64) (*BlockList, error) { + blockListName := fmt.Sprintf("tmpv4%d/%d-%d-blocklist", runID, runID, artifactID) + s, err := r.fs.Open(blockListName) + if err != nil { + return nil, err + } + + xdec := xml.NewDecoder(s) + blockList := &BlockList{} + err = xdec.Decode(blockList) + + delerr := r.fs.Delete(blockListName) + if delerr != nil { + log.Warn("Failed to delete blockList %s: %v", blockListName, delerr) + } + return blockList, err +} + func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) { var req FinalizeArtifactRequest @@ -356,18 +408,34 @@ func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) { ctx.Error(http.StatusNotFound, "Error artifact not found") return } - chunkMap, err := listChunksByRunID(r.fs, runID) + + var chunks []*chunkFileItem + blockList, err := r.readBlockList(runID, artifact.ID) if err != nil { - log.Error("Error merge chunks: %v", err) - ctx.Error(http.StatusInternalServerError, "Error merge chunks") - return - } - chunks, ok := chunkMap[artifact.ID] - if !ok { - log.Error("Error merge chunks") - ctx.Error(http.StatusInternalServerError, "Error merge chunks") - return + log.Warn("Failed to read BlockList, fallback to old behavior: %v", err) + chunkMap, err := listChunksByRunID(r.fs, runID) + if err != nil { + log.Error("Error merge chunks: %v", err) + ctx.Error(http.StatusInternalServerError, "Error merge chunks") + return + } + chunks, ok = chunkMap[artifact.ID] + if !ok { + log.Error("Error merge chunks") + ctx.Error(http.StatusInternalServerError, "Error merge chunks") + return + } + } else { + chunks, err = listChunksByRunIDV4(r.fs, runID, artifact.ID, blockList) + if err != nil { + log.Error("Error merge chunks: %v", err) + ctx.Error(http.StatusInternalServerError, "Error merge chunks") + return + } + artifact.FileSize = chunks[len(chunks)-1].End + 1 + artifact.FileCompressedSize = chunks[len(chunks)-1].End + 1 } + checksum := "" if req.Hash != nil { checksum = req.Hash.Value @@ -468,7 +536,7 @@ func (r *artifactV4Routes) getSignedArtifactURL(ctx *ArtifactContext) { } } if respData.SignedUrl == "" { - respData.SignedUrl = r.buildArtifactURL("DownloadArtifact", artifactName, ctx.ActionTask.ID) + respData.SignedUrl = r.buildArtifactURL("DownloadArtifact", artifactName, ctx.ActionTask.ID, artifact.ID) } r.sendProtbufBody(ctx, &respData) } diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index b2c25a2e70..96668b1ddf 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -7,12 +7,14 @@ import ( "bytes" "crypto/sha256" "encoding/hex" + "encoding/xml" "io" "net/http" "strings" "testing" "time" + "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/routers/api/actions" actions_service "code.gitea.io/gitea/services/actions" "code.gitea.io/gitea/tests" @@ -175,6 +177,134 @@ func TestActionsArtifactV4UploadSingleFileWithRetentionDays(t *testing.T) { assert.True(t, finalizeResp.Ok) } +func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + token, err := actions_service.CreateAuthorizationToken(48, 792, 193) + assert.NoError(t, err) + + // acquire artifact upload url + req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", toProtoJSON(&actions.CreateArtifactRequest{ + Version: 4, + Name: "artifactWithPotentialHarmfulBlockID", + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + var uploadResp actions.CreateArtifactResponse + protojson.Unmarshal(resp.Body.Bytes(), &uploadResp) + assert.True(t, uploadResp.Ok) + assert.Contains(t, uploadResp.SignedUploadUrl, "/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact") + + // get upload urls + idx := strings.Index(uploadResp.SignedUploadUrl, "/twirp/") + url := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=%2f..%2fmyfile" + blockListURL := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist" + + // upload artifact chunk + body := strings.Repeat("A", 1024) + req = NewRequestWithBody(t, "PUT", url, strings.NewReader(body)) + MakeRequest(t, req, http.StatusCreated) + + // verify that the exploit didn't work + _, err = storage.Actions.Stat("myfile") + assert.Error(t, err) + + // upload artifact blockList + blockList := &actions.BlockList{ + Latest: []string{ + "/../myfile", + }, + } + rawBlockList, err := xml.Marshal(blockList) + assert.NoError(t, err) + req = NewRequestWithBody(t, "PUT", blockListURL, bytes.NewReader(rawBlockList)) + MakeRequest(t, req, http.StatusCreated) + + t.Logf("Create artifact confirm") + + sha := sha256.Sum256([]byte(body)) + + // confirm artifact upload + req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact", toProtoJSON(&actions.FinalizeArtifactRequest{ + Name: "artifactWithPotentialHarmfulBlockID", + Size: 1024, + Hash: wrapperspb.String("sha256:" + hex.EncodeToString(sha[:])), + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + var finalizeResp actions.FinalizeArtifactResponse + protojson.Unmarshal(resp.Body.Bytes(), &finalizeResp) + assert.True(t, finalizeResp.Ok) +} + +func TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder(t *testing.T) { + defer tests.PrepareTestEnv(t)() + + token, err := actions_service.CreateAuthorizationToken(48, 792, 193) + assert.NoError(t, err) + + // acquire artifact upload url + req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", toProtoJSON(&actions.CreateArtifactRequest{ + Version: 4, + Name: "artifactWithChunksOutOfOrder", + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })).AddTokenAuth(token) + resp := MakeRequest(t, req, http.StatusOK) + var uploadResp actions.CreateArtifactResponse + protojson.Unmarshal(resp.Body.Bytes(), &uploadResp) + assert.True(t, uploadResp.Ok) + assert.Contains(t, uploadResp.SignedUploadUrl, "/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact") + + // get upload urls + idx := strings.Index(uploadResp.SignedUploadUrl, "/twirp/") + block1URL := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=block1" + block2URL := uploadResp.SignedUploadUrl[idx:] + "&comp=block&blockid=block2" + blockListURL := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist" + + // upload artifact chunks + bodyb := strings.Repeat("B", 1024) + req = NewRequestWithBody(t, "PUT", block2URL, strings.NewReader(bodyb)) + MakeRequest(t, req, http.StatusCreated) + + bodya := strings.Repeat("A", 1024) + req = NewRequestWithBody(t, "PUT", block1URL, strings.NewReader(bodya)) + MakeRequest(t, req, http.StatusCreated) + + // upload artifact blockList + blockList := &actions.BlockList{ + Latest: []string{ + "block1", + "block2", + }, + } + rawBlockList, err := xml.Marshal(blockList) + assert.NoError(t, err) + req = NewRequestWithBody(t, "PUT", blockListURL, bytes.NewReader(rawBlockList)) + MakeRequest(t, req, http.StatusCreated) + + t.Logf("Create artifact confirm") + + sha := sha256.Sum256([]byte(bodya + bodyb)) + + // confirm artifact upload + req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact", toProtoJSON(&actions.FinalizeArtifactRequest{ + Name: "artifactWithChunksOutOfOrder", + Size: 2048, + Hash: wrapperspb.String("sha256:" + hex.EncodeToString(sha[:])), + WorkflowRunBackendId: "792", + WorkflowJobRunBackendId: "193", + })). + AddTokenAuth(token) + resp = MakeRequest(t, req, http.StatusOK) + var finalizeResp actions.FinalizeArtifactResponse + protojson.Unmarshal(resp.Body.Bytes(), &finalizeResp) + assert.True(t, finalizeResp.Ok) +} + func TestActionsArtifactV4DownloadSingle(t *testing.T) { defer tests.PrepareTestEnv(t)() From a226064711899da07d6b1455a68ef758f2f3e7e0 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 29 Sep 2024 09:58:47 +0200 Subject: [PATCH 02/10] Fix artifact v4 upload above 8MB (#31664) (fix lint errors) --- tests/integration/api_actions_artifact_v4_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/integration/api_actions_artifact_v4_test.go b/tests/integration/api_actions_artifact_v4_test.go index 96668b1ddf..f55250f6c1 100644 --- a/tests/integration/api_actions_artifact_v4_test.go +++ b/tests/integration/api_actions_artifact_v4_test.go @@ -181,7 +181,7 @@ func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing defer tests.PrepareTestEnv(t)() token, err := actions_service.CreateAuthorizationToken(48, 792, 193) - assert.NoError(t, err) + require.NoError(t, err) // acquire artifact upload url req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", toProtoJSON(&actions.CreateArtifactRequest{ @@ -208,7 +208,7 @@ func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing // verify that the exploit didn't work _, err = storage.Actions.Stat("myfile") - assert.Error(t, err) + require.Error(t, err) // upload artifact blockList blockList := &actions.BlockList{ @@ -217,7 +217,7 @@ func TestActionsArtifactV4UploadSingleFileWithPotentialHarmfulBlockID(t *testing }, } rawBlockList, err := xml.Marshal(blockList) - assert.NoError(t, err) + require.NoError(t, err) req = NewRequestWithBody(t, "PUT", blockListURL, bytes.NewReader(rawBlockList)) MakeRequest(t, req, http.StatusCreated) @@ -244,7 +244,7 @@ func TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder(t *testing.T) { defer tests.PrepareTestEnv(t)() token, err := actions_service.CreateAuthorizationToken(48, 792, 193) - assert.NoError(t, err) + require.NoError(t, err) // acquire artifact upload url req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", toProtoJSON(&actions.CreateArtifactRequest{ @@ -282,7 +282,7 @@ func TestActionsArtifactV4UploadSingleFileWithChunksOutOfOrder(t *testing.T) { }, } rawBlockList, err := xml.Marshal(blockList) - assert.NoError(t, err) + require.NoError(t, err) req = NewRequestWithBody(t, "PUT", blockListURL, bytes.NewReader(rawBlockList)) MakeRequest(t, req, http.StatusCreated) From ff65d34f03f8596d3981461b0c95dbb684fb51b6 Mon Sep 17 00:00:00 2001 From: cloudchamb3r Date: Tue, 24 Sep 2024 02:09:57 +0900 Subject: [PATCH 03/10] Fix Bug in Issue/pulls list (#32081) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix #32080 ## After ### for opened issues Screenshot 2024-09-19 at 6 29 31 PM ### for closed issues Screenshot 2024-09-19 at 6 29 37 PM ### for all issues Screenshot 2024-09-20 at 12 07 12 PM (cherry picked from commit e1f0598c8f5af5ac95f5e13b74fbab99506762db) --- routers/web/repo/issue.go | 1 + templates/repo/issue/filter_actions.tmpl | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 01fd1e2725..5d13ccc97c 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -476,6 +476,7 @@ func issues(ctx *context.Context, milestoneID, projectID int64, isPullOption opt ctx.Data["PosterID"] = posterID ctx.Data["IsFuzzy"] = isFuzzy ctx.Data["Keyword"] = keyword + ctx.Data["IsShowClosed"] = isShowClosed switch { case isShowClosed.Value(): ctx.Data["State"] = "closed" diff --git a/templates/repo/issue/filter_actions.tmpl b/templates/repo/issue/filter_actions.tmpl index a341448bcc..58b1ef8ecd 100644 --- a/templates/repo/issue/filter_actions.tmpl +++ b/templates/repo/issue/filter_actions.tmpl @@ -1,9 +1,9 @@
{{if not .Repository.IsArchived}} - {{if .IsShowClosed}} + {{if and .IsShowClosed.Has .IsShowClosed.Value}} - {{else}} + {{else if and .IsShowClosed.Has (not .IsShowClosed.Value)}} {{end}} {{if $.IsRepoAdmin}} From 65b313626181b0e9fa4a763cd750a2df12fb5459 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 24 Sep 2024 09:30:05 +0800 Subject: [PATCH 04/10] Fix panic when cloning with wrong ssh format. (#32076) (cherry picked from commit 3f2d8f873035b614b4cdb447d8e16f5af82cefe8) --- cmd/serv.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/cmd/serv.go b/cmd/serv.go index 0e3006b36b..db67e36fa3 100644 --- a/cmd/serv.go +++ b/cmd/serv.go @@ -147,6 +147,12 @@ func runServ(c *cli.Context) error { return nil } + defer func() { + if err := recover(); err != nil { + _ = fail(ctx, "Internal Server Error", "Panic: %v\n%s", err, log.Stack(2)) + } + }() + keys := strings.Split(c.Args().First(), "-") if len(keys) != 2 || keys[0] != "key" { return fail(ctx, "Key ID format error", "Invalid key argument: %s", c.Args().First()) @@ -193,10 +199,7 @@ func runServ(c *cli.Context) error { } verb := words[0] - repoPath := words[1] - if repoPath[0] == '/' { - repoPath = repoPath[1:] - } + repoPath := strings.TrimPrefix(words[1], "/") var lfsVerb string if verb == lfsAuthenticateVerb { From 30b8b45e5e17f709198d58a1d7ea2448728bd9e4 Mon Sep 17 00:00:00 2001 From: Kemal Zebari <60799661+kemzeb@users.noreply.github.com> Date: Mon, 23 Sep 2024 20:38:08 -0700 Subject: [PATCH 05/10] Truncate commit message during Discord webhook push events (#31970) Resolves #31668. (cherry picked from commit aadbe0488f454b9f7f5a56765f4530f9d1e2c6ec) --- services/webhook/discord.go | 11 +++++++++-- services/webhook/discord_test.go | 14 ++++++++++++++ services/webhook/general_test.go | 10 +++++++++- 3 files changed, 32 insertions(+), 3 deletions(-) diff --git a/services/webhook/discord.go b/services/webhook/discord.go index b342b45690..af1dd79927 100644 --- a/services/webhook/discord.go +++ b/services/webhook/discord.go @@ -12,6 +12,7 @@ import ( "net/url" "strconv" "strings" + "unicode/utf8" webhook_model "code.gitea.io/gitea/models/webhook" "code.gitea.io/gitea/modules/git" @@ -179,8 +180,14 @@ func (d discordConvertor) Push(p *api.PushPayload) (DiscordPayload, error) { var text string // for each commit, generate attachment text for i, commit := range p.Commits { - text += fmt.Sprintf("[%s](%s) %s - %s", commit.ID[:7], commit.URL, - strings.TrimRight(commit.Message, "\r\n"), commit.Author.Name) + // limit the commit message display to just the summary, otherwise it would be hard to read + message := strings.TrimRight(strings.SplitN(commit.Message, "\n", 1)[0], "\r") + + // a limit of 50 is set because GitHub does the same + if utf8.RuneCountInString(message) > 50 { + message = fmt.Sprintf("%.47s...", message) + } + text += fmt.Sprintf("[%s](%s) %s - %s", commit.ID[:7], commit.URL, message, commit.Author.Name) // add linebreak to each commit but the last if i < len(p.Commits)-1 { text += "\n" diff --git a/services/webhook/discord_test.go b/services/webhook/discord_test.go index 73be143f46..cc5ec8a3e0 100644 --- a/services/webhook/discord_test.go +++ b/services/webhook/discord_test.go @@ -80,6 +80,20 @@ func TestDiscordPayload(t *testing.T) { assert.Equal(t, p.Sender.AvatarURL, pl.Embeds[0].Author.IconURL) }) + t.Run("PushWithLongCommitMessage", func(t *testing.T) { + p := pushTestMultilineCommitMessagePayload() + + pl, err := dc.Push(p) + require.NoError(t, err) + + assert.Len(t, pl.Embeds, 1) + assert.Equal(t, "[test/repo:test] 2 new commits", pl.Embeds[0].Title) + assert.Equal(t, "[2020558](http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778) This is a commit summary ⚠️⚠️⚠️⚠️ containing 你好... - user1\n[2020558](http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778) This is a commit summary ⚠️⚠️⚠️⚠️ containing 你好... - user1", pl.Embeds[0].Description) + assert.Equal(t, p.Sender.UserName, pl.Embeds[0].Author.Name) + assert.Equal(t, setting.AppURL+p.Sender.UserName, pl.Embeds[0].Author.URL) + assert.Equal(t, p.Sender.AvatarURL, pl.Embeds[0].Author.IconURL) + }) + t.Run("Issue", func(t *testing.T) { p := issueTestPayload() diff --git a/services/webhook/general_test.go b/services/webhook/general_test.go index 5b9bfdc1b2..2d991f8300 100644 --- a/services/webhook/general_test.go +++ b/services/webhook/general_test.go @@ -64,9 +64,17 @@ func forkTestPayload() *api.ForkPayload { } func pushTestPayload() *api.PushPayload { + return pushTestPayloadWithCommitMessage("commit message") +} + +func pushTestMultilineCommitMessagePayload() *api.PushPayload { + return pushTestPayloadWithCommitMessage("This is a commit summary ⚠️⚠️⚠️⚠️ containing 你好 ⚠️⚠️️\n\nThis is the message body.") +} + +func pushTestPayloadWithCommitMessage(message string) *api.PushPayload { commit := &api.PayloadCommit{ ID: "2020558fe2e34debb818a514715839cabd25e778", - Message: "commit message", + Message: message, URL: "http://localhost:3000/test/repo/commit/2020558fe2e34debb818a514715839cabd25e778", Author: &api.PayloadUser{ Name: "user1", From 5ccf79d05c8ba36c8db61d0461ffb238dddd9146 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 24 Sep 2024 15:12:06 +0800 Subject: [PATCH 06/10] Include collaboration repositories on dashboard source/forks/mirrors list (#31946) Fix #13489 In the original implementation, only `All` will display your owned and collaborated repositories. For other filters like `Source`, `Mirrors` and etc. will only display your owned repositories. This PR removed the limitations. Now except `collbrations`, other filters will always display your owned and collaborated repositories. (cherry picked from commit 4947bec8360c152daca23e120eae1732d3848492) --- web_src/js/components/DashboardRepoList.vue | 1 - 1 file changed, 1 deletion(-) diff --git a/web_src/js/components/DashboardRepoList.vue b/web_src/js/components/DashboardRepoList.vue index 69a8ced47d..e5874133b1 100644 --- a/web_src/js/components/DashboardRepoList.vue +++ b/web_src/js/components/DashboardRepoList.vue @@ -78,7 +78,6 @@ const sfc = { searchURL() { return `${this.subUrl}/repo/search?sort=updated&order=desc&uid=${this.uid}&team_id=${this.teamId}&q=${this.searchQuery }&page=${this.page}&limit=${this.searchLimit}&mode=${this.repoTypes[this.reposFilter].searchMode - }${this.reposFilter !== 'all' ? '&exclusive=1' : '' }${this.archivedFilter === 'archived' ? '&archived=true' : ''}${this.archivedFilter === 'unarchived' ? '&archived=false' : '' }${this.privateFilter === 'private' ? '&is_private=true' : ''}${this.privateFilter === 'public' ? '&is_private=false' : '' }`; From b496317b5a2aea970bc94ccf6fcde35cd417ec20 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Tue, 24 Sep 2024 15:42:08 +0800 Subject: [PATCH 07/10] Fix bug when deleting a migrated branch (#32075) After migrating a repository with pull request, the branch is missed and after the pull request merged, the branch cannot be deleted. (cherry picked from commit 5a8568459d22e57cac506465463660526ca6a08f) Conflicts: services/repository/branch.go conflict because of [GITEA] Fix typo in formatting error e71b5a038ecfd80630c98752472c2719b0e9659f --- services/repository/branch.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/services/repository/branch.go b/services/repository/branch.go index 27e50e5ced..f0e7120926 100644 --- a/services/repository/branch.go +++ b/services/repository/branch.go @@ -430,13 +430,12 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R } rawBranch, err := git_model.GetBranch(ctx, repo.ID, branchName) - if err != nil { + if err != nil && !git_model.IsErrBranchNotExist(err) { return fmt.Errorf("GetBranch: %v", err) } - if rawBranch.IsDeleted { - return nil - } + // database branch record not exist or it's a deleted branch + notExist := git_model.IsErrBranchNotExist(err) || rawBranch.IsDeleted commit, err := gitRepo.GetBranchCommit(branchName) if err != nil { @@ -444,8 +443,10 @@ func DeleteBranch(ctx context.Context, doer *user_model.User, repo *repo_model.R } if err := db.WithTx(ctx, func(ctx context.Context) error { - if err := git_model.AddDeletedBranch(ctx, repo.ID, branchName, doer.ID); err != nil { - return err + if !notExist { + if err := git_model.AddDeletedBranch(ctx, repo.ID, branchName, doer.ID); err != nil { + return err + } } return gitRepo.DeleteBranch(branchName, git.DeleteBranchOptions{ From 36af3348bce8a06c0601b3fa80bb1d2231e1bde2 Mon Sep 17 00:00:00 2001 From: yp05327 <576951401@qq.com> Date: Wed, 25 Sep 2024 03:34:08 +0900 Subject: [PATCH 08/10] Fix wrong status of `Set up Job` when first step is skipped (#32120) Fix #32089 (cherry picked from commit 6fa962f409c84477a7a4cf35b4a38a4a93fc3224) --- modules/actions/task_state.go | 51 ++++++++++++++++++------------ modules/actions/task_state_test.go | 19 +++++++++++ 2 files changed, 49 insertions(+), 21 deletions(-) diff --git a/modules/actions/task_state.go b/modules/actions/task_state.go index 31a74be3fd..1f36e021a5 100644 --- a/modules/actions/task_state.go +++ b/modules/actions/task_state.go @@ -18,8 +18,32 @@ func FullSteps(task *actions_model.ActionTask) []*actions_model.ActionTaskStep { return fullStepsOfEmptySteps(task) } - firstStep := task.Steps[0] + // firstStep is the first step that has run or running, not include preStep. + // For example, + // 1. preStep(Success) -> step1(Success) -> step2(Running) -> step3(Waiting) -> postStep(Waiting): firstStep is step1. + // 2. preStep(Success) -> step1(Skipped) -> step2(Success) -> postStep(Success): firstStep is step2. + // 3. preStep(Success) -> step1(Running) -> step2(Waiting) -> postStep(Waiting): firstStep is step1. + // 4. preStep(Success) -> step1(Skipped) -> step2(Skipped) -> postStep(Skipped): firstStep is nil. + // 5. preStep(Success) -> step1(Cancelled) -> step2(Cancelled) -> postStep(Cancelled): firstStep is nil. + var firstStep *actions_model.ActionTaskStep + // lastHasRunStep is the last step that has run. + // For example, + // 1. preStep(Success) -> step1(Success) -> step2(Running) -> step3(Waiting) -> postStep(Waiting): lastHasRunStep is step1. + // 2. preStep(Success) -> step1(Success) -> step2(Success) -> step3(Success) -> postStep(Success): lastHasRunStep is step3. + // 3. preStep(Success) -> step1(Success) -> step2(Failure) -> step3 -> postStep(Waiting): lastHasRunStep is step2. + // So its Stopped is the Started of postStep when there are no more steps to run. + var lastHasRunStep *actions_model.ActionTaskStep + var logIndex int64 + for _, step := range task.Steps { + if firstStep == nil && (step.Status.HasRun() || step.Status.IsRunning()) { + firstStep = step + } + if step.Status.HasRun() { + lastHasRunStep = step + } + logIndex += step.LogLength + } preStep := &actions_model.ActionTaskStep{ Name: preStepName, @@ -28,32 +52,17 @@ func FullSteps(task *actions_model.ActionTask) []*actions_model.ActionTaskStep { Status: actions_model.StatusRunning, } - if firstStep.Status.HasRun() || firstStep.Status.IsRunning() { + // No step has run or is running, so preStep is equal to the task + if firstStep == nil { + preStep.Stopped = task.Stopped + preStep.Status = task.Status + } else { preStep.LogLength = firstStep.LogIndex preStep.Stopped = firstStep.Started preStep.Status = actions_model.StatusSuccess - } else if task.Status.IsDone() { - preStep.Stopped = task.Stopped - preStep.Status = actions_model.StatusFailure - if task.Status.IsSkipped() { - preStep.Status = actions_model.StatusSkipped - } } logIndex += preStep.LogLength - // lastHasRunStep is the last step that has run. - // For example, - // 1. preStep(Success) -> step1(Success) -> step2(Running) -> step3(Waiting) -> postStep(Waiting): lastHasRunStep is step1. - // 2. preStep(Success) -> step1(Success) -> step2(Success) -> step3(Success) -> postStep(Success): lastHasRunStep is step3. - // 3. preStep(Success) -> step1(Success) -> step2(Failure) -> step3 -> postStep(Waiting): lastHasRunStep is step2. - // So its Stopped is the Started of postStep when there are no more steps to run. - var lastHasRunStep *actions_model.ActionTaskStep - for _, step := range task.Steps { - if step.Status.HasRun() { - lastHasRunStep = step - } - logIndex += step.LogLength - } if lastHasRunStep == nil { lastHasRunStep = preStep } diff --git a/modules/actions/task_state_test.go b/modules/actions/task_state_test.go index 28213d781b..ff0fd57195 100644 --- a/modules/actions/task_state_test.go +++ b/modules/actions/task_state_test.go @@ -137,6 +137,25 @@ func TestFullSteps(t *testing.T) { {Name: postStepName, Status: actions_model.StatusSkipped, LogIndex: 0, LogLength: 0, Started: 0, Stopped: 0}, }, }, + { + name: "first step is skipped", + task: &actions_model.ActionTask{ + Steps: []*actions_model.ActionTaskStep{ + {Status: actions_model.StatusSkipped, LogIndex: 0, LogLength: 0, Started: 0, Stopped: 0}, + {Status: actions_model.StatusSuccess, LogIndex: 10, LogLength: 80, Started: 10010, Stopped: 10090}, + }, + Status: actions_model.StatusSuccess, + Started: 10000, + Stopped: 10100, + LogLength: 100, + }, + want: []*actions_model.ActionTaskStep{ + {Name: preStepName, Status: actions_model.StatusSuccess, LogIndex: 0, LogLength: 10, Started: 10000, Stopped: 10010}, + {Status: actions_model.StatusSkipped, LogIndex: 0, LogLength: 0, Started: 0, Stopped: 0}, + {Status: actions_model.StatusSuccess, LogIndex: 10, LogLength: 80, Started: 10010, Stopped: 10090}, + {Name: postStepName, Status: actions_model.StatusSuccess, LogIndex: 90, LogLength: 10, Started: 10090, Stopped: 10100}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 8178d6eaba64d05799fd3b62fa889bd13bee07c7 Mon Sep 17 00:00:00 2001 From: Bruno Sofiato Date: Sat, 28 Sep 2024 17:13:55 -0300 Subject: [PATCH 09/10] Change the code search to sort results by relevance (#32134) Resolves #32129 Signed-off-by: Bruno Sofiato (cherry picked from commit 99d0510cb69c3c53cee05ef0e83ed02389925a90) --- modules/indexer/code/bleve/bleve.go | 2 ++ modules/indexer/code/elasticsearch/elasticsearch.go | 6 ++++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/modules/indexer/code/bleve/bleve.go b/modules/indexer/code/bleve/bleve.go index cf9fcbd8b5..ad854094e5 100644 --- a/modules/indexer/code/bleve/bleve.go +++ b/modules/indexer/code/bleve/bleve.go @@ -288,6 +288,8 @@ func (b *Indexer) Search(ctx context.Context, opts *internal.SearchOptions) (int searchRequest.AddFacet("languages", bleve.NewFacetRequest("Language", 10)) } + searchRequest.SortBy([]string{"-_score", "UpdatedAt"}) + result, err := b.inner.Indexer.SearchInContext(ctx, searchRequest) if err != nil { return 0, nil, nil, err diff --git a/modules/indexer/code/elasticsearch/elasticsearch.go b/modules/indexer/code/elasticsearch/elasticsearch.go index 0bda180fac..d64d99433d 100644 --- a/modules/indexer/code/elasticsearch/elasticsearch.go +++ b/modules/indexer/code/elasticsearch/elasticsearch.go @@ -318,7 +318,8 @@ func (b *Indexer) Search(ctx context.Context, opts *internal.SearchOptions) (int NumOfFragments(0). // return all highting content on fragments HighlighterType("fvh"), ). - Sort("repo_id", true). + Sort("_score", false). + Sort("updated_at", true). From(start).Size(pageSize). Do(ctx) if err != nil { @@ -349,7 +350,8 @@ func (b *Indexer) Search(ctx context.Context, opts *internal.SearchOptions) (int NumOfFragments(0). // return all highting content on fragments HighlighterType("fvh"), ). - Sort("repo_id", true). + Sort("_score", false). + Sort("updated_at", true). From(start).Size(pageSize). Do(ctx) if err != nil { From c9ea78eb1ce316f453aeadb6938ff2bf4b23d816 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sun, 29 Sep 2024 11:30:39 +0200 Subject: [PATCH 10/10] chore(release-notes): weekly cherry-pick week 2024-40 --- release-notes/5416.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 release-notes/5416.md diff --git a/release-notes/5416.md b/release-notes/5416.md new file mode 100644 index 0000000000..1368b952ae --- /dev/null +++ b/release-notes/5416.md @@ -0,0 +1,3 @@ +feat: [commit](https://codeberg.org/forgejo/forgejo/commit/8178d6eaba64d05799fd3b62fa889bd13bee07c7) Code search results when using the bleve indexer are sorted by relevance. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/b496317b5a2aea970bc94ccf6fcde35cd417ec20) After migrating a repository that contains merged pull requests, the branch is missing and cannot be deleted. +fix: [commit](https://codeberg.org/forgejo/forgejo/commit/a226064711899da07d6b1455a68ef758f2f3e7e0) Forgejo Actions artifact v4 upload above 8MB.