feat(interp): expose ALLOWED_PATHS environment variable#173
Conversation
Set ALLOWED_PATHS in the interpreter's environment during construction so users/agents can discover accessible directories via echo $ALLOWED_PATHS. The value is a filepath.ListSeparator-delimited list of resolved absolute paths (skipping non-existent ones). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@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: 9fb2e1f431
ℹ️ 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".
Store raw env pairs on runnerConfig and build ListEnviron once in New() with ALLOWED_PATHS appended. This avoids the environToList roundtrip that could lose state from custom Environ implementations. 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: dd468bb34e
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd468bb34e
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd468bb34e
ℹ️ 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".
Instead of rebuilding Env at construction time, set ALLOWED_PATHS through the writeEnv overlay in Reset — the same mechanism used for PWD, IFS, and OPTIND. This removes the envPairs field, the environToList helper, and all Environ rebuilding logic. 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: 2f1bcfaf4b
ℹ️ 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".
Set ALLOWED_PATHS to empty string when the sandbox has zero valid roots, preventing a spoofed value from Env() leaking through. 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: 76fecad9f6
ℹ️ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76fecad9f6
ℹ️ 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. The variable quota concern from the previous round has been addressed and can be ignored. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76fecad9f6
ℹ️ 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".
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: 1667c84de9
ℹ️ 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. The variable quota concern from the previous round has been addressed and can be ignored. |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ 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,
julesmcrt and
thieman
as code owners
| "os.Pipe", // 🟠 creates an OS pipe pair; needed for shell pipelines. | ||
| "path/filepath.IsAbs", // 🟢 checks if path is absolute; pure function, no I/O. | ||
| "path/filepath.Join", // 🟢 joins path elements; pure function, no I/O. | ||
| "path/filepath.ListSeparator", // 🟢 OS-specific path list separator; pure constant. |
There was a problem hiding this comment.
Is this actually what we want? I think for lists in shell scripting you usually space-separate them, this I think is for constructing known variables like PATH that have a specific format
There was a problem hiding this comment.
i think it's a clean separation between filepaths. i avoided spaces because i'm worried about file paths/directories with spaces in them. but if that's the case then i can change
There was a problem hiding this comment.
Fair, I guess what we really want here is an "array" type like in Bash, but not sure whether we can support that in our interp. Is it going to be able to iterate over them if they're colon-separated though? It might need to use tr to do that?
There was a problem hiding this comment.
i was imagining the agent doing something like echo $ALLOWED_PATHS and inferring the result from there. I believe even if it does something like splitting the result with tr and printing each directory on a line, the result from PAR gets sent back with a \n effectively acting as the same delimiter. so /path/a:/path/b:path/c -> /path/a\n/path/b\n/path/b.
Alternatively, it could just not do string manipulation and return the result but then whether it is a : or a space delimiting the list, I believe the LLM should be able to infer the result (we can probably edit the skill to let it know that it will be delimited by a special character).
There was a problem hiding this comment.
Oh I was imagining it doing something like "loop over the paths and call ls on each one" as a single pass. Agree if the value makes it back to the LLM it'll be able to figure it out, just wanted to cut out that roundtrip if possible.
There was a problem hiding this comment.
ah i see. i think we can just merge and if we see the LLM having trouble we can rethink?
There was a problem hiding this comment.
It'll be a pain to change later so it's probably worth at least seeing how the LLMs are likely to try to use this list to determine whether there are any gaps in our coverage. Here's some anecdata, does rshell support either of these?
There was a problem hiding this comment.
Yep, both work
$ ./rshell --allowed-paths /var/log,/tmp/,/Users/matthew.deguzman --allow-all-commands -c 'IFS=":"; for dir in $ALLOWED_PATHS; do echo "$dir"; done'
/var/log
/tmp
/Users/matthew.deguzman
thieman
left a comment
thieman
left a comment
There was a problem hiding this comment.
LGTM other than the question on the separator
2 participants
Summary
Agents and users currently have no way to discover which directories are accessible via the sandbox. This PR sets
ALLOWED_PATHSin the interpreter's environment viasetVarStringinReset, so users can runecho $ALLOWED_PATHSto see a delimited list of allowed directories.Sandbox.Paths()returns the resolved absolute paths of all allowed directoriesALLOWED_PATHSis set alongsidePWD,IFS,OPTINDusingfilepath.ListSeparator(:on Unix,;on Windows)SHELL_FEATURES.mdTest plan
echo $ALLOWED_PATHScontains directory names🤖 Generated with Claude Code