Fix absolute glob expansion in sandbox by AlexandreYang · Pull Request #268 · DataDog/rshell

AlexandreYang · 2026-05-22T10:31:06Z

What changed

Allow absolute glob expansion to traverse non-enumerated ancestors of configured AllowedPaths roots.
Keep out-of-sandbox absolute glob patterns literal so they do not disclose filenames.
Add regression coverage for absolute globs inside and outside the sandbox.

Why

The shell expander validates non-meta absolute path components with ReadDirForGlob before it reaches the directory containing the glob metacharacters. When AllowedPaths was set to a nested root, those ancestor reads were denied, so patterns such as /var/tmp/test-s-*-o stayed literal even though the target directory was allowed.

Validation

make fmt
go test ./interp -run 'TestAllowedPaths(AbsoluteGlobInside|AbsoluteGlobOutsideStaysLiteral|GlobInside|GlobOutside)$'
go test ./allowedpaths ./interp ./tests
go test ./...

AlexandreYang · 2026-05-22T10:32: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 · 2026-05-22T10:35:05Z

Codex Review: Didn't find any major issues. 👍

ℹ️ 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 · 2026-05-22T10:45:46Z

@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

Code Review — PR #268 (`alex/glob_bug`)

Assessment: safe to merge. The fix is correct and secure. One P3 (dead code) noted below.

What this PR does

Adds isAncestorOfRoot to the sandbox and uses it in readDirN to return (nil, nil) when the glob expander calls ReadDirForGlob on a directory that is an ancestor of an allowed root (rather than inside one). This is needed because mvdan.cc/sh's glob algorithm calls ReadDir2 on every path component — including non-meta ones — to verify they exist. Without the fix, a permission error on the ancestor component caused the entire absolute glob to stay unexpanded.

Security analysis

isAncestorOfRoot only returns true when the queried path is a genuine parent of an allowed root (not a sibling, not an unrelated path). Returning (nil, nil) exposes zero directory contents — the glob expander only uses the error value for non-meta components; the entries are discarded. The ancestor shortcut is gated behind maxEntries > 0, so it is exclusive to ReadDirForGlob and never reached by ReadDir.

All the path arithmetic goes through filepath.Clean / filepath.Rel, which neutralises .. traversal and cross-drive attacks on Windows.

Findings summary

#	Priority	File	Finding
1		`allowedpaths/sandbox.go:145`	Dead code: `rel == "."` unreachable after the equality guard

Positive observations

The maxEntries > 0 gate is exactly the right scoping — ancestor permissiveness is limited to glob expansion, not general ReadDir.
New scenario tests cover bracket ranges, negated classes, intermediate wildcards, nested patterns, and the outside-allowed-paths case. The outside case correctly sets skip_assert_against_bash: true.
filepath.Clean is applied to both sides before filepath.Rel, preventing any .. pollution in ancestor detection.

AlexandreYang · 2026-05-22T11:24:03Z

Iteration 1 self-review result: 0 P0, 0 P1, 0 P2, 1 P3 finding.

The fix is correct and secure. The single P3 finding is dead code ( check in is unreachable after the equality guard). No blocking issues.

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

@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

Review Summary

Reviewed the PR diff for absolute glob expansion under AllowedPaths, including allowedpaths/sandbox.go, the new Go regression tests, and the new scenario coverage.

Overall assessment: safe to merge.

#	Priority	File	Finding
—	—	—	No findings

Coverage Summary

Code path	Scenario test	Go test	Status
Absolute glob with non-meta ancestors and matches inside the allowed root	`tests/scenarios/shell/allowed_paths/absolute_glob_inside_allowed.yaml`	`interp/allowed_paths_test.go`	Covered
Absolute glob outside allowed roots stays literal without disclosing filenames	`tests/scenarios/shell/allowed_paths/absolute_glob_outside_allowed.yaml`	`interp/allowed_paths_test.go`	Covered
Bracket/range/negation absolute globs inside allowed roots	`tests/scenarios/shell/allowed_paths/absolute_glob_bracket_negation.yaml`	—	Covered
Multiple independent absolute glob patterns inside allowed roots	`tests/scenarios/shell/allowed_paths/absolute_glob_multiple_complex_patterns.yaml`	—	Covered
Nested mixed metacharacter absolute glob patterns inside allowed roots	`tests/scenarios/shell/allowed_paths/absolute_glob_nested_mixed_patterns.yaml`	—	Covered

