Preserve the query string on the static directory redirect

[januththedev]

Preserve the query string on the static directory redirect

Description

StaticDirectoryHandler builds the "directory without trailing slash → add trailing slash" 301 redirect from c.Request().URL.Path only:

p = c.Request().URL.Path
if fi.IsDir() && len(p) > 0 && p[len(p)-1] != '/' {
    return c.Redirect(http.StatusMovedPermanently, sanitizeURI(p+"/"))
}

URL.Path excludes the query, so the redirect target silently loses it.

Why it's wrong

• Go's own net/http.localRedirect — used by http.FileServer, http.ServeMux and http.ServeFile — explicitly appends r.URL.RawQuery: if q := r.URL.RawQuery; q != "" { newPath += "?" + q }. (Citing src/net/http/fs.go, func localRedirect, rather than a line range, since those lines drift between Go releases.)
• Echo's own AddTrailingSlash/RemoveTrailingSlash middleware already does this correctly (middleware/slash.go:64-70 builds uri += "?" + qs), so the static handler is inconsistent with the framework's own convention.
• It is a 301 MovedPermanently, which clients cache indefinitely. So GET /folder?v=2 becomes a permanent redirect to /folder/, and every subsequent request is served the wrong or an unparameterized variant. The damage is sticky, not transient.
• The impact reaches Echo.Static, Echo.StaticFS, Group.Static and Group.StaticFS — all four funnel through StaticDirectoryHandler.

This is the directory-redirect branch only; the middleware/static.go variant does not redirect at all, so it is unaffected.

The fix

uri := p + "/"
// Keep the query string on the redirect target, as the query is still part of what the client asked
// for. `net/http.localRedirect` (used by `http.FileServer`) does the same, and dropping it here turns
// e.g. `GET /folder?v=2` into a permanent redirect to `/folder/`, which clients cache and then serve
// the wrong (or no) variant for.
if q := c.Request().URL.RawQuery; q != "" {
    uri += "?" + q
}
return c.Redirect(http.StatusMovedPermanently, sanitizeURI(uri))

sanitizeURI still runs over the whole assembled URI, so the open-redirect guard is preserved. Worth answering the obvious objection directly: sanitizeURI only percent-encodes C0 control characters and DEL via escapeControlChars, and collapses a leading //, \\ or \/ — it never parses or unescapes the URI, so it cannot rewrite, reorder or drop the query. It is safe to run over the assembled path + "?" + query.

Tests

One case appended to the existing TestEcho_StaticFS table, asserting a 301 whose Location is /folder/?foo=bar&baz=1 for a request to /folder?foo=bar&baz=1.

• Before: Not equal: expected "/folder/?foo=bar&baz=1" actual "/folder/". After: passes.
• Full suite go test ./... -count=1: baseline on unmodified HEAD was 660 top-level PASS / 1709 subtests PASS, 0 FAIL; after the change 660 / 1710 / 0 FAIL — exactly one new passing subtest, zero regressions. go vet ./... clean.
• The existing TestEcho_StaticDirectoryRedirect_controlCharacters and the GHSA-3pmx-cf9f-34xr path-traversal cases still pass, confirming the security guard is intact.

I checked the open issues and PRs on labstack/echo and searched the static-handler area: no existing report or PR. The nearest open static PR is #2991 (HTML5 route fallback), unrelated.

Several real neighbouring defects are already being worked on upstream and I left them alone: the router panic on \: + :param at the same position (#3111/#3113), Context.FileFS nil-deref when Stat errors (#3115), the BodyLimit bypass on a single oversized Read (#3071, PRs #3072/#3087/#3092), gzip compressing when q=0 is requested (#3021), and Gzip.Write returning buffer length rather than len(b) (#2981). randomString's apparent uint8 overflow is unreachable — randomStringMaxByte is 207, so the counter can never wrap.

[zyzen]

Thanks — this looks right to me. I independently checked the claims rather than just reading the diff, and I have a few concrete notes.

Verified

• net/http does append the query — src/net/http/fs.go, func localRedirect (current master): if q := r.URL.RawQuery; q != "" { newPath += "?" + q }. ✅ The cited line range (777–783) has since drifted (it is ~787–799 now) — worth dropping the line numbers or adding @<sha> so the reference doesn't rot.
• The patch matches echo.go on master: the pre-change line is return c.Redirect(http.StatusMovedPermanently, sanitizeURI(p+"/")) (currently line 699), and c.Redirect( occurs only once in that file — so this branch is indeed the only place in echo.go where a redirect could drop the query.
• All entry points reach this branch: StaticDirectoryHandler is called from e.Static and e.StaticFS (echo.go lines 635 and 649), so Echo.* and Group.* static registrations all funnel through the code you patched. ✅
• sanitizeURI is safe on the assembled path?query (this was my first question when reading the diff): on master it only (a) percent-encodes C0 control characters / DEL via escapeControlChars and (b) collapses a leading //, \\ or \/ as its open-redirect guard. It does not parse or unescape the URI, so it cannot rewrite, reorder or drop your query. Might be worth one sentence in the description — it's the obvious "but what about the sanitizer?" objection.
• Test coverage: the expected Location: /folder/?foo=bar&baz=1 is consistent with localRedirect's behaviour. And because the pre-existing case still expects /folder/ (no ?) for a request without a query, the empty-query branch is covered by the existing table — nice.

Not verified by me (so I'm not claiming it)

• The test counts in the description (660 top-level / 1709 subtests, 0 FAIL afterwards) — that's your run; I didn't build or run the suite.
• The middleware/slash.go citation: fetching that path from my machine returned a 404 (tooling/network on my side), so I'm neither confirming nor disputing those exact lines. The consistency argument is sound regardless, since a static handler that redirects should behave like the framework's own trailing-slash middleware.

One CI note

The PR currently reports mergeable_state: unstable, which usually means a check is pending or failing — probably worth a glance before merging.

LGTM otherwise: small, well-scoped, matches the behaviour of the standard library it's mirroring, and the failure mode it fixes (a permanent, client-cached redirect quietly losing parameters) is exactly the kind of thing that stays broken for years.

[januththedev]

Thanks — that is a much better review than I expected, and all three points were worth making. I've updated the description:

• Dropped the drifting line range. Now cites src/net/http/fs.go, func localRedirect and quotes the statement itself (if q := r.URL.RawQuery; q != "" { newPath += "?" + q }), which won't rot across Go releases.
• Added the sanitizeURI note — yours, as the answer to the obvious objection, since I'd left it implicit.
• CI: mergeable_state is UNSTABLE because no checks have ever run on this branch. Both workflows (Run Tests, Run checks) are sitting at action_required rather than pending, so they need a maintainer to approve the run — nothing is failing.

One thing I did not verify and want to be honest about: I also cited middleware/slash.go:64-70, which is the same class of drifting line reference you flagged. I left it because I couldn't confirm it on my side either — but by your own argument it should go too, so say the word and I'll convert it to a function-level citation.

Thank you also for confirming c.Redirect( occurs only once in echo.go. I had asserted the directory-redirect branch was the only affected site, but I had only read around the patch; you checked it properly, which is the kind of thing I'd rather take from you than have a reviewer discover later.