Skip to content

🐛 bug: Fix Static middleware browser support for subdirectories #3673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ReneWerner87 merged 1 commit into from
Aug 11, 2025
Merged

🐛 bug: Fix Static middleware browser support for subdirectories #3673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/testdata/fs/css/test/style2.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
h1 {
color: black;
}
7 changes: 7 additions & 0 deletions middleware/static/static.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import (
// It returns an error if the path attempts to traverse directories.
func sanitizePath(p []byte, filesystem fs.FS) ([]byte, error) {
var s string

hasTrailingSlash := len(p) > 0 && p[len(p)-1] == '/'
Copy link

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider extracting the trailing slash check into a named constant or helper function for better readability and reusability.

Copilot uses AI. Check for mistakes.

Comment on lines +27 to +28
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. 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
  1. 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.go

Length 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 the hasTrailingSlash := … and the if hasTrailingSlash … s += “/” block
  • PathRewrite default branch (middleware/static/static.go:163–166)
    • Before you mangle path, record reqHasSlash := rawPath[len(rawPath)-1] == '/'
    • After sanitizePath(...), re-append “/” only if reqHasSlash was 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.

if bytes.IndexByte(p, '\\') >= 0 {
b := make([]byte, len(p))
copy(b, p)
Expand Down Expand Up @@ -66,6 +69,10 @@ func sanitizePath(p []byte, filesystem fs.FS) ([]byte, error) {
s = "/" + s
}

Copy link

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
// Only add a trailing slash if the path is not the root "/".
// This prevents turning "/" into "//".

Copilot uses AI. Check for mistakes.
if hasTrailingSlash && len(s) > 1 && s[len(s)-1] != '/' {
s += "/"
}

return utils.UnsafeBytes(s), nil
}

Expand Down
14 changes: 14 additions & 0 deletions middleware/static/static_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,20 @@ func Test_Static_FS_Browse(t *testing.T) {
require.NoError(t, err, "app.Test(req)")
require.Contains(t, string(body), "color")

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))

resp, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/dirfs/test/style2.css", nil))
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
require.Equal(t, fiber.MIMETextCSSCharsetUTF8, resp.Header.Get(fiber.HeaderContentType))

body, err = io.ReadAll(resp.Body)
require.NoError(t, err, "app.Test(req)")
require.Contains(t, string(body), "color")

resp, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/embed", nil))
require.NoError(t, err, "app.Test(req)")
require.Equal(t, 200, resp.StatusCode, "Status code")
Expand Down
Loading