🔥 feat: Add support for RootDir and RootFs #4035
Conversation
WalkthroughThis PR introduces upload root filesystem configuration to Fiber. Three new Config fields (RootDir, RootPerms, RootFs) control upload destination and permissions. SaveFile and SaveFileToStorage gain comprehensive path validation, symlink protection, and filesystem abstraction support. New error types signal validation failures, with extensive test coverage and documentation additions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SaveFile as SaveFile Handler
participant Validator as Path Validator
participant RootFS as RootFs/RootDir
participant Storage as Storage Backend
Client->>SaveFile: POST file with path
SaveFile->>Validator: validateUploadPath(path)
alt Invalid path
Validator-->>SaveFile: ErrInvalidUploadPath
SaveFile-->>Client: Error
else Path valid
Validator-->>SaveFile: Normalized path
SaveFile->>RootFS: Check path within root
alt Path escapes root
RootFS-->>SaveFile: ErrUploadPathEscapesRoot
SaveFile-->>Client: Error
else Path contained
RootFS-->>SaveFile: saveMultipartFileToFS()
SaveFile->>Storage: SetWithContext(storageKey, data)
Storage-->>SaveFile: Success
SaveFile-->>Client: File saved
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 improves the security and reliability of file uploads by introducing a comprehensive validation and normalization system for upload paths. It allows applications to define secure root directories or filesystems for uploads, preventing malicious path manipulations like directory traversal. The changes also standardize error reporting for invalid upload attempts and ensure that the upload environment is properly configured and writable at startup, leading to a more secure and predictable file handling mechanism. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
gaby
changed the title
![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 significantly enhances the security and robustness of file upload handling by introducing comprehensive path validation, root directory confinement, and symlink checks. The changes centralize upload logic into a new upload.go file, ensuring that all file saving operations (SaveFile and SaveFileToStorage) are hardened against path traversal and other malicious attempts. New configuration options (RootDir, RootFs) provide flexible control over upload destinations, and dedicated error types (ErrInvalidUploadPath, ErrUploadPathEscapesRoot) improve API-level error reporting. The accompanying tests thoroughly cover various scenarios, including valid and invalid paths, ensuring the new security measures function as intended. Overall, this is a well-executed and crucial security improvement.
There was a problem hiding this comment.
Pull request overview
This PR hardens multipart upload handling by introducing configurable upload roots (RootDir, RootFs), centralized upload path validation/normalization, and stronger traversal/symlink protections for SaveFile and SaveFileToStorage, with corresponding tests and docs.
Changes:
- Added
Config.RootDir/Config.RootFsandconfigureUploads()to pre-resolve, create, and probe upload roots (OS directory orfs.FS-backed) at app startup. - Introduced
upload.gohelpers (validateUploadPath,ensureUploadPathWithinRoot,ensureNoSymlinkFS, etc.) and wired them intoDefaultCtx.SaveFile/SaveFileToStorageto enforce relative, non-escaping paths and symlink checks. - Exported new upload errors (
ErrInvalidUploadPath,ErrUploadPathEscapesRoot), updated docs for config and context APIs, and extended tests to cover allowed paths and root traversal rejection.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
upload.go |
New upload configuration and helper functions for path validation, root resolution, symlink and writability checks backing the updated upload behavior. |
error.go |
Adds exported error values for invalid upload paths and root-escape attempts to standardize error reporting. |
app.go |
Extends Config with RootDir/RootFs, stores derived upload root state, and calls configureUploads() during app initialization. |
ctx.go |
Reimplements SaveFile and SaveFileToStorage to normalize/validate paths, enforce RootDir/RootFs boundaries, check symlinks, and write via either fasthttp or a custom fs.FS writer. |
ctx_test.go |
Updates SaveFile tests to use RootDir and adds traversal/absolute path rejection tests for both disk and storage uploads, plus a recordingStorage helper. |
docs/api/fiber.md |
Documents the new Config.RootDir and Config.RootFs options and their role in upload path resolution. |
docs/api/ctx.md |
Documents the new path constraints for SaveFile and SaveFileToStorage (relative-only, no .. or absolute prefixes, enforced against configured roots). |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4035 +/- ##
==========================================
- Coverage 91.21% 90.94% -0.27%
==========================================
Files 119 120 +1
Lines 11117 11368 +251
==========================================
+ Hits 10140 10339 +199
- Misses 620 649 +29
- Partials 357 380 +23
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:
|
![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: ca7e850 | Previous: 4b70666 | Ratio |
|---|---|---|---|
Benchmark_Ctx_IsProxyTrusted/NoProxyCheckParallel |
8.319 ns/op 0 B/op 0 allocs/op |
5.528 ns/op 0 B/op 0 allocs/op |
1.50 |
Benchmark_Ctx_IsProxyTrusted/NoProxyCheckParallel - ns/op |
8.319 ns/op |
5.528 ns/op |
1.50 |
This comment was automatically generated by workflow using github-action-benchmark.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces robust security enhancements for file uploads by adding RootDir and RootFs configurations. The changes centralize path normalization, validation, and writability probes, effectively preventing path traversal and symlink attacks. The new upload.go file encapsulates this logic, and the comprehensive test suite demonstrates thorough coverage of various scenarios, including edge cases and error conditions. The documentation updates clearly explain the new features and security implications for users. Overall, this is a well-implemented and critical improvement for the framework's file upload capabilities.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/api/ctx.md`:
- Around line 1713-1714: The note about "Storage keys are prefixed by the
configured root when one is set" is misplaced under the SaveFile section; move
that sentence to the SaveFileToStorage section (and remove it from the current
paragraph at lines near 1713), and also update the duplicate mention around
lines 1748-1749 so the storage-key prefixing detail only appears in the
SaveFileToStorage doc; ensure the SaveFile paragraph keeps only disk/path
constraints (relative paths, no .., resolved against Config.RootDir/RootFs) and
that SaveFileToStorage explains the root prefixed storage key behavior.
🧹 Nitpick comments (2)
app_test.go (2)
199-204: Preferrequireassertions for consistency.The test file predominantly uses
requirefrom testify for assertions. For consistency, consider usingrequire.Equalinstead of manual comparisons witht.Fatalf.♻️ Suggested refactor
- if tfs.mkdirPath != "uploads" { - t.Fatalf("expected RootFs prefix %q, got %q", "uploads", tfs.mkdirPath) - } - if tfs.mkdirPerm != tt.wantPerm { - t.Fatalf("expected RootPerms %o, got %o", tt.wantPerm, tfs.mkdirPerm) - } + require.Equal(t, "uploads", tfs.mkdirPath, "RootFs prefix mismatch") + require.Equal(t, tt.wantPerm, tfs.mkdirPerm, "RootPerms mismatch")
214-224: Preferrequireassertions for consistency.Similar to the previous test, this test should use
requireassertions for consistency with the rest of the test file.♻️ Suggested refactor
- if err != nil { - t.Fatalf("expected no error, got %v", err) - } - - if !strings.HasPrefix(normalized.osPath, ".") { - t.Fatalf("expected os path %q to preserve leading dot", normalized.osPath) - } - if normalized.slashPath != ".hidden/file.txt" { - t.Fatalf("expected slash path %q, got %q", ".hidden/file.txt", normalized.slashPath) - } + require.NoError(t, err) + require.True(t, strings.HasPrefix(normalized.osPath, "."), "expected os path %q to preserve leading dot", normalized.osPath) + require.Equal(t, ".hidden/file.txt", normalized.slashPath)
Motivation
github.com/gofiber/utils/v2trimming helpers for reliable, high-performance string trimming in hot code paths.Description
Config.RootDirandConfig.RootFsand initialize per-app upload state inconfigureUploads()(uploadRootDir,uploadRootEval,uploadRootPath,uploadRootFSPrefix,uploadRootFSWriter) to probe/create roots at startup and verify writability.validateUploadPath,isAbsUploadPath,containsDotDot,storageRootPrefix,cleanUploadRootPrefix,ensureUploadPathWithinRoot,ensureNoSymlinkFS,probeUploadDirWritable, andprobeUploadFSWritableinupload.goto centralize path normalization, validation, and probes; useutils.TrimLeftinstead ofstrings.TrimPrefixwhere appropriate.ErrInvalidUploadPathandErrUploadPathEscapesRoottoerror.goand update all upload-related code to return those errors.DefaultCtx.SaveFileandDefaultCtx.SaveFileToStorageto validate and normalize the provided path, resolve it against the configured root (OS dir orRootFsprefix), reject traversal/absolute attempts, and write either to theRootFsvia itsOpenFileor to the pre-resolvedRootDirpath; addsaveMultipartFileToFShelper for FS writer paths.Test_Ctx_SaveFile,Test_Ctx_SaveFile_RootDirTraversal,Test_Ctx_SaveFileToStorage,Test_Ctx_SaveFileToStorage_RootDirTraversal), and update docs (docs/api/ctx.md,docs/api/fiber.md) to document relative-path requirement and root resolution behavior.