fix: OIDC state (#3370)

Author: Salazareo

src/backend/controllers/oidc/OIDCController.test.ts

@@ -92,6 +92,7 @@ interface CapturedResponse {
     redirectUrl?: string;
     headers: Record<string, string>;
     cookies: Array<{ name: string; value: string; opts?: unknown }>;
+    clearedCookies: Array<{ name: string; opts?: unknown }>;
     contentType?: string;
 }
 
@@ -100,13 +101,15 @@ const makeReq = (init: {
     query?: Record<string, unknown>;
     params?: Record<string, unknown>;
     headers?: Record<string, string>;
+    cookies?: Record<string, string>;
     method?: string;
 }): Request => {
     return {
         body: init.body ?? {},
         query: init.query ?? {},
         params: init.params ?? {},
         headers: init.headers ?? {},
+        cookies: init.cookies ?? {},
         method: init.method ?? 'GET',
     } as unknown as Request;
 };
@@ -117,6 +120,7 @@ const makeRes = () => {
         body: undefined,
         headers: {},
         cookies: [],
+        clearedCookies: [],
     };
     const res = {
         json: vi.fn((value: unknown) => {
@@ -155,6 +159,10 @@ const makeRes = () => {
             captured.cookies.push({ name, value, opts });
             return res;
         }),
+        clearCookie: vi.fn((name: string, opts?: unknown) => {
+            captured.clearedCookies.push({ name, opts });
+            return res;
+        }),
         type: vi.fn(() => res),
     };
     return { res: res as unknown as Response, captured };
@@ -767,6 +775,177 @@ describe('OIDCController login callback', () => {
     });
 });
 
