From 3b274d4a1a3d39720f60e9d49b07650d19bc74c5 Mon Sep 17 00:00:00 2001 From: Matthew DeGuzman Date: Mon, 27 Apr 2026 14:28:12 -0400 Subject: [PATCH] feat(interp): add WarningsWriter option and Warnings accessor for sandbox diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AllowedPaths warnings are flushed to r.stderr during Runner construction. For library callers (e.g. PAR) that surface r.stderr in structured output, this means every script execution in a session ends up with a non-empty stderr whenever the configured allowlist contains a path that does not exist on the host (e.g. /var/log on Windows), even though exitCode is 0 — making the result look like a command failure. Decouple sandbox diagnostics from r.stderr so callers can attribute the two streams independently: - WarningsWriter(io.Writer) routes the buffered warnings to a dedicated sink. Defaults to r.stderr when unset, preserving today's behaviour byte-for-byte for callers that don't opt in. - Runner.Warnings() returns the warnings as []string for integrators that prefer to render them in their own structured output rather than scraping a writer. Closes the rshell side of #191. The PAR (datadog-agent) and remote-action UI (dd-source) changes will land separately and consume this API. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 2 +- interp/allowed_paths_test.go | 83 ++++++++++++++++++++++++++++++++++++ interp/api.go | 72 ++++++++++++++++++++++++++++--- 3 files changed, 151 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 9208fc3b..b8f75bf3 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,7 @@ Every access path is default-deny: **AllowedCommands** restricts which commands (builtins or external) the interpreter may execute. Commands must be specified with the `rshell:` namespace prefix (e.g. `rshell:cat`, `rshell:echo`). If not set, no commands are allowed. -**AllowedPaths** restricts all file operations to specified directories using Go's `os.Root` API (`openat` syscalls), making it immune to symlink traversal, TOCTOU races, and `..` escape attacks. +**AllowedPaths** restricts all file operations to specified directories using Go's `os.Root` API (`openat` syscalls), making it immune to symlink traversal, TOCTOU races, and `..` escape attacks. Configured directories that cannot be opened (missing, not a directory, no permission) are skipped with a diagnostic message; by default these messages are flushed once to the runner's stderr at construction time. Callers that need to keep stderr clean of sandbox diagnostics can route them to a dedicated sink with `WarningsWriter(io.Writer)` or retrieve them programmatically via `Runner.Warnings()`. > **Note:** The `ss` and `ip route` builtins bypass `AllowedPaths` for their `/proc/net/*` reads. Both builtins open kernel pseudo-filesystem paths (e.g. `/proc/net/tcp`, `/proc/net/route`) directly with `os.Open` rather than going through the sandboxed opener. These paths are hardcoded in the implementation and are never derived from user input, so there is no sandbox-escape risk. However, operators cannot use `AllowedPaths` to block `ss` from enumerating local sockets or `ip route` from reading the routing table — these reads succeed regardless of the configured path policy. diff --git a/interp/allowed_paths_test.go b/interp/allowed_paths_test.go index 8106977b..bf89c37a 100644 --- a/interp/allowed_paths_test.go +++ b/interp/allowed_paths_test.go @@ -9,6 +9,7 @@ import ( "bytes" "context" "errors" + "io" "os" "path/filepath" "runtime" @@ -289,3 +290,85 @@ func TestAllowedPathsClose(t *testing.T) { require.NoError(t, runner.Close()) require.NoError(t, runner.Close()) } + +func TestSandboxWarningsDefaultGoToStderr(t *testing.T) { + var outBuf, errBuf bytes.Buffer + runner, err := interp.New( + interp.StdIO(nil, &outBuf, &errBuf), + interp.AllowedPaths([]string{"/nonexistent/path/that/does/not/exist"}), + ) + require.NoError(t, err) + defer runner.Close() + + assert.Contains(t, errBuf.String(), "AllowedPaths: skipping", + "with no WarningsWriter option, warnings should fall back to stderr") + assert.Empty(t, outBuf.String(), "warnings must never land on stdout") +} + +func TestSandboxWarningsRoutedToDedicatedWriter(t *testing.T) { + var outBuf, errBuf, warnBuf bytes.Buffer + runner, err := interp.New( + interp.StdIO(nil, &outBuf, &errBuf), + interp.WarningsWriter(&warnBuf), + interp.AllowedPaths([]string{"/nonexistent/path/that/does/not/exist"}), + ) + require.NoError(t, err) + defer runner.Close() + + assert.Contains(t, warnBuf.String(), "AllowedPaths: skipping", + "warnings should be routed to the dedicated writer") + assert.Empty(t, errBuf.String(), + "with WarningsWriter set, stderr must remain clean of sandbox diagnostics") + assert.Empty(t, outBuf.String(), "warnings must never land on stdout") +} + +func TestSandboxWarningsAccessor(t *testing.T) { + t.Run("returns warnings as slice", func(t *testing.T) { + runner, err := interp.New( + interp.WarningsWriter(io.Discard), + interp.AllowedPaths([]string{ + "/nonexistent/path/one", + "/nonexistent/path/two", + }), + ) + require.NoError(t, err) + defer runner.Close() + + warnings := runner.Warnings() + require.Len(t, warnings, 2, "one entry per skipped path") + assert.Contains(t, warnings[0], "AllowedPaths: skipping") + assert.Contains(t, warnings[1], "AllowedPaths: skipping") + }) + + t.Run("returns nil when no warnings emitted", func(t *testing.T) { + dir := t.TempDir() + runner, err := interp.New( + interp.AllowedPaths([]string{dir}), + ) + require.NoError(t, err) + defer runner.Close() + + assert.Nil(t, runner.Warnings(), + "a clean configuration should yield no warnings") + }) + + t.Run("accessor still works when streaming flush is suppressed", func(t *testing.T) { + runner, err := interp.New( + interp.WarningsWriter(io.Discard), + interp.AllowedPaths([]string{"/nonexistent/path"}), + ) + require.NoError(t, err) + defer runner.Close() + + warnings := runner.Warnings() + require.Len(t, warnings, 1, + "io.Discard should suppress streaming output but not the accessor") + assert.Contains(t, warnings[0], "AllowedPaths: skipping") + }) +} + +func TestWarningsWriterRejectsNil(t *testing.T) { + _, err := interp.New(interp.WarningsWriter(nil)) + require.Error(t, err) + assert.Contains(t, err.Error(), "writer must not be nil") +} diff --git a/interp/api.go b/interp/api.go index 8fdda9a3..691cea13 100644 --- a/interp/api.go +++ b/interp/api.go @@ -55,10 +55,19 @@ type runnerConfig struct { 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. + // entries. Flushed to warningsWriter after all options are applied and + // defaults are set, so the output target is independent of option + // ordering. Retained on the runner after flush so callers can also + // retrieve them programmatically via [Runner.Warnings]. sandboxWarnings []byte + // warningsWriter is the destination for sandbox diagnostic messages + // (currently just AllowedPaths skip warnings). Defaults to r.stderr + // when unset; callers can route warnings to a separate sink (e.g. a + // dedicated buffer or logger) via [WarningsWriter] so they are not + // conflated with command stderr. + warningsWriter io.Writer + // hostPrefix is stored here so HostPrefix can be applied before or // after AllowedPaths. Applied to the sandbox in New() after all // options are processed. @@ -280,15 +289,21 @@ func New(opts ...RunnerOption) (*Runner, error) { if r.stdout == nil || r.stderr == nil { StdIO(r.stdin, r.stdout, r.stderr)(r) } + // Default sandbox warnings to r.stderr so today's behaviour is + // preserved for callers who do not opt in to a dedicated sink. + if r.warningsWriter == nil { + r.warningsWriter = r.stderr + } // Apply host prefix if set, now that both HostPrefix and AllowedPaths // have been processed regardless of option ordering. if r.hostPrefix != "" && r.sandbox != nil { r.sandbox.SetHostPrefix(r.hostPrefix) } - // Flush any sandbox warnings now that stderr is guaranteed to be set. + // Flush any sandbox warnings now that the warnings sink is guaranteed + // to be set. The buffer is retained on the runner so callers can also + // retrieve warnings via [Runner.Warnings]. if len(r.sandboxWarnings) > 0 { - r.stderr.Write(r.sandboxWarnings) - r.sandboxWarnings = nil + r.warningsWriter.Write(r.sandboxWarnings) } r.proc = builtins.NewProcProvider(r.procPath) return r, nil @@ -631,6 +646,53 @@ func (r *Runner) Close() error { return r.sandbox.Close() } +// WarningsWriter routes sandbox diagnostic messages (currently produced by +// [AllowedPaths] when a configured directory cannot be opened) to the given +// writer instead of the runner's stderr. +// +// Sandbox diagnostics are buffered during option processing and flushed once +// during [New], after all other options have been applied. They are not +// written again on subsequent runs. +// +// When unset, warnings fall back to the runner's stderr writer (whatever was +// supplied via [StdIO]), matching the historical behaviour. Callers that +// inspect stderr to detect command failure should pass a dedicated writer +// here so sandbox diagnostics are not conflated with command output. +// +// Passing [io.Discard] suppresses the streaming flush entirely; the messages +// remain accessible via [Runner.Warnings] regardless. +func WarningsWriter(w io.Writer) RunnerOption { + return func(r *Runner) error { + if w == nil { + return fmt.Errorf("WarningsWriter: writer must not be nil") + } + r.warningsWriter = w + return nil + } +} + +// Warnings returns the sandbox diagnostic messages collected during runner +// construction (currently produced by [AllowedPaths] when a configured +// directory cannot be opened), one entry per warning. The slice is empty when +// no warnings were emitted. +// +// Callers that integrate rshell as a library can use this to surface +// configuration problems in their own structured output (e.g. a "warnings" +// field in an API response) without parsing them out of stderr. +func (r *Runner) Warnings() []string { + if len(r.sandboxWarnings) == 0 { + return nil + } + s := string(r.sandboxWarnings) + // allowedpaths.New emits one warning per line, each terminated with + // "\n". Strip a single trailing newline before splitting so the result + // is one entry per warning rather than ending in a stray empty string. + if strings.HasSuffix(s, "\n") { + s = s[:len(s)-1] + } + return strings.Split(s, "\n") +} + // AllowedPaths restricts file and directory access to the specified directories. // Paths must be absolute directories that exist. When set, only files within // these directories can be opened, read, or executed.