Merge branch 'master' into sh-fix-git-large-upload-pack

authorization_test.go

@@ -78,7 +78,7 @@ func TestPreAuthorizeContentTypeFailure(t *testing.T) {
 		t, ts, "/authorize",
 		regexp.MustCompile(`/authorize\z`),
 		"",
-		200, 500)
+		200, 200)
 }
 
 func TestPreAuthorizeJWT(t *testing.T) {

internal/api/api.go

@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"mime"
 	"net/http"
 	"net/url"
 	"strconv"
@@ -198,14 +199,12 @@ func (api *API) PreAuthorize(suffix string, r *http.Request) (httpResponse *http
 	}()
 	requestsCounter.WithLabelValues(strconv.Itoa(httpResponse.StatusCode), authReq.Method).Inc()
 
-	if httpResponse.StatusCode != http.StatusOK {
+	// This may be a false positive, e.g. for .../info/refs, rather than a
+	// failure, so pass the response back
+	if httpResponse.StatusCode != http.StatusOK || !validResponseContentType(httpResponse) {
 		return httpResponse, nil, nil
 	}
 
-	if contentType := httpResponse.Header.Get("Content-Type"); contentType != ResponseContentType {
-		return httpResponse, nil, fmt.Errorf("preAuthorizeHandler: API responded with wrong content type: %v", contentType)
-	}
-
 	authResponse = &Response{}
 	// The auth backend validated the client request and told us additional
 	// request metadata. We must extract this information from the auth
@@ -220,17 +219,18 @@ func (api *API) PreAuthorize(suffix string, r *http.Request) (httpResponse *http
 func (api *API) PreAuthorizeHandler(next HandleFunc, suffix string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		httpResponse, authResponse, err := api.PreAuthorize(suffix, r)
+		if httpResponse != nil {
+			defer httpResponse.Body.Close()
+		}
+
 		if err != nil {
 			helper.Fail500(w, r, err)
 			return
 		}
-		if httpResponse != nil {
-			defer func() {
-				httpResponse.Body.Close()
-			}()
-		}
 
-		if httpResponse.StatusCode != http.StatusOK {
+		// The response couldn't be interpreted as a valid auth response, so
+		// pass it back (mostly) unmodified
+		if httpResponse != nil && authResponse == nil {
 			// NGINX response buffering is disabled on this path (with
 			// X-Accel-Buffering: no) but we still want to free up the Unicorn worker
 			// that generated httpResponse as fast as possible. To do this we buffer
@@ -286,3 +286,8 @@ func bufferResponse(r io.Reader) (*bytes.Buffer, error) {
 
 	return responseBody, nil
 }
+
+func validResponseContentType(resp *http.Response) bool {
+	parsed, _, err := mime.ParseMediaType(resp.Header.Get("Content-Type"))
+	return err == nil && parsed == ResponseContentType
+}

internal/upstream/routes.go

 package upstream
 
 import (
+	"mime"
 	"net/http"
 	"path"
 	"regexp"
@@ -30,14 +31,12 @@ type routeEntry struct {
 	matchers []matcherFunc
 }
 
-const projectPattern = `^/[^/]+/[^/]+/`
-const gitProjectPattern = `^/[^/]+/[^/]+\.git/`
-
-const apiPattern = `^/api/`
-
-// A project ID in an API request is either a number or two strings 'namespace/project'
-const projectsAPIPattern = `^/api/v3/projects/((\d+)|([^/]+/[^/]+))/`
-const ciAPIPattern = `^/ci/api/`
+const (
+	apiPattern        = `^/api/`
+	ciAPIPattern      = `^/ci/api/`
+	gitProjectPattern = `^/([^/]+/){1,}[^/]+\.git/`
+	projectPattern    = `^/([^/]+/){1,}[^/]+/`
+)
 
 func compileRegexp(regexpStr string) *regexp.Regexp {
 	if len(regexpStr) == 0 {
@@ -65,6 +64,14 @@ func wsRoute(regexpStr string, handler http.Handler, matchers ...matcherFunc) ro
 	}
 }
 
+// Creates matcherFuncs for a particular content type.
+func isContentType(contentType string) func(*http.Request) bool {
+	return func(r *http.Request) bool {
+		parsed, _, err := mime.ParseMediaType(r.Header.Get("Content-Type"))
+		return err == nil && contentType == parsed
+	}
+}
+
 func (ro *routeEntry) isMatch(cleanedPath string, req *http.Request) bool {
 	if ro.method != "" && req.Method != ro.method {
 		return false
@@ -117,9 +124,9 @@ func (u *Upstream) configureRoutes() {
 	u.Routes = []routeEntry{
 		// Git Clone
 		route("GET", gitProjectPattern+`info/refs\z`, git.GetInfoRefsHandler(api, &u.Config)),
-		route("POST", gitProjectPattern+`git-upload-pack\z`, contentEncodingHandler(git.PostRPC(api))),
-		route("POST", gitProjectPattern+`git-receive-pack\z`, contentEncodingHandler(git.PostRPC(api))),
-		route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, lfs.PutStore(api, proxy)),
+		route("POST", gitProjectPattern+`git-upload-pack\z`, contentEncodingHandler(git.PostRPC(api)), isContentType("application/x-git-upload-pack-request")),
+		route("POST", gitProjectPattern+`git-receive-pack\z`, contentEncodingHandler(git.PostRPC(api)), isContentType("application/x-git-receive-pack-request")),
+		route("PUT", gitProjectPattern+`gitlab-lfs/objects/([0-9a-f]{64})/([0-9]+)\z`, lfs.PutStore(api, proxy), isContentType("application/octet-stream")),
 
 		// CI Artifacts
 		route("POST", ciAPIPattern+`v1/builds/[0-9]+/artifacts\z`, contentEncodingHandler(artifacts.UploadArtifacts(api, proxy))),

main_test.go

@@ -178,6 +178,8 @@ func TestRegularProjectsAPI(t *testing.T) {
 		"/api/v3/projects/foo%2Fbar/repository/not/special",
 		"/api/v3/projects/123/not/special",
 		"/api/v3/projects/foo%2Fbar/not/special",
+		"/api/v3/projects/foo%2Fbar%2Fbaz/repository/not/special",
+		"/api/v3/projects/foo%2Fbar%2Fbaz%2Fqux/repository/not/special",
 	} {
 		resp, err := http.Get(ws.URL + resource)
 		if err != nil {
@@ -622,6 +624,73 @@ func TestGetInfoRefsHandledLocallyDueToEmptyGitalySocketPath(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(api.RequestHeader) != "" && r.Method != "GET" {
+			w.WriteHeader(500)
+			w.Write([]byte("non-GET request went through PreAuthorize handler"))
+		} else {
+			w.Header().Set("Content-Type", "text/html")
+			if _, err := w.Write(goodResponse); err != nil {
+				t.Fatalf("write upstream response: %v", err)
+			}
+		}
+	})
+	defer ts.Close()
+
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	// Each of these cases is a specially-handled path in Workhorse that may
+	// actually be a request to be sent to gitlab-rails.
+	for _, tc := range []struct {
+		method string
+		path   string
+	}{
+		{"GET", "/nested/group/project/blob/master/foo.git/info/refs"},
+		{"POST", "/nested/group/project/blob/master/foo.git/git-upload-pack"},
+		{"POST", "/nested/group/project/blob/master/foo.git/git-receive-pack"},
+		{"PUT", "/nested/group/project/blob/master/foo.git/gitlab-lfs/objects/0000000000000000000000000000000000000000000000000000000000000000/0"},
+		{"GET", "/nested/group/project/blob/master/environments/1/terminal.ws"},
+	} {
+		req, err := http.NewRequest(tc.method, ws.URL+tc.path, nil)
+		if err != nil {
+			t.Logf("Creating response for %+v failed: %v", tc, err)
+			t.Fail()
+			continue
+		}
+		resp, err := http.DefaultClient.Do(req)
+		if err != nil {
+			t.Logf("Reading response from workhorse for %+v failed: %s", tc, err)
+			t.Fail()
+			continue
+		}
+		defer resp.Body.Close()
+
+		respBody, err := ioutil.ReadAll(resp.Body)
+		if err != nil {
+			t.Logf("Reading response from workhorse for %+v failed: %s", tc, err)
+			t.Fail()
+		}
+
+		if resp.StatusCode != http.StatusOK {
+			t.Logf("Expected HTTP 200 response for %+v, got %d. Body: %s", tc, resp.StatusCode, string(respBody))
+			t.Fail()
+		}
+
+		if resp.Header.Get("Content-Type") != "text/html" {
+			t.Logf("Unexpected response content type for %+v: %s", tc, resp.Header.Get("Content-Type"))
+			t.Fail()
+		}
+
+		if bytes.Compare(respBody, goodResponse) != 0 {
+			t.Logf("Unexpected response body for %+v: %s", tc, string(respBody))
+			t.Fail()
+		}
+	}
+}
+
 func setupStaticFile(fpath, content string) error {
 	cwd, err := os.Getwd()
 	if err != nil {