Omit trailing slash when proxying pre-authorized routes with no suffix

In https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56731, we pulled in the upstream reverse proxy code to rebase a suffix onto a URL (e.g. `/api/v4/uploads` -> `/api/v4/uploads/authorize`). In the case of `info/refs`, `git-upload-pack`, etc. no suffix is added for the authorization endpoint. The changes in that merge request would cause Workhorse to append a trailing slash when proxying the request to Rails. However, in https://gitlab.com/gitlab-org/gitlab/-/issues/330787, this broke Geo maintenance mode since the middleware did not strip the trailing slash. To fix this, we restore the behavior and omit the trailing slash if no pre-authorize suffix is provided. The absence or present of a trailing slash usually does not cause a material difference, but we might as well preserve the format of the original request. Changelog: fixed

Omit trailing slash when proxying pre-authorized routes with no suffix
In https://gitlab.com/gitlab-org/gitlab/-/merge_requests/56731, we pulled in the upstream reverse proxy code to rebase a suffix onto a URL (e.g. `/api/v4/uploads` -> `/api/v4/uploads/authorize`). In the case of `info/refs`, `git-upload-pack`, etc. no suffix is added for the authorization endpoint. The changes in that merge request would cause Workhorse to append a trailing slash when proxying the request to Rails. However, in https://gitlab.com/gitlab-org/gitlab/-/issues/330787, this broke Geo maintenance mode since the middleware did not strip the trailing slash. To fix this, we restore the behavior and omit the trailing slash if no pre-authorize suffix is provided. The absence or present of a trailing slash usually does not cause a material difference, but we might as well preserve the format of the original request. Changelog: fixed
79fc9f2b · Stan Hu · Ash McKenzie · 29c0e30f · 79fc9f2b · 79fc9f2b
Commit 79fc9f2b authored May 12, 2021 by Stan Hu Committed by Ash McKenzie May 13, 2021
3 changed files
--- a/changelogs/unreleased/sh-avoid-trailing-slash-in-proxy.yml
+++ b/changelogs/unreleased/sh-avoid-trailing-slash-in-proxy.yml
+---
+title: Omit trailing slash when proxying pre-authorized routes with no suffix
+merge_request: 61638
+author:
+type: fixed
--- a/workhorse/internal/api/api.go
+++ b/workhorse/internal/api/api.go
@@ -168,7 +168,10 @@ func singleJoiningSlash(a, b string) string {

 // joinURLPath is taken from reverseproxy.go:joinURLPath
 func joinURLPath(a *url.URL, b string) (path string, rawpath string) {
-	if a.RawPath == "" && b == "" {
+	// Avoid adding a trailing slash if the suffix is empty
+	if b == "" {
+		return a.Path, a.RawPath
+	} else if a.RawPath == "" {
 		return singleJoiningSlash(a.Path, b), ""
 	}


--- a/workhorse/main_test.go
+++ b/workhorse/main_test.go
@@ -536,7 +536,11 @@ func TestApiContentTypeBlock(t *testing.T) {
 func TestAPIFalsePositivesAreProxied(t *testing.T) {
 	goodResponse := []byte(`<html></html>`)
 	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
-		if r.Header.Get(secret.RequestHeader) != "" && r.Method != "GET" {
+		url := r.URL.String()
+		if url[len(url)-1] == '/' {
+			w.WriteHeader(500)
+			w.Write([]byte("PreAuthorize request included a trailing slash"))
+		} else if r.Header.Get(secret.RequestHeader) != "" && r.Method != "GET" {
 			w.WriteHeader(500)
 			w.Write([]byte("non-GET request went through PreAuthorize handler"))
 		} else {