Normalize login return destinations (#2002)

* Normalize login return destinations

* Assert parsed redirect state unconditionally

* Use public Option helpers in auth checks

Author: RhysSullivan

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();

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 =>

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);

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";
@@ -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",
@@ -65,6 +64,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(Option.isSome(decoded)).toBe(true);
+      expect(Option.getOrThrow(decoded).returnTo).toBeUndefined();
+    }
+  }),
+);
+
 scenario(
   "Auth · the callback rejects forged or incomplete redirects without exchanging the code",
   {},