fix: prevent infinite redirect loop in beforeEnter guard#1919
fix: prevent infinite redirect loop in beforeEnter guard#1919madhuri-perumalla wants to merge 2 commits into
Conversation
github-actions
Bot
added
type:bug
📝 WalkthroughWalkthroughRouter gains a ChangesRedirect Loop Prevention
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Router
participant BeforeEnterGuard
Caller->>Router: push/replace(path)
Router->>BeforeEnterGuard: beforeEnter(path)
BeforeEnterGuard-->>Router: returns string (redirect)
Router->>Router: increment _redirectDepth
alt depth exceeds _maxRedirectDepth
Router->>Router: emit error event
Router-->>Caller: abort navigation
else depth within limit
Router->>Router: navigate to redirect target
Router->>Router: reset _redirectDepth on completion
end
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router/src/router.ts (1)
183-211: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_redirectDepthleaks across navigations when a redirect targets a non-existent route.Because
_redirectDepthis now instance state that is only cleared onfalse, overflow, or a terminal (non-redirect) result, the!matchearly return at Lines 186-189 does not reset it. If abeforeEnterredirect points to a path with no matching route, navigation aborts with the counter still elevated. A later, unrelated redirect chain then starts from a non-zero depth and can spuriously tripMaximum redirect depth ... Possible infinite redirect loop.The same leak exists inreplace(Lines 242-245).Reset the counter on the no-match path (in both
_navigateToandreplace):if (!match) { + this._redirectDepth = 0; this.events.emit('error', new Error(`No route found for path: ${path}`)); return; }🤖 Prompt for 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. In `@packages/router/src/router.ts` around lines 183 - 211, Reset the router’s redirect counter when navigation fails to match a route, since `_redirectDepth` in `Router._navigateTo` is currently left elevated after the `!match` early return. Update both `_navigateTo` and `replace` so the no-route-found path clears `_redirectDepth` before emitting the error and returning, keeping later `beforeEnter` redirect chains from inheriting stale state and tripping the max-depth guard spuriously.
🧹 Nitpick comments (1)
packages/router/src/router.ts (1)
239-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the shared
beforeEnterredirect-depth handling.Lines 195-211 in
_navigateToand Lines 249-265 here are identical guard/depth logic. A small helper (e.g._resolveGuard(path, guardResult, redirect)returning'stop' | 'redirect' | 'continue') would keep the two navigation flows from diverging as this logic evolves. Optional given the current size.🤖 Prompt for 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. In `@packages/router/src/router.ts` around lines 239 - 265, The redirect-depth and beforeEnter guard handling in replace duplicates the same logic used in _navigateTo, so extract it into a shared helper on Router to keep both navigation paths consistent. Create a small method such as _resolveGuard that takes the path, guardResult, and redirect action and returns a clear outcome like stop, redirect, or continue, then use it from replace and _navigateTo instead of maintaining two separate copies of the depth/error logic.
🤖 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 `@packages/router/src/router.test.ts`:
- Around line 244-289: Add a test in Router coverage for a `beforeEnter`
redirect to a missing route followed by a normal redirect chain, using the real
`Router` and `events.on('error')` like the existing `beforeEnter`/`replace`
tests. The new case should exercise the no-match path in `Router` navigation
(where `_redirectDepth` can leak) by redirecting from one route to a
non-existent target, then performing a successful redirect sequence and
asserting no unexpected error plus the final `currentPath` is correct. Reference
the existing `Router`, `beforeEnter`, `push`, and `replace` test patterns so
it’s easy to place alongside the current redirect-depth tests.
In `@packages/router/src/router.ts`:
- Around line 37-38: Update the router option doc comment for maxRedirectDepth
so it matches the actual behavior in router.ts: it does not throw, it emits an
error event via this.events.emit('error', ...) and returns. Adjust the wording
in the Router interface/option docs to say the redirect chain limit triggers an
error event instead of a thrown error, so consumers using push() or replace()
won’t expect try/catch handling.
---
Outside diff comments:
In `@packages/router/src/router.ts`:
- Around line 183-211: Reset the router’s redirect counter when navigation fails
to match a route, since `_redirectDepth` in `Router._navigateTo` is currently
left elevated after the `!match` early return. Update both `_navigateTo` and
`replace` so the no-route-found path clears `_redirectDepth` before emitting the
error and returning, keeping later `beforeEnter` redirect chains from inheriting
stale state and tripping the max-depth guard spuriously.
---
Nitpick comments:
In `@packages/router/src/router.ts`:
- Around line 239-265: The redirect-depth and beforeEnter guard handling in
replace duplicates the same logic used in _navigateTo, so extract it into a
shared helper on Router to keep both navigation paths consistent. Create a small
method such as _resolveGuard that takes the path, guardResult, and redirect
action and returns a clear outcome like stop, redirect, or continue, then use it
from replace and _navigateTo instead of maintaining two separate copies of the
depth/error logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d57cf997-0040-450a-98ee-e69f6c903fe3
📒 Files selected for processing (2)
packages/router/src/router.test.tspackages/router/src/router.ts
|
|
||
| it('beforeEnter infinite redirect loop is detected and prevented', () => { | ||
| const r = new Router({ maxRedirectDepth: 3 }); | ||
| const errorFn = vi.fn(); | ||
| r.events.on('error', errorFn); | ||
|
|
||
| // Create infinite redirect loop: /a -> /b -> /a -> /b -> ... | ||
| r.addRoute('/a', () => 'A', undefined, { beforeEnter: () => '/b' }); | ||
| r.addRoute('/b', () => 'B', undefined, { beforeEnter: () => '/a' }); | ||
|
|
||
| r.push('/a'); | ||
|
|
||
| expect(errorFn).toHaveBeenCalled(); | ||
| expect(errorFn.mock.calls[0][0].message).toContain('Maximum redirect depth'); | ||
| }); | ||
|
|
||
| it('replace with infinite redirect loop is detected and prevented', () => { | ||
| const r = new Router({ maxRedirectDepth: 3 }); | ||
| const errorFn = vi.fn(); | ||
| r.events.on('error', errorFn); | ||
|
|
||
| // Create infinite redirect loop: /a -> /b -> /a -> /b -> ... | ||
| r.addRoute('/a', () => 'A', undefined, { beforeEnter: () => '/b' }); | ||
| r.addRoute('/b', () => 'B', undefined, { beforeEnter: () => '/a' }); | ||
|
|
||
| r.replace('/a'); | ||
|
|
||
| expect(errorFn).toHaveBeenCalled(); | ||
| expect(errorFn.mock.calls[0][0].message).toContain('Maximum redirect depth'); | ||
| }); | ||
|
|
||
| it('beforeEnter redirect depth is reset on successful navigation', () => { | ||
| const r = new Router({ maxRedirectDepth: 3 }); | ||
| const errorFn = vi.fn(); | ||
| r.events.on('error', errorFn); | ||
|
|
||
| // Create redirect chain that doesn't loop: /a -> /b -> /c | ||
| r.addRoute('/a', () => 'A', undefined, { beforeEnter: () => '/b' }); | ||
| r.addRoute('/b', () => 'B', undefined, { beforeEnter: () => '/c' }); | ||
| r.addRoute('/c', () => 'C'); | ||
|
|
||
| r.push('/a'); | ||
|
|
||
| expect(errorFn).not.toHaveBeenCalled(); | ||
| expect(r.currentPath).toBe('/c'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tests assert observable behavior (error emission, message content, resulting currentPath) and use the real Router — good.
One coverage gap worth adding: a case where a beforeEnter redirect targets a non-existent route, then a subsequent legitimate redirect chain navigates successfully. This would guard against the _redirectDepth leak flagged in router.ts (no-match early return not resetting the counter).
🤖 Prompt for 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.
In `@packages/router/src/router.test.ts` around lines 244 - 289, Add a test in
Router coverage for a `beforeEnter` redirect to a missing route followed by a
normal redirect chain, using the real `Router` and `events.on('error')` like the
existing `beforeEnter`/`replace` tests. The new case should exercise the
no-match path in `Router` navigation (where `_redirectDepth` can leak) by
redirecting from one route to a non-existent target, then performing a
successful redirect sequence and asserting no unexpected error plus the final
`currentPath` is correct. Reference the existing `Router`, `beforeEnter`,
`push`, and `replace` test patterns so it’s easy to place alongside the current
redirect-depth tests.
| /** Maximum redirect chain depth before throwing error (default: 10) */ | ||
| maxRedirectDepth?: number; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc says "throwing error" but the router emits an error event instead.
The implementation calls this.events.emit('error', ...) and returns; it never throws. Consumers reading this doc may wrap push()/replace() in try/catch and silently miss the loop condition (which, with no error listener registered, is swallowed by EventEmitter.emit). Align the wording with the actual behavior.
📝 Suggested doc tweak
- /** Maximum redirect chain depth before throwing error (default: 10) */
+ /** Maximum redirect chain depth before an `error` event is emitted and navigation aborts (default: 10) */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Maximum redirect chain depth before throwing error (default: 10) */ | |
| maxRedirectDepth?: number; | |
| /** Maximum redirect chain depth before an `error` event is emitted and navigation aborts (default: 10) */ |
🤖 Prompt for 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.
In `@packages/router/src/router.ts` around lines 37 - 38, Update the router option
doc comment for maxRedirectDepth so it matches the actual behavior in router.ts:
it does not throw, it emits an error event via this.events.emit('error', ...)
and returns. Adjust the wording in the Router interface/option docs to say the
redirect chain limit triggers an error event instead of a thrown error, so
consumers using push() or replace() won’t expect try/catch handling.
1 participant
Description
This PR fixes an issue where
beforeEnterroute guards could redirect between routes indefinitely, resulting in infinite navigation loops that could freeze the application or cause a stack overflow.The router now tracks redirect depth during a navigation and throws an error when the configured maximum redirect limit is exceeded, preventing runaway redirect chains while preserving valid redirect behavior.
Related Issue
Closes #1918
Which package(s)?
Type of Change
type:bug)type:testing)Checklist
bun vitest runbun run buildbun run typecheckCONTRIBUTING.md.type: short description.markDirty()(if applicable).anytypes without an inline comment explaining why.GSSoC 2026 Participation
Notes for the Reviewer
Changes
maxRedirectDepthoption toRouterOptions(default:10).push()andreplace()navigation flows.Tests
push().replace().This change is backward compatible for existing applications while preventing application hangs caused by misconfigured route guards.
Summary by CodeRabbit