feat(rm): implement rm builtin for remediation mode#556
Conversation
Adds a minimal, hardened `rm` builtin scoped to non-recursive file deletion: only -v/--verbose and -h/--help are supported, directories are never removed (even empty ones, consistently across platforms), symlinks are unlinked without following, and each invocation is capped at 10 file operands to bound the blast radius of a mistaken glob. Introduces Sandbox.Remove and CallContext.Remove, wired the same way as the existing truncate/logrotate remediation-mode capabilities. Adds scenario, unit, pentest, and fuzz coverage, and documents rm alongside truncate/logrotate in README.md and SHELL_FEATURES.md.
…l commit A failed pathspec in the prior `git add` silently aborted staging for sandbox.go, sandbox_test.go, rm.go, and the docs before the first commit landed, so it shipped the pre-fix Sandbox.Remove (which reused resolveWriteTarget and incorrectly followed the final path component, breaking removal of self-referential/escaping symlinks) instead of the resolveRemoveTarget fix, plus the errors.New idiom cleanup and the README/SHELL_FEATURES/fuzz.yml doc updates.
|
@codex 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: 14c08bca86
ℹ️ 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".
Sandbox.Remove previously ran a symlink-component precheck and an Lstat directory check, then called os.Root.Remove separately on the same path — leaving a race window where an intermediate component swapped for a symlink between the checks and the removal could redirect the delete into a different, more-restrictive AllowedPaths root. Add writeopen.Unlink, an atomic no-follow openat-walk-then-unlinkat on Unix (mirroring the existing write-target walker) and a Lstat-then-Remove call on Windows (which already gets atomicity from os.Root's O_NOFOLLOW_ANY), and wire Sandbox.Remove through it via a new (*root).removeFile method. Also fix rm's documentation (SHELL_FEATURES.md, README.md, CallContext.Remove's doc comment), which incorrectly claimed only regular files and symlinks are removable — FIFOs, sockets, and device nodes are also removable, since the implementation only excludes directories. Addresses automated review feedback on PR #556. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e crashes The byte-level filter (reject NUL/single-quote in the raw name) wasn't sufficient: some names combine with the appended closing quote in ways the shell lexer's line-continuation handling still rejects as unparseable (CI found "1:4: reached EOF without closing quote" on corpus entry be32d37903cefe74). Parse the actual constructed script with the real parser and skip anything it rejects, mirroring how the du fuzz test already handles this. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92ff7efcbf
ℹ️ 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 prior implementation did a separate Lstat then Remove; since Windows' os.Root.Remove can delete empty directories, a file-to-directory swap in that window could make rm delete a directory. Replace it with a single NtCreateFile (FILE_NON_DIRECTORY_FILE + FILE_OPEN_REPARSE_POINT) followed by NtSetInformationFile (FILE_DISPOSITION_DELETE) on the same handle, so the type check and the delete apply to the exact same open object. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c74163e41e
ℹ️ 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".
…ing them rm file/, rm symlink-to-dir/, and rm file/. all require their operand to resolve as a directory per POSIX/GNU semantics. filepath.Clean drops the trailing separator before the sandbox unlink path is reached, so rm was deleting the wrong target instead of failing with Not a directory / Is a directory like GNU/BSD rm. Detect the syntactic requirement in rm.go itself (before any Clean-based path handling) and re-check with a following Stat to disambiguate the two error cases. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestRmSymlinkToDirectoryTreatedAsSymlink was failing on windows-latest CI: NtCreateFile with FILE_NON_DIRECTORY_FILE rejects opening a symlink/junction whose target is a directory with STATUS_FILE_IS_A_DIRECTORY, because NTFS sets FILE_ATTRIBUTE_DIRECTORY on the reparse point itself, not just its target. That's a real directory check bypassing FILE_OPEN_REPARSE_POINT's "operate on the link, not what it points to" semantics. Drop FILE_NON_DIRECTORY_FILE from the create flags and instead check FileAttributes after open: a directory attribute without a reparse-point attribute is a genuine directory (rejected); a reparse point is always removable regardless of what it resolves to, matching Lstat-based directory detection on Unix. Also guard rm.go's pre-existing IsDir() directory check against symlinks, for the same underlying reason on the LstatFile path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e60e35d489
ℹ️ 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".
…ix backslash handling Unlink now rejects a syntactically-directory operand (trailing "/", or a final "." / "..") whose target isn't actually a directory, closing the gap where only rm.go's own precheck enforced this. Sandbox.Remove captures that intent on the raw path before toAbs's filepath.Join strips the trailing separator, and re-encodes it onto relPath so Unlink's check actually fires. Also fixes rm.go's hasTrailingDirSyntax treating "\" as a separator on all platforms — on Unix it's a valid filename character, so `rm 'foo\'` was incorrectly rejected as "Not a directory" instead of removing the file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b63dc790cc
ℹ️ 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".
…ore dereferencing on rm rm --verbose=false/--help=false previously parsed via pflag's BoolP and proceeded to delete files, diverging from GNU rm's no_argument longopt behavior. Reuse df's noArgBool pattern (now exported from flagparser) so an explicit value on --help/--verbose is rejected before any file is touched. Sandbox.Remove's trailing-dir-syntax handling forces Unlink to dereference a same-root symlink's final component via a raw Fstatat/NtCreateFile call that never revalidated the target against AllowedPaths. Resolve the full symlink chain and require it to land inside a configured root before Unlink is allowed to dereference it, closing a sandbox-boundary stat leak for `rm link/` where link points outside every AllowedPaths root. Also fixes rm.go's own trailing-slash precheck, which previously masked the real StatFile error (e.g. permission denied, dangling symlink) as a blanket "Not a directory". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 011c8156d2
ℹ️ 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".
removeFile only re-checked trailing-slash operands (file/, dangling symlink/, self-referential symlink/) via StatFile when the initial LstatFile call succeeded. LstatFile dereferences the trailing slash itself and fails with ENOENT/ELOOP for dangling and self-referential symlinks, so those cases fell through to a raw callCtx.Remove call that leaked Sandbox.Stat's internal error formatting instead of a clean GNU-style message. Decouple the hasTrailingDirSyntax check from LstatFile's error so the StatFile-based switch always runs for trailing-slash operands. Also fix Sandbox.Stat/Lstat to rebuild their error using the caller's op/path instead of os.Root's internal "statat"/sandbox-relative path, so the normalized error identity (fs.ErrNotExist, etc.) survives a second PortableErrMsg pass at the builtin layer. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1803ac4b6f
ℹ️ 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".
| // trailing-dir-syntax enforcement still sees it. | ||
| requiresDir := writeopen.HasTrailingDirSyntax(path) | ||
|
|
||
| absPath := toAbs(path, cwd) |
There was a problem hiding this comment.
When the operand contains a symlinked intermediate component followed by .., this toAbs call uses filepath.Join and lexically collapses the path before the sandbox has a chance to apply filesystem path-resolution semantics. For example, with link -> /outside/sub, rm link/../file should resolve through the symlink and be denied or target /outside/file (GNU rm does the latter), but here it becomes <cwd>/file and deletes that in-sandbox file instead. That makes remediation-mode rm remove a different file than the operand names whenever symlink/.. appears in the path.
Useful? React with 👍 / 👎.
1 participant
Summary
rmbuiltin for remediation mode: non-recursive file deletion only, with-v/--verboseand-h/--helpas the only accepted flags (-r/-R/--recursive,-f,-i/-I,-d,--preserve-root,--no-preserve-root,--one-file-systemare all rejected as unknown flags).unlinkatsilently succeeds on empty dirs on macOS); symlinks are unlinked without following their target; each invocation is capped at 10 file operands, checked before any file is removed.Sandbox.Remove/CallContext.Removecapabilities, wired the same way as the existingtruncate/logrotateremediation-mode builtins. Includes aresolveRemoveTargetfix so removal correctly followsunlink(2)semantics (never resolving the final path component) — caught via pentest fuzzing on self-referential and sandbox-escaping symlinks.FuzzRmFilename,FuzzRmOperandCount, wired into.github/workflows/fuzz.yml); README.md and SHELL_FEATURES.md updated to documentrmalongsidetruncate/logrotate.Test plan
go build ./...,go vet ./...,gofmt -l .all cleango test ./...passes across the full repogo test ./builtins/tests/rm/... -v(unit + pentest, 34 tests) passesgo test ./allowedpaths/... -run TestSandboxRemove -vpasses-fuzztime=20s) on both new fuzz targets found no crasheshelp/help rmoutput in both read-only and remediation modesRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s— not run locally (no Docker in this environment); please run in CI or locally before merging🤖 Generated with Claude Code