Strip chrootPath from allowedPaths

1 files changed, 46 insertions, 33 deletions

diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go
index 1aa85f56..1e9dff1c 100644
--- a/internal/httpfs/http_fs.go
+++ b/internal/httpfs/http_fs.go
@@ -25,28 +25,28 @@ var (
 // fileSystemPaths implements the http.FileSystem interface
 type fileSystemPaths struct {
 	allowedPaths []string
-	// TODO: this is a temporary workaround for https://gitlab.com/gitlab-org/gitlab/-/issues/326117#note_546346101
-	// where daemon-inplace-chroot=true fails to serve zip archives when pages_serve_with_zip_file_protocol is enabled
-	// To be removed after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
+	// workaround for https://gitlab.com/gitlab-org/gitlab/-/issues/326117#note_546346101
+	// where daemon-inplace-chroot=true fails to serve zip archives when
+	// pages_serve_with_zip_file_protocol is enabled
+	// TODO: evaluate if we need to remove this field when we remove
+	// chroot https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
 	chrootPath string
 }
 
 // NewFileSystemPath creates a new fileSystemPaths that can be used to register
-// a file:// protocol with an http.Transport
+// a file:// protocol with an http.Transport.
+// When the daemon runs inside a chroot we need to strip chrootPath out of each
+// of the allowedPaths so that we are able to find the file correctly inside
+// the chroot. When Open is called, the same chrootPath will be stripped out of
+// the full filepath.
 func NewFileSystemPath(allowedPaths []string, chrootPath string) (http.FileSystem, error) {
-	for k, path := range allowedPaths {
-		var err error
-
-		if chrootPath != "" {
-			if !strings.HasPrefix(path, chrootPath) {
-				return nil, fmt.Errorf("allowed path %q is not in chroot path %q", path, chrootPath)
-			}
-
-			// strip chrootPath from each allowedPath
-			path = path[len(chrootPath):]
+	for k, allowedPath := range allowedPaths {
+		strippedPath, err := stripChrootPath(ensureEndingSlash(allowedPath), chrootPath)
+		if err != nil {
+			return nil, err
 		}
 
-		allowedPaths[k], err = filepath.Abs(path)
+		allowedPaths[k], err = filepath.Abs(strippedPath)
 		if err != nil {
 			return nil, err
 		}
@@ -65,34 +65,24 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) {
 		return nil, errInvalidChar
 	}
 
-	absPath := filepath.FromSlash(path.Clean("/" + name))
+	cleanedPath := filepath.FromSlash(path.Clean("/" + name))
 
 	// since deamon can run in a chroot, we allow to define a chroot path that will be stripped from
 	// the FS location
-	// TODO: To be removed after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
-	if p.chrootPath != "" {
-		if !strings.HasPrefix(absPath, p.chrootPath) {
-			log.WithError(os.ErrPermission).Errorf(
-				"requested filepath %q not in chroot path: %q", absPath, p.chrootPath)
-
-			return nil, os.ErrPermission
-		}
+	// TODO: evaluate if we need to remove this check when we remove chroot
+	// https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
+	strippedPath, err := stripChrootPath(cleanedPath, p.chrootPath)
+	if err != nil {
+		log.WithError(err).Error()
 
-		// since we run in a chroot path, strip it
-		absPath = absPath[len(p.chrootPath):]
+		return nil, os.ErrPermission
 	}
 
-	absPath, err := filepath.Abs(absPath)
+	absPath, err := filepath.Abs(strippedPath)
 	if err != nil {
 		return nil, err
 	}
 	for _, allowedPath := range p.allowedPaths {
-		// TODO: To be removed after we roll-out zip architecture completely https://gitlab.com/gitlab-org/gitlab-pages/-/issues/561
-		// return early if in chroot and we have stripped p.chrootPath from absPath && allowedPath
-		if p.chrootPath != "" && allowedPath == "/" {
-			return os.Open(absPath)
-		}
-
 		if strings.HasPrefix(absPath, allowedPath+"/") {
 			return os.Open(absPath)
 		}
@@ -105,3 +95,26 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) {
 	// https://github.com/golang/go/blob/release-branch.go1.15/src/net/http/fs.go#L635
 	return nil, os.ErrPermission
 }
+
+func ensureEndingSlash(path string) string {
+	if strings.HasSuffix(path, "/") {
+		return path
+	}
+
+	return path + "/"
+}
+
+func stripChrootPath(path, chrootPath string) (string, error) {
+	if chrootPath == "" {
+		return path, nil
+	}
+
+	if !strings.HasPrefix(path, chrootPath+"/") {
+		return "", fmt.Errorf("allowed path %q is not in chroot path %q", path, chrootPath)
+	}
+
+	// path will contain a leading `/`
+	path = path[len(chrootPath):]
+
+	return path, nil
+}