fix(http): respond with 405 Method Not Allowed for a wrong method on a registered path

[AbhiPra24]

Fixes #3853.

A request for a path that is registered, but not for the method used, now answers 405 with an Allow header instead of 404.

POST /thing    -> 405  Allow: GET, OPTIONS   {"error":{"message":"method not allowed"}}
GET  /nope     -> 404                        {"error":{"message":"route not registered"}}

How

• The PathPrefix("/") catch-all stays a real route and decides 404 vs 405 itself, using the new Router.AllowedMethods.
• It is deliberately not mux's NotFoundHandler / MethodNotAllowedHandler: gorilla/mux v1.8.1 applies the middleware chain only to a matched route (MatchErr == nil), so responses served from those fields skip Tracer, Logging, CORS and Metrics. The CORS preflight to a GET-only path came back 405 with no CORS headers, and every 404/405 vanished from logs, metrics and traces.
• Allow lists OPTIONS because the CORS middleware answers it on every path. It is listed once even when a route registers it.
• AllowedMethods does not walk the routes. mux has already scanned all of them to reach the catch-all, so it narrows the request path through the route index and matches only the routes registered under it. At 1000 routes an unregistered path costs 187 ns in that lookup, against 16031 ns for a second scan.
• Route registration order is unchanged (Methods().Path()).

Behaviour to know about

• ErrorMethodNotAllowed is a new predefined error, documented with the others.
• This changes a status code clients can see. A client that treats the 404 envelope as the answer to a wrong method will now get 405. Wrong HTTP method on a registered route returns 404, not 405 #3853 asks for it to go out in a minor release rather than a patch.
• The route index is built once. With the default mux matcher, a route registered after the first unmatched request is still served, but a wrong method on it answers 404 — what it did before this PR.
• With GOFR_ROUTER=trie, a method-restricted subrouter that carries its own NotFoundHandler answers 405 where the mux matcher answers 404 from that handler. The two matchers already differed on that request; it needs a route built on the embedded mux.Router.

Not in this PR

The os.Root static-file containment that earlier revisions carried for #3855 has been taken out, and #3855 is closed as not planned. Static file serving is unchanged from development.

@AbhiPra24's original commit is kept as the first commit.

[aryanmehrotra]

Thanks @AbhiPra24 — I ran this against a real server rather than reading the diff, and both bugs are real and both of your fixes work. There is one regression that blocks it, and it is not visible from the diff or from the unit tests.

Both issues reproduce on development

#3853  POST /only-get        -> 404, no Allow header       (should be 405)
#3855  GET /static/evil.txt  -> 200, body "TOPSECRET"      (file outside the served root)

Your fixes do what they claim

probe	development	this PR
POST /only-get	404	405, Allow: GET
DELETE /ping	404	405, Allow: GET, POST
GET /static/evil.txt	200 TOPSECRET	404
GET /static/evildir/passwords.txt	200	404
dotdot-inside.txt (relative .. landing back inside)	200	200 — correctly still served

Same results under GOFR_ROUTER=trie, so the two matchers stay consistent. The Allow values are right, and the symlink containment does not over-reject.

---

Blocking: CORS preflight now returns 405

OPTIONS /only-get   Origin: http://x.test   Access-Control-Request-Method: GET

development:  HTTP/1.1 200 OK
              Access-Control-Allow-Methods: GET, POST, OPTIONS
              Access-Control-Allow-Origin: *

this PR:      HTTP/1.1 405 Method Not Allowed
              Allow: GET
              (no CORS headers at all)

Every cross-origin browser call to a GoFr app breaks — the preflight fails, so the browser never sends the real request.

Why, from the pinned gorilla/mux v1.8.1 (mux.go:138-168): the middleware chain is applied only in the matched-route branch, guarded by match.MatchErr == nil:

for _, route := range r.routes {
    if route.Match(req, match) {
        if match.MatchErr == nil {                       // <- middleware only here
            for i := len(r.middlewares) - 1; i >= 0; i-- {
                match.Handler = r.middlewares[i].Middleware(match.Handler)
            }
        }
        return true
    }
}

if match.MatchErr == ErrMethodMismatch {
    if r.MethodNotAllowedHandler != nil {
        match.Handler = r.MethodNotAllowedHandler        // <- no middleware
        return true
    }
    ...
}
if r.NotFoundHandler != nil {
    match.Handler = r.NotFoundHandler                    // <- no middleware
    ...
}

middleware.CORS is registered through router.Use(...) (pkg/gofr/http_server.go:67-70, alongside Tracer, Logging and Metrics), so anything served by those two handler fields bypasses all four.

Blocking: 404 and 405 disappear from logs and metrics

Same root cause, and it also hits 404 — which previously went through PathPrefix("/"), a real route, and therefore had the full chain. Measured on the same app:

	development	this PR
