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.