Merge branch 'sh-fix-encoded-api-project-urls' into 'master'

Fix Workhorse acceleration for encoded project IDs in API

See merge request gitlab-org/gitlab!56731

changelogs/unreleased/sh-fix-encoded-api-project-urls.yml

+---
+title: Fix Workhorse acceleration for encoded project IDs in API
+merge_request: 56731
+author:
+type: performance

spec/features/file_uploads/maven_package_spec.rb

@@ -6,16 +6,17 @@ RSpec.describe 'Upload a maven package', :api, :js do
   include_context 'file upload requests helpers'
 
   let_it_be(:project) { create(:project) }
-  let_it_be(:user) { create(:user, :admin) }
+  let_it_be(:user) { project.owner }
   let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
 
-  let(:api_path) { "/projects/#{project.id}/packages/maven/com/example/my-app/1.0/my-app-1.0-20180724.124855-1.jar" }
+  let(:project_id) { project.id }
+  let(:api_path) { "/projects/#{project_id}/packages/maven/com/example/my-app/1.0/my-app-1.0-20180724.124855-1.jar" }
   let(:url) { capybara_url(api(api_path, personal_access_token: personal_access_token)) }
   let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
 
   subject { HTTParty.put(url, body: file.read) }
 
-  RSpec.shared_examples 'for a maven package' do
+  shared_examples 'for a maven package' do
     it 'creates package files' do
       expect { subject }
         .to change { Packages::Package.maven.count }.by(1)
@@ -25,9 +26,9 @@ RSpec.describe 'Upload a maven package', :api, :js do
     it { expect(subject.code).to eq(200) }
   end
 
-  RSpec.shared_examples 'for a maven sha1' do
+  shared_examples 'for a maven sha1' do
     let(:dummy_package) { double(Packages::Package) }
-    let(:api_path) { "/projects/#{project.id}/packages/maven/com/example/my-app/1.0/my-app-1.0-20180724.124855-1.jar.sha1" }
+    let(:api_path) { "/projects/#{project_id}/packages/maven/com/example/my-app/1.0/my-app-1.0-20180724.124855-1.jar.sha1" }
 
     before do
       # The sha verification done by the maven api is between:
@@ -42,8 +43,8 @@ RSpec.describe 'Upload a maven package', :api, :js do
     it { expect(subject.code).to eq(204) }
   end
 
-  RSpec.shared_examples 'for a maven md5' do
-    let(:api_path) { "/projects/#{project.id}/packages/maven/com/example/my-app/1.0/my-app-1.0-20180724.124855-1.jar.md5" }
+  shared_examples 'for a maven md5' do
+    let(:api_path) { "/projects/#{project_id}/packages/maven/com/example/my-app/1.0/my-app-1.0-20180724.124855-1.jar.md5" }
     let(:file) { StringIO.new('dummy_package') }
 
     it { expect(subject.code).to eq(200) }
@@ -52,4 +53,10 @@ RSpec.describe 'Upload a maven package', :api, :js do
   it_behaves_like 'handling file uploads', 'for a maven package'
   it_behaves_like 'handling file uploads', 'for a maven sha1'
   it_behaves_like 'handling file uploads', 'for a maven md5'
+
+  context 'with an encoded project ID' do
+    let(:project_id) { "#{project.namespace.path}%2F#{project.path}" }
+
+    it_behaves_like 'handling file uploads', 'for a maven package'
+  end
 end

spec/features/file_uploads/nuget_package_spec.rb

@@ -6,7 +6,7 @@ RSpec.describe 'Upload a nuget package', :api, :js do
   include_context 'file upload requests helpers'
 
   let_it_be(:project) { create(:project) }
-  let_it_be(:user) { create(:user, :admin) }
+  let_it_be(:user) { project.owner }
   let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
 
   let(:api_path) { "/projects/#{project.id}/packages/nuget/" }
@@ -21,7 +21,7 @@ RSpec.describe 'Upload a nuget package', :api, :js do
     )
   end
 
-  RSpec.shared_examples 'for a nuget package' do
+  shared_examples 'for a nuget package' do
     it 'creates package files' do
       expect { subject }
         .to change { Packages::Package.nuget.count }.by(1)

