feat(help): list configured AllowedPaths in help output by AlexandreYang · Pull Request #248 · DataDog/rshell

AlexandreYang · 2026-05-13T15:19:15Z

Summary

Add an Allowed paths: section to the default help output that lists the configured sandbox roots, one per line.
When no paths are configured, surface (no allowed paths configured — all filesystem access blocked) so the empty state is unambiguous (per interp/api.go semantics, an empty AllowedPaths blocks all filesystem access — it is not "unrestricted").
Expose Sandbox.Paths() to builtins via a new CallContext.AllowedPathsList accessor, wired in both runner_exec.go CallContext constructions.

Test plan

go build ./...
make fmt
go test ./builtins/tests/help/... — including three new tests covering single path, multiple paths line-per-line, and the empty-state notice.
go test ./tests/ -run TestShellScenarios — including two new scenarios (allowed_paths.yaml, no_allowed_paths.yaml).
Manual end-to-end with rshell --allow-all-commands [-p /tmp,/var/log] -c help.

Show the active sandbox roots in `help`, with an explicit notice when none are configured so operators can tell that state apart from "all filesystem access allowed".

AlexandreYang · 2026-05-13T15:20:22Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 153bb55ac1

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

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".

AlexandreYang

Self-Review (iteration 1)

Reviewed PR #248 — adds an Allowed paths: section to the help builtin output, wires a new CallContext.AllowedPathsList accessor backed by Sandbox.Paths(), and adds Go + scenario coverage.

Overall assessment: safe to merge with minor doc/wording polish. No P0/P1 findings.

Findings summary

#	File	Finding
1	`SHELL_FEATURES.md` / `README.md`	Docs not updated to mention the new `help` section (project rule requires keeping them in sync)
2	`builtins/help/help.go:191`	Empty-state notice claims "all filesystem access blocked" — slightly imprecise since `ss`, `ip route`, `df`, `ps` intentionally bypass AllowedPaths for kernel-state reads
3	`builtins/help/help.go` package docstring (lines 8-16, outside diff)	Package overview doesn't mention the new section (cannot inline-comment since lines aren't in the diff)

Coverage

Code path	Scenario test	Go test	Status
Configured paths render	`tests/scenarios/cmd/help/allowed_paths.yaml`	`help_test.go:411`	Covered
Multiple paths line-per-line	—	`help_test.go:421`	Covered
Empty AllowedPaths notice	`tests/scenarios/cmd/help/no_allowed_paths.yaml`	`help_test.go:432`	Covered
`r.sandbox == nil` branch in accessors	—	—	Implicit (same observable behavior as empty list)

Positive observations

Nil-safe accessors in both CallContext constructions in runner_exec.go — defensive against r.sandbox == nil.
printAllowedPaths early-returns when callCtx.AllowedPathsList itself is nil — backward-compatible with any caller that builds a CallContext without wiring the new accessor.
Empty-state messaging follows the precedent set by the existing "All commands are allowed in this session" line.
skip_assert_against_bash: true is correctly set on both new scenario YAMLs (bash has no AllowedPaths concept).

AlexandreYang · 2026-05-13T15:32:05Z

Iteration 1 self-review result

P0: 0
P1: 0
P2: 1 (SHELL_FEATURES.md / README.md not updated)
P3: 2 (empty-state wording precision; package docstring overview)

P0_P1_P2_COUNT = 1. Continuing loop.

- SHELL_FEATURES.md / README.md: mention the new "Allowed paths" section in the help output and the empty-state notice (project rule requires docs to stay in sync). - builtins/help/help.go: refine empty-state wording from "all filesystem access blocked" to "no filesystem paths are reachable" since `ss`, `ip route`, `df`, and `ps` intentionally bypass AllowedPaths for kernel-state reads; expand the function and package doc comments. - tests/scenarios/cmd/help/{allowed_paths,no_allowed_paths}.yaml: use required `|+` block scalar for `expect.stderr` per AGENTS.md. - builtins/tests/help/help_test.go: update assertion to new wording.

AlexandreYang · 2026-05-13T15:35:50Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-Review (iteration 2)

