Merge branch 'sh-extract-base-from-multipart-uploads' into 'master'

Fix uploads accelerated by Workhorse not working with paths [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!57889

app/uploaders/object_storage.rb

@@ -187,6 +187,7 @@ module ObjectStorage
             hash[:TempPath] = workhorse_local_upload_path
           end
 
+          hash[:FeatureFlagExtractBase] = Feature.enabled?(:workhorse_extract_filename_base)
           hash[:MaximumSize] = maximum_size if maximum_size.present?
         end
       end

config/feature_flags/development/workhorse_extract_filename_base.yml

+---
+name: workhorse_extract_filename_base
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57889
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/326379
+milestone: '13.11'
+type: development
+group: group::source code
+default_enabled: false

spec/uploaders/object_storage_spec.rb

@@ -441,6 +441,22 @@ RSpec.describe ObjectStorage do
       end
     end
 
+    shared_examples 'extracts base filename' do
+      it "returns true for ExtractsBase" do
+        expect(subject[:FeatureFlagExtractBase]).to be true
+      end
+
+      context 'when workhorse_extract_filename_base is disabled' do
+        before do
+          stub_feature_flags(workhorse_extract_filename_base: false)
+        end
+
+        it "returns false for ExtractsBase" do
+          expect(subject[:FeatureFlagExtractBase]).to be false
+        end
+      end
+    end
+
     shared_examples 'uses local storage' do
       it_behaves_like 'returns the maximum size given' do
         it "returns temporary path" do
@@ -502,6 +518,7 @@ RSpec.describe ObjectStorage do
       end
 
       it_behaves_like 'uses local storage'
+      it_behaves_like 'extracts base filename'
     end
 
     context 'when object storage is enabled' do
@@ -509,6 +526,8 @@ RSpec.describe ObjectStorage do
         allow(Gitlab.config.uploads.object_store).to receive(:enabled) { true }
       end
 
+      it_behaves_like 'extracts base filename'
+
       context 'when direct upload is enabled' do
         before do
           allow(Gitlab.config.uploads.object_store).to receive(:direct_upload) { true }

workhorse/internal/api/api.go

@@ -149,6 +149,8 @@ type Response struct {
 	ProcessLsifReferences bool
 	// The maximum accepted size in bytes of the upload
 	MaximumSize int64
+	// Feature flag used to determine whether to strip the multipart filename of any directories
+	FeatureFlagExtractBase bool
 }
 
 // singleJoiningSlash is taken from reverseproxy.go:singleJoiningSlash

workhorse/internal/filestore/save_file_opts.go

@@ -63,6 +63,8 @@ type SaveFileOpts struct {
 	PresignedCompleteMultipart string
 	// PresignedAbortMultipart is a presigned URL for AbortMultipartUpload
 	PresignedAbortMultipart string
+	// FeatureFlagExtractBase uses the base of the filename and strips directories
+	FeatureFlagExtractBase bool
 }
 
 // UseWorkhorseClientEnabled checks if the options require direct access to object storage
@@ -88,16 +90,17 @@ func GetOpts(apiResponse *api.Response) (*SaveFileOpts, error) {
 	}
 
 	opts := SaveFileOpts{
-		LocalTempPath:      apiResponse.TempPath,
-		RemoteID:           apiResponse.RemoteObject.ID,
-		RemoteURL:          apiResponse.RemoteObject.GetURL,
-		PresignedPut:       apiResponse.RemoteObject.StoreURL,
-		PresignedDelete:    apiResponse.RemoteObject.DeleteURL,
-		PutHeaders:         apiResponse.RemoteObject.PutHeaders,
-		UseWorkhorseClient: apiResponse.RemoteObject.UseWorkhorseClient,
-		RemoteTempObjectID: apiResponse.RemoteObject.RemoteTempObjectID,
-		Deadline:           time.Now().Add(timeout),
-		MaximumSize:        apiResponse.MaximumSize,
+		FeatureFlagExtractBase: apiResponse.FeatureFlagExtractBase,
+		LocalTempPath:          apiResponse.TempPath,
+		RemoteID:               apiResponse.RemoteObject.ID,
+		RemoteURL:              apiResponse.RemoteObject.GetURL,
+		PresignedPut:           apiResponse.RemoteObject.StoreURL,
+		PresignedDelete:        apiResponse.RemoteObject.DeleteURL,
+		PutHeaders:             apiResponse.RemoteObject.PutHeaders,
+		UseWorkhorseClient:     apiResponse.RemoteObject.UseWorkhorseClient,
+		RemoteTempObjectID:     apiResponse.RemoteObject.RemoteTempObjectID,
+		Deadline:               time.Now().Add(timeout),
+		MaximumSize:            apiResponse.MaximumSize,
 	}
 
 	if opts.LocalTempPath != "" && opts.RemoteID != "" {

workhorse/internal/filestore/save_file_opts_test.go

@@ -57,13 +57,18 @@ func TestSaveFileOptsLocalAndRemote(t *testing.T) {
 
 func TestGetOpts(t *testing.T) {
 	tests := []struct {
-		name             string
-		multipart        *api.MultipartUploadParams
-		customPutHeaders bool
-		putHeaders       map[string]string
+		name                   string
+		multipart              *api.MultipartUploadParams
+		customPutHeaders       bool
+		putHeaders             map[string]string
+		FeatureFlagExtractBase bool
 	}{
 		{
 			name: "Single upload",
+		},
+		{
+			name:                   "Single upload w/ FeatureFlagExtractBase enabled",
+			FeatureFlagExtractBase: true,
 		}, {
 			name: "Multipart upload",
 			multipart: &api.MultipartUploadParams{
@@ -93,6 +98,7 @@ func TestGetOpts(t *testing.T) {
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			apiResponse := &api.Response{
+				FeatureFlagExtractBase: test.FeatureFlagExtractBase,
 				RemoteObject: api.RemoteObject{
 					Timeout:          10,
 					ID:               "id",
@@ -108,6 +114,7 @@ func TestGetOpts(t *testing.T) {
 			opts, err := filestore.GetOpts(apiResponse)
 			require.NoError(t, err)
 
+			require.Equal(t, apiResponse.FeatureFlagExtractBase, opts.FeatureFlagExtractBase)
 			require.Equal(t, apiResponse.TempPath, opts.LocalTempPath)
 			require.WithinDuration(t, deadline, opts.Deadline, time.Second)
 			require.Equal(t, apiResponse.RemoteObject.ID, opts.RemoteID)

workhorse/internal/upload/rewrite.go

@@ -8,6 +8,7 @@ import (
 	"io/ioutil"
 	"mime/multipart"
 	"net/http"
+	"path/filepath"
 	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
@@ -114,6 +115,10 @@ func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipa
 
 	filename := p.FileName()
 
+	if opts.FeatureFlagExtractBase {
+		filename = filepath.Base(filename)
+	}
+
 	if strings.Contains(filename, "/") || filename == "." || filename == ".." {
 		return fmt.Errorf("illegal filename: %q", filename)
 	}

workhorse/internal/upload/uploads_test.go

@@ -325,14 +325,20 @@ func TestInvalidFileNames(t *testing.T) {
 	defer os.RemoveAll(tempPath)
 
 	for _, testCase := range []struct {
-		filename string
-		code     int
+		filename               string
+		code                   int
+		FeatureFlagExtractBase bool
+		expectedPrefix         string
 	}{
-		{"foobar", 200}, // sanity check for test setup below
-		{"foo/bar", 500},
-		{"/../../foobar", 500},
-		{".", 500},
-		{"..", 500},
+		{"foobar", 200, false, "foobar"}, // sanity check for test setup below
+		{"foo/bar", 500, false, ""},
+		{"foo/bar", 200, true, "bar"},
+		{"foo/bar/baz", 200, true, "baz"},
+		{"/../../foobar", 500, false, ""},
+		{"/../../foobar", 200, true, "foobar"},
+		{".", 500, false, ""},
+		{"..", 500, false, ""},
+		{"./", 500, false, ""},
 	} {
 		buffer := &bytes.Buffer{}
 
@@ -350,10 +356,12 @@ func TestInvalidFileNames(t *testing.T) {
 		apiResponse := &api.Response{TempPath: tempPath}
 		preparer := &DefaultPreparer{}
 		opts, _, err := preparer.Prepare(apiResponse)
+		opts.FeatureFlagExtractBase = testCase.FeatureFlagExtractBase
 		require.NoError(t, err)
 
 		HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &SavedFileTracker{Request: httpRequest}, opts)
 		require.Equal(t, testCase.code, response.Code)
+		require.Equal(t, testCase.expectedPrefix, opts.TempFilePrefix)
 	}
 }