Skip to content

Commit a3d7239

Browse files
committed
Bug fix: When getting logged out, logging back in redirected you to /resources/platform-changelogs
Root cause recap: Background fetcher (useRecentChangelogs polling /resources/platform-changelogs every 60s) was the first thing to hit requireUserId after a session loss. That stuffed its own URL into ?redirectTo=..., the login flow stored it in a cookie, and /magic faithfully redirected the user to a JSON-only fetcher route after sign-in → blank page. Pre-existing bug, surfaced more often by the new session-expiry feature. Fixed by sanitizing redirectTo at three layers.
1 parent 434ca8f commit a3d7239

4 files changed

Lines changed: 38 additions & 10 deletions

File tree

apps/webapp/app/routes/login.magic/route.tsx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { TextLink } from "~/components/primitives/TextLink";
2323
import { authenticator } from "~/services/auth.server";
2424
import { commitSession, getUserSession } from "~/services/sessionStorage.server";
2525
import { setRedirectTo, commitSession as commitRedirectSession } from "~/services/redirectTo.server";
26+
import { sanitizeRedirectPath } from "~/utils";
2627
import {
2728
checkMagicLinkEmailRateLimit,
2829
checkMagicLinkEmailDailyRateLimit,
@@ -60,11 +61,14 @@ export async function loader({ request }: LoaderFunctionArgs) {
6061
const session = await getUserSession(request);
6162
const error = session.get("auth:error");
6263

63-
// Get redirectTo from URL params and store in session if present
64+
// Get redirectTo from URL params and store in session if present.
65+
// Sanitize to drop non-page paths (fetcher routes, callbacks) which would
66+
// render blank if the user was sent there post-login.
6467
const url = new URL(request.url);
65-
const redirectTo = url.searchParams.get("redirectTo");
68+
const sanitized = sanitizeRedirectPath(url.searchParams.get("redirectTo"));
69+
const redirectTo = sanitized === "/" ? null : sanitized;
6670
const headers = new Headers();
67-
71+
6872
if (redirectTo) {
6973
const redirectSession = await setRedirectTo(request, redirectTo);
7074
headers.append("Set-Cookie", await commitRedirectSession(redirectSession));

apps/webapp/app/routes/magic.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,13 @@ import { getRedirectTo } from "~/services/redirectTo.server";
88
import { commitSession, getSession } from "~/services/sessionStorage.server";
99
import { commitAuthenticatedSession } from "~/services/sessionDuration.server";
1010
import { trackAndClearReferralSource } from "~/services/referralSource.server";
11+
import { sanitizeRedirectPath } from "~/utils";
1112

1213
export async function loader({ request }: LoaderFunctionArgs) {
13-
const redirectTo = await getRedirectTo(request);
14+
// Defense-in-depth: sanitize the cookie value to drop non-page paths in case
15+
// a stale cookie from before sanitization shipped is still in the browser.
16+
const sanitized = sanitizeRedirectPath(await getRedirectTo(request));
17+
const redirectTo = sanitized === "/" ? undefined : sanitized;
1418

1519
const auth = await authenticator.authenticate("email-link", request, {
1620
failureRedirect: "/login/magic", // If auth fails, the failureRedirect will be thrown as a Response

apps/webapp/app/services/session.server.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { redirect } from "@remix-run/node";
22
import { getUserById } from "~/models/user.server";
3+
import { sanitizeRedirectPath } from "~/utils";
34
import { authenticator } from "./auth.server";
45
import { getImpersonationId } from "./impersonation.server";
56
import { getEffectiveSessionDuration, isSessionExpired } from "./sessionDuration.server";
@@ -50,9 +51,11 @@ export async function requireUserId(request: Request, redirectTo?: string) {
5051
const userId = await getUserId(request);
5152
if (!userId) {
5253
const url = new URL(request.url);
53-
const searchParams = new URLSearchParams([
54-
["redirectTo", redirectTo ?? `${url.pathname}${url.search}`],
55-
]);
54+
// Only propagate the originating URL when it's a real user-navigable page.
55+
// Fetcher endpoints (e.g. /resources/*) and auth callbacks would render
56+
// blank or loop if used as a post-login destination.
57+
const finalRedirectTo = sanitizeRedirectPath(redirectTo ?? `${url.pathname}${url.search}`);
58+
const searchParams = new URLSearchParams([["redirectTo", finalRedirectTo]]);
5659
throw redirect(`/login?${searchParams}`);
5760
}
5861
return userId;

apps/webapp/app/utils.ts

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,22 @@ import { useMatches } from "@remix-run/react";
33

44
const DEFAULT_REDIRECT = "/";
55

6+
// Pathnames that are NOT user-navigable destinations: fetcher endpoints,
7+
// OAuth/auth callbacks, JSON APIs, the magic-link redemption route, and the
8+
// auth flow routes themselves (which would create a redirect loop).
9+
const NON_NAVIGABLE_PREFIXES = ["/resources/", "/auth/", "/admin/", "/api/", "/engine/"];
10+
const NON_NAVIGABLE_EXACT = new Set(["/magic", "/logout", "/login", "/login/magic", "/login/mfa"]);
11+
12+
function isNavigablePath(pathname: string): boolean {
13+
if (NON_NAVIGABLE_EXACT.has(pathname)) return false;
14+
return !NON_NAVIGABLE_PREFIXES.some((prefix) => pathname.startsWith(prefix));
15+
}
16+
617
/**
718
* This should be used any time the redirect path is user-provided
819
* (Like the query string on our login/signup pages). This avoids
9-
* open-redirect vulnerabilities.
20+
* open-redirect vulnerabilities and prevents redirecting users to
21+
* non-page routes (e.g. fetcher endpoints) that would render blank.
1022
* @param {string} path The redirect destination
1123
* @param {string} defaultRedirect The redirect to use if the to is unsafe.
1224
*/
@@ -28,16 +40,21 @@ export function sanitizeRedirectPath(
2840
return defaultRedirect;
2941
} catch {}
3042

43+
let parsed: URL;
3144
try {
3245
// ensure it's a valid relative path
33-
const url = new URL(path, "https://example.com");
34-
if (url.hostname !== "example.com") {
46+
parsed = new URL(path, "https://example.com");
47+
if (parsed.hostname !== "example.com") {
3548
return defaultRedirect;
3649
}
3750
} catch {
3851
return defaultRedirect;
3952
}
4053

54+
if (!isNavigablePath(parsed.pathname)) {
55+
return defaultRedirect;
56+
}
57+
4158
return path;
4259
}
4360

0 commit comments

Comments
 (0)