🐛 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.
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.
![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".
ctx.go
Outdated
| if err := os.MkdirAll(filepath.Dir(absolutePath), c.app.config.RootPerms); err != nil { | ||
| return fmt.Errorf("failed to prepare upload path: %w", err) | ||
| } |
There was a problem hiding this comment.
The directory creation in SaveFile uses RootPerms for intermediate directories, but this may not be the intended behavior. Typically, intermediate directories should have different permissions than the root directory (often 0o755 for intermediate paths vs 0o750 for the root). This could lead to overly restrictive permissions on nested upload directories, potentially causing permission issues when other processes need to access these paths.
| if err := os.MkdirAll(filepath.Dir(absolutePath), c.app.config.RootPerms); err != nil { | |
| return fmt.Errorf("failed to prepare upload path: %w", err) | |
| } | |
| dirPath := filepath.Dir(absolutePath) | |
| // Create intermediate directories with 0o755 | |
| if err := os.MkdirAll(dirPath, 0o755); err != nil { | |
| return fmt.Errorf("failed to prepare upload path: %w", err) | |
| } | |
| // Set the final directory's permissions to RootPerms | |
| if err := os.Chmod(dirPath, c.app.config.RootPerms); err != nil { | |
| return fmt.Errorf("failed to set upload directory permissions: %w", err) | |
| } |
| func getRootDir(app *App) (string, error) { | ||
| root := app.config.RootDir | ||
| if root == "" { | ||
| root = "." | ||
| } | ||
|
|
||
| perms := app.config.RootPerms | ||
| if perms == 0 { | ||
| perms = 0o750 | ||
| } | ||
|
|
||
| absoluteRoot, err := filepath.Abs(root) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid upload root: %w", err) | ||
| } | ||
|
|
||
| if err = os.MkdirAll(absoluteRoot, perms); err != nil { | ||
| return "", fmt.Errorf("invalid upload root: %w", err) | ||
| } | ||
|
|
||
| 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) | ||
| } | ||
|
|
||
| info, err := os.Stat(absoluteRoot) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid upload root: %w", err) | ||
| } | ||
|
|
||
| if !info.IsDir() { | ||
| return "", fmt.Errorf("invalid upload root: %s is not a directory", absoluteRoot) | ||
| } | ||
|
|
||
| return absoluteRoot, nil | ||
| } |
There was a problem hiding this comment.
The getRootDir function is called on every file upload to validate and create the root directory. This creates unnecessary filesystem overhead for repeated uploads. Consider caching the validated root directory path after the first successful resolution, as the root configuration doesn't change during the application's lifetime. This would eliminate redundant filepath.Abs, os.MkdirAll, filepath.EvalSymlinks, and os.Stat calls on every upload.
| func rejectSymlinkTraversal(uploadFS fs.FS, normalized string) error { | ||
| if uploadFS == nil { | ||
| return nil | ||
| } | ||
|
|
||
| parts := strings.Split(normalized, "/") | ||
| parent := "." | ||
|
|
||
| for i, part := range parts { | ||
| entries, err := fs.ReadDir(uploadFS, parent) | ||
| if err != nil { | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("invalid upload path: %w", err) | ||
| } | ||
|
|
||
| var entry fs.DirEntry | ||
|
|
||
| for _, e := range entries { | ||
| if e.Name() == part { | ||
| entry = e | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if entry == nil { | ||
| return nil | ||
| } | ||
|
|
||
| if entry.Type()&fs.ModeSymlink != 0 { | ||
| return errUploadSymlinkPath | ||
| } | ||
|
|
||
| info, err := entry.Info() | ||
| if err != nil { | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("invalid upload path: %w", err) | ||
| } | ||
|
|
||
| if i < len(parts)-1 && !info.IsDir() { | ||
| return errUploadTraversal | ||
| } | ||
|
|
||
| if parent == "." { | ||
| parent = part | ||
| } else { | ||
| parent = pathpkg.Join(parent, part) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The rejectSymlinkTraversal function performs directory traversal on every upload, which can cause significant performance degradation especially for deeply nested paths or directories with many entries. Each directory is read fully using fs.ReadDir, and this happens for every path component. For frequent uploads, this overhead can be substantial. Consider caching directory metadata or implementing a more efficient validation approach.
Summary
Key Changes:
Added comprehensive test coverage for security scenarios including traversal prevention and absolute path rejection