Repository navigation
Harden archive path handling - #419
stuartwdouglas merged 1 commit into
Conversation
0bf817e
into
block:fix-reject-path-traversal
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 1026461094
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if err != nil { | ||
| return err | ||
| } | ||
| if err := rejectSymlink(linkTarget); err != nil { |
There was a problem hiding this comment.
When extracting into an existing tree containing pivot -> /outside, an archive hard-link entry whose Linkname is pivot/secret passes safeArchivePath and this check because Lstat follows the parent symlink and sees the outside regular file; os.Link then creates an in-tree hard link to that outside inode. A later build step that writes the restored path can therefore modify a file outside directory, so validate every source component or restrict hard links to previously extracted in-root regular entries.
Useful? React with 👍 / 👎.
| return errors.Errorf("not a directory: %s", baseDir) | ||
| } | ||
| for _, path := range includePaths { | ||
| if strings.Contains(path, "\\") || !filepath.IsLocal(path) { |
There was a problem hiding this comment.
When baseDir/link is a symlink to an outside directory, link/secret is lexically local and passes this check; os.Stat and the Linux GNU tar 1.35 command used below resolve the intermediate symlink, archiving the outside file as link/secret. Since this cache can back onto remote instances, the containment check can still leak files outside baseDir; walk the parent components with Lstat before invoking tar (while allowing a final symlink if desired).
AGENTS.md reference: AGENTS.md:L1-L5
Useful? React with 👍 / 👎.
| } | ||
| *dirs = append(*dirs, dirMode{path: target, mode: mode}) | ||
| case tar.TypeReg, 0: | ||
| return extractRegularFile(directory, rel, target, mode, r) |
There was a problem hiding this comment.
When cached artifacts are consumed by timestamp-sensitive tools such as Make, Ninja, or Git's index, this regular-file branch forwards only mode and contents, so each new file gets extraction time instead of hdr.ModTime. In the Linux GNU tar 1.35 path replaced here, -m/--touch is documented as “don't extract file modified time” and was not passed, so this changes every restored mtime and can trigger rebuilds or incorrect freshness decisions; apply the header mtime after writing.
AGENTS.md reference: AGENTS.md:L1-L2
Useful? React with 👍 / 👎.
| case errors.Is(err, io.EOF): | ||
| return applyDirModes(dirs) |
There was a problem hiding this comment.
When a cache object is a valid zstd frame whose decompressed payload is empty (or only one truncated zero block), archive/tar.Reader.Next returns io.EOF immediately and this branch reports success, so Client.Restore can report a hit while restoring nothing. The previous Linux GNU tar 1.35 extraction rejects zero-byte input as not a tar archive; track decompressed framing or byte count and distinguish a real tar terminator from EOF before any framing.
Useful? React with 👍 / 👎.
No description provided.