Merge branch 'uploads-check'

.gitignore

 test/data
 test/scratch
 gitlab-workhorse
+test/public

.gitlab-ci.yml

 before_script:
-  - rm -rf /usr/local/go
-  - apt-get update -qq
-  - apt-get install -y curl unzip bzip2
+  - test -f /.dockerinit && apt-get update -qq && apt-get install -y curl unzip bzip2
   - curl -O https://storage.googleapis.com/golang/go1.5.2.linux-amd64.tar.gz
   - echo 'cae87ed095e8d94a81871281d35da7829bd1234e  go1.5.2.linux-amd64.tar.gz' | shasum -c -
-  - tar -C /usr/local -xzf go1.5.2.linux-amd64.tar.gz
+  - test -f /.dockerinit && rm -rf /usr/local/go && tar -C /usr/local -xzf go1.5.2.linux-amd64.tar.gz
   - export PATH=/usr/local/go/bin:$PATH
 
 test:

main.go

@@ -50,7 +50,9 @@ const projectPattern = `^/[^/]+/[^/]+/`
 const gitProjectPattern = `^/[^/]+/[^/]+\.git/`
 
 const apiPattern = `^/api/`
-const projectsAPIPattern = `^/api/v3/projects/[^/]+/`
+
+// 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/`
 
@@ -98,6 +100,16 @@ var httpRoutes = [...]httpRoute{
 		),
 	},
 
