Merge branch 'remove/chrootpath' into 'master'

refactor: clean up chrootPath from the httpfs package

Closes #598

See merge request gitlab-org/gitlab-pages!559

3 files changed, 7 insertions, 76 deletions

diff --git a/internal/httpfs/http_fs.go b/internal/httpfs/http_fs.go
index 3578352f..58cc9e74 100644
--- a/internal/httpfs/http_fs.go
+++ b/internal/httpfs/http_fs.go
@@ -8,7 +8,6 @@ package httpfs
 
 import (
 	"errors"
-	"fmt"
 	"net/http"
 	"os"
 	"path"
@@ -25,27 +24,15 @@ var (
 // fileSystemPaths implements the http.FileSystem interface
 type fileSystemPaths struct {
 	allowedPaths []string
-	// 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.
-// 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) {
+func NewFileSystemPath(allowedPaths []string) (http.FileSystem, error) {
 	for k, allowedPath := range allowedPaths {
-		strippedPath, err := stripChrootPath(ensureEndingSlash(allowedPath), chrootPath)
-		if err != nil {
-			return nil, err
-		}
+		strippedPath := ensureEndingSlash(allowedPath)
 
+		var err error
 		allowedPaths[k], err = filepath.Abs(strippedPath)
 		if err != nil {
 			return nil, err
@@ -54,7 +41,6 @@ func NewFileSystemPath(allowedPaths []string, chrootPath string) (http.FileSyste
 
 	return &fileSystemPaths{
 		allowedPaths: allowedPaths,
-		chrootPath:   chrootPath,
 	}, nil
 }
 
@@ -67,18 +53,7 @@ func (p *fileSystemPaths) Open(name string) (http.File, error) {
 
 	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: 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(os.ErrPermission)
-
-		return nil, os.ErrPermission
-	}
-
-	absPath, err := filepath.Abs(strippedPath)
+	absPath, err := filepath.Abs(cleanedPath)
 	if err != nil {
 		return nil, err
 	}
@@ -104,18 +79,3 @@ func ensureEndingSlash(path string) string {
 
 	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
-}
diff --git a/internal/httpfs/http_fs_test.go b/internal/httpfs/http_fs_test.go
index cbdbc0e1..ce404a11 100644
--- a/internal/httpfs/http_fs_test.go
+++ b/internal/httpfs/http_fs_test.go
@@ -19,7 +19,6 @@ func TestFSOpen(t *testing.T) {
 
 	tests := map[string]struct {
 		allowedPaths    []string
-		chrootPath      string
 		fileName        string
 		expectedContent string
 		expectedErrMsg  string
@@ -59,23 +58,11 @@ func TestFSOpen(t *testing.T) {
 			fileName:       wd + "/../httpfs/testdata/file1.txt",
 			expectedErrMsg: os.ErrPermission.Error(),
 		},
-		"chroot_path_not_found_when_not_in_real_chroot": {
-			allowedPaths:   []string{wd + "/testdata"},
-			fileName:       wd + "/testdata/file1.txt",
-			expectedErrMsg: "no such file or directory",
-			chrootPath:     wd + "/testdata",
-		},
-		"chroot_path_empty_finds_file": {
-			allowedPaths:    []string{wd + "/testdata"},
-			fileName:        wd + "/testdata/file1.txt",
-			expectedContent: "file1.txt\n",
-			chrootPath:      "",
-		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			p, err := NewFileSystemPath(test.allowedPaths, test.chrootPath)
+			p, err := NewFileSystemPath(test.allowedPaths)
 			require.NoError(t, err)
 
 			got, err := p.Open(test.fileName)
@@ -101,7 +88,6 @@ func TestFileSystemPathCanServeHTTP(t *testing.T) {
 
 	tests := map[string]struct {
 		path               string
-		chrootPath         string
 		fileName           string
 		escapeURL          bool
 		expectedStatusCode int
@@ -150,26 +136,12 @@ func TestFileSystemPathCanServeHTTP(t *testing.T) {
 			expectedStatusCode: http.StatusForbidden,
 			expectedContent:    "403 Forbidden\n",
 		},
-		"chroot_path_fails_in_unit_test_not_found_when_not_in_real_chroot": {
-			path:               wd + "/testdata",
-			fileName:           "file1.txt",
-			chrootPath:         wd + "/testdata",
-			expectedStatusCode: http.StatusNotFound,
-			expectedContent:    "404 page not found\n",
-		},
-		"chroot_path_empty_in_unit_test_file_found": {
-			path:               wd + "/testdata",
-			fileName:           "file1.txt",
-			chrootPath:         "",
-			expectedStatusCode: http.StatusOK,
-			expectedContent:    "file1.txt\n",
-		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
 			transport := httptransport.NewTransport()
-			fs, err := NewFileSystemPath([]string{test.path}, test.chrootPath)
+			fs, err := NewFileSystemPath([]string{test.path})
 			require.NoError(t, err)
 
 			transport.RegisterProtocol("file", http.NewFileTransport(fs))
diff --git a/internal/vfs/zip/vfs.go b/internal/vfs/zip/vfs.go
index 4118dd4a..c90d7614 100644
--- a/internal/vfs/zip/vfs.go
+++ b/internal/vfs/zip/vfs.go
@@ -123,8 +123,7 @@ func (fs *zipVFS) Reconfigure(cfg *config.Config) error {
 }
 
 func (fs *zipVFS) reconfigureTransport(cfg *config.Config) error {
-	// TODO: remove chrootPath from httpsfs package: https://gitlab.com/gitlab-org/gitlab-pages/-/issues/598
-	fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths, "")
+	fsTransport, err := httpfs.NewFileSystemPath(cfg.Zip.AllowedPaths)
 	if err != nil {
 		return err
 	}