Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions SHELL_FEATURES.md
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ Blocked features are rejected before execution with exit code 2.
- ✅ Empty by default — no parent environment variables are inherited
- ✅ Caller-provided variables via the `Env` option
- ✅ `IFS` is set to space/tab/newline by default
- ✅ `ALLOWED_PATHS` — when `AllowedPaths` is configured, set to a `filepath.ListSeparator`-delimited list of resolved allowed directories (`:` on Unix, `;` on Windows)
- ❌ No automatic inheritance from the host process
- ❌ `export`, `readonly` are blocked

Expand Down
12 changes: 12 additions & 0 deletions allowedpaths/sandbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,18 @@ func (s *Sandbox) HostPrefix() string {
return s.hostPrefix
}

// Paths returns the resolved absolute paths of all allowed directories.
func (s *Sandbox) Paths() []string {
if s == nil {
return nil
}
paths := make([]string, len(s.roots))
for i, r := range s.roots {
paths[i] = r.absPath
}
return paths
}

// Close releases all os.Root file descriptors. It is safe to call multiple times.
func (s *Sandbox) Close() error {
if s == nil {
Expand Down
102 changes: 52 additions & 50 deletions analysis/symbols_interp.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,56 +17,58 @@ package analysis
//
// The permanently banned packages (reflect, unsafe) apply here too.
var interpAllowedSymbols = []string{
"bytes.Buffer", // 🟢 in-memory byte buffer; pure data structure, no I/O.
"context.Background", // 🟢 returns the empty background context; used in StdIO option where no run-scoped context is available.
"context.CancelFunc", // 🟢 function type returned by WithTimeout/WithCancel; pure function type, no side effects.
"context.Context", // 🟢 deadline/cancellation plumbing; pure interface, no side effects.
"context.WithTimeout", // 🟢 derives a context with a deadline; needed for execution timeout support.
"context.WithValue", // 🟢 derives a context carrying a key-value pair; pure function.
"errors.As", // 🟢 error type assertion; pure function, no I/O.
"errors.New", // 🟢 creates a sentinel error value; pure function, no I/O.
"fmt.Errorf", // 🟢 formatted error creation; pure function, no I/O.
"fmt.Fprintf", // 🟠 formatted write to an io.Writer; delegates to Write, no filesystem access.
"fmt.Fprintln", // 🟠 writes to an io.Writer with newline; delegates to Write, no filesystem access.
"fmt.Sprintf", // 🟢 string formatting; pure function, no I/O.
"io.Closer", // 🟢 interface type for closing; no side effects.
"io.Copy", // 🟠 copies from Reader to Writer; no filesystem access, delegates to Read/Write.
"io.Discard", // 🟢 write sink that discards all data; no side effects.
"io.LimitReader", // 🟢 wraps a Reader with a byte cap; pure function, no I/O.
"io.Reader", // 🟢 interface type for reading; no side effects.
"io.ReadWriteCloser", // 🟢 combined interface type; no side effects.
"io.Writer", // 🟢 interface type for writing; no side effects.
"io/fs.DirEntry", // 🟢 interface type for directory entries; no side effects.
"io/fs.FileInfo", // 🟢 interface type for file metadata; no side effects.
"io/fs.ReadDirFile", // 🟢 read-only directory handle interface; no write capability.
"maps.Insert", // 🟢 inserts all key-value pairs from one map into another; pure function.
"os.DirEntry", // 🟢 type alias for fs.DirEntry; no side effects.
"os.File", // 🟠 file handle type; interpreter needs file I/O for redirects and pipes.
"os.FileMode", // 🟢 file permission bits type; pure type.
"os.Getwd", // 🟠 returns current working directory; read-only.
"os.O_RDONLY", // 🟢 read-only file flag constant; pure constant.
"os.PathError", // 🟢 error type wrapping path and operation; pure type.
"os.Pipe", // 🟠 creates an OS pipe pair; needed for shell pipelines.
"path/filepath.IsAbs", // 🟢 checks if path is absolute; pure function, no I/O.
"path/filepath.Join", // 🟢 joins path elements; pure function, no I/O.
"runtime.GOOS", // 🟢 current OS name constant; pure constant, no I/O.
"strconv.Itoa", // 🟢 int-to-string conversion; pure function, no I/O.
"strings.Builder", // 🟢 efficient string concatenation; pure in-memory buffer, no I/O.
"strings.ContainsRune", // 🟢 checks if a rune is in a string; pure function, no I/O.
"strings.NewReader", // 🟢 wraps a string as an io.Reader; pure function, no I/O; used by ParseScript.
"strings.Index", // 🟢 finds substring index; pure function, no I/O.
"strings.HasPrefix", // 🟢 pure function for prefix matching; no I/O.
"strings.HasSuffix", // 🟢 pure function for suffix matching; no I/O.
"strings.Split", // 🟢 splits a string by separator; pure function, no I/O.
"strings.ToUpper", // 🟢 converts string to uppercase; pure function, no I/O.
"strings.TrimLeft", // 🟢 trims leading characters; pure function, no I/O.
"sync.Mutex", // 🟢 mutual exclusion lock; concurrency primitive, no I/O.
"sync.Once", // 🟢 ensures a function runs exactly once; concurrency primitive, no I/O.
"sync.WaitGroup", // 🟢 waits for goroutines to finish; concurrency primitive, no I/O.
"sync/atomic.Int64", // 🟢 atomic int64 counter; concurrency primitive, no I/O.
"time.Duration", // 🟢 numeric duration type; pure type, no side effects.
"time.Now", // 🟠 returns current time; read-only, no mutation.
"time.Time", // 🟢 time value type; pure data, no side effects.
"bytes.Buffer", // 🟢 in-memory byte buffer; pure data structure, no I/O.
"context.Background", // 🟢 returns the empty background context; used in StdIO option where no run-scoped context is available.
"context.CancelFunc", // 🟢 function type returned by WithTimeout/WithCancel; pure function type, no side effects.
"context.Context", // 🟢 deadline/cancellation plumbing; pure interface, no side effects.
"context.WithTimeout", // 🟢 derives a context with a deadline; needed for execution timeout support.
"context.WithValue", // 🟢 derives a context carrying a key-value pair; pure function.
"errors.As", // 🟢 error type assertion; pure function, no I/O.
"errors.New", // 🟢 creates a sentinel error value; pure function, no I/O.
"fmt.Errorf", // 🟢 formatted error creation; pure function, no I/O.
"fmt.Fprintf", // 🟠 formatted write to an io.Writer; delegates to Write, no filesystem access.
"fmt.Fprintln", // 🟠 writes to an io.Writer with newline; delegates to Write, no filesystem access.
"fmt.Sprintf", // 🟢 string formatting; pure function, no I/O.
"io.Closer", // 🟢 interface type for closing; no side effects.
"io.Copy", // 🟠 copies from Reader to Writer; no filesystem access, delegates to Read/Write.
"io.Discard", // 🟢 write sink that discards all data; no side effects.
"io.LimitReader", // 🟢 wraps a Reader with a byte cap; pure function, no I/O.
"io.Reader", // 🟢 interface type for reading; no side effects.
"io.ReadWriteCloser", // 🟢 combined interface type; no side effects.
"io.Writer", // 🟢 interface type for writing; no side effects.
"io/fs.DirEntry", // 🟢 interface type for directory entries; no side effects.
"io/fs.FileInfo", // 🟢 interface type for file metadata; no side effects.
"io/fs.ReadDirFile", // 🟢 read-only directory handle interface; no write capability.
"maps.Insert", // 🟢 inserts all key-value pairs from one map into another; pure function.
"os.DirEntry", // 🟢 type alias for fs.DirEntry; no side effects.
"os.File", // 🟠 file handle type; interpreter needs file I/O for redirects and pipes.
"os.FileMode", // 🟢 file permission bits type; pure type.
"os.Getwd", // 🟠 returns current working directory; read-only.
"os.O_RDONLY", // 🟢 read-only file flag constant; pure constant.
"os.PathError", // 🟢 error type wrapping path and operation; pure type.
"os.Pipe", // 🟠 creates an OS pipe pair; needed for shell pipelines.
"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.ListSeparator", // 🟢 OS-specific path list separator; pure constant.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually what we want? I think for lists in shell scripting you usually space-separate them, this I think is for constructing known variables like PATH that have a specific format

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's a clean separation between filepaths. i avoided spaces because i'm worried about file paths/directories with spaces in them. but if that's the case then i can change

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, I guess what we really want here is an "array" type like in Bash, but not sure whether we can support that in our interp. Is it going to be able to iterate over them if they're colon-separated though? It might need to use tr to do that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was imagining the agent doing something like echo $ALLOWED_PATHS and inferring the result from there. I believe even if it does something like splitting the result with tr and printing each directory on a line, the result from PAR gets sent back with a \n effectively acting as the same delimiter. so /path/a:/path/b:path/c -> /path/a\n/path/b\n/path/b.

Alternatively, it could just not do string manipulation and return the result but then whether it is a : or a space delimiting the list, I believe the LLM should be able to infer the result (we can probably edit the skill to let it know that it will be delimited by a special character).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I was imagining it doing something like "loop over the paths and call ls on each one" as a single pass. Agree if the value makes it back to the LLM it'll be able to figure it out, just wanted to cut out that roundtrip if possible.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah i see. i think we can just merge and if we see the LLM having trouble we can rethink?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be a pain to change later so it's probably worth at least seeing how the LLMs are likely to try to use this list to determine whether there are any gaps in our coverage. Here's some anecdata, does rshell support either of these?

Image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, both work

$ ./rshell --allowed-paths /var/log,/tmp/,/Users/matthew.deguzman --allow-all-commands -c 'IFS=":"; for dir in $ALLOWED_PATHS; do echo "$dir"; done'
/var/log
/tmp
/Users/matthew.deguzman

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Word, send it

"runtime.GOOS", // 🟢 current OS name constant; pure constant, no I/O.
"strconv.Itoa", // 🟢 int-to-string conversion; pure function, no I/O.
"strings.Builder", // 🟢 efficient string concatenation; pure in-memory buffer, no I/O.
"strings.ContainsRune", // 🟢 checks if a rune is in a string; pure function, no I/O.
"strings.NewReader", // 🟢 wraps a string as an io.Reader; pure function, no I/O; used by ParseScript.
"strings.Index", // 🟢 finds substring index; pure function, no I/O.
"strings.HasPrefix", // 🟢 pure function for prefix matching; no I/O.
"strings.HasSuffix", // 🟢 pure function for suffix 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.
"strings.ToUpper", // 🟢 converts string to uppercase; pure function, no I/O.
"strings.TrimLeft", // 🟢 trims leading characters; pure function, no I/O.
"sync.Mutex", // 🟢 mutual exclusion lock; concurrency primitive, no I/O.
"sync.Once", // 🟢 ensures a function runs exactly once; concurrency primitive, no I/O.
"sync.WaitGroup", // 🟢 waits for goroutines to finish; concurrency primitive, no I/O.
"sync/atomic.Int64", // 🟢 atomic int64 counter; concurrency primitive, no I/O.
"time.Duration", // 🟢 numeric duration type; pure type, no side effects.
"time.Now", // 🟠 returns current time; read-only, no mutation.
"time.Time", // 🟢 time value type; pure data, no side effects.

// --- mvdan.cc/sh/v3/expand --- (shell word expansion library)

Expand Down
98 changes: 98 additions & 0 deletions interp/allowed_paths_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,101 @@ func TestHostPrefixDefaultWhenNotSet(t *testing.T) {

assert.Empty(t, runner.sandbox.HostPrefix())
}

// TestAllowedPathsEnvVar verifies that ALLOWED_PATHS is set in the
// interpreter's environment with the resolved absolute paths.
func TestAllowedPathsEnvVar(t *testing.T) {
dir1 := t.TempDir()
dir2 := t.TempDir()

stdout, _, _ := runScriptInternal(t, `echo $ALLOWED_PATHS`, dir1,
AllowedPaths([]string{dir1, dir2}),
)

expected := dir1 + string(filepath.ListSeparator) + dir2
assert.Equal(t, expected+"\n", stdout)
}

// TestAllowedPathsEnvVarSinglePath verifies ALLOWED_PATHS with one path.
func TestAllowedPathsEnvVarSinglePath(t *testing.T) {
dir := t.TempDir()

stdout, _, _ := runScriptInternal(t, `echo $ALLOWED_PATHS`, dir,
AllowedPaths([]string{dir}),
)

assert.Equal(t, dir+"\n", stdout)
}

// TestAllowedPathsEnvVarManyDirs verifies ALLOWED_PATHS with several directories.
func TestAllowedPathsEnvVarManyDirs(t *testing.T) {
dirs := make([]string, 5)
for i := range dirs {
dirs[i] = t.TempDir()
}

stdout, _, _ := runScriptInternal(t, `echo $ALLOWED_PATHS`, dirs[0],
AllowedPaths(dirs),
)

expected := strings.Join(dirs, string(filepath.ListSeparator))
assert.Equal(t, expected+"\n", stdout)
}

// TestAllowedPathsEnvVarNestedDirs verifies ALLOWED_PATHS with deeply
// nested directories.
func TestAllowedPathsEnvVarNestedDirs(t *testing.T) {
root := t.TempDir()
nested1 := filepath.Join(root, "a", "b", "c")
nested2 := filepath.Join(root, "x", "y")
require.NoError(t, os.MkdirAll(nested1, 0755))
require.NoError(t, os.MkdirAll(nested2, 0755))

stdout, _, _ := runScriptInternal(t, `echo $ALLOWED_PATHS`, root,
AllowedPaths([]string{nested1, nested2}),
)

expected := nested1 + string(filepath.ListSeparator) + nested2
assert.Equal(t, expected+"\n", stdout)
}

// TestAllowedPathsEnvVarParentAndChild verifies ALLOWED_PATHS when both
// a parent and child directory are allowed.
func TestAllowedPathsEnvVarParentAndChild(t *testing.T) {
root := t.TempDir()
child := filepath.Join(root, "sub", "dir")
require.NoError(t, os.MkdirAll(child, 0755))

stdout, _, _ := runScriptInternal(t, `echo $ALLOWED_PATHS`, root,
AllowedPaths([]string{root, child}),
)

expected := root + string(filepath.ListSeparator) + child
assert.Equal(t, expected+"\n", stdout)
}

// TestAllowedPathsEnvVarSkipsNonexistent verifies that ALLOWED_PATHS only
// contains directories that were successfully opened.
func TestAllowedPathsEnvVarSkipsNonexistent(t *testing.T) {
dir := t.TempDir()

stdout, _, _ := runScriptInternal(t, `echo $ALLOWED_PATHS`, dir,
AllowedPaths([]string{"/nonexistent/path", dir}),
)

assert.Equal(t, dir+"\n", stdout)
}

// TestAllowedPathsEnvVarNotSetWithoutSandbox verifies that ALLOWED_PATHS
// is not set when AllowedPaths is not configured.
func TestAllowedPathsEnvVarNotSetWithoutSandbox(t *testing.T) {
dir := t.TempDir()

runner, err := New()
require.NoError(t, err)
defer runner.Close()

runner.Dir = dir
v := runner.Env.Get("ALLOWED_PATHS")
assert.False(t, v.IsSet(), "ALLOWED_PATHS should not be set without AllowedPaths")
}
13 changes: 9 additions & 4 deletions interp/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"fmt"
"io"
"os"
"path/filepath"
"strings"
"sync/atomic"
"time"
Expand Down Expand Up @@ -230,7 +231,6 @@ func New(opts ...RunnerOption) (*Runner, error) {
}
}

// Set the default fallbacks, if necessary.
// Default to an empty environment to avoid propagating parent env vars.
if r.Env == nil {
r.Env = expand.ListEnviron()
Expand Down Expand Up @@ -429,11 +429,16 @@ func (r *Runner) Reset() {
// blocks all external execution, limiting the practical impact of this vector.
r.setVarString("IFS", " \t\n")
r.setVarString("OPTIND", "1")
if r.sandbox != nil {
r.setVarString("ALLOWED_PATHS", strings.Join(r.sandbox.Paths(), string(filepath.ListSeparator)))
Comment thread
matt-dz marked this conversation as resolved.
Comment thread
matt-dz marked this conversation as resolved.
}
Comment thread
matt-dz marked this conversation as resolved.
Comment thread
matt-dz marked this conversation as resolved.

// Reset the total-bytes counter so that the interpreter's own initial
// variable assignments (PWD, IFS, OPTIND above) do not count against the
// user-visible MaxTotalVarsBytes cap. Those values are small and bounded;
// only the variables that a script itself creates or modifies should count.
// variable assignments (PWD, IFS, OPTIND, ALLOWED_PATHS above) do not
// count against the user-visible MaxTotalVarsBytes cap. Those values are
// small and bounded; only the variables that a script itself creates or
// modifies should count. ALLOWED_PATHS is operator-configured and
// typically a few hundred bytes, so this is safe.
if ov, ok := r.writeEnv.(*overlayEnviron); ok {
ov.totalBytes = 0
}
Expand Down
18 changes: 18 additions & 0 deletions tests/scenarios/shell/allowed_paths/allowed_paths_env_var.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
description: ALLOWED_PATHS environment variable lists resolved allowed directories
# skip: ALLOWED_PATHS env var is an rshell-specific feature
skip_assert_against_bash: true
setup:
files:
- path: dir1/file.txt
content: "a\n"
- path: dir2/file.txt
content: "b\n"
input:
allowed_paths: ["dir1", "dir2"]
script: |+
echo $ALLOWED_PATHS
expect:
stdout_contains:
- "dir1"
- "dir2"
exit_code: 0
Loading