🐛 bug: fix regex route constraint parsing with literal >#4292
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe parameter route parser in ChangesParameter Constraint End-Marker Parsing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
There was a problem hiding this comment.
Code Review
This pull request modifies the route parser in path.go to allow multiple closing characters within parameter constraints, which is necessary for regex patterns containing the > character. A corresponding test case was added to path_testcases_test.go. The review feedback identifies a potential panic scenario with malformed routes and suggests adding a check to ensure the constraint start position is set before processing the end character.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4292 +/- ##
==========================================
+ Coverage 91.26% 91.32% +0.06%
==========================================
Files 132 132
Lines 12933 12933
==========================================
+ Hits 11803 11811 +8
+ Misses 715 708 -7
+ Partials 415 414 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a route parameter constraint parsing regression where the parser treated the first unescaped > inside a <...> constraint as the terminator, truncating regex constraints that contain literal > and potentially degrading them into an unintended no-op constraint.
Changes:
- Update
analyseParameterPartto keep updatingparamConstraintEndPositionfor each unescaped>so the final closing delimiter is used. - Add a regression testcase covering a regex constraint that contains a literal
>([^>]+) and verifying correct match/reject behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
path.go |
Adjusts constraint end parsing to use the last unescaped > within the constraint instead of the first. |
path_testcases_test.go |
Adds a regression testcase ensuring regex constraints with literal > are parsed and enforced correctly. |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
path.go:380
paramConstraintEndPositionis set as soon as the first unescaped>is encountered after<, which makes the parser treat the constraint as “closed” for theparameterEndCharscheck. If the constraint body contains a>followed later by-or.(both areparameterEndChars), the loop can break before reaching the real closing>, leaving the constraint truncated/ignored. Consider only accepting>as the constraint terminator when it’s followed by a parameter boundary (e.g., end-of-string or aparameterEndCharsrune like/,?,-,.), so internal>inside regex data (e.g.:p<regex(^foo>bar-baz$)>) doesn’t prematurely “close” the constraint.
for i := range search {
if paramConstraintStartPosition == -1 && search[i] == paramConstraintStart && (i == 0 || search[i-1] != escapeChar) {
paramConstraintStartPosition = i + 1
continue
}
if paramConstraintStartPosition != -1 && search[i] == paramConstraintEnd && (i == 0 || search[i-1] != escapeChar) {
paramConstraintEndPosition = i + 1
continue
}
if parameterEndChars[search[i]] {
if (paramConstraintStartPosition == -1 && paramConstraintEndPosition == -1) ||
(paramConstraintStartPosition != -1 && paramConstraintEndPosition != -1) {
paramEndPosition = i
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@path.go`:
- Line 373: The if-statement line containing paramConstraintStartPosition,
search, paramConstraintEnd and escapeChar is not gofmt-formatted; run gofmt -w
path.go (or use your IDE's format command / goimports) to reformat the file so
the if-condition line is corrected and CI will pass, ensuring the expression
spanning "if paramConstraintStartPosition != -1 && search[i] ==
paramConstraintEnd && (i == 0 || search[i-1] != escapeChar) {" is properly
formatted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 422f768 | Previous: dfc64f5 | Ratio |
|---|---|---|---|
Benchmark_Compress/Zstd (github.com/gofiber/fiber/v3/middleware/compress) - B/op |
1 B/op |
0 B/op |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Motivation
>inside a parameter constraint was recorded as the constraint terminator, which truncated regex constraints containing literal>and could degrade them into a no-opnoConstraintallowing unintended matches.Description
analyseParameterPartinpath.gosoparamConstraintEndPositionis updated for every unescaped>found, ensuring the closing delimiter of the full constraint is used rather than the first>inside constraint data.path_testcases_test.gofor the pattern/:param<regex(^[^>]+$)>to assert a safe value matches and a value containing>is rejected.path.goandpath_testcases_test.goand preserve existing parser behavior for all other cases.