🐛 bug: rank wildcards behind character classes in redirect ordering - #4607
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRedirect rule ordering now ranks Fiber wildcard patterns after comparable non-wildcard patterns. Pattern-width scanning handles quoted spans, dot wildcards, and overflow-safe multiplication. Tests cover ordering, quoted stars, fallback matching, and width limits. ChangesRedirect wildcard ordering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4607 +/- ##
==========================================
- Coverage 93.69% 93.67% -0.03%
==========================================
Files 140 140
Lines 15982 16004 +22
==========================================
+ Hits 14975 14991 +16
- Misses 624 629 +5
- Partials 383 384 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02c72b2e0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Saturating a wildcard rule at maxPatternWidth erased the width of every construct after it, so "/p/*[ab]" tied "/p/*[a-z]" and lost the tie-break to key order. It also fed the clamp value into the group multiplication, which overflows an int on a 32-bit build and sorts the resulting negative first. Fiber expands "*" to "(.*)", so measure it as the widest single position, the same as "." — that still ranks a catch-all behind a character class pinning the same prefix, while what follows the wildcard keeps separating two rules that both open with one. Multiply through a saturating helper so a nest of groups cannot overflow, and skip "\Q ... \E" spans, where a star names itself rather than opening a wildcard.
Go's parser quotes to the end of the pattern when the "\E" is missing, so the scanner has to stop there too.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8de760b067
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A width saturates, so counting the wildcard among the multipliers pushed wide rules into the clamp 256x sooner: "/p/*[a-z][ab]x11" and "/p/*[ab]x12" both reached maxPatternWidth and tied, and key order handed a shared path to the broader rule. A wildcard matches a run of any length, which no number the widths are compared against can stand for, so rank it ahead of the width instead. Two rules that both carry one are then separated by everything beside it, which is the figure the width already computes: 53248 against 4096 for the pair above.
The inline branch pair only took its "wildcard sorts second" arm when map iteration happened to hand the wildcard rule in as the left operand, so whether it ran varied per run. A rank of 0 or 1 reads like the keys around it and is asserted directly instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b021114ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
classWidth stopped at the "]" closing "[:digit:]", which does not close the class holding it, so the scan resumed inside the brackets and measured the members beyond as pattern syntax. With the wildcard now ranked ahead of the width, that also had a "*" among those members read as Fiber's wildcard, and "/p/[[:digit:]*]" — an exact one-character rule — sorted behind the "/p/." catch-all it should outrank.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1dfec7a7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two rules were still ordered by what they look like rather than what they compile to. A POSIX name counted as one member, so "[[:digit:]]" scored narrower than the "[09]" it contains and shadowed it. Measure the name by the bytes it matches, negation included. The replacement runs over the whole key, so a backslash does not spare the star that follows it: "\*" becomes "\(.*)", an escaped parenthesis and then a live wildcard. Read as a literal star, "/p/(\*" ranked as pinning every position while it matched any suffix at all.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15e8f7d417
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
classWidth sums its members rather than unioning them, so overlapping sets are counted twice and the total can run past the 256 bytes a class is drawn from. Past that the sum says nothing about the count, and taking a complement from it came out negative: "[^[:alpha:][:^digit:]]" matches the ten digits, scored 256-298, and floored to 1 it sorted ahead of the "[09]" it contains. Read such a class as matching every byte instead, so one this scan cannot measure never shadows one it can. That also caps a non-negated class at the 256 it could match at most.
The scanner changes that grew around this fix — a POSIX size table, an overflow-safe multiply, quoted-span handling in width(), a clamp for classes whose members overlap, a case for the star behind a backslash — were each chasing a construct review turned up, not the bug this PR is for. They touched measurement code that was already imprecise about those same constructs before this branch, so they were fixing the scanner rather than the ordering. What is left is the one thing the bug needed: rank a rule carrying a wildcard behind one that does not, and leave the width to separate two rules that both carry one. The scanner is untouched.
3 participants
Motivation
/api/*shadow a narrower character-class rule such as/api/[ab]when their literal prefix and pinned lengths tied, so a request could be handled by the catch-all instead of the specific rule meant for it.Description
wildcardRankreturns 1 for a rule carrying Fiber's*and 0 otherwise, so the wildcard rule sorts second.maxPatternWidth, and two saturated rules tie again. Keeping it out of the width also leaves the width free to go on separating two rules that both carry one, so/p/*[ab]still beats/p/*[a-z].wildcardRankskips character classes and\Q ... \Espans, sinceNewexpands every*before compiling and the result is a class ([(.*)]) or literal text rather than a live wildcard.The pattern scanner is untouched.
Files changed:
middleware/redirect/redirect.go,middleware/redirect/redirect_test.go.Testing
go test ./middleware/redirect -count=1passes, as doesGOARCH=386 go test ./middleware/redirect -count=1;go vet ./middleware/redirectandgofmt -lare clean.Test_Redirect_RuleOrderIsBySpecificity: the class outranking the wildcard on/api/a(query preserved), the wildcard still taking/api/z, and/p/*[ab]beating/p/*[a-z]on/p/za. The first fails without the new key.Test_Redirect_NestedAlternationLosesTheTieBreakpinwildcardRankfor a bare star, a class, a quoted star, an unterminated\Q,[*], and a star after an escape.wildcardRankis at 100% coverage.Note on scope
An earlier revision of this branch also reworked the pattern scanner — a POSIX class size table, an overflow-safe multiply, quoted-span handling inside
width(), a clamp for classes whose members overlap, and a case for a star behind a backslash. Each addressed a real construct raised in review, but all of them were fixing pre-existing imprecision in how the scanner measures rules rather than the ordering bug this PR is for, and none was reachable from the reported issue. They have been reverted to keep this change to its subject. The measurement gaps are still there, unchanged from before this branch, and are better handled on their own.Codex Task