feat(find): implement -execdir command {} ;#125
Conversation
|
@codex conduct a comprehensive security and code review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 734d67b7d5
ℹ️ 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".
|
@codex conduct a comprehensive security and code review |
Add -execdir predicate to find, allowing execution of allowed builtin
commands in each matched file's parent directory with ./basename as
the argument. This is the safer alternative to -exec (no TOCTOU on the
path, no path traversal in arguments) and -exec remains blocked.
Key design decisions:
- \; mode only — + (batch) mode rejected with clear error
- AllowedCommands enforced at parse-time AND eval-time (defense-in-depth)
- Sub-commands run through the interpreter with the same sandbox/AllowedPaths
- No shell re-parsing — {} replacement produces discrete Go string args
- ./basename prefix prevents argument injection via dash-prefixed filenames
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
filepath.Join cleans its result, so Join(cwd, ".") = cwd and Dir(cwd) points one level above the search root. Replace with joinPath (simple concatenation) so "." is preserved as the last component and filepath.Dir correctly strips it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
In find syntax, '+' is only a batch-mode terminator when it
immediately follows '{}'. Previously the parser rejected any '+'
token, blocking valid commands like -execdir echo + {} \;. Now '+'
is only rejected in the {} + position.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
matt-dz
force-pushed
the
matt-dz/implement-find-exec
branch
from
3c20b8a to
6a80911
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c20b8ad59
ℹ️ 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".
…exts The child CallContext built by RunCommand was missing RunCommand and WorkDir, so builtins invoked via -execdir that themselves need sub-command execution (e.g. nested find) would fail with "command execution not available". Extract RunCommand into a named variable so the child can reference it recursively. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
filepath.Dir("/cwd/dir/") returns "/cwd/dir" (the directory itself),
not "/cwd" (its parent). Trim trailing slashes before taking Dir so
-execdir runs in the correct parent directory for paths like "dir/".
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Take main's emoji-annotated comment style and add our -execdir symbols (path/filepath.Dir, path/filepath.IsAbs, strings.Contains) to both the find per-command list and the global allowed list. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 342c0e407e
ℹ️ 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".
baseName("/") returns "/", so "./" + "/" produced ".//". Special-case
the root entry to pass "/" directly, matching GNU find behavior.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9343bd191
ℹ️ 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".
The trailing-slash trim converted C:/ to C:, making filepath.Dir return the drive-relative C:. instead of the drive root. Skip the trim when the slash immediately follows a colon (drive separator). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb857ae78f
ℹ️ 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".
baseName strips trailing slashes, so dir/ became ./dir instead of ./dir/. Re-append the slash when the original relPath ends with /, matching GNU find and preserving slash-sensitive sub-command semantics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the parser gate that rejected arguments containing {} unless
they were exactly {}. Now {}.bak, prefix-{}-suffix, etc. are accepted
and replaced via strings.ReplaceAll, matching GNU find behavior.
This is safe because arguments are discrete Go strings — no shell
re-parsing occurs.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
- P1: rename shadowed fn → cmdFn inside runCmd closure to avoid masking the outer builtins.Lookup result (variable shadowing) - P2: remove redundant strings.Contains guard before ReplaceAll; drop strings.Contains from find's allowed symbols - P3: use accumulator pattern in collectExecDirCmds to avoid intermediate slice allocations from append-spread Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b9dd6d4d0
ℹ️ 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".
The trailing-slash guard matched any path ending in :/ (e.g. /tmp/d:/), not just Windows drive roots like C:/. Tighten to len == 3 && [1] == ':' so only X:/ is preserved. Extract the trim+Dir logic into execDirParentDir for direct unit testing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex conduct a comprehensive security and code review |
|
Codex Review: Didn't find any major issues. Another round soon, please! ℹ️ 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". |
matt-dz
requested review from
AlexandreYang,
astuyve,
julesmcrt and
thieman
as code owners
filepath.Dir returns backslashes on Windows. Apply filepath.ToSlash to the result before comparing against forward-slash expectations, matching the shell's path normalization convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
### What does this PR do? See changes: - https://github.com/DataDog/rshell/releases/tag/v0.0.7 - https://github.com/DataDog/rshell/releases/tag/v0.0.8 Besides the minor fixes to improve security and memory usage, we have those changes to rshell commands: - feat(find): implement -execdir command {} ; by @matt-dz in DataDog/rshell#125 - feat(find): implement -exec command {} ; by @matt-dz in DataDog/rshell#133 - feat(ip): add ip route show/get builtin (Linux only) by @AlexandreYang in DataDog/rshell#135 ### Testing ✅ Tested e2e with local agent ### Describe how you validated your changes Test rshell works e2e ### Additional Notes Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com>
See changes: - https://github.com/DataDog/rshell/releases/tag/v0.0.7 - https://github.com/DataDog/rshell/releases/tag/v0.0.8 Besides the minor fixes to improve security and memory usage, we have those changes to rshell commands: - feat(find): implement -execdir command {} ; by @matt-dz in DataDog/rshell#125 - feat(find): implement -exec command {} ; by @matt-dz in DataDog/rshell#133 - feat(ip): add ip route show/get builtin (Linux only) by @AlexandreYang in DataDog/rshell#135 ✅ Tested e2e with local agent Test rshell works e2e Co-authored-by: alexandre.yang <alexandre.yang@datadoghq.com>
2 participants
Summary
Implements
find -execdir CMD [ARG]... \;— the safer alternative to-execthat runs commands in each matched file's parent directory, passing./basenameas the argument.\;mode only —+(batch) mode is deferred and rejected with a clear error message-execstays blocked —-execdiris strictly safer (no TOCTOU on the path, no path traversal in arguments);-execcan be unblocked as a follow-upAllowedCommandsenforced — checked at both parse-time validation AND evaluation-time (defense-in-depth)AllowedPathsrestrictions{}replacement produces discrete Go string arguments; metacharacters in filenames are inert bytes, never re-parsed as shell commands./prefix — prevents argument injection via dash-prefixed filenames (e.g. file named-rfbecomes./-rf)Changes
builtins/builtins.goWorkDirandRunCommandtoCallContextinterp/runner_exec.goRunCommand— executes builtins through sandbox with custom working directorybuiltins/find/expr.goexprExecDirkind,parseExecDirPredicate(), removed-execdirfrom blocked listbuiltins/find/eval.goevalExecDir()—{}→./basenamereplacement, double-checksCommandAllowedbuiltins/find/find.goexecDirParentcomputation, post-parse command validation, help/doc updatesallowedsymbols/symbols_builtins.gostrings.Contains,filepath.{Dir,IsAbs,Join}to find's allowed symbolsSHELL_FEATURES.mdTests
tests/scenarios/cmd/find/execdir/) — basic usage, action suppression, predicate interactions (AND/OR/NOT/quit/prune), error casesbuiltin_find_pentest_test.go) — CWE-78 command injection via filenames (8 variants), CWE-88 argument injection, CWE-426 PATH injection, CWE-284 access control, GTFOBins validationexpr_test.goTest plan
gofmt -l .— cleango build ./...— buildsGOOS=windows go build ./...— cross-compilesgo test ./builtins/find/ -v -count=1— all passgo test ./allowedsymbols/ -v -count=1— all passgo test ./tests/ -run TestShellScenarios$ -count=1 -timeout 120s— all passgo test ./... -count=1 -timeout 120s— all passRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s— requires Docker🤖 Generated with Claude Code