app_http_response_count{path="/nope",status="404"}	1	absent
app_http_response_count{path="/only-get"}	1	absent
access-log lines for /nope	3	0
access-log lines for /only-get	2	0

Only the /ping 200 is still counted. A scanner walking unknown paths, or a client using the wrong verb, becomes completely invisible to logs, metrics and traces.

A shape that keeps the middleware

Rather than moving to mux's handler fields, keep the catch-all as a real route and decide inside it:

a.httpServer.router.PathPrefix("/").Handler(handler{
    function:  catchAllHandler,   // 405 + Allow when AllowedMethods(r) is non-empty, else 404
    container: a.container,
})

AllowedMethods already skips routes with no methods, so the catch-all does not report itself. Because the catch-all matches, match.MatchErr stays nil, the whole chain runs, and CORS still intercepts OPTIONS before the handler is reached. Your AllowedMethods, ErrorMethodNotAllowed and the Allow formatting all carry over unchanged — this only changes where the decision is made.

If you go that way, worth re-checking whether the Path(...).Methods(...) reorder in Router.Add is still needed, since ErrMethodMismatch would no longer be what drives the 405.

---

Smaller things

An unrelated line was deleted. gofr.go drops:

if a.container.Logger != nil {
    a.container.Logger.Infof("Registered HTTP server on port: %d", a.httpServer.port)
}

Confirmed by running both: 1 such line on development, 0 here. It sits right where the catch-all was replaced, so it looks accidental.

Two new lint failures, both in the new test:

pkg/gofr/http/router_test.go:1007:21  G306: Expect WriteFile permissions to be 0600 or less (gosec)
pkg/gofr/http/router_test.go:1011:21  G306: (same)

Allow omits HEAD and OPTIONS. Allow: GET while OPTIONS /only-get is answered 405. RFC 9110 §10.2.1 has Allow list what the resource supports; fixing the CORS path will change this anyway, so it is worth settling both together.

On #3855, this departs from what the issue specified. The issue rules out this approach by name:

Adding filepath.EvalSymlinks before the comparison would close the common case but reintroduces the TOCTOU window os.Root exists to eliminate — the link can be repointed between the resolve and the open.

Being fair to your version: the residual window needs write access inside the served tree, which is a far higher bar than the original bug, and EvalSymlinks avoids the breaking change os.Root brings (an absolute symlink pointing inside the root keeps working, where os.Root would reject it). That may well be the better trade — but it is a deliberate divergence from the design the issue landed on, and it should be argued in the PR description rather than left implicit.

Two unrelated issues in one PR. #3853 is self-contained and, once the middleware regression is fixed, ready. #3855 has an open design question. Splitting them would let the 405 work merge without waiting on that discussion.

---

What I ran

check	result
go build ./...	clean
go test ./pkg/gofr/http/...	pass
go test ./pkg/gofr	fails identically on development (metrics-port FATAL) — not this PR
gofmt -l	2 dirty files, both pre-existing
golangci-lint --new-from-rev	2 new, listed above

The unit tests all pass, which is worth saying plainly: nothing in them exercises CORS, logging or metrics on a 404/405 path, which is why an end-to-end run was the only thing that surfaced it. A test asserting that a 404 still carries Access-Control-Allow-Origin would have caught it and would be worth adding alongside the fix.

Good work on both root causes — the symlink reproduction in particular is a real vulnerability and the containment logic you wrote handles the ..-back-inside case correctly, which is the part that is easy to get wrong.

[Umang01-hash]

Nice fixes — 405+Allow and the os.Root symlink containment both check out (verified live: escaping abs/rel symlinks 404, 405 flows through the metrics middleware). One thing to address before merge, plus a few small notes.

MEDIUM: catchAll runs AllowedMethods on every unmatched request, including pure 404s. mux already walked all routes to reach the catch-all, so this is a second full O(routes) walk — 404 cost ~doubles (measured ~32µs/req at 1000 routes, all attacker-reachable). Please collect the Allow list without a second walk, or skip AllowedMethods when it isn't a method mismatch, so a 404 flood doesn't pay twice.

[Umang01-hash]

NIT: AllowedMethods is new permanent exported API on http.Router (forced by the cross-package call from catchAll). Additive/non-breaking, just flagging that it widens the public surface — fine if intended.

[aryanmehrotra]

Intended, and yes it is permanent surface. catchAll lives in pkg/gofr and the router in pkg/gofr/http, so it has to be exported to be called; the alternative was moving the 404/405 decision into the http package, which would have put handler construction (it needs the container) on the wrong side of that boundary. Leaving this thread open for you to close if you are fine with it.

[aryanmehrotra]

