Skip to content

Commit 83fe746

Browse files
ChristopherHXChristopherHX
committed
Ensure sorting of chunks before extracting it's size
* Better error messages for decoding in log
1 parent f2d1845 commit 83fe746

2 files changed

Lines changed: 84 additions & 73 deletions

File tree

‎routers/api/actions/artifactsv4.go‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ import (
9595
"io"
9696
"net/http"
9797
"net/url"
98+
"sort"
9899
"strconv"
99100
"strings"
100101
"time"
@@ -181,10 +182,15 @@ func (r artifactV4Routes) verifySignature(ctx *ArtifactContext, endp string) (*a
181182
sig := ctx.Req.URL.Query().Get("sig")
182183
expires := ctx.Req.URL.Query().Get("expires")
183184
artifactName := ctx.Req.URL.Query().Get("artifactName")
184-
dsig, _ := base64.RawURLEncoding.DecodeString(sig)
185-
taskID, _ := strconv.ParseInt(rawTaskID, 10, 64)
186-
artifactID, _ := strconv.ParseInt(rawArtifactID, 10, 64)
187-
185+
dsig, errSig := base64.RawURLEncoding.DecodeString(sig)
186+
taskID, errTask := strconv.ParseInt(rawTaskID, 10, 64)
187+
artifactID, errArtifactID := strconv.ParseInt(rawArtifactID, 10, 64)
188+
err := errors.Join(errSig, errTask, errArtifactID)
189+
if err != nil {
190+
log.Error("Error decoding signature values: %v", err)
191+
ctx.HTTPError(http.StatusBadRequest, "Error decoding signature values")
192+
return nil, "", false
193+
}
188194
expecedsig := r.buildSignature(endp, expires, artifactName, taskID, artifactID)
189195
if !hmac.Equal(dsig, expecedsig) {
190196
log.Error("Error unauthorized")
@@ -406,6 +412,9 @@ func (r *artifactV4Routes) finalizeArtifact(ctx *ArtifactContext) {
406412
ctx.HTTPError(http.StatusInternalServerError, "Error merge chunks")
407413
return
408414
}
415+
sort.Slice(chunks, func(i, j int) bool {
416+
return chunks[i].Start < chunks[j].Start
417+
})
409418
artifact.FileSize = chunks[len(chunks)-1].End + 1
410419
artifact.FileCompressedSize = chunks[len(chunks)-1].End + 1
411420

‎tests/integration/api_actions_artifact_v4_test.go‎

Lines changed: 71 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -101,78 +101,80 @@ func TestActionsArtifactV4UploadSingleFile(t *testing.T) {
101101
}
102102

103103
for _, entry := range table {
104-
// acquire artifact upload url
105-
req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", toProtoJSON(&actions.CreateArtifactRequest{
106-
Version: entry.version,
107-
Name: entry.name,
108-
WorkflowRunBackendId: "792",
109-
WorkflowJobRunBackendId: "193",
110-
})).AddTokenAuth(token)
111-
resp := MakeRequest(t, req, http.StatusOK)
112-
var uploadResp actions.CreateArtifactResponse
113-
protojson.Unmarshal(resp.Body.Bytes(), &uploadResp)
114-
assert.True(t, uploadResp.Ok)
115-
assert.Contains(t, uploadResp.SignedUploadUrl, "/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact")
116-
117-
h := sha256.New()
118-
119-
blocks := make([]string, 0, util.Iif(entry.blockID, entry.append+1, 0))
120-
121-
// get upload url
122-
idx := strings.Index(uploadResp.SignedUploadUrl, "/twirp/")
123-
for i := range entry.append + 1 {
124-
url := uploadResp.SignedUploadUrl[idx:]
125-
// See https://learn.microsoft.com/en-us/rest/api/storageservices/append-block
126-
// See https://learn.microsoft.com/en-us/rest/api/storageservices/put-block
127-
if entry.blockID {
128-
blockID := base64.RawURLEncoding.EncodeToString(fmt.Append([]byte("SOME_BIG_BLOCK_ID_"), i))
129-
blocks = append(blocks, blockID)
130-
url += "&comp=block&blockid=" + blockID
131-
} else {
132-
url += "&comp=appendBlock"
104+
t.Run(entry.name, func(t *testing.T) {
105+
// acquire artifact upload url
106+
req := NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/CreateArtifact", toProtoJSON(&actions.CreateArtifactRequest{
107+
Version: entry.version,
108+
Name: entry.name,
109+
WorkflowRunBackendId: "792",
110+
WorkflowJobRunBackendId: "193",
111+
})).AddTokenAuth(token)
112+
resp := MakeRequest(t, req, http.StatusOK)
113+
var uploadResp actions.CreateArtifactResponse
114+
protojson.Unmarshal(resp.Body.Bytes(), &uploadResp)
115+
assert.True(t, uploadResp.Ok)
116+
assert.Contains(t, uploadResp.SignedUploadUrl, "/twirp/github.actions.results.api.v1.ArtifactService/UploadArtifact")
117+
118+
h := sha256.New()
119+
120+
blocks := make([]string, 0, util.Iif(entry.blockID, entry.append+1, 0))
121+
122+
// get upload url
123+
idx := strings.Index(uploadResp.SignedUploadUrl, "/twirp/")
124+
for i := range entry.append + 1 {
125+
url := uploadResp.SignedUploadUrl[idx:]
126+
// See https://learn.microsoft.com/en-us/rest/api/storageservices/append-block
127+
// See https://learn.microsoft.com/en-us/rest/api/storageservices/put-block
128+
if entry.blockID {
129+
blockID := base64.RawURLEncoding.EncodeToString(fmt.Append([]byte("SOME_BIG_BLOCK_ID_"), i))
130+
blocks = append(blocks, blockID)
131+
url += "&comp=block&blockid=" + blockID
132+
} else {
133+
url += "&comp=appendBlock"
134+
}
135+
136+
// upload artifact chunk
137+
body := strings.Repeat("A", 1024)
138+
_, _ = h.Write([]byte(body))
139+
var bodyReader io.Reader = strings.NewReader(body)
140+
if entry.noLength {
141+
bodyReader = io.MultiReader(bodyReader)
142+
}
143+
req = NewRequestWithBody(t, "PUT", url, bodyReader)
144+
MakeRequest(t, req, http.StatusCreated)
133145
}
134146

135-
// upload artifact chunk
136-
body := strings.Repeat("A", 1024)
137-
_, _ = h.Write([]byte(body))
138-
var bodyReader io.Reader = strings.NewReader(body)
139-
if entry.noLength {
140-
bodyReader = io.MultiReader(bodyReader)
147+
if entry.blockID && entry.append > 0 {
148+
// https://learn.microsoft.com/en-us/rest/api/storageservices/put-block-list
149+
blockListURL := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist"
150+
// upload artifact blockList
151+
blockList := &actions.BlockList{
152+
Latest: blocks,
153+
}
154+
rawBlockList, err := xml.Marshal(blockList)
155+
assert.NoError(t, err)
156+
req = NewRequestWithBody(t, "PUT", blockListURL, bytes.NewReader(rawBlockList))
157+
MakeRequest(t, req, http.StatusCreated)
141158
}
142-
req = NewRequestWithBody(t, "PUT", url, bodyReader)
143-
MakeRequest(t, req, http.StatusCreated)
144-
}
145-
146-
if entry.blockID && entry.append > 0 {
147-
// https://learn.microsoft.com/en-us/rest/api/storageservices/put-block-list
148-
blockListURL := uploadResp.SignedUploadUrl[idx:] + "&comp=blocklist"
149-
// upload artifact blockList
150-
blockList := &actions.BlockList{
151-
Latest: blocks,
152-
}
153-
rawBlockList, err := xml.Marshal(blockList)
154-
assert.NoError(t, err)
155-
req = NewRequestWithBody(t, "PUT", blockListURL, bytes.NewReader(rawBlockList))
156-
MakeRequest(t, req, http.StatusCreated)
157-
}
158-
159-
sha := h.Sum(nil)
160-
161-
t.Logf("Create artifact confirm")
162-
163-
// confirm artifact upload
164-
req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact", toProtoJSON(&actions.FinalizeArtifactRequest{
165-
Name: entry.name,
166-
Size: int64(entry.append+1) * 1024,
167-
Hash: wrapperspb.String("sha256:" + hex.EncodeToString(sha)),
168-
WorkflowRunBackendId: "792",
169-
WorkflowJobRunBackendId: "193",
170-
})).
171-
AddTokenAuth(token)
172-
resp = MakeRequest(t, req, http.StatusOK)
173-
var finalizeResp actions.FinalizeArtifactResponse
174-
protojson.Unmarshal(resp.Body.Bytes(), &finalizeResp)
175-
assert.True(t, finalizeResp.Ok)
159+
160+
sha := h.Sum(nil)
161+
162+
t.Logf("Create artifact confirm")
163+
164+
// confirm artifact upload
165+
req = NewRequestWithBody(t, "POST", "/twirp/github.actions.results.api.v1.ArtifactService/FinalizeArtifact", toProtoJSON(&actions.FinalizeArtifactRequest{
166+
Name: entry.name,
167+
Size: int64(entry.append+1) * 1024,
168+
Hash: wrapperspb.String("sha256:" + hex.EncodeToString(sha)),
169+
WorkflowRunBackendId: "792",
170+
WorkflowJobRunBackendId: "193",
171+
})).
172+
AddTokenAuth(token)
173+
resp = MakeRequest(t, req, http.StatusOK)
174+
var finalizeResp actions.FinalizeArtifactResponse
175+
protojson.Unmarshal(resp.Body.Bytes(), &finalizeResp)
176+
assert.True(t, finalizeResp.Ok)
177+
})
176178
}
177179
}
178180

0 commit comments

Comments
 (0)