Fix uploads accelerated by Workhorse not working with paths

Filenames with slashes were allowed before we added Workhorse
acceleration in the uploads API via
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/57250. However,
users have scripted their uploads to use paths, and Workhorse now
rejects files outright with slashes.

To accommodate this, we now extract the base of the filename so that
file uploads with paths can still work. We introduce the feature flag
`workhorse_extract_filename_base` to roll this change out.

Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/326350

app/uploaders/object_storage.rb

@@ -187,6 +187,7 @@ module ObjectStorage
             hash[:TempPath] = workhorse_local_upload_path
           end
 
+          hash[:ExtractBase] = 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[:ExtractBase]).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[:ExtractBase]).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
+	ExtractBase 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
+	// ExtractBase uses the base of the filename and strips directories
+	ExtractBase bool
 }
 
 // UseWorkhorseClientEnabled checks if the options require direct access to object storage
@@ -88,6 +90,7 @@ func GetOpts(apiResponse *api.Response) (*SaveFileOpts, error) {
 	}
 
 	opts := SaveFileOpts{
+		ExtractBase:        apiResponse.ExtractBase,
 		LocalTempPath:      apiResponse.TempPath,
 		RemoteID:           apiResponse.RemoteObject.ID,
 		RemoteURL:          apiResponse.RemoteObject.GetURL,

workhorse/internal/filestore/save_file_opts_test.go

@@ -61,9 +61,14 @@ func TestGetOpts(t *testing.T) {
 		multipart        *api.MultipartUploadParams
 		customPutHeaders bool
 		putHeaders       map[string]string
+		extractBase      bool
 	}{
 		{
 			name: "Single upload",
+		},
+		{
+			name:        "Single upload w/ extractBase enabled",
+			extractBase: 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{
+				ExtractBase: test.extractBase,
 				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.ExtractBase, opts.ExtractBase)
 			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.ExtractBase {
+		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
+		extractBase    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.ExtractBase = testCase.extractBase
 		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)
 	}
 }