From ef429488c6398954bab38b2142185f3e23c555f7 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Sat, 15 Aug 2026 16:48:55 -0700 Subject: [PATCH] fix: cap loader redirect hops and reject cycles Loader redirect() follows had no hop budget or cycle set, so A to B to A recursed in handleLocation and preload until hang or heap exhaustion. Cap follows at 10 hops, reject repeated locations, and throw a clear error on both navigation and preload paths. Signed-off-by: Sebastien Tardif --- CHANGELOG.md | 5 + README.md | 7 +- src/router.ts | 95 ++++++++++++++---- test/router-control-results.test.ts | 146 +++++++++++++++++++++++++++- 4 files changed, 231 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5844dc5..2a73a39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Fixed + +- Cap loader redirect follows at 10 hops and reject cycles so A to B to A + cannot hang navigation or preload. + ### CI - Refresh development dependencies, pnpm, and pinned GitHub Actions without changing the router API or runtime requirements. diff --git a/README.md b/README.md index 1ba5950..2a28a2f 100644 --- a/README.md +++ b/README.md @@ -151,8 +151,11 @@ definePage({ ``` When a redirect is thrown during a real navigation (not a preload), the router -chases it with `history: "replace"`. A `notFound` sets the router status to -`"notFound"` and exposes the payload on the match's `error`. +chases it with `history: "replace"`. Preload follows the same redirects in the +cache only. Both paths stop after 10 hops or if a location repeats, and they +throw a `Redirect hop limit` or `Redirect cycle detected` error. A `notFound` +sets the router status to `"notFound"` and exposes the payload on the match's +`error`. Unmatched locations also produce `notFound` router state. Applications decide how to present or redirect that state; the router does not choose a default diff --git a/src/router.ts b/src/router.ts index 3b6f2e6..76dcb1f 100644 --- a/src/router.ts +++ b/src/router.ts @@ -41,6 +41,38 @@ const DEFAULT_STALE_TIME = 0; const DEFAULT_STALE_RELOAD_MODE = "background" as const; const DEFAULT_PRELOAD_STALE_TIME = 30_000; const DEFAULT_GC_TIME = 30 * 60_000; +const MAX_REDIRECT_HOPS = 10; + +type RedirectFollowState = { + hops: number; + seen: Set; +}; + +function locationKey(location: RouteLocation): string { + const normalized = normalizeLocation(location); + return `${normalized.pathname}${normalized.search}${normalized.hash}`; +} + +function beginRedirectFollow( + from: RouteLocation, + to: RouteLocation, + previous?: RedirectFollowState, +): RedirectFollowState { + const hops = (previous?.hops ?? 0) + 1; + const seen = previous?.seen ?? new Set(); + const fromKey = locationKey(from); + const toKey = locationKey(to); + seen.add(fromKey); + if (hops > MAX_REDIRECT_HOPS) { + throw new Error( + `Redirect hop limit of ${MAX_REDIRECT_HOPS} exceeded while following ${fromKey} -> ${toKey}.`, + ); + } + if (seen.has(toKey)) { + throw new Error(`Redirect cycle detected: ${[...seen, toKey].join(" -> ")}.`); + } + return { hops, seen }; +} function isCurrentRun(current: NavigationRun | null, run: NavigationRun): boolean { return current === run && !run.controller.signal.aborted; @@ -134,6 +166,7 @@ export function createRouter< context: TLoadContext, navigationOptions: RouterNavigationOptions = {}, requestedLocation = locationForPath(compiled.pathForRoute(routeId, basePath)), + redirectFollow?: RedirectFollowState, ): Promise => { const route = compiled.byId.get(routeId); if (!route) { @@ -305,22 +338,31 @@ export function createRouter< if (!hookOptions.shouldRun()) { return; } + let failure = error; if (isRouteRedirect(error)) { - matches.updateMatch(match.id, (current) => ({ - ...current, - status: "redirected", - isFetching: false, - error, - updatedAt: Date.now(), - })); - matches.setStatus("redirected"); - currentRun = null; - if (hookOptions.cause !== "preload") { - await handleLocation(error.location, context, false, "replace"); + let follow: RedirectFollowState | undefined; + try { + follow = beginRedirectFollow(location, error.location, redirectFollow); + } catch (redirectError) { + failure = redirectError; + } + if (follow) { + matches.updateMatch(match.id, (current) => ({ + ...current, + status: "redirected", + isFetching: false, + error, + updatedAt: Date.now(), + })); + matches.setStatus("redirected"); + currentRun = null; + if (hookOptions.cause !== "preload") { + await handleLocation(error.location, context, false, "replace", follow); + } + return; } - return; } - const status = isRouteNotFound(error) ? "notFound" : "error"; + const status = isRouteNotFound(failure) ? "notFound" : "error"; const failedMatch = matches.getMatch(match.id); if (failedMatch) { const currentActive = targetPublished ? previous : matches.getActiveMatch(); @@ -336,7 +378,7 @@ export function createRouter< ...current, status, isFetching: false, - error, + error: failure, updatedAt: Date.now(), })); if (!targetPublished) { @@ -352,7 +394,7 @@ export function createRouter< if (isCurrentRun(currentRun, run)) { currentRun = null; } - throw error; + throw failure; } if (!hookOptions.shouldRun()) { return; @@ -432,6 +474,7 @@ export function createRouter< context: TLoadContext, revalidate = false, historyMode: RouterNavigationOptions["history"] = "none", + redirectFollow?: RedirectFollowState, ): Promise => { const normalized = normalizeLocation(location); const matched = compiled.routeIdFromPath(normalized.pathname, basePath); @@ -446,13 +489,20 @@ export function createRouter< }); return; } - await navigate(matched, context, { history: historyMode, revalidate }, normalized); + await navigate( + matched, + context, + { history: historyMode, revalidate }, + normalized, + redirectFollow, + ); }; const preloadAtLocation = ( routeId: TRouteId, context: TLoadContext, location: RouteLocation, + redirectFollow?: RedirectFollowState, ): Promise => { const route = compiled.byId.get(routeId); if (!route) { @@ -499,7 +549,8 @@ export function createRouter< .catch((error: unknown) => { if (isRouteRedirect(error)) { matches.removeCached(match.id); - return preloadLocation(error.location, context); + const follow = beginRedirectFollow(location, error.location, redirectFollow); + return preloadLocation(error.location, context, follow); } matches.removeCached(match.id); return undefined; @@ -509,10 +560,16 @@ export function createRouter< const preloadRoute = (routeId: TRouteId, context: TLoadContext): Promise => preloadAtLocation(routeId, context, locationForPath(compiled.pathForRoute(routeId, basePath))); - const preloadLocation = (location: RouteLocation, context: TLoadContext): Promise => { + const preloadLocation = ( + location: RouteLocation, + context: TLoadContext, + redirectFollow?: RedirectFollowState, + ): Promise => { const normalized = normalizeLocation(location); const routeId = compiled.routeIdFromPath(normalized.pathname, basePath); - return routeId ? preloadAtLocation(routeId, context, normalized) : Promise.resolve(); + return routeId + ? preloadAtLocation(routeId, context, normalized, redirectFollow) + : Promise.resolve(); }; return { diff --git a/test/router-control-results.test.ts b/test/router-control-results.test.ts index 89b588c..59bebd9 100644 --- a/test/router-control-results.test.ts +++ b/test/router-control-results.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { createRouter, definePage, notFound, redirect, type RouteLocation } from "../src/index"; -type RouteId = "source" | "target" | "missing"; +type RouteId = "source" | "target" | "missing" | "alpha" | "beta" | "gamma"; type TestContext = { label: string; }; @@ -50,6 +50,150 @@ describe("router control results", () => { }); }); + it("follows a loader redirect chain under the hop cap", async () => { + const router = createRouter({ + routes: [ + definePage<"alpha", TestContext, TestModule, TestData>({ + id: "alpha", + path: "/alpha", + component: () => ({ view: "alpha" }), + loader: () => redirect(location("/beta")), + }), + definePage<"beta", TestContext, TestModule, TestData>({ + id: "beta", + path: "/beta", + component: () => ({ view: "beta" }), + loader: () => redirect(location("/gamma")), + }), + definePage<"gamma", TestContext, TestModule, TestData>({ + id: "gamma", + path: "/gamma", + component: () => ({ view: "gamma" }), + loader: (context) => ({ label: context.label }), + }), + ], + }); + + await router.navigate("alpha", { label: "chained" }); + + const state = router.getState(); + const [match] = state.matches; + expect(state.status).toBe("success"); + expect(state.location).toEqual(location("/gamma")); + expect(match).toMatchObject({ + routeId: "gamma", + status: "success", + data: { label: "chained" }, + module: { view: "gamma" }, + }); + }); + + it("rejects a loader redirect cycle during navigation", async () => { + const router = createRouter({ + routes: [ + definePage<"alpha", TestContext, TestModule, TestData>({ + id: "alpha", + path: "/alpha", + component: () => ({ view: "alpha" }), + loader: () => redirect(location("/beta")), + }), + definePage<"beta", TestContext, TestModule, TestData>({ + id: "beta", + path: "/beta", + component: () => ({ view: "beta" }), + loader: () => redirect(location("/alpha")), + }), + ], + }); + + await expect(router.navigate("alpha", { label: "loop" })).rejects.toThrow( + /Redirect cycle detected: \/alpha -> \/beta -> \/alpha/, + ); + expect(router.getState().status).toBe("error"); + }); + + it("rejects navigation after the redirect hop limit", async () => { + const hopCount = 11; + const ids = Array.from({ length: hopCount + 1 }, (_, index) => `hop${index}`); + const router = createRouter({ + routes: ids.map((id, index) => + definePage({ + id, + path: `/${id}`, + component: () => ({ view: id }), + loader: + index < hopCount + ? () => redirect(location(`/${ids[index + 1]}`)) + : (context) => ({ label: context.label }), + }), + ), + }); + + await expect(router.navigate("hop0", { label: "over" })).rejects.toThrow( + /Redirect hop limit of 10 exceeded while following \/hop10 -> \/hop11/, + ); + expect(router.getState().status).toBe("error"); + }); + + it("follows a preload redirect chain under the hop cap", async () => { + const router = createRouter({ + routes: [ + definePage<"alpha", TestContext, TestModule, TestData>({ + id: "alpha", + path: "/alpha", + component: () => ({ view: "alpha" }), + loader: () => redirect(location("/beta")), + }), + definePage<"beta", TestContext, TestModule, TestData>({ + id: "beta", + path: "/beta", + component: () => ({ view: "beta" }), + loader: () => redirect(location("/gamma")), + }), + definePage<"gamma", TestContext, TestModule, TestData>({ + id: "gamma", + path: "/gamma", + component: () => ({ view: "gamma" }), + loader: (context) => ({ label: context.label }), + }), + ], + }); + + await router.preloadRoute("alpha", { label: "preloaded" }); + + const cached = router.getState().cachedMatches; + expect(cached).toHaveLength(1); + expect(cached[0]).toMatchObject({ + routeId: "gamma", + status: "success", + preload: true, + data: { label: "preloaded" }, + }); + }); + + it("rejects a loader redirect cycle during preload", async () => { + const router = createRouter({ + routes: [ + definePage<"alpha", TestContext, TestModule, TestData>({ + id: "alpha", + path: "/alpha", + component: () => ({ view: "alpha" }), + loader: () => redirect(location("/beta")), + }), + definePage<"beta", TestContext, TestModule, TestData>({ + id: "beta", + path: "/beta", + component: () => ({ view: "beta" }), + loader: () => redirect(location("/alpha")), + }), + ], + }); + + await expect(router.preloadRoute("alpha", { label: "loop" })).rejects.toThrow( + /Redirect cycle detected: \/alpha -> \/beta -> \/alpha/, + ); + }); + it("publishes not-found route state from loaders", async () => { const result = notFound({ code: "missing-record" }); const router = createRouter({