Skip to content

Commit 6102fbd

Browse files
committed
fix(cloud): require CSRF state in the WorkOS login callback
1 parent fff7ed6 commit 6102fbd

3 files changed

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

204204
const result = yield* workos.authenticateWithCode(query.code);
Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
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 { 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 }),
59+
getAccount: async (id: string) => ({ id }),
60+
upsertOrganization: async (org: { id: string; name: string }) => ({
61+
...org,
62+
slug: org.id,
63+
}),
64+
getOrganization: async (id: string) => ({ id, name: `Org ${id}`, slug: id }),
65+
getOrganizationBySlug: async (slug: string) => ({ id: slug, name: slug, slug }),
66+
deleteOrganizationCascade: async () => {},
67+
}),
68+
),
69+
});
70+
71+
// Only the public group is under test; the session group (and its SessionAuth
72+
// middleware, which needs a live DB) is out of scope — the callback route lives
73+
// in CloudAuthPublicApi and requires no middleware.
74+
const PublicApi = HttpApi.make("cloudWeb").add(CloudAuthPublicApi);
75+
76+
const App = HttpApiBuilder.layer(PublicApi).pipe(
77+
Layer.provide(CloudAuthPublicHandlers),
78+
Layer.provide(stubWorkOS),
79+
Layer.provide(stubUsers),
80+
Layer.provide(HttpServer.layerServices),
81+
);
82+
83+
const run = (request: Request) => {
84+
const handler = HttpRouter.toWebHandler(App, { disableLogger: true }).handler;
85+
return handler(request);
86+
};
87+
88+
const callbackUrl = (state?: string, code = "code_1") =>
89+
`https://executor.test/auth/callback${state ? `?state=${encodeURIComponent(state)}` : ""}${state ? "&" : "?"}code=${code}`;
90+
91+
describe("workos callback · CSRF state hardening", () => {
92+
it("rejects a callback with NO state (the former bypass) before any WorkOS call", async () => {
93+
const res = await run(new Request(callbackUrl(undefined), { redirect: "manual" }));
94+
expect(res.status).toBe(400);
95+
expect(await res.text()).toContain("Invalid login state");
96+
expect(res.headers.get("set-cookie") ?? "").not.toContain(SESSION_COOKIE);
97+
});
98+
99+
it("rejects a state that does not match the login cookie", async () => {
100+
const res = await run(
101+
new Request(callbackUrl("attacker-controlled-state"), { redirect: "manual" }),
102+
);
103+
expect(res.status).toBe(400);
104+
expect(await res.text()).toContain("Invalid login state");
105+
});
106+
107+
it("accepts a fresh state matching the cookie and issues a session (302 + cookie)", async () => {
108+
// /login sets the cookie; simulate its value for this callback.
109+
const state = encodeLoginState({ nonce: "nonce-123", returnTo: "/" });
110+
const res = await run(
111+
new Request(callbackUrl(state), {
112+
headers: { cookie: `${STATE_COOKIE}=${state}` },
113+
redirect: "manual",
114+
}),
115+
);
116+
expect(res.status).toBe(302);
117+
expect(res.headers.get("set-cookie") ?? "").toContain(SESSION_COOKIE);
118+
});
119+
120+
it("rejects a replayed state (single-use contract preserved downstream)", async () => {
121+
// Replay of a state whose cookie is gone (already consumed by the login
122+
// round-trip) must fail closed.
123+
const state = encodeLoginState({ nonce: "nonce-replay", returnTo: "/" });
124+
const first = await run(
125+
new Request(callbackUrl(state), {
126+
headers: { cookie: `${STATE_COOKIE}=${state}` },
127+
redirect: "manual",
128+
}),
129+
);
130+
expect(first.status).toBe(302);
131+
132+
// Second callback: same state, no cookie (session-store consumed it).
133+
const replay = await run(new Request(callbackUrl(state), { redirect: "manual" }));
134+
expect(replay.status).toBe(400);
135+
});
136+
});

0 commit comments

Comments
 (0)