Pushed three commits on top of 6daf48a68: a merge of development (one conflict, in router.go, with the endpoint normalisation from #4149), 2e47deb70 and bc9bdd350. Replies are on the review threads. Two things from going over the whole diff again that are not on any thread:

os.Root needs read permission on directories, not only search

This is a behaviour change against development, it comes with the containment, and it was not documented until bc9bdd350.

os.Root opens the served directory and each directory on the way to a file for reading. os.Open on the joined path only had to traverse them. So a directory the process can enter but not list — mode 0711 owned by another user — behaves differently:

Request	development	this PR
file under a 0311 subdirectory	200	403
served root made 0311 after registration	200	403
served root 0311 at registration	200	endpoint not registered, error logged, 404

Measured on darwin; os/root_unix.go is shared with Linux but I have not run it there. Test_StaticFileServing_UnlistableDirectory pins the subdirectory row, and the static-files page now says read permission is required.

Known difference left in place: trie matcher, method-restricted subrouter with its own NotFoundHandler

sub := r.PathPrefix("/nf").Methods("GET").Subrouter()
sub.NotFoundHandler = custom
sub.Path("/x").Handler(h)

POST /nf/unknown answers 404 from custom with the mux matcher, and 405 Allow: GET, OPTIONS with GOFR_ROUTER=trie. The two matchers already disagreed on this request before the PR (404 from custom against 404 route not registered); the PR turns a body difference into a status difference. It needs a route built on the embedded mux.Router rather than through App, and the opt-in matcher. Not fixed here.

Local test coverage

pkg/gofr/http/... passes with -race, and with GOFR_ROUTER=trie. In pkg/gofr, 337 top-level tests pass in each matcher; 14 were skipped on my machine because another process holds ports 2121 and 8000 (TestInitMetricsServer_DefaultPort and the TestGraphQL_* / TestApp_setupGraphQL* family). Those are covered by CI only.

[aryanmehrotra]

Scope change in f9432bd7b: the os.Root static-file containment is out of this PR, and #3855 is closed as not planned (reasoning is on the issue). Static file serving is byte-for-byte what development has; router.go and router_test.go were rebuilt from development with only the 405 changes applied.

That supersedes the os.Root section of my previous comment and the static-files doc paragraph it mentions. The trie/subrouter note and the review-thread replies still stand.

What is left is #3853 only: the 405 with Allow, the index-backed AllowedMethods, the OPTIONS de-duplication, and the ErrorMethodNotAllowed doc entry. Title and description are updated to match.

[aryanmehrotra]

Force-pushed to correct the author on my four commits (a stray local git identity). No content change: the tree at the new head c4182aed1 is identical to the previous head f9432bd7b.

SHAs quoted in the review threads map as follows:

• 2e47deb70 → 3ec32ecb9 (index-backed AllowedMethods)
• bc9bdd350 → 66dd2ac3c (OPTIONS listed once, docs)
• f9432bd7b → c4182aed1 (static-file containment removed)

[aryanmehrotra]

Approving at c4182ae.

[Umang01-hash]

Verified locally at head (622e2a6): wrong method → 405 with a correct Allow, unknown path → 404, bare and preflight OPTIONS still answered by CORS, and the full middleware chain (CORS/logging/tracer/metrics) runs for the catch-all. AllowedMethods is race-clean under 200 concurrent calls in both matcher modes, and the encoded-path union doesn't over-produce false 405s. Nicely done, and the tests are excellent.

One question, not blocking: HEAD on a GET-only route now returns 405 (Allow: GET, OPTIONS) where it was 404 before, and Allow omits HEAD since GoFr doesn't auto-register it for GET. Is 405-without-HEAD the intended answer, or should HEAD ride along with GET? Fine either way — just flagging the behavior change.

[Umang01-hash]

LGTM, nicely done. I verified the load-bearing gorilla/mux claims against the v1.8.1 source myself: middleware builds only on a matched route (mux.go:141), and a later route clears ErrMethodMismatch (route.go:76/87) — so using a real catch-all route (not mux's handlers) and re-scanning via AllowedMethods is the right call. Real-server E2E: POST /thing -> 405 Allow: GET, OPTIONS with the envelope, GET /nope -> 404, and both show up in the structured logs with a trace_id (the whole point — 404/405 now run the full chain). Revert-to-red confirms the 405 tests fail without the fix; -race clean in both router modes; AllowedMethods returns the exact method set across params/regex/host/header/subrouter/multi-method.

One thing to make sure reaches the release notes: 404 -> 405 for wrong-method-on-registered-path is a client-visible status change (RFC-correct, and #3853 asks for a minor release). One tiny inline nit.

[Umang01-hash]

Nit: AllowedMethods sorts internally, but appending OPTIONS after the sort leaves the header unsorted — a PATCH-only path returns Allow: "PATCH, OPTIONS" (reproduced on a real server). RFC 9110 treats Allow as an unordered set so it's cosmetic, but it undoes the sort.Strings intent. Easy fix: append OPTIONS before sorting, or sort after.