spec/features/file_uploads/rubygem_package_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Upload a RubyGems package', :api, :js do
+  include_context 'file upload requests helpers'
+
+  let_it_be(:project) { create(:project) }
+  let_it_be(:user) { project.owner }
+  let_it_be(:personal_access_token) { create(:personal_access_token, user: user) }
+
+  let(:api_path) { "/projects/#{project_id}/packages/rubygems/api/v1/gems" }
+  let(:url) { capybara_url(api(api_path)) }
+  let(:file) { fixture_file_upload('spec/fixtures/dk.png') }
+
+  subject do
+    HTTParty.post(
+      url,
+      headers: { 'Authorization' => personal_access_token.token },
+      body: { file: file }
+    )
+  end
+
+  shared_examples 'for a Rubygems package' do
+    it 'creates package files' do
+      expect { subject }
+        .to change { Packages::Package.rubygems.count }.by(1)
+        .and change { Packages::PackageFile.count }.by(1)
+    end
+
+    it { expect(subject.code).to eq(201) }
+  end
+
+  context 'with an integer project ID' do
+    let(:project_id) { project.id }
+
+    it_behaves_like 'handling file uploads', 'for a Rubygems package'
+  end
+
+  context 'with an encoded project ID' do
+    let(:project_id) { "#{project.namespace.path}%2F#{project.path}" }
+
+    it_behaves_like 'handling file uploads', 'for a Rubygems package'
+  end
+end

workhorse/internal/api/api.go

@@ -151,7 +151,7 @@ type Response struct {
 	MaximumSize int64
 }
 
-// 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 +164,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 {

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,7 +297,7 @@ 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)),

workhorse/upload_test.go

@@ -41,7 +41,7 @@ func testArtifactsUpload(t *testing.T, uploadArtifacts uploadArtifactsFunction)
 	reqBody, contentType, err := multipartBodyWithFile()
 	require.NoError(t, err)
 
-	ts := signedUploadTestServer(t, nil)
+	ts := signedUploadTestServer(t, nil, nil)
 	defer ts.Close()
 
 	ws := startWorkhorseServer(ts.URL)
@@ -66,7 +66,7 @@ func expectSignedRequest(t *testing.T, r *http.Request) {
 	require.NoError(t, err)
 }
 
-func uploadTestServer(t *testing.T, extraTests func(r *http.Request)) *httptest.Server {
+func uploadTestServer(t *testing.T, authorizeTests func(r *http.Request), extraTests func(r *http.Request)) *httptest.Server {
 	return testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
 		if strings.HasSuffix(r.URL.Path, "/authorize") {
 			expectSignedRequest(t, r)
@@ -74,6 +74,10 @@ func uploadTestServer(t *testing.T, extraTests func(r *http.Request)) *httptest.
 			w.Header().Set("Content-Type", api.ResponseContentType)
 			_, err := fmt.Fprintf(w, `{"TempPath":"%s"}`, scratchDir)
 			require.NoError(t, err)
+
+			if authorizeTests != nil {
+				authorizeTests(r)
+			}
 			return
 		}
 
@@ -91,10 +95,10 @@ func uploadTestServer(t *testing.T, extraTests func(r *http.Request)) *httptest.
 	})
 }
 
