Add latest changes from gitlab-org/gitlab@13-11-stable-eev13.11.0-rc43

7 files changed, 105 insertions, 36 deletions

diff --git a/workhorse/internal/api/api.go b/workhorse/internal/api/api.go
index a420288a95a..445ca3a94cf 100644
--- a/workhorse/internal/api/api.go
+++ b/workhorse/internal/api/api.go
@@ -149,9 +149,11 @@ 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:NewSingleHostReverseProxy
+// singleJoiningSlash is taken from reverseproxy.go:singleJoiningSlash
 func singleJoiningSlash(a, b string) string {
 	aslash := strings.HasSuffix(a, "/")
 	bslash := strings.HasPrefix(b, "/")
@@ -164,14 +166,36 @@ func singleJoiningSlash(a, b string) string {
 	return a + b
 }
 
+// joinURLPath is taken from reverseproxy.go:joinURLPath
+func joinURLPath(a *url.URL, b string) (path string, rawpath string) {
+	if a.RawPath == "" && b == "" {
+		return singleJoiningSlash(a.Path, b), ""
+	}
+
+	// Same as singleJoiningSlash, but uses EscapedPath to determine
+	// whether a slash should be added
+	apath := a.EscapedPath()
+	bpath := b
+
+	aslash := strings.HasSuffix(apath, "/")
+	bslash := strings.HasPrefix(bpath, "/")
+
+	switch {
+	case aslash && bslash:
+		return a.Path + bpath[1:], apath + bpath[1:]
+	case !aslash && !bslash:
+		return a.Path + "/" + bpath, apath + "/" + bpath
+	}
+	return a.Path + bpath, apath + bpath
+}
+
 // rebaseUrl is taken from reverseproxy.go:NewSingleHostReverseProxy
 func rebaseUrl(url *url.URL, onto *url.URL, suffix string) *url.URL {
 	newUrl := *url
 	newUrl.Scheme = onto.Scheme
 	newUrl.Host = onto.Host
-	if suffix != "" {
-		newUrl.Path = singleJoiningSlash(url.Path, suffix)
-	}
+	newUrl.Path, newUrl.RawPath = joinURLPath(url, suffix)
+
 	if onto.RawQuery == "" || newUrl.RawQuery == "" {
 		newUrl.RawQuery = onto.RawQuery + newUrl.RawQuery
 	} else {
diff --git a/workhorse/internal/filestore/save_file_opts.go b/workhorse/internal/filestore/save_file_opts.go
index d0b2c6ec809..f42e21b5f2e 100644
--- a/workhorse/internal/filestore/save_file_opts.go
+++ b/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 != "" {
diff --git a/workhorse/internal/filestore/save_file_opts_test.go b/workhorse/internal/filestore/save_file_opts_test.go
index facfb1cdc85..aa7018525ab 100644
--- a/workhorse/internal/filestore/save_file_opts_test.go
+++ b/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)
diff --git a/workhorse/internal/upload/exif/exif.go b/workhorse/internal/upload/exif/exif.go
index 2f8218c3bc3..db3c45431c0 100644
--- a/workhorse/internal/upload/exif/exif.go
+++ b/workhorse/internal/upload/exif/exif.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"os"
 	"os/exec"
 	"regexp"
 
@@ -109,6 +110,10 @@ func (c *cleaner) startProcessing(stdin io.Reader) error {
 }
 
 func FileTypeFromSuffix(filename string) FileType {
+	if os.Getenv("SKIP_EXIFTOOL") == "1" {
+		return TypeUnknown
+	}
+
 	jpegMatch := regexp.MustCompile(`(?i)^[^\n]*\.(jpg|jpeg)$`)
 	if jpegMatch.MatchString(filename) {
 		return TypeJPEG
diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go
index ba6bd0e501a..85063d65c1b 100644
--- a/workhorse/internal/upload/rewrite.go
+++ b/workhorse/internal/upload/rewrite.go
@@ -9,6 +9,7 @@ import (
 	"mime/multipart"
 	"net/http"
 	"os"
+	"path/filepath"
 	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
@@ -117,6 +118,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)
 	}
@@ -187,7 +192,10 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageTy
 		return nil, err
 	}
 
-	tmpfile.Seek(0, io.SeekStart)
+	if _, err := tmpfile.Seek(0, io.SeekStart); err != nil {
+		return nil, err
+	}
+
 	isValidType := false
 	switch imageType {
 	case exif.TypeJPEG:
@@ -196,7 +204,10 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string, imageTy
 		isValidType = isTIFF(tmpfile)
 	}
 
-	tmpfile.Seek(0, io.SeekStart)
+	if _, err := tmpfile.Seek(0, io.SeekStart); err != nil {
+		return nil, err
+	}
+
 	if !isValidType {
 		log.WithContextFields(ctx, log.Fields{
 			"filename":  filename,
diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go
index 0885f31d5a4..d77d73b5f48 100644
--- a/workhorse/internal/upload/uploads_test.go
+++ b/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)
 	}
 }
 
diff --git a/workhorse/internal/upstream/routes.go b/workhorse/internal/upstream/routes.go
index fb8a07a8031..d95646b91f7 100644
--- a/workhorse/internal/upstream/routes.go
+++ b/workhorse/internal/upstream/routes.go
@@ -57,6 +57,7 @@ const (
 	ciAPIPattern         = `^/ci/api/`
 	gitProjectPattern    = `^/.+\.git/`
 	projectPattern       = `^/([^/]+/){1,}[^/]+/`
+	apiProjectPattern    = apiPattern + `v4/projects/[^/]+/` // API: Projects can be encoded via group%2Fsubgroup%2Fproject
 	snippetUploadPattern = `^/uploads/personal_snippet`
 	userUploadPattern    = `^/uploads/user`
 	importPattern        = `^/import/`
@@ -253,32 +254,39 @@ func configureRoutes(u *upstream) {
 		u.route("", apiPattern+`v4/jobs/request\z`, ciAPILongPolling),
 		u.route("", ciAPIPattern+`v1/builds/register.json\z`, ciAPILongPolling),
 
+		// Not all API endpoints support encoded project IDs
+		// (e.g. `group%2Fproject`), but for the sake of consistency we
+		// use the apiProjectPattern regex throughout. API endpoints
+		// that do not support this will return 400 regardless of
+		// whether they are accelerated by Workhorse or not.  See
+		// https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56731.
+
 		// Maven Artifact Repository
-		u.route("PUT", apiPattern+`v4/projects/[0-9]+/packages/maven/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
+		u.route("PUT", apiProjectPattern+`packages/maven/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
 
 		// Conan Artifact Repository
 		u.route("PUT", apiPattern+`v4/packages/conan/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
-		u.route("PUT", apiPattern+`v4/projects/[0-9]+/packages/conan/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
+		u.route("PUT", apiProjectPattern+`packages/conan/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
 
 		// Generic Packages Repository
-		u.route("PUT", apiPattern+`v4/projects/[0-9]+/packages/generic/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
+		u.route("PUT", apiProjectPattern+`packages/generic/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
 
 		// NuGet Artifact Repository
-		u.route("PUT", apiPattern+`v4/projects/[0-9]+/packages/nuget/`, upload.Accelerate(api, signingProxy, preparers.packages)),
+		u.route("PUT", apiProjectPattern+`packages/nuget/`, upload.Accelerate(api, signingProxy, preparers.packages)),
 
 		// PyPI Artifact Repository
-		u.route("POST", apiPattern+`v4/projects/[0-9]+/packages/pypi`, upload.Accelerate(api, signingProxy, preparers.packages)),
+		u.route("POST", apiProjectPattern+`packages/pypi`, upload.Accelerate(api, signingProxy, preparers.packages)),
 
 		// Debian Artifact Repository
-		u.route("PUT", apiPattern+`v4/projects/[0-9]+/packages/debian/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
+		u.route("PUT", apiProjectPattern+`packages/debian/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
 
 		// Gem Artifact Repository
-		u.route("POST", apiPattern+`v4/projects/[0-9]+/packages/rubygems/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
+		u.route("POST", apiProjectPattern+`packages/rubygems/`, upload.BodyUploader(api, signingProxy, preparers.packages)),
 
 		// We are porting API to disk acceleration
 		// we need to declare each routes until we have fixed all the routes on the rails codebase.
 		// Overall status can be seen at https://gitlab.com/groups/gitlab-org/-/epics/1802#current-status
-		u.route("POST", apiPattern+`v4/projects/[0-9]+/wikis/attachments\z`, uploadAccelerateProxy),
+		u.route("POST", apiProjectPattern+`wikis/attachments\z`, uploadAccelerateProxy),
 		u.route("POST", apiPattern+`graphql\z`, uploadAccelerateProxy),
 		u.route("POST", apiPattern+`v4/groups/import`, upload.Accelerate(api, signingProxy, preparers.uploads)),
 		u.route("POST", apiPattern+`v4/projects/import`, upload.Accelerate(api, signingProxy, preparers.uploads)),
@@ -289,11 +297,14 @@ func configureRoutes(u *upstream) {
 		u.route("POST", importPattern+`gitlab_group`, upload.Accelerate(api, signingProxy, preparers.uploads)),
 
 		// Metric image upload
-		u.route("POST", apiPattern+`v4/projects/[0-9]+/issues/[0-9]+/metric_images\z`, upload.Accelerate(api, signingProxy, preparers.uploads)),
+		u.route("POST", apiProjectPattern+`issues/[0-9]+/metric_images\z`, upload.Accelerate(api, signingProxy, preparers.uploads)),
 
 		// Requirements Import via UI upload acceleration
 		u.route("POST", projectPattern+`requirements_management/requirements/import_csv`, upload.Accelerate(api, signingProxy, preparers.uploads)),
 
+		// Uploads via API
+		u.route("POST", apiProjectPattern+`uploads\z`, upload.Accelerate(api, signingProxy, preparers.uploads)),
+
 		// Explicitly proxy API requests
 		u.route("", apiPattern, proxy),
 		u.route("", ciAPIPattern, proxy),