fix: sign out browsers whose session has already ended

Sign-out was declared on the session-authenticated API group, so a browser whose sealed session no longer authenticated at click time got a 401 from SessionAuth instead of being signed out. The console posts sign-out as a top-level form navigation, so that error body was rendered as the page: a screenful of {"_tag":"Unauthorized"}. Move the endpoint to the public group and read the sealed session off the request cookie. Nothing here needs authorizing — the request can only end the session whose cookie it presents — and the handler already fell back to "/" when the cookie would not unseal. Cookies are dropped only when the request actually presented one, so the now-public endpoint cannot be used cross-site to sign a user out.

Rhys Sullivan committed 2 months ago

Commit 16ea2841 parent a5de72e3 files · +99 −67Browse files

Unified · Split

3 files

apps/cloud/src/auth/api.ts +9 −1
apps/cloud/src/auth/handlers.ts +38 −31
e2e/cloud/logout-stale-session.test.ts +52 −35

apps/cloud/src/auth/api.ts+9−1

 /** Public auth endpoints — no authentication required */
 export class CloudAuthPublicApi extends HttpApiGroup.make("cloudAuthPublic")
   .add(HttpApiEndpoint.get("login", "/auth/login", { query: AuthLoginSearch }))
+  // Sign-out is PUBLIC on purpose. The console posts it as a top-level form
+  // navigation (the WorkOS hop is cross-origin, so it can't be a fetch), which
+  // means an error response is rendered as the page — and a browser whose
+  // session has already ended is exactly the browser most likely to click it
+  // (a second tab, a re-submit from history, an expired or revoked session).
+  // Behind SessionAuth all of those got a raw `{"_tag":"Unauthorized"}` screen
+  // instead of being signed out. There is nothing to authorize here anyway:
+  // the request can only end the session whose cookie it presents.
+  .add(HttpApiEndpoint.post("logout", "/auth/logout"))
   .add(
     HttpApiEndpoint.get("callback", "/auth/callback", {
       query: AuthCallbackSearch,
       error: AuthErrors,
     }),
   )
-  .add(HttpApiEndpoint.post("logout", "/auth/logout"))
   .add(
     HttpApiEndpoint.get("organizations", "/auth/organizations", {
       success: AuthOrganizationsResponse,

apps/cloud/src/auth/handlers.ts+38−31

   HttpServerResponse.setCookieUnsafe(response, name, "", DELETE_COOKIE_OPTIONS);
 // ---------------------------------------------------------------------------
-// Single non-protected API surface — public (login/callback) + session
+// Single non-protected API surface — public (login/callback/logout) + session
-// (me/logout/organizations/switch-organization). The session group has SessionAuth on it.
+// (me/organizations/switch-organization). The session group has SessionAuth on it.
 // ---------------------------------------------------------------------------
 export const NonProtectedApi = HttpApi.make("cloudWeb").add(CloudAuthPublicApi).add(CloudAuthApi);
           );
         }),
       )
+      .handleRaw("logout", ({ request }) =>
+        Effect.gen(function* () {
+          const workos = yield* WorkOSClient;
+          // The session this browser presents, NOT one the middleware vouched
+          // for — signing out of a session that has already ended must still
+          // sign the browser out (see the group declaration in ./api.ts).
+          const sealedSession = request.cookies["wos-session"] ?? "";
+          // WorkOS's documented sign-out: send the browser through the WorkOS
+          // logout endpoint, which ends the AuthKit session upstream and then
+          // redirects to the registered sign-out URL. Without this hop, the
+          // hosted session survives and the next "Sign in" silently
+          // re-authenticates (issue #1445). Fail-open when the cookie won't
+          // unseal — there is then nothing to end upstream, and local sign-out
+          // must still complete, so fall back to "/".
+          const origin = env.VITE_PUBLIC_SITE_URL ?? "";
+          const logoutUrl = sealedSession
+            ? yield* workos.logoutUrl(sealedSession, origin ? `${origin}/` : undefined)
+            : null;
+          const response = HttpServerResponse.redirect(logoutUrl ?? "/", { status: 302 });
+          // Drop only what this browser actually presented. Both cookies are
+          // SameSite=Lax, so a cross-site form POST carries neither — it gets
+          // the bare redirect and cannot be used to sign anyone out.
+          if (!sealedSession && request.cookies[AUTH_HINT_COOKIE] === undefined) return response;
+          // The auth-hint travels with the session: leaving it behind would
+          // make the next page load optimistically paint the app shell for a
+          // signed-out browser.
+          return deleteResponseCookie(
+            deleteResponseCookie(response, "wos-session"),
+            AUTH_HINT_COOKIE,
+          );
+        }),
+      )
       // CLI device-login discovery. The WorkOS device endpoints live on the
       // WorkOS API host (`WORKOS_API_URL`, or api.workos.com in production,
       // the SAME base the SDK uses, so e2e points the CLI at the emulator with
           };
         }),
       )
