fix: deduplicate HTTP methods in 405 Allow header#1047
Conversation
| for m, name := range reverseMethodMap { | ||
| if allowed&m != 0 { | ||
| w.Header().Add("Allow", name) | ||
| } |
There was a problem hiding this comment.
Hi, thank you for this contribution.
Would you mind splitting the tests into a first commit?
I feel like the bitwise implementation is a bit too complex. Perhaps we can iterate on the second commit?
There was a problem hiding this comment.
Good call, thanks for the feedback! I've split it into two commits:
test: add test for duplicate methods in 405 Allow header— just the test that reproduces Duplicate HTTP methods in 405 Allow header #996fix: deduplicate HTTP methods in 405 Allow header— the actual fix
I also replaced the bitwise approach with a simple seen-map to deduplicate the methodsAllowed slice before building the closure. It keeps the original iteration pattern over the slice (instead of iterating reverseMethodMap) so the behavior is more straightforward. Let me know what you think!
44ecca2 to
843c7c2
Compare
|
Hi @VojtechVitek, just a friendly follow-up — I've split the commits as you suggested (test first, then fix) and replaced the bitwise approach with a simpler seen-map for deduplication. Would love your thoughts on the updated approach when you get a chance. Thanks! |
Reproduces go-chi#996: when overlapping wildcard routes match the same path (e.g., /article/1-2-3 matching /article/{a}, /article/{b}-{c}, /article/{b}-{c}-{d}), the Allow header in 405 responses contains duplicate entries like "Allow: POST POST POST POST". This test currently fails and will be fixed in the next commit.
Use slices.Sort + slices.Compact to deduplicate the methodsAllowed slice before constructing the handler closure. This avoids duplicate entries in the Allow header when overlapping wildcard routes match the same path. Fixes go-chi#996
843c7c2 to
9b59565
Compare
|
ping - small fix for the duplicate methods in the 405 Allow header. would appreciate a look when you have time |
themavik
left a comment
There was a problem hiding this comment.
slices.Sort + Compact on the methodsAllowed slice removes duplicate methodTyp entries before methodNotAllowedHandler builds Allow. nit: sorting can change method order relative to registration—note that if anyone snapshots the header literally.
Summary
Fixes #996
When overlapping wildcard routes match the same path (e.g.,
/article/1-2-3matching/article/{a},/article/{b}-{c},/article/{b}-{c}-{d}), the tree traversal appends the same HTTP method tomethodsAllowedmultiple times. This causes theAllowheader in 405 responses to contain duplicate entries:Root cause: In
tree.go,findRouteiterates through matching nodes and appends each node's endpoint methods torctx.methodsAllowed. When multiple wildcard patterns match the same path, the same method gets appended once per matching node.Fix: In
methodNotAllowedHandler, deduplicate themethodsAllowedslice using a seen-map before building the closure. Each method now appears exactly once in the header regardless of how many overlapping routes matched.Before:
After:
Commits
test: add test for duplicate methods in 405 Allow header— AddsTestMethodNotAllowedDuplicateAllowthat reproduces the exact scenario from Duplicate HTTP methods in 405 Allow header #996 (registers overlapping wildcard routes with the same method and asserts theAllowheader has no duplicates).fix: deduplicate HTTP methods in 405 Allow header— DeduplicatesmethodsAllowedusing a simple seen-map before constructing the handler closure.Test plan
TestMethodNotAllowedDuplicateAllowreproduces the exact scenario from Duplicate HTTP methods in 405 Allow header #996TestMethodNotAllowedcontinues to pass (verifies multiple distinct methods like GET and HEAD still appear correctly)TestMuxNestedMethodNotAllowedcontinues to passgo test ./...