Merge branch 'sh-fix-invalid-multipart-status-code' into 'master'

workhorse: Downgrade incomplete multipart uploads to 400 errors

See merge request https://gitlab.com/gitlab-org/gitlab/-/merge_requests/144710



Merged-by: Patrick Bajao <ebajao@gitlab.com>
Approved-by: Matthias Käppler <mkaeppler@gitlab.com>
Approved-by: Patrick Bajao <ebajao@gitlab.com>
Approved-by: Jaime Martinez <jmartinez@gitlab.com>
Co-authored-by: Stan Hu <stanhu@gmail.com>

workhorse/internal/upload/rewrite.go

@@ -25,8 +25,9 @@ const maxFilesAllowed = 10
 
 // ErrInjectedClientParam means that the client sent a parameter that overrides one of our own fields
 var (
-	ErrInjectedClientParam  = errors.New("injected client parameter")
-	ErrTooManyFilesUploaded = fmt.Errorf("upload request contains more than %v files", maxFilesAllowed)
+	ErrInjectedClientParam    = errors.New("injected client parameter")
+	ErrTooManyFilesUploaded   = fmt.Errorf("upload request contains more than %v files", maxFilesAllowed)
+	ErrUnexpectedMultipartEOF = fmt.Errorf("unexpected EOF when reading multipart data")
 )
 
 var (
@@ -87,6 +88,14 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, fi
 			if err == io.EOF {
 				break
 			}
+			// Unfortunately as described in https://github.com/golang/go/issues/54133,
+			// we don't have a good way to determine the actual error cause of NextPart()
+			// without an ugly string comparison.
+			// Note that io.EOF is treated differently from an unexpected EOF:
+			// https://github.com/golang/go/blob/69d6c7b8ee62b4db5a8f6399e15f27d47b209a29/src/mime/multipart/multipart.go#L395-L405
+			if err.Error() == "multipart: NextPart: EOF" {
+				return ErrUnexpectedMultipartEOF
+			}
 			return err
 		}
 

workhorse/internal/upload/uploads.go

@@ -55,7 +55,7 @@ func interceptMultipartFiles(w http.ResponseWriter, r *http.Request, h http.Hand
 		switch err {
 		case http.ErrNotMultipart:
 			h.ServeHTTP(w, r)
-		case ErrInjectedClientParam, http.ErrMissingBoundary:
+		case ErrInjectedClientParam, ErrUnexpectedMultipartEOF, http.ErrMissingBoundary:
 			fail.Request(w, r, err, fail.WithStatus(http.StatusBadRequest))
 		case ErrTooManyFilesUploaded:
 			fail.Request(w, r, err, fail.WithStatus(http.StatusBadRequest), fail.WithBody(err.Error()))

workhorse/internal/upload/uploads_test.go

@@ -333,6 +333,30 @@ func TestInvalidFileNames(t *testing.T) {
 	}
 }
 
+func TestIncompleteMultipartData(t *testing.T) {
+	testhelper.ConfigureSecret()
+
+	buffer := &bytes.Buffer{}
+
+	writer := multipart.NewWriter(buffer)
+	file, err := writer.CreateFormFile("file", "somefile.txt")
+	require.NoError(t, err)
+	fmt.Fprint(file, "test")
+	writer.Close()
+
+	// Truncate the buffer to simulate an incomplete multipart request
+	truncatedBuffer := buffer.Bytes()[:buffer.Len()-1]
+	customReader := bytes.NewReader(truncatedBuffer)
+
+	httpRequest, err := http.NewRequest("POST", "/example", customReader)
+	require.NoError(t, err)
+	httpRequest.Header.Set("Content-Type", writer.FormDataContentType())
+
+	response := httptest.NewRecorder()
+	testInterceptMultipartFiles(t, response, httpRequest, nilHandler, &SavedFileTracker{Request: httpRequest})
+	require.Equal(t, 400, response.Code)
+}
+
 func TestBadMultipartHeader(t *testing.T) {
 	httpRequest, err := http.NewRequest("POST", "/example", bytes.NewReader(nil))
 	require.NoError(t, err)