Reviewed the cumulative PR diff at HEAD 39cc4470. All iteration-1 findings have been addressed:

P2 docs — SHELL_FEATURES.md and README.md now document the Allowed paths: section and empty-state notice. ✓
P3 empty-state wording — refined to "no filesystem paths are reachable" and the function comment now calls out the ss/ip route/df/ps kernel-state bypasses. ✓
P3 package docstring — overview now includes the new section. ✓
P3 (Codex) YAML block scalars — both scenario YAMLs use the required |+ form for expect.stderr. ✓

Overall assessment: safe to merge. No P0/P1/P2/P3 findings in this iteration.

Findings summary

None — clean iteration.

Coverage

Code path	Scenario test	Go test	Status
Configured paths render	`tests/scenarios/cmd/help/allowed_paths.yaml`	`help_test.go:411`	Covered
Multiple paths line-per-line	—	`help_test.go:421`	Covered
Empty AllowedPaths notice	`tests/scenarios/cmd/help/no_allowed_paths.yaml`	`help_test.go:432`	Covered
`r.sandbox == nil` branch	—	—	Implicit (observably identical to empty list)
Docs strings in `SHELL_FEATURES.md` / `README.md`	n/a (prose)	n/a	Updated

Verification

make fmt ✓
go build ./... ✓
go test ./builtins/tests/help/... ✓
go test ./tests/ -run TestShellScenarios ✓

AlexandreYang · 2026-05-13T15:37:22Z

Iteration 2 self-review result

P0: 0
P1: 0
P2: 0
P3: 0

P0_P1_P2_COUNT = 0. Clean iteration. Continuing loop to build success streak.

AlexandreYang · 2026-05-13T15:38:41Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-Review (iteration 3)

HEAD unchanged from iteration 2 (39cc4470). Re-confirmed all dimensions on the cumulative diff:

Security — no new file access; Sandbox.Paths() returns strings already exposed via $ALLOWED_PATHS. Nil-safe accessors. No injection, TOCTOU, or DoS surface introduced.
Bash compatibility — both new scenario YAMLs carry skip_assert_against_bash: true (correct; bash has no AllowedPaths concept).
Correctness — len(paths) == 0 empty-state branch handles both r.sandbox == nil and Sandbox{roots: []}.
Coverage — adequate (3 Go tests + 2 scenarios).
Code quality — naming consistent with existing printFeatureTable / printUnsupportedSummary siblings.
Platform — output is plain strings; filepath.Abs already produces OS-native paths in the sandbox layer.

Overall assessment: safe to merge. No P0/P1/P2/P3 findings.

Findings summary

None — clean iteration.

AlexandreYang · 2026-05-13T15:39:19Z

Iteration 3 self-review result — P0:0 P1:0 P2:0 P3:0. P0_P1_P2_COUNT = 0. Clean.

AlexandreYang · 2026-05-13T15:39:55Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-Review (iteration 4)

HEAD unchanged from iteration 3 (39cc4470). Re-confirmed all dimensions:

Security, bash compat, correctness, coverage, code quality, platform — all clean.

Overall assessment: safe to merge. No P0/P1/P2/P3 findings.

None — clean iteration.

AlexandreYang · 2026-05-13T15:40:18Z

Iteration 4 self-review result — P0:0 P1:0 P2:0 P3:0. P0_P1_P2_COUNT = 0. Clean.

AlexandreYang · 2026-05-13T15:40:48Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-Review (iteration 5)

HEAD unchanged from iteration 4 (39cc4470). Re-confirmed all dimensions across the cumulative diff:

Security: no new fs access, nil-safe accessors, no escape surface
Bash compat: scenarios correctly skip_assert_against_bash: true
Correctness: empty/nil paths both handled
Coverage: 3 Go tests + 2 scenarios cover all new code paths
Code quality: helper naming and call-site placement match siblings
Platform: filepath-native paths

Overall assessment: safe to merge. No P0/P1/P2/P3 findings.

None — clean iteration.

AlexandreYang · 2026-05-13T15:41:12Z

Iteration 5 self-review result — P0:0 P1:0 P2:0 P3:0. P0_P1_P2_COUNT = 0. Clean.

