[codex] add remediation commands and output redirects#250
Conversation
|
@codex review Please review this PR for correctness, security regressions around |
![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: 3336dc7d7a
ℹ️ 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 review I pushed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fef9cff228
ℹ️ 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 review I pushed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1afa5698c
ℹ️ 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 review I pushed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ec31ec740
ℹ️ 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".
| callCtx.Errf("tee: file write is not available\n") | ||
| return builtins.Result{Code: 1} | ||
| } | ||
| f, err := callCtx.OpenFileForWrite(ctx, args[0], *appendFlag) |
There was a problem hiding this comment.
When HostCommandHandler is unset and the default exec handler rejects host execution, tee out still reaches this OpenFileForWrite call before InvokeHostCommandWithFiles discovers that no host command can run. In the non-append case that open uses O_TRUNC|O_CREATE, so an allowed target is created or emptied even though the command exits 127 and no host tee ran; for example, with only rshell:tee allowed and no host handler, tee out reports rshell: tee: unknown command while truncating out to zero bytes. Check host execution availability before opening/truncating the target, or defer the destructive open until the host command path is known to be executable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ebb8741e by tracking whether a host execution path was explicitly configured and having tee check that before opening its write target. When no HostCommandHandler/ExecHandler is configured, tee now returns through the rejecting host path without creating or truncating the target. Added regression coverage for both existing and missing target files.
Validation:
make fmtgo test ./interp -run TestRemediationgo test ./...RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review I pushed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ebb8741edc
ℹ️ 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".
| return nil, &os.PathError{Op: "open", Path: path, Err: os.ErrPermission} | ||
| } | ||
|
|
||
| f, err := ar.root.OpenFile(relPath, flag, perm) |
There was a problem hiding this comment.
docs/RULES.md says builtins must not follow symlinks during write operations, but this write path opens the user-supplied target with os.Root.OpenFile, which follows symlinks that remain inside an allowed root. In a writable allowed directory, tee link or > link can therefore truncate/append to a different allowed file selected by a symlink, and OpenExistingForWrite below has the same issue for truncate/logrotate; use a no-follow check/open before any write-capable fd is created.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 263fcfc6 by rejecting symlink components before creating any write-capable sandbox fd. On Unix the final write open also uses O_NOFOLLOW, with ELOOP normalized to a permission denial for terminal symlink races. This covers stdout redirection plus the fd handoff used by tee, truncate, and logrotate.
Validation:
make fmtgo test ./allowedpathsgo test ./interp -run TestRemediationgo test ./interp/tests -run TestRedirStdoutToFileRejectsSymlinkTargetgo test ./analysis -run 'TestAllowedPathsAllowedSymbols|TestVerificationAllowedpathsCleanPass'go test ./...RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review I pushed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 263fcfc645
ℹ️ 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".
| return nil, err | ||
| } | ||
|
|
||
| f, err := ar.openFileNoFollow(relPath, flag, perm) |
There was a problem hiding this comment.
When an allowed-path write target already exists as a FIFO or another special file, this path falls through the symlink-only check and then opens it with O_WRONLY; on Unix, opening a FIFO for write blocks until a reader appears, so echo x > fifo, tee fifo, or truncate/logrotate fifo can hang the interpreter before the host command or context timeout can run. The repo rules require dangerous builtins to reject special files by default, so validate the final component with Lstat/mode checks (allow only regular files, or a missing final component for creates) before this blocking open.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5ebd2aff by extending the sandbox write-target validation to reject any existing final component that is not a regular file before creating an O_WRONLY fd. Missing final components are still allowed for create/truncate redirects, but FIFOs and other special files now fail with permission denied without blocking. Added timeout-backed regression coverage for redirects plus tee, truncate, and logrotate.
Validation:
make fmtgo test ./allowedpaths -run 'TestOpen.*FIFO|TestOpen.*Symlink'go test ./interp -run 'TestRemediation.*FIFO|TestRemediation.*SymlinkTarget'go test ./interp/tests -run 'TestRedirStdoutToFileRejects.*'go test ./...RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review I pushed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ebd2aff8e
ℹ️ 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".
| absPath := toAbs(path, cwd) | ||
|
|
||
| ar, relPath, ok := s.resolve(absPath) |
There was a problem hiding this comment.
When a write target ends in a path separator, this new write path normalizes it before validation/opening, so echo data > foo/ inside an allowed directory silently creates or truncates the regular file foo instead of failing as bash/open(2) do for a directory-qualified target. The same normalization lets guarded mutations such as truncate -s 0 foo/ validate and delegate an fd for foo, so a script can mutate a file even though the supplied operand should be invalid; preserve and reject trailing separators before calling toAbs/resolve for write targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2488f5d6 by rejecting trailing path separators in OpenForWrite and OpenExistingForWrite before toAbs/filepath.Join can normalize them away. This prevents redirects and fd-backed guarded host mutations from treating foo/ as foo. Added sandbox tests, redirect coverage, remediation command coverage for truncate, tee, and logrotate, plus a bash-compared scenario that verifies the file remains unchanged.
Validation:
make fmtgo test ./allowedpaths -run TrailingSeparatorgo test ./interp -run 'Remediation.*TrailingSeparator'go test ./interp/tests -run RedirStdoutToFileRejectsTrailingSeparatorgo test ./tests -run TestShellScenarios/shell/blocked_redirects/trailing_separator_targetgo test ./analysis -run 'TestAllowedPathsAllowedSymbols|TestVerificationAllowedpathsCleanPass'go test ./...RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review I pushed |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2488f5d6b6
ℹ️ 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".
| // Output redirection is supported for stdout (default or fd 1). | ||
| // Stderr file redirection remains blocked except for /dev/null. | ||
| if rd.N == nil || rd.N.Value == "1" || redirectTargetIsDevNull(rd) { | ||
| return nil |
There was a problem hiding this comment.
When a script uses cmd >log 2>&1 or cmd >>log 2>&1, this newly allowed stdout file redirection opens log, and the existing DplOut runtime path then assigns stderr to the current stdout writer. That bypasses the explicit policy that 2> FILE, &> FILE, and &>> FILE remain blocked, so any stderr output is written to an AllowedPaths file despite the validation comments and feature docs saying stderr file redirection is unsupported except for /dev/null.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1555d3a0 by tracking whether stdout/stderr currently point at file-backed redirects and rejecting 2>&1/2>&2 when the target fd is file-backed. /dev/null fd duplication still works, and left-to-right 2>&1 >file still sends stderr to the original stdout rather than the file. Added Go coverage plus an rshell-specific scenario for the blocked >file 2>&1 case.
Validation:
make fmtgo test ./interp/tests -run 'RedirDupStderrToStdout|RedirDupStderrToStdoutFileRejected|RedirDupStderrToOriginalStdoutBeforeFileRedirect'go test ./tests -run TestShellScenarios/shell/blocked_redirects/stderr_dup_to_stdout_filego test ./...RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
| func HostExtraFilePath(index int) string { | ||
| return fmt.Sprintf("/dev/fd/%d", hostExtraFileBaseFD+index) | ||
| } |
There was a problem hiding this comment.
For Windows runs of the guarded file-mutating builtins, this hard-coded /dev/fd/N operand cannot be used by a normal host-command handler: go doc os/exec.Cmd.ExtraFiles says ExtraFiles is not supported on Windows, and Windows has no /dev/fd/3 path for the delegated tee, truncate, or logrotate process to open. Since the project supports Windows, these builtins either need a Windows-specific handoff mechanism or should reject/avoid this delegation path on Windows instead of passing an unusable Unix path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 1555d3a0 by adding builtins.HostExtraFilesSupported() and guarding the fd-backed host handoff before opening write targets. On Windows, tee, truncate, and logrotate now reject with a clear error instead of passing /dev/fd/N; InvokeHostCommandWithFiles also has a defensive platform check. Added Windows-only regression tests to verify the guarded builtins reject before opening/mutating their targets.
Validation:
make fmtgo test ./builtinsgo test ./interp -run TestRemediationgo test ./...RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review I pushed |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
|
@codex review I pushed Validation:
Please re-review for any remaining correctness/security/test coverage issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2da2636cc2
ℹ️ 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".
| expandedArg = r.literal(targetState.source.Word) | ||
| redirectArgs[targetState.source] = expandedArg |
There was a problem hiding this comment.
When a later stdout redirect is the source for 2>&1, this preflight expands that redirect word immediately, before earlier redirect words are processed. For example, echo ok > "$(printf first >&2; printf first.txt)" > "$(printf second >&2; printf /dev/null)" 2>&1 now prints secondfirst, while bash expands redirections left-to-right and prints firstsecond; if those substitutions mutate files, the side effects also happen in the wrong order. The preflight needs to avoid executing a later redirect substitution ahead of earlier redirects, or otherwise preserve left-to-right redirect-word expansion while still rejecting unsupported fd-dup cases before opening file targets.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d8436fd9 by expanding dynamic output redirect targets at their own position during fd-dup preflight instead of lazily expanding the target when 2>&1 is encountered. That preserves left-to-right redirect-word expansion order while still caching the expanded values so unsupported stderr-to-file fd duplication can be rejected before opening file targets.
Added regression coverage for echo ok > "$(printf first >&2; printf first.txt)" > "$(printf second >&2; printf /dev/null)" 2>&1, which now emits firstsecond and keeps the first redirect target empty.
Validation:
make fmtgo test ./interp/tests -run 'TestRedirDupStderrTo(DynamicDevNullPreservesRedirectExpansionOrder|CommandSubstitutedDevNullWithCommandWordExpansion|DynamicDevNullWithCommandWordExpansion|DynamicStdoutFileRejectedBeforeCommandWordExpansion|StdoutFileRejectedBeforeCommandWordExpansion)'go test ./...RSHELL_COMPLIANCE_TEST=1 go test -run TestCompliance ./tests/RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review I pushed Validation:
Please re-review for any remaining correctness/security/test coverage issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8436fd973
ℹ️ 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".
| if r.exit.ok() { | ||
| var err error | ||
| redirectArgs, err = r.preflightFileBackedFdDupRedirects(st.Redirs) | ||
| if err != nil { |
There was a problem hiding this comment.
When an earlier output redirect cannot be opened (for example it is outside AllowedPaths or has a missing parent), this full preflight still expands later redirect targets before any redirect is applied. That lets side-effecting substitutions in later targets run even though bash stops at the first failed redirect; e.g. with only the working dir allowed, echo hi > /no/such/out > "$(printf side | write_file side.txt >/dev/null; printf out2)" creates side.txt before reporting the first redirect error. The safety preflight needs to avoid running command substitutions that are after a redirect which would have failed to open.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 28593f3b by narrowing fd-dup preflight expansion to the redirects that appear before the last >& duplication, and by stopping that preflight when an earlier output redirect is already known to fail before open. The actual redirect loop still performs the authoritative sandboxed open and reports the first redirect error; the preflight now avoids expanding later redirect words in that case.
Added regression coverage for both the no-fd-dup case and a variable-backed failed redirect before 2>&1, verifying that the later command substitution does not create side.txt or out2.
Validation:
make fmtgo test ./interp/tests -run 'TestRedir(PreflightPreservesFailedEarlierRedirectShortCircuit|DupStderrToDynamicDevNullPreservesRedirectExpansionOrder|DupStderrToCommandSubstitutedDevNullWithCommandWordExpansion|DupStderrToDynamicStdoutFileRejectedBeforeCommandWordExpansion)'go test ./...RSHELL_COMPLIANCE_TEST=1 go test -run TestCompliance ./tests/RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review I pushed Validation:
Please re-review for any remaining correctness/security/test coverage issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28593f3bd5
ℹ️ 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".
| info, err := r.sandbox.Lstat(arg, dir) | ||
| if err == nil { | ||
| if info.Mode()&fs.ModeSymlink != 0 || info.IsDir() || !info.Mode().IsRegular() { | ||
| return true | ||
| } | ||
| return r.sandbox.Access(arg, dir, preflightAccessWrite) != nil |
There was a problem hiding this comment.
When an earlier stdout redirect targets an existing file through a symlinked directory, Lstat(arg) follows the intermediate symlink and reports the final regular file as writable, so the fd-dup preflight keeps going even though the real OpenForWrite later rejects that symlink component. For example, with link -> real and real/out.txt present, echo hi > link/out.txt > "$(printf side | write_file side.txt >/dev/null; printf out2)" 2>&1 expands the second redirect and creates side.txt before failing with the fd-dup error; the authoritative redirect loop would have failed on link/out.txt first and never expanded the later word. The preflight needs to mirror the no-follow parent validation used by the real open before deciding an earlier redirect cannot short-circuit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 62e1f53b by adding a no-open redirect preflight path validator that mirrors the write-open no-follow parent checks before deciding an earlier redirect can keep preflight expansion moving. The later redirect word is no longer expanded when an earlier redirect would fail because a parent component is a symlink.
I also folded in the requested write-disable option: DisableFileWrites() / --disable-file-writes blocks real file writes from redirects and removes write-open capabilities from write_file, tee, truncate, and logrotate, while still allowing /dev/null redirects.
Validation:
make fmtgo test ./cmd/rshell -run TestDisableFileWritesgo test ./interp/... -run TestDisableFileWritesgo test ./...RSHELL_COMPLIANCE_TEST=1 go test -run TestCompliance ./tests/RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review I pushed This update also includes the requested file-write kill switch: Validation:
Please re-review for any remaining correctness/security/test coverage issues. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@codex review I pushed Validation:
Please re-review for any remaining correctness/security/test coverage issues. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65b8ed4f54
ℹ️ 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".
| if timedOut { | ||
| callCtx.Errf("kill: timed out waiting for pid %d to exit\n", pid) | ||
| } | ||
| return builtins.Result{} |
There was a problem hiding this comment.
When the target process ignores SIGTERM or remains visible until its parent reaps it, waitForExit returns timedOut=true; this branch prints kill: timed out... but still returns a zero status, so remediation callers can treat an unconfirmed kill as successful. The JSON path has the same effect because timed_out can be true while exit_code remains 0, so please make timeout propagate as a non-zero result/receipt.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 00f123ecce1ea1c2284abe3f1740ee848ffd878a: kill --timeout now returns a non-zero result on timeout, and JSON receipts set exit_code: 1 with the timeout stderr while keeping timed_out: true.
Added Unix regression coverage for both plain and --json timeout paths using a helper process that ignores SIGTERM.
Validated:
make fmtgo test ./...RSHELL_COMPLIANCE_TEST=1 go test -run TestCompliance ./tests/RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review Pushed Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00f123ecce
ℹ️ 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".
| return false, nil | ||
| } | ||
| defer proc.Release() | ||
| return true, nil |
There was a problem hiding this comment.
On Windows, when kill is used with the default non-zero timeout, this liveness check treats any PID that os.FindProcess can open as still alive and never checks whether the process handle is already signaled. For processes whose object is still open after TerminateProcess succeeds (for example a child whose parent has not waited yet), waitForExit will keep seeing true until the timeout and report a failed kill even though the target has exited; query the handle state (e.g. a zero-time wait) instead of returning true from FindProcess alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ec8487e81d15831844538acc135fbf7266d65f31: Windows pidAlive now opens the process with SYNCHRONIZE and uses a zero-time WaitForSingleObject to distinguish an exited/signaled process handle from a still-running one.
I also found the same exited-but-unreaped shape locally on Unix, where signal(0) can continue to succeed for zombies. The Unix path now treats procinfo state Z as exited, and I added a default-timeout regression that confirms kill PID succeeds after terminating a helper process whose parent has not waited yet.
Validated:
make fmtgo test ./interp -run TestRemediationKill -count=1go test ./analysis -count=1env GOTOOLCHAIN=local GOOS=windows GOARCH=amd64 go test -c -o /private/tmp/rshell-kill-windows.test.exe ./builtins/killgo test ./...RSHELL_COMPLIANCE_TEST=1 go test -run TestCompliance ./tests/RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review Pushed Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec8487e81d
ℹ️ 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".
| infos, infoErr := procinfo.GetByPIDs(ctx, "", []int{pid}) | ||
| if infoErr == nil { | ||
| if len(infos) == 0 || infos[0].State == "Z" { | ||
| return false, nil |
There was a problem hiding this comment.
When the kill wait context is already canceled (for example the runner timeout fires between sending the signal and this liveness poll), procinfo.GetByPIDs returns an empty slice with a nil error because it stops on ctx.Err(). This branch then treats that as the process having exited, so kill --timeout ... can report success and skip the shell cancellation/timeout path even though the target is still alive; check ctx.Err() before interpreting an empty procinfo result as exit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 453f2a96: Unix pidAlive now preserves cancellation by treating an empty procinfo result from a canceled context as still alive, so waitForExit reaches the runner cancellation path instead of reporting success.
Added TestPIDAlivePreservesCanceledContext to cover the canceled-context liveness edge.
Validated:
make fmtgo test ./builtins/kill -count=1go test ./interp -run TestRemediationKill -count=1go test ./...RSHELL_COMPLIANCE_TEST=1 go test -run TestCompliance ./tests/RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s
|
@codex review Pushed Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 453f2a96b0
ℹ️ 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".
| func (r *root) openFileNoFollow(rel string, flag int, perm os.FileMode) (*os.File, error) { | ||
| // Keep no-follow on the same open that returns the writable handle; | ||
| // a separate pre-check can be raced by swapping in a reparse point. | ||
| f, err := r.root.OpenFile(rel, flag|syscall.FILE_FLAG_OPEN_REPARSE_POINT, perm) |
There was a problem hiding this comment.
On Windows, this only applies FILE_FLAG_OPEN_REPARSE_POINT to the final path passed to OpenFile; intermediate components such as allowed\linkdir\target.log are still traversed before the final open. That means write surfaces like write_file, tee, truncate, and stdout redirects can mutate through an intermediate symlink/junction inside AllowedPaths, while the Unix implementation explicitly walks each component with no-follow and the repo policy requires write operations not to follow symlinks. Please validate/open each path component without following reparse points before returning the writable handle.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5ce4e4f7: Windows write opens now walk each path component relative to the already-open sandbox root using NtCreateFile with OBJ_DONT_REPARSE, so intermediate reparse points are rejected before returning the writable handle. The final file open is also fd-relative/no-follow, and O_CREATE/O_APPEND/O_TRUNC semantics are preserved on the returned handle.
Added Windows regression coverage for intermediate symlink escapes and OpenExistingForWrite through a symlink parent, and updated the allowedpaths syscall allowlist for the intentionally narrow Windows kernel surface.
Validation:
make fmtgo test ./allowedpaths -count=1env GOTOOLCHAIN=local GOOS=windows GOARCH=amd64 go test -c -o /private/tmp/rshell-allowedpaths-windows.test.exe ./allowedpathsgo test ./analysisgo test ./...RSHELL_COMPLIANCE_TEST=1 go test -run TestCompliance ./tests/RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s -count=1
|
@codex review Pushed Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ce4e4f798
ℹ️ 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".
| if len(callCommandFields) == 0 { | ||
| r.errf("%s\n", stdoutFileRedirectionWithoutCommandError) | ||
| r.exit.code = 2 |
There was a problem hiding this comment.
When a stdout file redirect is present and the first command word expands to zero fields (for example empty=; $empty echo ok >out with echo allowed), this early check exits with “stdout file redirection without a command” before expandRemainingCallFields can promote the next expanded word to the command. Bash treats echo as the command in that scenario, and the old all-argument expansion path would have produced the same field list, so valid redirected commands are rejected solely because the first syntactic word vanished.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa2c7195: redirect preflight now expands leading syntactic words until it finds the first non-empty command field, so an empty first-word expansion can still promote the next word to the command before redirect validation. Added a bash-compatible scenario for empty=; $empty echo ok > output.txt.
| func hasTrailingPathSeparator(path string) bool { | ||
| if path == "" { | ||
| return false | ||
| } | ||
| return os.IsPathSeparator(path[len(path)-1]) | ||
| } |
There was a problem hiding this comment.
This only catches literal trailing separators, but write paths are then normalized by filepath.Join/Clean; a target like file/. does not end in a separator, is cleaned to file, and OpenForWrite/OpenExistingForWrite can truncate or mutate the existing regular file. On normal path resolution and in bash, > file/. or truncate -s 0 file/. fails because the final component requires file to be a directory, so this can unexpectedly clobber an allowed file instead of rejecting the malformed target.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fa2c7195: write-open preflight now rejects final . path components before normalization can collapse file/. to file. Added sandbox coverage for OpenForWrite, OpenExistingForWrite, redirect preflight, and a remediation truncate regression that confirms the host handler is not called and the file is unchanged.
|
@codex review Pushed Changes:
Validation:
|
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ 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". |
2 participants
Summary
>and>>throughAllowedPaths, while keeping stderr/all-fd/clobber/read-write redirects restrictedtruncate,systemctl,kill,logrotate, andteeExecHandler/HostCommandHandlerand update help/docs/scenariosTests
make fmtgo test ./...RSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash -timeout 120s