Skip to content

Commit 0a3f0c1

Browse files
jbrunton96Frooodle
andauthored
Fix redirect bugs in SaaS (#7721)
# Description of Changes Fixes various bugs that affected SaaS (and some self-hosted): - Refreshing on Editor caused the user to be redirected to Processor - User was unable to access Processor in SaaS - Deep link hijacking fixes - Fix double prefix `/app/app` issue - Fix going from tool -> editor -> processor -> editor putting you back into tool --------- Co-authored-by: Anthony Stirling <77850077+Frooodle@users.noreply.github.com>
1 parent 849d616 commit 0a3f0c1

23 files changed

Lines changed: 545 additions & 83 deletions

frontend/editor/src/core/components/shared/quickNav/QuickNavRailHost.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ const SIZE = "1.125rem";
2020
export function QuickNavRailHost() {
2121
const { t } = useTranslation();
2222
const navigate = useNavigate();
23-
const { pathname, search } = useLocation();
23+
const { pathname } = useLocation();
2424
const host = useQuickNavHost();
2525

2626
const appMounted = Boolean(host?.appMounted);
@@ -74,7 +74,7 @@ export function QuickNavRailHost() {
7474
returnHome();
7575
return;
7676
}
77-
saveEditorReturnPath(pathname + search);
77+
saveEditorReturnPath();
7878
go(PORTAL_BASENAME);
7979
},
8080
},

