From 137f1ffc87be9a187e36893d846ef27146edd327 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Tue, 15 Sep 2026 00:03:05 -0700 Subject: [PATCH 1/3] Normalize login return destinations --- apps/cloud/src/auth/return-to.test.ts | 10 +++++++ apps/cloud/src/auth/return-to.ts | 29 +++++++++++++------ .../auth/workos-callback-state.node.test.ts | 15 ++++++++++ e2e/cloud/auth-session.test.ts | 25 ++++++++++++++++ 4 files changed, 70 insertions(+), 9 deletions(-) diff --git a/apps/cloud/src/auth/return-to.test.ts b/apps/cloud/src/auth/return-to.test.ts index 6178edf175..5d9ce05543 100644 --- a/apps/cloud/src/auth/return-to.test.ts +++ b/apps/cloud/src/auth/return-to.test.ts @@ -23,6 +23,13 @@ describe("isSafeReturnTo", () => { const unsafe = [ "https://evil.example", // absolute URL — off-origin redirect "//evil.example", // protocol-relative — same thing in disguise + "/\\evil.example", // browsers normalize backslashes to slashes + "/\t/evil.example", // URL parsing strips embedded tabs + "/\n/evil.example", + "/\r/evil.example", + "/safe/../api/auth/me", // normalized API destination + "/safe/%2e%2e/api/auth/me", + "/api/oauth/callback/../logout", "/api/auth/logout", // API endpoints are never a landing page "/api/oauth/callback/extra?state=oauth-state", // only the exact OAuth callback resumes "/api", // bare /api too @@ -47,6 +54,9 @@ describe("safeReturnTo", () => { it("passes a safe path through", () => { expect(safeReturnTo("/tools")).toBe("/tools"); }); + it("returns the canonical destination while preserving its query and fragment", () => { + expect(safeReturnTo("/old/../tools?view=all#list")).toBe("/tools?view=all#list"); + }); it("nulls unsafe and absent values", () => { expect(safeReturnTo("https://evil.example")).toBeNull(); expect(safeReturnTo(null)).toBeNull(); diff --git a/apps/cloud/src/auth/return-to.ts b/apps/cloud/src/auth/return-to.ts index 259b3ea6f9..35386dfaf9 100644 --- a/apps/cloud/src/auth/return-to.ts +++ b/apps/cloud/src/auth/return-to.ts @@ -11,18 +11,29 @@ // Pure string code — imported by server handlers and the login page alike. // --------------------------------------------------------------------------- -const pathPart = (path: string): string => path.split(/[?#]/, 1)[0] ?? ""; +const RETURN_TO_ORIGIN = "https://executor.invalid"; -const isOAuthCallbackReturnTo = (path: string): boolean => pathPart(path) === "/api/oauth/callback"; +/** Parse a same-origin landing path, or return null for absent or unsafe input. */ +export const safeReturnTo = (path: string | null | undefined): string | null => { + if (!path || !path.startsWith("/") || path.startsWith("//")) return null; + // Browsers treat backslashes as path separators and strip some control + // characters. Reject those spellings before interpreting the destination. + for (const character of path) { + if (character === "\\" || character <= " " || character === "\u007f") return null; + } -export const isSafeReturnTo = (path: string): boolean => - path.startsWith("/") && - !path.startsWith("//") && - (!/^\/api(\/|$)/.test(path) || isOAuthCallbackReturnTo(path)); + // The fixed origin and single leading slash guarantee a parseable URL. + // Check the normalized pathname so dot segments cannot bypass the API gate. + const destination = new URL(path, RETURN_TO_ORIGIN); + if (destination.origin !== RETURN_TO_ORIGIN) return null; + if (/^\/api(\/|$)/.test(destination.pathname) && destination.pathname !== "/api/oauth/callback") { + return null; + } + return `${destination.pathname}${destination.search}${destination.hash}`; +}; -/** The validated returnTo, or null when absent/unsafe. */ -export const safeReturnTo = (path: string | null | undefined): string | null => - path && isSafeReturnTo(path) ? path : null; +/** Whether a value parses as a same-origin landing path. */ +export const isSafeReturnTo = (path: string): boolean => safeReturnTo(path) !== null; /** The /login URL that comes back to `returnTo` ("/" needs no parameter). */ export const loginPath = (returnTo: string): string => diff --git a/apps/cloud/src/auth/workos-callback-state.node.test.ts b/apps/cloud/src/auth/workos-callback-state.node.test.ts index 891ce8f6a4..4c1799428b 100644 --- a/apps/cloud/src/auth/workos-callback-state.node.test.ts +++ b/apps/cloud/src/auth/workos-callback-state.node.test.ts @@ -113,6 +113,21 @@ const callbackUrl = (state?: string, code = "code_1") => `https://executor.test/auth/callback${state ? `?state=${encodeURIComponent(state)}` : ""}${state ? "&" : "?"}code=${code}`; describe("workos callback · CSRF state hardening", () => { + for (const returnTo of ["/\\evil.example", "/safe/../api/auth/me"]) { + it(`keeps an unsafe return destination on the homepage: ${JSON.stringify(returnTo)}`, async () => { + const state = encodeLoginState({ nonce: "redirect-boundary", returnTo }); + const res = await run( + new Request(callbackUrl(state), { + headers: { cookie: `${STATE_COOKIE}=${state}` }, + redirect: "manual", + }), + ); + expect(res.status).toBe(302); + expect(res.headers.get("location")).toBe("/"); + expect(res.headers.get("set-cookie") ?? "").toContain(SESSION_COOKIE); + }); + } + it("rejects a callback with NO state (the former bypass) before any WorkOS call", async () => { const res = await run(new Request(callbackUrl(undefined), { redirect: "manual" })); expect(res.status).toBe(400); diff --git a/e2e/cloud/auth-session.test.ts b/e2e/cloud/auth-session.test.ts index 272517b5f2..5870172459 100644 --- a/e2e/cloud/auth-session.test.ts +++ b/e2e/cloud/auth-session.test.ts @@ -65,6 +65,31 @@ scenario( }), ); +scenario( + "Auth · login refuses return paths that normalize outside the allowed pages", + {}, + Effect.gen(function* () { + yield* Api; + const target = yield* Target; + for (const returnTo of [ + "/\\evil.example", + "/safe/../api/auth/me", + "/safe/%2e%2e/api/auth/me", + ]) { + const login = new URL("/api/auth/login", target.baseUrl); + login.searchParams.set("returnTo", returnTo); + const response = yield* Effect.promise(() => fetch(login, { redirect: "manual" })); + expect(response.status).toBe(302); + const state = new URL(response.headers.get("location") ?? "").searchParams.get("state") ?? ""; + const decoded = decodeLoginState( + Result.getOrElse(Encoding.decodeBase64UrlString(state), () => ""), + ); + expect(decoded._tag).toBe("Some"); + if (decoded._tag === "Some") expect(decoded.value.returnTo).toBeUndefined(); + } + }), +); + scenario( "Auth · the callback rejects forged or incomplete redirects without exchanging the code", {}, From 94aaaa29fea6af0d0189d70ff200075bc466328d Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Tue, 15 Sep 2026 00:08:10 -0700 Subject: [PATCH 2/3] Assert parsed redirect state unconditionally --- e2e/cloud/auth-session.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/cloud/auth-session.test.ts b/e2e/cloud/auth-session.test.ts index 5870172459..33d14ae673 100644 --- a/e2e/cloud/auth-session.test.ts +++ b/e2e/cloud/auth-session.test.ts @@ -5,7 +5,7 @@ // callback refusing forged/incomplete redirects, the sealed-session cookie // actually authorizing the session API, and logout dropping the cookie. import { expect } from "@effect/vitest"; -import { Effect, Encoding, Result, Schema } from "effect"; +import { Effect, Encoding, Option, Result, Schema } from "effect"; import { scenario } from "../src/scenario"; import { Api, Target } from "../src/services"; @@ -85,7 +85,7 @@ scenario( Result.getOrElse(Encoding.decodeBase64UrlString(state), () => ""), ); expect(decoded._tag).toBe("Some"); - if (decoded._tag === "Some") expect(decoded.value.returnTo).toBeUndefined(); + expect(Option.getOrThrow(decoded).returnTo).toBeUndefined(); } }), ); From 5cc69279b44fbe802344cedd07a8f413768e20f9 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Tue, 15 Sep 2026 00:08:44 -0700 Subject: [PATCH 3/3] Use public Option helpers in auth checks --- e2e/cloud/auth-session.test.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/e2e/cloud/auth-session.test.ts b/e2e/cloud/auth-session.test.ts index 33d14ae673..c4de81ca79 100644 --- a/e2e/cloud/auth-session.test.ts +++ b/e2e/cloud/auth-session.test.ts @@ -46,11 +46,10 @@ scenario( const decoded = decodeLoginState( Result.getOrElse(Encoding.decodeBase64UrlString(state), () => ""), ); - expect(decoded._tag, "the state decodes as our login-state envelope").toBe("Some"); - expect( - decoded._tag === "Some" ? decoded.value.nonce : "", - "the state carries an unguessable CSRF nonce", - ).toMatch(/^[0-9a-f]{64}$/); + expect(Option.isSome(decoded), "the state decodes as our login-state envelope").toBe(true); + expect(Option.getOrThrow(decoded).nonce, "the state carries an unguessable CSRF nonce").toMatch( + /^[0-9a-f]{64}$/, + ); expect( authorizeUrl.searchParams.get("redirect_uri"), "AuthKit is told to come back to this deployment's callback", @@ -84,7 +83,7 @@ scenario( const decoded = decodeLoginState( Result.getOrElse(Encoding.decodeBase64UrlString(state), () => ""), ); - expect(decoded._tag).toBe("Some"); + expect(Option.isSome(decoded)).toBe(true); expect(Option.getOrThrow(decoded).returnTo).toBeUndefined(); } }),