From 67f929326b8233d5477f6ed4c15734faeae7a942 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Wed, 27 Oct 2021 10:17:52 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@14-2-stable-ee --- .../internal/artifacts/artifacts_upload_test.go | 2 +- workhorse/internal/upload/rewrite.go | 9 ++++- workhorse/internal/upload/uploads.go | 3 ++ workhorse/internal/upload/uploads_test.go | 43 +++++++++++++++++----- 4 files changed, 45 insertions(+), 12 deletions(-) (limited to 'workhorse') diff --git a/workhorse/internal/artifacts/artifacts_upload_test.go b/workhorse/internal/artifacts/artifacts_upload_test.go index ce078c78559..488636043bc 100644 --- a/workhorse/internal/artifacts/artifacts_upload_test.go +++ b/workhorse/internal/artifacts/artifacts_upload_test.go @@ -270,7 +270,7 @@ func TestUploadHandlerForMultipleFiles(t *testing.T) { require.NoError(t, s.writer.Close()) response := testUploadArtifacts(t, s.writer.FormDataContentType(), s.url, s.buffer) - require.Equal(t, http.StatusInternalServerError, response.Code) + require.Equal(t, http.StatusBadRequest, response.Code) } func TestUploadFormProcessing(t *testing.T) { diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index 7945fa1f34d..79ebfe950c5 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -25,7 +25,10 @@ import ( ) // ErrInjectedClientParam means that the client sent a parameter that overrides one of our own fields -var ErrInjectedClientParam = errors.New("injected client parameter") +var ( + ErrInjectedClientParam = errors.New("injected client parameter") + ErrMultipleFilesUploaded = errors.New("upload request contains more than one file") +) var ( multipartUploadRequests = promauto.NewCounterVec( @@ -114,6 +117,10 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr } func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *filestore.SaveFileOpts) error { + if rew.filter.Count() > 0 { + return ErrMultipleFilesUploaded + } + multipartFiles.WithLabelValues(rew.filter.Name()).Inc() filename := filepath.Base(p.FileName()) diff --git a/workhorse/internal/upload/uploads.go b/workhorse/internal/upload/uploads.go index 7a93fa4a9d8..8f4c8bb95f0 100644 --- a/workhorse/internal/upload/uploads.go +++ b/workhorse/internal/upload/uploads.go @@ -21,6 +21,7 @@ type MultipartFormProcessor interface { ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error Finalize(ctx context.Context) error Name() string + Count() int } func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, preauth *api.Response, filter MultipartFormProcessor, opts *filestore.SaveFileOpts) { @@ -34,6 +35,8 @@ func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, p switch err { case ErrInjectedClientParam: helper.CaptureAndFail(w, r, err, "Bad Request", http.StatusBadRequest) + case ErrMultipleFilesUploaded: + helper.CaptureAndFail(w, r, err, "Uploading multiple files is not allowed", http.StatusBadRequest) case http.ErrNotMultipart: h.ServeHTTP(w, r) case filestore.ErrEntityTooLarge: diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index 5fae5cf7bcb..a655cabd2e7 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -18,7 +18,6 @@ import ( "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab/workhorse/internal/api" - "gitlab.com/gitlab-org/gitlab/workhorse/internal/filestore" "gitlab.com/gitlab-org/gitlab/workhorse/internal/helper" "gitlab.com/gitlab-org/gitlab/workhorse/internal/objectstore/test" "gitlab.com/gitlab-org/gitlab/workhorse/internal/proxy" @@ -28,11 +27,7 @@ import ( var nilHandler = http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) -type testFormProcessor struct{} - -func (a *testFormProcessor) ProcessFile(ctx context.Context, formName string, file *filestore.FileHandler, writer *multipart.Writer) error { - return nil -} +type testFormProcessor struct{ SavedFileTracker } func (a *testFormProcessor) ProcessField(ctx context.Context, formName string, writer *multipart.Writer) error { if formName != "token" && !strings.HasPrefix(formName, "file.") && !strings.HasPrefix(formName, "other.") { @@ -45,10 +40,6 @@ func (a *testFormProcessor) Finalize(ctx context.Context) error { return nil } -func (a *testFormProcessor) Name() string { - return "" -} - func TestUploadTempPathRequirement(t *testing.T) { apiResponse := &api.Response{} preparer := &DefaultPreparer{} @@ -259,6 +250,38 @@ func TestUploadProcessingField(t *testing.T) { require.Equal(t, 500, response.Code) } +func TestUploadingMultipleFiles(t *testing.T) { + testhelper.ConfigureSecret() + + tempPath, err := ioutil.TempDir("", "uploads") + require.NoError(t, err) + defer os.RemoveAll(tempPath) + + var buffer bytes.Buffer + + writer := multipart.NewWriter(&buffer) + _, err = writer.CreateFormFile("file", "my.file") + require.NoError(t, err) + _, err = writer.CreateFormFile("file", "my.file") + require.NoError(t, err) + require.NoError(t, writer.Close()) + + httpRequest, err := http.NewRequest("PUT", "/url/path", &buffer) + require.NoError(t, err) + httpRequest.Header.Set("Content-Type", writer.FormDataContentType()) + + response := httptest.NewRecorder() + apiResponse := &api.Response{TempPath: tempPath} + preparer := &DefaultPreparer{} + opts, _, err := preparer.Prepare(apiResponse) + require.NoError(t, err) + + HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) + + require.Equal(t, 400, response.Code) + require.Equal(t, "Uploading multiple files is not allowed\n", response.Body.String()) +} + func TestUploadProcessingFile(t *testing.T) { tempPath, err := ioutil.TempDir("", "uploads") require.NoError(t, err) -- cgit v1.2.3