-func signedUploadTestServer(t *testing.T, extraTests func(r *http.Request)) *httptest.Server {
+func signedUploadTestServer(t *testing.T, authorizeTests func(r *http.Request), extraTests func(r *http.Request)) *httptest.Server {
 	t.Helper()
 
-	return uploadTestServer(t, func(r *http.Request) {
+	return uploadTestServer(t, authorizeTests, func(r *http.Request) {
 		expectSignedRequest(t, r)
 
 		if extraTests != nil {
@@ -113,20 +117,38 @@ func TestAcceleratedUpload(t *testing.T) {
 		{"POST", `/uploads/personal_snippet`, true},
 		{"POST", `/uploads/user`, true},
 		{"POST", `/api/v4/projects/1/wikis/attachments`, false},
+		{"POST", `/api/v4/projects/group%2Fproject/wikis/attachments`, false},
+		{"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/wikis/attachments`, false},
 		{"POST", `/api/graphql`, false},
 		{"PUT", "/api/v4/projects/9001/packages/nuget/v1/files", true},
+		{"PUT", "/api/v4/projects/group%2Fproject/packages/nuget/v1/files", true},
+		{"PUT", "/api/v4/projects/group%2Fsubgroup%2Fproject/packages/nuget/v1/files", true},
 		{"POST", `/api/v4/groups/import`, true},
+		{"POST", `/api/v4/groups/import/`, true},
 		{"POST", `/api/v4/projects/import`, true},
+		{"POST", `/api/v4/projects/import/`, true},
 		{"POST", `/import/gitlab_project`, true},
+		{"POST", `/import/gitlab_project/`, true},
 		{"POST", `/import/gitlab_group`, true},
+		{"POST", `/import/gitlab_group/`, true},
 		{"POST", `/api/v4/projects/9001/packages/pypi`, true},
+		{"POST", `/api/v4/projects/group%2Fproject/packages/pypi`, true},
+		{"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/packages/pypi`, true},
 		{"POST", `/api/v4/projects/9001/issues/30/metric_images`, true},
+		{"POST", `/api/v4/projects/group%2Fproject/issues/30/metric_images`, true},
+		{"POST", `/api/v4/projects/group%2Fsubgroup%2Fproject/issues/30/metric_images`, true},
 		{"POST", `/my/project/-/requirements_management/requirements/import_csv`, true},
+		{"POST", `/my/project/-/requirements_management/requirements/import_csv/`, true},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.resource, func(t *testing.T) {
 			ts := uploadTestServer(t,
+				func(r *http.Request) {
+					resource := strings.TrimRight(tt.resource, "/")
+					// Validate %2F characters haven't been unescaped
+					require.Equal(t, resource+"/authorize", r.URL.String())
+				},
 				func(r *http.Request) {
 					if tt.signedFinalization {
 						expectSignedRequest(t, r)
@@ -186,6 +208,55 @@ func multipartBodyWithFile() (io.Reader, string, error) {
 	return result, writer.FormDataContentType(), writer.Close()
 }
 
+func unacceleratedUploadTestServer(t *testing.T) *httptest.Server {
+	return testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
+		require.False(t, strings.HasSuffix(r.URL.Path, "/authorize"))
+		require.Empty(t, r.Header.Get(upload.RewrittenFieldsHeader))
+
+		w.WriteHeader(200)
+	})
+}
+
+func TestUnacceleratedUploads(t *testing.T) {
+	tests := []struct {
+		method   string
+		resource string
+	}{
+		{"POST", `/api/v4/projects/group/subgroup/project/wikis/attachments`},
+		{"POST", `/api/v4/projects/group/project/wikis/attachments`},
+		{"PUT", "/api/v4/projects/group/subgroup/project/packages/nuget/v1/files"},
+		{"PUT", "/api/v4/projects/group/project/packages/nuget/v1/files"},
+		{"POST", `/api/v4/projects/group/subgroup/project/packages/pypi`},
+		{"POST", `/api/v4/projects/group/project/packages/pypi`},
+		{"POST", `/api/v4/projects/group/subgroup/project/packages/pypi`},
+		{"POST", `/api/v4/projects/group/project/issues/30/metric_images`},
+		{"POST", `/api/v4/projects/group/subgroup/project/issues/30/metric_images`},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.resource, func(t *testing.T) {
+			ts := unacceleratedUploadTestServer(t)
+
+			defer ts.Close()
+			ws := startWorkhorseServer(ts.URL)
+			defer ws.Close()
+
+			reqBody, contentType, err := multipartBodyWithFile()
+			require.NoError(t, err)
+
+			req, err := http.NewRequest(tt.method, ws.URL+tt.resource, reqBody)
+			require.NoError(t, err)
+
+			req.Header.Set("Content-Type", contentType)
+			resp, err := http.DefaultClient.Do(req)
+			require.NoError(t, err)
+			require.Equal(t, 200, resp.StatusCode)
+
+			resp.Body.Close()
+		})
+	}
+}
+
 func TestBlockingRewrittenFieldsHeader(t *testing.T) {
 	canary := "untrusted header passed by user"
 	testCases := []struct {
@@ -433,6 +504,11 @@ func TestPackageFilesUpload(t *testing.T) {
 		{"PUT", "/api/v4/projects/2412/packages/generic/mypackage/0.0.1/myfile.tar.gz"},
 		{"PUT", "/api/v4/projects/2412/packages/debian/libsample0_1.2.3~alpha2-1_amd64.deb"},
 		{"POST", "/api/v4/projects/2412/packages/rubygems/api/v1/gems/sample.gem"},
+		{"PUT", "/api/v4/projects/group%2Fproject/packages/conan/v1/files"},
+		{"PUT", "/api/v4/projects/group%2Fproject/packages/maven/v1/files"},
+		{"PUT", "/api/v4/projects/group%2Fproject/packages/generic/mypackage/0.0.1/myfile.tar.gz"},
+		{"PUT", "/api/v4/projects/group%2Fproject/packages/debian/libsample0_1.2.3~alpha2-1_amd64.deb"},
+		{"POST", "/api/v4/projects/group%2Fproject/packages/rubygems/api/v1/gems/sample.gem"},
 	}
 
 	for _, r := range routes {