diff --git a/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb b/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb index bfa7be5bb5cdbb667800b4f036aeceea5bc0dd35..e885c0c44131d71a1eceb9a121e633d33639b0b8 100644 --- a/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb +++ b/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb @@ -27,10 +27,10 @@ expect(page).to have_content('dk.png') end - upload_design(gif_fixture, count: 2) + upload_design([gif_fixture, logo_svg_fixture, big_image_fixture], count: 4) - expect(page).to have_selector('.js-design-list-item', count: 2) - expect(page.all('.js-design-list-item').map(&:text)).to eq(['dk.png', 'banana_sample.gif']) + expect(page).to have_selector('.js-design-list-item', count: 4) + expect(page.all('.js-design-list-item').map(&:text)).to eq(['dk.png', 'banana_sample.gif', 'logo_sample.svg', 'big-image.png']) end end @@ -50,8 +50,16 @@ def gif_fixture Rails.root.join('spec', 'fixtures', 'banana_sample.gif') end - def upload_design(fixture, count:) - attach_file(:upload_file, fixture, match: :first, make_visible: true) + def logo_svg_fixture + Rails.root.join('spec', 'fixtures', 'logo_sample.svg') + end + + def big_image_fixture + Rails.root.join('spec', 'fixtures', 'big-image.png') + end + + def upload_design(fixtures, count:) + attach_file(:upload_file, fixtures, multiple: true, match: :first, make_visible: true) wait_for('designs uploaded') do issue.reload.designs.count == count diff --git a/workhorse/internal/artifacts/artifacts_upload_test.go b/workhorse/internal/artifacts/artifacts_upload_test.go index df1c30dcff0b7692b7cc20836f74a7d3cf5dd535..3e8a52be1a1c6b09141ee7daa015e5aadd7ef3e4 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.StatusBadRequest, response.Code) + require.Equal(t, http.StatusInternalServerError, response.Code) } func TestUploadFormProcessing(t *testing.T) { diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index 3dfab120188be6d466fd52faad3ed4a255af0349..b9324ac8b7b756a2d31447817adc280bcbf5c54d 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -24,10 +24,12 @@ import ( "gitlab.com/gitlab-org/gitlab/workhorse/internal/upload/exif" ) +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") - ErrMultipleFilesUploaded = errors.New("upload request contains more than one file") + ErrInjectedClientParam = errors.New("injected client parameter") + ErrTooManyFilesUploaded = fmt.Errorf("upload request contains more than %v files", maxFilesAllowed) ) var ( @@ -117,8 +119,8 @@ 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 + if rew.filter.Count() >= maxFilesAllowed { + return ErrTooManyFilesUploaded } multipartFiles.WithLabelValues(rew.filter.Name()).Inc() diff --git a/workhorse/internal/upload/saved_file_tracker.go b/workhorse/internal/upload/saved_file_tracker.go index cd41e661b7d77bb930794126bc42d1c7496cdb7c..e6f9a8c9a88909cd172c6f00b548c3eb40ab843a 100644 --- a/workhorse/internal/upload/saved_file_tracker.go +++ b/workhorse/internal/upload/saved_file_tracker.go @@ -27,6 +27,10 @@ func (s *SavedFileTracker) Count() int { } func (s *SavedFileTracker) ProcessFile(_ context.Context, fieldName string, file *filestore.FileHandler, _ *multipart.Writer) error { + if _, ok := s.rewrittenFields[fieldName]; ok { + return fmt.Errorf("the %v field has already been processed", fieldName) + } + s.Track(fieldName, file.LocalPath) return nil } diff --git a/workhorse/internal/upload/saved_file_tracker_test.go b/workhorse/internal/upload/saved_file_tracker_test.go index b34dd9aed4ffaa7ad29709f3be66a3fbe07ef94c..ba927db253e2e90a333759e927b5edd8a75e1c7d 100644 --- a/workhorse/internal/upload/saved_file_tracker_test.go +++ b/workhorse/internal/upload/saved_file_tracker_test.go @@ -37,3 +37,14 @@ func TestSavedFileTracking(t *testing.T) { require.Contains(t, rewrittenFields, "test") } + +func TestDuplicatedFileProcessing(t *testing.T) { + tracker := SavedFileTracker{} + file := &filestore.FileHandler{} + + require.NoError(t, tracker.ProcessFile(context.Background(), "file", file, nil)) + + err := tracker.ProcessFile(context.Background(), "file", file, nil) + require.Error(t, err) + require.Equal(t, "the file field has already been processed", err.Error()) +} diff --git a/workhorse/internal/upload/uploads.go b/workhorse/internal/upload/uploads.go index 8f4c8bb95f08a9b1c90e20ba4aacbfb79fe8d1a7..b408d2603793607345c45c76467de22f19539984 100644 --- a/workhorse/internal/upload/uploads.go +++ b/workhorse/internal/upload/uploads.go @@ -35,8 +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 ErrTooManyFilesUploaded: + helper.CaptureAndFail(w, r, err, err.Error(), 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 b5db8003d41b80e80cb3fd7265b5a34612a20237..7c22b3b4e2859cc99750198d77463318cc1b1ef0 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -260,10 +260,10 @@ func TestUploadingMultipleFiles(t *testing.T) { 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) + for i := 0; i < 11; i++ { + _, err = writer.CreateFormFile(fmt.Sprintf("file %v", i), "my.file") + require.NoError(t, err) + } require.NoError(t, writer.Close()) httpRequest, err := http.NewRequest("PUT", "/url/path", &buffer) @@ -279,7 +279,7 @@ func TestUploadingMultipleFiles(t *testing.T) { 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()) + require.Equal(t, "upload request contains more than 10 files\n", response.Body.String()) } func TestUploadProcessingFile(t *testing.T) {