diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2022-06-22 09:08:52 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2022-06-22 09:08:52 +0300 |
commit | 57b795ee00fbe7a17fa0ad2eb21987eab5fc4aa4 (patch) | |
tree | d88fdfb3b26c0309d4bc0331b4fdab41e2c886dc /workhorse | |
parent | 0c924987e1d6f0453eea407e227efd2122b760fd (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'workhorse')
-rw-r--r-- | workhorse/internal/testhelper/testhelper.go | 7 | ||||
-rw-r--r-- | workhorse/internal/upload/artifacts_store_test.go | 5 | ||||
-rw-r--r-- | workhorse/internal/upload/artifacts_upload_test.go | 15 | ||||
-rw-r--r-- | workhorse/internal/upload/body_uploader_test.go | 25 | ||||
-rw-r--r-- | workhorse/internal/upload/destination/destination.go | 27 | ||||
-rw-r--r-- | workhorse/internal/upload/destination/destination_test.go | 35 | ||||
-rw-r--r-- | workhorse/internal/upload/uploads_test.go | 40 | ||||
-rw-r--r-- | workhorse/upload_test.go | 26 |
8 files changed, 80 insertions, 100 deletions
diff --git a/workhorse/internal/testhelper/testhelper.go b/workhorse/internal/testhelper/testhelper.go index 6ea5c1c73e1..3fe824cbe20 100644 --- a/workhorse/internal/testhelper/testhelper.go +++ b/workhorse/internal/testhelper/testhelper.go @@ -154,6 +154,13 @@ type UploadClaims struct { jwt.RegisteredClaims } +func GetUploadParams(t testing.TB, r *http.Request, name string) map[string]string { + t.Helper() + token, err := jwt.ParseWithClaims(r.PostFormValue(name+".gitlab-workhorse-upload"), &UploadClaims{}, ParseJWT) + require.NoError(t, err) + return token.Claims.(*UploadClaims).Upload +} + func Retry(t testing.TB, timeout time.Duration, fn func() error) { t.Helper() start := time.Now() diff --git a/workhorse/internal/upload/artifacts_store_test.go b/workhorse/internal/upload/artifacts_store_test.go index 7032313fbde..ea757e20d88 100644 --- a/workhorse/internal/upload/artifacts_store_test.go +++ b/workhorse/internal/upload/artifacts_store_test.go @@ -84,8 +84,9 @@ func TestUploadHandlerSendingToExternalStorage(t *testing.T) { responseProcessorCalled := 0 responseProcessor := func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "store-id", r.FormValue("file.remote_id")) - require.NotEmpty(t, r.FormValue("file.remote_url")) + fileParams := testhelper.GetUploadParams(t, r, "file") + require.Equal(t, "store-id", fileParams["remote_id"]) + require.NotEmpty(t, fileParams["remote_url"]) w.WriteHeader(200) responseProcessorCalled++ } diff --git a/workhorse/internal/upload/artifacts_upload_test.go b/workhorse/internal/upload/artifacts_upload_test.go index c94129092c6..f92f2535d0b 100644 --- a/workhorse/internal/upload/artifacts_upload_test.go +++ b/workhorse/internal/upload/artifacts_upload_test.go @@ -65,26 +65,31 @@ func testArtifactsUploadServer(t *testing.T, authResponse *api.Response, bodyPro if r.Method != "POST" { t.Fatal("Expected POST request") } + + fileParams := testhelper.GetUploadParams(t, r, "file") if opts.IsLocalTempFile() { - if r.FormValue("file.path") == "" { + fPath := fileParams["path"] + if fPath == "" { t.Fatal("Expected file to be present") return } - _, err := os.ReadFile(r.FormValue("file.path")) + _, err := os.ReadFile(fPath) if err != nil { t.Fatal("Expected file to be readable") return } } else { - if r.FormValue("file.remote_url") == "" { + if fileParams["remote_url"] == "" { t.Fatal("Expected file to be remote accessible") return } } - if r.FormValue("metadata.path") != "" { - metadata, err := os.ReadFile(r.FormValue("metadata.path")) + if r.FormValue("metadata.gitlab-workhorse-upload") != "" { + metadataParams := testhelper.GetUploadParams(t, r, "metadata") + + metadata, err := os.ReadFile(metadataParams["path"]) if err != nil { t.Fatal("Expected metadata to be readable") return diff --git a/workhorse/internal/upload/body_uploader_test.go b/workhorse/internal/upload/body_uploader_test.go index 837d119e72e..010b1de7427 100644 --- a/workhorse/internal/upload/body_uploader_test.go +++ b/workhorse/internal/upload/body_uploader_test.go @@ -92,15 +92,16 @@ func echoProxy(t *testing.T, expectedBodyLength int) http.Handler { require.Equal(t, "application/x-www-form-urlencoded", r.Header.Get("Content-Type"), "Wrong Content-Type header") - require.Contains(t, r.PostForm, "file.md5") - require.Contains(t, r.PostForm, "file.sha1") - require.Contains(t, r.PostForm, "file.sha256") - require.Contains(t, r.PostForm, "file.sha512") + fileParams := testhelper.GetUploadParams(t, r, "file") + require.Contains(t, fileParams, "md5") + require.Contains(t, fileParams, "sha1") + require.Contains(t, fileParams, "sha256") + require.Contains(t, fileParams, "sha512") - require.Contains(t, r.PostForm, "file.path") - require.Contains(t, r.PostForm, "file.size") - require.Contains(t, r.PostForm, "file.gitlab-workhorse-upload") - require.Equal(t, strconv.Itoa(expectedBodyLength), r.PostFormValue("file.size")) + require.Contains(t, fileParams, "path") + require.Contains(t, fileParams, "size") + + require.Equal(t, strconv.Itoa(expectedBodyLength), fileParams["size"]) token, err := jwt.ParseWithClaims(r.Header.Get(RewrittenFieldsHeader), &MultipartClaims{}, testhelper.ParseJWT) require.NoError(t, err, "Wrong JWT header") @@ -110,10 +111,7 @@ func echoProxy(t *testing.T, expectedBodyLength int) http.Handler { t.Fatalf("Unexpected rewritten_fields value: %v", rewrittenFields) } - token, jwtErr := jwt.ParseWithClaims(r.PostFormValue("file.gitlab-workhorse-upload"), &testhelper.UploadClaims{}, testhelper.ParseJWT) - require.NoError(t, jwtErr, "Wrong signed upload fields") - - uploadFields := token.Claims.(*testhelper.UploadClaims).Upload + uploadFields := testhelper.GetUploadParams(t, r, "file") require.Contains(t, uploadFields, "name") require.Contains(t, uploadFields, "path") require.Contains(t, uploadFields, "remote_url") @@ -124,9 +122,10 @@ func echoProxy(t *testing.T, expectedBodyLength int) http.Handler { require.Contains(t, uploadFields, "sha256") require.Contains(t, uploadFields, "sha512") - path := r.PostFormValue("file.path") + path := uploadFields["path"] uploaded, err := os.Open(path) require.NoError(t, err, "File not uploaded") + defer uploaded.Close() //sending back the file for testing purpose io.Copy(w, uploaded) diff --git a/workhorse/internal/upload/destination/destination.go b/workhorse/internal/upload/destination/destination.go index 5e145e2cb2a..039c534b552 100644 --- a/workhorse/internal/upload/destination/destination.go +++ b/workhorse/internal/upload/destination/destination.go @@ -68,42 +68,31 @@ func (fh *FileHandler) MD5() string { // GitLabFinalizeFields returns a map with all the fields GitLab Rails needs in order to finalize the upload. func (fh *FileHandler) GitLabFinalizeFields(prefix string) (map[string]string, error) { - // TODO: remove `data` these once rails fully and exclusively support `signedData` (https://gitlab.com/gitlab-org/gitlab/-/issues/324873) - data := make(map[string]string) - signedData := make(map[string]string) - key := func(field string) string { - if prefix == "" { - return field - } - - return fmt.Sprintf("%s.%s", prefix, field) - } - - for k, v := range map[string]string{ + signedData := map[string]string{ "name": fh.Name, "path": fh.LocalPath, "remote_url": fh.RemoteURL, "remote_id": fh.RemoteID, "size": strconv.FormatInt(fh.Size, 10), "upload_duration": strconv.FormatFloat(fh.uploadDuration, 'f', -1, 64), - } { - data[key(k)] = v - signedData[k] = v } for hashName, hash := range fh.hashes { - data[key(hashName)] = hash signedData[hashName] = hash } - claims := uploadClaims{Upload: signedData, RegisteredClaims: secret.DefaultClaims} + claims := uploadClaims{ + Upload: signedData, + RegisteredClaims: secret.DefaultClaims, + } jwtData, err := secret.JWTTokenString(claims) if err != nil { return nil, err } - data[key("gitlab-workhorse-upload")] = jwtData - return data, nil + return map[string]string{ + prefix + ".gitlab-workhorse-upload": jwtData, + }, nil } type consumer interface { diff --git a/workhorse/internal/upload/destination/destination_test.go b/workhorse/internal/upload/destination/destination_test.go index 6ebe163468b..f9a78ec8b72 100644 --- a/workhorse/internal/upload/destination/destination_test.go +++ b/workhorse/internal/upload/destination/destination_test.go @@ -3,7 +3,6 @@ package destination_test import ( "context" "errors" - "fmt" "os" "path" "strconv" @@ -220,14 +219,21 @@ func TestUpload(t *testing.T) { fields, err := fh.GitLabFinalizeFields("file") require.NoError(t, err) - checkFileHandlerWithFields(t, fh, fields, "file") - token, jwtErr := jwt.ParseWithClaims(fields["file.gitlab-workhorse-upload"], &testhelper.UploadClaims{}, testhelper.ParseJWT) require.NoError(t, jwtErr) uploadFields := token.Claims.(*testhelper.UploadClaims).Upload - checkFileHandlerWithFields(t, fh, uploadFields, "") + require.Equal(t, fh.Name, uploadFields["name"]) + require.Equal(t, fh.LocalPath, uploadFields["path"]) + require.Equal(t, fh.RemoteURL, uploadFields["remote_url"]) + require.Equal(t, fh.RemoteID, uploadFields["remote_id"]) + require.Equal(t, strconv.FormatInt(test.ObjectSize, 10), uploadFields["size"]) + require.Equal(t, test.ObjectMD5, uploadFields["md5"]) + require.Equal(t, test.ObjectSHA1, uploadFields["sha1"]) + require.Equal(t, test.ObjectSHA256, uploadFields["sha256"]) + require.Equal(t, test.ObjectSHA512, uploadFields["sha512"]) + require.NotEmpty(t, uploadFields["upload_duration"]) }) } } @@ -463,24 +469,3 @@ func TestUploadRemoteFileWithLimit(t *testing.T) { }) } } - -func checkFileHandlerWithFields(t *testing.T, fh *destination.FileHandler, fields map[string]string, prefix string) { - key := func(field string) string { - if prefix == "" { - return field - } - - return fmt.Sprintf("%s.%s", prefix, field) - } - - require.Equal(t, fh.Name, fields[key("name")]) - require.Equal(t, fh.LocalPath, fields[key("path")]) - require.Equal(t, fh.RemoteURL, fields[key("remote_url")]) - require.Equal(t, fh.RemoteID, fields[key("remote_id")]) - require.Equal(t, strconv.FormatInt(test.ObjectSize, 10), fields[key("size")]) - require.Equal(t, test.ObjectMD5, fields[key("md5")]) - require.Equal(t, test.ObjectSHA1, fields[key("sha1")]) - require.Equal(t, test.ObjectSHA256, fields[key("sha256")]) - require.Equal(t, test.ObjectSHA512, fields[key("sha512")]) - require.NotEmpty(t, fields[key("upload_duration")]) -} diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index ffe9fec302e..6ad31c7fe7a 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -89,14 +89,16 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) { require.Empty(t, r.MultipartForm.File, "Expected to not receive any files") require.Equal(t, "test", r.FormValue("token"), "Expected to receive token") - require.Equal(t, "my.file", r.FormValue("file.name"), "Expected to receive a filename") - filePath = r.FormValue("file.path") + fileParams := testhelper.GetUploadParams(t, r, "file") + require.Equal(t, "my.file", fileParams["name"], "Expected to receive a filename") + + filePath = fileParams["path"] require.True(t, strings.HasPrefix(filePath, tempPath), "Expected to the file to be in tempPath") - require.Empty(t, r.FormValue("file.remote_url"), "Expected to receive empty remote_url") - require.Empty(t, r.FormValue("file.remote_id"), "Expected to receive empty remote_id") - require.Equal(t, "4", r.FormValue("file.size"), "Expected to receive the file size") + require.Empty(t, fileParams["remote_url"], "Expected to receive empty remote_url") + require.Empty(t, fileParams["remote_id"], "Expected to receive empty remote_id") + require.Equal(t, "4", fileParams["size"], "Expected to receive the file size") hashes := map[string]string{ "md5": "098f6bcd4621d373cade4e832627b4f6", @@ -106,10 +108,10 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) { } for algo, hash := range hashes { - require.Equal(t, hash, r.FormValue("file."+algo), "file hash %s", algo) + require.Equal(t, hash, fileParams[algo], "file hash %s", algo) } - require.Len(t, r.MultipartForm.Value, 12, "multipart form values") + require.Len(t, fileParams, 10, "multipart form values") w.WriteHeader(202) fmt.Fprint(w, "RESPONSE") @@ -147,7 +149,7 @@ func TestUploadHandlerRewritingMultiPartData(t *testing.T) { } func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) { - var filePath string + testhelper.ConfigureSecret() tests := []struct { name string @@ -155,13 +157,8 @@ func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) { response int }{ { - name: "injected file.path", - field: "file.path", - response: 400, - }, - { - name: "injected file.remote_id", - field: "file.remote_id", + name: "injected file.gitlab-workhorse-upload", + field: "file.gitlab-workhorse-upload", response: 400, }, { @@ -194,6 +191,7 @@ func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) { require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) + defer cancel() httpRequest = httpRequest.WithContext(ctx) httpRequest.Header.Set("Content-Type", writer.FormDataContentType()) response := httptest.NewRecorder() @@ -202,9 +200,6 @@ func TestUploadHandlerDetectingInjectedMultiPartData(t *testing.T) { testInterceptMultipartFiles(t, response, httpRequest, handler, &testFormProcessor{}) require.Equal(t, test.response, response.Code) - - cancel() // this will trigger an async cleanup - waitUntilDeleted(t, filePath) }) } } @@ -425,7 +420,8 @@ func TestUploadHandlerRemovingExif(t *testing.T) { err := r.ParseMultipartForm(100000) require.NoError(t, err) - size, err := strconv.Atoi(r.FormValue("file.size")) + fileParams := testhelper.GetUploadParams(t, r, "file") + size, err := strconv.Atoi(fileParams["size"]) require.NoError(t, err) require.True(t, size < len(content), "Expected the file to be smaller after removal of exif") require.True(t, size > 0, "Expected to receive not empty file") @@ -443,7 +439,8 @@ func TestUploadHandlerRemovingExifTiff(t *testing.T) { err := r.ParseMultipartForm(100000) require.NoError(t, err) - size, err := strconv.Atoi(r.FormValue("file.size")) + fileParams := testhelper.GetUploadParams(t, r, "file") + size, err := strconv.Atoi(fileParams["size"]) require.NoError(t, err) require.True(t, size < len(content), "Expected the file to be smaller after removal of exif") require.True(t, size > 0, "Expected to receive not empty file") @@ -461,7 +458,8 @@ func TestUploadHandlerRemovingExifInvalidContentType(t *testing.T) { err := r.ParseMultipartForm(100000) require.NoError(t, err) - output, err := os.ReadFile(r.FormValue("file.path")) + fileParams := testhelper.GetUploadParams(t, r, "file") + output, err := os.ReadFile(fileParams["path"]) require.NoError(t, err) require.Equal(t, content, output, "Expected the file to be same as before") diff --git a/workhorse/upload_test.go b/workhorse/upload_test.go index dedda4ea655..c4669ade4a1 100644 --- a/workhorse/upload_test.go +++ b/workhorse/upload_test.go @@ -81,9 +81,10 @@ func uploadTestServer(t *testing.T, authorizeTests func(r *http.Request), extraT } require.NoError(t, r.ParseMultipartForm(100000)) + require.Len(t, r.MultipartForm.Value, 1) // Expect 1 key: "file.gitlab-workhorse-upload" - const nValues = 11 // file name, path, remote_url, remote_id, size, md5, sha1, sha256, sha512, upload_duration, gitlab-workhorse-upload for just the upload (no metadata because we are not POSTing a valid zip file) - require.Len(t, r.MultipartForm.Value, nValues) + const nValues = 10 // file name, path, remote_url, remote_id, size, md5, sha1, sha256, sha512, upload_duration (no metadata because we are not POSTing a valid zip file) + require.Len(t, testhelper.GetUploadParams(t, r, "file"), nValues) require.Empty(t, r.MultipartForm.File, "multipart form files") @@ -174,10 +175,7 @@ func TestAcceleratedUpload(t *testing.T) { t.Fatalf("Unexpected rewritten_fields value: %v", rewrittenFields) } - token, jwtErr := jwt.ParseWithClaims(r.PostFormValue("file.gitlab-workhorse-upload"), &testhelper.UploadClaims{}, testhelper.ParseJWT) - require.NoError(t, jwtErr) - - uploadFields := token.Claims.(*testhelper.UploadClaims).Upload + uploadFields := testhelper.GetUploadParams(t, r, "file") require.Contains(t, uploadFields, "name") require.Contains(t, uploadFields, "path") require.Contains(t, uploadFields, "remote_url") @@ -339,12 +337,11 @@ func TestLfsUpload(t *testing.T) { case resource: expectSignedRequest(t, r) - // Expect the request to point to a file on disk containing the data - require.NoError(t, r.ParseForm()) - require.Equal(t, oid, r.Form.Get("file.sha256"), "Invalid SHA256 populated") - require.Equal(t, strconv.Itoa(len(reqBody)), r.Form.Get("file.size"), "Invalid size populated") + fileParams := testhelper.GetUploadParams(t, r, "file") + require.Equal(t, oid, fileParams["sha256"], "Invalid SHA256 populated") + require.Equal(t, strconv.Itoa(len(reqBody)), fileParams["size"], "Invalid size populated") - tempfile, err := os.ReadFile(r.Form.Get("file.path")) + tempfile, err := os.ReadFile(fileParams["path"]) require.NoError(t, err) require.Equal(t, reqBody, string(tempfile), "Temporary file has the wrong body") @@ -462,13 +459,12 @@ func packageUploadTestServer(t *testing.T, method string, resource string, reqBo case resource: expectSignedRequest(t, r) - // Expect the request to point to a file on disk containing the data - require.NoError(t, r.ParseForm()) + fileParams := testhelper.GetUploadParams(t, r, "file") len := strconv.Itoa(len(reqBody)) - require.Equal(t, len, r.Form.Get("file.size"), "Invalid size populated") + require.Equal(t, len, fileParams["size"], "Invalid size populated") - tmpFilePath := r.Form.Get("file.path") + tmpFilePath := fileParams["path"] fileData, err := os.ReadFile(tmpFilePath) defer os.Remove(tmpFilePath) |