+// -- Browser binding / login-CSRF --------------------------------------
+
+describe('OIDCController browser binding', () => {
+    const NONCE_COOKIE = 'puter_oidc_nonce';
+
+    const stubIdP = (sub: string, email: string) => {
+        vi.spyOn(oidc(), 'exchangeCodeForTokens').mockResolvedValue({
+            access_token: 'access',
+            id_token: 'id',
+        } as never);
+        vi.spyOn(oidc(), 'getUserInfo').mockResolvedValue({
+            sub,
+            email,
+            email_verified: true,
+        } as never);
+    };
+
+    it('/start sets an HttpOnly nonce cookie matching the nonce embedded in state', async () => {
+        const { res, captured } = makeRes();
+        await callRoute(
+            'get',
+            '/auth/oidc/:provider/start',
+            makeReq({ params: { provider: 'custom' } }),
+            res,
+        );
+
+        const nonceCookie = captured.cookies.find(
+            (c) => c.name === NONCE_COOKIE,
+        );
+        expect(nonceCookie).toBeTruthy();
+        expect(nonceCookie?.value).toBeTruthy();
+        expect((nonceCookie?.opts as { httpOnly?: boolean })?.httpOnly).toBe(
+            true,
+        );
+
+        // The cookie value must equal the nonce baked into the signed state.
+        const state = new URL(captured.redirectUrl ?? '').searchParams.get(
+            'state',
+        );
+        const decoded = oidc().verifyState(state!);
+        expect(decoded?.nonce).toBe(nonceCookie?.value);
+    });
+
+    it('completes login when the nonce cookie matches the state nonce', async () => {
+        const sub = `sub-${Math.random().toString(36).slice(2, 8)}`;
+        const email = `bind-${Math.random().toString(36).slice(2, 8)}@test.local`;
+        const nonce = 'browser-nonce-match';
+        const state = oidc().signState({
+            provider: 'custom',
+            redirect_uri: TEST_ORIGIN + '/',
+            nonce,
+        });
+        stubIdP(sub, email);
+
+        const { res, captured } = makeRes();
+        await callRoute(
+            'get',
+            '/auth/oidc/callback/login',
+            makeReq({
+                query: { code: 'c', state },
+                cookies: { [NONCE_COOKIE]: nonce },
+            }),
+            res,
+        );
+
+        // Session cookie issued; single-use nonce cookie cleared.
+        expect(captured.cookies).toHaveLength(1);
+        expect(captured.redirectUrl).toBe(TEST_ORIGIN + '/');
+        expect(
+            captured.clearedCookies.some((c) => c.name === NONCE_COOKIE),
+        ).toBe(true);
+    });
+
+    it('rejects login (no session cookie) when the nonce cookie is absent — the login-CSRF case', async () => {
+        const state = oidc().signState({
+            provider: 'custom',
+            redirect_uri: TEST_ORIGIN + '/',
+            nonce: 'attacker-flow-nonce',
+        });
+        // If enforcement were missing, this would resolve a user and set a
+        // session cookie for the victim's browser. It must not get that far.
+        const exchangeSpy = vi.spyOn(oidc(), 'exchangeCodeForTokens');
+
+        const { res, captured } = makeRes();
+        await callRoute(
+            'get',
+            '/auth/oidc/callback/login',
+            // Victim's browser has no nonce cookie for the attacker's flow.
+            makeReq({ query: { code: 'c', state }, cookies: {} }),
+            res,
+        );
+
+        expect(captured.redirectStatus).toBe(302);
+        expect(captured.redirectUrl).toContain('auth_error=1');
+        expect(captured.cookies).toHaveLength(0);
+        // We bail before ever exchanging the code.
+        expect(exchangeSpy).not.toHaveBeenCalled();
+    });
+
+    it('rejects login when the nonce cookie does not match the state nonce', async () => {
+        const state = oidc().signState({
+            provider: 'custom',
+            redirect_uri: TEST_ORIGIN + '/',
+            nonce: 'expected-nonce',
+        });
+        const exchangeSpy = vi.spyOn(oidc(), 'exchangeCodeForTokens');
+
+        const { res, captured } = makeRes();
+        await callRoute(
+            'get',
+            '/auth/oidc/callback/login',
+            makeReq({
+                query: { code: 'c', state },
+                cookies: { [NONCE_COOKIE]: 'a-different-nonce' },
+            }),
+            res,
+        );
+
+        expect(captured.redirectUrl).toContain('auth_error=1');
+        expect(captured.cookies).toHaveLength(0);
+        expect(exchangeSpy).not.toHaveBeenCalled();
+    });
+
+    it('rejects the revalidate callback (400) when the nonce cookie is missing', async () => {
+        const state = oidc().signState({
+            provider: 'custom',
+            flow: 'revalidate',
+            user_uuid: uuidv4(),
+            nonce: 'reval-nonce',
+        });
+        const exchangeSpy = vi.spyOn(oidc(), 'exchangeCodeForTokens');
+
+        const { res, captured } = makeRes();
+        await callRoute(
+            'get',
+            '/auth/oidc/callback/revalidate',
+            makeReq({ query: { code: 'c', state }, cookies: {} }),
+            res,
+        );
+
+        expect(captured.statusCode).toBe(400);
+        expect(exchangeSpy).not.toHaveBeenCalled();
+    });
+
+    it('lets legacy nonce-less state through (deploy grace) without touching the nonce cookie', async () => {
+        const sub = `sub-${Math.random().toString(36).slice(2, 8)}`;
+        const email = `legacy-${Math.random().toString(36).slice(2, 8)}@test.local`;
+        // No `nonce` field — mimics a state signed before this shipped.
+        const state = oidc().signState({
+            provider: 'custom',
+            redirect_uri: TEST_ORIGIN + '/',
+        });
+        stubIdP(sub, email);
+
+        const { res, captured } = makeRes();
+        await callRoute(
+            'get',
+            '/auth/oidc/callback/login',
+            makeReq({ query: { code: 'c', state }, cookies: {} }),
+            res,
+        );
+
+        // Proceeds as before; no nonce cookie is cleared for legacy states.
+        expect(captured.cookies).toHaveLength(1);
+        expect(captured.redirectUrl).toBe(TEST_ORIGIN + '/');
+        expect(
+            captured.clearedCookies.some((c) => c.name === NONCE_COOKIE),
+        ).toBe(false);
+    });
+});
+
 // ── /auth/oidc/callback/signup ──────────────────────────────────────
 
 describe('OIDCController signup callback', () => {

src/backend/controllers/oidc/OIDCController.ts

@@ -17,6 +17,7 @@
  * along with this program.  If not, see <https://www.gnu.org/licenses/>.
  */
 
+import crypto from 'node:crypto';
 import type { Request, Response } from 'express';
 import { HttpError } from '../../core/http/HttpError.js';
 import type { PuterRouter } from '../../core/http/PuterRouter.js';
@@ -26,6 +27,12 @@ import { sessionCookieFlags } from '../../util/cookieFlags.js';
 const REVALIDATION_COOKIE_NAME = 'puter_revalidation';
 const REVALIDATION_EXPIRY_SEC = 300;
 
+// Companion cookie that binds an OIDC flow to the browser that started it.
+// Expiry mirrors STATE_EXPIRY_SEC in OIDCService — the state and its
+// browser-binding cookie must expire together.
+const OIDC_NONCE_COOKIE_NAME = 'puter_oidc_nonce';
+const OIDC_NONCE_EXPIRY_SEC = 600;
+
 const OIDC_ERROR_REDIRECT_MAP: Record<string, Record<string, string>> = {
     login: { account_not_found: 'signup', other: 'login' },
     signup: { account_already_exists: 'login', other: 'signup' },
@@ -73,6 +80,14 @@ function appendQueryParam(url: string, key: string, value: string): string {
     return `${url}${sep}${encodeURIComponent(key)}=${encodeURIComponent(value)}`;
 }
 
+/** Length-safe constant-time string compare (never throws on mismatch). */
+function constantTimeEqual(a: string, b: string): boolean {
+    const ba = Buffer.from(a);
+    const bb = Buffer.from(b);
+    if (ba.length !== bb.length) return false;
+    return crypto.timingSafeEqual(ba, bb);
+}
+
 /**
  * True iff `target` parses as a URL whose origin equals `origin`. Used to
  * clamp OIDC redirect targets — `startsWith` would accept
@@ -211,6 +226,17 @@ export class OIDCController extends PuterController {
                     statePayload.flow = 'revalidate';
                 }
 
+                // Bind this flow to the initiating browser: a single-use
+                // nonce lives both in the signed `state` and in an HttpOnly
+                // companion cookie. The callback requires them to match, so a
+                // `state` captured from an attacker's own flow can't be
+                // replayed in a victim's browser (login-CSRF / session
+                // fixation).
+                const browserNonce = crypto
+                    .randomBytes(32)
+                    .toString('base64url');
+                statePayload.nonce = browserNonce;
+
                 const state = this.services.oidc.signState(statePayload);
                 const url = await this.services.oidc.getAuthorizationUrl(
                     provider,
@@ -224,6 +250,15 @@ export class OIDCController extends PuterController {
                         { legacyCode: 'internal_error' },
                     );
 
+                res.cookie(OIDC_NONCE_COOKIE_NAME, browserNonce, {
+                    // Same flags as the session cookie: SameSite=None;Secure
+                    // on HTTPS so the cookie survives Apple's cross-site
+                    // form_post callback; Lax on plain-HTTP self-host.
+                    ...sessionCookieFlags(this.config),
+                    httpOnly: true,
+                    maxAge: OIDC_NONCE_EXPIRY_SEC * 1000,
+                    path: '/',
+                });
                 res.redirect(302, url);
             },
         );
@@ -237,7 +272,7 @@ export class OIDCController extends PuterController {
 
         const loginCb = async (req: Request, res: Response) => {
             const origin = this.config.origin ?? '';
-            const result = await this.#processCallback(req, 'login');
+            const result = await this.#processCallback(req, res, 'login');
             if ('error' in result) {
                 console.warn(`OIDC login callback error: ${result.error}`);
                 return res.redirect(
@@ -300,7 +335,7 @@ export class OIDCController extends PuterController {
 
         const signupCb = async (req: Request, res: Response) => {
             const origin = this.config.origin ?? '';
-            const result = await this.#processCallback(req, 'signup');
+            const result = await this.#processCallback(req, res, 'signup');
             if ('error' in result) {
                 return res.redirect(
                     302,
@@ -365,7 +400,7 @@ export class OIDCController extends PuterController {
             req: Request,
             res: Response,
         ): Promise<void> => {
-            const result = await this.#processCallback(req, 'revalidate');
+            const result = await this.#processCallback(req, res, 'revalidate');
             if ('error' in result) {
                 res.status(400).send(result.error);
                 return;
@@ -517,6 +552,7 @@ if (window.opener) {
 
     async #processCallback(
         req: Request,
+        res: Response,
         flow: string,
     ): Promise<
         | { error: string }
@@ -540,6 +576,28 @@ if (window.opener) {
         if (!stateDecoded || !stateDecoded.provider)
             return { error: 'Invalid or expired state.' };
 
+        // Enforce the browser binding set at /start. Every state minted by
+        // the current /start carries a nonce, so this covers all live flows.
+        // States signed before this shipped have no nonce and pass through
+        // until they expire (STATE_EXPIRY, 10 min) so in-flight logins don't
+        // break on deploy — a caller can't forge a nonce-less state because
+        // /start always adds one and the state is server-signed.
+        const expectedNonce =
+            typeof stateDecoded.nonce === 'string' ? stateDecoded.nonce : '';
+        if (expectedNonce) {
+            const cookieNonce = req.cookies?.[OIDC_NONCE_COOKIE_NAME];
+            // Single-use: drop the cookie regardless of the outcome.
+            res.clearCookie(OIDC_NONCE_COOKIE_NAME, { path: '/' });
+            if (
+                typeof cookieNonce !== 'string' ||
+                !constantTimeEqual(cookieNonce, expectedNonce)
+            ) {
+                return {
+                    error: 'This sign-in could not be verified for your browser. Please start again.',
+                };
+            }
+        }
+
         const provider = String(stateDecoded.provider);
         const callbackUrl = this.services.oidc.getCallbackUrl(flow);
         if (!callbackUrl) return { error: 'Invalid flow.' };