🐛 bug: Handle invalid path in filesystem #3688
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughExpands filesystem middleware error handling: treats fs.ErrInvalid like fs.ErrNotExist, attempts NotFoundFile fallback on either, and returns 404 for ErrInvalid. Tests add a new route using http.FS with os.DirFS and verify invalid path returns 404. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant M as Filesystem Middleware
participant FS as fs.FS
C->>M: HTTP GET /path
M->>FS: Open(path)
alt Open returns file
FS-->>M: file handle
M-->>C: Serve file (200)
else ErrNotExist or ErrInvalid
alt cfg.NotFoundFile set
M->>FS: Open(cfg.NotFoundFile)
alt Open succeeds
FS-->>M: file handle
M-->>C: Serve NotFoundFile (200)
else error
M-->>C: 404 Not Found
end
else no NotFoundFile
M-->>C: 404 Not Found
end
else Other error
M-->>C: Error (wrapped)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @rokostik, 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 addresses a bug in the v2 filesystem middleware where accessing an invalid path using os.DirFS would result in a HTTP 500 Internal Server Error instead of the expected HTTP 404 Not Found. The changes ensure that fs.ErrInvalid errors are correctly handled, leading to a 404 response for such cases, improving the robustness and user experience of the filesystem middleware.
Highlights
- Improved Error Handling for Invalid Paths: The filesystem middleware now explicitly checks for
fs.ErrInvalidin addition tofs.ErrNotExistwhen attempting to open files. This ensures that requests for invalid paths correctly return a 404 Not Found status. - Enhanced
NotFoundFileLogic: The logic for serving aNotFoundFilehas been updated to also considerfs.ErrInvaliderrors, providing a more consistent fallback mechanism. - New Test Case for Invalid Paths: A dedicated test case has been added to verify that invalid paths correctly trigger a 404 response, confirming the bug fix.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly handles invalid paths in the filesystem middleware by treating fs.ErrInvalid similar to fs.ErrNotExist, thus returning a 404 status instead of a 500 internal server error. The change is accompanied by a relevant test case that validates the fix. The implementation is sound. I have one suggestion regarding code duplication that could improve maintainability.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
middleware/filesystem/filesystem.go (2)
226-233: Align SendFile error handling with middleware (treat fs.ErrInvalid as Not Found).SendFile still returns 500 on fs.ErrInvalid (e.g., using http.FS(os.DirFS(...)) with an invalid path). For consistency with the middleware and to avoid surprising 500s, treat fs.ErrInvalid the same as fs.ErrNotExist.
Apply:
func SendFile(c *fiber.Ctx, filesystem http.FileSystem, path string) error { file, err := filesystem.Open(path) if err != nil { - if errors.Is(err, fs.ErrNotExist) { + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrInvalid) { return fiber.ErrNotFound } return fmt.Errorf("failed to open: %w", err) }
147-149: Minor DRY: Extract a helper for “not found” error checks.You repeat the ErrNotExist/ErrInvalid checks twice. A tiny helper improves readability and keeps behavior consistent.
Use the helper in both places:
- if err != nil && (errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrInvalid)) && cfg.NotFoundFile != "" { + if err != nil && isMissingOrInvalid(err) && cfg.NotFoundFile != "" { file, err = cfg.Root.Open(cfg.NotFoundFile) } } if err != nil { - if errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrInvalid) { + if isMissingOrInvalid(err) { return c.Status(fiber.StatusNotFound).Next() }And add this helper in the file (e.g., near other top-level helpers):
func isMissingOrInvalid(err error) bool { return errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrInvalid) }Also applies to: 151-153
middleware/filesystem/filesystem_test.go (1)
124-128: Nice: Asserts 404 on invalid path under os.DirFS-backed route. Add one more test to cover SPA fallback on ErrInvalid.You already cover SPA fallback for ErrNotExist via /spatest. To exercise the new ErrInvalid fallback path, also test that NotFoundFile is served when the path is invalid under http.FS(os.DirFS(...)).
Consider adding:
// New route using http.FS(os.DirFS(...)) with SPA fallback enabled app.Use("/spaosfs", New(Config{ Root: http.FS(os.DirFS("../../.github/testdata/fs")), Index: "index.html", NotFoundFile: "index.html", })) // And a table test entry: { name: "Should return index.html for invalid path under http.FS(os.DirFS(...))", url: "/spaosfs/](//invalid", statusCode: 200, contentType: "text/html", },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
middleware/filesystem/filesystem.go(1 hunks)middleware/filesystem/filesystem_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/filesystem/filesystem_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/filesystem/filesystem_test.go
🧬 Code Graph Analysis (1)
middleware/filesystem/filesystem_test.go (1)
middleware/filesystem/filesystem.go (2)
New(81-220)Config(17-63)
🔇 Additional comments (4)
middleware/filesystem/filesystem.go (2)
147-149: Good: NotFoundFile fallback now triggers on fs.ErrInvalid too.This closes the gap for http.FS(os.DirFS(...)) where invalid paths return fs.ErrInvalid. Solid fix.
151-153: Correct: Map fs.ErrInvalid to 404 (Not Found).Returning 404 instead of 500 for invalid paths matches user expectation and aligns with ErrNotExist handling.
middleware/filesystem/filesystem_test.go (2)
8-8: Import addition is appropriate."os" is required for os.DirFS; all good.
44-47: Good coverage: route backed by http.FS(os.DirFS(...)).This reproduces the real-world scenario that previously yielded a 500 and validates the fix.
gaby
changed the title
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
4 participants
Description
When using
os.DirFSas the filesystem root and accessing an invalid path the server return a HTTP status 500. This PR changes this to return 404.Minimal example to reproduce the issue:
Run and navigate to
http://localhost:3000/](//invalidwhich causes an internal error.Type of change