fix(connect): validate OAuth state nonce server-side to prevent CSRF (#223)

apps/backend/src/__tests__/connect.test.ts

@@ -1,8 +1,6 @@
 import { describe, it, expect, beforeEach, vi } from 'vitest';
 import Fastify from 'fastify';
-import jwt from '@fastify/jwt';
 import { connectRoutes } from '../routes/connect.js';
-import type { PrismaClient } from '@prisma/client';
 
 process.env.PUBLIC_APP_URL = 'http://localhost:3000';
 process.env.BACKEND_URL = 'http://localhost:3001';
@@ -11,9 +9,12 @@
 process.env.GITHUB_CLIENT_SECRET = 'test-client-secret';
 process.env.ENCRYPTION_KEY = '12345678901234567890123456789012';
 
+const VALID_NONCE = 'a'.repeat(64);
+const USER_ID = 'user-abc';
+
 const mockRedis = {
-  get: vi.fn(),
   set: vi.fn(),
+  get: vi.fn(),
   del: vi.fn(),
 };
 
@@ -25,163 +26,223 @@
   },
 };
 
-global.fetch = vi.fn();
-
-async function buildApp() {
+async function buildApp(authenticatedUserId = USER_ID) {
   const app = Fastify();
-  await app.register(jwt, { secret: 'test-secret' });
-  app.decorate('prisma', mockPrisma as unknown as PrismaClient);
   app.decorate('redis', mockRedis as any);
-
-  app.decorate('authenticate', async (request: any, reply: any) => {
-    try {
-      await request.jwtVerify();
-    } catch (err) {
-      reply.status(401).send({ error: 'Unauthorized' });
-    }
+  app.decorate('prisma', mockPrisma as any);
+  app.decorate('authenticate', async (request: any) => {
+    request.user = { id: authenticatedUserId };
   });
-
   app.register(connectRoutes, { prefix: '/api/connect' });
   await app.ready();
   return app;
 }
 
+describe('GET /api/connect/github', () => {
+  beforeEach(() => vi.clearAllMocks());
+
+  it('stores nonce in redis with userId and redirects to GitHub', async () => {
+    const app = await buildApp();
+    const res = await app.inject({ method: 'GET', url: '/api/connect/github' });
+
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toMatch(/^https:\/\/github\.com\/login\/oauth\/authorize/);
+    expect(mockRedis.set).toHaveBeenCalledOnce();
+
+    const [key, value, , ttl] = mockRedis.set.mock.calls[0];
+    expect(key).toMatch(/^oauth:connect-nonce:[0-9a-f]{64}$/);
+    const stored = JSON.parse(value);
+    expect(stored.userId).toBe(USER_ID);
+    expect(ttl).toBe(600);
+
+    // nonce in redirect state must match stored key
+    const location = res.headers.location as string;
+    const stateParam = new URL(location).searchParams.get('state');
+    expect(key).toBe(`oauth:connect-nonce:${stateParam}`);
+  });
+
+  it('state param is 64 lowercase hex chars', async () => {
+    const app = await buildApp();
+    const res = await app.inject({ method: 'GET', url: '/api/connect/github' });
+    const location = res.headers.location as string;
+    const stateParam = new URL(location).searchParams.get('state');
+    expect(stateParam).toMatch(/^[0-9a-f]{64}$/);
+  });
+});
+
 describe('GET /api/connect/github/callback', () => {
-  beforeEach(() => {
-    vi.clearAllMocks();
+  beforeEach(() => vi.clearAllMocks());
+
+  it('valid flow: completes connect, writes github_follow token, redirects to settings', async () => {
+    mockRedis.get.mockResolvedValue(JSON.stringify({ userId: USER_ID }));
+    mockRedis.del.mockResolvedValue(1);
+    mockPrisma.oAuthToken.upsert.mockResolvedValue({});
+
+    global.fetch = vi.fn().mockResolvedValue({
+      json: async () => ({ access_token: 'gh-token', scope: 'user:follow' }),
+    }) as any;
+
+    const app = await buildApp();
+    const res = await app.inject({
+      method: 'GET',
+      url: `/api/connect/github/callback?code=validcode&state=${VALID_NONCE}`,
+    });
+
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toContain('connected=github');
+    expect(mockRedis.del).toHaveBeenCalledWith(`oauth:connect-nonce:${VALID_NONCE}`);
+    expect(mockPrisma.oAuthToken.upsert).toHaveBeenCalledWith(
+      expect.objectContaining({
+        where: { userId_platform: { userId: USER_ID, platform: 'github_follow' } },
+      })
+    );
   });
 
-  it('redirects with missing_params if code or state is missing', async () => {
+  it('missing code redirects with missing_params error', async () => {
     const app = await buildApp();
-
-    // Missing code
-    let res = await app.inject({
+    const res = await app.inject({
       method: 'GET',
-      url: '/api/connect/github/callback?state=somestate',
+      url: `/api/connect/github/callback?state=${VALID_NONCE}`,
     });
     expect(res.statusCode).toBe(302);
-    expect(res.headers.location).toBe('http://localhost:3000/settings?error=missing_params');
+    expect(res.headers.location).toContain('error=missing_params');
+    expect(mockRedis.get).not.toHaveBeenCalled();
+  });
 
-    // Missing state
-    res = await app.inject({
+  it('missing state redirects with missing_params error', async () => {
+    const app = await buildApp();
+    const res = await app.inject({
       method: 'GET',
-      url: '/api/connect/github/callback?code=somecode',
+      url: '/api/connect/github/callback?code=validcode',
     });
     expect(res.statusCode).toBe(302);
-    expect(res.headers.location).toBe('http://localhost:3000/settings?error=missing_params');
+    expect(res.headers.location).toContain('error=missing_params');
   });
 
-  it('redirects with connect_failed if state is invalid/malformed', async () => {
+  it('malformed state (too short) redirects with connect_failed', async () => {
     const app = await buildApp();
-    const invalidState = Buffer.from(JSON.stringify({ wrongKey: 'value' })).toString('base64');
-
     const res = await app.inject({
       method: 'GET',
-      url: `/api/connect/github/callback?code=testcode&state=${invalidState}`,
+      url: '/api/connect/github/callback?code=validcode&state=tooshort',
     });
-
     expect(res.statusCode).toBe(302);
-    expect(res.headers.location).toBe('http://localhost:3000/settings?error=connect_failed');
+    expect(res.headers.location).toContain('error=connect_failed');
+    expect(mockRedis.get).not.toHaveBeenCalled();
   });
 
-  it('redirects with invalid_state if nonce is not found in Redis (CSRF/Expired)', async () => {
-    mockRedis.get.mockResolvedValue(null);
+  it('malformed state (non-hex chars) redirects with connect_failed', async () => {
     const app = await buildApp();
-    const validState = Buffer.from(JSON.stringify({ userId: 'user-1', nonce: 'nonce-123' })).toString('base64');
-
+    const badState = 'z'.repeat(64);
     const res = await app.inject({
       method: 'GET',
-      url: `/api/connect/github/callback?code=testcode&state=${validState}`,
+      url: `/api/connect/github/callback?code=validcode&state=${badState}`,
     });
-
-    expect(mockRedis.get).toHaveBeenCalledWith('oauth:nonce:nonce-123');
     expect(res.statusCode).toBe(302);
-    expect(res.headers.location).toBe('http://localhost:3000/settings?error=invalid_state');
+    expect(res.headers.location).toContain('error=connect_failed');
+    expect(mockRedis.get).not.toHaveBeenCalled();
   });
 
-  it('redirects with invalid_state if Redis userId does not match state userId', async () => {
-    mockRedis.get.mockResolvedValue('different-user-id');
+  it('forged base64-JSON state is rejected as malformed', async () => {
     const app = await buildApp();
-    const validState = Buffer.from(JSON.stringify({ userId: 'user-1', nonce: 'nonce-123' })).toString('base64');
-
+    const forgedState = Buffer.from(JSON.stringify({ userId: USER_ID, nonce: 'x'.repeat(32) })).toString('base64');
     const res = await app.inject({
       method: 'GET',
-      url: `/api/connect/github/callback?code=testcode&state=${validState}`,
+      url: `/api/connect/github/callback?code=validcode&state=${forgedState}`,
     });
-
     expect(res.statusCode).toBe(302);
-    expect(res.headers.location).toBe('http://localhost:3000/settings?error=invalid_state');
+    expect(res.headers.location).toContain('error=connect_failed');
+    expect(mockRedis.get).not.toHaveBeenCalled();
   });
 
-  it('successfully exchanges code, upserts token, and redirects on valid flow (Web)', async () => {
-    mockRedis.get.mockResolvedValue('user-1');
-    (global.fetch as any).mockResolvedValue({
-      json: vi.fn().mockResolvedValue({ access_token: 'github-access-token', scope: 'user:follow' })
+  it('unknown nonce (not in Redis) redirects with connect_failed', async () => {
+    mockRedis.get.mockResolvedValue(null);
+    const app = await buildApp();
+    const res = await app.inject({
+      method: 'GET',
+      url: `/api/connect/github/callback?code=validcode&state=${VALID_NONCE}`,
     });
-    mockPrisma.oAuthToken.upsert.mockResolvedValue({});
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toContain('error=connect_failed');
+    expect(mockRedis.del).not.toHaveBeenCalled();
+  });
 
+  it('expired nonce (Redis returns null) redirects with connect_failed', async () => {
+    mockRedis.get.mockResolvedValue(null);
     const app = await buildApp();
-    const validState = Buffer.from(JSON.stringify({ userId: 'user-1', nonce: 'web_nonce-123' })).toString('base64');
-
     const res = await app.inject({
       method: 'GET',
-      url: `/api/connect/github/callback?code=testcode&state=${validState}`,
+      url: `/api/connect/github/callback?code=validcode&state=${VALID_NONCE}`,
     });
-
-    // Nonce should be deleted immediately
-    expect(mockRedis.del).toHaveBeenCalledWith('oauth:nonce:web_nonce-123');
-
-    // Code exchange should be triggered
-    expect(global.fetch).toHaveBeenCalledWith('https://github.com/login/oauth/access_token', expect.objectContaining({
-      method: 'POST',
-      body: expect.stringContaining('testcode')
-    }));
-
-    // Upsert should be called
-    expect(mockPrisma.oAuthToken.upsert).toHaveBeenCalledWith(expect.objectContaining({
-      where: { userId_platform: { userId: 'user-1', platform: 'github_follow' } }
-    }));
-
-    // Redirects to web success
     expect(res.statusCode).toBe(302);
-    expect(res.headers.location).toBe('http://localhost:3000/settings?connected=github');
+    expect(res.headers.location).toContain('error=connect_failed');
   });
 
-  it('redirects to mobile scheme if nonce starts with mobile_', async () => {
-    mockRedis.get.mockResolvedValue('user-1');
-    (global.fetch as any).mockResolvedValue({
-      json: vi.fn().mockResolvedValue({ access_token: 'github-access-token', scope: 'user:follow' })
+  it('forged state (random nonce not stored in Redis) redirects with connect_failed', async () => {
+    mockRedis.get.mockResolvedValue(null);
+    const forgedNonce = 'b'.repeat(64);
+    const app = await buildApp();
+    const res = await app.inject({
+      method: 'GET',
+      url: `/api/connect/github/callback?code=validcode&state=${forgedNonce}`,
     });
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toContain('error=connect_failed');
+  });
+
+  it('replay attack: second use of same nonce fails after first succeeds', async () => {
+    mockRedis.del.mockResolvedValue(1);
     mockPrisma.oAuthToken.upsert.mockResolvedValue({});
 
+    global.fetch = vi.fn().mockResolvedValue({
+      json: async () => ({ access_token: 'gh-token', scope: 'user:follow' }),
+    }) as any;
+
+    // First call succeeds
+    mockRedis.get.mockResolvedValueOnce(JSON.stringify({ userId: USER_ID }));
+    const app = await buildApp();
+    const res1 = await app.inject({
+      method: 'GET',
+      url: `/api/connect/github/callback?code=code1&state=${VALID_NONCE}`,
+    });
+    expect(res1.statusCode).toBe(302);
+    expect(res1.headers.location).toContain('connected=github');
+
+    // Second call: nonce deleted, Redis returns null
+    mockRedis.get.mockResolvedValueOnce(null);
+    const res2 = await app.inject({
+      method: 'GET',
+      url: `/api/connect/github/callback?code=code2&state=${VALID_NONCE}`,
+    });
+    expect(res2.statusCode).toBe(302);
+    expect(res2.headers.location).toContain('error=connect_failed');
+  });
+
+  it('corrupt nonce data in Redis redirects with connect_failed', async () => {
+    mockRedis.get.mockResolvedValue('not-valid-json{{{');
+    mockRedis.del.mockResolvedValue(1);
     const app = await buildApp();
-    const validState = Buffer.from(JSON.stringify({ userId: 'user-1', nonce: 'mobile_nonce-123' })).toString('base64');
-
     const res = await app.inject({
       method: 'GET',
-      url: `/api/connect/github/callback?code=testcode&state=${validState}`,
+      url: `/api/connect/github/callback?code=validcode&state=${VALID_NONCE}`,
     });
-
     expect(res.statusCode).toBe(302);
-    expect(res.headers.location).toBe('devcard://connect?connected=github');
+    expect(res.headers.location).toContain('error=connect_failed');
   });
 
-  it('redirects with connect_failed if token exchange returns an error', async () => {
-    mockRedis.get.mockResolvedValue('user-1');
-    (global.fetch as any).mockResolvedValue({
-      json: vi.fn().mockResolvedValue({ error: 'bad_verification_code' })
-    });
+  it('GitHub token exchange failure redirects with connect_failed', async () => {
+    mockRedis.get.mockResolvedValue(JSON.stringify({ userId: USER_ID }));
+    mockRedis.del.mockResolvedValue(1);
+
+    global.fetch = vi.fn().mockResolvedValue({
+      json: async () => ({ error: 'bad_verification_code' }),
+    }) as any;
 
     const app = await buildApp();
-    const validState = Buffer.from(JSON.stringify({ userId: 'user-1', nonce: 'nonce-123' })).toString('base64');
-
     const res = await app.inject({
       method: 'GET',
-      url: `/api/connect/github/callback?code=testcode&state=${validState}`,
+      url: `/api/connect/github/callback?code=badcode&state=${VALID_NONCE}`,
     });
-
-    expect(mockPrisma.oAuthToken.upsert).not.toHaveBeenCalled();
     expect(res.statusCode).toBe(302);
-    expect(res.headers.location).toBe('http://localhost:3000/settings?error=connect_failed');
+    expect(res.headers.location).toContain('error=connect_failed');
   });
-});
+});

apps/backend/src/__tests__/oauth-scope.test.ts

@@ -12,9 +12,9 @@
  */
 
 import { describe, it, expect, beforeEach, vi } from 'vitest';
 import Fastify from 'fastify';
 import { connectRoutes } from '../routes/connect.js';
 import { followRoutes } from '../routes/follow.js';
 import type { PrismaClient } from '@prisma/client';
 
 // ── Mocks ─────────────────────────────────────────────────────────────────────
@@ -38,24 +38,35 @@
 
 // ── Connect-route test harness ────────────────────────────────────────────────
 
-function makeConnectState(userId: string): string {
-  return Buffer.from(JSON.stringify({ userId, nonce: 'test-nonce' })).toString('base64');
+// 64 lowercase hex chars — matches the format generated by randomBytes(32).toString('hex')
+const TEST_NONCE = 'a'.repeat(64);
+
+function makeConnectState(_userId: string): string {
+  // State carries only the nonce; userId is stored server-side in Redis.
+  return TEST_NONCE;
 }
 
-function buildConnectApp(mockPrisma: Partial<PrismaClient>) {
+function buildConnectApp(mockPrisma: Partial<PrismaClient>, userId = USER_ID) {
+  const mockRedis = {
+    get: vi.fn().mockResolvedValue(JSON.stringify({ userId })),
+    del: vi.fn().mockResolvedValue(1),
+    set: vi.fn().mockResolvedValue('OK'),
+  };
+
   const app = Fastify({ logger: false });
   app.decorate('prisma', mockPrisma as PrismaClient);
-  app.decorate('authenticate', async (req: any) => { req.user = { id: USER_ID }; });
+  app.decorate('redis', mockRedis as any);
+  app.decorate('authenticate', async (req: any) => { req.user = { id: userId }; });
   app.register(connectRoutes, { prefix: '/api/connect' });
   return app.ready().then(() => app);
 }
 
 // ── Follow-route test harness ─────────────────────────────────────────────────
 
 function buildFollowApp(mockPrisma: Partial<PrismaClient>) {
   const app = Fastify({ logger: false });
   app.decorate('prisma', mockPrisma as PrismaClient);
   app.decorate('authenticate', async (req: any) => { req.user = { id: USER_ID }; });
   app.register(followRoutes, { prefix: '/api/follow' });
   return app.ready().then(() => app);
 }