Skip to content

fix: cap loader redirect hops and reject cycles #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SebTardif wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: cap loader redirect hops and reject cycles #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 5 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
95 changes: 76 additions & 19 deletions src/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>;
};

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<string>();
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;
Expand Down Expand Up @@ -134,6 +166,7 @@ export function createRouter<
context: TLoadContext,
navigationOptions: RouterNavigationOptions = {},
requestedLocation = locationForPath(compiled.pathForRoute(routeId, basePath)),
redirectFollow?: RedirectFollowState,
): Promise<void> => {
const route = compiled.byId.get(routeId);
if (!route) {
Expand Down Expand Up @@ -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();
Expand All @@ -336,7 +378,7 @@ export function createRouter<
...current,
status,
isFetching: false,
error,
error: failure,
updatedAt: Date.now(),
}));
if (!targetPublished) {
Expand All @@ -352,7 +394,7 @@ export function createRouter<
if (isCurrentRun(currentRun, run)) {
currentRun = null;
}
throw error;
throw failure;
}
if (!hookOptions.shouldRun()) {
return;
Expand Down Expand Up @@ -432,6 +474,7 @@ export function createRouter<
context: TLoadContext,
revalidate = false,
historyMode: RouterNavigationOptions["history"] = "none",
redirectFollow?: RedirectFollowState,
): Promise<void> => {
const normalized = normalizeLocation(location);
const matched = compiled.routeIdFromPath(normalized.pathname, basePath);
Expand All @@ -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<void> => {
const route = compiled.byId.get(routeId);
if (!route) {
Expand Down Expand Up @@ -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;
Expand All @@ -509,10 +560,16 @@ export function createRouter<
const preloadRoute = (routeId: TRouteId, context: TLoadContext): Promise<void> =>
preloadAtLocation(routeId, context, locationForPath(compiled.pathForRoute(routeId, basePath)));

const preloadLocation = (location: RouteLocation, context: TLoadContext): Promise<void> => {
const preloadLocation = (
location: RouteLocation,
context: TLoadContext,
redirectFollow?: RedirectFollowState,
): Promise<void> => {
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 {
Expand Down
146 changes: 145 additions & 1 deletion test/router-control-results.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
};
Expand Down Expand Up @@ -50,6 +50,150 @@ describe("router control results", () => {
});
});

it("follows a loader redirect chain under the hop cap", async () => {
const router = createRouter<RouteId, TestContext, TestModule, TestData>({
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<RouteId, TestContext, TestModule, TestData>({
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<string, TestContext, TestModule, TestData>({
routes: ids.map((id, index) =>
definePage<string, TestContext, TestModule, TestData>({
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<RouteId, TestContext, TestModule, TestData>({
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<RouteId, TestContext, TestModule, TestData>({
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<RouteId, TestContext, TestModule, TestData>({
Expand Down
Loading