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

Replace client-side-only state validation in the GitHub connect flow with
server-side nonce verification backed by Redis.

- Generate a 32-byte cryptographically secure nonce on connect initiation
- Store {userId} in Redis under oauth:connect-nonce:<nonce> with a 10-minute TTL
- State parameter carries only the nonce; userId is never trusted from the callback
- On callback: validate nonce format, look up Redis, reject unknown/expired nonces
- Delete nonce immediately after first successful validation (replay protection)
- Drop Math.random()-based state generation in favour of randomBytes(32)
- Add 13 tests covering: valid flow, missing params, malformed state, unknown nonce,
  expired nonce, forged state, replay attack, corrupt Redis data, token exchange failure

Author: Ridanshi

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

@@ -1,39 +1,229 @@
-import { describe, it, expect } from 'vitest';
+import { describe, it, expect, beforeEach, vi } from 'vitest';
+import Fastify from 'fastify';
+import { connectRoutes } from '../routes/connect.js';
 
-// Mock test for GitHub OAuth callback state validation
-// Note: This test verifies the expected behavior of the
-// /api/connect/github/callback endpoint when invalid or
-// malformed OAuth state values are received.
-//
-// The implementation in connect.ts now:
-// - safely parses OAuth state via parseGoogleState()
-// - validates required fields (userId + nonce)
-// - redirects invalid callbacks safely
-//
-// Security note:
-// OAuth state validation helps prevent tampered callback
-// requests and malformed state payload attacks.
+const VALID_NONCE = 'a'.repeat(64);
+const USER_ID = 'user-abc';
 
-describe('GET /api/connect/github/callback - Invalid OAuth State', () => {
+const mockRedis = {
+  set: vi.fn(),
+  get: vi.fn(),
+  del: vi.fn(),
+};
 
-    it('should redirect with connect_failed when state is invalid', async () => {
-        // Expected behavior:
-        // parseGoogleState('invalid_state') -> null
-        // reply.redirect(`${PUBLIC_APP_URL}/settings?error=connect_failed`)
+const mockPrisma = {
+  oAuthToken: {
+    findMany: vi.fn(),
+    upsert: vi.fn(),
+    delete: vi.fn(),
+  },
+};
 
-        expect(true).toBe(true);
+const mockEncryption = {
+  encrypt: vi.fn().mockReturnValue('encrypted-token'),
+};
+
+async function buildApp(authenticatedUserId = USER_ID) {
+  const app = Fastify();
+  app.decorate('redis', mockRedis as any);
+  app.decorate('prisma', mockPrisma as any);
+  app.decorate('encryption', mockEncryption 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());
+
+  it('valid flow: completes connect and 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}`);
+  });
+
+  it('missing code redirects with missing_params error', async () => {
+    const app = await buildApp();
+    const res = await app.inject({
+      method: 'GET',
+      url: `/api/connect/github/callback?state=${VALID_NONCE}`,
+    });
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toContain('error=missing_params');
+    expect(mockRedis.get).not.toHaveBeenCalled();
+  });
+
+  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=validcode',
+    });
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toContain('error=missing_params');
+  });
+
+  it('malformed state (too short) redirects with connect_failed', async () => {
+    const app = await buildApp();
+    const res = await app.inject({
+      method: 'GET',
+      url: '/api/connect/github/callback?code=validcode&state=tooshort',
     });
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toContain('error=connect_failed');
+    expect(mockRedis.get).not.toHaveBeenCalled();
+  });
 
-    it('should reject malformed oauth state payloads', async () => {
-        // Example malformed payload:
-        // { invalid: true }
-        //
-        // Expected:
-        // - missing userId
-        // - missing nonce
-        // - redirect to connect_failed
+  it('malformed state (non-hex chars) redirects with connect_failed', async () => {
+    const app = await buildApp();
+    const badState = 'z'.repeat(64);
+    const res = await app.inject({
+      method: 'GET',
+      url: `/api/connect/github/callback?code=validcode&state=${badState}`,
+    });
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toContain('error=connect_failed');
+    expect(mockRedis.get).not.toHaveBeenCalled();
+  });
 
-        expect(true).toBe(true);
+  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}`,
     });
+    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 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('error=connect_failed');
+  });
+
+  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 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('error=connect_failed');
+  });
+
+  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 res = await app.inject({
+      method: 'GET',
+      url: `/api/connect/github/callback?code=badcode&state=${VALID_NONCE}`,
+    });
+    expect(res.statusCode).toBe(302);
+    expect(res.headers.location).toContain('error=connect_failed');
+  });
+});