From fd7eec877cfa06cba87c708dfa1cb1a7a09662dc Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sat, 13 Jun 2026 12:27:54 -0700 Subject: [PATCH 1/4] test: guard group catch-all against 405-masking and route shadowing Adds regression tests around automatic group catch-all (404) route registration. v5 removed that auto-registration; if it is restored (e.g. PR #2996) these tests ensure it does not bring back the issues that motivated the removal: - wrong HTTP method on an existing group route must still return 405 (with Allow header), not be masked to 404 by the catch-all; - the group catch-all must not shadow concrete sibling routes, root routes, or other sibling groups' routes. All four pass on current master (v5). The 405 test fails against the restore-v4-behavior approach in #2996, pinning down that tradeoff. Co-Authored-By: Claude Opus 4.8 (1M context) --- group_autoregister_regression_test.go | 89 +++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 group_autoregister_regression_test.go diff --git a/group_autoregister_regression_test.go b/group_autoregister_regression_test.go new file mode 100644 index 000000000..24be12e43 --- /dev/null +++ b/group_autoregister_regression_test.go @@ -0,0 +1,89 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors + +package echo + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +// These tests guard the behavior re-introduced by restoring automatic group +// catch-all (404) route registration (PR #2996). v5 originally removed that +// registration because the implicit catch-all could: +// 1. mask 405 Method Not Allowed as 404, and +// 2. shadow sibling/root routes. +// Restoring auto-registration must NOT bring those regressions back. + +// passthroughMW is a no-op middleware that forces a group to register its +// implicit catch-all routes (a group only auto-registers when it has middleware). +func passthroughMW(next HandlerFunc) HandlerFunc { + return func(c *Context) error { return next(c) } +} + +// 1. Wrong method on an existing group route must still return 405 (with Allow +// header), not be swallowed as 404 by the group's catch-all. +func TestGroup_autoCatchAll_wrongMethodStillReturns405(t *testing.T) { + e := New() + g := e.Group("/api", passthroughMW) + g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) + + req := httptest.NewRequest(http.MethodPost, "/api/users", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusMethodNotAllowed, rec.Code, + "POST to a GET-only group route must be 405, not masked to 404 by the catch-all") + assert.Contains(t, rec.Header().Get(HeaderAllow), http.MethodGet, + "405 response must advertise allowed methods") +} + +// 2. The group's catch-all must not shadow a concrete sibling route under the +// same prefix: a matched route returns its own handler, not the 404 catch-all. +func TestGroup_autoCatchAll_doesNotShadowConcreteSiblingRoute(t *testing.T) { + e := New() + g := e.Group("/api", passthroughMW) + g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) + g.GET("/health", func(c *Context) error { return c.String(http.StatusOK, "health") }) + + for path, want := range map[string]string{"/api/users": "users", "/api/health": "health"} { + status, body := request(http.MethodGet, path, e) + assert.Equal(t, http.StatusOK, status, "concrete route %s must win over the group catch-all", path) + assert.Equal(t, want, body, "concrete route %s must run its own handler", path) + } + + // Only a genuinely unmatched path under the prefix hits the catch-all (404). + status, _ := request(http.MethodGet, "/api/nope", e) + assert.Equal(t, http.StatusNotFound, status, "unmatched path under the prefix should hit the catch-all 404") +} + +// 3. The group's catch-all (prefixed) must not shadow routes outside the group, +// including root-level routes. +func TestGroup_autoCatchAll_doesNotShadowRootRoute(t *testing.T) { + e := New() + e.GET("/health", func(c *Context) error { return c.String(http.StatusOK, "root-health") }) + g := e.Group("/api", passthroughMW) + g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) + + status, body := request(http.MethodGet, "/health", e) + assert.Equal(t, http.StatusOK, status, "root route must be unaffected by a group's catch-all") + assert.Equal(t, "root-health", body) +} + +// 4. Two sibling groups must not shadow each other's routes via their catch-alls. +func TestGroup_autoCatchAll_siblingGroupsDoNotShadow(t *testing.T) { + e := New() + v1 := e.Group("/api/v1", passthroughMW) + v1.GET("/ping", func(c *Context) error { return c.String(http.StatusOK, "v1") }) + v2 := e.Group("/api/v2", passthroughMW) + v2.GET("/ping", func(c *Context) error { return c.String(http.StatusOK, "v2") }) + + for path, want := range map[string]string{"/api/v1/ping": "v1", "/api/v2/ping": "v2"} { + status, body := request(http.MethodGet, path, e) + assert.Equal(t, http.StatusOK, status, "%s must resolve to its own group handler", path) + assert.Equal(t, want, body) + } +} From be357cdcab9b46c8fe791728d2c1405175bd0f25 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sat, 13 Jun 2026 12:35:32 -0700 Subject: [PATCH 2/4] test: rewrite group method-handling tests after review Addresses review of the original regression file (PR #3003): its comments described an automatic group catch-all "triggered by middleware" that does not exist on master (v5), so the tests passed for the wrong reason and the no-op middleware was inert. Rewrite to assert v5's actual, verified behavior: - method mismatch on a group route -> 405 with full Allow header - OPTIONS on a registered group route -> 204 with Allow (preflight-relevant) - concrete routes resolve; group prefix does not affect root routes The 405 and OPTIONS tests are real gates: a group-level catch-all (manual or the auto-registration proposed in #2996) masks both as 404, which these tests would then catch. Drops the false premise, the inert middleware, and the in-comment PR reference. Co-Authored-By: Claude Opus 4.8 (1M context) --- group_autoregister_regression_test.go | 89 --------------------------- 1 file changed, 89 deletions(-) delete mode 100644 group_autoregister_regression_test.go diff --git a/group_autoregister_regression_test.go b/group_autoregister_regression_test.go deleted file mode 100644 index 24be12e43..000000000 --- a/group_autoregister_regression_test.go +++ /dev/null @@ -1,89 +0,0 @@ -// SPDX-License-Identifier: MIT -// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors - -package echo - -import ( - "net/http" - "net/http/httptest" - "testing" - - "github.com/stretchr/testify/assert" -) - -// These tests guard the behavior re-introduced by restoring automatic group -// catch-all (404) route registration (PR #2996). v5 originally removed that -// registration because the implicit catch-all could: -// 1. mask 405 Method Not Allowed as 404, and -// 2. shadow sibling/root routes. -// Restoring auto-registration must NOT bring those regressions back. - -// passthroughMW is a no-op middleware that forces a group to register its -// implicit catch-all routes (a group only auto-registers when it has middleware). -func passthroughMW(next HandlerFunc) HandlerFunc { - return func(c *Context) error { return next(c) } -} - -// 1. Wrong method on an existing group route must still return 405 (with Allow -// header), not be swallowed as 404 by the group's catch-all. -func TestGroup_autoCatchAll_wrongMethodStillReturns405(t *testing.T) { - e := New() - g := e.Group("/api", passthroughMW) - g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) - - req := httptest.NewRequest(http.MethodPost, "/api/users", nil) - rec := httptest.NewRecorder() - e.ServeHTTP(rec, req) - - assert.Equal(t, http.StatusMethodNotAllowed, rec.Code, - "POST to a GET-only group route must be 405, not masked to 404 by the catch-all") - assert.Contains(t, rec.Header().Get(HeaderAllow), http.MethodGet, - "405 response must advertise allowed methods") -} - -// 2. The group's catch-all must not shadow a concrete sibling route under the -// same prefix: a matched route returns its own handler, not the 404 catch-all. -func TestGroup_autoCatchAll_doesNotShadowConcreteSiblingRoute(t *testing.T) { - e := New() - g := e.Group("/api", passthroughMW) - g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) - g.GET("/health", func(c *Context) error { return c.String(http.StatusOK, "health") }) - - for path, want := range map[string]string{"/api/users": "users", "/api/health": "health"} { - status, body := request(http.MethodGet, path, e) - assert.Equal(t, http.StatusOK, status, "concrete route %s must win over the group catch-all", path) - assert.Equal(t, want, body, "concrete route %s must run its own handler", path) - } - - // Only a genuinely unmatched path under the prefix hits the catch-all (404). - status, _ := request(http.MethodGet, "/api/nope", e) - assert.Equal(t, http.StatusNotFound, status, "unmatched path under the prefix should hit the catch-all 404") -} - -// 3. The group's catch-all (prefixed) must not shadow routes outside the group, -// including root-level routes. -func TestGroup_autoCatchAll_doesNotShadowRootRoute(t *testing.T) { - e := New() - e.GET("/health", func(c *Context) error { return c.String(http.StatusOK, "root-health") }) - g := e.Group("/api", passthroughMW) - g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) - - status, body := request(http.MethodGet, "/health", e) - assert.Equal(t, http.StatusOK, status, "root route must be unaffected by a group's catch-all") - assert.Equal(t, "root-health", body) -} - -// 4. Two sibling groups must not shadow each other's routes via their catch-alls. -func TestGroup_autoCatchAll_siblingGroupsDoNotShadow(t *testing.T) { - e := New() - v1 := e.Group("/api/v1", passthroughMW) - v1.GET("/ping", func(c *Context) error { return c.String(http.StatusOK, "v1") }) - v2 := e.Group("/api/v2", passthroughMW) - v2.GET("/ping", func(c *Context) error { return c.String(http.StatusOK, "v2") }) - - for path, want := range map[string]string{"/api/v1/ping": "v1", "/api/v2/ping": "v2"} { - status, body := request(http.MethodGet, path, e) - assert.Equal(t, http.StatusOK, status, "%s must resolve to its own group handler", path) - assert.Equal(t, want, body) - } -} From 39dbf3fb6e61f5e187b2fec91b207989ee3ba3df Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sat, 13 Jun 2026 12:35:55 -0700 Subject: [PATCH 3/4] test: add rewritten group method-handling tests The previous commit recorded only the deletion of the old file; this adds the rewritten suite (group_method_handling_test.go). Co-Authored-By: Claude Opus 4.8 (1M context) --- group_method_handling_test.go | 85 +++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 group_method_handling_test.go diff --git a/group_method_handling_test.go b/group_method_handling_test.go new file mode 100644 index 000000000..905e78a0b --- /dev/null +++ b/group_method_handling_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors + +package echo + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +// These tests lock in v5's method-handling semantics for routes registered through +// a Group. v5 resolves method mismatches (405) and OPTIONS at the router level and +// does NOT register any implicit per-group catch-all route. +// +// They double as a regression gate. Registering a group-level catch-all — whether +// manually via g.RouteNotFound("/*", ...) or automatically (as proposed in #2996 to +// fix CORS-on-group preflight) — makes that catch-all match every method, which masks +// both 405 and v5's automatic OPTIONS response as 404. Verified empirically: with such +// a catch-all in place, "POST /api/users" returns 404 instead of 405 and +// "OPTIONS /api/users" returns 404 instead of 204. If that behavior reaches this +// branch, the first two tests below fail. + +// A method mismatch on an existing group route must return 405 with the allowed +// methods, not be masked to 404. +func TestGroupRoute_methodMismatchReturns405(t *testing.T) { + e := New() + g := e.Group("/api") + g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) + + req := httptest.NewRequest(http.MethodPost, "/api/users", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusMethodNotAllowed, rec.Code, + "POST to a GET-only group route must be 405, not masked to 404") + assert.Equal(t, "OPTIONS, GET", rec.Header().Get(HeaderAllow), + "405 response must advertise the allowed methods") +} + +// OPTIONS on an existing group route is answered automatically by Echo (204 + +// Allow). This is the behavior CORS preflight relies on, so it must not be masked. +func TestGroupRoute_automaticOPTIONS(t *testing.T) { + e := New() + g := e.Group("/api") + g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) + + req := httptest.NewRequest(http.MethodOptions, "/api/users", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusNoContent, rec.Code, + "OPTIONS on a registered group route must be auto-answered (204), not masked to 404") + assert.Equal(t, "OPTIONS, GET", rec.Header().Get(HeaderAllow), + "automatic OPTIONS response must advertise the allowed methods") +} + +// A matched concrete route resolves to its own handler; only a genuinely unmatched +// path under the prefix is a 404. +func TestGroupRoute_concreteRoutesResolve(t *testing.T) { + e := New() + g := e.Group("/api") + g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) + + status, body := request(http.MethodGet, "/api/users", e) + assert.Equal(t, http.StatusOK, status) + assert.Equal(t, "users", body) + + status, _ = request(http.MethodGet, "/api/nope", e) + assert.Equal(t, http.StatusNotFound, status) +} + +// A group prefix must not affect routing of routes registered outside the group. +func TestGroup_doesNotAffectRootRoutes(t *testing.T) { + e := New() + e.GET("/health", func(c *Context) error { return c.String(http.StatusOK, "root") }) + g := e.Group("/api") + g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) + + status, body := request(http.MethodGet, "/health", e) + assert.Equal(t, http.StatusOK, status) + assert.Equal(t, "root", body) +} From 3efd66d7a140758c010d9d8eb56475427415bf35 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Sat, 13 Jun 2026 12:43:27 -0700 Subject: [PATCH 4/4] test: add companion test demonstrating group catch-all masking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses final review: converts the "verified empirically" comment into an actual test. TestGroupRoute_catchAllMasksMethodHandling registers a group-wide catch-all and asserts it masks both the 405 method-mismatch and the automatic OPTIONS (204) response as 404 — the regression the 405/OPTIONS gate tests guard against. Makes the rationale self-proving in-repo. Co-Authored-By: Claude Opus 4.8 (1M context) --- group_method_handling_test.go | 38 +++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/group_method_handling_test.go b/group_method_handling_test.go index 905e78a0b..4a8cf0979 100644 --- a/group_method_handling_test.go +++ b/group_method_handling_test.go @@ -18,10 +18,9 @@ import ( // They double as a regression gate. Registering a group-level catch-all — whether // manually via g.RouteNotFound("/*", ...) or automatically (as proposed in #2996 to // fix CORS-on-group preflight) — makes that catch-all match every method, which masks -// both 405 and v5's automatic OPTIONS response as 404. Verified empirically: with such -// a catch-all in place, "POST /api/users" returns 404 instead of 405 and -// "OPTIONS /api/users" returns 404 instead of 204. If that behavior reaches this -// branch, the first two tests below fail. +// both 405 and v5's automatic OPTIONS response as 404 — demonstrated directly by +// TestGroupRoute_catchAllMasksMethodHandling below. If that masking becomes the +// default (e.g. #2996 lands), the first two tests below fail. // A method mismatch on an existing group route must return 405 with the allowed // methods, not be masked to 404. @@ -83,3 +82,34 @@ func TestGroup_doesNotAffectRootRoutes(t *testing.T) { assert.Equal(t, http.StatusOK, status) assert.Equal(t, "root", body) } + +// Characterization of the regression the 405/OPTIONS tests above guard against: +// registering a group-wide catch-all (the manual equivalent of #2996's auto- +// registration) makes it match every method, so method mismatches and the automatic +// OPTIONS response are masked as 404 even though the concrete route still resolves. +// If a future change teaches the catch-all to preserve method semantics, update this. +func TestGroupRoute_catchAllMasksMethodHandling(t *testing.T) { + e := New() + g := e.Group("/api") + g.GET("/users", func(c *Context) error { return c.String(http.StatusOK, "users") }) + g.RouteNotFound("/*", func(c *Context) error { return c.NoContent(http.StatusNotFound) }) + + // The concrete route still resolves. + status, body := request(http.MethodGet, "/api/users", e) + assert.Equal(t, http.StatusOK, status) + assert.Equal(t, "users", body) + + // But the catch-all masks the method mismatch (would be 405) ... + post := httptest.NewRequest(http.MethodPost, "/api/users", nil) + postRec := httptest.NewRecorder() + e.ServeHTTP(postRec, post) + assert.Equal(t, http.StatusNotFound, postRec.Code, + "a group-wide catch-all masks the 405 method-mismatch as 404") + + // ... and the automatic OPTIONS response (would be 204). + opts := httptest.NewRequest(http.MethodOptions, "/api/users", nil) + optsRec := httptest.NewRecorder() + e.ServeHTTP(optsRec, opts) + assert.Equal(t, http.StatusNotFound, optsRec.Code, + "a group-wide catch-all masks the automatic OPTIONS (204) response as 404") +}