From 7bc7eb91d327404eb782fe844dd078f5740fa648 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 9 Jun 2026 22:00:26 +0200 Subject: [PATCH 1/2] Fix prefix-boundary hole in filer root-escape checks The root containment check in WorkspaceRootPath.Join and localRootPath.Join used a bare prefix match, so paths resolving to a sibling directory that shares the root as a string prefix (for example "/root-evil" for root "/root") passed the check. Require a separator boundary after the root, keep exact-root joins allowed, and preserve the unrooted local filer and "/"-rooted filer behavior. Co-authored-by: Isaac --- libs/filer/local_root_path.go | 18 +++++++++- libs/filer/local_root_path_test.go | 46 ++++++++++++++++++++++++++ libs/filer/workspace_root_path.go | 11 +++++- libs/filer/workspace_root_path_test.go | 16 +++++++++ 4 files changed, 89 insertions(+), 2 deletions(-) diff --git a/libs/filer/local_root_path.go b/libs/filer/local_root_path.go index 3f8843093aa..ed300c40fa1 100644 --- a/libs/filer/local_root_path.go +++ b/libs/filer/local_root_path.go @@ -19,7 +19,23 @@ func NewLocalRootPath(root string) localRootPath { func (rp *localRootPath) Join(name string) (string, error) { absPath := filepath.Join(rp.rootPath, name) - if !strings.HasPrefix(absPath, rp.rootPath) { + + // An empty root carries no restriction (cmd/fs constructs unrooted local filers). + if rp.rootPath == "" { + return absPath, nil + } + + // Don't allow escaping the specified root using relative paths. + // Joining exactly the root must stay allowed: calls like ReadDir(".") resolve to it. + // Any other path must extend the root past a separator boundary; a plain prefix + // check would also accept siblings like "/root-evil" for root "/root". + // The suffix guard covers roots that already end in a separator after + // cleaning ("/" or a Windows drive root like `C:\`). + root := rp.rootPath + if !strings.HasSuffix(root, string(filepath.Separator)) { + root += string(filepath.Separator) + } + if absPath != rp.rootPath && !strings.HasPrefix(absPath, root) { return "", fmt.Errorf("relative path escapes root: %s", name) } return absPath, nil diff --git a/libs/filer/local_root_path_test.go b/libs/filer/local_root_path_test.go index 1a39c4463b3..40b04bd866d 100644 --- a/libs/filer/local_root_path_test.go +++ b/libs/filer/local_root_path_test.go @@ -52,6 +52,12 @@ func testUnixLocalRootPath(t *testing.T, uncleanRoot string) { assert.NoError(t, err) assert.Equal(t, cleanRoot, remotePath) + // Escaping the root and re-entering it is allowed. + // All roots in this test end in the path element "path". + remotePath, err = rp.Join("../path/x") + assert.NoError(t, err) + assert.Equal(t, cleanRoot+"/x", remotePath) + _, err = rp.Join("..") assert.ErrorContains(t, err, `relative path escapes root: ..`) @@ -78,6 +84,16 @@ func testUnixLocalRootPath(t *testing.T, uncleanRoot string) { _, err = rp.Join("../..") assert.ErrorContains(t, err, `relative path escapes root: ../..`) + + // Siblings that share the root as a string prefix must be rejected. + _, err = rp.Join("../path-evil") + assert.ErrorContains(t, err, `relative path escapes root: ../path-evil`) + + _, err = rp.Join("../path-evil/x") + assert.ErrorContains(t, err, `relative path escapes root: ../path-evil/x`) + + _, err = rp.Join("../pathx") + assert.ErrorContains(t, err, `relative path escapes root: ../pathx`) } func TestUnixLocalRootPath(t *testing.T) { @@ -128,6 +144,22 @@ func testWindowsLocalRootPath(t *testing.T, uncleanRoot string) { _, err = rp.Join(`a\..\..`) assert.ErrorContains(t, err, `relative path escapes root`) + + // Escaping the root and re-entering it is allowed. + // All roots in this test end in the path element "path". + remotePath, err = rp.Join(`..\path\x`) + assert.NoError(t, err) + assert.Equal(t, cleanRoot+`\x`, remotePath) + + // Siblings that share the root as a string prefix must be rejected. + _, err = rp.Join(`..\path-evil`) + assert.ErrorContains(t, err, `relative path escapes root`) + + _, err = rp.Join(`..\path-evil\x`) + assert.ErrorContains(t, err, `relative path escapes root`) + + _, err = rp.Join(`..\pathx`) + assert.ErrorContains(t, err, `relative path escapes root`) } func TestWindowsLocalRootPath(t *testing.T) { @@ -140,3 +172,17 @@ func TestWindowsLocalRootPath(t *testing.T) { testWindowsLocalRootPath(t, `c:\some\root\path\.`) testWindowsLocalRootPath(t, `C:\some\root\..\path\`) } + +func TestLocalRootPathEmptyRoot(t *testing.T) { + // An empty root is the unrooted local filer used by cmd/fs; + // it allows any path, including relative paths outside the CWD. + rp := NewLocalRootPath("") + + p, err := rp.Join("a/b") + assert.NoError(t, err) + assert.Equal(t, filepath.Clean("a/b"), p) + + p, err = rp.Join("../a") + assert.NoError(t, err) + assert.Equal(t, filepath.Clean("../a"), p) +} diff --git a/libs/filer/workspace_root_path.go b/libs/filer/workspace_root_path.go index d5163924a15..f3bc8777187 100644 --- a/libs/filer/workspace_root_path.go +++ b/libs/filer/workspace_root_path.go @@ -26,7 +26,16 @@ func (p *WorkspaceRootPath) Join(name string) (string, error) { absPath := path.Join(p.rootPath, name) // Don't allow escaping the specified root using relative paths. - if !strings.HasPrefix(absPath, p.rootPath) { + // Joining exactly the root must stay allowed: calls like ReadDir(".") resolve to it. + // Any other path must extend the root past a separator boundary; a plain prefix + // check would also accept siblings like "/root-evil" for root "/root". + // The suffix guard covers filers rooted at "/" (see cmd/fs), where the cleaned + // root already ends in a separator. + root := p.rootPath + if !strings.HasSuffix(root, "/") { + root += "/" + } + if absPath != p.rootPath && !strings.HasPrefix(absPath, root) { return "", fmt.Errorf("relative path escapes root: %s", name) } diff --git a/libs/filer/workspace_root_path_test.go b/libs/filer/workspace_root_path_test.go index 73746d274f8..f5ee23f6dab 100644 --- a/libs/filer/workspace_root_path_test.go +++ b/libs/filer/workspace_root_path_test.go @@ -51,6 +51,12 @@ func testRootPath(t *testing.T, uncleanRoot string) { assert.NoError(t, err) assert.Equal(t, cleanRoot, remotePath) + // Escaping the root and re-entering it is allowed. + // All roots in this test end in the path element "path". + remotePath, err = rp.Join("../path/x") + assert.NoError(t, err) + assert.Equal(t, cleanRoot+"/x", remotePath) + _, err = rp.Join("..") assert.ErrorContains(t, err, `relative path escapes root: ..`) @@ -77,6 +83,16 @@ func testRootPath(t *testing.T, uncleanRoot string) { _, err = rp.Join("../..") assert.ErrorContains(t, err, `relative path escapes root: ../..`) + + // Siblings that share the root as a string prefix must be rejected. + _, err = rp.Join("../path-evil") + assert.ErrorContains(t, err, `relative path escapes root: ../path-evil`) + + _, err = rp.Join("../path-evil/x") + assert.ErrorContains(t, err, `relative path escapes root: ../path-evil/x`) + + _, err = rp.Join("../pathx") + assert.ErrorContains(t, err, `relative path escapes root: ../pathx`) } func TestRootPathClean(t *testing.T) { From a6edb6abdeee8688ca62a33dd0df8dee1fd9b0a4 Mon Sep 17 00:00:00 2001 From: simonfaltum Date: Tue, 9 Jun 2026 23:55:58 +0200 Subject: [PATCH 2/2] Remove redundant comments Co-authored-by: Isaac --- libs/filer/local_root_path.go | 4 +--- libs/filer/local_root_path_test.go | 5 +---- libs/filer/workspace_root_path.go | 3 +-- libs/filer/workspace_root_path_test.go | 1 - 4 files changed, 3 insertions(+), 10 deletions(-) diff --git a/libs/filer/local_root_path.go b/libs/filer/local_root_path.go index ed300c40fa1..f2bbe93be3c 100644 --- a/libs/filer/local_root_path.go +++ b/libs/filer/local_root_path.go @@ -25,12 +25,10 @@ func (rp *localRootPath) Join(name string) (string, error) { return absPath, nil } - // Don't allow escaping the specified root using relative paths. // Joining exactly the root must stay allowed: calls like ReadDir(".") resolve to it. // Any other path must extend the root past a separator boundary; a plain prefix // check would also accept siblings like "/root-evil" for root "/root". - // The suffix guard covers roots that already end in a separator after - // cleaning ("/" or a Windows drive root like `C:\`). + // Cleaned roots like "/" or `C:\` already end in a separator. root := rp.rootPath if !strings.HasSuffix(root, string(filepath.Separator)) { root += string(filepath.Separator) diff --git a/libs/filer/local_root_path_test.go b/libs/filer/local_root_path_test.go index 40b04bd866d..a665a9de7bc 100644 --- a/libs/filer/local_root_path_test.go +++ b/libs/filer/local_root_path_test.go @@ -52,7 +52,6 @@ func testUnixLocalRootPath(t *testing.T, uncleanRoot string) { assert.NoError(t, err) assert.Equal(t, cleanRoot, remotePath) - // Escaping the root and re-entering it is allowed. // All roots in this test end in the path element "path". remotePath, err = rp.Join("../path/x") assert.NoError(t, err) @@ -145,7 +144,6 @@ func testWindowsLocalRootPath(t *testing.T, uncleanRoot string) { _, err = rp.Join(`a\..\..`) assert.ErrorContains(t, err, `relative path escapes root`) - // Escaping the root and re-entering it is allowed. // All roots in this test end in the path element "path". remotePath, err = rp.Join(`..\path\x`) assert.NoError(t, err) @@ -174,8 +172,7 @@ func TestWindowsLocalRootPath(t *testing.T) { } func TestLocalRootPathEmptyRoot(t *testing.T) { - // An empty root is the unrooted local filer used by cmd/fs; - // it allows any path, including relative paths outside the CWD. + // An empty root is the unrooted local filer used by cmd/fs. rp := NewLocalRootPath("") p, err := rp.Join("a/b") diff --git a/libs/filer/workspace_root_path.go b/libs/filer/workspace_root_path.go index f3bc8777187..5da25c86ae8 100644 --- a/libs/filer/workspace_root_path.go +++ b/libs/filer/workspace_root_path.go @@ -29,8 +29,7 @@ func (p *WorkspaceRootPath) Join(name string) (string, error) { // Joining exactly the root must stay allowed: calls like ReadDir(".") resolve to it. // Any other path must extend the root past a separator boundary; a plain prefix // check would also accept siblings like "/root-evil" for root "/root". - // The suffix guard covers filers rooted at "/" (see cmd/fs), where the cleaned - // root already ends in a separator. + // Cleaned roots like "/" (see cmd/fs) already end in a separator. root := p.rootPath if !strings.HasSuffix(root, "/") { root += "/" diff --git a/libs/filer/workspace_root_path_test.go b/libs/filer/workspace_root_path_test.go index f5ee23f6dab..beadf833b5b 100644 --- a/libs/filer/workspace_root_path_test.go +++ b/libs/filer/workspace_root_path_test.go @@ -51,7 +51,6 @@ func testRootPath(t *testing.T, uncleanRoot string) { assert.NoError(t, err) assert.Equal(t, cleanRoot, remotePath) - // Escaping the root and re-entering it is allowed. // All roots in this test end in the path element "path". remotePath, err = rp.Join("../path/x") assert.NoError(t, err)