Positive Observations

The new ReadDirForGlob behavior is scoped to glob expansion only (maxEntries > 0), leaving normal directory reads denied outside AllowedPaths.
Ancestor traversal returns no directory entries, so glob patterns cannot enumerate out-of-sandbox parent directories.
The test coverage checks both expansion inside the allowed tree and literal preservation outside it.

P0/P1/P2 findings: 0. P3 findings: 0.

AlexandreYang · 2026-05-22T12:22:31Z

Iteration 1 self-review result: P0=0, P1=0, P2=0, P3=0.

Summary: reviewed the absolute glob AllowedPaths diff, full changed implementation context, and the new Go/scenario coverage. No blocking findings were identified.

chatgpt-codex-connector · 2026-05-22T12:23:55Z

Codex Review: Didn't find any major issues. You're on a roll.

ℹ️ 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 · 2026-05-22T12:24:17Z

@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

Review Summary

Iteration 2 self-review of PR #268. The head SHA remains 82d1045ad10c10b4c2443307cd77efdbdaca3b13; the reviewed files are unchanged from iteration 1. I rechecked the sandbox ancestor traversal, glob expansion call path, and the absolute-glob scenario coverage.

Overall assessment: safe to merge.

#	Priority	File	Finding
—	—	—	No findings

Coverage Summary

Code path	Scenario test	Go test	Status
Absolute glob expansion inside an allowed nested root	yes	yes	Covered
Outside-allowed absolute glob remains literal	yes	yes	Covered
Bracket/range/negation and nested mixed absolute globs	yes	—	Covered

Verification run during this pass:

go test ./interp -run 'TestAllowedPaths(AbsoluteGlobInside|AbsoluteGlobOutsideStaysLiteral|GlobInside|GlobOutside)$'\n- go test ./tests -run 'TestShellScenarios/shell/allowed_paths/absolute_glob' -count=1\n\nP0/P1/P2 findings: 0. P3 findings: 0.

AlexandreYang · 2026-05-22T12:24:53Z

Iteration 2 self-review result: P0=0, P1=0, P2=0, P3=0.

Summary: re-reviewed the unchanged PR head and reran the focused absolute-glob Go/scenario tests. No blocking findings were identified.

AlexandreYang · 2026-05-22T12:26:06Z

@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

Review Summary

Iteration 3 self-review of PR #268. The PR head is still 82d1045ad10c10b4c2443307cd77efdbdaca3b13. I rechecked the same seven-file diff, with focus on the ReadDirForGlob ancestor shortcut, lack of directory-entry disclosure, bash-compatible sorting/expansion behavior, and coverage for inside/outside AllowedPaths.

Overall assessment: safe to merge.

#	Priority	File	Finding
—	—	—	No findings

Verification run during this pass:

go test ./interp -run 'TestAllowedPaths(AbsoluteGlobInside|AbsoluteGlobOutsideStaysLiteral|GlobInside|GlobOutside)$'\n- go test ./tests -run 'TestShellScenarios/shell/allowed_paths/absolute_glob' -count=1\n\nP0/P1/P2 findings: 0. P3 findings: 0.

AlexandreYang · 2026-05-22T12:26:36Z

Iteration 3 self-review result: P0=0, P1=0, P2=0, P3=0.

Summary: re-reviewed the unchanged seven-file diff and reran focused absolute-glob tests. No blocking findings were identified.

AlexandreYang · 2026-05-22T12:27:44Z

@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

Review Summary

Iteration 4 self-review of PR #268. The PR head remains 82d1045ad10c10b4c2443307cd77efdbdaca3b13, and the changed file set is unchanged. I rechecked the sandbox/glob interaction and the regression coverage again.

Overall assessment: safe to merge.

#	Priority	File	Finding
—	—	—	No findings

Verification run during this pass:

