Commit 4c51a867 authored by Nick Thomas's avatar Nick Thomas

Merge branch 'sh-remove-workhorse-extract-filename-base-ff' into 'master'

Remove workhorse_extract_filename_base feature flag [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!60070
parents 259f52dd 66c4fbb3
......@@ -187,7 +187,6 @@ module ObjectStorage
hash[:TempPath] = workhorse_local_upload_path
end
hash[:FeatureFlagExtractBase] = Feature.enabled?(:workhorse_extract_filename_base, default_enabled: :yaml)
hash[:MaximumSize] = maximum_size if maximum_size.present?
end
end
......
---
title: Remove workhorse_extract_filename_base feature flag
merge_request: 60070
author:
type: changed
---
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: true
......@@ -441,22 +441,6 @@ 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
......@@ -518,7 +502,6 @@ 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
......@@ -526,8 +509,6 @@ 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 }
......
......@@ -149,7 +149,7 @@ 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
// DEPRECATED: Feature flag used to determine whether to strip the multipart filename of any directories
FeatureFlagExtractBase bool
}
......
......@@ -63,8 +63,6 @@ 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
......@@ -90,17 +88,16 @@ func GetOpts(apiResponse *api.Response) (*SaveFileOpts, error) {
}
opts := SaveFileOpts{
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,
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 != "" {
......
......@@ -57,19 +57,15 @@ func TestSaveFileOptsLocalAndRemote(t *testing.T) {
func TestGetOpts(t *testing.T) {
tests := []struct {
name string
multipart *api.MultipartUploadParams
customPutHeaders bool
putHeaders map[string]string
FeatureFlagExtractBase bool
name string
multipart *api.MultipartUploadParams
customPutHeaders bool
putHeaders map[string]string
}{
{
name: "Single upload",
},
{
name: "Single upload w/ FeatureFlagExtractBase enabled",
FeatureFlagExtractBase: true,
}, {
name: "Multipart upload",
multipart: &api.MultipartUploadParams{
PartSize: 10,
......@@ -98,7 +94,6 @@ 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",
......@@ -114,7 +109,6 @@ 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)
......
......@@ -116,11 +116,7 @@ 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 {
multipartFiles.WithLabelValues(rew.filter.Name()).Inc()
filename := p.FileName()
if opts.FeatureFlagExtractBase {
filename = filepath.Base(filename)
}
filename := filepath.Base(p.FileName())
if strings.Contains(filename, "/") || filename == "." || filename == ".." {
return fmt.Errorf("illegal filename: %q", filename)
......
......@@ -325,20 +325,17 @@ func TestInvalidFileNames(t *testing.T) {
defer os.RemoveAll(tempPath)
for _, testCase := range []struct {
filename string
code int
FeatureFlagExtractBase bool
expectedPrefix string
filename string
code int
expectedPrefix string
}{
{"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, ""},
{"foobar", 200, "foobar"}, // sanity check for test setup below
{"foo/bar", 200, "bar"},
{"foo/bar/baz", 200, "baz"},
{"/../../foobar", 200, "foobar"},
{".", 500, ""},
{"..", 500, ""},
{"./", 500, ""},
} {
buffer := &bytes.Buffer{}
......@@ -356,7 +353,6 @@ 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)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment