diff --git a/allowedpaths/sandbox.go b/allowedpaths/sandbox.go index 904716ee..f83c2b8e 100644 --- a/allowedpaths/sandbox.go +++ b/allowedpaths/sandbox.go @@ -8,6 +8,7 @@ package allowedpaths import ( + "bytes" "errors" "fmt" "io" @@ -36,33 +37,32 @@ type Sandbox struct { roots []root } -// New validates paths and eagerly opens os.Root handles so the -// allowed directories are pinned before the caller can modify them between -// construction and the first run. -func New(paths []string) (*Sandbox, error) { - roots := make([]root, len(paths)) - for i, p := range paths { +// New creates a sandbox from an allowlist of directory paths. Paths that do +// not exist or cannot be opened are silently skipped — the sandbox operates +// with whatever paths are available at construction time. +// +// Diagnostic messages about skipped paths are collected into warnings. The +// caller is responsible for writing them to the appropriate output stream. +func New(paths []string) (sb *Sandbox, warnings []byte, err error) { + var buf bytes.Buffer + roots := make([]root, 0, len(paths)) + for _, p := range paths { abs, err := filepath.Abs(p) if err != nil { - return nil, fmt.Errorf("AllowedPaths: cannot resolve %q: %w", p, err) + fmt.Fprintf(&buf, "AllowedPaths: skipping %q: %v\n", p, err) + continue } r, err := os.OpenRoot(abs) if err != nil { - for _, prev := range roots[:i] { - if prev.root != nil { - prev.root.Close() - } - } - - info, statErr := os.Stat(abs) - if statErr == nil && !info.IsDir() { - return nil, fmt.Errorf("AllowedPaths: %q is not a directory", abs) - } - return nil, fmt.Errorf("AllowedPaths: cannot open root %q: %w", abs, err) + // AllowedPaths is a suggestion, not a requirement. If we can't + // open a path (missing, not a directory, no permission, etc.), + // skip it and work with whatever paths are available. + fmt.Fprintf(&buf, "AllowedPaths: skipping %q: %v\n", abs, err) + continue } - roots[i] = root{absPath: abs, root: r} + roots = append(roots, root{absPath: abs, root: r}) } - return &Sandbox{roots: roots}, nil + return &Sandbox{roots: roots}, buf.Bytes(), nil } // resolve returns the matching os.Root and the path relative to it for the diff --git a/allowedpaths/sandbox_test.go b/allowedpaths/sandbox_test.go index e36ccc14..5e52340c 100644 --- a/allowedpaths/sandbox_test.go +++ b/allowedpaths/sandbox_test.go @@ -42,7 +42,7 @@ func TestSandboxOpenRejectsWriteFlags(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "test.txt"), []byte("data"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -74,7 +74,7 @@ func TestReadDirLimited(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(dir, fmt.Sprintf("f%02d", i)), nil, 0644)) } - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -193,7 +193,7 @@ func TestReadDirNCapExceeded(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(dir, fmt.Sprintf("f%02d", i)), nil, 0644)) } - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -306,3 +306,57 @@ func TestCollectDirEntries(t *testing.T) { assert.Equal(t, "cherry", entries[2].Name()) }) } + +func TestNewSkipsNonexistentPaths(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "test.txt"), []byte("data"), 0644)) + + sb, _, err := New([]string{"/nonexistent/path", dir}) + require.NoError(t, err, "nonexistent paths should be skipped") + defer sb.Close() + + // The existing directory should still be accessible. + f, err := sb.Open(filepath.Join(dir, "test.txt"), dir, os.O_RDONLY, 0) + require.NoError(t, err) + f.Close() +} + +func TestNewAllPathsNonexistent(t *testing.T) { + sb, _, err := New([]string{"/does/not/exist", "/also/missing"}) + require.NoError(t, err, "all-nonexistent paths should succeed with empty sandbox") + defer sb.Close() + + // Sandbox should block all access. + _, err = sb.Stat("/tmp", "/tmp") + assert.ErrorIs(t, err, os.ErrPermission) +} + +func TestNewEmptyPaths(t *testing.T) { + sb, _, err := New([]string{}) + require.NoError(t, err, "empty path list should succeed") + defer sb.Close() + + // Sandbox should block all access. + _, err = sb.Stat("/tmp", "/tmp") + assert.ErrorIs(t, err, os.ErrPermission) +} + +func TestNewMixedExistingAndNonexistent(t *testing.T) { + dirA := t.TempDir() + dirB := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dirA, "a.txt"), []byte("aaa"), 0644)) + require.NoError(t, os.WriteFile(filepath.Join(dirB, "b.txt"), []byte("bbb"), 0644)) + + sb, _, err := New([]string{dirA, "/nonexistent", dirB}) + require.NoError(t, err, "nonexistent path between valid dirs should be skipped") + defer sb.Close() + + // Both existing directories should be accessible. + f, err := sb.Open(filepath.Join(dirA, "a.txt"), dirA, os.O_RDONLY, 0) + require.NoError(t, err, "first existing dir should work") + f.Close() + + f, err = sb.Open(filepath.Join(dirB, "b.txt"), dirB, os.O_RDONLY, 0) + require.NoError(t, err, "second existing dir should work") + f.Close() +} diff --git a/allowedpaths/sandbox_unix_test.go b/allowedpaths/sandbox_unix_test.go index 2d45e74c..fdd1a6b3 100644 --- a/allowedpaths/sandbox_unix_test.go +++ b/allowedpaths/sandbox_unix_test.go @@ -26,7 +26,7 @@ func TestAccessFIFODoesNotBlock(t *testing.T) { fifoPath := filepath.Join(dir, "pipe") require.NoError(t, syscall.Mkfifo(fifoPath, 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -54,7 +54,7 @@ func TestAccessReadPermissionDenied(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "noread.txt"), []byte("secret"), 0200)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -72,7 +72,7 @@ func TestAccessWriteDenied(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "readonly.txt"), []byte("data"), 0444)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -90,7 +90,7 @@ func TestAccessExecDenied(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "noexec.txt"), []byte("data"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -103,7 +103,7 @@ func TestAccessReadAllowed(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "readable.txt"), []byte("data"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -115,7 +115,7 @@ func TestAccessWriteAllowed(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "writable.txt"), []byte("data"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -127,7 +127,7 @@ func TestAccessExecAllowed(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "script.sh"), []byte("#!/bin/sh"), 0755)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -138,7 +138,7 @@ func TestAccessExecAllowed(t *testing.T) { func TestAccessNonexistent(t *testing.T) { dir := t.TempDir() - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -153,7 +153,7 @@ func TestAccessOutsideSandbox(t *testing.T) { outside := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("secret"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -166,7 +166,7 @@ func TestAccessDirectory(t *testing.T) { dir := t.TempDir() require.NoError(t, os.Mkdir(filepath.Join(dir, "subdir"), 0755)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -180,7 +180,7 @@ func TestAccessSymlinkWithinSandbox(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(dir, "target.txt"), []byte("data"), 0644)) require.NoError(t, os.Symlink("target.txt", filepath.Join(dir, "link.txt"))) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -195,7 +195,7 @@ func TestAccessSymlinkEscapeBlocked(t *testing.T) { 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(dir, "escape.txt"))) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -213,7 +213,7 @@ func TestAccessCombinedModes(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "rx.sh"), []byte("#!/bin/sh"), 0555)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -236,7 +236,7 @@ func TestAccessReadRegularFileOpenFile(t *testing.T) { } dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "writeonly.txt"), []byte("data"), 0200)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() assert.ErrorIs(t, sb.Access("writeonly.txt", dir, 0x04), os.ErrPermission) @@ -247,7 +247,7 @@ func TestAccessReadRegularFileOpenFile(t *testing.T) { func TestAccessReadRegularFileAllowed(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "readable.txt"), []byte("data"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() assert.NoError(t, sb.Access("readable.txt", dir, 0x04)) @@ -259,7 +259,7 @@ func TestAccessReadRegularFileAllowed(t *testing.T) { func TestAccessFIFOReadFallsBackToModeBits(t *testing.T) { dir := t.TempDir() require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "pipe"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -281,7 +281,7 @@ func TestAccessFIFOReadDeniedModeBits(t *testing.T) { } dir := t.TempDir() require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "pipe"), 0200)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -300,7 +300,7 @@ func TestAccessFIFOReadDeniedModeBits(t *testing.T) { func TestAccessDirectoryReadUsesModeBits(t *testing.T) { dir := t.TempDir() require.NoError(t, os.Mkdir(filepath.Join(dir, "subdir"), 0755)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() assert.NoError(t, sb.Access("subdir", dir, 0x04)) @@ -314,7 +314,7 @@ func TestAccessDirectoryReadDenied(t *testing.T) { } dir := t.TempDir() require.NoError(t, os.Mkdir(filepath.Join(dir, "noread"), 0300)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() assert.ErrorIs(t, sb.Access("noread", dir, 0x04), os.ErrPermission) @@ -329,7 +329,7 @@ func TestAccessReadWriteCombined(t *testing.T) { dir := t.TempDir() // 0444 = readable but not writable require.NoError(t, os.WriteFile(filepath.Join(dir, "readonly.txt"), []byte("data"), 0444)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() // Read-only succeeds @@ -347,7 +347,7 @@ func TestAccessFdRelativeSymlink(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "target.txt"), []byte("data"), 0644)) require.NoError(t, os.Symlink("target.txt", filepath.Join(dir, "link.txt"))) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() assert.NoError(t, sb.Access("link.txt", dir, 0x04)) @@ -360,7 +360,7 @@ func TestAccessFdRelativeEscapeBlocked(t *testing.T) { 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(dir, "escape.txt"))) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() assert.Error(t, sb.Access("escape.txt", dir, 0x04)) @@ -381,7 +381,7 @@ func TestAccessSocketFallsBackToStat(t *testing.T) { defer syscall.Close(fd) require.NoError(t, syscall.Bind(fd, &syscall.SockaddrUnix{Name: sockPath})) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -397,7 +397,7 @@ func TestAccessFIFONonBlocking(t *testing.T) { dir := t.TempDir() require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "fifo"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() diff --git a/allowedpaths/sandbox_windows_test.go b/allowedpaths/sandbox_windows_test.go index 1b221b0b..317281b9 100644 --- a/allowedpaths/sandbox_windows_test.go +++ b/allowedpaths/sandbox_windows_test.go @@ -22,7 +22,7 @@ func TestAccessReadAllowedWindows(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "readable.txt"), []byte("data"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -34,7 +34,7 @@ func TestAccessReadAllowedWindows(t *testing.T) { func TestAccessNonexistentWindows(t *testing.T) { dir := t.TempDir() - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -49,7 +49,7 @@ func TestAccessOutsideSandboxWindows(t *testing.T) { outside := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("secret"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -63,7 +63,7 @@ func TestAccessDirectoryReadWindows(t *testing.T) { dir := t.TempDir() require.NoError(t, os.Mkdir(filepath.Join(dir, "subdir"), 0755)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -78,7 +78,7 @@ func TestAccessSymlinkEscapeBlockedWindows(t *testing.T) { 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(dir, "escape.txt"))) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -93,7 +93,7 @@ func TestAccessSymlinkWithinSandboxWindows(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(dir, "target.txt"), []byte("data"), 0644)) require.NoError(t, os.Symlink("target.txt", filepath.Join(dir, "link.txt"))) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -106,7 +106,7 @@ func TestAccessWriteDeniedWindows(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "readonly.txt"), []byte("data"), 0444)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() @@ -119,7 +119,7 @@ func TestAccessExecAlwaysDeniedWindows(t *testing.T) { dir := t.TempDir() require.NoError(t, os.WriteFile(filepath.Join(dir, "data.txt"), []byte("data"), 0644)) - sb, err := New([]string{dir}) + sb, _, err := New([]string{dir}) require.NoError(t, err) defer sb.Close() diff --git a/analysis/symbols_allowedpaths.go b/analysis/symbols_allowedpaths.go index aac1dd5a..c129e39d 100644 --- a/analysis/symbols_allowedpaths.go +++ b/analysis/symbols_allowedpaths.go @@ -17,11 +17,13 @@ package analysis // // The permanently banned packages (reflect, unsafe) apply here too. var allowedpathsAllowedSymbols = []string{ + "bytes.Buffer", // 🟢 in-memory byte buffer; collects sandbox warnings for deferred output. "context.Context", // 🟢 context type used to signal cancellation; no I/O or side effects. "errors.As", // 🟢 error type assertion; pure function, no I/O. "errors.Is", // 🟢 error comparison; pure function, no I/O. "errors.New", // 🟢 creates a simple error value; pure function, no I/O. "fmt.Errorf", // 🟢 formatted error creation; pure function, no I/O. + "fmt.Fprintf", // 🟠 writes warning messages to in-memory buffer during sandbox construction. "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. diff --git a/interp/allowed_paths_test.go b/interp/allowed_paths_test.go index 7a34d606..8106977b 100644 --- a/interp/allowed_paths_test.go +++ b/interp/allowed_paths_test.go @@ -57,23 +57,23 @@ func runScript(t *testing.T, script, dir string, opts ...interp.RunnerOption) (s } func TestAllowedPathsOption(t *testing.T) { - t.Run("invalid path rejected", func(t *testing.T) { - _, err := interp.New( + t.Run("nonexistent path skipped", func(t *testing.T) { + runner, err := interp.New( interp.AllowedPaths([]string{"/nonexistent/path/that/does/not/exist"}), ) - require.Error(t, err) - assert.Contains(t, err.Error(), "AllowedPaths") + require.NoError(t, err, "nonexistent paths should be skipped, not rejected") + runner.Close() }) - t.Run("file not directory rejected", func(t *testing.T) { + t.Run("file path skipped", func(t *testing.T) { tmpFile := filepath.Join(t.TempDir(), "file.txt") require.NoError(t, os.WriteFile(tmpFile, []byte("test"), 0644)) - _, err := interp.New( + runner, err := interp.New( interp.AllowedPaths([]string{tmpFile}), ) - require.Error(t, err) - assert.Contains(t, err.Error(), "not a directory") + require.NoError(t, err, "file paths should be skipped (not directories)") + runner.Close() }) t.Run("valid directory accepted", func(t *testing.T) { diff --git a/interp/api.go b/interp/api.go index 94e10489..0f2bc50d 100644 --- a/interp/api.go +++ b/interp/api.go @@ -51,6 +51,11 @@ type runnerConfig struct { // nil (default) blocks all file access; populate via AllowedPaths option. sandbox *allowedpaths.Sandbox + // sandboxWarnings holds diagnostic messages about skipped AllowedPaths + // entries. Written to stderr after all options are applied and defaults + // are set, so the output target is independent of option ordering. + sandboxWarnings []byte + // allowedCommands is the set of command names (builtins or external) that // the interpreter is permitted to execute. If nil and allowAllCommands is // false, no commands are allowed. @@ -235,6 +240,11 @@ func New(opts ...RunnerOption) (*Runner, error) { if r.stdout == nil || r.stderr == nil { StdIO(r.stdin, r.stdout, r.stderr)(r) } + // Flush any sandbox warnings now that stderr is guaranteed to be set. + if len(r.sandboxWarnings) > 0 { + r.stderr.Write(r.sandboxWarnings) + r.sandboxWarnings = nil + } r.proc = builtins.NewProcProvider(r.procPath) return r, nil } @@ -549,11 +559,12 @@ func (r *Runner) Close() error { // An empty slice also blocks all file access. func AllowedPaths(paths []string) RunnerOption { return func(r *Runner) error { - sb, err := allowedpaths.New(paths) + sb, warnings, err := allowedpaths.New(paths) if err != nil { return err } r.sandbox = sb + r.sandboxWarnings = warnings return nil } } diff --git a/tests/scenarios/shell/allowed_paths/nonexistent_path_skipped.yaml b/tests/scenarios/shell/allowed_paths/nonexistent_path_skipped.yaml new file mode 100644 index 00000000..4bbbec88 --- /dev/null +++ b/tests/scenarios/shell/allowed_paths/nonexistent_path_skipped.yaml @@ -0,0 +1,17 @@ +description: Nonexistent paths in AllowedPaths are skipped, existing paths still work +# skip: AllowedPaths skip logic is an rshell-specific feature not present in bash +skip_assert_against_bash: true +setup: + files: + - path: allowed/data.txt + content: "hello\n" +input: + allowed_paths: ["nonexistent_dir", "allowed"] + script: |+ + cat allowed/data.txt +expect: + stdout: |+ + hello + stderr_contains: + - "AllowedPaths: skipping" + exit_code: 0