go test ./interp -run 'TestAllowedPaths(AbsoluteGlobInside|AbsoluteGlobOutsideStaysLiteral|GlobInside|GlobOutside)$'\n- go test ./tests -run 'TestShellScenarios/shell/allowed_paths/absolute_glob' -count=1\n\nP0/P1/P2 findings: 0. P3 findings: 0.

AlexandreYang · 2026-05-22T12:28:17Z

Iteration 4 self-review result: P0=0, P1=0, P2=0, P3=0.

Summary: re-reviewed the unchanged PR head and reran focused absolute-glob tests. No blocking findings were identified.

AlexandreYang · 2026-05-22T12:29:34Z

@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

Review Summary

Iteration 5 self-review of PR #268. The PR head remains 82d1045ad10c10b4c2443307cd77efdbdaca3b13. I rechecked the seven-file diff, the sandbox/glob call path, and the coverage for absolute globs inside and outside AllowedPaths.

Overall assessment: safe to merge.

#	Priority	File	Finding
—	—	—	No findings

Verification run during this pass:

go test ./interp -run 'TestAllowedPaths(AbsoluteGlobInside|AbsoluteGlobOutsideStaysLiteral|GlobInside|GlobOutside)$'\n- go test ./tests -run 'TestShellScenarios/shell/allowed_paths/absolute_glob' -count=1\n\nP0/P1/P2 findings: 0. P3 findings: 0.

AlexandreYang · 2026-05-22T12:30:07Z

Iteration 5 self-review result: P0=0, P1=0, P2=0, P3=0.

Summary: final self-review pass on the unchanged PR head, with focused absolute-glob tests passing. No blocking findings were identified.

chatgpt-codex-connector · 2026-05-22T12:31:33Z

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

ℹ️ 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 · 2026-05-22T12:32:11Z

Review-Fix Loop Summary

PR: Fix absolute glob expansion in sandbox #268 (Fix absolute glob expansion in sandbox #268)
Iterations completed: 5
Final status: CLEAN
Stopped because: SUCCESS_COUNT reached 5 consecutive clean iterations

Iteration log

#	Fixes applied	CI status
1	—	Passing (no failing checks; `devflow/mergegate` pending)
2	—	Passing (no failing checks; `devflow/mergegate` pending)
3	—	Passing (no failing checks; `devflow/mergegate` pending)
4	—	Passing (no failing checks; `devflow/mergegate` pending)
5	—	Passing (no failing checks; `devflow/mergegate` pending)

Final state

Unresolved threads: 0 (authors: none)
P0/P1/P2 findings: 0 (P3 findings are not blocking)
CI: Passing (no failing checks; devflow/mergegate pending)

julesmcrt · 2026-05-22T13:17:34Z

Heads up — one case this PR doesn't cover: when a glob metacharacter appears in an ancestor of the allowed root.

Example: with --allowed-paths /var/tmp and pattern /var/*/test-s-*-o, the result stays literal even though /var/tmp/test-s-1-o is accessible.

Why: ReadDirForGlob("/var") returns (nil, nil) (empty listing). When globDir evaluates * against /var, it iterates zero entries, so newMatches stays empty and the pattern falls through as literal.

How to make it work: return synthetic directory entries for the next-level path components leading to each contained root, instead of an empty listing. With --allowed-paths /var/tmp:

ReadDirForGlob("/") → [{name: "var", IsDir: true}]
ReadDirForGlob("/var") → [{name: "tmp", IsDir: true}]
ReadDirForGlob("/var/tmp") → real listing (already handled)

Then /var/*/test-s-*-o expands against the synthetic entries and reaches the real listing at /var/tmp.

The trade-off: this discloses the names of allowed roots through ancestor globbing — minor, since the operator configured them, but the current empty-listing choice has a real privacy benefit. Happy to keep the current scope; if so, a scenario test asserting literal fallthrough for `/var//test-s--o` would lock in the intentional limitation.

julesmcrt

Creating a Feature Request for #268 (comment)

