fix(router): handle escaped colons and inline verbs in v4 - #3132
Conversation
vishr
left a comment
There was a problem hiding this comment.
Review at 7d355b8. The backport matches #3113 + #3133 closely (removal correctly left out, since v4 has no Remove), no exported signatures change, CI is green, and Find stays at 0 allocs/op with no measurable slowdown on the router benchmarks.
It carries the same Find regressions as #3133 (see the review there), so it should take the same fix. As v4 is the LTS line, these matter more here: every item below changes behavior for routes that work on v4 today. Each was reproduced against this head and v4 (5440f99).
Must fix
- Method mismatch after the split (router.go#L466, #L700).
canMatchStaticSuffixaccepts anyisHandlernode regardless of method, and the split is never retried.- GET
/r/:name\:cancel+ POST/r/:name:POST /r/foo:cancelreturns 405 here, 200 on v4 withname=foo:cancel.
- GET
- Split taken on a guess (router.go#L474). It returns true whenever the suffix node has a parameter or wildcard child, even if the rest of the path cannot match through it, and there is no fallback to the unsplit value.
/r/:name\:v:id/end+/r/:name/other:GET /r/a:vq/otherreturns 404 here, matches/r/:name/otheron v4.
- Static-over-parameter priority broken (router.go#L700). A leaf parameter under the verb suffix takes the rest of the path, including segments a static sibling should match.
/r/:name\:x:id+/r/:name/q:GET /r/a:x/qgoes to the first route withname=a,id=/q. v4 routes it to/r/:name/q.
- Empty parameter value (router.go#L699). The loop starts at
split := 0, soGET /r/:cancelmatches/r/:name\:cancelwithname="". Start at 1. c.ParamNames()is now nil for static routes (router.go#L217). v4 usedpnames := []string{}. Code that compares against[]string{}or JSON-encodes it ([]becomesnull) changes on a patch upgrade. Keep the empty slice.
Suggested direction for 1–3, same as #3133: drop canMatchStaticSuffix and make the colon split a backtrack point in Find (try the split; on failure, retry with the value running to the next /).
Should fix
- Keep v4 and v5 diffable. Use
slices.Contains(paramMarkers, searchOffset)instead of the loop at router.go#L362 (v4 go.mod is go 1.25, and #3133 does this). Backport v5'srouteTreePathhelper instead of building the tree path inline inAdd(router.go#L227). Reverseallocations (router.go#L168).uri.WriteString(fmt.Sprintf("%v", ...))makes a temporary string per parameter, andparseRoutePathnow runs on every call.fmt.Fprint(&uri, params[n])matches v5.
Notes
- Sibling
/r/:namestops matching slashes once/r/:name\:cancelis registered, because the parameter node is no longer a leaf. Consistent with adding/r/:name/edit, but on v4 LTS it deserves a line in the release notes. - Encoded colon. Matching runs on the raw path, so
/r/foo%3Acancel(whatencodeURIComponentproduces) reaches the generic route withname=foo%3Acancelinstead of the verb route. Probably fine, but worth a test that pins the behavior.
vishr
left a comment
There was a problem hiding this comment.
Re-review at 9e3f23d. All eight findings from the previous review are fixed: method fallback, the guessed split, static sibling priority, empty values, non-nil ParamNames(), slices.Contains/routeTreePath parity, Reverse via fmt.Fprint, and a test pinning %3A. The inline-verb flag is set per host router, and the router benchmarks stay at 0 allocs/op.
The new retry matcher is the same algorithm as #3133 at 9b86d77 (see the review there), and it has the same problems. Each item below was reproduced against this head. On v4 LTS these change behavior for apps that work today, so they need fixing before a patch release.
Must fix
- CPU denial of service (router.go#L700). Every colon adds a full re-search pass that rescans the segment, so cost grows with the square of the colon count. GET
/r/:name\:cancel, request/r/a+ 40,000:: 1.3 s, then 404. v4 today answers the same request in microseconds. - Group middleware or a catch-all route disables the fallback (router.go#L767). The unsplit retry only runs after a whole pass fails, and an any-route or
RouteNotFoundnode reached by backtracking counts as a match.- Group
/rwith middleware, routes/:nameand/:name\:cancel:GET /r/foo:otherreturns 404. - Routes
/r/:name,/r/:name\:canceland/*:GET /r/foo:othergoes to/*. - Group middleware + GET
/:name\:cancel+ POST/:name:POST /r/foo:cancelreturns 404; v4 today returns 200.
- Group
- A trailing leaf param stops at
/after any split (router.go#L685). GET/r/:name\:x/:rest:GET /r/a:x/b/creturns 404. It also affects unrelated branches reached by backtracking:/api/:name\:x/q,/:page,/*:GET /api/b:x/zzzgoes to/*instead of/:page. - Empty params for a
RouteNotFoundfallback (router.go#L770).fallbackValuesis captured after backtracking has clearedparamValues. GET +RouteNotFoundon/r/:name\:cancel, POST/r/:name:PUT /r/foo:cancelreaches the not-found handler withc.Param("name") == "". Thecopyat #L785 has no effect.
The cross-branch priority and Allow header issues from the #3133 review come from the same code and should be checked here too.
Root cause and suggestion
Same as #3133: the split is handled by restarting the search with a global plan instead of as a backtrack point on the param node.
- Re-enter the same param with the next split position before backtracking to its parent.
- Count only splits where the colon child's prefix matches (
strings.HasPrefix(search[split:], child.prefix))./r/urn:a:b:c:d:ecurrently runs 6 full passes before reaching the generic route. - Or limit inline verbs to the last route segment, which covers #3111 with a single check and no retries.
Should fix
- Two copies of
Find(router_plain.go#L13). The ~200-line body is duplicated and has already diverged (returnvsbreakon static-backtrack failure). Keep one body and gate the split block onr.hasInlineVerb && currentNode.hasColonChild. Please keep this in step with #3133 so v4 and v5 stay diffable.
Backport of the v5 change. Replace the whole-search retry plan with a backtrack point on the param node: when a candidate split fails, the same param node is retried with the next literal-colon split and finally the whole path segment before routing backtracks to its parent. Static > param > any priority of ancestors, the leaf rule of later params, group middleware and catch-all routes are no longer affected by a failed split. An escaped colon after a parameter starts an inline verb only when the rest of that path segment is static (`/:name\:cancel`, optionally followed by `/...`). Split candidates are then scanned once per segment, so a request with many colons is routed in linear time. Other escaped colons after a parameter keep their older meaning as part of the parameter name. The duplicated fast-path Find and the router-level inline verb flag are removed. The param scan uses strings.IndexByte, which keeps router benchmarks within about 0.3% of v4 (geomean).
A wildcard ends the route search, but a param value above it that ended at an inline verb split is now retried with the next split and the whole segment, so a verb route with a wildcard cannot shadow a generic route for other methods. The retry now runs only when routing backtracks from the inline verb child into its param node, instead of on every dead end. An escaped colon after a parameter keeps its older meaning (part of the parameter name) unless the first one in the segment starts an inline verb, so a later `\:` cannot turn such a legacy route into a verb route. Reverse writes placeholders for such names without the backslash, as before. Dead hasColonChild bookkeeping for split nodes is removed.
When a wildcard fails below a param value that ended at an inline verb split, keep backtracking one node at a time instead of jumping to that param node, so the other routes below the split are tried before the next split. Without a pending split a failed wildcard still ends the search as before, and the check does not change the routing state. Adds tests for nested splits, routes below a split and a RouteNotFound wildcard below a split.
An escaped colon after a param name that contains ':' or '*' keeps its older meaning as part of the name. The escaped colon check is shared in the route syntax scanner. Adds tests for RouteNotFound on the whole segment and Reverse placeholders.
Registering an inline verb route under a param gave the param node a static child, so a sibling route ending in that param (`/files/:path`) stopped matching values across slashes. When the inline verb child is the node's only child, a param value without a split now takes the rest of the path, as a leaf param does.
Drop the param and any child checks that cannot fail for a param node, and pin 405, RouteNotFound and wildcard fallbacks for a param value that spans slashes next to an inline verb route.
Summary
Backports the escaped-colon router fix in #3113 and the inline-verb follow-up in #3133 to v4. Routes with escaped literal colons and parameter routes stay reachable in either registration order, including paths with encoded NUL bytes.
/:name\:cancel) when the rest of that path segment is static. Other escaped colons after a parameter keep their older meaning as part of the parameter name.ParamNames()stays a non-nil empty slice for static routes.Reverseplaceholders drop the backslash of escaped colons, as before.v4 has no route removal API, so the removal fix in #3133 applies only to v5.
Compatibility notes (release notes)
/x/:id\:ynow registers parameteridand a literal:y. Before, it registered one parameter namedid\:ythat matched any segment. This is the fix for A\:route and a:paramroute at the same position makeServeHTTPpanic or return 404 #3111; apps that relied on the old reading of such routes see different params and matching.Performance
Router benchmarks against v4, 8 interleaved runs on Apple M3 Max: geomean +0.15%, 0 allocs/op everywhere.
Verification
go test ./... -count=1,go test -race .,go vet ./...,staticcheck .%3Ahandling.Refs #3111.