From 3988c28080217f042c89123bfe1d2ae5553c7400 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 11:50:14 -0400 Subject: [PATCH 1/9] feat(allowedpaths): support symlinks whose targets resolve within a different allowed root When an os.Root operation fails with "path escapes from parent", the sandbox now walks the relative path component by component to find the escaping symlink, reads its target, resolves it to an absolute path, and retries through whichever allowed root matches. This handles both leaf symlinks (e.g. link.txt -> /other-root/file.txt) and intermediate directory symlinks (e.g. pods/ -> /other-root/). A depth cap of 10 hops prevents infinite loops from circular symlinks. Closes #167 Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/sandbox.go | 236 +++++++++++++++++- allowedpaths/sandbox_unix_test.go | 199 +++++++++++++++ analysis/symbols_allowedpaths.go | 5 + .../allowed_paths/cross_root_symlink_dir.yaml | 18 ++ .../cross_root_symlink_outside_blocked.yaml | 18 ++ .../cross_root_symlink_read.yaml | 18 ++ 6 files changed, 493 insertions(+), 1 deletion(-) create mode 100644 tests/scenarios/shell/allowed_paths/cross_root_symlink_dir.yaml create mode 100644 tests/scenarios/shell/allowed_paths/cross_root_symlink_outside_blocked.yaml create mode 100644 tests/scenarios/shell/allowed_paths/cross_root_symlink_read.yaml diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index 4da1f32b..17e91d39 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -19,6 +19,13 @@ import ( "strings" ) +// Access mode bits for permission checks. +const ( + modeRead = 0x04 + modeWrite = 0x02 + modeExecute = 0x01 +) + // MaxGlobEntries is the maximum number of directory entries read per single // glob expansion step. ReadDirForGlob returns an error for directories that // exceed this limit to prevent memory exhaustion during pattern matching. @@ -65,6 +72,21 @@ func New(paths []string) (sb *Sandbox, warnings []byte, err error) { return &Sandbox{roots: roots}, buf.Bytes(), nil } +// isPathEscapeError reports whether err is the unexported "path escapes +// from parent" error from os.Root. Stable per Hyrum's Law. +func isPathEscapeError(err error) bool { + var pe *os.PathError + if errors.As(err, &pe) { + return pe.Err != nil && pe.Err.Error() == "path escapes from parent" + } + return false +} + +// maxSymlinkHops is the maximum number of symlink resolutions performed +// when following cross-root symlinks. Prevents infinite loops from +// circular symlinks. +const maxSymlinkHops = 10 + // resolve returns the matching os.Root and the path relative to it for the // given absolute path. It returns false if no root matches. func (s *Sandbox) resolve(absPath string) (*os.Root, string, bool) { @@ -84,6 +106,137 @@ func (s *Sandbox) resolve(absPath string) (*os.Root, string, bool) { return nil, "", false } +// resolveRoot is like resolve but returns the internal root entry instead +// of the os.Root handle, so callers can access accessCheck and absPath. +func (s *Sandbox) resolveRoot(absPath string) (*root, string, bool) { + if s == nil { + return nil, "", false + } + for i := range s.roots { + rel, err := filepath.Rel(s.roots[i].absPath, absPath) + if err != nil { + continue + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + continue + } + return &s.roots[i], rel, true + } + return nil, "", false +} + +// resolveRootFollowingSymlinks is the cross-root symlink fallback for +// resolveRoot. +func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, bool) { + for hops := 0; hops < maxSymlinkHops; hops++ { + ar, rel, ok := s.resolveRoot(absPath) + if !ok { + return nil, "", false + } + + components := strings.Split(rel, string(filepath.Separator)) + symlinkFound := false + for i, comp := range components { + if comp == "." { + continue + } + partial := strings.Join(components[:i+1], string(filepath.Separator)) + info, err := ar.root.Lstat(partial) + if err != nil { + return nil, "", false + } + if info.Mode()&fs.ModeSymlink == 0 { + continue + } + target, err := ar.root.Readlink(partial) + if err != nil { + return nil, "", false + } + if !filepath.IsAbs(target) { + parentAbs := absPath + for j := len(components) - 1; j >= i; j-- { + parentAbs = filepath.Dir(parentAbs) + } + target = filepath.Join(parentAbs, target) + } + if i+1 < len(components) { + remaining := strings.Join(components[i+1:], string(filepath.Separator)) + target = filepath.Join(target, remaining) + } + absPath = filepath.Clean(target) + symlinkFound = true + break + } + if !symlinkFound { + return ar, rel, true + } + } + return nil, "", false +} + +// resolveFollowingSymlinks resolves absPath to a (root, relPath) pair, +// following symlinks that cross between allowed roots. It walks the +// relative path component by component; when a component is a symlink, +// its target is resolved to an absolute path and matched against all +// roots, then resolution continues with the remaining components. +// +// This is only called as a fallback when the primary os.Root operation +// fails, so there is no overhead on the happy path. +func (s *Sandbox) resolveFollowingSymlinks(absPath string) (*os.Root, string, bool) { + for hops := 0; hops < maxSymlinkHops; hops++ { + root, rel, ok := s.resolve(absPath) + if !ok { + return nil, "", false + } + + // Walk rel component by component looking for symlinks. + components := strings.Split(rel, string(filepath.Separator)) + symlinkFound := false + for i, comp := range components { + if comp == "." { + continue + } + // Build the path up to and including this component. + partial := strings.Join(components[:i+1], string(filepath.Separator)) + info, err := root.Lstat(partial) + if err != nil { + return nil, "", false + } + if info.Mode()&fs.ModeSymlink == 0 { + continue + } + // Found a symlink — read its target. + target, err := root.Readlink(partial) + if err != nil { + return nil, "", false + } + // Resolve target to absolute path. + if !filepath.IsAbs(target) { + // Relative to the directory containing the symlink. + parentAbs := absPath + for j := len(components) - 1; j >= i; j-- { + parentAbs = filepath.Dir(parentAbs) + } + target = filepath.Join(parentAbs, target) + } + // Append remaining components after the symlink. + if i+1 < len(components) { + remaining := strings.Join(components[i+1:], string(filepath.Separator)) + target = filepath.Join(target, remaining) + } + absPath = filepath.Clean(target) + symlinkFound = true + break + } + if !symlinkFound { + // No symlinks found — return the resolved root and path. + return root, rel, true + } + // Loop again with the new absPath (may chain through another root). + } + return nil, "", false // too many hops +} + // Access checks whether the resolved path is accessible with the given mode. // All operations go through os.Root to stay within the sandbox. // Mode: 0x04 = read, 0x02 = write, 0x01 = execute. @@ -118,7 +271,23 @@ func (s *Sandbox) Access(path string, cwd string, mode uint32) error { // performs the permission check (fd-relative OpenFile with // O_NONBLOCK for reads on Unix, mode-bit inspection for // everything else). - _, err = ar.accessCheck(rel, mode&0x04 != 0, mode&0x02 != 0, mode&0x01 != 0) + checkRead := mode&modeRead != 0 + checkWrite := mode&modeWrite != 0 + checkExec := mode&modeExecute != 0 + + _, err = ar.accessCheck(rel, checkRead, checkWrite, checkExec) + if err == nil { + return nil + } + if !isPathEscapeError(err) { + return &os.PathError{Op: "access", Path: path, Err: os.ErrPermission} + } + // Symlink escapes this root — resolve across all roots. + resolved, resolvedRel, ok := s.resolveRootFollowingSymlinks(absPath) + if !ok { + return &os.PathError{Op: "access", Path: path, Err: os.ErrPermission} + } + _, err = resolved.accessCheck(resolvedRel, checkRead, checkWrite, checkExec) if err != nil { return &os.PathError{Op: "access", Path: path, Err: os.ErrPermission} } @@ -163,6 +332,18 @@ func (s *Sandbox) Open(path string, cwd string, flag int, perm os.FileMode) (io. } f, err := root.OpenFile(relPath, flag, perm) + if err == nil { + return f, nil + } + if !isPathEscapeError(err) { + return nil, PortablePathError(err) + } + // Symlink escapes this root — resolve across all roots. + r, rel, ok := s.resolveFollowingSymlinks(absPath) + if !ok { + return nil, PortablePathError(err) + } + f, err = r.OpenFile(rel, flag, perm) if err != nil { return nil, PortablePathError(err) } @@ -197,6 +378,11 @@ func (s *Sandbox) readDirN(path string, cwd string, maxEntries int) ([]fs.DirEnt } f, err := root.Open(relPath) + if err != nil && isPathEscapeError(err) { + if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok { + f, err = r.Open(rel) + } + } if err != nil { return nil, PortablePathError(err) } @@ -238,6 +424,11 @@ func (s *Sandbox) OpenDir(path string, cwd string) (fs.ReadDirFile, error) { } f, err := root.Open(relPath) + if err != nil && isPathEscapeError(err) { + if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok { + f, err = r.Open(rel) + } + } if err != nil { return nil, PortablePathError(err) } @@ -256,6 +447,11 @@ func (s *Sandbox) IsDirEmpty(path string, cwd string) (bool, error) { } f, err := root.Open(relPath) + if err != nil && isPathEscapeError(err) { + if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok { + f, err = r.Open(rel) + } + } if err != nil { return false, PortablePathError(err) } @@ -283,6 +479,11 @@ func (s *Sandbox) ReadDirLimited(path string, cwd string, offset, maxRead int) ( return nil, false, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} } f, err := root.Open(relPath) + if err != nil && isPathEscapeError(err) { + if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok { + f, err = r.Open(rel) + } + } if err != nil { return nil, false, PortablePathError(err) } @@ -378,6 +579,17 @@ func (s *Sandbox) Stat(path string, cwd string) (fs.FileInfo, error) { } info, err := root.Stat(relPath) + if err == nil { + return info, nil + } + if !isPathEscapeError(err) { + return nil, PortablePathError(err) + } + r, rel, ok := s.resolveFollowingSymlinks(absPath) + if !ok { + return nil, PortablePathError(err) + } + info, err = r.Stat(rel) if err != nil { return nil, PortablePathError(err) } @@ -401,6 +613,17 @@ func (s *Sandbox) Lstat(path string, cwd string) (fs.FileInfo, error) { } info, err := root.Lstat(relPath) + if err == nil { + return info, nil + } + if !isPathEscapeError(err) { + return nil, PortablePathError(err) + } + r, rel, ok := s.resolveFollowingSymlinks(absPath) + if !ok { + return nil, PortablePathError(err) + } + info, err = r.Lstat(rel) if err != nil { return nil, PortablePathError(err) } @@ -417,6 +640,17 @@ func (s *Sandbox) Readlink(path string, cwd string) (string, error) { } target, err := root.Readlink(relPath) + if err == nil { + return target, nil + } + if !isPathEscapeError(err) { + return "", PortablePathError(err) + } + r, rel, ok := s.resolveFollowingSymlinks(absPath) + if !ok { + return "", PortablePathError(err) + } + target, err = r.Readlink(rel) if err != nil { return "", PortablePathError(err) } diff --git a/allowedpaths/sandbox_unix_test.go b/allowedpaths/sandbox_unix_test.go index fdd1a6b3..a6f8ac3b 100644 --- a/allowedpaths/sandbox_unix_test.go +++ b/allowedpaths/sandbox_unix_test.go @@ -414,3 +414,202 @@ func TestAccessFIFONonBlocking(t *testing.T) { t.Fatal("Access blocked on FIFO — O_NONBLOCK not effective") } } + +// --- Cross-root symlink tests --- + +// TestCrossRootSymlinkOpen verifies that a symlink in one allowed root +// pointing to a file in another allowed root can be opened. +func TestCrossRootSymlinkOpen(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir1, "file.txt"), []byte("hello"), 0644)) + require.NoError(t, os.Symlink(filepath.Join(dir1, "file.txt"), filepath.Join(dir2, "link.txt"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + f, err := sb.Open("link.txt", dir2, os.O_RDONLY, 0) + require.NoError(t, err) + defer f.Close() + + buf := make([]byte, 64) + n, _ := f.Read(buf) + assert.Equal(t, "hello", string(buf[:n])) +} + +// TestCrossRootSymlinkStat verifies that Stat follows a cross-root symlink. +func TestCrossRootSymlinkStat(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir1, "file.txt"), []byte("hello"), 0644)) + require.NoError(t, os.Symlink(filepath.Join(dir1, "file.txt"), filepath.Join(dir2, "link.txt"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + info, err := sb.Stat("link.txt", dir2) + require.NoError(t, err) + assert.Equal(t, int64(5), info.Size()) + assert.False(t, info.Mode()&os.ModeSymlink != 0, "Stat should follow the symlink") +} + +// TestCrossRootSymlinkAccess verifies that Access works through a cross-root symlink. +func TestCrossRootSymlinkAccess(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir1, "file.txt"), []byte("hello"), 0644)) + require.NoError(t, os.Symlink(filepath.Join(dir1, "file.txt"), filepath.Join(dir2, "link.txt"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + assert.NoError(t, sb.Access("link.txt", dir2, modeRead)) +} + +// TestCrossRootSymlinkRelativeTarget verifies cross-root resolution with +// a relative symlink target (e.g. ../dir1/file.txt). +func TestCrossRootSymlinkRelativeTarget(t *testing.T) { + parent := t.TempDir() + dir1 := filepath.Join(parent, "dir1") + dir2 := filepath.Join(parent, "dir2") + require.NoError(t, os.MkdirAll(dir1, 0755)) + require.NoError(t, os.MkdirAll(dir2, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(dir1, "file.txt"), []byte("data"), 0644)) + require.NoError(t, os.Symlink("../dir1/file.txt", filepath.Join(dir2, "link.txt"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + f, err := sb.Open("link.txt", dir2, os.O_RDONLY, 0) + require.NoError(t, err) + defer f.Close() + + buf := make([]byte, 64) + n, _ := f.Read(buf) + assert.Equal(t, "data", string(buf[:n])) +} + +// TestCrossRootSymlinkIntermediateDir verifies that an intermediate directory +// symlink crossing roots is resolved correctly. +func TestCrossRootSymlinkIntermediateDir(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + subdir := filepath.Join(dir1, "sub") + require.NoError(t, os.MkdirAll(subdir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(subdir, "file.txt"), []byte("deep"), 0644)) + // dir2/link -> dir1/sub (directory symlink) + require.NoError(t, os.Symlink(subdir, filepath.Join(dir2, "link"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + f, err := sb.Open(filepath.Join("link", "file.txt"), dir2, os.O_RDONLY, 0) + require.NoError(t, err) + defer f.Close() + + buf := make([]byte, 64) + n, _ := f.Read(buf) + assert.Equal(t, "deep", string(buf[:n])) +} + +// TestCrossRootSymlinkOutsideAllRoots verifies that a symlink pointing +// outside all allowed roots is still blocked. +func TestCrossRootSymlinkOutsideAllRoots(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + outside := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("secret"), 0644)) + require.NoError(t, os.Symlink(filepath.Join(outside, "secret.txt"), filepath.Join(dir1, "escape.txt"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + _, err = sb.Open("escape.txt", dir1, os.O_RDONLY, 0) + assert.Error(t, err) +} + +// TestCrossRootSymlinkLoopBlocked verifies that circular symlinks between +// roots are detected and rejected after maxSymlinkHops. +func TestCrossRootSymlinkLoopBlocked(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + // dir1/a.txt -> dir2/b.txt -> dir1/a.txt (circular) + require.NoError(t, os.Symlink(filepath.Join(dir2, "b.txt"), filepath.Join(dir1, "a.txt"))) + require.NoError(t, os.Symlink(filepath.Join(dir1, "a.txt"), filepath.Join(dir2, "b.txt"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + _, err = sb.Open("a.txt", dir1, os.O_RDONLY, 0) + assert.Error(t, err, "circular cross-root symlinks should be rejected") + + _, err = sb.Stat("a.txt", dir1) + assert.Error(t, err, "circular cross-root symlinks should be rejected") +} + +// TestCrossRootSymlinkChainLimit verifies that a long chain of cross-root +// symlinks exceeding maxSymlinkHops is rejected. +func TestCrossRootSymlinkChainLimit(t *testing.T) { + dirs := make([]string, maxSymlinkHops+2) + for i := range dirs { + dirs[i] = t.TempDir() + } + // Last directory has the real file. + require.NoError(t, os.WriteFile(filepath.Join(dirs[len(dirs)-1], "file.txt"), []byte("end"), 0644)) + + // Each dir[i]/link.txt -> dir[i+1]/link.txt, except the penultimate + // which points to the real file. + for i := 0; i < len(dirs)-1; i++ { + target := filepath.Join(dirs[i+1], "link.txt") + if i == len(dirs)-2 { + target = filepath.Join(dirs[i+1], "file.txt") + } + require.NoError(t, os.Symlink(target, filepath.Join(dirs[i], "link.txt"))) + } + + sb, _, err := New(dirs) + require.NoError(t, err) + defer sb.Close() + + _, err = sb.Open("link.txt", dirs[0], os.O_RDONLY, 0) + assert.Error(t, err, "symlink chain exceeding maxSymlinkHops should be rejected") +} + +// TestCrossRootSymlinkSiblingDirs verifies that a symlink in one sibling +// directory pointing into another sibling directory can be read when both +// are in AllowedPaths. +func TestCrossRootSymlinkSiblingDirs(t *testing.T) { + parent := t.TempDir() + dir1 := filepath.Join(parent, "dir1") + dir2 := filepath.Join(parent, "dir2") + require.NoError(t, os.MkdirAll(dir1, 0755)) + require.NoError(t, os.MkdirAll(dir2, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(dir1, "file.txt"), []byte("abc"), 0644)) + // dir2/sym.txt -> ../dir1/file.txt + require.NoError(t, os.Symlink("../dir1/file.txt", filepath.Join(dir2, "sym.txt"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + f, err := sb.Open(filepath.Join(dir2, "sym.txt"), "/", os.O_RDONLY, 0) + require.NoError(t, err, "cross-root symlink between sibling dirs should be readable") + defer f.Close() + + buf := make([]byte, 64) + n, _ := f.Read(buf) + assert.Equal(t, "abc", string(buf[:n])) + + info, err := sb.Stat(filepath.Join(dir2, "sym.txt"), "/") + require.NoError(t, err) + assert.Equal(t, int64(3), info.Size()) + + assert.NoError(t, sb.Access(filepath.Join(dir2, "sym.txt"), "/", modeRead)) +} diff --git a/analysis/symbols_allowedpaths.go b/analysis/symbols_allowedpaths.go index c129e39d..ca509b25 100644 --- a/analysis/symbols_allowedpaths.go +++ b/analysis/symbols_allowedpaths.go @@ -27,6 +27,7 @@ var allowedpathsAllowedSymbols = []string{ "io.EOF", // 🟢 sentinel error value; pure constant. "io.ReadWriteCloser", // 🟢 combined interface type; no side effects. "io/fs.DirEntry", // 🟢 interface type for directory entries; no side effects. + "io/fs.ModeSymlink", // 🟢 file mode bit for symlinks; pure constant. "io/fs.ErrExist", // 🟢 sentinel error for "already exists"; pure constant. "io/fs.ErrNotExist", // 🟢 sentinel error for "does not exist"; pure constant. "io/fs.ErrPermission", // 🟢 sentinel error for permission denied; pure constant. @@ -45,6 +46,8 @@ var allowedpathsAllowedSymbols = []string{ "os.Root", // 🟠 sandboxed directory root type; core of the filesystem sandbox. "os.Stat", // 🟠 returns file info for a path; needed for sandbox path validation. "path/filepath.Abs", // 🟢 returns absolute path; pure path computation. + "path/filepath.Clean", // 🟢 normalizes a path; pure function, no I/O. + "path/filepath.Dir", // 🟢 returns directory portion of a path; pure function, no I/O. "path/filepath.IsAbs", // 🟢 checks if path is absolute; pure function, no I/O. "path/filepath.Join", // 🟢 joins path elements; pure function, no I/O. "path/filepath.Rel", // 🟢 returns relative path; pure path computation. @@ -54,6 +57,8 @@ var allowedpathsAllowedSymbols = []string{ "strings.Compare", // 🟢 compares two strings lexicographically; pure function, no I/O. "strings.EqualFold", // 🟢 case-insensitive string comparison; pure function, no I/O. "strings.HasPrefix", // 🟢 pure function for prefix matching; no I/O. + "strings.Join", // 🟢 joins string slices; pure function, no I/O. + "strings.Split", // 🟢 splits a string by separator; pure function, no I/O. "syscall.ByHandleFileInformation", // 🟢 Windows file identity structure; pure type for file metadata. "syscall.EISDIR", // 🟢 "is a directory" errno constant; pure constant. "syscall.Errno", // 🟢 system call error number type; pure type. diff --git a/tests/scenarios/shell/allowed_paths/cross_root_symlink_dir.yaml b/tests/scenarios/shell/allowed_paths/cross_root_symlink_dir.yaml new file mode 100644 index 00000000..bd4efaee --- /dev/null +++ b/tests/scenarios/shell/allowed_paths/cross_root_symlink_dir.yaml @@ -0,0 +1,18 @@ +description: Symlink to a directory in another allowed root can be traversed +# skip: allowed_paths sandbox restriction is an rshell-specific feature not present in bash +skip_assert_against_bash: true +setup: + files: + - path: dir1/sub/file.txt + content: "deep content\n" + - path: dir2/link + symlink: ../dir1/sub +input: + allowed_paths: ["dir1", "dir2"] + script: |+ + cat dir2/link/file.txt +expect: + stdout: |+ + deep content + stderr: |+ + exit_code: 0 diff --git a/tests/scenarios/shell/allowed_paths/cross_root_symlink_outside_blocked.yaml b/tests/scenarios/shell/allowed_paths/cross_root_symlink_outside_blocked.yaml new file mode 100644 index 00000000..95fe373c --- /dev/null +++ b/tests/scenarios/shell/allowed_paths/cross_root_symlink_outside_blocked.yaml @@ -0,0 +1,18 @@ +description: Symlink pointing outside all allowed roots is still blocked +# skip: allowed_paths sandbox restriction is an rshell-specific feature not present in bash +skip_assert_against_bash: true +setup: + files: + - path: outside/secret.txt + content: "secret\n" + - path: dir1/escape.txt + symlink: ../outside/secret.txt +input: + allowed_paths: ["dir1"] + script: |+ + cat dir1/escape.txt +expect: + stdout: |+ + stderr_contains: + - "path escapes from parent" + exit_code: 1 diff --git a/tests/scenarios/shell/allowed_paths/cross_root_symlink_read.yaml b/tests/scenarios/shell/allowed_paths/cross_root_symlink_read.yaml new file mode 100644 index 00000000..c6e137fc --- /dev/null +++ b/tests/scenarios/shell/allowed_paths/cross_root_symlink_read.yaml @@ -0,0 +1,18 @@ +description: Symlink in one allowed root pointing to a file in another allowed root can be read +# skip: allowed_paths sandbox restriction is an rshell-specific feature not present in bash +skip_assert_against_bash: true +setup: + files: + - path: dir1/file.txt + content: "cross-root content\n" + - path: dir2/link.txt + symlink: ../dir1/file.txt +input: + allowed_paths: ["dir1", "dir2"] + script: |+ + cat dir2/link.txt +expect: + stdout: |+ + cross-root content + stderr: |+ + exit_code: 0 From f5430697c389f1f5d83d74262de611f3ee6d29f7 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 12:03:24 -0400 Subject: [PATCH 2/9] refactor(allowedpaths): deduplicate symlink resolution and fix review findings - resolveFollowingSymlinks now delegates to resolveRootFollowingSymlinks instead of duplicating the component-walking logic - Remove unused loop variable (for i, comp -> for i := range) - Fix double negative in test assertion (assert.False x != 0 -> assert.Zero) Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/sandbox.go | 81 ++++++++----------------------- allowedpaths/sandbox_unix_test.go | 2 +- 2 files changed, 20 insertions(+), 63 deletions(-) diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index 17e91d39..6a12be99 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -125,56 +125,7 @@ func (s *Sandbox) resolveRoot(absPath string) (*root, string, bool) { return nil, "", false } -// resolveRootFollowingSymlinks is the cross-root symlink fallback for -// resolveRoot. -func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, bool) { - for hops := 0; hops < maxSymlinkHops; hops++ { - ar, rel, ok := s.resolveRoot(absPath) - if !ok { - return nil, "", false - } - - components := strings.Split(rel, string(filepath.Separator)) - symlinkFound := false - for i, comp := range components { - if comp == "." { - continue - } - partial := strings.Join(components[:i+1], string(filepath.Separator)) - info, err := ar.root.Lstat(partial) - if err != nil { - return nil, "", false - } - if info.Mode()&fs.ModeSymlink == 0 { - continue - } - target, err := ar.root.Readlink(partial) - if err != nil { - return nil, "", false - } - if !filepath.IsAbs(target) { - parentAbs := absPath - for j := len(components) - 1; j >= i; j-- { - parentAbs = filepath.Dir(parentAbs) - } - target = filepath.Join(parentAbs, target) - } - if i+1 < len(components) { - remaining := strings.Join(components[i+1:], string(filepath.Separator)) - target = filepath.Join(target, remaining) - } - absPath = filepath.Clean(target) - symlinkFound = true - break - } - if !symlinkFound { - return ar, rel, true - } - } - return nil, "", false -} - -// resolveFollowingSymlinks resolves absPath to a (root, relPath) pair, +// resolveRootFollowingSymlinks resolves absPath to a (root, relPath) pair, // following symlinks that cross between allowed roots. It walks the // relative path component by component; when a component is a symlink, // its target is resolved to an absolute path and matched against all @@ -182,9 +133,9 @@ func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, b // // This is only called as a fallback when the primary os.Root operation // fails, so there is no overhead on the happy path. -func (s *Sandbox) resolveFollowingSymlinks(absPath string) (*os.Root, string, bool) { - for hops := 0; hops < maxSymlinkHops; hops++ { - root, rel, ok := s.resolve(absPath) +func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, bool) { + for range maxSymlinkHops { + ar, rel, ok := s.resolveRoot(absPath) if !ok { return nil, "", false } @@ -192,13 +143,12 @@ func (s *Sandbox) resolveFollowingSymlinks(absPath string) (*os.Root, string, bo // Walk rel component by component looking for symlinks. components := strings.Split(rel, string(filepath.Separator)) symlinkFound := false - for i, comp := range components { - if comp == "." { + for i := range components { + if components[i] == "." { continue } - // Build the path up to and including this component. partial := strings.Join(components[:i+1], string(filepath.Separator)) - info, err := root.Lstat(partial) + info, err := ar.root.Lstat(partial) if err != nil { return nil, "", false } @@ -206,13 +156,12 @@ func (s *Sandbox) resolveFollowingSymlinks(absPath string) (*os.Root, string, bo continue } // Found a symlink — read its target. - target, err := root.Readlink(partial) + target, err := ar.root.Readlink(partial) if err != nil { return nil, "", false } // Resolve target to absolute path. if !filepath.IsAbs(target) { - // Relative to the directory containing the symlink. parentAbs := absPath for j := len(components) - 1; j >= i; j-- { parentAbs = filepath.Dir(parentAbs) @@ -229,14 +178,22 @@ func (s *Sandbox) resolveFollowingSymlinks(absPath string) (*os.Root, string, bo break } if !symlinkFound { - // No symlinks found — return the resolved root and path. - return root, rel, true + return ar, rel, true } - // Loop again with the new absPath (may chain through another root). } return nil, "", false // too many hops } +// resolveFollowingSymlinks is like resolveRootFollowingSymlinks but returns +// the *os.Root handle instead of the internal root entry. +func (s *Sandbox) resolveFollowingSymlinks(absPath string) (*os.Root, string, bool) { + ar, rel, ok := s.resolveRootFollowingSymlinks(absPath) + if !ok { + return nil, "", false + } + return ar.root, rel, true +} + // Access checks whether the resolved path is accessible with the given mode. // All operations go through os.Root to stay within the sandbox. // Mode: 0x04 = read, 0x02 = write, 0x01 = execute. diff --git a/allowedpaths/sandbox_unix_test.go b/allowedpaths/sandbox_unix_test.go index a6f8ac3b..63e9cfc7 100644 --- a/allowedpaths/sandbox_unix_test.go +++ b/allowedpaths/sandbox_unix_test.go @@ -452,7 +452,7 @@ func TestCrossRootSymlinkStat(t *testing.T) { info, err := sb.Stat("link.txt", dir2) require.NoError(t, err) assert.Equal(t, int64(5), info.Size()) - assert.False(t, info.Mode()&os.ModeSymlink != 0, "Stat should follow the symlink") + assert.Zero(t, info.Mode()&os.ModeSymlink, "Stat should follow the symlink") } // TestCrossRootSymlinkAccess verifies that Access works through a cross-root symlink. From feab3b26cb31bae0ce89531e9911fa65d8afae56 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 12:08:50 -0400 Subject: [PATCH 3/9] fix(allowedpaths): return real errors for cross-root symlink targets When Lstat fails on a component during symlink resolution (e.g. the target file doesn't exist), return the current root and rel instead of false. This lets the caller retry the operation and get the real error (ENOENT) instead of the original "path escapes from parent". Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/sandbox.go | 5 ++++- allowedpaths/sandbox_unix_test.go | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index 6a12be99..cfa65e38 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -150,7 +150,10 @@ func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, b partial := strings.Join(components[:i+1], string(filepath.Separator)) info, err := ar.root.Lstat(partial) if err != nil { - return nil, "", false + // Component doesn't exist or isn't accessible. It can't + // be a symlink we need to resolve, so return what we have + // and let the caller get the real error. + return ar, rel, true } if info.Mode()&fs.ModeSymlink == 0 { continue diff --git a/allowedpaths/sandbox_unix_test.go b/allowedpaths/sandbox_unix_test.go index 63e9cfc7..dd78f631 100644 --- a/allowedpaths/sandbox_unix_test.go +++ b/allowedpaths/sandbox_unix_test.go @@ -534,6 +534,23 @@ func TestCrossRootSymlinkOutsideAllRoots(t *testing.T) { assert.Error(t, err) } +// TestCrossRootSymlinkMissingTarget verifies that a cross-root symlink +// pointing to a non-existent file returns ENOENT, not the escape error. +func TestCrossRootSymlinkMissingTarget(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + // link points into dir1, but the target file doesn't exist. + require.NoError(t, os.Symlink(filepath.Join(dir1, "missing.txt"), filepath.Join(dir2, "link.txt"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + _, err = sb.Open("link.txt", dir2, os.O_RDONLY, 0) + require.Error(t, err) + assert.Contains(t, err.Error(), "no such file or directory", "should report file not found, not path escape") +} + // TestCrossRootSymlinkLoopBlocked verifies that circular symlinks between // roots are detected and rejected after maxSymlinkHops. func TestCrossRootSymlinkLoopBlocked(t *testing.T) { From bc717470d2fd0f22c41a74be76b6b00763af8057 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 12:20:16 -0400 Subject: [PATCH 4/9] fix(allowedpaths): fix off-by-one in symlink hop limit The loop needs N+1 iterations for N symlink hops: N iterations to resolve each symlink, plus 1 final iteration to confirm the resolved path has no more symlinks and return success. Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/sandbox.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index cfa65e38..6b3487a4 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -134,7 +134,9 @@ func (s *Sandbox) resolveRoot(absPath string) (*root, string, bool) { // This is only called as a fallback when the primary os.Root operation // fails, so there is no overhead on the happy path. func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, bool) { - for range maxSymlinkHops { + // N+1 iterations: up to N to resolve symlinks, 1 more to confirm + // the final path has no more symlinks and return success. + for range maxSymlinkHops + 1 { ar, rel, ok := s.resolveRoot(absPath) if !ok { return nil, "", false From 7f891cfcee2524b7948df0b45e1f70459e904835 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 13:57:23 -0400 Subject: [PATCH 5/9] fix(allowedpaths): preserve terminal symlink in Lstat and Readlink fallback resolveRootFollowingSymlinks now accepts a preserveLast flag. When true, the final path component is not resolved even if it is a symlink. Lstat and Readlink pass true so they operate on the symlink itself rather than its target. All other callers pass false for full resolution. Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/sandbox.go | 32 ++++++++++++++--------- allowedpaths/sandbox_unix_test.go | 43 +++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 12 deletions(-) diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index 6b3487a4..e17eb387 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -131,9 +131,12 @@ func (s *Sandbox) resolveRoot(absPath string) (*root, string, bool) { // its target is resolved to an absolute path and matched against all // roots, then resolution continues with the remaining components. // +// When preserveLast is true, the final path component is not resolved +// even if it is a symlink. This preserves lstat/readlink semantics. +// // This is only called as a fallback when the primary os.Root operation // fails, so there is no overhead on the happy path. -func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, bool) { +func (s *Sandbox) resolveRootFollowingSymlinks(absPath string, preserveLast bool) (*root, string, bool) { // N+1 iterations: up to N to resolve symlinks, 1 more to confirm // the final path has no more symlinks and return success. for range maxSymlinkHops + 1 { @@ -149,6 +152,11 @@ func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, b if components[i] == "." { continue } + // When preserveLast is set, skip the final component so that + // Lstat and Readlink operate on the symlink itself. + if preserveLast && i == len(components)-1 { + break + } partial := strings.Join(components[:i+1], string(filepath.Separator)) info, err := ar.root.Lstat(partial) if err != nil { @@ -191,8 +199,8 @@ func (s *Sandbox) resolveRootFollowingSymlinks(absPath string) (*root, string, b // resolveFollowingSymlinks is like resolveRootFollowingSymlinks but returns // the *os.Root handle instead of the internal root entry. -func (s *Sandbox) resolveFollowingSymlinks(absPath string) (*os.Root, string, bool) { - ar, rel, ok := s.resolveRootFollowingSymlinks(absPath) +func (s *Sandbox) resolveFollowingSymlinks(absPath string, preserveLast bool) (*os.Root, string, bool) { + ar, rel, ok := s.resolveRootFollowingSymlinks(absPath, preserveLast) if !ok { return nil, "", false } @@ -245,7 +253,7 @@ func (s *Sandbox) Access(path string, cwd string, mode uint32) error { return &os.PathError{Op: "access", Path: path, Err: os.ErrPermission} } // Symlink escapes this root — resolve across all roots. - resolved, resolvedRel, ok := s.resolveRootFollowingSymlinks(absPath) + resolved, resolvedRel, ok := s.resolveRootFollowingSymlinks(absPath, false) if !ok { return &os.PathError{Op: "access", Path: path, Err: os.ErrPermission} } @@ -301,7 +309,7 @@ func (s *Sandbox) Open(path string, cwd string, flag int, perm os.FileMode) (io. return nil, PortablePathError(err) } // Symlink escapes this root — resolve across all roots. - r, rel, ok := s.resolveFollowingSymlinks(absPath) + r, rel, ok := s.resolveFollowingSymlinks(absPath, false) if !ok { return nil, PortablePathError(err) } @@ -341,7 +349,7 @@ func (s *Sandbox) readDirN(path string, cwd string, maxEntries int) ([]fs.DirEnt f, err := root.Open(relPath) if err != nil && isPathEscapeError(err) { - if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok { + if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { f, err = r.Open(rel) } } @@ -387,7 +395,7 @@ func (s *Sandbox) OpenDir(path string, cwd string) (fs.ReadDirFile, error) { f, err := root.Open(relPath) if err != nil && isPathEscapeError(err) { - if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok { + if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { f, err = r.Open(rel) } } @@ -410,7 +418,7 @@ func (s *Sandbox) IsDirEmpty(path string, cwd string) (bool, error) { f, err := root.Open(relPath) if err != nil && isPathEscapeError(err) { - if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok { + if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { f, err = r.Open(rel) } } @@ -442,7 +450,7 @@ func (s *Sandbox) ReadDirLimited(path string, cwd string, offset, maxRead int) ( } f, err := root.Open(relPath) if err != nil && isPathEscapeError(err) { - if r, rel, ok := s.resolveFollowingSymlinks(absPath); ok { + if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { f, err = r.Open(rel) } } @@ -547,7 +555,7 @@ func (s *Sandbox) Stat(path string, cwd string) (fs.FileInfo, error) { if !isPathEscapeError(err) { return nil, PortablePathError(err) } - r, rel, ok := s.resolveFollowingSymlinks(absPath) + r, rel, ok := s.resolveFollowingSymlinks(absPath, false) if !ok { return nil, PortablePathError(err) } @@ -581,7 +589,7 @@ func (s *Sandbox) Lstat(path string, cwd string) (fs.FileInfo, error) { if !isPathEscapeError(err) { return nil, PortablePathError(err) } - r, rel, ok := s.resolveFollowingSymlinks(absPath) + r, rel, ok := s.resolveFollowingSymlinks(absPath, true) if !ok { return nil, PortablePathError(err) } @@ -608,7 +616,7 @@ func (s *Sandbox) Readlink(path string, cwd string) (string, error) { if !isPathEscapeError(err) { return "", PortablePathError(err) } - r, rel, ok := s.resolveFollowingSymlinks(absPath) + r, rel, ok := s.resolveFollowingSymlinks(absPath, true) if !ok { return "", PortablePathError(err) } diff --git a/allowedpaths/sandbox_unix_test.go b/allowedpaths/sandbox_unix_test.go index dd78f631..1f869b29 100644 --- a/allowedpaths/sandbox_unix_test.go +++ b/allowedpaths/sandbox_unix_test.go @@ -551,6 +551,49 @@ func TestCrossRootSymlinkMissingTarget(t *testing.T) { assert.Contains(t, err.Error(), "no such file or directory", "should report file not found, not path escape") } +// TestCrossRootSymlinkLstatPreservesTerminal verifies that Lstat through a +// cross-root intermediate symlink preserves the terminal symlink. +func TestCrossRootSymlinkLstatPreservesTerminal(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + // dir1/sub/leaf.txt -> some_target (a symlink) + subdir := filepath.Join(dir1, "sub") + require.NoError(t, os.MkdirAll(subdir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(subdir, "real.txt"), []byte("data"), 0644)) + require.NoError(t, os.Symlink("real.txt", filepath.Join(subdir, "leaf.txt"))) + // dir2/bridge -> dir1/sub (cross-root directory symlink) + require.NoError(t, os.Symlink(subdir, filepath.Join(dir2, "bridge"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + // Lstat should report leaf.txt as a symlink, not the target. + info, err := sb.Lstat(filepath.Join("bridge", "leaf.txt"), dir2) + require.NoError(t, err) + assert.NotZero(t, info.Mode()&os.ModeSymlink, "Lstat should report symlink mode for terminal component") +} + +// TestCrossRootSymlinkReadlinkPreservesTerminal verifies that Readlink +// through a cross-root intermediate symlink reads the terminal symlink. +func TestCrossRootSymlinkReadlinkPreservesTerminal(t *testing.T) { + dir1 := t.TempDir() + dir2 := t.TempDir() + subdir := filepath.Join(dir1, "sub") + require.NoError(t, os.MkdirAll(subdir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(subdir, "real.txt"), []byte("data"), 0644)) + require.NoError(t, os.Symlink("real.txt", filepath.Join(subdir, "leaf.txt"))) + require.NoError(t, os.Symlink(subdir, filepath.Join(dir2, "bridge"))) + + sb, _, err := New([]string{dir1, dir2}) + require.NoError(t, err) + defer sb.Close() + + target, err := sb.Readlink(filepath.Join("bridge", "leaf.txt"), dir2) + require.NoError(t, err) + assert.Equal(t, "real.txt", target) +} + // TestCrossRootSymlinkLoopBlocked verifies that circular symlinks between // roots are detected and rejected after maxSymlinkHops. func TestCrossRootSymlinkLoopBlocked(t *testing.T) { From b7c9fbf0fffe2d15839a1a3fc70019996db9156b Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 14:16:36 -0400 Subject: [PATCH 6/9] fix(allowedpaths): clean absPath before computing symlink parent filepath.Dir on a trailing-slash path (e.g. "/dir/link/") strips only the slash, producing the wrong parent for relative symlink resolution. Clean absPath at entry to resolveRootFollowingSymlinks. Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/sandbox.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index e17eb387..fc1995a6 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -137,6 +137,9 @@ func (s *Sandbox) resolveRoot(absPath string) (*root, string, bool) { // This is only called as a fallback when the primary os.Root operation // fails, so there is no overhead on the happy path. func (s *Sandbox) resolveRootFollowingSymlinks(absPath string, preserveLast bool) (*root, string, bool) { + // Clean trailing slashes so filepath.Dir computes the correct parent + // when resolving relative symlink targets. + absPath = filepath.Clean(absPath) // N+1 iterations: up to N to resolve symlinks, 1 more to confirm // the final path has no more symlinks and return success. for range maxSymlinkHops + 1 { From aa61471efd27041b981330c30f825774e1344f61 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 15:32:55 -0400 Subject: [PATCH 7/9] Address review: regression test, extract helper, reply to nits - Add TestIsPathEscapeError regression test to catch Go stdlib changes - Extract openWithSymlinkFallback helper to deduplicate the repeated escape-check-and-retry pattern in readDirN, OpenDir, IsDirEmpty, and ReadDirLimited Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/sandbox.go | 40 +++++++++++++------------------ allowedpaths/sandbox_unix_test.go | 23 ++++++++++++++++++ analysis/symbols_allowedpaths.go | 1 + 3 files changed, 40 insertions(+), 24 deletions(-) diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index fc1995a6..6d607a2b 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -210,6 +210,18 @@ func (s *Sandbox) resolveFollowingSymlinks(absPath string, preserveLast bool) (* return ar.root, rel, true } +// openWithSymlinkFallback opens relPath through root, falling back to +// cross-root symlink resolution if the open fails with a path escape. +func (s *Sandbox) openWithSymlinkFallback(root *os.Root, relPath, absPath string) (*os.File, error) { + f, err := root.Open(relPath) + if err != nil && isPathEscapeError(err) { + if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { + f, err = r.Open(rel) + } + } + return f, err +} + // Access checks whether the resolved path is accessible with the given mode. // All operations go through os.Root to stay within the sandbox. // Mode: 0x04 = read, 0x02 = write, 0x01 = execute. @@ -350,12 +362,7 @@ func (s *Sandbox) readDirN(path string, cwd string, maxEntries int) ([]fs.DirEnt return nil, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} } - f, err := root.Open(relPath) - if err != nil && isPathEscapeError(err) { - if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { - f, err = r.Open(rel) - } - } + f, err := s.openWithSymlinkFallback(root, relPath, absPath) if err != nil { return nil, PortablePathError(err) } @@ -396,12 +403,7 @@ func (s *Sandbox) OpenDir(path string, cwd string) (fs.ReadDirFile, error) { return nil, &os.PathError{Op: "opendir", Path: path, Err: os.ErrPermission} } - f, err := root.Open(relPath) - if err != nil && isPathEscapeError(err) { - if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { - f, err = r.Open(rel) - } - } + f, err := s.openWithSymlinkFallback(root, relPath, absPath) if err != nil { return nil, PortablePathError(err) } @@ -419,12 +421,7 @@ func (s *Sandbox) IsDirEmpty(path string, cwd string) (bool, error) { return false, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} } - f, err := root.Open(relPath) - if err != nil && isPathEscapeError(err) { - if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { - f, err = r.Open(rel) - } - } + f, err := s.openWithSymlinkFallback(root, relPath, absPath) if err != nil { return false, PortablePathError(err) } @@ -451,12 +448,7 @@ func (s *Sandbox) ReadDirLimited(path string, cwd string, offset, maxRead int) ( if !ok { return nil, false, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} } - f, err := root.Open(relPath) - if err != nil && isPathEscapeError(err) { - if r, rel, ok := s.resolveFollowingSymlinks(absPath, false); ok { - f, err = r.Open(rel) - } - } + f, err := s.openWithSymlinkFallback(root, relPath, absPath) if err != nil { return nil, false, PortablePathError(err) } diff --git a/allowedpaths/sandbox_unix_test.go b/allowedpaths/sandbox_unix_test.go index 1f869b29..f9568edc 100644 --- a/allowedpaths/sandbox_unix_test.go +++ b/allowedpaths/sandbox_unix_test.go @@ -187,6 +187,29 @@ func TestAccessSymlinkWithinSandbox(t *testing.T) { assert.NoError(t, sb.Access("link.txt", dir, 0x04)) } +// TestIsPathEscapeError is a regression test for isPathEscapeError. +// If Go changes the "path escapes from parent" error string in a future +// version, this test will fail and alert us to update the detection. +func TestIsPathEscapeError(t *testing.T) { + dir := t.TempDir() + outside := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(outside, "file.txt"), []byte("data"), 0644)) + require.NoError(t, os.Symlink(filepath.Join(outside, "file.txt"), filepath.Join(dir, "escape.txt"))) + + r, err := os.OpenRoot(dir) + require.NoError(t, err) + defer r.Close() + + _, err = r.OpenFile("escape.txt", os.O_RDONLY, 0) + require.Error(t, err) + assert.True(t, isPathEscapeError(err), "expected path escape error, got: %v", err) + + // Non-escape errors should not match. + _, err = r.OpenFile("nonexistent.txt", os.O_RDONLY, 0) + require.Error(t, err) + assert.False(t, isPathEscapeError(err), "ENOENT should not be a path escape error") +} + // TestAccessSymlinkEscapeBlocked verifies that Access blocks symlinks // that resolve outside the sandbox. func TestAccessSymlinkEscapeBlocked(t *testing.T) { diff --git a/analysis/symbols_allowedpaths.go b/analysis/symbols_allowedpaths.go index ca509b25..73cc7c7b 100644 --- a/analysis/symbols_allowedpaths.go +++ b/analysis/symbols_allowedpaths.go @@ -36,6 +36,7 @@ var allowedpathsAllowedSymbols = []string{ "io/fs.ReadDirFile", // 🟢 read-only directory handle interface; no write capability. "os.DevNull", // 🟢 platform null device path constant; pure constant. "os.ErrPermission", // 🟢 sentinel error for permission denied; pure constant. + "os.File", // 🟠 file handle returned by os.Root.Open; needed for cross-root symlink fallback. "os.FileMode", // 🟢 file permission bits type; pure type. "os.Getgid", // 🟠 returns the numeric group id of the caller; read-only syscall. "os.Getgroups", // 🟠 returns supplementary group ids; read-only syscall. From 33f507ae1b9c79130b5d6ff144804639721db328 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 15:40:24 -0400 Subject: [PATCH 8/9] refactor(allowedpaths): consolidate resolve and resolveRoot into one function Remove the duplicate resolve that returned *os.Root. The single resolve now returns *root, and callers access .root where they need the os.Root handle. Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/sandbox.go | 59 ++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 39 deletions(-) diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index 6d607a2b..705dbe85 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -87,28 +87,9 @@ func isPathEscapeError(err error) bool { // circular symlinks. const maxSymlinkHops = 10 -// resolve returns the matching os.Root and the path relative to it for the -// given absolute path. It returns false if no root matches. -func (s *Sandbox) resolve(absPath string) (*os.Root, string, bool) { - if s == nil { - return nil, "", false - } - for _, ar := range s.roots { - rel, err := filepath.Rel(ar.absPath, absPath) - if err != nil { - continue - } - if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - continue - } - return ar.root, rel, true - } - return nil, "", false -} - -// resolveRoot is like resolve but returns the internal root entry instead -// of the os.Root handle, so callers can access accessCheck and absPath. -func (s *Sandbox) resolveRoot(absPath string) (*root, string, bool) { +// resolve returns the matching root entry and the path relative to it for +// the given absolute path. It returns false if no root matches. +func (s *Sandbox) resolve(absPath string) (*root, string, bool) { if s == nil { return nil, "", false } @@ -143,7 +124,7 @@ func (s *Sandbox) resolveRootFollowingSymlinks(absPath string, preserveLast bool // N+1 iterations: up to N to resolve symlinks, 1 more to confirm // the final path has no more symlinks and return success. for range maxSymlinkHops + 1 { - ar, rel, ok := s.resolveRoot(absPath) + ar, rel, ok := s.resolve(absPath) if !ok { return nil, "", false } @@ -311,12 +292,12 @@ func (s *Sandbox) Open(path string, cwd string, flag int, perm os.FileMode) (io. absPath := toAbs(path, cwd) - root, relPath, ok := s.resolve(absPath) + ar, relPath, ok := s.resolve(absPath) if !ok { return nil, &os.PathError{Op: "open", Path: path, Err: os.ErrPermission} } - f, err := root.OpenFile(relPath, flag, perm) + f, err := ar.root.OpenFile(relPath, flag, perm) if err == nil { return f, nil } @@ -357,12 +338,12 @@ func (s *Sandbox) ReadDirForGlob(path string, cwd string) ([]fs.DirEntry, error) func (s *Sandbox) readDirN(path string, cwd string, maxEntries int) ([]fs.DirEntry, error) { absPath := toAbs(path, cwd) - root, relPath, ok := s.resolve(absPath) + ar, relPath, ok := s.resolve(absPath) if !ok { return nil, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} } - f, err := s.openWithSymlinkFallback(root, relPath, absPath) + f, err := s.openWithSymlinkFallback(ar.root, relPath, absPath) if err != nil { return nil, PortablePathError(err) } @@ -398,12 +379,12 @@ func (s *Sandbox) readDirN(path string, cwd string, maxEntries int) ([]fs.DirEnt func (s *Sandbox) OpenDir(path string, cwd string) (fs.ReadDirFile, error) { absPath := toAbs(path, cwd) - root, relPath, ok := s.resolve(absPath) + ar, relPath, ok := s.resolve(absPath) if !ok { return nil, &os.PathError{Op: "opendir", Path: path, Err: os.ErrPermission} } - f, err := s.openWithSymlinkFallback(root, relPath, absPath) + f, err := s.openWithSymlinkFallback(ar.root, relPath, absPath) if err != nil { return nil, PortablePathError(err) } @@ -416,12 +397,12 @@ func (s *Sandbox) OpenDir(path string, cwd string) (fs.ReadDirFile, error) { func (s *Sandbox) IsDirEmpty(path string, cwd string) (bool, error) { absPath := toAbs(path, cwd) - root, relPath, ok := s.resolve(absPath) + ar, relPath, ok := s.resolve(absPath) if !ok { return false, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} } - f, err := s.openWithSymlinkFallback(root, relPath, absPath) + f, err := s.openWithSymlinkFallback(ar.root, relPath, absPath) if err != nil { return false, PortablePathError(err) } @@ -444,11 +425,11 @@ func (s *Sandbox) IsDirEmpty(path string, cwd string) (bool, error) { // O(n) memory regardless of offset value, where n = min(maxRead, entries). func (s *Sandbox) ReadDirLimited(path string, cwd string, offset, maxRead int) ([]fs.DirEntry, bool, error) { absPath := toAbs(path, cwd) - root, relPath, ok := s.resolve(absPath) + ar, relPath, ok := s.resolve(absPath) if !ok { return nil, false, &os.PathError{Op: "readdir", Path: path, Err: os.ErrPermission} } - f, err := s.openWithSymlinkFallback(root, relPath, absPath) + f, err := s.openWithSymlinkFallback(ar.root, relPath, absPath) if err != nil { return nil, false, PortablePathError(err) } @@ -538,12 +519,12 @@ func (s *Sandbox) Stat(path string, cwd string) (fs.FileInfo, error) { absPath := toAbs(path, cwd) - root, relPath, ok := s.resolve(absPath) + ar, relPath, ok := s.resolve(absPath) if !ok { return nil, &os.PathError{Op: "stat", Path: path, Err: os.ErrPermission} } - info, err := root.Stat(relPath) + info, err := ar.root.Stat(relPath) if err == nil { return info, nil } @@ -572,12 +553,12 @@ func (s *Sandbox) Lstat(path string, cwd string) (fs.FileInfo, error) { absPath := toAbs(path, cwd) - root, relPath, ok := s.resolve(absPath) + ar, relPath, ok := s.resolve(absPath) if !ok { return nil, &os.PathError{Op: "lstat", Path: path, Err: os.ErrPermission} } - info, err := root.Lstat(relPath) + info, err := ar.root.Lstat(relPath) if err == nil { return info, nil } @@ -599,12 +580,12 @@ func (s *Sandbox) Lstat(path string, cwd string) (fs.FileInfo, error) { func (s *Sandbox) Readlink(path string, cwd string) (string, error) { absPath := toAbs(path, cwd) - root, relPath, ok := s.resolve(absPath) + ar, relPath, ok := s.resolve(absPath) if !ok { return "", &os.PathError{Op: "readlink", Path: path, Err: os.ErrPermission} } - target, err := root.Readlink(relPath) + target, err := ar.root.Readlink(relPath) if err == nil { return target, nil } From 1ee336aebd1281270650bc086882afaa8d5d79e7 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Thu, 2 Apr 2026 15:44:57 -0400 Subject: [PATCH 9/9] fix(allowedpaths): fix Windows build after resolve return type change FileIdentity in portable_windows.go used the old resolve() that returned *os.Root. Updated to use .root on the returned *root entry. Co-Authored-By: Claude Opus 4.6 (1M context) --- allowedpaths/portable_windows.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/allowedpaths/portable_windows.go b/allowedpaths/portable_windows.go index 6f64b06d..888582fd 100644 --- a/allowedpaths/portable_windows.go +++ b/allowedpaths/portable_windows.go @@ -26,11 +26,11 @@ func IsErrIsDirectory(err error) bool { // GetFileInformationByHandle (volume serial + file index). // The path and sandbox are needed to open the file through the sandbox. func FileIdentity(absPath string, _ fs.FileInfo, sandbox *Sandbox) (uint64, uint64, bool) { - root, relPath, ok := sandbox.resolve(absPath) + ar, relPath, ok := sandbox.resolve(absPath) if !ok { return 0, 0, false } - f, err := root.OpenFile(relPath, os.O_RDONLY, 0) + f, err := ar.root.OpenFile(relPath, os.O_RDONLY, 0) if err != nil { return 0, 0, false }