diff --git a/gin.go b/gin.go index 2e033bf3..434b1651 100644 --- a/gin.go +++ b/gin.go @@ -743,6 +743,7 @@ func (engine *Engine) handleHTTPRequest(c *Context) { if tree.method == httpMethod { continue } + *c.skippedNodes = (*c.skippedNodes)[:0] if value := tree.root.getValue(rPath, nil, c.skippedNodes, unescape); value.handlers != nil { allowed = append(allowed, tree.method) } diff --git a/routes_test.go b/routes_test.go index 1cae3fce..4e85db40 100644 --- a/routes_test.go +++ b/routes_test.go @@ -527,6 +527,23 @@ func TestRouteNotAllowedEnabled3(t *testing.T) { assert.Contains(t, allowed, http.MethodPost) } +func TestRouteNotAllowedDoesNotReuseSkippedNodes(t *testing.T) { + router := New() + router.HandleMethodNotAllowed = true + router.POST("/b", func(c *Context) {}) + router.POST("/:p0", func(c *Context) {}) + router.PUT("/a/:p1", func(c *Context) {}) + + // The POST probe for the Allow header matches static "/b" while + // recording the skipped wildcard ":p0". handleHTTPRequest must reset + // c.skippedNodes before probing the PUT tree, or the stale entry + // leaks the POST route into the PUT lookup and PUT is wrongly + // reported in the Allow header. + w := PerformRequest(router, http.MethodGet, "/b") + assert.Equal(t, http.StatusMethodNotAllowed, w.Code) + assert.Equal(t, http.MethodPost, w.Header().Get("Allow")) +} + func TestRouteNotAllowedDisabled(t *testing.T) { router := New() router.HandleMethodNotAllowed = false diff --git a/tree.go b/tree.go index 580abbaf..0d9cf6bd 100644 --- a/tree.go +++ b/tree.go @@ -410,6 +410,36 @@ type skippedNode struct { paramsCount int16 } +// rollbackToSkippedNode unwinds the skipped-wildcard stack after the walk hit +// a dead end. It pops entries until it finds one whose saved remaining path +// ends with the current remaining path, i.e. a branch point on the walk that +// led here; entries from abandoned branches are discarded. If one is found, +// the walk state saved there (remaining path, current node, captured params) +// is restored and true is returned; the caller must then resume the walk +// loop, which retries from the branch point with its wildcard child. +func rollbackToSkippedNode( + skippedNodes *[]skippedNode, + path *string, + n **node, + params *Params, + globalParamsCount *int16, +) bool { + for length := len(*skippedNodes); length > 0; length-- { + sn := (*skippedNodes)[length-1] + *skippedNodes = (*skippedNodes)[:length-1] + if strings.HasSuffix(sn.path, *path) { + *path = sn.path + *n = sn.node + if params != nil { + *params = (*params)[:sn.paramsCount] + } + *globalParamsCount = sn.paramsCount + return true + } + } + return false +} + // Returns the handle registered with the given path (key). The values of // wildcards are saved to a map. // If no handle can be found, a TSR (trailing slash redirect) recommendation is @@ -457,18 +487,8 @@ walk: // Outer loop for walking the tree // If the path at the end of the loop is not equal to '/' and the current node has no child nodes // the current node needs to roll back to last valid skippedNode if path != "/" { - for length := len(*skippedNodes); length > 0; length-- { - skippedNode := (*skippedNodes)[length-1] - *skippedNodes = (*skippedNodes)[:length-1] - if strings.HasSuffix(skippedNode.path, path) { - path = skippedNode.path - n = skippedNode.node - if value.params != nil { - *value.params = (*value.params)[:skippedNode.paramsCount] - } - globalParamsCount = skippedNode.paramsCount - continue walk - } + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk } } @@ -531,6 +551,11 @@ walk: // Outer loop for walking the tree // ... but we can't value.tsr = len(path) == end+1 + if !value.tsr { + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk + } + } return value } @@ -544,6 +569,11 @@ walk: // Outer loop for walking the tree n = n.children[0] value.tsr = (n.path == "/" && n.handlers != nil) || (n.path == "" && n.indices == "/") } + if !value.tsr { + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk + } + } return value case catchAll: @@ -588,18 +618,8 @@ walk: // Outer loop for walking the tree // If the current path does not equal '/' and the node does not have a registered handle and the most recently matched node has a child node // the current node needs to roll back to last valid skippedNode if n.handlers == nil && path != "/" { - for length := len(*skippedNodes); length > 0; length-- { - skippedNode := (*skippedNodes)[length-1] - *skippedNodes = (*skippedNodes)[:length-1] - if strings.HasSuffix(skippedNode.path, path) { - path = skippedNode.path - n = skippedNode.node - if value.params != nil { - *value.params = (*value.params)[:skippedNode.paramsCount] - } - globalParamsCount = skippedNode.paramsCount - continue walk - } + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk } // n = latestNode.children[len(latestNode.children)-1] } @@ -645,18 +665,8 @@ walk: // Outer loop for walking the tree // roll back to last valid skippedNode if !value.tsr && path != "/" { - for length := len(*skippedNodes); length > 0; length-- { - skippedNode := (*skippedNodes)[length-1] - *skippedNodes = (*skippedNodes)[:length-1] - if strings.HasSuffix(skippedNode.path, path) { - path = skippedNode.path - n = skippedNode.node - if value.params != nil { - *value.params = (*value.params)[:skippedNode.paramsCount] - } - globalParamsCount = skippedNode.paramsCount - continue walk - } + if rollbackToSkippedNode(skippedNodes, &path, &n, value.params, &globalParamsCount) { + continue walk } } diff --git a/tree_test.go b/tree_test.go index 23339af4..57b5672c 100644 --- a/tree_test.go +++ b/tree_test.go @@ -1111,3 +1111,36 @@ func TestTreeFindCaseInsensitivePathWildcardParamAndStaticChild(t *testing.T) { t.Errorf("Wrong result for '/prefix/something': %s", string(out)) } } + +func TestTreeParamFallbackAfterStaticDeadEnd(t *testing.T) { + tests := []struct { + name string + routes []string + request testRequests + }{ + { + name: "param has child but no handler", + routes: []string{"/:p0/:p1", "/bc/:p1/a"}, + request: testRequests{ + {"/bc/bc", false, "/:p0/:p1", Params{{Key: "p0", Value: "bc"}, {Key: "p1", Value: "bc"}}}, + }, + }, + { + name: "param has no child for remaining path", + routes: []string{"/:p0/:p1/:p2", "/bc/:p1"}, + request: testRequests{ + {"/bc/bc/bc", false, "/:p0/:p1/:p2", Params{{Key: "p0", Value: "bc"}, {Key: "p1", Value: "bc"}, {Key: "p2", Value: "bc"}}}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tree := &node{} + for _, route := range tt.routes { + tree.addRoute(route, fakeHandler(route)) + } + checkRequests(t, tree, tt.request) + }) + } +}