frontend/editor/src/core/contexts/ToolWorkflowContext.tsx

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ import {
2929
isBaseWorkbench,
3030
} from "@app/types/workbench";
3131
import { useNavigationUrlSync } from "@app/hooks/useUrlSync";
32+
import { stripBasePath } from "@app/constants/app";
33+
import { EDITOR_BASENAME } from "@app/routes/editorBasename";
3234
import { filterToolRegistryByQuery } from "@app/utils/toolSearch";
3335
import { useToolHistory } from "@app/hooks/tools/useUserToolActivity";
3436
import {
@@ -373,15 +375,28 @@ export function ToolWorkflowProvider({ children }: ToolWorkflowProviderProps) {
373375
// This runs once to navigate to the user's preferred tab (read/automate)
374376
// instead of always starting on the tools tab.
375377
const hasAppliedStartupView = React.useRef(false);
378+
// Set when the startup view picks the tool, so the URL sync knows this
379+
// selection came from a preference and must not be written to the address.
380+
const startupSelectedToolRef = React.useRef<ToolId | null>(null);
376381
useEffect(() => {
377382
if (hasAppliedStartupView.current) return;
383+
// The URL wins: the startup view decides what you see when you arrive at the
384+
// editor's home, never what a deep link to a tool shows. Without this, a
385+
// "Reader" preference rewrote every /<tool> link to /read.
386+
const path = stripBasePath(window.location.pathname);
387+
if (path !== "/" && path !== EDITOR_BASENAME) {
388+
hasAppliedStartupView.current = true;
389+
return;
390+
}
378391
const startupView = preferences.defaultStartupView;
379392
if (startupView === "read") {
380393
hasAppliedStartupView.current = true;
394+
startupSelectedToolRef.current = "read";
381395
setReaderMode(true);
382396
actions.setSelectedTool("read");
383397
} else if (startupView === "automate") {
384398
hasAppliedStartupView.current = true;
399+
startupSelectedToolRef.current = "automate";
385400
actions.setSelectedTool("automate");
386401
setLeftPanelView("toolContent");
387402
}
@@ -573,6 +588,7 @@ export function ToolWorkflowProvider({ children }: ToolWorkflowProviderProps) {
573588
handleBackToTools,
574589
allTools,
575590
true,
591+
startupSelectedToolRef,
576592
);
577593

578594
// Ref-backed wrappers so callback identities stay stable across renders.
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
import { beforeEach, describe, expect, it, vi } from "vitest";
2+
import { renderHook } from "@testing-library/react";
3+
import { useRef } from "react";
4+
import type { ToolId } from "@app/types/toolId";
5+
6+
const h = vi.hoisted(() => ({
7+
updateToolRoute: vi.fn(),
8+
clearToolRoute: vi.fn(),
9+
}));
10+
11+
vi.mock("@app/utils/urlRouting", () => ({
12+
parseToolRoute: () => ({ workbench: "fileEditor", toolId: null }),
13+
updateToolRoute: h.updateToolRoute,
14+
clearToolRoute: h.clearToolRoute,
15+
}));
16+
vi.mock("@app/utils/scarfTracking", () => ({ firePixel: vi.fn() }));
17+
vi.mock("@app/contexts/AppConfigContext", () => ({
18+
useAppConfig: () => ({ config: { premiumEnabled: true } }),
19+
}));
20+
21+
import { useNavigationUrlSync } from "@app/hooks/useUrlSync";
22+
23+
const registry = {
24+
read: { name: "Read", workbench: "viewer" },
25+
compress: { name: "Compress", workbench: "fileEditor" },
26+
} as never;
27+
28+
/** Drives the hook the way ToolWorkflowContext does, with a startup marker. */
29+
function useHarness(selectedTool: ToolId | null, startupTool: ToolId | null) {
30+
const ref = useRef<ToolId | null>(startupTool);
31+
useNavigationUrlSync(selectedTool, vi.fn(), vi.fn(), registry, true, ref);
32+
return ref;
33+
}
34+
35+
describe("useNavigationUrlSync — startup-view selections", () => {
36+
beforeEach(() => h.updateToolRoute.mockClear());
37+
38+
// The default-startup-view preference selects a tool to change the *view*.
39+
// Writing it to the address turned every visit to /editor into /read.
40+
it("never writes the URL for the startup-applied tool", () => {
41+
const { rerender } = renderHook(
42+
({ tool }: { tool: ToolId | null }) => useHarness(tool, "read" as ToolId),
43+
{ initialProps: { tool: null as ToolId | null } },
44+
);
45+
rerender({ tool: "read" as ToolId });
46+
expect(h.updateToolRoute).not.toHaveBeenCalled();
47+
});
48+
49+
// The effect re-runs whenever the registry identity changes, so a marker that
50+
// was consumed on first sight let the second run write /read anyway.
51+
it("survives a re-run for the same tool", () => {
52+
const { rerender } = renderHook(
53+
({ tool }: { tool: ToolId | null }) => useHarness(tool, "read" as ToolId),
54+
{ initialProps: { tool: null as ToolId | null } },
55+
);
56+
rerender({ tool: "read" as ToolId });
57+
rerender({ tool: "read" as ToolId });
58+
rerender({ tool: "read" as ToolId });
59+
expect(h.updateToolRoute).not.toHaveBeenCalled();
60+
});
61+
62+
it("still writes the URL when the user picks a different tool", () => {
63+
const { rerender } = renderHook(
64+
({ tool }: { tool: ToolId | null }) => useHarness(tool, "read" as ToolId),
65+
{ initialProps: { tool: null as ToolId | null } },
66+
);
67+
rerender({ tool: "read" as ToolId });
68+
rerender({ tool: "compress" as ToolId });
69+
expect(h.updateToolRoute).toHaveBeenCalledWith("compress", registry, false);
70+
});
71+
72+
it("writes the URL for a tool chosen without a startup marker", () => {
73+
const { rerender } = renderHook(
74+
({ tool }: { tool: ToolId | null }) => useHarness(tool, null),
75+
{ initialProps: { tool: null as ToolId | null } },
76+
);
77+
rerender({ tool: "read" as ToolId });
78+
expect(h.updateToolRoute).toHaveBeenCalledWith("read", registry, false);
79+
});
80+
});

frontend/editor/src/core/hooks/useUrlSync.ts

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
* URL synchronization hooks for tool routing with registry support
33
*/
44

5-
import { useEffect, useCallback, useRef } from "react";
5+
import { useEffect, useCallback, useRef, type MutableRefObject } from "react";
66
import { ToolId } from "@app/types/toolId";
77
import {
88
parseToolRoute,
@@ -24,6 +24,11 @@ export function useNavigationUrlSync(
2424
clearToolSelection: () => void,
2525
registry: ToolRegistry,
2626
enableSync: boolean = true,
27+
/**
28+
* Tool the default-startup-view preference selected, if any. That selection
29+
* sets the view, not the address, so it must not be written to the URL.
30+
*/
31+
startupSelectedToolRef?: MutableRefObject<ToolId | null>,
2732
) {
2833
const { config } = useAppConfig();
2934
const premiumEnabled = config?.premiumEnabled;
@@ -77,8 +82,16 @@ export function useNavigationUrlSync(
7782
useEffect(() => {
7883
if (!enableSync) return;
7984

85+
const startupTool = startupSelectedToolRef?.current ?? null;
86+
8087
if (selectedTool) {
81-
updateToolRoute(selectedTool, registry, false); // Use pushState for user navigation
88+
// A startup-view selection is a view preference, not a navigation: writing
89+
// it here rewrote /editor to /read on every load. The effect re-runs
90+
// whenever the registry identity changes, so the marker has to survive
91+
// until the selection actually moves off it (cleared below).
92+
if (startupTool !== selectedTool) {
93+
updateToolRoute(selectedTool, registry, false); // Use pushState for user navigation
94+
}
8295
} else if (prevSelectedTool.current !== null) {
8396
// Only clear URL if we had a tool before (user navigated away)
8497
// Don't clear on initial load when both current and previous are null
@@ -88,8 +101,19 @@ export function useNavigationUrlSync(
88101
}
89102
}
90103

104+
// Spent once the user leaves the startup-applied tool, so re-picking it
105+
// later is a real navigation and does update the URL.
106+
if (
107+
startupSelectedToolRef &&
108+
startupTool !== null &&
109+
prevSelectedTool.current === startupTool &&
110+
selectedTool !== startupTool
111+
) {
112+
startupSelectedToolRef.current = null;
113+
}
114+
91115
prevSelectedTool.current = selectedTool;
92-
}, [selectedTool, registry, enableSync]);
116+
}, [selectedTool, registry, enableSync, startupSelectedToolRef]);
93117

94118
// Handle browser back/forward navigation
95119
useEffect(() => {
Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
2+
3+
vi.mock("@app/components/toast", () => ({ alert: vi.fn() }));
4+
vi.mock("@app/services/specialErrorToasts", () => ({
5+
showSpecialErrorToast: vi.fn(() => false),
6+
}));
7+
vi.mock("@app/services/saasErrorInterceptor", () => ({
8+
handleSaaSError: vi.fn(() => false),
9+
}));
10+
vi.mock("@app/services/errorUtils", () => ({
11+
broadcastErroredFiles: vi.fn(),
12+
extractErrorFileIds: vi.fn(() => []),
13+
normalizeAxiosErrorData: vi.fn(async (d: unknown) => d),
14+
}));
15+
16+
const hrefs: string[] = [];
17+
18+
/** Serve the app from `base`, sitting on `pathname`, then load the handler fresh. */
19+
async function loadAt(base: string, pathname: string) {
20+
document.head.innerHTML = `<base href="${base}" />`;
21+
Object.defineProperty(window, "location", {
22+
configurable: true,
23+
value: {
24+
pathname,
25+
search: "",
26+
origin: "http://localhost:3000",
27+
get href() {
28+
// Absolute: jsdom resolves <base href> against this.
29+
return "http://localhost:3000" + pathname;
30+
},
31+
set href(v: string) {
32+
hrefs.push(v);
33+
},
34+
},
35+
});
36+
vi.resetModules();
37+
return (await import("@app/services/httpErrorHandler")).handleHttpError;
38+
}
39+
40+
const unauthorized = {
41+
isAxiosError: true,
42+
message: "unauthorized",
43+
config: {},
44+
response: { status: 401, data: {} },
45+
};
46+
47+
beforeEach(() => {
48+
hrefs.length = 0;
49+
sessionStorage.clear();
50+
localStorage.clear();
51+
});
52+
afterEach(() => vi.resetModules());
53+
54+
describe("401 return path is router-relative", () => {
55+
// Login replays this through navigate(), which re-applies the router
56+
// basename. Carrying /app here produced /app/app/compress.
57+
it("strips the base path on a subpath deploy", async () => {
58+
const handle = await loadAt("/app/", "/app/compress");
59+
await handle(unauthorized);
60+
61+
expect(sessionStorage.getItem("stirling_post_login_path")).toBe(
62+
"/compress",
63+
);
64+
expect(hrefs[0]).toBe("/app/login?from=%2Fcompress");
65+
});
66+
67+
it("is unchanged at the origin root", async () => {
68+
const handle = await loadAt("/", "/compress");
69+
await handle(unauthorized);
70+
71+
expect(sessionStorage.getItem("stirling_post_login_path")).toBe(
72+
"/compress",
73+
);
74+
expect(hrefs[0]).toBe("/login?from=%2Fcompress");
75+
});
76+
});

frontend/editor/src/core/services/httpErrorHandler.ts

Lines changed: 8 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ import {
1212
clampText,
1313
extractAxiosErrorMessage,
1414
} from "@app/services/httpErrorUtils";
15-
import { withBasePath } from "@app/constants/app";
15+
import { stripBasePath, withBasePath } from "@app/constants/app";
16+
import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect";
1617

1718
// Module-scoped state to reduce global variable usage
1819
const recentSpecialByEndpoint: Record<string, number> = {};
@@ -21,26 +22,9 @@ const SPECIAL_SUPPRESS_MS = 1500; // brief window to suppress generic duplicate
2122
// Mirrors the key in proprietary/auth/springAuthClient.ts; AuthCallback consumes it.
2223
const POST_LOGIN_REDIRECT_STORAGE_KEY = "stirling_post_login_path";
2324

24-
function isSafePostLoginPath(path: string): boolean {
25-
if (
26-
!path.startsWith("/") ||
27-
path.startsWith("//") ||
28-
path.startsWith("/\\")
29-
) {
30-
return false;
31-
}
32-
const lowered = path.toLowerCase();
33-
return (
34-
!lowered.startsWith("/login") &&
35-
!lowered.startsWith("/auth/") &&
36-
!lowered.startsWith("/oauth2") &&
37-
!lowered.startsWith("/saml2")
38-
);
39-
}
40-
4125
function stashPostLoginRedirect(path: string): void {
4226
try {
43-
if (typeof window === "undefined" || !isSafePostLoginPath(path)) return;
27+
if (typeof window === "undefined" || !isSafePostLoginRedirect(path)) return;
4428
window.sessionStorage.setItem(POST_LOGIN_REDIRECT_STORAGE_KEY, path);
4529
} catch {
4630
// sessionStorage unavailable (private mode) — fail open
@@ -128,7 +112,11 @@ export async function handleHttpError(error: unknown): Promise<boolean> {
128112
console.debug("[httpErrorHandler] 401 detected, redirecting to login");
129113
// Spring 302-strips the ?from= query from /login, so stash the return
130114
// path in sessionStorage (AuthCallback reads it after SSO round-trip).
131-
const currentLocation = window.location.pathname + window.location.search;
115+
// Router-relative, not browser-relative: every consumer replays this
116+
// through navigate(), which re-applies the basename. Keeping the base
117+
// path here yields /app/app/<tool> on a subpath deploy.
118+
const currentLocation =
119+
stripBasePath(window.location.pathname) + window.location.search;
132120
stashPostLoginRedirect(currentLocation);
133121
let hadStoredJwt = false;
134122
try {
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import { describe, expect, it } from "vitest";
2+
import { isSafePostLoginRedirect } from "@app/services/postLoginRedirect";
3+
4+
// Core default. Rejects off-origin forms and the auth routes every build has
5+
// (/login, /auth/…); Spring SSO routes are the proprietary override's concern.
6+
describe("isSafePostLoginRedirect (core base)", () => {
7+
it("accepts same-origin router paths", () => {
8+
expect(isSafePostLoginRedirect("/editor")).toBe(true);
9+
expect(isSafePostLoginRedirect("/compress")).toBe(true);
10+
expect(isSafePostLoginRedirect("/editor?foo=bar")).toBe(true);
11+
expect(isSafePostLoginRedirect("/oauth/consent?x=1")).toBe(true);
12+
expect(isSafePostLoginRedirect("/")).toBe(true);
13+
});
14+
15+
it("rejects empty and non-string values", () => {
16+
expect(isSafePostLoginRedirect(null)).toBe(false);
17+
expect(isSafePostLoginRedirect(undefined)).toBe(false);
18+
expect(isSafePostLoginRedirect("")).toBe(false);
19+
expect(isSafePostLoginRedirect(42 as unknown)).toBe(false);
20+
});
21+
22+
it("rejects off-origin and protocol-relative forms", () => {
23+
expect(isSafePostLoginRedirect("//evil.example.com")).toBe(false);
24+
expect(isSafePostLoginRedirect("/\\evil.example.com")).toBe(false);
25+
expect(isSafePostLoginRedirect("https://evil.example.com")).toBe(false);
26+
expect(isSafePostLoginRedirect("editor")).toBe(false);
27+
});
28+
29+
it("rejects the universal auth routes so returning back can never loop", () => {
30+
expect(isSafePostLoginRedirect("/login")).toBe(false);
31+
expect(isSafePostLoginRedirect("/login?next=%2Feditor")).toBe(false);
32+
expect(isSafePostLoginRedirect("/auth/callback")).toBe(false);
33+
});
34+
35+
it("leaves the Spring SSO routes to the proprietary override", () => {
36+
expect(isSafePostLoginRedirect("/oauth2/authorize")).toBe(true);
37+
expect(isSafePostLoginRedirect("/saml2/login")).toBe(true);
38+
});
39+
});
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
/**
2+
* Is `path` safe to send a user back to after they log in?
3+
*/
4+
export function isSafePostLoginRedirect(path: unknown): path is string {
5+
if (typeof path !== "string" || path.length === 0) return false;
6+
if (
7+
!path.startsWith("/") ||
8+
path.startsWith("//") ||
9+
path.startsWith("/\\")
10+
) {
11+
return false;
12+
}
13+
const lowered = path.toLowerCase();
14+
return !lowered.startsWith("/login") && !lowered.startsWith("/auth/");
15+
}

0 commit comments

Comments
 (0)