AlexandreYang · 2026-05-13T15:41:45Z

@codex review this PR

Please use the following severity format for all findings:

P0 (red): Exploitable vulnerability with high impact (RCE, sandbox bypass, data breach). Blocking merge.
P1 (orange): Likely exploitable or high-risk — correctness bugs vs bash, data races, panics.
P2 (yellow): Potential vulnerability, bash divergence, missing test coverage, missing docs.
P3 (blue): Style, minor simplification, hardening suggestion, nice-to-have test.

Prefix each finding title with its priority label, e.g. "P0: ...", "P1: ...", etc.
Include a summary table at the top with columns: # | Priority | File | Finding.

AlexandreYang

Self-Review (iteration 6)

HEAD unchanged from iteration 5 (39cc4470). Cumulative diff re-confirmed clean across security, bash compatibility, correctness, coverage, code quality, and platform compatibility.

Overall assessment: safe to merge. No P0/P1/P2/P3 findings.

None — clean iteration.

AlexandreYang · 2026-05-13T15:42:11Z

Iteration 6 self-review result — P0:0 P1:0 P2:0 P3:0. P0_P1_P2_COUNT = 0. Clean.

AlexandreYang · 2026-05-13T15:42:42Z

Review-Fix Loop Summary

PR: feat(help): list configured AllowedPaths in help output #248 (feat(help): list configured AllowedPaths in help output #248)
Iterations completed: 6
Final status: CLEAN
Stopped because: SUCCESS_COUNT reached 5 consecutive clean iterations

Iteration log

#	Unresolved threads	P0/P1/P2 findings	Fixes applied	CI status
1	3 (then resolved)	1 (P2 docs)	3 fixed	Passing/Pending
2	0	0	—	Passing/Pending
3	0	0	—	Passing/Pending
4	0	0	—	Passing/Pending
5	0	0	—	Passing/Pending
6	0	0	—	39 SUCCESS / 1 pending / 0 failing

Final state

Unresolved threads: 0 (from AlexandreYang, chatgpt-codex-connector, chatgpt-codex-connector[bot])
P0/P1/P2 findings: 0 (P3 findings are not blocking)
CI: 39 SUCCESS / 1 IN_PROGRESS / 0 FAILURE

Remaining issues

None.

chatgpt-codex-connector · 2026-05-13T15:45:05Z

Codex Review: Didn't find any major issues. Delightful!

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

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".

feat(help): list configured AllowedPaths in help output

153bb55

Show the active sandbox roots in `help`, with an explicit notice when none are configured so operators can tell that state apart from "all filesystem access allowed".

chatgpt-codex-connector Bot reviewed May 13, 2026

View reviewed changes

Comment thread builtins/help/help.go Outdated

Comment thread tests/scenarios/cmd/help/allowed_paths.yaml Outdated

AlexandreYang commented May 13, 2026

View reviewed changes

Comment thread builtins/help/help.go Outdated

AlexandreYang commented May 13, 2026

View reviewed changes

AlexandreYang marked this pull request as ready for review May 13, 2026 16:08

AlexandreYang requested review from astuyve, julesmcrt, matt-dz, thieman and val06 as code owners May 13, 2026 16:08

julesmcrt approved these changes May 15, 2026

View reviewed changes

AlexandreYang added this pull request to the merge queue May 15, 2026

Merged via the queue into main with commit 67e29cd May 15, 2026
40 checks passed

AlexandreYang deleted the alex/allowed-paths-in-help branch May 15, 2026 12:11

Uh oh!

Conversation

AlexandreYang commented May 13, 2026

Summary

Test plan

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-Review (iteration 1)

Findings summary

Coverage

Positive observations

Uh oh!

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-Review (iteration 2)

Findings summary

Coverage

Verification

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-Review (iteration 3)

Findings summary

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-Review (iteration 4)

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-Review (iteration 5)

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Self-Review (iteration 6)

Uh oh!

AlexandreYang commented May 13, 2026

Uh oh!

AlexandreYang commented May 13, 2026

Review-Fix Loop Summary

Iteration log

Final state

Remaining issues

Uh oh!

chatgpt-codex-connector Bot commented May 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels