Add latest changes from gitlab-org/gitlab@master

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)