fix(tonic-xds): match routes on request path and anchor safe_regex - #2804
Open
YutaoMa wants to merge 5 commits into
Open
fix(tonic-xds): match routes on request path and anchor safe_regex#2804YutaoMa wants to merge 5 commits into
YutaoMa wants to merge 5 commits into
Conversation
## Summary
Two independent route-matching bugs, each of which silently selects the
wrong route. Both are masked by a catch-all prefix route, and the first
hides the second: while every request matched "/", no regex route could
match at all.
## What changed
- **Match on the request path.** The path was read from the `:path`
header, which never resolves, so every request matched "/" rather than
the method being invoked. It now comes from the request URI, with the
query string excluded as the matcher requires.
- **Anchor `safe_regex` to the whole path.** Patterns were matched
anywhere in the path, so a route matched any path merely containing
the pattern. Envoy requires the entire path to match, so patterns are
now compiled as `\A(?:{})\z`.
Anchoring is applied to the path matcher only; the two header-value
regex sites are left to a follow-up. There is no public API change.
## Testing
Three tests, each failing without its fix: one drives the routing layer
with a specific route ordered ahead of a catch-all, two go through
validation to check the anchors.
YutaoMa
changed the title
YutaoMa
marked this pull request as ready for review
August 13, 2026 23:52
gu0keno0
reviewed
Aug 14, 2026
| /// regardless of those flags, whereas `^`/`$` become line anchors under | ||
| /// `(?m)`. | ||
| pub(crate) fn new(pattern: &str) -> Result<Self, regex::Error> { | ||
| Regex::new(&format!("{ANCHOR_PREFIX}{pattern}{ANCHOR_SUFFIX}")).map(Self) |
Contributor
There was a problem hiding this comment.
Validate the regex first so that injection such as "foo)|bar(?:" will not cause loopholes
Contributor
Author
There was a problem hiding this comment.
Good catch, added a pre-anchoring compile so patterns with unbalanced parens get rejected.
gu0keno0
reviewed
Aug 14, 2026
| } | ||
|
|
||
| #[test] | ||
| fn an_invalid_pattern_is_rejected() { |
Contributor
There was a problem hiding this comment.
Can add to this test as well.
Contributor
Author
There was a problem hiding this comment.
Added tests for the above cases.
gu0keno0
reviewed
Aug 14, 2026
gu0keno0
left a comment
gu0keno0
left a comment
Contributor
There was a problem hiding this comment.
One comment about regex safety, otherwise LGTM
gu0keno0
approved these changes
Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Two route-matching bugs were discovered while testing
tonic-xds::RoutingLayer. Previously because the routing config wasn't wired in, the buggy path wouldn't be exercised in production. Now they are surfaced and needs fixing:RoutingLayerused to fetch:pathheader from theHeaderMap, but that pseudo header isn't actually inserted there. (Rusthttp::HeaderMapdoes not allow storing pseudo headers).StringMatcher::SafeRegexdoes partial match, while the spec mandates full match (see https://github.com/envoyproxy/envoy/blob/743baafef00f4f8d5fc234e6ee5cb7ed87cba148/api/envoy/type/matcher/v3/regex.proto#L66)Solution
safe_regexpatterns are now anchored as\A(?:{})\zin the new typeSafeRegex. We enforce full match by only exposing that matching function from the type. For debuggability theDebugimplementation does print the original unanchored pattern.