diff options
author | feistel <6742251-feistel@users.noreply.gitlab.com> | 2021-08-26 08:11:53 +0300 |
---|---|---|
committer | feistel <6742251-feistel@users.noreply.gitlab.com> | 2021-08-26 08:11:53 +0300 |
commit | aa333ba6e7c58fccb090318d4a2591a3cba645c6 (patch) | |
tree | d0a80c7c71f8929ffe6ce3b5c2deb939836338e7 /internal/httpfs | |
parent | 05596078dbefb4449ca1c85494ead8206174040c (diff) |
refactor: clean up chrootPath from the httpfs package
Diffstat (limited to 'internal/httpfs')
-rw-r--r-- | internal/httpfs/http_fs.go | 48 | ||||
-rw-r--r-- | internal/httpfs/http_fs_test.go | 32 |
2 files changed, 6 insertions, 74 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 f47e17ff..3d1092b6 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)) |