-      .handleRaw("logout", () =>
-        Effect.gen(function* () {
-          const workos = yield* WorkOSClient;
-          const session = yield* SessionContext;
-          // WorkOS's documented sign-out: send the browser through the WorkOS
-          // logout endpoint, which ends the AuthKit session upstream and then
-          // redirects to the registered sign-out URL. Without this hop, the
-          // hosted session survives and the next "Sign in" silently
-          // re-authenticates (issue #1445). Fail-open when the cookie won't
-          // unseal: local sign-out must still complete, so fall back to "/".
-          const origin = env.VITE_PUBLIC_SITE_URL ?? "";
-          const logoutUrl = yield* workos.logoutUrl(
-            session.sealedSession,
-            origin ? `${origin}/` : undefined,
-          );
-          // The auth-hint travels with the session: leaving it behind would
-          // make the next page load optimistically paint the app shell for a
-          // signed-out browser.
-          return deleteResponseCookie(
-            deleteResponseCookie(
-              HttpServerResponse.redirect(logoutUrl ?? "/", { status: 302 }),
-              "wos-session",
-            ),
-            AUTH_HINT_COOKIE,
-          );
-        }),
-      )
       .handle("organizations", () =>
         Effect.gen(function* () {
           const workos = yield* WorkOSClient;

e2e/cloud/logout-stale-session.test.ts+52−35

@@ -5,14 +5,14 @@ · 4 unmodified lines
5	// it is a screenful of raw API JSON where the homepage should be. Reported	5	// it is a screenful of raw API JSON where the homepage should be. Reported
6	// from the field as `{"_tag":"Unauthorized"}`.	6	// from the field as `{"_tag":"Unauthorized"}`.
7	//	7	//
8	// The endpoint sits behind SessionAuth, so a browser whose sealed session no	8	// The endpoint used to sit behind SessionAuth, so a browser whose sealed
9	// longer authenticates AT THE MOMENT OF THE CLICK gets the middleware's 401	9	// session no longer authenticated AT THE MOMENT OF THE CLICK got the
10	// instead of being signed out. Reaching that state needs nothing exotic: a	10	// middleware's 401 instead of being signed out. Reaching that state needs
11	// second open tab does it (sign out in one, the other's shell is still painted	11	// nothing exotic: a second open tab does it (sign out in one, the other's
12	// but its cookie is gone), and an expired or upstream-revoked session lands on	12	// shell is still painted but its cookie is gone), and an expired or
13	// the same branch. Signing out of a session that is already over is the one	13	// upstream-revoked session lands on the same branch. Signing out of a session
14	// request that must never fail — the user is asking for the state they are	14	// that is already over is the one request that must never fail — the user is
15	// already in.	15	// asking for the state they are already in.
16	import { expect } from "@effect/vitest";	16	import { expect } from "@effect/vitest";
17	import { Effect } from "effect";	17	import { Effect } from "effect";
18	import type { Page } from "playwright";	18	import type { Page } from "playwright";
@@ -20,6 +20,9 @@ import type { Page } from "playwright"; · 1 unmodified line
20	import { scenario } from "../src/scenario";	20	import { scenario } from "../src/scenario";
21	import { Api, Browser, Target } from "../src/services";	21	import { Api, Browser, Target } from "../src/services";
22		22
		23	/** The display-only identity cookie the SSR gate mints (non-HttpOnly). */
		24	const HINT_COOKIE = "executor-auth-hint";
		25
23	/** First `Set-Cookie` header for `name`, as the raw header string. */	26	/** First `Set-Cookie` header for `name`, as the raw header string. */
24	const setCookieFor = (response: Response, name: string): string => {	27	const setCookieFor = (response: Response, name: string): string => {
25	for (const header of response.headers.getSetCookie()) {	28	for (const header of response.headers.getSetCookie()) {
@@ -52,10 +55,6 @@ scenario( · 26 unmodified lines
52	yield* Api;	55	yield* Api;
53	const target = yield* Target;	56	const target = yield* Target;
54		57
55	// Two ways a real browser arrives here holding no live session: it never
56	// had one (the cookie was cleared, or sign-out is re-submitted from
57	// history), and it holds one that no longer authenticates (expired, or
58	// revoked upstream). Both POST the same form the shell posts.
59	const signOut = (cookie?: string) =>	58	const signOut = (cookie?: string) =>
60	Effect.promise(() =>	59	Effect.promise(() =>
61	fetch(new URL("/api/auth/logout", target.baseUrl), {	60	fetch(new URL("/api/auth/logout", target.baseUrl), {
@@ -65,30 +64,48 @@ scenario( · 3 unmodified lines
65	}),	64	}),
66	);	65	);
67		66
68	for (const [label, cookie] of [	67	// A session cookie that no longer authenticates — expired, or revoked
69	["no session cookie at all", undefined],	68	// upstream while the tab sat open. There is nothing to end at WorkOS, but
70	["a session cookie that no longer authenticates", "wos-session=stale-sealed-session"],	69	// this browser must still leave signed out: a stale session cookie left in
71	] as const) {	70	// place keeps routing / into the app instead of marketing.
72	const response = yield* signOut(cookie);	71	const stale = yield* signOut("wos-session=stale-sealed-session");
73	const body = yield* Effect.promise(() => response.text());	72	const staleBody = yield* Effect.promise(() => stale.text());
74		73
75	// The response IS the page the user reads. This is the reported symptom.	74	// The response IS the page the user reads. This is the reported symptom.
76	expect(body, `sign-out with ${label} never renders an API error envelope`).not.toContain(	75	expect(staleBody, "sign-out never renders an API error envelope").not.toContain("_tag");
77	"_tag",	76	expect(stale.status, "sign-out sends the browser onward").toBe(302);
78	);	77	expect(
79	expect(response.status, `sign-out with ${label} sends the browser onward`).toBe(302);	78	new URL(stale.headers.get("location") ?? "", target.baseUrl).pathname,
80	expect(	79	"sign-out lands the browser home",
81	new URL(response.headers.get("location") ?? "", target.baseUrl).pathname,	80	).toBe("/");
82	`sign-out with ${label} lands the browser home`,	81	expect(setCookieFor(stale, "wos-session"), "the dead session cookie is dropped").toContain(
83	).toBe("/");	82	"Max-Age=0",
84	// Nothing to end upstream without a session, but the browser must still	83	);
85	// leave with its cookies dropped — a stale session cookie left in place	84	expect(
86	// keeps routing / into the app instead of marketing.	85	setCookieFor(stale, HINT_COOKIE),
87	expect(	86	"the auth hint never outlives the session it describes",
88	setCookieFor(response, "wos-session"),	87	).toContain("Max-Age=0");
89	`sign-out with ${label} still clears the session cookie`,	88
90	).toContain("Max-Age=0");	89	// A POST carrying no cookies at all is what a CROSS-SITE form reaching
91	}	90	// this endpoint looks like: both cookies are SameSite=Lax, so another
		91	// origin's POST arrives bare. Signing out is public, so it must answer
		92	// such a request without touching this browser's session — otherwise any
		93	// site could sign our users out.
		94	const bare = yield* signOut();
		95	const bareBody = yield* Effect.promise(() => bare.text());
		96
		97	expect(bareBody, "a session-less sign-out renders no error envelope either").not.toContain(
		98	"_tag",
		99	);
		100	expect(bare.status, "it is sent home like any other sign-out").toBe(302);
		101	expect(
		102	new URL(bare.headers.get("location") ?? "", target.baseUrl).pathname,
		103	"it lands home",
		104	).toBe("/");
		105	expect(
		106	bare.headers.getSetCookie(),
		107	"a request that presented no session cannot expire anyone else's",
		108	).toEqual([]);
92	}),	109	}),
93	);	110	);
94		111