fix: prevent path traversal during plugin extraction#755
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the ZIP extraction logic in ZipPluginDecoder to prevent path traversal vulnerabilities (Zip Slip) by introducing path validation helpers (safeExtractPath and pathIsInside) and adds corresponding unit tests. The review feedback suggests wrapping the file reader in an io.LimitReader to protect against Zip Bomb attacks, and explicitly checking for .. in the path validation to ensure robust path traversal prevention.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if entryPath == "." || | ||
| path.IsAbs(entryPath) || | ||
| strings.HasPrefix(entryPath, "../") || | ||
| strings.Contains(entryPath, "/../") || | ||
| strings.Contains(entryPath, `\`) || | ||
| filepath.IsAbs(filepath.FromSlash(entryPath)) { |
There was a problem hiding this comment.
The current path traversal validation checks if entryPath starts with ../ using strings.HasPrefix(entryPath, "../"). However, if entryPath is exactly .. (which can happen if the entry name is .. or resolves to it via path.Clean), it will bypass this check because it does not start with ../. While pathIsInside will eventually catch and reject it, it is safer, more explicit, and more efficient to reject .. directly in the first validation block.
| if entryPath == "." || | |
| path.IsAbs(entryPath) || | |
| strings.HasPrefix(entryPath, "../") || | |
| strings.Contains(entryPath, "/../") || | |
| strings.Contains(entryPath, `\`) || | |
| filepath.IsAbs(filepath.FromSlash(entryPath)) { | |
| if entryPath == "." || | |
| entryPath == ".." || | |
| path.IsAbs(entryPath) || | |
| strings.HasPrefix(entryPath, "../") || | |
| strings.Contains(entryPath, "/../") || | |
| strings.Contains(entryPath, `\\`) || | |
| filepath.IsAbs(filepath.FromSlash(entryPath)) { |
| return nil | ||
| } | ||
|
|
||
| func safeExtractPath(dst, entryName string) (string, error) { |
There was a problem hiding this comment.
func safeExtractPath(dst, entryName string) (string, error) {
if entryName == "" || strings.Contains(entryName, `\`) {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
}
for _, part := range strings.Split(entryName, "/") {
if part == ".." {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
}
}
entryPath := path.Clean(entryName)
if entryPath == "." || path.IsAbs(entryPath) {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
}
targetPath := filepath.Join(dst, filepath.FromSlash(entryPath))
if !pathIsInside(dst, targetPath) {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
}
return targetPath, nil
}
| entryPath := path.Clean(entryName) | ||
| if entryPath == "." || path.IsAbs(entryPath) { | ||
| return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName) | ||
| } |
There was a problem hiding this comment.
not check targetPath := filepath.Join(dst, filepath.FromSlash(entryPath))
if !pathIsInside(dst, targetPath) {
return "", fmt.Errorf("%w: %q", errUnsafeZipPath, entryName)
} ?
Summary
Tests
go test ./pkg/plugin_packager/decoder ./pkg/plugin_packager/packagerNotes:
go test ./pkg/plugin_packageris blocked locally becausepkg/license/private_key/PRIVATE_KEY.pemis not present in the checkout.go test -race ./pkg/plugin_packager/decoderis blocked locally because the current Go environment has cgo disabled.