Repository navigation
fix(security): reject path traversal in proxied request paths - #418
Conversation
Go's ServeMux only cleans the escaped request path, so encoded dot segments (e.g. "..%2F", "%2E%2E") survive routing and appear decoded in r.PathValue and r.URL.Path. Several strategies spliced these values into upstream URLs, some of which carry credentials. Reject dot segments, backslashes and NUL bytes globally before OPA, and validate path values in the github-releases, git, hermit, host and artifactory strategies as defence in depth. Escape GitHub URL segments and build the hermit redirect URL structurally. Co-authored-by: Pi <noreply@pi.dev>
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: e8917b1f9e
ℹ️ 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".
| // escapePathSegments escapes each slash-separated segment of path so decoded | ||
| // characters such as "?" and "#" can't alter the structure of the URL. |
There was a problem hiding this comment.
This comment describes an obvious six-line private helper rather than a non-obvious block of at least 20 lines, contrary to the repository's explicit commenting convention; remove it and let the function name and implementation convey the behavior.
AGENTS.md reference: AGENTS.md:L19-L22
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bf817ed5f
ℹ️ 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".
| 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 an include such as link/secret has baseDir/link pointing outside baseDir, filepath.IsLocal accepts it and the following os.Stat resolves the symlink; the exact GNU tar invocation used below then archives the outside file under link/secret (verified with tar -cpf ... -C base -- link/secret). Validate each parent without following a path outside the base before passing the include to tar.
Useful? React with 👍 / 👎.
| if err != nil { | ||
| return err | ||
| } | ||
| if err := rejectSymlink(linkTarget); err != nil { |
There was a problem hiding this comment.
When the extraction destination already contains a symlinked directory, a hard-link header such as Linkname: "escape/secret" passes this final-component Lstat: its parent symlink is followed, and os.Link below links the outside file into the extracted tree. Apply the same parent-component symlink checks to linkTarget that are applied to the destination path before creating the hard link.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in 592d595: hard-link sources now get the same parent-symlink check as destinations, and symlink targets may only use .. as leading components, which also closes chained-symlink escapes (e.g. a/b -> .., s -> a/b, u -> s/..) that passed the per-entry lexical check.
| } | ||
| *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 restoring a snapshot used as a build tree, only the header mode is passed to the extractor, so every regular file receives its extraction time and hdr.ModTime is discarded; directory timestamps are likewise lost. This can change decisions made by mtime-based build tools such as make, so apply the archived timestamps after writing files and after extracting directory contents.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Fixed in 592d595: regular files and directories get their archived mtimes (directories after their contents are written).
| return nil, errors.Errorf("initialise OPA middleware: %w", err) | ||
| } | ||
|
|
||
| // Reject before OPA so policies can't be bypassed with encoded dot segments. |
There was a problem hiding this comment.
This comment annotates a one-line middleware wrapper rather than a non-obvious block of at least 20 lines; the same issue remains on the newly added validation blocks in internal/strategy/git/git.go, github_releases.go, and hermit.go. Remove these short-block comments as required by the repository convention.
AGENTS.md reference: AGENTS.md:L19-L22
Useful? React with 👍 / 👎.
The Go tar reader stops at the end-of-archive marker, leaving record padding unread. Closing the pipe while zstd was still writing it made Extract fail intermittently with "zstd failed: signal: broken pipe". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 218cfed6cb
ℹ️ 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".
| mode := tarMode(hdr) | ||
| switch hdr.Typeflag { | ||
| case tar.TypeDir: | ||
| if err := mkdirAllWithin(directory, rel, mode|0o700); err != nil { |
There was a problem hiding this comment.
When restoring over an existing checkout where a path changed from a regular file or symlink into a directory, mkdirAllWithin rejects the final component as a non-directory instead of replacing it. GNU tar previously handled this transition, and GitRestoreCmd.extract deliberately restores directly over existing checkouts, so switching between commits with such a type change now makes the entire restore fail; remove the conflicting final component before creating the archived directory while retaining the parent-symlink checks.
Useful? React with 👍 / 👎.
| if err := rejectSymlink(target); err != nil { | ||
| return err |
There was a problem hiding this comment.
When an existing checkout contains a symlink at a path that is a regular file in the restored snapshot, this explicit rejection aborts extraction. The following removeExistingNonDir already uses Lstat and unlinks the destination without following it, so the symlink can be replaced safely; retaining this check breaks restores across ordinary Git file-type changes that the previous tar extractor supported.
Useful? React with 👍 / 👎.
| func tarMode(hdr *tar.Header) fs.FileMode { | ||
| return fs.FileMode(uint32(hdr.Mode & int64(fs.ModePerm))) | ||
| } |
There was a problem hiding this comment.
For archives containing setuid, setgid, or sticky files/directories, masking the tar mode with fs.ModePerm drops those bits, even though Extract and the snapshot wrapper promise to preserve file permissions. This changes shared setgid directories and sticky directories on every restore; translate the tar header's special Unix mode bits into fs.ModeSetuid, fs.ModeSetgid, and fs.ModeSticky before applying the mode.
Useful? React with 👍 / 👎.
…imes Hard-link sources only rejected a symlink in their final component, so a symlinked parent (pre-existing in the destination, or built from a chain of archived symlinks such as "a/b -> ..", "s -> a/b", "u -> s/..") let an archive link files from outside the destination. Check every parent of the link source, and reject symlink targets that use ".." after a name, since that resolves relative to wherever the name points rather than lexically. Also restore archived modification times on files and directories, which tar -xpf did before the Go extractor replaced it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* origin/main: fix(client): resume broken parallel get chunks from their offset (#423) fix(git): limit size of request bodies buffered in memory (#422) fix: don't truncate downloads when the cache write fails (#420) fix(security): reject path traversal in proxied request paths (#418) fix: security/sanitize cached git mirrors (#417) fix(security): harden server-side LFS snapshots (#416) # Conflicts: # internal/strategy/git/snapshot.go
Summary: