Conversation
WalkthroughAdds configurable upload root support (RootDir, RootFs, RootPerms), comprehensive upload path validation and containment/symlink protections, RootFs-aware save logic, new upload-related errors, extensive tests, and updated docs and logging for upload handling. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SaveFileHandler as SaveFile Handler
participant Validator as Path Validator
participant RootStore as RootFs/RootDir
participant Storage as Storage Backend
Client->>SaveFileHandler: POST multipart with path
SaveFileHandler->>Validator: validateUploadPath(path)
alt invalid path
Validator-->>SaveFileHandler: ErrInvalidUploadPath
SaveFileHandler-->>Client: error response
else valid path
Validator-->>SaveFileHandler: normalized path
SaveFileHandler->>RootStore: ensure path contained and writable
alt escapes root or symlink detected
RootStore-->>SaveFileHandler: ErrUploadPathEscapesRoot
SaveFileHandler-->>Client: error response
else contained
SaveFileHandler->>RootStore: saveMultipartFileToFS / write to disk
RootStore-->>Storage: provide storage key (prefixed)
Storage-->>SaveFileHandler: success
SaveFileHandler-->>Client: success response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
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.07% 90.77% -0.31%
==========================================
Files 122 123 +1
Lines 11550 11801 +251
==========================================
+ Hits 10519 10712 +193
- Misses 651 685 +34
- Partials 380 404 +24
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: 0d76119 | Previous: d6eb497 | Ratio |
|---|---|---|---|
Benchmark_Compress/Zstd (github.com/gofiber/fiber/v3/middleware/compress) - B/op |
1 B/op |
0 B/op |
+∞ |
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)
| func (c fiber.Ctx) SaveFile(fh *multipart.FileHeader, path string) error | ||
| ``` | ||
|
|
||
| Paths must be relative and cannot contain `..` segments or absolute prefixes. When `Config.RootDir` or `Config.RootFs` is set, the path is resolved against that root and attempts to escape it are rejected. Storage keys are prefixed by the configured root when one is set. |
| } | ||
| if !hasPathPrefix(parentEval, rootEval) { | ||
| return ErrUploadPathEscapesRoot | ||
| } |
| for _, entry := range entries { | ||
| if entry.Name() == name && entry.Type()&fs.ModeSymlink != 0 { | ||
| return ErrUploadPathEscapesRoot | ||
| } | ||
| } | ||
| } |
| if errors.Is(err, fs.ErrNotExist) { | ||
| return nil | ||
| } |
| cleanOS := filepath.Clean(path) | ||
|
|
||
| cleanSlash := pathpkg.Clean("/" + filepath.ToSlash(path)) | ||
| cleanSlash = utils.TrimLeft(cleanSlash, '/') | ||
|
|
||
| if cleanOS == "." || cleanOS == "" || cleanSlash == "." || cleanSlash == "" { | ||
| return uploadPath{}, ErrInvalidUploadPath | ||
| } | ||
| if !fs.ValidPath(cleanSlash) { | ||
| return uploadPath{}, ErrInvalidUploadPath | ||
| } | ||
|
|
||
| return uploadPath{ | ||
| osPath: cleanOS, | ||
| slashPath: cleanSlash, | ||
| }, nil |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ctx.go (2)
552-558: RootFs and RootDir checks are not mutually exclusive here.In
SaveFile, theRootFsbranch returns early, but here inSaveFileToStorage, both checks run independently. This appears intentional since storage is abstract and the validation still makes sense, but it's worth confirming this asymmetry is by design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ctx.go` around lines 552 - 558, The code currently runs both RootFs and RootDir validations in SaveFileToStorage (calling ensureNoSymlinkFS for c.app.config.RootFs and the existing RootDir check), which differs from SaveFile where the RootFs branch returns early; decide and enforce one consistent behavior: either keep both checks (intentionally validate against both storage backends) or make them mutually exclusive like SaveFile. Update SaveFileToStorage to match the chosen behavior by adjusting the conditional flow around c.app.config.RootFs and c.app.config.RootDir, referencing storageUploadPath(normalized.slashPath), ensureNoSymlinkFS, and ensureNoSymlink so the function returns/continues consistently with SaveFile.
619-619: Hardcoded file permissions vs configurableRootPerms.The file is created with hardcoded
0o644permissions, whileapp.config.RootPerms(defaulting to0o750) is used for directory creation. Consider whether file permissions should also be configurable, or document thatRootPermsapplies only to directories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ctx.go` at line 619, The code currently hardcodes 0o644 for file creation in the fsys.OpenFile call (dst, err := fsys.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)) while directories use app.config.RootPerms; make file permissions configurable instead of hardcoding: add a new config field (e.g., app.config.FilePerms) with a sensible default (0o644), or explicitly document that RootPerms only applies to directories and leave as-is; then replace the hardcoded 0o644 with app.config.FilePerms (or use app.config.RootPerms if you decide files should follow directory perms), and update the config initialization, comments/docs, and any relevant tests to cover the new behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ctx_test.go`:
- Around line 5374-5377: The test currently constructs expectedPath using the
production helpers storageUploadPath and storageRootPrefix (in the
SaveFileToStorage test), which can mask regressions; change the test to build
the expected storage key directly from the documented storage-key contract
(e.g., the literal path format the storage layer should produce for rootDir and
filename) instead of calling storageUploadPath/storageRootPrefix. Update the
assertion that compares storage.setKeys (used in the test harness) to use this
hard-coded expected string so the test independently verifies SaveFileToStorage
behavior.
- Around line 4801-4808: The tests currently perform traversal assertions inside
an HTTP handler by calling c.FormFile and c.SaveFile and using
require.NoError/require.Error there; move those assertions out of the handler so
they run in the test goroutine: call the handler-equivalent API directly (e.g.,
invoke ctx.SaveFile or the exported SaveFile helper) from the test and assert
with require.ErrorIs against the exported upload/traversal error constant
instead of a generic require.Error; if you must keep an in-handler check,
replace require.* calls inside the handler with testing.T methods
(t.Fatalf/t.Errorf) to avoid linter issues, but preferred fix is converting the
test to directly call SaveFile/FormFile equivalents and assert ErrorIs against
the exported upload error symbol.
In `@ctx.go`:
- Around line 631-637: The current error handling in the io.Copy failure path
discards the original copy error when dst.Close() also fails; update the block
that handles io.Copy(writer, file) in ctx.go to preserve both errors by
capturing the copy error (err) and the close error (closeErr) and returning a
combined error (e.g., using errors.Join(copyErr, closeErr) or wrapping them
together) instead of returning only closeErr; ensure references to writer, file,
dst.Close(), and io.Copy remain and that the returned error message includes
both failure contexts.
---
Nitpick comments:
In `@ctx.go`:
- Around line 552-558: The code currently runs both RootFs and RootDir
validations in SaveFileToStorage (calling ensureNoSymlinkFS for
c.app.config.RootFs and the existing RootDir check), which differs from SaveFile
where the RootFs branch returns early; decide and enforce one consistent
behavior: either keep both checks (intentionally validate against both storage
backends) or make them mutually exclusive like SaveFile. Update
SaveFileToStorage to match the chosen behavior by adjusting the conditional flow
around c.app.config.RootFs and c.app.config.RootDir, referencing
storageUploadPath(normalized.slashPath), ensureNoSymlinkFS, and ensureNoSymlink
so the function returns/continues consistently with SaveFile.
- Line 619: The code currently hardcodes 0o644 for file creation in the
fsys.OpenFile call (dst, err := fsys.OpenFile(path,
os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644)) while directories use
app.config.RootPerms; make file permissions configurable instead of hardcoding:
add a new config field (e.g., app.config.FilePerms) with a sensible default
(0o644), or explicitly document that RootPerms only applies to directories and
leave as-is; then replace the hardcoded 0o644 with app.config.FilePerms (or use
app.config.RootPerms if you decide files should follow directory perms), and
update the config initialization, comments/docs, and any relevant tests to cover
the new behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: cc3cdbe8-2a0c-49c7-8747-373f2b9ef35e
📒 Files selected for processing (8)
app.goapp_test.goctx.goctx_test.godocs/api/ctx.mddocs/api/fiber.mddocs/whats_new.mderror.go
✅ Files skipped from review due to trivial changes (1)
- docs/api/ctx.md
🚧 Files skipped from review as they are similar to previous changes (4)
- error.go
- docs/whats_new.md
- docs/api/fiber.md
- app_test.go
| app.Post("/test", func(c Ctx) error { | ||
| fh, err := c.FormFile("file") | ||
| require.NoError(t, err) | ||
|
|
||
| err = c.SaveFile(fh, targetPath) | ||
| require.Error(t, err) | ||
| return c.SendStatus(StatusOK) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move these traversal assertions out of the handler and lock the exported error down.
These new RootDir traversal tests only check require.Error(...), so they will still pass on the wrong failure mode, and they add require calls inside Fiber handlers. Converting them to direct ctx.SaveFile* tests would keep assertions in the test goroutine and let each case assert require.ErrorIs(...) against the intended exported upload error.
Based on learnings: In the Fiber codebase, the linter does not allow require assertions from within HTTP handlers (including net/http-style handlers). Use t.Fatalf, t.Errorf, or similar testing.T methods for error handling inside handler functions instead.
Also applies to: 5024-5031
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ctx_test.go` around lines 4801 - 4808, The tests currently perform traversal
assertions inside an HTTP handler by calling c.FormFile and c.SaveFile and using
require.NoError/require.Error there; move those assertions out of the handler so
they run in the test goroutine: call the handler-equivalent API directly (e.g.,
invoke ctx.SaveFile or the exported SaveFile helper) from the test and assert
with require.ErrorIs against the exported upload/traversal error constant
instead of a generic require.Error; if you must keep an in-handler check,
replace require.* calls inside the handler with testing.T methods
(t.Fatalf/t.Errorf) to avoid linter issues, but preferred fix is converting the
test to directly call SaveFile/FormFile equivalents and assert ErrorIs against
the exported upload error symbol.
| err := ctx.SaveFileToStorage(fileHeader, "rootdir.txt", storage) | ||
| require.NoError(t, err) | ||
| expectedPath := storageUploadPath(storageRootPrefix(rootDir), "rootdir.txt") | ||
| require.Equal(t, []string{expectedPath}, storage.setKeys) |
There was a problem hiding this comment.
Keep this expected storage key independent from the production helpers.
Line 5376 builds expectedPath with storageUploadPath(storageRootPrefix(rootDir), ...), so this test can still pass if the new RootDir key-normalization logic regresses in both places. Build the expected key from the documented storage-key contract instead of calling the helpers under test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ctx_test.go` around lines 5374 - 5377, The test currently constructs
expectedPath using the production helpers storageUploadPath and
storageRootPrefix (in the SaveFileToStorage test), which can mask regressions;
change the test to build the expected storage key directly from the documented
storage-key contract (e.g., the literal path format the storage layer should
produce for rootDir and filename) instead of calling
storageUploadPath/storageRootPrefix. Update the assertion that compares
storage.setKeys (used in the test harness) to use this hard-coded expected
string so the test independently verifies SaveFileToStorage behavior.
| if _, err = io.Copy(writer, file); err != nil { | ||
| closeErr := dst.Close() | ||
| if closeErr != nil { | ||
| return fmt.Errorf("failed to close upload destination: %w", closeErr) | ||
| } | ||
| return fmt.Errorf("failed to copy upload data: %w", err) | ||
| } |
There was a problem hiding this comment.
Original copy error is lost when close also fails.
When io.Copy fails and dst.Close() also fails, only the close error is returned. The original copy error provides more diagnostic value. Consider using errors.Join or wrapping both.
Proposed fix to preserve both errors
if _, err = io.Copy(writer, file); err != nil {
closeErr := dst.Close()
if closeErr != nil {
- return fmt.Errorf("failed to close upload destination: %w", closeErr)
+ return fmt.Errorf("failed to copy upload data: %w (also failed to close: %v)", err, closeErr)
}
return fmt.Errorf("failed to copy upload data: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if _, err = io.Copy(writer, file); err != nil { | |
| closeErr := dst.Close() | |
| if closeErr != nil { | |
| return fmt.Errorf("failed to close upload destination: %w", closeErr) | |
| } | |
| return fmt.Errorf("failed to copy upload data: %w", err) | |
| } | |
| if _, err = io.Copy(writer, file); err != nil { | |
| closeErr := dst.Close() | |
| if closeErr != nil { | |
| return fmt.Errorf("failed to copy upload data: %w (also failed to close: %v)", err, closeErr) | |
| } | |
| return fmt.Errorf("failed to copy upload data: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ctx.go` around lines 631 - 637, The current error handling in the io.Copy
failure path discards the original copy error when dst.Close() also fails;
update the block that handles io.Copy(writer, file) in ctx.go to preserve both
errors by capturing the copy error (err) and the close error (closeErr) and
returning a combined error (e.g., using errors.Join(copyErr, closeErr) or
wrapping them together) instead of returning only closeErr; ensure references to
writer, file, dst.Close(), and io.Copy remain and that the returned error
message includes both failure contexts.
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.