Merge branch '97-insecure-redirects' into 'master'v0.6.1

Serve a secure redirect in case of accessing /foo

See merge request gitlab/gitlab-pages!3

4 files changed, 84 insertions, 58 deletions

diff --git a/CHANGELOG b/CHANGELOG
index e9312f3a..554ef8ec 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,5 +1,7 @@
-v 0.6.0
+v 0.6.1
+- Sanitize redirects by issuing a complete URI
 
+v 0.6.0
 - Use namsral/flag to support environment vars for config !40
 - Cleanup the README file !41
 - Add an artifacts proxy to GitLab Pages !44 !46
diff --git a/VERSION b/VERSION
index a918a2aa..ee6cdce3 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-0.6.0
+0.6.1
diff --git a/domain.go b/domain.go
index 28c89077..5d406fdc 100644
--- a/domain.go
+++ b/domain.go
@@ -17,6 +17,15 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httputil"
 )
 
+type locationDirectoryError struct {
+	FullPath     string
+	RelativePath string
+}
+
+func (l *locationDirectoryError) Error() string {
+	return "location error accessing directory where file expected"
+}
+
 type domain struct {
 	Group            string
 	Project          string
@@ -115,20 +124,45 @@ func (d *domain) serveCustomFile(w http.ResponseWriter, r *http.Request, code in
 	return nil
 }
 
-func (d *domain) resolvePath(projectName string, subPath ...string) (fullPath string, err error) {
+// Resolve the HTTP request to a path on disk, converting requests for
+// directories to requests for index.html inside the directory if appropriate.
+func (d *domain) resolvePath(projectName string, subPath ...string) (string, error) {
 	publicPath := filepath.Join(d.Group, projectName, "public")
 
-	fullPath = filepath.Join(publicPath, filepath.Join(subPath...))
-	fullPath, err = filepath.EvalSymlinks(fullPath)
+	// Don't use filepath.Join as cleans the path,
+	// where we want to traverse full path as supplied by user
+	// (including ..)
+	testPath := publicPath + "/" + strings.Join(subPath, "/")
+	fullPath, err := filepath.EvalSymlinks(testPath)
 	if err != nil {
-		return
+		return "", err
 	}
 
+	// The requested path resolved to somewhere outside of the public/ directory
 	if !strings.HasPrefix(fullPath, publicPath+"/") && fullPath != publicPath {
-		err = fmt.Errorf("%q should be in %q", fullPath, publicPath)
-		return
+		return "", fmt.Errorf("%q should be in %q", fullPath, publicPath)
+	}
+
+	fi, err := os.Lstat(fullPath)
+	if err != nil {
+		return "", err
+	}
+
+	// The requested path is a directory, so try index.html via recursion
+	if fi.IsDir() {
+		return "", &locationDirectoryError{
+			FullPath:     fullPath,
+			RelativePath: strings.TrimPrefix(fullPath, publicPath),
+		}
+	}
+
+	// The file exists, but is not a supported type to serve. Perhaps a block
+	// special device or something else that may be a security risk.
+	if !fi.Mode().IsRegular() {
+		return "", fmt.Errorf("%s: is not a regular file", fullPath)
 	}
-	return
+
+	return fullPath, nil
 }
 
 func (d *domain) tryNotFound(w http.ResponseWriter, r *http.Request, projectName string) error {
@@ -137,12 +171,6 @@ func (d *domain) tryNotFound(w http.ResponseWriter, r *http.Request, projectName
 		return err
 	}
 
-	// Make sure that file is not symlink
-	fi, err := os.Lstat(page404)
-	if err != nil && !fi.Mode().IsRegular() {
-		return err
-	}
-
 	err = d.serveCustomFile(w, r, http.StatusNotFound, page404)
 	if err != nil {
 		return err
@@ -150,45 +178,30 @@ func (d *domain) tryNotFound(w http.ResponseWriter, r *http.Request, projectName
 	return nil
 }
 
-func (d *domain) checkPath(w http.ResponseWriter, r *http.Request, path string) (fullPath string, err error) {
-	fullPath = path
-	fi, err := os.Lstat(fullPath)
-	if err != nil {
-		return
-	}
-
-	switch {
-	// If the URL doesn't end with /, send location to client
-	case fi.IsDir() && !endsWithSlash(r.URL.Path):
-		newURL := *r.URL
-		newURL.Path += "/"
-		http.Redirect(w, r, newURL.String(), 302)
-
-		// If this is directory, we try the index.html
-	case fi.IsDir():
-		fullPath = filepath.Join(fullPath, "index.html")
-		fi, err = os.Lstat(fullPath)
-		if err != nil {
-			return
+func (d *domain) tryFile(w http.ResponseWriter, r *http.Request, projectName, pathSuffix string, subPath ...string) error {
+	fullPath, err := d.resolvePath(projectName, subPath...)
+
+	if locationError, _ := err.(*locationDirectoryError); locationError != nil {
+		if endsWithSlash(r.URL.Path) {
+			fullPath, err = d.resolvePath(projectName, filepath.Join(subPath...), "index.html")
+		} else {
+			redirectPath := "//" + r.Host + "/"
+			if pathSuffix != "" {
+				redirectPath += pathSuffix + "/"
+			}
+			if locationError.RelativePath != "" {
+				redirectPath += strings.TrimPrefix(locationError.RelativePath, "/") + "/"
+			}
+			http.Redirect(w, r, redirectPath, 302)
+			return nil
 		}
-
-		// We don't allow to open the regular file
-	case !fi.Mode().IsRegular():
-		err = fmt.Errorf("%s: is not a regular file", fullPath)
 	}
-	return
-}
 
-func (d *domain) tryFile(w http.ResponseWriter, r *http.Request, projectName string, subPath ...string) error {
-	path, err := d.resolvePath(projectName, subPath...)
 	if err != nil {
 		return err
 	}
-	path, err = d.checkPath(w, r, path)
-	if err != nil {
-		return err
-	}
-	return d.serveFile(w, r, path)
+
+	return d.serveFile(w, r, fullPath)
 }
 
 func (d *domain) serveFromGroup(w http.ResponseWriter, r *http.Request) {
@@ -196,12 +209,12 @@ func (d *domain) serveFromGroup(w http.ResponseWriter, r *http.Request) {
 	split := strings.SplitN(r.URL.Path, "/", 3)
 
 	// Try to serve file for http://group.example.com/subpath/... => /group/subpath/...
-	if len(split) >= 2 && d.tryFile(w, r, split[1], split[2:]...) == nil {
+	if len(split) >= 2 && d.tryFile(w, r, split[1], split[1], split[2:]...) == nil {
 		return
 	}
 
 	// Try to serve file for http://group.example.com/... => /group/group.example.com/...
-	if r.Host != "" && d.tryFile(w, r, strings.ToLower(r.Host), r.URL.Path) == nil {
+	if r.Host != "" && d.tryFile(w, r, strings.ToLower(r.Host), "", r.URL.Path) == nil {
 		return
 	}
 
@@ -221,7 +234,7 @@ func (d *domain) serveFromGroup(w http.ResponseWriter, r *http.Request) {
 
 func (d *domain) serveFromConfig(w http.ResponseWriter, r *http.Request) {
 	// Try to serve file for http://host/... => /group/project/...
-	if d.tryFile(w, r, d.Project, r.URL.Path) == nil {
+	if d.tryFile(w, r, d.Project, "", r.URL.Path) == nil {
 		return
 	}
 
diff --git a/domain_test.go b/domain_test.go
index e1d5154f..26be21eb 100644
--- a/domain_test.go
+++ b/domain_test.go
@@ -24,18 +24,24 @@ func TestGroupServeHTTP(t *testing.T) {
 
 	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/", nil, "main-dir")
 	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/index.html", nil, "main-dir")
-	assert.True(t, assert.HTTPRedirect(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project", nil))
+	assert.HTTPRedirect(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project", nil)
+	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project", nil,
+		`<a href="//group.test.io/project/">Found</a>`)
 	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project/", nil, "project-subdir")
 	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project/index.html", nil, "project-subdir")
-	assert.True(t, assert.HTTPRedirect(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project/subdir", nil))
+	assert.HTTPRedirect(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project/subdir", nil)
+	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project/subdir", nil,
+		`<a href="//group.test.io/project/subdir/">Found</a>`)
 	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project/subdir/", nil, "project-subsubdir")
 	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project2/", nil, "project2-main")
 	assert.HTTPBodyContains(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project2/index.html", nil, "project2-main")
-	assert.True(t, assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/symlink", nil))
-	assert.True(t, assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/symlink/index.html", nil))
-	assert.True(t, assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/symlink/subdir/", nil))
-	assert.True(t, assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project/fifo", nil))
-	assert.True(t, assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/not-existing-file", nil))
+	assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io//about.gitlab.com/%2e%2e", nil)
+	assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/symlink", nil)
+	assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/symlink/index.html", nil)
+	assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/symlink/subdir/", nil)
+	assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project/fifo", nil)
+	assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/not-existing-file", nil)
+	assert.HTTPError(t, testGroup.ServeHTTP, "GET", "http://group.test.io/project//about.gitlab.com/%2e%2e", nil)
 }
 
 func TestDomainServeHTTP(t *testing.T) {
@@ -49,9 +55,14 @@ func TestDomainServeHTTP(t *testing.T) {
 		},
 	}
 
+	assert.HTTPBodyContains(t, testDomain.ServeHTTP, "GET", "/", nil, "project2-main")
 	assert.HTTPBodyContains(t, testDomain.ServeHTTP, "GET", "/index.html", nil, "project2-main")
 	assert.HTTPRedirect(t, testDomain.ServeHTTP, "GET", "/subdir", nil)
+	assert.HTTPBodyContains(t, testDomain.ServeHTTP, "GET", "/subdir", nil,
+		`<a href="/subdir/">Found</a>`)
 	assert.HTTPBodyContains(t, testDomain.ServeHTTP, "GET", "/subdir/", nil, "project2-subdir")
+	assert.HTTPBodyContains(t, testDomain.ServeHTTP, "GET", "/subdir/index.html", nil, "project2-subdir")
+	assert.HTTPError(t, testDomain.ServeHTTP, "GET", "//about.gitlab.com/%2e%2e", nil)
 	assert.HTTPError(t, testDomain.ServeHTTP, "GET", "/not-existing-file", nil)
 }