diff --git a/frontend/editor/src/core/components/shared/quickNav/QuickNavRailHost.tsx b/frontend/editor/src/core/components/shared/quickNav/QuickNavRailHost.tsx index 5c748b36cc..3fba4d7e1f 100644 --- a/frontend/editor/src/core/components/shared/quickNav/QuickNavRailHost.tsx +++ b/frontend/editor/src/core/components/shared/quickNav/QuickNavRailHost.tsx @@ -20,7 +20,7 @@ const SIZE = "1.125rem"; export function QuickNavRailHost() { const { t } = useTranslation(); const navigate = useNavigate(); - const { pathname, search } = useLocation(); + const { pathname } = useLocation(); const host = useQuickNavHost(); const appMounted = Boolean(host?.appMounted); @@ -74,7 +74,7 @@ export function QuickNavRailHost() { returnHome(); return; } - saveEditorReturnPath(pathname + search); + saveEditorReturnPath(); go(PORTAL_BASENAME); }, }, diff --git a/frontend/editor/src/core/contexts/ToolWorkflowContext.tsx b/frontend/editor/src/core/contexts/ToolWorkflowContext.tsx index 070442a966..b4ef874184 100644 --- a/frontend/editor/src/core/contexts/ToolWorkflowContext.tsx +++ b/frontend/editor/src/core/contexts/ToolWorkflowContext.tsx @@ -29,6 +29,8 @@ import { isBaseWorkbench, } from "@app/types/workbench"; import { useNavigationUrlSync } from "@app/hooks/useUrlSync"; +import { stripBasePath } from "@app/constants/app"; +import { EDITOR_BASENAME } from "@app/routes/editorBasename"; import { filterToolRegistryByQuery } from "@app/utils/toolSearch"; import { useToolHistory } from "@app/hooks/tools/useUserToolActivity"; import { @@ -373,15 +375,28 @@ export function ToolWorkflowProvider({ children }: ToolWorkflowProviderProps) { // This runs once to navigate to the user's preferred tab (read/automate) // instead of always starting on the tools tab. const hasAppliedStartupView = React.useRef(false); + // Set when the startup view picks the tool, so the URL sync knows this + // selection came from a preference and must not be written to the address. + const startupSelectedToolRef = React.useRef(null); useEffect(() => { if (hasAppliedStartupView.current) return; + // The URL wins: the startup view decides what you see when you arrive at the + // editor's home, never what a deep link to a tool shows. Without this, a + // "Reader" preference rewrote every / link to /read. + const path = stripBasePath(window.location.pathname); + if (path !== "/" && path !== EDITOR_BASENAME) { + hasAppliedStartupView.current = true; + return; + } const startupView = preferences.defaultStartupView; if (startupView === "read") { hasAppliedStartupView.current = true; + startupSelectedToolRef.current = "read"; setReaderMode(true); actions.setSelectedTool("read"); } else if (startupView === "automate") { hasAppliedStartupView.current = true; + startupSelectedToolRef.current = "automate"; actions.setSelectedTool("automate"); setLeftPanelView("toolContent"); } @@ -573,6 +588,7 @@ export function ToolWorkflowProvider({ children }: ToolWorkflowProviderProps) { handleBackToTools, allTools, true, + startupSelectedToolRef, ); // Ref-backed wrappers so callback identities stay stable across renders. diff --git a/frontend/editor/src/core/hooks/useUrlSync.test.tsx b/frontend/editor/src/core/hooks/useUrlSync.test.tsx new file mode 100644 index 0000000000..e35c7dbd44 --- /dev/null +++ b/frontend/editor/src/core/hooks/useUrlSync.test.tsx @@ -0,0 +1,80 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { renderHook } from "@testing-library/react"; +import { useRef } from "react"; +import type { ToolId } from "@app/types/toolId"; + +const h = vi.hoisted(() => ({ + updateToolRoute: vi.fn(), + clearToolRoute: vi.fn(), +})); + +vi.mock("@app/utils/urlRouting", () => ({ + parseToolRoute: () => ({ workbench: "fileEditor", toolId: null }), + updateToolRoute: h.updateToolRoute, + clearToolRoute: h.clearToolRoute, +})); +vi.mock("@app/utils/scarfTracking", () => ({ firePixel: vi.fn() })); +vi.mock("@app/contexts/AppConfigContext", () => ({ + useAppConfig: () => ({ config: { premiumEnabled: true } }), +})); + +import { useNavigationUrlSync } from "@app/hooks/useUrlSync"; + +const registry = { + read: { name: "Read", workbench: "viewer" }, + compress: { name: "Compress", workbench: "fileEditor" }, +} as never; + +/** Drives the hook the way ToolWorkflowContext does, with a startup marker. */ +function useHarness(selectedTool: ToolId | null, startupTool: ToolId | null) { + const ref = useRef(startupTool); + useNavigationUrlSync(selectedTool, vi.fn(), vi.fn(), registry, true, ref); + return ref; +} + +describe("useNavigationUrlSync — startup-view selections", () => { + beforeEach(() => h.updateToolRoute.mockClear()); + + // The default-startup-view preference selects a tool to change the *view*. + // Writing it to the address turned every visit to /editor into /read. + it("never writes the URL for the startup-applied tool", () => { + const { rerender } = renderHook( + ({ tool }: { tool: ToolId | null }) => useHarness(tool, "read" as ToolId), + { initialProps: { tool: null as ToolId | null } }, + ); + rerender({ tool: "read" as ToolId }); + expect(h.updateToolRoute).not.toHaveBeenCalled(); + }); + + // The effect re-runs whenever the registry identity changes, so a marker that + // was consumed on first sight let the second run write /read anyway. + it("survives a re-run for the same tool", () => { + const { rerender } = renderHook( + ({ tool }: { tool: ToolId | null }) => useHarness(tool, "read" as ToolId), + { initialProps: { tool: null as ToolId | null } }, + ); + rerender({ tool: "read" as ToolId }); + rerender({ tool: "read" as ToolId }); + rerender({ tool: "read" as ToolId }); + expect(h.updateToolRoute).not.toHaveBeenCalled(); + }); + + it("still writes the URL when the user picks a different tool", () => { + const { rerender } = renderHook( + ({ tool }: { tool: ToolId | null }) => useHarness(tool, "read" as ToolId), + { initialProps: { tool: null as ToolId | null } }, + ); + rerender({ tool: "read" as ToolId }); + rerender({ tool: "compress" as ToolId }); + expect(h.updateToolRoute).toHaveBeenCalledWith("compress", registry, false); + }); + + it("writes the URL for a tool chosen without a startup marker", () => { + const { rerender } = renderHook( + ({ tool }: { tool: ToolId | null }) => useHarness(tool, null), + { initialProps: { tool: null as ToolId | null } }, + ); + rerender({ tool: "read" as ToolId }); + expect(h.updateToolRoute).toHaveBeenCalledWith("read", registry, false); + }); +}); diff --git a/frontend/editor/src/core/hooks/useUrlSync.ts b/frontend/editor/src/core/hooks/useUrlSync.ts index 5fad71ba29..76fd67178a 100644 --- a/frontend/editor/src/core/hooks/useUrlSync.ts +++ b/frontend/editor/src/core/hooks/useUrlSync.ts @@ -2,7 +2,7 @@ * URL synchronization hooks for tool routing with registry support */ -import { useEffect, useCallback, useRef } from "react"; +import { useEffect, useCallback, useRef, type MutableRefObject } from "react"; import { ToolId } from "@app/types/toolId"; import { parseToolRoute, @@ -24,6 +24,11 @@ export function useNavigationUrlSync( clearToolSelection: () => void, registry: ToolRegistry, enableSync: boolean = true, + /** + * Tool the default-startup-view preference selected, if any. That selection + * sets the view, not the address, so it must not be written to the URL. + */ + startupSelectedToolRef?: MutableRefObject, ) { const { config } = useAppConfig(); const premiumEnabled = config?.premiumEnabled; @@ -77,8 +82,16 @@ export function useNavigationUrlSync( useEffect(() => { if (!enableSync) return; + const startupTool = startupSelectedToolRef?.current ?? null; + if (selectedTool) { - updateToolRoute(selectedTool, registry, false); // Use pushState for user navigation + // A startup-view selection is a view preference, not a navigation: writing + // it here rewrote /editor to /read on every load. The effect re-runs + // whenever the registry identity changes, so the marker has to survive + // until the selection actually moves off it (cleared below). + if (startupTool !== selectedTool) { + updateToolRoute(selectedTool, registry, false); // Use pushState for user navigation + } } else if (prevSelectedTool.current !== null) { // Only clear URL if we had a tool before (user navigated away) // Don't clear on initial load when both current and previous are null @@ -88,8 +101,19 @@ export function useNavigationUrlSync( } } + // Spent once the user leaves the startup-applied tool, so re-picking it + // later is a real navigation and does update the URL. + if ( + startupSelectedToolRef && + startupTool !== null && + prevSelectedTool.current === startupTool && + selectedTool !== startupTool + ) { + startupSelectedToolRef.current = null; + } + prevSelectedTool.current = selectedTool; - }, [selectedTool, registry, enableSync]); + }, [selectedTool, registry, enableSync, startupSelectedToolRef]); // Handle browser back/forward navigation useEffect(() => { diff --git a/frontend/editor/src/core/services/httpErrorHandler.basePath.test.ts b/frontend/editor/src/core/services/httpErrorHandler.basePath.test.ts new file mode 100644 index 0000000000..27a4231a2c --- /dev/null +++ b/frontend/editor/src/core/services/httpErrorHandler.basePath.test.ts @@ -0,0 +1,76 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@app/components/toast", () => ({ alert: vi.fn() })); +vi.mock("@app/services/specialErrorToasts", () => ({ + showSpecialErrorToast: vi.fn(() => false), +})); +vi.mock("@app/services/saasErrorInterceptor", () => ({ + handleSaaSError: vi.fn(() => false), +})); +vi.mock("@app/services/errorUtils", () => ({ + broadcastErroredFiles: vi.fn(), + extractErrorFileIds: vi.fn(() => []), + normalizeAxiosErrorData: vi.fn(async (d: unknown) => d), +})); + +const hrefs: string[] = []; + +/** Serve the app from `base`, sitting on `pathname`, then load the handler fresh. */ +async function loadAt(base: string, pathname: string) { + document.head.innerHTML = ``; + Object.defineProperty(window, "location", { + configurable: true, + value: { + pathname, + search: "", + origin: "http://localhost:3000", + get href() { + // Absolute: jsdom resolves against this. + return "http://localhost:3000" + pathname; + }, + set href(v: string) { + hrefs.push(v); + }, + }, + }); + vi.resetModules(); + return (await import("@app/services/httpErrorHandler")).handleHttpError; +} + +const unauthorized = { + isAxiosError: true, + message: "unauthorized", + config: {}, + response: { status: 401, data: {} }, +}; + +beforeEach(() => { + hrefs.length = 0; + sessionStorage.clear(); + localStorage.clear(); +}); +afterEach(() => vi.resetModules()); + +describe("401 return path is router-relative", () => { + // Login replays this through navigate(), which re-applies the router + // basename. Carrying /app here produced /app/app/compress. + it("strips the base path on a subpath deploy", async () => { + const handle = await loadAt("/app/", "/app/compress"); + await handle(unauthorized); + + expect(sessionStorage.getItem("stirling_post_login_path")).toBe( + "/compress", + ); + expect(hrefs[0]).toBe("/app/login?from=%2Fcompress"); + }); + + it("is unchanged at the origin root", async () => { + const handle = await loadAt("/", "/compress"); + await handle(unauthorized); + + expect(sessionStorage.getItem("stirling_post_login_path")).toBe( + "/compress", + ); + expect(hrefs[0]).toBe("/login?from=%2Fcompress"); + }); +}); diff --git a/frontend/editor/src/core/services/httpErrorHandler.ts b/frontend/editor/src/core/services/httpErrorHandler.ts index dd0c692d97..30272c381e 100644 --- a/frontend/editor/src/core/services/httpErrorHandler.ts +++ b/frontend/editor/src/core/services/httpErrorHandler.ts @@ -12,7 +12,8 @@ import { clampText, extractAxiosErrorMessage, } from "@app/services/httpErrorUtils"; -import { withBasePath } from "@app/constants/app"; +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 @@ -128,7 +112,11 @@ export async function handleHttpError(error: unknown): Promise { console.debug("[httpErrorHandler] 401 detected, redirecting to login"); // Spring 302-strips the ?from= query from /login, so stash the return // path in sessionStorage (AuthCallback reads it after SSO round-trip). - const currentLocation = window.location.pathname + window.location.search; + // Router-relative, not browser-relative: every consumer replays this + // through navigate(), which re-applies the basename. Keeping the base + // path here yields /app/app/ on a subpath deploy. + const currentLocation = + stripBasePath(window.location.pathname) + window.location.search; stashPostLoginRedirect(currentLocation); let hadStoredJwt = false; try { 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..d9ffe82940 --- /dev/null +++ b/frontend/editor/src/core/services/postLoginRedirect.ts @@ -0,0 +1,15 @@ +/** + * 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/core/services/workbenchSession.test.ts b/frontend/editor/src/core/services/workbenchSession.test.ts index d9feec8d1c..0258b83623 100644 --- a/frontend/editor/src/core/services/workbenchSession.test.ts +++ b/frontend/editor/src/core/services/workbenchSession.test.ts @@ -55,10 +55,14 @@ describe("workbench session record", () => { }); describe("editor return path", () => { - it("is consumed by the first take", () => { - saveEditorReturnPath("/compress?x=1"); + it("captures the live address bar and is consumed by the first take", () => { + // The editor writes its tool route via raw history.pushState, so the save + // must read window.location, not a lagging router location. + window.history.pushState({}, "", "/compress?x=1"); + saveEditorReturnPath(); expect(takeEditorReturnPath()).toBe("/compress?x=1"); expect(takeEditorReturnPath()).toBeNull(); + window.history.pushState({}, "", "/"); }); }); diff --git a/frontend/editor/src/core/services/workbenchSession.ts b/frontend/editor/src/core/services/workbenchSession.ts index 67f6d1abfc..2f23db8655 100644 --- a/frontend/editor/src/core/services/workbenchSession.ts +++ b/frontend/editor/src/core/services/workbenchSession.ts @@ -2,6 +2,7 @@ // does not cost the user their workbench. sessionStorage on purpose: per-tab, tabs never clobber. import type { StirlingFileStub } from "@app/types/fileContext"; +import { stripBasePath } from "@app/constants/app"; const SESSION_KEY = "stirling.workbench.session"; /** Bumped when the record's shape or meaning changes, so an old one is discarded rather than @@ -153,8 +154,10 @@ export function isSeedableView( return view !== undefined && SEEDABLE_VIEWS.includes(view); } -export function saveEditorReturnPath(path: string): void { +export function saveEditorReturnPath(): void { try { + const path = + stripBasePath(window.location.pathname) + window.location.search; sessionStorage.setItem(RETURN_PATH_KEY, path); } catch { // Best-effort: the switch back just lands on the editor root. 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/auth/supabase/UseSession.tsx b/frontend/editor/src/proprietary/auth/supabase/UseSession.tsx index 385f3f4e35..07670bfff5 100644 --- a/frontend/editor/src/proprietary/auth/supabase/UseSession.tsx +++ b/frontend/editor/src/proprietary/auth/supabase/UseSession.tsx @@ -16,6 +16,7 @@ import type { import { getSupabaseClient } from "@app/auth/supabase/supabaseClient"; import { AuthContext } from "@app/auth/context"; import { isAdminRole } from "@app/auth/roles"; +import { getApiBaseUrl } from "@app/services/apiClientConfig"; import { defaultTranslate, type AuthContextValue, @@ -154,14 +155,23 @@ export function SupabaseAuthProvider({ return; } let cancelled = false; + // Same API base the rest of the app uses: SaaS serves the frontend and the + // API from different hosts, so a root-relative path never reaches /me. + const apiBase = (getApiBaseUrl() || "").replace(/\/+$/, ""); + const meUrl = `${apiBase}/api/v1/auth/me`; const loadAccess = () => { - void fetch("/api/v1/auth/me", { + void fetch(meUrl, { headers: { Authorization: `Bearer ${token}`, Accept: "application/json", }, }) - .then((res) => (res.ok ? res.json() : null)) + .then((res) => { + // Must throw, not resolve null: swallowing a non-ok leaves + // portalAccess undefined and hangs the portal gate on a spinner. + if (!res.ok) throw new Error(`auth/me responded ${res.status}`); + return res.json(); + }) .then( ( data: { diff --git a/frontend/editor/src/proprietary/auth/supabase/portalAccessFetch.test.tsx b/frontend/editor/src/proprietary/auth/supabase/portalAccessFetch.test.tsx new file mode 100644 index 0000000000..3b3d9b37c8 --- /dev/null +++ b/frontend/editor/src/proprietary/auth/supabase/portalAccessFetch.test.tsx @@ -0,0 +1,128 @@ +import { beforeEach, afterEach, describe, expect, it, vi } from "vitest"; +import { render, waitFor } from "@testing-library/react"; +import { useContext } from "react"; + +const h = vi.hoisted(() => ({ apiBase: "/" })); + +vi.mock("@app/services/apiClientConfig", () => ({ + getApiBaseUrl: () => h.apiBase, +})); + +const sbSession = { + access_token: "supabase-token", + user: { + id: "u1", + email: "user@example.com", + is_anonymous: false, + app_metadata: {}, + user_metadata: {}, + }, +}; + +vi.mock("@app/auth/supabase/supabaseClient", () => ({ + getSupabaseClient: () => ({ + auth: { + getSession: () => Promise.resolve({ data: { session: sbSession } }), + onAuthStateChange: () => ({ + data: { subscription: { unsubscribe: () => {} } }, + }), + refreshSession: () => Promise.resolve({ data: {}, error: null }), + signOut: () => Promise.resolve({ error: null }), + }, + }), +})); + +import { SupabaseAuthProvider } from "@app/auth/supabase/UseSession"; +import { AuthContext } from "@app/auth/context"; + +function Probe() { + const v = useContext(AuthContext); + return ( + <> + {String(v?.portalAccess)} + {/* Raw, un-defaulted value: this is what SaasPortalGate reads to decide + "access not known yet" vs "denied". undefined = spinner forever. */} + {String(v?.user?.portalAccess)} + + ); +} + +const mount = () => + render( + + + , + ); + +let fetchMock: ReturnType; + +beforeEach(() => { + fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); +}); +afterEach(() => vi.unstubAllGlobals()); + +describe("supabase provider portalAccess lookup", () => { + // SaaS serves the frontend and the API from different hosts; a root-relative + // path silently missed /me, so a granted non-admin was denied the Processor. + it("calls /me on the configured API base, not the page origin", async () => { + h.apiBase = "https://api.example.com"; + fetchMock.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ user: { portalAccess: true } }), + }); + mount(); + await waitFor(() => + expect(fetchMock).toHaveBeenCalledWith( + "https://api.example.com/api/v1/auth/me", + expect.anything(), + ), + ); + }); + + it("keeps a same-origin base as a single leading slash", async () => { + h.apiBase = "/"; + fetchMock.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ user: { portalAccess: true } }), + }); + mount(); + await waitFor(() => + expect(fetchMock).toHaveBeenCalledWith( + "/api/v1/auth/me", + expect.anything(), + ), + ); + }); + + it("grants access when /me says so", async () => { + h.apiBase = "https://api.example.com"; + fetchMock.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ user: { portalAccess: true } }), + }); + const { getByTestId } = mount(); + await waitFor(() => expect(getByTestId("access").textContent).toBe("true")); + }); + + // A non-ok used to resolve to null and return early, leaving the raw + // portalAccess undefined forever - SaasPortalGate reads that as "still + // loading" and hangs on a spinner instead of falling back. + it("resolves the raw portalAccess when /me returns non-ok", async () => { + h.apiBase = "https://api.example.com"; + fetchMock.mockResolvedValue({ + ok: false, + status: 401, + json: () => Promise.resolve({}), + }); + const { getByTestId } = mount(); + await waitFor(() => expect(getByTestId("raw").textContent).toBe("false")); + }); + + it("leaves the raw portalAccess defined when the request rejects", async () => { + h.apiBase = "https://api.example.com"; + fetchMock.mockRejectedValue(new Error("network down")); + const { getByTestId } = mount(); + await waitFor(() => expect(getByTestId("raw").textContent).toBe("false")); + }); +}); diff --git a/frontend/editor/src/proprietary/hooks/useOtherAppSwitch.test.tsx b/frontend/editor/src/proprietary/hooks/useOtherAppSwitch.test.tsx index c2a81c53d9..4acdc7a17a 100644 --- a/frontend/editor/src/proprietary/hooks/useOtherAppSwitch.test.tsx +++ b/frontend/editor/src/proprietary/hooks/useOtherAppSwitch.test.tsx @@ -7,9 +7,10 @@ const mocks = vi.hoisted(() => ({ portalAccess: true, })); +// The hook reads window.location for the return path (not useLocation), because +// the editor's raw history.pushState leaves react-router's location stale. vi.mock("react-router-dom", () => ({ useNavigate: () => mocks.navigate, - useLocation: () => ({ pathname: "/compress", search: "?mode=fast" }), })); vi.mock("@app/auth/context", () => ({ useAuth: () => ({ portalAccess: mocks.portalAccess }), @@ -27,6 +28,7 @@ beforeEach(() => { sessionStorage.clear(); vi.clearAllMocks(); mocks.portalAccess = true; + window.history.pushState({}, "", "/"); }); describe("useOtherAppSwitch", () => { @@ -45,6 +47,7 @@ describe("useOtherAppSwitch", () => { }); it("records where to return to, then navigates to the processor", () => { + window.history.pushState({}, "", "/compress?mode=fast"); const { result } = renderHook(() => useOtherAppSwitch()); result.current?.onOpen(); mocks.requestNavigation.mock.calls[0][0](); diff --git a/frontend/editor/src/proprietary/hooks/useOtherAppSwitch.ts b/frontend/editor/src/proprietary/hooks/useOtherAppSwitch.ts index dff36a06f4..69b55aff35 100644 --- a/frontend/editor/src/proprietary/hooks/useOtherAppSwitch.ts +++ b/frontend/editor/src/proprietary/hooks/useOtherAppSwitch.ts @@ -1,4 +1,4 @@ -import { useLocation, useNavigate } from "react-router-dom"; +import { useNavigate } from "react-router-dom"; import { useAuth } from "@app/auth/context"; import { useNavigationActions } from "@app/contexts/NavigationContext"; import { PORTAL_BASENAME } from "@app/routes/portalBasename"; @@ -12,7 +12,6 @@ import { type NavFooterAppLink } from "@app/components/shared/navFooter/NavFoote export function useOtherAppSwitch(): NavFooterAppLink | null { const { portalAccess } = useAuth(); const navigate = useNavigate(); - const location = useLocation(); const { actions } = useNavigationActions(); if (!portalAccess) return null; return { @@ -20,7 +19,7 @@ export function useOtherAppSwitch(): NavFooterAppLink | null { onOpen: () => // Through the guard, so unsaved edits get the same warning as any other navigation. actions.requestNavigation(() => { - saveEditorReturnPath(location.pathname + location.search); + saveEditorReturnPath(); navigate(PORTAL_BASENAME); }), }; diff --git a/frontend/editor/src/proprietary/services/apiClientSetup.ts b/frontend/editor/src/proprietary/services/apiClientSetup.ts index 3f50e1bb4c..459f527080 100644 --- a/frontend/editor/src/proprietary/services/apiClientSetup.ts +++ b/frontend/editor/src/proprietary/services/apiClientSetup.ts @@ -1,5 +1,5 @@ import { AxiosInstance, AxiosError, InternalAxiosRequestConfig } from "axios"; -import { withBasePath } from "@app/constants/app"; +import { stripBasePath, withBasePath } from "@app/constants/app"; import { getBrowserId } from "@app/utils/browserIdentifier"; import { setPostLoginRedirectPath } from "@app/auth/spring/springAuthClient"; @@ -94,8 +94,10 @@ async function refreshAuthToken(client: AxiosInstance): Promise { // Redirect to login const loginPath = withBasePath("/login"); if (window.location.pathname !== loginPath) { + // Router-relative: Login replays this through navigate(), which applies + // the basename itself. See the same note in httpErrorHandler. setPostLoginRedirectPath( - window.location.pathname + window.location.search, + stripBasePath(window.location.pathname) + window.location.search, ); console.log("[API Client] Redirecting to login page..."); window.location.href = loginPath; 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/hooks/useOtherAppSwitch.ts b/frontend/editor/src/saas/hooks/useOtherAppSwitch.ts index 67d0aa7d49..e97d4d0ed3 100644 --- a/frontend/editor/src/saas/hooks/useOtherAppSwitch.ts +++ b/frontend/editor/src/saas/hooks/useOtherAppSwitch.ts @@ -1,4 +1,4 @@ -import { useLocation, useNavigate } from "react-router-dom"; +import { useNavigate } from "react-router-dom"; import { usePortalAccess } from "@app/hooks/usePortalAccess"; import { useNavigationActions } from "@app/contexts/NavigationContext"; import { PORTAL_BASENAME } from "@app/routes/portalBasename"; @@ -13,7 +13,6 @@ import { type NavFooterAppLink } from "@app/components/shared/navFooter/NavFoote export function useOtherAppSwitch(): NavFooterAppLink | null { const portalAccess = usePortalAccess(); const navigate = useNavigate(); - const location = useLocation(); const { actions } = useNavigationActions(); if (!portalAccess) return null; return { @@ -21,7 +20,7 @@ export function useOtherAppSwitch(): NavFooterAppLink | null { onOpen: () => // Through the guard, so unsaved edits get the same warning as any other navigation. actions.requestNavigation(() => { - saveEditorReturnPath(location.pathname + location.search); + saveEditorReturnPath(); navigate(PORTAL_BASENAME); }), }; 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) {