fix(http): normalize trailing slash in static file endpoints (#3827)

[ogMaverick12]

Description:

Fixes #3827.

App.AddStaticFiles normalized the endpoint with strings.TrimPrefix(endpoint, "/"), which strips only a leading slash. A trailing slash survived into the stored endpoint, causing AddStaticFiles("static/", dir) to register a dead route and return 404s despite logging success.

Replace strings.TrimPrefix with strings.Trim so static, /static, static/, and /static/ all normalize to /static.

Verified with a new table-driven regression test covering all four endpoint forms. The test reproduces the failure on the original implementation and passes with the fix. The relevant pkg/gofr and pkg/gofr/http test suites pass, and go vet ./pkg/gofr/ is clean.

Breaking Changes (if applicable):

None. This only changes previously-dead trailing-slash inputs from returning 404 to serving the intended static files.

Additional Information:

No new dependencies. The regression test exercises the stored endpoint through the real router and verifies both /static and /static/index.html.

Checklist:

• All new code is covered by unit tests.
• I have reviewed the code comments and documentation for clarity.
• This PR does not decrease the overall code coverage.
• No new dependencies were added.

[ogMaverick12]

Hi @aryanmehrotra — when you get a moment, would you mind taking a look? This fixes #3827, which you had flagged during the review of #3820. It's a one-line normalization change (TrimPrefix → Trim) plus a regression test covering all four endpoint forms. Thanks!

[aryanmehrotra]

Thanks for picking this up, and for the write-up — the diagnosis is exactly right, and I reproduced it both ways locally: with TrimPrefix restored your test fails (/static → 404, stored endpoint "/static/"), and with strings.Trim it passes. I also checked the root mount still works, since examples/using-html-template registers AddStaticFiles("/", "./static") — that's unchanged, and //static// now normalizes too, which was broken before as well.

One thing I'd like sorted before merge, and it's about where the fix sits rather than whether it works.

Router.AddStaticFiles is what actually builds Path(endpoint) and PathPrefix(endpoint + "/"), and it's exported — so a caller reaching it directly still gets a dead route that logs success:

endpoint "/static/"   GET /static -> 404
endpoint "static"     GET /static -> 404
endpoint "//static//" GET /static -> 404

Worth noting that your own test calls it directly, which is a good sign that it's a real entry point.

We already have the precedent for this one function down: pkg/gofr/http/router.go normalizes the other argument with filepath.Abs(dirName) for exactly this class of bug — an unclean input silently producing a route nothing matches — with Test_StaticFileServing_DirectoryNameForms covering it. The endpoint argument is the mirror image and deserves the same treatment:

func (rou *Router) AddStaticFiles(logger logging.Logger, endpoint, dirName string) {
	// The route patterns below are built from endpoint verbatim, and ServeHTTP normalizes
	// incoming paths with path.Clean — so an endpoint carrying a leading or trailing slash
	// registers a pattern no request can ever match. Normalize here, where the patterns are
	// built, so a direct caller cannot register a dead route either.
	endpoint = "/" + strings.Trim(endpoint, "/")

	absDir, err := filepath.Abs(dirName)
	...

Keep your gofr.go line as well — the comment there is right that it's what makes the error logs agree on one form. And could we mirror the test as Test_StaticFileServing_EndpointForms right next to Test_StaticFileServing_DirectoryNameForms in router_test.go? That's where this behaviour is now guaranteed.

Two practical notes for the next push, neither of them a problem with the change itself:

• The branch is behind development and the Actions workflows haven't run on it yet — only the Snyk check reported. Lets get a rebase up so we can see the full suite green.
• Adding /static to the test tips goconst over its threshold and it surfaces on factory.go:59. CI runs with only-new-issues: true so it won't block, but a shared const would be tidier.

On process: the issue had no maintainer comment agreeing the approach, and someone else had asked for it a month earlier without a PR. The fix here matches what the issue itself suggested so I'm not asking you to unwind anything — just flagging that for the next one, a quick check on the issue before opening the PR saves everyone the rework risk.

[ogMaverick12]

Done — thanks for the thorough check. Normalization now also lives in Router.AddStaticFiles (kept the gofr.go line for the log consistency), with Test_StaticFileServing_EndpointForms next to DirectoryNameForms and a shared const for the goconst nit. Rebased onto development so the full suite can run. Both new tests were verified to fail pre-fix (404s) and pass post-fix. And noted on the process point — I'll check for maintainer agreement on the issue before opening the next PR.

[ogMaverick12]

@aryanmehrotra — flagging that the requested changes are pushed and ready for another look when you have a moment. Thanks!

[aryanmehrotra]

This is exactly it — thanks for turning it around so quickly.

I re-verified rather than taking the description on trust, and everything holds up. I mutation-tested the two tests separately: reverting only the router.go normalization fails Test_StaticFileServing_EndpointForms on the bare, trailing and leading+trailing cases while the app-level test stays green; reverting only the gofr.go line fails TestAddStaticFilesEndpointForms on the stored value while the router test stays green. So neither test is carrying the other, which is what I wanted from splitting it across the two layers.

I also ran it as a real app rather than only through httptest — app.AddStaticFiles("assets/", "./static") against a running server answers 404 with {"error":{"message":"route not registered"}} before the change and 200 with the file after, and a genuine miss under the endpoint still 404s.

Checked the cases the normalization could have broken, since it now runs before the endpoint == "/" branch: the root mount still serves (examples/using-html-template depends on it), "" and "//" resolve to root, //static// and /a/b/ normalize, and /staticother is still refused against a /static endpoint — so the sibling-prefix guard from #3820 is intact.

Lint is clean too: with the const in place the finding count is identical to the base, so nothing new surfaced.

One thing outstanding on my side rather than yours — the Actions workflows haven't run on this branch yet, only Snyk. I'll get those triggered so we can see the full matrix before merging.

[ogMaverick12]

Thank you for the thorough verification — the separate mutation testing of both layers is especially reassuring. CI is fully green on my side, so please let me know if anything else is needed before merge. @aryanmehrotra

[Umang01-hash]

APPROVE — verified at head 3538ad9.

The bug is real and I reproduced it: with the old leading-only TrimPrefix, a trailing-slash endpoint registers Path("/static/") + PathPrefix("/static//"), the server logs 'registered static files at endpoint /static//' (success), yet every request 404s because ServeHTTP path.Clean's them to /static and /static/x which match neither pattern. Normalizing with strings.Trim at both entry points (gofr.go App + router.go direct caller) fixes it.

Verified:

• Revert-red: restoring TrimPrefix turns the trailing_slash and leading_and_trailing_slash subtests RED; restored -> green. The tests drive the real router.ServeHTTP asserting 200 + body, across all four endpoint forms.
• Live E2E: AddStaticFiles("static/", ...) now serves /static/hello.txt=200, /static/index.html=200, /static=200. DELETE -> 405 with Allow: GET, HEAD (method guard intact). /static/../main.go and %2e%2e -> 404, no traversal leak.
• No breaking change (signature unchanged; only previously-dead routes start working). No security impact — the endpoint normalization doesn't touch the containment guard.
• gofmt/vet/build clean; full static-file regression suites pass; golangci-lint --new-from-rev = 0 issues.

Nit: the branch (fix/cron-docs-3876) and title (#3827) are mismatched leftover naming — cosmetic.

[Umang01-hash]

Verified against head 76c8fb5. Real dead-route bug: a trailing-slash endpoint registered Path("/static/")+PathPrefix("/static//"), but ServeHTTP's path.Clean turns an incoming /static/ into /static so it matched nothing and 404'd. strings.Trim(endpoint, "/") at both registration sites fixes it. Ran locally: full pkg/gofr/http suite -race and pkg/gofr -short both green; both changed lines are mutation-pinned (reverting router.go OR gofr.go reddens a test); golangci-lint --new-from-rev clean. No exported-API break, no regression to correct /static usage.

Nit (non-code): branch is fix/cron-docs-3876 but the PR is the static-file fix #3827 — reused branch, cosmetic.

[aryanmehrotra]

Re-checked at 5e95d23da after the two development merges, and it still holds up.

• The diff against the merge base is still exactly the 4 files. The router work that came in through the merges (perf(http): cut six per-request allocations from the trie router #4165's trie matcher and markOwned) doesn't change how static routes are matched.
• Both new tests pass under the default matcher and under GOFR_ROUTER=trie. The mutation split still holds in both: reverting only router.go fails Test_StaticFileServing_EndpointForms while the app-level test stays green, and reverting only gofr.go fails TestAddStaticFilesEndpointForms while the router test stays green.
• pkg/gofr and pkg/gofr/http/... pass, go vet is clean, and gofmt is clean.

One optional follow-up, not a blocker. The new comment in Router.AddStaticFiles describes the general invariant: an unclean endpoint registers a pattern no request can match. strings.Trim only covers the leading and trailing slash cases of that. These still register a dead route that logs success:

endpoint "a//b"          GET /a/b/index.html    -> 404
endpoint "./static"      GET /static/index.html -> 404
endpoint "static/./"     GET /static/index.html -> 404
endpoint "/a/../static"  GET /static/index.html -> 404

Using the same path.Clean that ServeHTTP applies to the request makes both sides agree by construction:

endpoint = path.Clean("/" + endpoint)

I ran that change locally. All four cases above answer 200 under both matchers. "", "//" and "/" still resolve to the root mount, and the existing pkg/gofr and pkg/gofr/http suites stay green. If you'd rather keep this PR scoped to the issue as filed, that's fine. It works as a small follow-up too, with those four rows added to Test_StaticFileServing_EndpointForms.

On CI: the pipeline passed on 3538ad9d1 and 76c8fb53f. The runs on the latest merge are waiting for maintainer approval because the PR comes from a fork. I'll get them approved, and I'm good to approve the PR once they're green.

[ogMaverick12]

Hi @aryanmehrotra @Umang01-hash — quick status update from my side, requesting a final look/merge when you have a moment. All review feedback is addressed, nothing new outstanding that I can see:

1. Code: App.AddStaticFiles (pkg/gofr/gofr.go) normalizes with "/" + strings.Trim(endpoint, "/"), and Router.AddStaticFiles (pkg/gofr/http/router.go) normalizes at the pattern-building site too, so direct callers cannot register a dead route. TestAddStaticFilesEndpointForms + Test_StaticFileServing_EndpointForms cover all four endpoint forms with a shared const (goconst-clean), verified to fail pre-fix and pass post-fix. Branch is up to date with development (0 behind).

2. Local verification: go vet ./pkg/gofr/ clean; both new tests pass; full pkg/gofr/http suite shows only pre-existing Windows/Go-1.27 environment failures (JSON error-string wording, mode-0000 permission bits, path-separator cases) — the Sept 8 full-matrix run (14 jobs) and the Sept 25 re-run of this head were both green on Linux CI.

3. CI: the Sept 25 merge-head run (5e95d23) shows success (14/14 jobs) but is not linked back to the PR checks (only Snyk reports), so the PR still shows BLOCKED. I just pushed an empty trigger commit (842431d, no code change) so a fresh pull_request run is queued — it is currently action_required (fork-approval gate, 0 jobs). Could someone please approve the workflow run from the Actions tab? Once that goes green, this should be ready to merge. Thanks!

[ogMaverick12]

Hi @aryanmehrotra @Umang01-hash — just confirming from my side that all of the requested changes are in (router-level normalization, mirrored endpoint-forms test, shared const, and rebase), with CI green on this code. I don't see anything left outstanding on my end. Please let me know if you'd like anything else adjusted, otherwise this should be ready to merge whenever suits you. Thanks again for the thorough reviews!

[aryanmehrotra]

Re-checked at d0eee521f. The two commits since 5e95d23da are an empty CI trigger and a development merge; the PR's own 4-file diff is unchanged. Both new tests pass under the default matcher and GOFR_ROUTER=trie, and undoing either layer's fix still fails only its own test. CI green. Approving. The path.Clean follow-up from my last comment is still optional.