From 4a0c27e8ab6897173382b13fdf5308994a3d884f Mon Sep 17 00:00:00 2001 From: pt-act <211776491+pt-act@users.noreply.github.com> Date: Sun, 30 Aug 2026 15:52:55 +0100 Subject: [PATCH 1/5] fix(cloud): require CSRF state in the WorkOS login callback --- .changeset/tidy-login-state.md | 17 ++ apps/cloud/src/auth/handlers.ts | 22 +-- .../auth/workos-callback-state.node.test.ts | 149 ++++++++++++++++++ 3 files changed, 177 insertions(+), 11 deletions(-) create mode 100644 .changeset/tidy-login-state.md create mode 100644 apps/cloud/src/auth/workos-callback-state.node.test.ts diff --git a/.changeset/tidy-login-state.md b/.changeset/tidy-login-state.md new file mode 100644 index 0000000000..7cbf222e3f --- /dev/null +++ b/.changeset/tidy-login-state.md @@ -0,0 +1,17 @@ +--- +"@executor-js/cloud": patch +--- + +fix: make login CSRF state mandatory in the WorkOS callback + +The callback previously skipped its CSRF check whenever the redirect carried +no `state` value ("some WorkOS-initiated redirects don't include one"). That +bypass let an attacker complete their own OAuth round-trip and redirect a +victim's browser through the callback with the attacker's `code` and no +`state`, silently signing the victim into the attacker's account (login CSRF). + +The check is now unconditional: a callback without a state matching the +`wos-login-state` cookie set on `/login` is rejected with 400. This is a +breaking change for any client relying on the undocumented no-state entry +path; server-initiated flows that cannot carry state must be redesigned with +a signed nonce instead of re-adding the bypass. diff --git a/apps/cloud/src/auth/handlers.ts b/apps/cloud/src/auth/handlers.ts index a6f9a9e5d3..ea4849d5b4 100644 --- a/apps/cloud/src/auth/handlers.ts +++ b/apps/cloud/src/auth/handlers.ts @@ -188,17 +188,17 @@ export const CloudAuthPublicHandlers = HttpApiBuilder.group( const workos = yield* WorkOSClient; const users = yield* UserStoreService; const cookieState = request.cookies[STATE_COOKIE] ?? null; - // CSRF check is only enforced when the redirect carries a state - // value — some WorkOS-initiated redirects don't include one. - // When state is present, it MUST match the cookie we set on - // /login. - if (query.state !== undefined) { - if (!cookieState || !timingSafeEqual(cookieState, query.state)) { - return deleteResponseCookie( - HttpServerResponse.text("Invalid login state", { status: 400 }), - STATE_COOKIE, - ); - } + // CSRF is unconditional: every callback must carry a state that + // matches the cookie set on /login. There is no legitimate + // no-state entry path — omitting state previously allowed an + // attacker to complete their own OAuth round-trip and redirect a + // victim's browser through this callback, signing the victim into + // the attacker's account (login CSRF). + if (!cookieState || !timingSafeEqual(cookieState, query.state ?? "")) { + return deleteResponseCookie( + HttpServerResponse.text("Invalid login state", { status: 400 }), + STATE_COOKIE, + ); } const result = yield* workos.authenticateWithCode(query.code); diff --git a/apps/cloud/src/auth/workos-callback-state.node.test.ts b/apps/cloud/src/auth/workos-callback-state.node.test.ts new file mode 100644 index 0000000000..ae5aaa9e0f --- /dev/null +++ b/apps/cloud/src/auth/workos-callback-state.node.test.ts @@ -0,0 +1,149 @@ +// --------------------------------------------------------------------------- +// Focused tests — the WorkOS login callback's CSRF gate. +// +// The callback's CSRF check must be unconditional: no state ⇒ 400 before any +// WorkOS call; a replayed (already consumed) state ⇒ 400; a fresh state +// matching the cookie ⇒ 302 + session. +// +// Test seams follow repo conventions: @effect/vitest, Layer.succeed stubs +// (see org-selector-auth.node.test.ts), and HttpRouter.toWebHandler for the +// HTTP surface (see api.request-scope.node.test.ts). +// --------------------------------------------------------------------------- + +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Layer } from "effect"; +import { HttpRouter, HttpServer } from "effect/unstable/http"; +import { HttpApiBuilder } from "effect/unstable/httpapi"; +import { HttpApi } from "effect/unstable/httpapi"; + +import { CloudAuthPublicHandlers } from "./handlers"; +import { CloudAuthPublicApi } from "./api"; +import { UserStoreService } from "./context"; +import { WorkOSClient, type WorkOSClientService } from "./workos"; +import { encodeLoginState } from "./login-state"; + +// The route under test serves under the `/api` prefix in the composed app; +// toWebHandler mounts the raw group, so paths here are relative to the group. +const SESSION_COOKIE = "wos-session"; +const STATE_COOKIE = "wos-login-state"; + +const STUB_USER_ID = "user_test"; +const STUB_SESSION = "sealed-session-stub"; +const STUB_ORG_ID = "org_test"; + +const stubWorkOS = Layer.succeed( + WorkOSClient, + new Proxy({} as WorkOSClientService, { + get: (_t, prop) => { + if (prop === "authenticateWithCode") { + return () => + Effect.succeed({ + user: { id: STUB_USER_ID, email: "u@test" }, + organizationId: STUB_ORG_ID, + sealedSession: STUB_SESSION, + }); + } + if (prop === "listUserMemberships") { + return () => Effect.succeed({ data: [] }); + } + return () => Effect.die(`unexpected WorkOSClient.${String(prop)} call`); + }, + }), +); + +const stubUsers = Layer.succeed(UserStoreService)({ + use: (_op, fn) => + Effect.promise(() => + fn({ + ensureAccount: async (id: string) => ({ id, createdAt: new Date() }), + getAccount: async (id: string) => ({ id, createdAt: new Date() }), + upsertOrganization: async (org: { id: string; name: string }) => ({ + ...org, + slug: org.id, + createdAt: new Date(), + }), + getOrganization: async (id: string) => ({ + id, + name: "Org " + id, + slug: id, + createdAt: new Date(), + }), + getOrganizationBySlug: async (slug: string) => ({ + id: slug, + name: slug, + slug, + createdAt: new Date(), + }), + deleteOrganizationCascade: async () => {}, + }), + ), +}); + +// Only the public group is under test; the session group (and its SessionAuth +// middleware, which needs a live DB) is out of scope — the callback route lives +// in CloudAuthPublicApi and requires no middleware. +const PublicApi = HttpApi.make("cloudWeb").add(CloudAuthPublicApi); + +const App = HttpApiBuilder.layer(PublicApi).pipe( + Layer.provide(CloudAuthPublicHandlers), + Layer.provide(stubWorkOS), + Layer.provide(stubUsers), + Layer.provide(HttpServer.layerServices), +); + +const run = (request: Request) => { + const handler = HttpRouter.toWebHandler(App, { disableLogger: true }).handler; + // beta.59: the handler type expects a context argument; this layer stack + // needs none at runtime — pass undefined like the api.request-scope tests. + return handler(request, undefined as never); +}; + +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", () => { + 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); + expect(await res.text()).toContain("Invalid login state"); + expect(res.headers.get("set-cookie") ?? "").not.toContain(SESSION_COOKIE); + }); + + it("rejects a state that does not match the login cookie", async () => { + const res = await run( + new Request(callbackUrl("attacker-controlled-state"), { redirect: "manual" }), + ); + expect(res.status).toBe(400); + expect(await res.text()).toContain("Invalid login state"); + }); + + it("accepts a fresh state matching the cookie and issues a session (302 + cookie)", async () => { + // /login sets the cookie; simulate its value for this callback. + const state = encodeLoginState({ nonce: "nonce-123", 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("set-cookie") ?? "").toContain(SESSION_COOKIE); + }); + + it("rejects a replayed state (single-use contract preserved downstream)", async () => { + // Replay of a state whose cookie is gone (already consumed by the login + // round-trip) must fail closed. + const state = encodeLoginState({ nonce: "nonce-replay", returnTo: "/" }); + const first = await run( + new Request(callbackUrl(state), { + headers: { cookie: `${STATE_COOKIE}=${state}` }, + redirect: "manual", + }), + ); + expect(first.status).toBe(302); + + // Second callback: same state, no cookie (session-store consumed it). + const replay = await run(new Request(callbackUrl(state), { redirect: "manual" })); + expect(replay.status).toBe(400); + }); +}); From 97848019674a2cdf1c0bb75517ee19c237ab1219 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 12 Sep 2026 10:12:06 -0700 Subject: [PATCH 2/5] Test queue timeout with a controlled clock --- apps/cloud/src/mcp/session-build-semaphore.test.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/apps/cloud/src/mcp/session-build-semaphore.test.ts b/apps/cloud/src/mcp/session-build-semaphore.test.ts index 3d4ad76343..584b65ee0e 100644 --- a/apps/cloud/src/mcp/session-build-semaphore.test.ts +++ b/apps/cloud/src/mcp/session-build-semaphore.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, beforeEach } from "@effect/vitest"; +import { describe, expect, it, beforeEach, afterEach, vi } from "@effect/vitest"; import { acquireBuildSlot, @@ -13,6 +13,10 @@ describe("session-build-semaphore", () => { resetBuildSlotsForTest(); }); + afterEach(() => { + vi.useRealTimers(); + }); + it("grants up to the cap immediately, with no wait", async () => { const results = await Promise.all([ acquireBuildSlot().promise, @@ -214,6 +218,7 @@ describe("session-build-semaphore", () => { }); it("proceeds without a slot when the queue wait exceeds the timeout, and does not count it as active", async () => { + vi.useFakeTimers(); await Promise.all([ acquireBuildSlot().promise, acquireBuildSlot().promise, @@ -223,6 +228,10 @@ describe("session-build-semaphore", () => { expect(currentActiveBuildsForTest()).toBe(4); const timedOutHandle = acquireBuildSlot(10); + await vi.advanceTimersByTimeAsync(9); + expect(currentQueueLengthForTest()).toBe(1); + expect(currentActiveBuildsForTest()).toBe(4); + await vi.advanceTimersByTimeAsync(1); const result = await timedOutHandle.promise; expect(result).toEqual({ acquired: false, waitMs: expect.any(Number), timedOut: true }); From c818ad794a80b77cc5b0328ff82ef08484402986 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 12 Sep 2026 11:39:35 -0700 Subject: [PATCH 3/5] Exercise browser binding and replay protection for login state --- apps/cloud/src/auth/handlers.ts | 2 +- .../auth/workos-callback-state.node.test.ts | 25 +++++-- e2e/cloud/login-csrf.test.ts | 67 +++++++++++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) create mode 100644 e2e/cloud/login-csrf.test.ts diff --git a/apps/cloud/src/auth/handlers.ts b/apps/cloud/src/auth/handlers.ts index fe386cd7cc..a568dadecf 100644 --- a/apps/cloud/src/auth/handlers.ts +++ b/apps/cloud/src/auth/handlers.ts @@ -210,7 +210,7 @@ export const CloudAuthPublicHandlers = HttpApiBuilder.group( let sealedSession = result.sealedSession; // Resume where the SSR gate interrupted them. The state passed the - // CSRF check above whenever it's present, but it's still a + // CSRF check above, but it's still a // round-tripped value, so the returnTo inside it is re-validated like // any other untrusted path. const returnTo = safeReturnTo(decodeLoginState(query.state)?.returnTo) ?? "/"; 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 ae5aaa9e0f..3787f57b01 100644 --- a/apps/cloud/src/auth/workos-callback-state.node.test.ts +++ b/apps/cloud/src/auth/workos-callback-state.node.test.ts @@ -10,7 +10,7 @@ // HTTP surface (see api.request-scope.node.test.ts). // --------------------------------------------------------------------------- -import { describe, expect, it } from "@effect/vitest"; +import { afterAll, describe, expect, it } from "@effect/vitest"; import { Effect, Layer } from "effect"; import { HttpRouter, HttpServer } from "effect/unstable/http"; import { HttpApiBuilder } from "effect/unstable/httpapi"; @@ -91,11 +91,13 @@ const App = HttpApiBuilder.layer(PublicApi).pipe( Layer.provide(HttpServer.layerServices), ); +const app = HttpRouter.toWebHandler(App, { disableLogger: true }); +afterAll(() => app.dispose()); + const run = (request: Request) => { - const handler = HttpRouter.toWebHandler(App, { disableLogger: true }).handler; // beta.59: the handler type expects a context argument; this layer stack // needs none at runtime — pass undefined like the api.request-scope tests. - return handler(request, undefined as never); + return app.handler(request, undefined as never); }; const callbackUrl = (state?: string, code = "code_1") => @@ -109,9 +111,24 @@ describe("workos callback · CSRF state hardening", () => { expect(res.headers.get("set-cookie") ?? "").not.toContain(SESSION_COOKIE); }); + it("rejects missing state even when the browser has a login cookie", async () => { + const res = await run( + new Request(callbackUrl(undefined), { + headers: { cookie: `${STATE_COOKIE}=victim-login-state` }, + redirect: "manual", + }), + ); + expect(res.status).toBe(400); + expect(await res.text()).toBe("Invalid login state"); + expect(res.headers.get("set-cookie") ?? "").not.toContain(SESSION_COOKIE); + }); + it("rejects a state that does not match the login cookie", async () => { const res = await run( - new Request(callbackUrl("attacker-controlled-state"), { redirect: "manual" }), + new Request(callbackUrl("attacker-controlled-state"), { + headers: { cookie: `${STATE_COOKIE}=victim-login-state` }, + redirect: "manual", + }), ); expect(res.status).toBe(400); expect(await res.text()).toContain("Invalid login state"); diff --git a/e2e/cloud/login-csrf.test.ts b/e2e/cloud/login-csrf.test.ts new file mode 100644 index 0000000000..c31a5b6dcf --- /dev/null +++ b/e2e/cloud/login-csrf.test.ts @@ -0,0 +1,67 @@ +import { randomUUID } from "node:crypto"; + +import { expect } from "@effect/vitest"; +import { Effect } from "effect"; + +import { scenario } from "../src/scenario"; +import { Browser, Target } from "../src/services"; + +scenario( + "Login CSRF · state is required, bound to the browser, and consumed after login", + { timeout: 180_000 }, + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const email = `csrf-${randomUUID()}@e2e.test`; + yield* browser.session({ label: "anonymous" }, async ({ page, step }) => { + const interceptCallback = async (): Promise => { + let callback: string | undefined; + await page.route("**/api/auth/callback?**", async (route) => { + callback = route.request().url(); + await route.abort(); + }); + await page.goto(new URL("/api/auth/login", target.baseUrl).toString()); + await page.getByPlaceholder("new-user@example.com").fill(email); + await page.getByRole("button", { name: /Continue/ }).click(); + await expect.poll(() => callback).toBeDefined(); + await page.unroute("**/api/auth/callback?**"); + if (!callback) throw new Error("AuthKit did not return a callback"); + return callback; + }; + await step("Refuse a valid authorization code with no state", async () => { + const callback = new URL(await interceptCallback()); + callback.searchParams.delete("state"); + const response = await page.request.get(callback.toString(), { maxRedirects: 0 }); + expect(response.status()).toBe(400); + expect(await response.text()).toBe("Invalid login state"); + expect( + (await page.context().cookies()).some((cookie) => cookie.name === "wos-session"), + ).toBe(false); + }); + await step("Refuse a state from another login", async () => { + const callback = new URL(await interceptCallback()); + callback.searchParams.set("state", "another-browser-state"); + const response = await page.request.get(callback.toString(), { maxRedirects: 0 }); + expect(response.status()).toBe(400); + expect(await response.text()).toBe("Invalid login state"); + expect( + (await page.context().cookies()).some((cookie) => cookie.name === "wos-session"), + ).toBe(false); + }); + await step("Complete a fresh login, then reject the same callback again", async () => { + const callback = await interceptCallback(); + await page.goto(callback); + await page.waitForURL((url) => url.pathname === "/create-org", { timeout: 30_000 }); + const cookies = await page.context().cookies(); + expect(cookies.some((cookie) => cookie.name === "wos-session")).toBe(true); + expect(cookies.some((cookie) => cookie.name === "wos-login-state")).toBe(false); + const me = await page.request.get(new URL("/api/auth/me", target.baseUrl).toString()); + expect(me.status()).toBe(200); + expect(await me.json()).toMatchObject({ user: { email } }); + const replay = await page.request.get(callback, { maxRedirects: 0 }); + expect(replay.status()).toBe(400); + expect(await replay.text()).toBe("Invalid login state"); + }); + }); + }), +); From 703765bb5a2e27d0895ae171db267582f3af959a Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 12 Sep 2026 11:45:57 -0700 Subject: [PATCH 4/5] Capture provider redirect before testing callback state --- e2e/cloud/login-csrf.test.ts | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/e2e/cloud/login-csrf.test.ts b/e2e/cloud/login-csrf.test.ts index c31a5b6dcf..fbc237e8b3 100644 --- a/e2e/cloud/login-csrf.test.ts +++ b/e2e/cloud/login-csrf.test.ts @@ -16,15 +16,23 @@ scenario( yield* browser.session({ label: "anonymous" }, async ({ page, step }) => { const interceptCallback = async (): Promise => { let callback: string | undefined; - await page.route("**/api/auth/callback?**", async (route) => { - callback = route.request().url(); - await route.abort(); + // Pause the real provider response before its redirect reaches the app. + // Playwright does not route subsequent hops of a redirect chain. + await page.route("**/user_management/authorize/submit", async (route) => { + const response = await route.fetch({ maxRedirects: 0 }); + expect(response.status()).toBe(302); + callback = response.headers().location; + await route.fulfill({ + status: 200, + contentType: "text/plain", + body: "Authorization ready for callback validation", + }); }); await page.goto(new URL("/api/auth/login", target.baseUrl).toString()); await page.getByPlaceholder("new-user@example.com").fill(email); await page.getByRole("button", { name: /Continue/ }).click(); await expect.poll(() => callback).toBeDefined(); - await page.unroute("**/api/auth/callback?**"); + await page.unroute("**/user_management/authorize/submit"); if (!callback) throw new Error("AuthKit did not return a callback"); return callback; }; From ade940393d2d4020a7346f41f7b69e96dbc15257 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Sat, 12 Sep 2026 11:50:43 -0700 Subject: [PATCH 5/5] Wait for key revocation before checking authentication --- e2e/cloud/org-api-keys-console.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/e2e/cloud/org-api-keys-console.test.ts b/e2e/cloud/org-api-keys-console.test.ts index 828dcb9afb..f4b34dafb8 100644 --- a/e2e/cloud/org-api-keys-console.test.ts +++ b/e2e/cloud/org-api-keys-console.test.ts @@ -110,7 +110,9 @@ scenario( .getByRole("heading", { name: "Revoke organization key" }) .waitFor({ state: "hidden", timeout: 30_000 }); - // The revoked value no longer authenticates. + // The dialog closes when revocation starts. Wait for the confirmed + // provider mutation before asserting the key no longer authenticates. + await page.getByText("Revoked e2e backend reader", { exact: true }).waitFor(); const after = await fetch(new URL("/api/admin/users", target.baseUrl), { headers: { authorization: `Bearer ${mintedValue}` }, });