🐛 bug: Fix Static middleware browser support for subdirectories #3673
Conversation
WalkthroughA new CSS test file was added for static file serving tests. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StaticMiddleware
participant FileSystem
Client->>StaticMiddleware: GET /dirfs/test/style2.css
StaticMiddleware->>StaticMiddleware: sanitizePath("/dirfs/test/style2.css")
StaticMiddleware->>FileSystem: Open sanitized path
FileSystem-->>StaticMiddleware: Return file contents
StaticMiddleware-->>Client: Respond with CSS file and 200 status
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 comments)
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 @efectn, 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!
I've addressed an issue in the static middleware where browsing subdirectories failed because the sanitizePath function incorrectly removed trailing slashes from paths. This fix ensures that if an original path had a trailing slash, it is preserved in the sanitized path, allowing fasthttp to correctly identify and serve content from directories. This resolves the problem of accessing index pages or files within subdirectories when a trailing slash is present in the request URL.
Highlights
- Fix for Static Middleware Subdirectory Browsing: The
sanitizePathfunction inmiddleware/static/static.gohas been updated to correctly handle paths with trailing slashes. Previously,sanitizePathwould remove trailing slashes, which caused issues withfasthttp's file system handling for subdirectories. - Preservation of Trailing Slashes: A new
hasTrailingSlashboolean is introduced to check if the original path had a trailing slash. If so, and the sanitized path is not just a root path and doesn't end with a slash, a trailing slash is re-appended. - New Test Case for Subdirectory Browsing: A new CSS file (
.github/testdata/fs/css/test/style2.css) and corresponding test cases inmiddleware/static/static_test.gohave been added to validate that static files within subdirectories are now correctly served.
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 or fill out our survey to provide feedback.
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.
Pull Request Overview
This PR fixes a subdirectory browsing issue in the static middleware where paths with trailing slashes were being stripped by sanitizePath, preventing fasthttp's filesystem handler from properly redirecting to index pages. The fix preserves the original trailing slash after path sanitization.
- Fixed trailing slash handling in
sanitizePathfunction to maintain compatibility with fasthttp filesystem redirects - Added test coverage for subdirectory browsing functionality
- Added test file to support the new test cases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
middleware/static/static.go |
Modified sanitizePath to preserve trailing slashes when present in original path |
middleware/static/static_test.go |
Added test cases for subdirectory browsing with and without trailing slashes |
.github/testdata/fs/css/test/style2.css |
Added test file to support subdirectory browsing tests |
| func sanitizePath(p []byte, filesystem fs.FS) ([]byte, error) { | ||
| var s string | ||
|
|
||
| hasTrailingSlash := len(p) > 0 && p[len(p)-1] == '/' |
There was a problem hiding this comment.
[nitpick] Consider extracting the trailing slash check into a named constant or helper function for better readability and reusability.
| } | ||
| s = "/" + s | ||
| } | ||
|
|
There was a problem hiding this comment.
The condition len(s) > 1 prevents adding a trailing slash to the root path "/", but this logic could be clearer with a comment explaining why root paths are excluded.
| // Only add a trailing slash if the path is not the root "/". | |
| // This prevents turning "/" into "//". |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3673 +/- ##
==========================================
- Coverage 91.84% 91.83% -0.02%
==========================================
Files 113 113
Lines 11447 11453 +6
==========================================
+ Hits 10514 10518 +4
- Misses 669 671 +2
Partials 264 264
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:
|
There was a problem hiding this comment.
Code Review
This pull request effectively fixes an issue with browsing subdirectories when using the static middleware. The change correctly preserves the trailing slash on directory paths by updating sanitizePath, which prevents an unnecessary redirect from fasthttp. The fix is well-implemented and includes new test cases that validate both the corrected directory browsing behavior and ensure that file serving within subdirectories remains functional. Benchmarks show no performance regression. The changes are solid and address the issue well.
![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
🧹 Nitpick comments (1)
middleware/static/static_test.go (1)
654-667: Good coverage for nested dir browse and asset; add explicit slash edge-casesThe new checks for /dirfs/test and /dirfs/test/style2.css are on point. To lock in behavior around trailing slashes, consider adding:
- Directory with explicit slash:
- GET /dirfs/test/ → 200, Content-Type text/html; charset=utf-8 (index/listing)
- File with an erroneous trailing slash (should not be treated as a directory):
- GET /dirfs/style.css/ → 404 (or a redirect if FS changes behavior in the future)
If you adopt the refactor to preserve the original trailing slash, these tests help ensure we don’t accidentally start serving files when a trailing slash is present.
Example additions:
@@ func Test_Static_FS_Browse(t *testing.T) { - require.Contains(t, string(body), "color") + require.Contains(t, string(body), "color") + // Directory explicitly with trailing slash + resp, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/dirfs/test/", nil)) + require.NoError(t, err, "app.Test(req)") + require.Equal(t, 200, resp.StatusCode, "Status code") + require.Equal(t, fiber.MIMETextHTMLCharsetUTF8, resp.Header.Get(fiber.HeaderContentType)) + // File path with trailing slash should not be served as a directory + resp, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/dirfs/style.css/", nil)) + require.NoError(t, err, "app.Test(req)") + require.Equal(t, 404, resp.StatusCode, "Status code")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/testdata/fs/css/test/style2.css(1 hunks)middleware/static/static.go(2 hunks)middleware/static/static_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: repeated
- GitHub Check: Analyse
- GitHub Check: Compare
🔇 Additional comments (1)
.github/testdata/fs/css/test/style2.css (1)
1-3: Test asset looks goodCSS fixture is simple and purpose-built for assertions. No issues.
| hasTrailingSlash := len(p) > 0 && p[len(p)-1] == '/' | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Preserve trailing slash from the original request, not from the rewritten path
PathRewrite appends a trailing slash to the working path unconditionally in the default branch. Because hasTrailingSlash is computed from p inside sanitizePath (after that rewrite), it will be true for most non-root requests, including file paths. This means sanitizePath will also append a trailing slash post-clean, producing paths like /style.css/ even when the original request was /style.css.
While fasthttp.FS appears to tolerate this (tests pass), it diverges from the stated intent “preserve the original trailing slash” and risks subtle inconsistencies or future regressions.
Recommended refactor:
- Compute whether the original request had a trailing slash before any rewriting.
- Apply the trailing-slash preservation after sanitizePath returns, or pass a flag into sanitizePath.
Pros:
- Accurately preserves client semantics.
- Avoids forcing slash on file paths.
- Keeps sanitizePath single-responsibility (validate/clean).
Cons:
- Small signature/usage change or extra logic in PathRewrite.
Suggested changes:
- Keep sanitizePath focused; remove the post-clean append there:
- hasTrailingSlash := len(p) > 0 && p[len(p)-1] == '/'
+ // trailing slash preservation is handled by the caller (based on original request)- if hasTrailingSlash && len(s) > 1 && s[len(s)-1] != '/' {
- s += "/"
- }
+ // caller may re-append a trailing slash after validation/cleaning- In PathRewrite, capture the request’s original trailing slash before mutating path and apply it after sanitizePath:
Go (outside the changed ranges; illustrative patch):
fs.PathRewrite = func(fctx *fasthttp.RequestCtx) []byte {
raw := fctx.Path() // original request path
reqHasTrailingSlash := len(raw) > 0 && raw[len(raw)-1] == '/'
path := fctx.Path()
// ... existing prefix handling and default-case adjustments ...
// (keep current logic that ensures leading slash etc.)
sanitized, err := sanitizePath(path, fs.FS)
if err != nil {
return []byte("/__fiber_invalid__")
}
if reqHasTrailingSlash && len(sanitized) > 1 && sanitized[len(sanitized)-1] != '/' {
sanitized = append(sanitized, '/')
}
return sanitized
}This ensures trailing slashes are preserved only when they existed on the client’s request.
To confirm sanitizePath isn’t used elsewhere in a way that would need updating:
Add a short comment explaining why trailing slash is preserved (fasthttp FS directory handling), and why it’s done post-clean to avoid invalid fs.ValidPath(".../").
Also applies to: 72-74
🏁 Script executed:
#!/bin/bash
# Find all sanitizePath call sites and their contexts
rg -n --no-heading "sanitizePath\\(" -A 3 -B 3
# Inspect rewrite code to verify trailing slash appending occurs pre-sanitize
rg -n --no-heading "PathRewrite\\s*=\\s*func\\(" -A 120 | sed -n '1,200p'Length of output: 8863
🏁 Script executed:
#!/bin/bash
# Display the full sanitizePath implementation for review
sed -n '1,120p' middleware/static/static.goLength of output: 2546
Preserve the client’s original trailing-slash, not the one injected by PathRewrite
Currently, PathRewrite unconditionally appends “/” whenever it strips the route prefix for directories, which makes
sanitizePath see a trailing slash on almost every non-file request—and then re-appends it post-clean. This diverges from the intent (“preserve only the original trailing slash”) and forces e.g. /style.css → /style.css/ even if the browser never asked for it.
Key locations to update:
- sanitizePath (middleware/static/static.go:27–28 & forty-seven lines later)
• Remove thehasTrailingSlash := …and theif hasTrailingSlash … s += “/”block - PathRewrite default branch (middleware/static/static.go:163–166)
• Before you manglepath, recordreqHasSlash := rawPath[len(rawPath)-1] == '/'
• AftersanitizePath(...), re-append “/” only ifreqHasSlashwas true
Suggested diffs:
--- a/middleware/static/static.go
+++ b/middleware/static/static.go
@@ func sanitizePath(p []byte, filesystem fs.FS) ([]byte, error) {
- hasTrailingSlash := len(p) > 0 && p[len(p)-1] == '/'
…
- if hasTrailingSlash && len(s) > 1 && s[len(s)-1] != '/' {
- s += "/"
- }
+ // trailing-slash preservation is handled by PathRewrite (caller) based on the original request--- a/middleware/static/static.go
+++ b/middleware/static/static.go
@@ fs.PathRewrite = func(fctx *fasthttp.RequestCtx) []byte {
- path := fctx.Path()
+ raw := fctx.Path()
+ reqHasSlash := len(raw) > 0 && raw[len(raw)-1] == '/'
path := raw
… // existing prefixLogic
// default (non-file) branch:
- if len(path) == 0 || path[len(path)-1] != '/' {
- path = append(path, '/')
- }
+ // do not append here; sanitizePath will clean, and we’ll re-append below--- a/middleware/static/static.go
+++ b/middleware/static/static.go
@@ fs.PathRewrite = func(fctx *fasthttp.RequestCtx) []byte {
sanitized, err := sanitizePath(path, fs.FS)
if err != nil {
return []byte("/__fiber_invalid__")
}
+ if reqHasSlash && len(sanitized) > 1 && sanitized[len(sanitized)-1] != '/' {
+ sanitized = append(sanitized, '/')
+ }
return sanitized
}These changes will:
• Accurately preserve only the client-provided slash.
• Keep sanitizePath focused on cleaning/validation.
• Require no other call sites to change (sanitizePath is only used here and in its own tests).
• (Optional) Add a comment on why trailing-slash logic lives in PathRewrite—so future readers won’t re-introduce it in sanitizePath.
🤖 Prompt for AI Agents
In middleware/static/static.go around lines 27-28 and 74-75, remove the code
that detects and re-appends a trailing slash in sanitizePath to stop preserving
slashes added by PathRewrite. Then, in the PathRewrite function around lines
163-166, before modifying the path, record whether the original request path had
a trailing slash in a variable like reqHasSlash. After calling sanitizePath,
re-append the trailing slash only if reqHasSlash is true. This ensures only the
client-provided trailing slash is preserved, keeping sanitizePath focused on
cleaning and validation.
gaby
changed the title
Description
This issue fixes the browsing issue of sub directories when using the static middleware. The issue arises as
sanitizePathremoves the trailing slash whereas fasthttp fs redirects to index pages only if the path has trailing slash. Fixed it by putting old trailing slash to the end of the path. https://github.com/valyala/fasthttp/blob/master/fs.go#L1105Fixes #3667
Benchmarks:
Old:
New:
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md