### What does this PR do? Bumps `github.com/DataDog/rshell` from `v0.0.18` to `v0.0.20` and updates the corresponding `go.sum` entries. Adds focused `runCommand` coverage that pins stdout and stderr output-limit sentinels as action-level errors instead of normal `RunCommandOutputs`, since the output schema has no truncation marker. ### Motivation Pick up the latest `rshell` release in the Agent dependency set. Mainly for this security fix: DataDog/rshell#268 ### Describe how you validated your changes - `dda inv test --targets=./pkg/privateactionrunner/bundles/remoteaction/rshell --extra-args="-run ^TestRunCommandOutputLimitsReturnActionErrors$ -count=1 -timeout=30s"` - `git diff --check` ### Additional Notes The output-limit test documents the intentional integration contract: partial stdout/stderr from rshell output caps is not returned as a normal command result without an explicit truncation field. Co-authored-by: alexandre.yang <[email protected]>

Backport of #51247 --- Bumps `github.com/DataDog/rshell` from `v0.0.18` to `v0.0.20` and updates the corresponding `go.sum` entries. Adds focused `runCommand` coverage that pins stdout and stderr output-limit sentinels as action-level errors instead of normal `RunCommandOutputs`, since the output schema has no truncation marker. Pick up the latest `rshell` release in the Agent dependency set. Mainly for this security fix: DataDog/rshell#268 - `dda inv test --targets=./pkg/privateactionrunner/bundles/remoteaction/rshell --extra-args="-run ^TestRunCommandOutputLimitsReturnActionErrors$ -count=1 -timeout=30s"` - `git diff --check` The output-limit test documents the intentional integration contract: partial stdout/stderr from rshell output caps is not returned as a normal command result without an explicit truncation field. (cherry picked from commit 9101702) ### What does this PR do? ### Motivation ### Describe how you validated your changes ### Additional Notes Co-authored-by: florent.clarret <[email protected]>

fix absolute glob expansion in sandbox

e26a526

AlexandreYang marked this pull request as ready for review May 22, 2026 10:31

AlexandreYang requested review from astuyve, julesmcrt, matt-dz, thieman and val06 as code owners May 22, 2026 10:31

add absolute glob scenario coverage

1f2d39b

AlexandreYang marked this pull request as draft May 22, 2026 10:35

AlexandreYang changed the title ~~[codex] fix absolute glob expansion in sandbox~~ Fix absolute glob expansion in sandbox May 22, 2026

AlexandreYang marked this pull request as ready for review May 22, 2026 10:36

add complex absolute glob scenarios

30d974e

AlexandreYang marked this pull request as draft May 22, 2026 10:46

AlexandreYang marked this pull request as ready for review May 22, 2026 10:49

AlexandreYang commented May 22, 2026

View reviewed changes

Comment thread allowedpaths/sandbox.go Outdated

[iter 1] Remove unreachable rel == "." check in isAncestorOfRoot

82d1045

AlexandreYang commented May 22, 2026

View reviewed changes

julesmcrt approved these changes May 22, 2026

View reviewed changes

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

Merged via the queue into main with commit 3565456 May 22, 2026
40 checks passed

AlexandreYang deleted the alex/glob_bug branch May 22, 2026 13:29

julesmcrt mentioned this pull request May 22, 2026

[Nice-to-have, may never be done] Support glob metacharacters in ancestors of allowed paths #270

Open

AlexandreYang mentioned this pull request May 22, 2026

Bump rshell to v0.0.20 DataDog/datadog-agent#51247

Merged

AlexandreYang mentioned this pull request May 26, 2026

Bump rshell to v0.0.20 (#51247) DataDog/datadog-agent#51304

Merged

Uh oh!

Conversation

AlexandreYang commented May 22, 2026

What changed

Why

Validation

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 22, 2026

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Code Review — PR #268 (alex/glob_bug)

What this PR does

Security analysis

Findings summary

Positive observations

Uh oh!

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Review Summary

Coverage Summary

Positive Observations

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 22, 2026

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Review Summary

Coverage Summary

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

AlexandreYang left a comment

Choose a reason for hiding this comment

Review Summary

Uh oh!

AlexandreYang commented May 22, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 22, 2026

Uh oh!

AlexandreYang commented May 22, 2026

Review-Fix Loop Summary

Iteration log

Final state

Uh oh!

julesmcrt commented May 22, 2026

Uh oh!

julesmcrt left a comment

Choose a reason for hiding this comment

Code Review — PR #268 (`alex/glob_bug`)