From 00d1ddcc89a9a79dcd8008302bba6d1a927bb6ca Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 8 Sep 2020 15:21:56 +0200 Subject: [PATCH 1/2] Fix `symlink` specs on OSX The fix is a little unconventional: - we treat the absolute paths as local to `rootPath` - this actually makes sense, given that this VFS should only work in this context --- internal/serving/disk/symlink/path_test.go | 6 +++++- internal/vfs/local/root.go | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/serving/disk/symlink/path_test.go b/internal/serving/disk/symlink/path_test.go index 6b0a41f3f..c802a410a 100644 --- a/internal/serving/disk/symlink/path_test.go +++ b/internal/serving/disk/symlink/path_test.go @@ -26,6 +26,10 @@ func tmpDir(t *testing.T) (vfs.Root, string, func()) { tmpDir, err := ioutil.TempDir("", "symlink_tests") require.NoError(t, err) + // On some systems `/tmp` can be a symlink + tmpDir, err = filepath.EvalSymlinks(tmpDir) + require.NoError(t, err) + root, err := fs.Root(context.Background(), tmpDir) require.NoError(t, err) @@ -70,7 +74,7 @@ var EvalSymlinksTests = []EvalSymlinksTest{ {"test/link2/..", "test"}, {"test/dir/link3", "."}, {"test/link2/link3/test", "test"}, - {"test/linkabs", "../.."}, + {"test/linkabs", "."}, {"test/link4/..", "test"}, {"src/versions/current/modules/test", "src/pool/test"}, } diff --git a/internal/vfs/local/root.go b/internal/vfs/local/root.go index 0ed2206df..19facf457 100644 --- a/internal/vfs/local/root.go +++ b/internal/vfs/local/root.go @@ -66,6 +66,9 @@ func (r *Root) Readlink(ctx context.Context, name string) (string, error) { } if filepath.IsAbs(target) { + // target is always scoped to the current `r.rootPath` + target = filepath.Join(r.rootPath, target) + return filepath.Rel(filepath.Dir(fullPath), target) } -- GitLab From 46841cad4db6c7bd016a69999a5aa6298a05ad0f Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 9 Sep 2020 11:05:43 +0200 Subject: [PATCH 2/2] Fix handling of absolute paths for `vfs/local` This resolves absolute paths to be relative if within a `rootPath` or absolute if outside. The `EvalSymlink` and the later usage of `VFS` will ensure that path is sanitised --- internal/serving/disk/symlink/path_test.go | 2 +- internal/vfs/local/root.go | 13 ++++++--- internal/vfs/local/root_test.go | 32 +++++++++++++++++++--- 3 files changed, 38 insertions(+), 9 deletions(-) diff --git a/internal/serving/disk/symlink/path_test.go b/internal/serving/disk/symlink/path_test.go index c802a410a..23978037d 100644 --- a/internal/serving/disk/symlink/path_test.go +++ b/internal/serving/disk/symlink/path_test.go @@ -74,7 +74,7 @@ var EvalSymlinksTests = []EvalSymlinksTest{ {"test/link2/..", "test"}, {"test/dir/link3", "."}, {"test/link2/link3/test", "test"}, - {"test/linkabs", "."}, + {"test/linkabs", "/"}, {"test/link4/..", "test"}, {"src/versions/current/modules/test", "src/pool/test"}, } diff --git a/internal/vfs/local/root.go b/internal/vfs/local/root.go index 19facf457..9cd67a9f0 100644 --- a/internal/vfs/local/root.go +++ b/internal/vfs/local/root.go @@ -65,13 +65,18 @@ func (r *Root) Readlink(ctx context.Context, name string) (string, error) { return "", err } - if filepath.IsAbs(target) { - // target is always scoped to the current `r.rootPath` - target = filepath.Join(r.rootPath, target) - + // If `target` is local to `rootPath` return relative + if strings.HasPrefix(target, r.rootPath+"/") { return filepath.Rel(filepath.Dir(fullPath), target) } + // If `target` is absolute return as-is making `EvalSymlinks` + // to discover misuse of a root path + if filepath.IsAbs(target) { + return target, nil + } + + // If `target` is relative, return as-is return target, nil } diff --git a/internal/vfs/local/root_test.go b/internal/vfs/local/root_test.go index c8ee6616e..f89e7736d 100644 --- a/internal/vfs/local/root_test.go +++ b/internal/vfs/local/root_test.go @@ -119,7 +119,8 @@ func TestReadlink(t *testing.T) { func TestReadlinkAbsolutePath(t *testing.T) { // create structure as: // /tmp/dir: directory - // /tmp/dir/symlink: points to `/tmp/file` + // /tmp/dir/symlink: points to `/tmp/file` outside of the `/tmp/dir` + // /tmp/dir/symlink2: points to `/tmp/dir/file` tmpDir, cleanup := tmpDir(t) defer cleanup() @@ -132,13 +133,36 @@ func TestReadlinkAbsolutePath(t *testing.T) { err = os.Symlink(filePath, symlinkPath) require.NoError(t, err) - root, err := localVFS.Root(context.Background(), dirPath) + symlinkPath = filepath.Join(dirPath, "symlink2") + dirFilePath := filepath.Join(dirPath, "file") + err = os.Symlink(dirFilePath, symlinkPath) require.NoError(t, err) - targetPath, err := root.Readlink(context.Background(), "symlink") + root, err := localVFS.Root(context.Background(), dirPath) require.NoError(t, err) - assert.Equal(t, "../file", targetPath, "the relative path is returned") + tests := map[string]struct { + path string + expectedTarget string + }{ + "the absolute path is returned for file outside of `/tmp/dir": { + path: "symlink", + expectedTarget: filePath, + }, + "the relative path is returned for file inside the `/tmp/dir": { + path: "symlink2", + expectedTarget: "file", + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + targetPath, err := root.Readlink(context.Background(), test.path) + require.NoError(t, err) + + assert.Equal(t, test.expectedTarget, targetPath) + }) + } } func TestLstat(t *testing.T) { -- GitLab