Skip to content

Commit 5ecb881

Browse files
ra-co88pt-actRhysSullivan
authored
fix(cloud): require CSRF state in the WorkOS login callback (#1886)
* fix(cloud): require CSRF state in the WorkOS login callback * Test queue timeout with a controlled clock * Exercise browser binding and replay protection for login state * Capture provider redirect before testing callback state * Wait for key revocation before checking authentication --------- Co-authored-by: pt-act <211776491+pt-act@users.noreply.github.com> Co-authored-by: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com>
1 parent 929b233 commit 5ecb881

5 files changed

Lines changed: 273 additions & 13 deletions

File tree

.changeset/tidy-login-state.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
"@executor-js/cloud": patch
3+
---
4+
5+
fix: make login CSRF state mandatory in the WorkOS callback
6+
7+
The callback previously skipped its CSRF check whenever the redirect carried
8+
no `state` value ("some WorkOS-initiated redirects don't include one"). That
9+
bypass let an attacker complete their own OAuth round-trip and redirect a
10+
victim's browser through the callback with the attacker's `code` and no
11+
`state`, silently signing the victim into the attacker's account (login CSRF).
12+
13+
The check is now unconditional: a callback without a state matching the
14+
`wos-login-state` cookie set on `/login` is rejected with 400. This is a
15+
breaking change for any client relying on the undocumented no-state entry
16+
path; server-initiated flows that cannot carry state must be redesigned with
17+
a signed nonce instead of re-adding the bypass.

apps/cloud/src/auth/handlers.ts

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -189,17 +189,17 @@ export const CloudAuthPublicHandlers = HttpApiBuilder.group(
189189
const workos = yield* WorkOSClient;
190190
const users = yield* UserStoreService;
191191
const cookieState = request.cookies[STATE_COOKIE] ?? null;
192-
// CSRF check is only enforced when the redirect carries a state
193-
// value — some WorkOS-initiated redirects don't include one.
194-
// When state is present, it MUST match the cookie we set on
195-
// /login.
196-
if (query.state !== undefined) {
197-
if (!cookieState || !timingSafeEqual(cookieState, query.state)) {
198-
return deleteResponseCookie(
199-
HttpServerResponse.text("Invalid login state", { status: 400 }),
200-
STATE_COOKIE,
201-
);
202-
}
192+
// CSRF is unconditional: every callback must carry a state that
193+
// matches the cookie set on /login. There is no legitimate
194+
// no-state entry path — omitting state previously allowed an
195+
// attacker to complete their own OAuth round-trip and redirect a
196+
// victim's browser through this callback, signing the victim into
197+
// the attacker's account (login CSRF).
198+
if (!cookieState || !timingSafeEqual(cookieState, query.state ?? "")) {
199+
return deleteResponseCookie(
200+
HttpServerResponse.text("Invalid login state", { status: 400 }),
201+
STATE_COOKIE,
202+
);
203203
}
204204

205205
const result = yield* workos.authenticateWithCode(query.code);
@@ -210,7 +210,7 @@ export const CloudAuthPublicHandlers = HttpApiBuilder.group(
210210
let sealedSession = result.sealedSession;
211211

212212
// Resume where the SSR gate interrupted them. The state passed the
213-
// CSRF check above whenever it's present, but it's still a
213+
// CSRF check above, but it's still a
214214
// round-tripped value, so the returnTo inside it is re-validated like
215215
// any other untrusted path.
216216
const returnTo = safeReturnTo(decodeLoginState(query.state)?.returnTo) ?? "/";
Lines changed: 166 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,166 @@
1+
// ---------------------------------------------------------------------------
2+
// Focused tests — the WorkOS login callback's CSRF gate.
3+
//
4+
// The callback's CSRF check must be unconditional: no state ⇒ 400 before any
5+
// WorkOS call; a replayed (already consumed) state ⇒ 400; a fresh state
6+
// matching the cookie ⇒ 302 + session.
7+
//
8+
// Test seams follow repo conventions: @effect/vitest, Layer.succeed stubs
9+
// (see org-selector-auth.node.test.ts), and HttpRouter.toWebHandler for the
10+
// HTTP surface (see api.request-scope.node.test.ts).
11+
// ---------------------------------------------------------------------------
12+
13+
import { afterAll, describe, expect, it } from "@effect/vitest";
14+
import { Effect, Layer } from "effect";
15+
import { HttpRouter, HttpServer } from "effect/unstable/http";
16+
import { HttpApiBuilder } from "effect/unstable/httpapi";
17+
import { HttpApi } from "effect/unstable/httpapi";
18+
19+
import { CloudAuthPublicHandlers } from "./handlers";
20+
import { CloudAuthPublicApi } from "./api";
21+
import { UserStoreService } from "./context";
22+
import { WorkOSClient, type WorkOSClientService } from "./workos";
23+
import { encodeLoginState } from "./login-state";
24+
25+
// The route under test serves under the `/api` prefix in the composed app;
26+
// toWebHandler mounts the raw group, so paths here are relative to the group.
27+
const SESSION_COOKIE = "wos-session";
28+
const STATE_COOKIE = "wos-login-state";
29+
30+
const STUB_USER_ID = "user_test";
31+
const STUB_SESSION = "sealed-session-stub";
32+
const STUB_ORG_ID = "org_test";
33+
34+
const stubWorkOS = Layer.succeed(
35+
WorkOSClient,
36+
new Proxy({} as WorkOSClientService, {
37+
get: (_t, prop) => {
38+
if (prop === "authenticateWithCode") {
39+
return () =>
40+
Effect.succeed({
41+
user: { id: STUB_USER_ID, email: "u@test" },
42+
organizationId: STUB_ORG_ID,
43+
sealedSession: STUB_SESSION,
44+
});
45+
}
46+
if (prop === "listUserMemberships") {
47+
return () => Effect.succeed({ data: [] });
48+
}
49+
return () => Effect.die(`unexpected WorkOSClient.${String(prop)} call`);
50+
},
51+
}),
52+
);
53+
54+
const stubUsers = Layer.succeed(UserStoreService)({
55+
use: (_op, fn) =>
56+
Effect.promise(() =>
57+
fn({
58+
ensureAccount: async (id: string) => ({ id, createdAt: new Date() }),
59+
getAccount: async (id: string) => ({ id, createdAt: new Date() }),
60+
upsertOrganization: async (org: { id: string; name: string }) => ({
61+
...org,
62+
slug: org.id,
63+
createdAt: new Date(),
64+
}),
65+
getOrganization: async (id: string) => ({
66+
id,
67+
name: "Org " + id,
68+
slug: id,
69+
createdAt: new Date(),
70+
}),
71+
getOrganizationBySlug: async (slug: string) => ({
72+
id: slug,
73+
name: slug,
74+
slug,
75+
createdAt: new Date(),
76+
}),
77+
deleteOrganizationCascade: async () => {},
78+
}),
79+
),
80+
});
81+
82+
// Only the public group is under test; the session group (and its SessionAuth
83+
// middleware, which needs a live DB) is out of scope — the callback route lives
84+
// in CloudAuthPublicApi and requires no middleware.
85+
const PublicApi = HttpApi.make("cloudWeb").add(CloudAuthPublicApi);
86+
87+
const App = HttpApiBuilder.layer(PublicApi).pipe(
88+
Layer.provide(CloudAuthPublicHandlers),
89+
Layer.provide(stubWorkOS),
90+
Layer.provide(stubUsers),
91+
Layer.provide(HttpServer.layerServices),
92+
);
93+
94+
const app = HttpRouter.toWebHandler(App, { disableLogger: true });
95+
afterAll(() => app.dispose());
96+
97+
const run = (request: Request) => {
98+
// beta.59: the handler type expects a context argument; this layer stack
99+
// needs none at runtime — pass undefined like the api.request-scope tests.
100+
return app.handler(request, undefined as never);
101+
};
102+
103+
const callbackUrl = (state?: string, code = "code_1") =>
104+
`https://executor.test/auth/callback${state ? `?state=${encodeURIComponent(state)}` : ""}${state ? "&" : "?"}code=${code}`;
105+
106+
describe("workos callback · CSRF state hardening", () => {
107+
it("rejects a callback with NO state (the former bypass) before any WorkOS call", async () => {
108+
const res = await run(new Request(callbackUrl(undefined), { redirect: "manual" }));
109+
expect(res.status).toBe(400);
110+
expect(await res.text()).toContain("Invalid login state");
111+
expect(res.headers.get("set-cookie") ?? "").not.toContain(SESSION_COOKIE);
112+
});
113+
114+
it("rejects missing state even when the browser has a login cookie", async () => {
115+
const res = await run(
116+
new Request(callbackUrl(undefined), {
117+
headers: { cookie: `${STATE_COOKIE}=victim-login-state` },
118+
redirect: "manual",
119+
}),
120+
);
121+
expect(res.status).toBe(400);
122+
expect(await res.text()).toBe("Invalid login state");
123+
expect(res.headers.get("set-cookie") ?? "").not.toContain(SESSION_COOKIE);
124+
});
125+
126+
it("rejects a state that does not match the login cookie", async () => {
127+
const res = await run(
128+
new Request(callbackUrl("attacker-controlled-state"), {
129+
headers: { cookie: `${STATE_COOKIE}=victim-login-state` },
130+
redirect: "manual",
131+
}),
132+
);
133+
expect(res.status).toBe(400);
134+
expect(await res.text()).toContain("Invalid login state");
135+
});
136+
137+
it("accepts a fresh state matching the cookie and issues a session (302 + cookie)", async () => {
138+
// /login sets the cookie; simulate its value for this callback.
139+
const state = encodeLoginState({ nonce: "nonce-123", returnTo: "/" });
140+
const res = await run(
141+
new Request(callbackUrl(state), {
142+
headers: { cookie: `${STATE_COOKIE}=${state}` },
143+
redirect: "manual",
144+
}),
145+
);
146+
expect(res.status).toBe(302);
147+
expect(res.headers.get("set-cookie") ?? "").toContain(SESSION_COOKIE);
148+
});
149+
150+
it("rejects a replayed state (single-use contract preserved downstream)", async () => {
151+
// Replay of a state whose cookie is gone (already consumed by the login
152+
// round-trip) must fail closed.
153+
const state = encodeLoginState({ nonce: "nonce-replay", returnTo: "/" });
154+
const first = await run(
155+
new Request(callbackUrl(state), {
156+
headers: { cookie: `${STATE_COOKIE}=${state}` },
157+
redirect: "manual",
158+
}),
159+
);
160+
expect(first.status).toBe(302);
161+
162+
// Second callback: same state, no cookie (session-store consumed it).
163+
const replay = await run(new Request(callbackUrl(state), { redirect: "manual" }));
164+
expect(replay.status).toBe(400);
165+
});
166+
});

e2e/cloud/login-csrf.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
import { randomUUID } from "node:crypto";
2+
3+
import { expect } from "@effect/vitest";
4+
import { Effect } from "effect";
5+
6+
import { scenario } from "../src/scenario";
7+
import { Browser, Target } from "../src/services";
8+
9+
scenario(
10+
"Login CSRF · state is required, bound to the browser, and consumed after login",
11+
{ timeout: 180_000 },
12+
Effect.gen(function* () {
13+
const target = yield* Target;
14+
const browser = yield* Browser;
15+
const email = `csrf-${randomUUID()}@e2e.test`;
16+
yield* browser.session({ label: "anonymous" }, async ({ page, step }) => {
17+
const interceptCallback = async (): Promise<string> => {
18+
let callback: string | undefined;
19+
// Pause the real provider response before its redirect reaches the app.
20+
// Playwright does not route subsequent hops of a redirect chain.
21+
await page.route("**/user_management/authorize/submit", async (route) => {
22+
const response = await route.fetch({ maxRedirects: 0 });
23+
expect(response.status()).toBe(302);
24+
callback = response.headers().location;
25+
await route.fulfill({
26+
status: 200,
27+
contentType: "text/plain",
28+
body: "Authorization ready for callback validation",
29+
});
30+
});
31+
await page.goto(new URL("/api/auth/login", target.baseUrl).toString());
32+
await page.getByPlaceholder("new-user@example.com").fill(email);
33+
await page.getByRole("button", { name: /Continue/ }).click();
34+
await expect.poll(() => callback).toBeDefined();
35+
await page.unroute("**/user_management/authorize/submit");
36+
if (!callback) throw new Error("AuthKit did not return a callback");
37+
return callback;
38+
};
39+
await step("Refuse a valid authorization code with no state", async () => {
40+
const callback = new URL(await interceptCallback());
41+
callback.searchParams.delete("state");
42+
const response = await page.request.get(callback.toString(), { maxRedirects: 0 });
43+
expect(response.status()).toBe(400);
44+
expect(await response.text()).toBe("Invalid login state");
45+
expect(
46+
(await page.context().cookies()).some((cookie) => cookie.name === "wos-session"),
47+
).toBe(false);
48+
});
49+
await step("Refuse a state from another login", async () => {
50+
const callback = new URL(await interceptCallback());
51+
callback.searchParams.set("state", "another-browser-state");
52+
const response = await page.request.get(callback.toString(), { maxRedirects: 0 });
53+
expect(response.status()).toBe(400);
54+
expect(await response.text()).toBe("Invalid login state");
55+
expect(
56+
(await page.context().cookies()).some((cookie) => cookie.name === "wos-session"),
57+
).toBe(false);
58+
});
59+
await step("Complete a fresh login, then reject the same callback again", async () => {
60+
const callback = await interceptCallback();
61+
await page.goto(callback);
62+
await page.waitForURL((url) => url.pathname === "/create-org", { timeout: 30_000 });
63+
const cookies = await page.context().cookies();
64+
expect(cookies.some((cookie) => cookie.name === "wos-session")).toBe(true);
65+
expect(cookies.some((cookie) => cookie.name === "wos-login-state")).toBe(false);
66+
const me = await page.request.get(new URL("/api/auth/me", target.baseUrl).toString());
67+
expect(me.status()).toBe(200);
68+
expect(await me.json()).toMatchObject({ user: { email } });
69+
const replay = await page.request.get(callback, { maxRedirects: 0 });
70+
expect(replay.status()).toBe(400);
71+
expect(await replay.text()).toBe("Invalid login state");
72+
});
73+
});
74+
}),
75+
);

e2e/cloud/org-api-keys-console.test.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,9 @@ scenario(
110110
.getByRole("heading", { name: "Revoke organization key" })
111111
.waitFor({ state: "hidden", timeout: 30_000 });
112112

113-
// The revoked value no longer authenticates.
113+
// The dialog closes when revocation starts. Wait for the confirmed
114+
// provider mutation before asserting the key no longer authenticates.
115+
await page.getByText("Revoked e2e backend reader", { exact: true }).waitFor();
114116
const after = await fetch(new URL("/api/admin/users", target.baseUrl), {
115117
headers: { authorization: `Bearer ${mintedValue}` },
116118
});

0 commit comments

Comments
 (0)