🐛 bug: align upload root config #3934
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @gaby, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security and configurability of the file upload mechanism. It introduces dedicated configuration options for defining the root directory, file system, and permissions for uploads, alongside implementing comprehensive path validation to prevent common security vulnerabilities such as directory traversal and symlink attacks. The changes ensure that all file uploads are strictly confined to the intended root directory, improving the overall robustness of the application's file handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3934 +/- ##
==========================================
- Coverage 91.56% 91.34% -0.23%
==========================================
Files 119 119
Lines 10165 10266 +101
==========================================
+ Hits 9308 9377 +69
- Misses 544 560 +16
- Partials 313 329 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant security enhancements to file uploads by adding robust path validation and sanitization. The changes are well-structured, centralizing path resolution logic and adding comprehensive tests. I have a couple of suggestions for refactoring and test cleanup to further improve the code.
| func rejectSymlinkTraversal(uploadFS fs.FS, normalized string) error { | ||
| if uploadFS == nil { | ||
| return nil | ||
| } | ||
|
|
||
| parts := strings.Split(normalized, "/") | ||
| current := "." | ||
| for i, part := range parts { | ||
| entries, err := fs.ReadDir(uploadFS, current) | ||
| if err != nil { | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("invalid upload path: %w", err) | ||
| } | ||
|
|
||
| var entry fs.DirEntry | ||
| var found bool | ||
| for _, e := range entries { | ||
| if e.Name() == part { | ||
| entry = e | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if !found { | ||
| return nil | ||
| } | ||
|
|
||
| if entry.Type()&fs.ModeSymlink != 0 { | ||
| return errUploadSymlinkRoute | ||
| } | ||
|
|
||
| if i < len(parts)-1 && !entry.IsDir() { | ||
| return errUploadTraversal | ||
| } | ||
|
|
||
| if current == "." { | ||
| current = part | ||
| continue | ||
| } | ||
|
|
||
| current = pathpkg.Join(current, part) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The rejectSymlinkTraversal function can be refactored for better readability and performance. The current implementation iterates through directory entries for each path segment, which can be inefficient for directories with many files. Additionally, the logic for updating the current path is more complex than necessary.
Using fs.Stat for each path component is more direct and avoids reading entire directories. This also simplifies the path traversal logic, making the function easier to understand and maintain.
func rejectSymlinkTraversal(uploadFS fs.FS, normalized string) error {
if uploadFS == nil {
return nil
}
currentPath := ""
parts := strings.Split(normalized, "/")
for i, part := range parts {
currentPath = pathpkg.Join(currentPath, part)
entryInfo, err := fs.Stat(uploadFS, currentPath)
if err != nil {
if errors.Is(err, fs.ErrNotExist) {
// If a path component doesn't exist, it can't be a symlink.
// The path will be created later, so this is fine.
return nil
}
return fmt.Errorf("invalid upload path: %w", err)
}
// Check for symlinks. For os.DirFS, fs.Stat uses os.Lstat, which does not follow symlinks.
if entryInfo.Mode()&fs.ModeSymlink != 0 {
return errUploadSymlinkRoute
}
// All but the last part of the path must be directories
if i < len(parts)-1 && !entryInfo.IsDir() {
return errUploadTraversal
}
}
return nil
}There was a problem hiding this comment.
Pull request overview
This pull request enhances the security of file upload operations by introducing configurable upload root directory management and comprehensive path validation. The changes prevent directory traversal, absolute path exploits, and symlink-based attacks by validating all upload paths against a configured root directory before allowing file writes.
Key Changes:
- Added three new configuration options:
RootDir(upload root directory),RootFS(filesystem implementation for validation), andRootPerms(permissions for root directory creation) - Implemented multi-layer path validation in
SaveFileandSaveFileToStoragemethods that rejects absolute paths, directory traversal attempts, and symlink escapes - Added comprehensive test coverage for security scenarios including traversal prevention and absolute path rejection
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| error.go | Adds four new error variables for upload path validation failures |
| app.go | Introduces RootDir, RootFS, and RootPerms configuration fields with appropriate defaults |
| ctx.go | Implements path validation logic through resolveUploadPath, sanitizeUploadPath, rejectSymlinkTraversal, and helper functions; updates SaveFile and SaveFileToStorage to use validation |
| ctx_test.go | Adds security-focused tests for path traversal prevention, absolute path rejection, and validates storage upload path validation |
| docs/whats_new.md | Documents the new upload security features in the "What's New" section |
| docs/api/ctx.md | Updates SaveFile and SaveFileToStorage documentation to describe the new path validation behavior and configuration options |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: b550622 | Previous: dd0b041 | Ratio |
|---|---|---|---|
Benchmark_Compress/Zstd - B/op |
1 B/op |
0 B/op |
+∞ |
Benchmark_Compress_Levels/Zstd_LevelDefault - B/op |
1 B/op |
0 B/op |
+∞ |
Benchmark_Compress_Levels/Zstd_LevelBestCompression - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
|
/gemini review |
|
@codex review |
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the security of file uploads by introducing new configuration options (RootDir, RootFS, RootPerms) and robust path validation for the SaveFile and SaveFileToStorage methods. The changes include comprehensive path sanitization to reject absolute paths, prevent directory traversal attempts (e.g., using ..), and ensure files are saved strictly within the configured RootDir. A critical rejectSymlinkTraversal function was added to prevent symlink-based attacks by validating each path segment against the RootFS, while filepath.EvalSymlinks is used on the RootDir itself to prevent root directory manipulation. The isWithinRoot function provides a final containment check. New tests were added to verify these security measures, and review comments emphasized the importance and effectiveness of these layered validations against common path manipulation vulnerabilities.
| func rejectSymlinkTraversal(uploadFS fs.FS, normalized string) error { | ||
| if uploadFS == nil { | ||
| return nil | ||
| } | ||
|
|
||
| parts := strings.Split(normalized, "/") | ||
| current := "." | ||
|
|
||
| for i, part := range parts { | ||
| next := part | ||
| if current != "." { | ||
| next = pathpkg.Join(current, part) | ||
| } | ||
|
|
||
| info, err := fs.Stat(uploadFS, next) | ||
| if err != nil { | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("invalid upload path: %w", err) | ||
| } | ||
|
|
||
| if info.Mode()&fs.ModeSymlink != 0 { | ||
| return errUploadSymlinkPath | ||
| } | ||
|
|
||
| if i < len(parts)-1 && !info.IsDir() { | ||
| return errUploadTraversal | ||
| } | ||
|
|
||
| current = next | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The rejectSymlinkTraversal function is a crucial security component. By iterating through each part of the normalized path and checking for symlinks using fs.Stat against the uploadFS (which is rooted at RootDir), it effectively prevents symlink traversal attacks where an attacker might try to create or overwrite files outside the designated upload root by exploiting symlinks within the provided path. The check !info.IsDir() for intermediate parts also prevents writing into a file disguised as a directory.
| resolvedRoot, err := filepath.EvalSymlinks(absoluteRoot) | ||
| if err == nil { | ||
| absoluteRoot = resolvedRoot | ||
| } else if !errors.Is(err, fs.ErrNotExist) { | ||
| return "", fmt.Errorf("invalid upload root: %w", err) | ||
| } |
There was a problem hiding this comment.
The use of filepath.EvalSymlinks on the absoluteRoot is a critical security measure. It resolves any symbolic links in the root path itself, preventing an attacker from configuring the upload root to point to a sensitive directory via a symlink. The error handling for fs.ErrNotExist is also correct, allowing the root to be a non-existent path that will be created.
| if filepath.IsAbs(path) { | ||
| return "", errUploadAbsolute | ||
| } |
There was a problem hiding this comment.
Rejecting absolute paths (filepath.IsAbs) and directory traversal segments (containsParentDir) early in sanitizeUploadPath is a robust first line of defense against common path manipulation attacks. This ensures that user-provided paths are always relative and do not attempt to escape the intended directory structure.
| func isWithinRoot(root, target string) bool { | ||
| rel, err := filepath.Rel(root, target) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| return rel != ".." && !strings.HasPrefix(rel, "../") && rel != "..\\" && !strings.HasPrefix(rel, "..\\") | ||
| } |
There was a problem hiding this comment.
The isWithinRoot function provides an excellent final check to ensure that the target path, after all normalization and resolution, truly remains within the root directory. Using filepath.Rel and checking for .. or ../ prefixes is a robust way to confirm this containment, especially important for cross-platform compatibility with both Unix-like and Windows paths.
| // RootFS provides an fs.FS implementation rooted at RootDir used to validate upload targets. | ||
| // | ||
| // Default: os.DirFS(RootDir) |
There was a problem hiding this comment.
The comment for RootFS suggests a default of os.DirFS(RootDir). While this is indeed the fallback logic in resolveUploadPath, the New function itself doesn't explicitly set this default. It might be clearer to either set app.config.RootFS = os.DirFS(app.config.RootDir) in the New function (after RootDir is initialized) or adjust the comment to reflect that it's a runtime default if not explicitly configured.
| normalized := pathpkg.Clean(rawNormalized) | ||
| normalized = utils.TrimLeft(normalized, '/') | ||
| if normalized == "" || normalized == "." { | ||
| return "", errUploadTraversal |
There was a problem hiding this comment.
The combination of pathpkg.Clean and utils.TrimLeft followed by checks for empty or . paths is effective for normalizing and validating the path. This helps catch edge cases where cleaning might result in an unexpected path, further preventing traversal or writing to the root itself when a specific file is expected.
| if !fs.ValidPath(normalized) { | ||
| return "", errUploadTraversal | ||
| } |
There was a problem hiding this comment.
Using fs.ValidPath(normalized) is a good standard check for ensuring the path conforms to io/fs requirements, which can help prevent issues with invalid characters or structures that might bypass other checks.
![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.
ℹ️ 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".
| info, err := fs.Stat(uploadFS, next) | ||
| if err != nil { |
There was a problem hiding this comment.
Upload validation walks path components with fs.Stat, but Stat follows symlinks, so the subsequent ModeSymlink check in rejectSymlinkTraversal never triggers. If the configured upload root already contains a symlinked directory (e.g., uploads/link -> /tmp/outside), a caller can save a file under that link and the write will escape the configured root despite the new validation. Use an lstat-style check (e.g., fs.ReadDir/DirEntry.Type or os.Lstat) to detect symlink components before writing.
Useful? React with 👍 / 👎.
Summary
Key Changes:
Added comprehensive test coverage for security scenarios including traversal prevention and absolute path rejection