From 112fbfdef31e5ff838faf2cf5bb51faed8fe2e26 Mon Sep 17 00:00:00 2001 From: James Brunton Date: Fri, 28 Aug 2026 09:09:35 +0100 Subject: [PATCH] Fix redirect bug on SaaS --- .../src/core/services/httpErrorHandler.ts | 20 +------- .../core/services/postLoginRedirect.test.ts | 39 ++++++++++++++ .../src/core/services/postLoginRedirect.ts | 11 ++++ .../auth/spring/springAuthClient.ts | 22 ++------ .../services/postLoginRedirect.test.ts | 25 +++++++++ .../proprietary/services/postLoginRedirect.ts | 7 +++ .../editor/src/saas/routes/AuthCallback.tsx | 21 ++++---- frontend/editor/src/saas/routes/Login.tsx | 13 ++--- .../src/saas/services/apiClient.test.ts | 51 +++++++++++++++++-- .../editor/src/saas/services/apiClient.ts | 25 ++++++--- 10 files changed, 172 insertions(+), 62 deletions(-) create mode 100644 frontend/editor/src/core/services/postLoginRedirect.test.ts create mode 100644 frontend/editor/src/core/services/postLoginRedirect.ts create mode 100644 frontend/editor/src/proprietary/services/postLoginRedirect.test.ts create mode 100644 frontend/editor/src/proprietary/services/postLoginRedirect.ts diff --git a/frontend/editor/src/core/services/httpErrorHandler.ts b/frontend/editor/src/core/services/httpErrorHandler.ts index 616b8effea..30272c381e 100644 --- a/frontend/editor/src/core/services/httpErrorHandler.ts +++ b/frontend/editor/src/core/services/httpErrorHandler.ts @@ -13,6 +13,7 @@ import { extractAxiosErrorMessage, } from "@app/services/httpErrorUtils"; import { stripBasePath, withBasePath } from "@app/constants/app"; +import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect"; // Module-scoped state to reduce global variable usage const recentSpecialByEndpoint: Record = {}; @@ -21,26 +22,9 @@ const SPECIAL_SUPPRESS_MS = 1500; // brief window to suppress generic duplicate // Mirrors the key in proprietary/auth/springAuthClient.ts; AuthCallback consumes it. const POST_LOGIN_REDIRECT_STORAGE_KEY = "stirling_post_login_path"; -function isSafePostLoginPath(path: string): boolean { - if ( - !path.startsWith("/") || - path.startsWith("//") || - path.startsWith("/\\") - ) { - return false; - } - const lowered = path.toLowerCase(); - return ( - !lowered.startsWith("/login") && - !lowered.startsWith("/auth/") && - !lowered.startsWith("/oauth2") && - !lowered.startsWith("/saml2") - ); -} - function stashPostLoginRedirect(path: string): void { try { - if (typeof window === "undefined" || !isSafePostLoginPath(path)) return; + if (typeof window === "undefined" || !isSafePostLoginRedirect(path)) return; window.sessionStorage.setItem(POST_LOGIN_REDIRECT_STORAGE_KEY, path); } catch { // sessionStorage unavailable (private mode) — fail open diff --git a/frontend/editor/src/core/services/postLoginRedirect.test.ts b/frontend/editor/src/core/services/postLoginRedirect.test.ts new file mode 100644 index 0000000000..9f1687f392 --- /dev/null +++ b/frontend/editor/src/core/services/postLoginRedirect.test.ts @@ -0,0 +1,39 @@ +import { describe, expect, it } from "vitest"; +import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect"; + +// Core default. Rejects off-origin forms and the auth routes every build has +// (/login, /auth/…); Spring SSO routes are the proprietary override's concern. +describe("isSafePostLoginRedirect (core base)", () => { + it("accepts same-origin router paths", () => { + expect(isSafePostLoginRedirect("/editor")).toBe(true); + expect(isSafePostLoginRedirect("/compress")).toBe(true); + expect(isSafePostLoginRedirect("/editor?foo=bar")).toBe(true); + expect(isSafePostLoginRedirect("/oauth/consent?x=1")).toBe(true); + expect(isSafePostLoginRedirect("/")).toBe(true); + }); + + it("rejects empty and non-string values", () => { + expect(isSafePostLoginRedirect(null)).toBe(false); + expect(isSafePostLoginRedirect(undefined)).toBe(false); + expect(isSafePostLoginRedirect("")).toBe(false); + expect(isSafePostLoginRedirect(42 as unknown)).toBe(false); + }); + + it("rejects off-origin and protocol-relative forms", () => { + expect(isSafePostLoginRedirect("//evil.example.com")).toBe(false); + expect(isSafePostLoginRedirect("/\\evil.example.com")).toBe(false); + expect(isSafePostLoginRedirect("https://evil.example.com")).toBe(false); + expect(isSafePostLoginRedirect("editor")).toBe(false); + }); + + it("rejects the universal auth routes so returning back can never loop", () => { + expect(isSafePostLoginRedirect("/login")).toBe(false); + expect(isSafePostLoginRedirect("/login?next=%2Feditor")).toBe(false); + expect(isSafePostLoginRedirect("/auth/callback")).toBe(false); + }); + + it("leaves the Spring SSO routes to the proprietary override", () => { + expect(isSafePostLoginRedirect("/oauth2/authorize")).toBe(true); + expect(isSafePostLoginRedirect("/saml2/login")).toBe(true); + }); +}); diff --git a/frontend/editor/src/core/services/postLoginRedirect.ts b/frontend/editor/src/core/services/postLoginRedirect.ts new file mode 100644 index 0000000000..7386ab8a7f --- /dev/null +++ b/frontend/editor/src/core/services/postLoginRedirect.ts @@ -0,0 +1,11 @@ +/** + * Is `path` safe to send a user back to after they log in? + */ +export function isSafePostLoginRedirect(path: unknown): path is string { + if (typeof path !== "string" || path.length === 0) return false; + if (!path.startsWith("/") || path.startsWith("//") || path.startsWith("/\\")) { + return false; + } + const lowered = path.toLowerCase(); + return !lowered.startsWith("/login") && !lowered.startsWith("/auth/"); +} diff --git a/frontend/editor/src/proprietary/auth/spring/springAuthClient.ts b/frontend/editor/src/proprietary/auth/spring/springAuthClient.ts index b9583e3dee..6d887ce8a9 100644 --- a/frontend/editor/src/proprietary/auth/spring/springAuthClient.ts +++ b/frontend/editor/src/proprietary/auth/spring/springAuthClient.ts @@ -16,6 +16,7 @@ import { AxiosError, type AxiosRequestConfig } from "axios"; import { getSpringAuthConfig } from "@app/auth/config"; import { type OAuthProvider } from "@app/auth/spring/oauthTypes"; import { resetOAuthState } from "@app/auth/spring/oauthStorage"; +import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect"; import type { AuthUser as User, AuthSession as Session, @@ -100,23 +101,10 @@ function persistRedirectPath(path: string): void { } } -// Same-origin relative path, not pointing at auth plumbing. Rejects protocol-relative -// URLs to guard against open-redirect abuse if the stored value is tampered with. -export function isSafePostLoginRedirect(path: unknown): path is string { - if (typeof path !== "string" || path.length === 0) return false; - if (!path.startsWith("/") || path.startsWith("//")) return false; - if (path.startsWith("/\\")) return false; - const lowered = path.toLowerCase(); - if ( - lowered.startsWith("/login") || - lowered.startsWith("/auth/") || - lowered.startsWith("/oauth2") || - lowered.startsWith("/saml2") - ) { - return false; - } - return true; -} +// The safe-return-path rule lives in the shared @app/services/postLoginRedirect +// extension point (proprietary override adds the Spring SSO routes). Re-exported +// here so existing importers via @app/auth keep resolving it. +export { isSafePostLoginRedirect }; export function setPostLoginRedirectPath( path: string | null | undefined, diff --git a/frontend/editor/src/proprietary/services/postLoginRedirect.test.ts b/frontend/editor/src/proprietary/services/postLoginRedirect.test.ts new file mode 100644 index 0000000000..6f2451f336 --- /dev/null +++ b/frontend/editor/src/proprietary/services/postLoginRedirect.test.ts @@ -0,0 +1,25 @@ +import { describe, expect, it } from "vitest"; +import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect"; + +// Proprietary override: the core base plus the Spring SSO routes (/oauth2, /saml2). +describe("isSafePostLoginRedirect (proprietary override)", () => { + it("still accepts ordinary router paths", () => { + expect(isSafePostLoginRedirect("/editor")).toBe(true); + expect(isSafePostLoginRedirect("/share/abc123?x=1")).toBe(true); + expect(isSafePostLoginRedirect("/")).toBe(true); + }); + + it("inherits the base rejections", () => { + expect(isSafePostLoginRedirect("")).toBe(false); + expect(isSafePostLoginRedirect(null)).toBe(false); + expect(isSafePostLoginRedirect("//evil.example")).toBe(false); + expect(isSafePostLoginRedirect("/\\evil")).toBe(false); + expect(isSafePostLoginRedirect("/login")).toBe(false); + expect(isSafePostLoginRedirect("/auth/callback")).toBe(false); + }); + + it("also rejects the Spring SSO routes", () => { + expect(isSafePostLoginRedirect("/oauth2/authorization/google")).toBe(false); + expect(isSafePostLoginRedirect("/saml2/authenticate/x")).toBe(false); + }); +}); diff --git a/frontend/editor/src/proprietary/services/postLoginRedirect.ts b/frontend/editor/src/proprietary/services/postLoginRedirect.ts new file mode 100644 index 0000000000..5cd6546fa7 --- /dev/null +++ b/frontend/editor/src/proprietary/services/postLoginRedirect.ts @@ -0,0 +1,7 @@ +import { isSafePostLoginRedirect as isSafeBaseRedirect } from "@core/services/postLoginRedirect"; + +export function isSafePostLoginRedirect(path: unknown): path is string { + if (!isSafeBaseRedirect(path)) return false; + const lowered = path.toLowerCase(); + return !lowered.startsWith("/oauth2") && !lowered.startsWith("/saml2"); +} diff --git a/frontend/editor/src/saas/routes/AuthCallback.tsx b/frontend/editor/src/saas/routes/AuthCallback.tsx index b69619b85d..3fe081ed82 100644 --- a/frontend/editor/src/saas/routes/AuthCallback.tsx +++ b/frontend/editor/src/saas/routes/AuthCallback.tsx @@ -5,6 +5,7 @@ import { supabase } from "@app/auth/supabase"; import { Button } from "@app/ui/Button"; import { withBasePath } from "@app/constants/app"; import { readPendingConnect } from "@app/routes/pendingConnect"; +import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect"; import { AuthShell } from "@app/auth/ui/AuthShell"; import ErrorMessage from "@app/auth/ui/ErrorMessage"; import { Spinner } from "@app/ui/Spinner"; @@ -136,18 +137,16 @@ export default function AuthCallback() { // else on the editor. // Explicit `next` first, so a sign-in started for another reason is not // hijacked by a remembered connect request. - const explicitNext = url.searchParams.get("next"); + const explicitNext = + url.searchParams.get("next") ?? url.searchParams.get("from"); const pendingConnect = readPendingConnect(); - const destination = - explicitNext && - explicitNext.startsWith("/") && - !explicitNext.startsWith("//") - ? explicitNext - : pendingConnect - ? `/link?request=${encodeURIComponent(pendingConnect)}` - : next.startsWith("/") && !next.startsWith("//") - ? next - : await resolveLandingPath(); + const destination = isSafePostLoginRedirect(explicitNext) + ? explicitNext + : pendingConnect + ? `/link?request=${encodeURIComponent(pendingConnect)}` + : isSafePostLoginRedirect(next) + ? next + : await resolveLandingPath(); console.log("[Auth Callback Debug] Redirecting to:", destination); setTimeout(() => navigate(destination, { replace: true }), 1500); diff --git a/frontend/editor/src/saas/routes/Login.tsx b/frontend/editor/src/saas/routes/Login.tsx index 418616104e..ba251561fd 100644 --- a/frontend/editor/src/saas/routes/Login.tsx +++ b/frontend/editor/src/saas/routes/Login.tsx @@ -14,6 +14,7 @@ import { getBaseUrl, withBasePath, } from "@app/constants/app"; +import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect"; import LinkRoundedIcon from "@mui/icons-material/LinkRounded"; // Import login components @@ -48,14 +49,14 @@ export default function Login() { } }, []); - // Same-origin relative path to return to after login (e.g. the OAuth - // consent page). Same sanitization rules as AuthCallback's `next`. + // Same-origin router path to return to after login (e.g. the OAuth consent + // page, or the editor a 401 bounced the user off). `?next=` is what this app + // writes; `?from=` is what the shared core 401 handler writes. const nextPath = useMemo(() => { try { - const next = new URL(window.location.href).searchParams.get("next"); - return next && next.startsWith("/") && !next.startsWith("//") - ? next - : null; + const params = new URL(window.location.href).searchParams; + const candidate = params.get("next") ?? params.get("from"); + return isSafePostLoginRedirect(candidate) ? candidate : null; } catch (_) { return null; } diff --git a/frontend/editor/src/saas/services/apiClient.test.ts b/frontend/editor/src/saas/services/apiClient.test.ts index 45646bf8d8..af4a1d90f4 100644 --- a/frontend/editor/src/saas/services/apiClient.test.ts +++ b/frontend/editor/src/saas/services/apiClient.test.ts @@ -219,10 +219,11 @@ describe("apiClient", () => { // Import apiClient after mocking const { default: apiClient } = await import("@app/services/apiClient"); - // Mock window.location for redirect test + // On /editor when the session dies: the return path must ride along so the + // login screen sends the user back here, not to the role-based landing. Object.defineProperty(window, "location", { writable: true, - value: { href: "" }, + value: { href: "", pathname: "/editor", search: "" }, }); const mockAdapter = vi.fn((config) => { @@ -245,8 +246,50 @@ describe("apiClient", () => { } catch (_) { // Verify refresh was attempted expect(supabase.auth.refreshSession).toHaveBeenCalled(); - // Verify redirect to login - expect(window.location.href).toBe("/login"); + // Verify redirect to login carries the return path + expect(window.location.href).toBe("/login?next=%2Feditor"); + } + }); + + it("does not redirect (or loop) when already on the login page", async () => { + expectConsole.error(/\[API Client\] Token refresh failed/); + const oldSession = { access_token: "old", user: { id: "user-123" } }; + vi.mocked(supabase.auth.getSession).mockResolvedValue({ + data: { session: oldSession as unknown as Session }, + error: null, + }); + vi.mocked(supabase.auth.refreshSession).mockResolvedValue({ + data: { user: null, session: null }, + error: { + name: "AuthError", + message: "Refresh failed", + status: 400, + code: "auth_error", + } as unknown as AuthError, + }); + + const { default: apiClient } = await import("@app/services/apiClient"); + + Object.defineProperty(window, "location", { + writable: true, + value: { href: "", pathname: "/login", search: "?next=%2Feditor" }, + }); + + apiClient.defaults.adapter = vi.fn((config) => + Promise.reject( + Object.assign(new Error("Unauthorized"), { + response: { status: 401, data: { error: "Unauthorized" } }, + config, + }), + ), + ); + + try { + await apiClient.get("/api/v1/test"); + expect(true).toBe(false); + } catch (_) { + // Left untouched: no second redirect off the login page. + expect(window.location.href).toBe(""); } }); }); diff --git a/frontend/editor/src/saas/services/apiClient.ts b/frontend/editor/src/saas/services/apiClient.ts index 64cd05fd01..73ebe97e47 100644 --- a/frontend/editor/src/saas/services/apiClient.ts +++ b/frontend/editor/src/saas/services/apiClient.ts @@ -5,8 +5,9 @@ import { classifyPaygError, handlePaygError, } from "@app/services/paygErrorInterceptor"; -import { withBasePath } from "@app/constants/app"; +import { stripBasePath, withBasePath } from "@app/constants/app"; import { getBrowserId } from "@app/utils/browserIdentifier"; +import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect"; // Helper: decode base64url JWT payload safely function decodeJwtPayload(token: string): Record | null { @@ -113,6 +114,21 @@ function refreshSessionOnce(): ReturnType { return inFlightRefresh; } +// Hard-redirect to /login, carrying where the user was so the login screen can +// return them there instead of falling through to the role-based landing (which +// sends processor users to the processor - the "refresh /editor bounces me to +// the processor" bug). Router-relative, matching what Login reads via `?next=`. +function redirectToLogin(): void { + const loginPath = withBasePath("/login"); + // Already on the login page: another redirect would just loop. + if (window.location.pathname === loginPath) return; + const returnPath = + stripBasePath(window.location.pathname) + window.location.search; + window.location.href = isSafePostLoginRedirect(returnPath) + ? `${loginPath}?next=${encodeURIComponent(returnPath)}` + : loginPath; +} + // Response interceptor for handling token refresh apiClient.interceptors.response.use( (response) => response, @@ -173,7 +189,7 @@ apiClient.interceptors.response.use( // The session genuinely can't be recovered. Send protected requests // to login; public ones just fail quietly (no redirect). if (!isPublicEndpoint) { - window.location.href = withBasePath("/login"); + redirectToLogin(); } return Promise.reject(error); @@ -194,10 +210,7 @@ apiClient.interceptors.response.use( console.debug( "[API Client] No session to refresh, 401 on protected endpoint", ); - const loginPath = withBasePath("/login"); - if (window.location.pathname !== loginPath) { - window.location.href = loginPath; - } + redirectToLogin(); return Promise.reject(error); } } catch (refreshError) {