fix(allowedpaths): resolve ".." against the walked path, not lexically, after a symlink - #564
fix(allowedpaths): resolve ".." against the walked path, not lexically, after a symlink#564julesmcrt wants to merge 3 commits into
Conversation
…y, after a symlink toAbs used filepath.Join, which collapses ".." lexically before any symlink component is examined. For a path like "link/../file" where link resolves outside the sandbox, this discarded "link" entirely and produced "<cwd>/file" instead of kernel-correct resolution through link's actual target. Read-only operations (Access, Open reads, Stat, Lstat, Readlink, ReadDir*) now use a new resolveAbsPath that walks components one at a time, following a symlink only when a later ".." in the path could pop through it — this fixes the divergence while leaving every other symlink component untouched so existing escape detection and its error messages are unaffected. Write operations keep the plain lexical join: writeopen's O_NOFOLLOW walk needs literal symlink component names intact to reject symlink write targets. Fixes #561.
|
@codex 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: 36abaf46b5
ℹ️ 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".
…solver Address chatgpt-codex-connector review comments on PR #564: absolute Windows paths were losing their drive letter when resolveAbsPath reset the root to a bare separator, and splitComponents only split on the native separator so forward-slash ".." components went unrecognized on Windows. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffbbd5465c
ℹ️ 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".
| resolved = targetVolume + string(filepath.Separator) | ||
| pending = append(splitComponents(targetAbs[len(targetVolume):]), pending...) |
There was a problem hiding this comment.
When an AllowedPaths entry is itself a symlink, an absolute symlink target under that root's real directory is rewritten here to the canonical path, but the later Open/Stat lookup only calls s.resolve, which matches the configured absPath spelling. For example with /tmp/linkroot -> /tmp/realroot and sub/link -> /tmp/realroot/other/real, sub/link/../sibling.txt should read the file under the allowed root, but now resolves to /tmp/realroot/... and is denied; map canonical paths back to the configured root or resolve against canonical roots for read operations.
AGENTS.md reference: AGENTS.md:L39-L42
Useful? React with 👍 / 👎.
Address a chatgpt-codex-connector review comment on PR #564: when the new eager path resolver hit a component that failed Lstat (nonexistent) or was a regular file used as an intermediate component, it fell back to lexical joining, letting a later ".." collapse past it and land on a different, existing file — diverging from the ENOENT/ENOTDIR a real open/stat syscall would produce at that component. Fail resolution outright instead of guessing at the target. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
toAbsusedfilepath.Join, which collapses..lexically before any symlink component is examined. For a path likelink/../filewherelinkresolves outside the sandbox, this discardedlinkentirely and produced<cwd>/fileinstead of the kernel-correct resolution throughlink's actual target.Access,Openreads,Stat,Lstat,Readlink,ReadDir*) now use a newresolveAbsPaththat walks the path one component at a time, following a symlink component only when a later..in the path could actually pop through it. Every other symlink component is left untouched so existing escape detection (os.Root's native "path escapes from parent", the cross-rootresolveFollowingSymlinksfallback) and its error messages are unaffected.Openwith write flags,Truncate,TruncateToZeroIfAtLeast) keep the plain lexicaltoAbs:writeopen'sO_NOFOLLOWwalk needs literal symlink component names intact to detect and reject symlink write targets — eagerly resolving them here would defeat that check.TestDotDotAfterSymlinkFollowsKernelSemanticsandTestDotDotAfterSymlinkEscapeDeniedcovering the exact scenario from the issue (both the correct-resolution and escape-denied cases).Test plan
go build ./...go test ./...(full suite, includingallowedpathsandanalysissymbol-allowlist checks)gofmt -l ./go vet ./...cleanRSHELL_BASH_TEST=1 go test ./tests/ -run TestShellScenariosAgainstBash— not run locally (Docker unavailable in this environment); no scenario YAML files were changed by this fix, only Go code inallowedpaths/, so this should be safe but worth confirming in CI.🤖 Generated with Claude Code