+	// For legacy reasons, user uploads are stored under the document root.
+	// To prevent anybody who knows/guesses the URL of a user-uploaded file
+	// from downloading it we make sure requests to /uploads/ do _not_ pass
+	// through handleServeFile.
+	httpRoute{"", regexp.MustCompile(`^/uploads/`),
+		handleRailsError(documentRoot,
+			proxyRequest,
+		),
+	},
+
 	// Serve static files or forward the requests
 	httpRoute{"", nil,
 		handleServeFile(documentRoot, CacheDisabled,

main_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"log"
 	"net/http"
@@ -19,6 +20,7 @@ import (
 
 const scratchDir = "test/scratch"
 const testRepoRoot = "test/data"
+const testDocumentRoot = "test/public"
 const testRepo = "group/test.git"
 const testProject = "group/test"
 
@@ -285,6 +287,143 @@ func TestDeniedXSendfileDownload(t *testing.T) {
 	deniedXSendfileDownload(t, contentFilename, "foo/uploads/bar")
 }
 
+func TestAllowedStaticFile(t *testing.T) {
+	content := "PUBLIC"
+	if err := setupStaticFile("static file.txt", content); err != nil {
+		t.Fatalf("create public/static file.txt: %v", err)
+	}
+
+	proxied := false
+	ts := testServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
+		proxied = true
+		w.WriteHeader(404)
+	})
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	for _, resource := range []string{
+		"/static%20file.txt",
+		"/static file.txt",
+	} {
+		resp, err := http.Get(ws.URL + resource)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer resp.Body.Close()
+		buf := &bytes.Buffer{}
+		if _, err := io.Copy(buf, resp.Body); err != nil {
+			t.Fatal(err)
+		}
+		if buf.String() != content {
+			t.Fatalf("GET %q: Expected %q, got %q", resource, content, buf.String())
+		}
+		if resp.StatusCode != 200 {
+			t.Fatalf("GET %q: expected 200, got %d", resource, resp.StatusCode)
+		}
+		if proxied {
+			t.Fatalf("GET %q: should not have made it to backend", resource)
+		}
+	}
+}
+
+func TestAllowedPublicUploadsFile(t *testing.T) {
+	content := "PRIVATE but allowed"
+	if err := setupStaticFile("uploads/static file.txt", content); err != nil {
+		t.Fatalf("create public/uploads/static file.txt: %v", err)
+	}
+
+	proxied := false
+	ts := testServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
+		proxied = true
+		w.Header().Add("X-Sendfile", *documentRoot+r.URL.Path)
+		w.WriteHeader(200)
+	})
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	for _, resource := range []string{
+		"/uploads/static%20file.txt",
+		"/uploads/static file.txt",
+	} {
+		resp, err := http.Get(ws.URL + resource)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer resp.Body.Close()
+		buf := &bytes.Buffer{}
+		if _, err := io.Copy(buf, resp.Body); err != nil {
+			t.Fatal(err)
+		}
+		if buf.String() != content {
+			t.Fatalf("GET %q: Expected %q, got %q", resource, content, buf.String())
+		}
+		if resp.StatusCode != 200 {
+			t.Fatalf("GET %q: expected 200, got %d", resource, resp.StatusCode)
+		}
+		if !proxied {
+			t.Fatalf("GET %q: never made it to backend", resource)
+		}
+	}
+}
+
+func TestDeniedPublicUploadsFile(t *testing.T) {
+	content := "PRIVATE"
+	if err := setupStaticFile("uploads/static.txt", content); err != nil {
+		t.Fatalf("create public/uploads/static.txt: %v", err)
+	}
+
+	proxied := false
+	ts := testServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, _ *http.Request) {
+		proxied = true
+		w.WriteHeader(404)
+	})
+	defer ts.Close()
+	ws := startWorkhorseServer(ts.URL)
+	defer ws.Close()
+
+	for _, resource := range []string{
+		"/uploads/static.txt",
+		"/uploads%2Fstatic.txt",
+	} {
+		resp, err := http.Get(ws.URL + resource)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer resp.Body.Close()
+		buf := &bytes.Buffer{}
+		if _, err := io.Copy(buf, resp.Body); err != nil {
+			t.Fatal(err)
+		}
+		if buf.String() == content {
+			t.Fatalf("GET %q: Got private file contents which should have been blocked by upstream", resource)
+		}
+		if resp.StatusCode != 404 {
+			t.Fatalf("GET %q: expected 404, got %d", resource, resp.StatusCode)
+		}
+		if !proxied {
+			t.Fatalf("GET %q: never made it to backend", resource)
+		}
+	}
+}
+
+func setupStaticFile(fpath, content string) error {
+	cwd, err := os.Getwd()
+	if err != nil {
+		return err
+	}
+	*documentRoot = path.Join(cwd, testDocumentRoot)
+	if err := os.MkdirAll(path.Join(*documentRoot, path.Dir(fpath)), 0755); err != nil {
+		return err
+	}
+	static_file := path.Join(*documentRoot, fpath)
+	if err := ioutil.WriteFile(static_file, []byte(content), 0666); err != nil {
+		return err
+	}
+	return nil
+}
+
 func prepareDownloadDir(t *testing.T) {
 	if err := os.RemoveAll(scratchDir); err != nil {
 		t.Fatal(err)

servefile.go

@@ -16,6 +16,9 @@ const (
 	CacheExpireMax
 )
 
+// BUG/QUIRK: If a client requests 'foo%2Fbar' and 'foo/bar' exists,
+// handleServeFile will serve foo/bar instead of passing the request
+// upstream.
 func handleServeFile(documentRoot *string, cache CacheMode, notFoundHandler serviceHandleFunc) serviceHandleFunc {
 	return func(w http.ResponseWriter, r *gitRequest) {
 		file := filepath.Join(*documentRoot, r.relativeURIPath)

upstream.go

@@ -107,7 +107,7 @@ func (u *upstream) ServeHTTP(ow http.ResponseWriter, r *http.Request) {
 	}
 
 	// Check URL Root
-	URIPath := cleanURIPath(r.URL.EscapedPath())
+	URIPath := cleanURIPath(r.URL.Path)
 	if !strings.HasPrefix(URIPath, u.relativeURLRoot) && URIPath+"/" != u.relativeURLRoot {
 		httpError(&w, r, fmt.Sprintf("Not found %q", URIPath), http.StatusNotFound)
 		return