feat(interp): default working dir to first allowed path#225
Conversation
When AllowedPaths is set but Runner.Dir isn't explicitly configured, start in the first allowed path instead of os.Getwd(). This avoids landing the shell in a directory the sandbox can't access. Falls back to os.Getwd() when no allowed paths are configured (or all failed to open), preserving the previous behaviour for unsandboxed runs. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| if paths := r.sandbox.Paths(); len(paths) > 0 { | ||
| r.Dir = paths[0] | ||
| } else { | ||
| dir, err := os.Getwd() |
There was a problem hiding this comment.
instead of os.Getwd()
Can/shall we default to None / empty string ? and block access when file access commands are used based on default folder?
There was a problem hiding this comment.
[Claude Opus 4.7] Looked at how datadog-agent calls rshell (pkg/privateactionrunner/bundles/remoteaction/rshell/run_command.go): it never sets Runner.Dir, so it relies on this fallback.
In both deployment modes the agent process cwd is /:
- K8s: agent Dockerfile (
Dockerfiles/agent/Dockerfile) sets no WORKDIR in thereleasestage → Docker default/. - Bare metal: systemd service with no
WorkingDirectory=→ systemd default/.
So today the script always starts in / (outside any allowed path), which makes pwd, $PWD, and relative reads useless. With this PR it starts in the first allowed path — strictly more useful, and the os.Getwd() fallback only matters when AllowedPaths is empty/all-skipped (sandbox blocks all file access anyway, so Dir is only cosmetic for pwd/$PWD). Defaulting to "" would break legitimate relative-path scripts without security gain. Happy to revisit in a follow-up if you'd like.
Adds: - interp/allowed_paths_test.go: TestDefaultDirFromAllowedPaths covers the New() defaults — first-allowed-path when sandbox has paths, os.Getwd() when no sandbox, and os.Getwd() when all paths are skipped. - cmd/rshell/main_test.go: TestDefaultDirIsFirstAllowedPath and TestDefaultDirPicksFirstOfMany exercise the behaviour end-to-end via the CLI by reading a marker file with a relative path. Note: a YAML scenario test isn't a good fit here because the scenario test framework always overrides runner.Dir to the temp dir after New() (tests/scenarios_test.go line 229), so the new default never takes effect through that path. The CLI tests cover the same end-to-end shape. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
2 participants
Summary
AllowedPathsis configured butRunner.Dirisn't explicitly set, the shell now starts in the first allowed path instead ofos.Getwd().os.Getwd()when no allowed paths are configured, or when all configured paths failed to open — preserving previous behaviour for unsandboxed runs.Runner.Dirassignments are still respected, even if outside the allowlist.Test plan
go test ./interp/...passesrshell -p /tmp -c 'pwd'→/tmprshell -p /var,/tmp -c 'pwd'→/var(first entry)rshell -c 'pwd'(no-p) →os.Getwd()(unchanged)🤖 Generated with Claude Code