[codex] Parse path access modes in allowed paths#271
Conversation
This comment has been minimized.
This comment has been minimized.
|
@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: e0fd9d8702
ℹ️ 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 strings.HasSuffix(path, suffix.text) && len(path) > len(suffix.text) { | ||
| return path[:len(path)-len(suffix.text)], suffix.mode |
There was a problem hiding this comment.
On POSIX filesystems : is a valid filename character, so this unconditionally treats an existing allowlist entry such as /srv/tenant:rw as the mode-suffixed path /srv/tenant. If /srv/tenant also exists, a configuration that previously confined the runner to the literal tenant:rw directory now silently widens the sandbox to its sibling/parent-named directory; if it does not exist, the intended directory is skipped. The mode parser needs an unambiguous opt-in/escape or validation path that does not reinterpret legacy literal path names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6fcaad31 by preserving existing literal paths that end in :ro or :rw as read-only paths. Mode parsing now only strips the suffix when the literal suffixed path is absent, so legacy POSIX directories like /srv/tenant:rw no longer widen to /srv/tenant.
Regression coverage:
TestResolveAllowedPathModePreservesExistingLiteralPathTestAllowedPathModeDoesNotWidenExistingLiteralSuffixPath
Validation:
make fmtgo test ./allowedpathsgo test ./analysisgo test ./interpgo test ./...
|
@codex review Pushed Validation:
|
|
@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: 61e87e1375
ℹ️ 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 _, err := os.Lstat(path); err == nil || !errors.Is(err, os.ErrNotExist) { | ||
| return path, pathModeReadOnly |
There was a problem hiding this comment.
On Windows, an allowed path like C:\data:rw probes the unsuffixed-looking literal first, but a colon after the final component is not a valid directory name and can make os.Lstat(path) return an invalid-name error rather than os.ErrNotExist. Because this branch preserves the path for any non-ErrNotExist error, AllowedPaths([]string{dir + ":rw"}) keeps the :rw suffix, OpenRoot then skips it, and the documented :ro/:rw syntax does not work for normal Windows directories. This should strip the suffix for Windows invalid-name literal probes while still preserving real POSIX literals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6cf4c4dc by making allowed-path mode resolution platform-specific. Non-Windows builds keep the literal-path preservation check for existing :ro/:rw paths, while Windows builds strip terminal mode suffixes directly so normal Windows directories like C:\data:rw parse as C:\data with rw mode instead of being skipped.
Regression coverage:
TestResolveAllowedPathModeStripsWindowsSuffix- Existing POSIX literal preservation tests remain on non-Windows
Validation:
make fmtgo test ./allowedpathsgo test ./analysisgo test ./interpenv GOTOOLCHAIN=local GOCACHE=/private/tmp/rshell-go-cache GOOS=windows GOARCH=amd64 go test -c ./allowedpaths -o /private/tmp/rshell-allowedpaths-windows.test.exego test ./...
|
@codex review Pushed Validation:
|
|
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". |
1 participant
Summary
This POC refactors
AllowedPathsto accept optional terminal path mode suffixes::rodefault to read-only:rwis parsed and stored as read/write metadata for future write-aware actionsALLOWED_PATHS/help-visible roots are exposed:rocan override a broader:rwCurrent rshell write behavior remains unchanged: write-mode paths are parsed, but file opens with write flags are still denied.
Validation
make fmtgo test ./allowedpathsgo test ./analysisgo test ./interpgo test ./...Notes
The commit was created unsigned in the Codex execution environment because
dd-gitsigncould not access an SSH agent (SSH_AUTH_SOCKwas unset).