Reject paths outside the repo root in repo drivers and AI file tools#9723
Reject paths outside the repo root in repo drivers and AI file tools#9723nishantmonu51 wants to merge 1 commit into
Conversation
|
@codex: review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc3a9b7d0a
ℹ️ 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".
| // Repo drivers must use it instead of joining untrusted paths onto the root directly. | ||
| func ResolveRepoPath(root, path string) (string, error) { |
There was a problem hiding this comment.
Reject traversal globs before walking repo roots
This containment helper is only applied to single file paths, but ListGlob remains on the old path: both repo drivers still pass caller-controlled globs to doublestar.GlobWalk(os.DirFS(root), ...), and the public ListFiles RPC forwards req.Glob unchanged. For inputs such as ../* or ../../**, os.DirFS is not a chroot and the glob walk can enumerate parent-directory entries, so the security fix still leaves a repo API that can escape the root; reject .. in globs or resolve the glob base with the same containment check before walking.
Useful? React with 👍 / 👎.
The AI file tools (
read_file,write_file,develop_file) and the runtime's file APIs passed caller-supplied paths straight torepo.Get/Put/Delete, where both thefileandadmindrivers joined them onto the repo root with no containment check. A path with..segments (e.g./models/../../../../etc/passwd) could therefore read, write, or delete files outside the project root.drivers.ResolveRepoPath, which resolves a repo-relative path against the root and errors if the result escapes it. Both repo drivers now use it everywhere they previously didfilepath.Join(root, path)(Get,Hash,Stat,Put,MkdirAll,Rename,Delete)...segments up front, so the model gets a clear error instead of a filesystem error.ResolveRepoPath, traversal assertions in the shared repo driver test suite, and traversal cases for theread_fileandwrite_filetools.Checklist: