exec-server: canonicalize bound filesystem paths#25149
Conversation
![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: ab25267ba1
ℹ️ 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".
starr-openai
force-pushed
the
starr/skills-path-ref-1b-bound-canonicalize
branch
3 times, most recently
from
865271a to
1170d1c
Compare
starr-openai
force-pushed
the
starr/skills-path-ref-1-env-path-ref
branch
from
f1f663f to
b444724
Compare
starr-openai
force-pushed
the
starr/skills-path-ref-1b-bound-canonicalize
branch
2 times, most recently
from
1119f59 to
641000f
Compare
starr-openai
force-pushed
the
starr/skills-path-ref-1-env-path-ref
branch
from
b444724 to
55ec07d
Compare
starr-openai
force-pushed
the
starr/skills-path-ref-1b-bound-canonicalize
branch
from
641000f to
8c24577
Compare
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
Good for me after having double checked my comments
| base_path: &AbsolutePathBuf, | ||
| path: &Path, | ||
| ) -> FileSystemResult<AbsolutePathBuf> { | ||
| Ok(base_path.join(path)) |
There was a problem hiding this comment.
I might be crazy... but what if
- A user gets an
EnvironmentPathReffor one allowed skill/root path. - Some later code calls something like
join("../outside"),join("/absolute/path"), orjoin("~/.codex") AbsolutePathBuf::joinnormalizes that into a valid absolute path outside the original ref.- Now the caller has a same-filesystem
EnvironmentPathRefto a path it was
never handed
I we want to preserve path authority, should we do everything in relative for this function?
I might be completely wrong here so I invite you to double check
| .map(|path| path.map(|path| self.with_path(path))) | ||
| } | ||
|
|
||
| pub async fn canonicalize( |
There was a problem hiding this comment.
The new canonicalize method accepts a sandbox, but local() binds LOCAL_FS, which has no runtime paths, so won't the callers hit the helper-path error instead of sandboxed canonicalization?
| EnvironmentPathRef::new(Arc::clone(&LOCAL_FS), path) | ||
| } | ||
|
|
||
| #[async_trait] |
There was a problem hiding this comment.
Side note but if you want to learn cool stuff about Rust, ask ChatGPT more about async_trait, how does it work, why this is useful and why this is necessary
This is a very interesting example
2 participants
Summary
Stack
Validation
cd /Users/starr/code/codex-worktrees/pr-25098-restack4/codex-rs && just fmt