🐛 fix(rewrite,redirect): anchor rules to the start of the path#4483
Conversation
Rule regexes appended $ but never prepended ^, so a pattern matched any suffix of the path. A rule then fired on unrelated routes (e.g. /users/* also rewrote /api/users/1, and /old also redirected /very/old). Anchor both ends so a rule matches the whole path. Adds regression tests for both middlewares. Closes gofiber#4476
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughBoth ChangesRedirect and Rewrite start-anchor fix
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@middleware/redirect/redirect_test.go`:
- Around line 136-163: Add t.Parallel() as the first statement in
Test_Redirect_StartAnchor so the new redirect test follows the repository’s Go
test concurrency requirement. Locate the test by its name in redirect_test.go
and update it without changing the existing assertions or setup.
In `@middleware/rewrite/rewrite_test.go`:
- Around line 178-211: Add t.Parallel() at the start of Test_Rewrite_StartAnchor
to match the repo’s test concurrency requirement for new Go tests. Update the
test function in rewrite_test.go so it calls t.Parallel() immediately after the
function begins, before creating the Fiber app or setting up rewrite rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ab63607-d237-42de-8691-b562a387b1f9
📒 Files selected for processing (4)
middleware/redirect/redirect.gomiddleware/redirect/redirect_test.gomiddleware/rewrite/rewrite.gomiddleware/rewrite/rewrite_test.go
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4483 +/- ##
=======================================
Coverage 92.90% 92.90%
=======================================
Files 138 138
Lines 13595 13595
=======================================
Hits 12631 12631
Misses 597 597
Partials 367 367
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:
|
Repo requires t.Parallel() at the start of added Go tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
3 participants
Description
The
rewriteandredirectmiddlewares build each rule's regex by replacing*with(.*)and appending$, but never anchor the start of the path. Without a leading^the pattern matches any suffix, so a rule fires on unrelated routes:rewriterule/users/*also rewrites/api/users/1.redirectrule/oldalso redirects/very/old(a request gets redirected by a rule whose path it only happens to end with).This anchors both ends (
^...$) so a rule matches the whole path. The middlewares already appended$, so the intent was a full-path match; only the start anchor was missing.Fixes #4476
Changes introduced
middleware/rewrite: prepend^when compiling each rule regex.middleware/redirect: prepend^when compiling each rule regex.Regression tests in both packages: a rule matches from the start (
/users/1-> rewritten,/old-> redirected) but no longer fires on a path that merely ends with the rule (/api/users/1,/very/old). Both new tests fail onmainand pass with this change.Benchmarks: No benchmark changes; the existing rule-matching benchmarks are unaffected (one extra anchor character per compiled rule, no per-request cost).
Documentation Update: None required; this corrects behavior to match the existing
$-anchored intent and the echo middleware it is based on (which anchors at the start).Changelog/What's New: Bug fix - rewrite/redirect rules now anchor to the start of the path.
Migration Guide: A rule that previously relied on suffix matching will no longer fire mid-path; such matches were unintended (the reported bug).
API Alignment with Express: No public API change.
API Longevity: No signature or config change; behavior only.
Examples: Covered by the new tests.
Type of change
Checklist
mainconfirmation).go test ./middleware/rewrite/... ./middleware/redirect/...).gofmtandgo vetclean.Note: I also flagged in the issue that rule strings are not escaped before compilation (regex metacharacters in a rule like
/v1.0/*are interpreted as regex). I kept this PR to the anchoring fix to stay minimal; happy to address escaping in a follow-up if wanted.