From f29c39e763749aa9a9ed4d66edcf9f5b1a361c4c Mon Sep 17 00:00:00 2001 From: Ridanshi Date: Sat, 13 Jun 2026 00:03:48 +0530 Subject: [PATCH] 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: 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 --- apps/backend/src/__tests__/connect.test.ts | 273 +++++++++++------- .../backend/src/__tests__/oauth-scope.test.ts | 26 +- apps/backend/src/routes/connect.ts | 103 ++++--- 3 files changed, 240 insertions(+), 162 deletions(-) diff --git a/apps/backend/src/__tests__/connect.test.ts b/apps/backend/src/__tests__/connect.test.ts index 8e8604c3..84e82f55 100644 --- a/apps/backend/src/__tests__/connect.test.ts +++ b/apps/backend/src/__tests__/connect.test.ts @@ -1,12 +1,9 @@ -import jwt from '@fastify/jwt'; import Fastify from 'fastify'; -import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { connectRoutes } from '../routes/connect.js'; import { encrypt } from '../utils/encryption.js'; -import type { PrismaClient } from '@prisma/client'; - process.env.PUBLIC_APP_URL = 'http://localhost:3000'; process.env.BACKEND_URL = 'http://localhost:3001'; process.env.MOBILE_REDIRECT_URI = 'devcard://connect'; @@ -14,9 +11,13 @@ process.env.GITHUB_CLIENT_ID = 'test-client-id'; process.env.GITHUB_CLIENT_SECRET = 'test-client-secret'; process.env.ENCRYPTION_KEY = '12345678901234567890123456789012'; +const VALID_NONCE = 'a'.repeat(64); +const USER_ID = 'user-1'; + const mockRedis = { - get: vi.fn(), set: vi.fn(), + get: vi.fn(), + getdel: vi.fn(), del: vi.fn(), }; @@ -31,167 +32,238 @@ const mockPrisma = { global.fetch = vi.fn(); -async function buildApp(): Promise> { +async function buildApp(authenticatedUserId = USER_ID): Promise> { 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 { - 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; } -function authHeader(app: any): { authorization: string } { - return { authorization: `Bearer ${app.jwt.sign({ id: 'user-1' })}` }; -} +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); + + 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.getdel.mockResolvedValue(JSON.stringify({ userId: USER_ID })); + mockPrisma.oAuthToken.upsert.mockResolvedValue({}); + + (global.fetch as any).mockResolvedValue({ + json: async () => ({ access_token: 'gh-token', scope: 'user:follow' }), + }); + + 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.getdel).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.getdel).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'); + expect(mockRedis.getdel).not.toHaveBeenCalled(); }); - 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.getdel).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.getdel).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.getdel).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.getdel.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'); + }); + it('expired nonce (Redis returns null) redirects with connect_failed', async () => { + mockRedis.getdel.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.getdel.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 () => { mockPrisma.oAuthToken.upsert.mockResolvedValue({}); + (global.fetch as any).mockResolvedValue({ + json: async () => ({ access_token: 'gh-token', scope: 'user:follow' }), + }); + + const app = await buildApp(); + + // First call succeeds + mockRedis.getdel.mockResolvedValueOnce(JSON.stringify({ userId: USER_ID })); + 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 already consumed, Redis returns null + mockRedis.getdel.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.getdel.mockResolvedValue('not-valid-json{{{'); 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'); + it('Redis outage during callback redirects with server_error', async () => { + mockRedis.getdel.mockRejectedValue(new Error('Redis unavailable')); + 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=server_error'); + expect(mockPrisma.oAuthToken.upsert).not.toHaveBeenCalled(); + }); + + it('GitHub token exchange failure redirects with connect_failed', async () => { + mockRedis.getdel.mockResolvedValue(JSON.stringify({ userId: USER_ID })); + (global.fetch as any).mockResolvedValue({ - json: vi.fn().mockResolvedValue({ error: 'bad_verification_code' }) + json: async () => ({ error: 'bad_verification_code' }), }); 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'); + expect(mockPrisma.oAuthToken.upsert).not.toHaveBeenCalled(); }); +}); + +describe('GET /api/connect/github/autodiscover', () => { + beforeEach(() => vi.clearAllMocks()); it('returns cached discovery suggestions when Redis stores the response', async () => { const cachedResponse = [{ platform: 'twitter', username: 'octocat', confidence: 'high' }]; @@ -201,7 +273,6 @@ describe('GET /api/connect/github/callback', () => { const res = await app.inject({ method: 'GET', url: '/api/connect/github/autodiscover', - headers: authHeader(app), }); expect(res.statusCode).toBe(200); @@ -228,7 +299,6 @@ describe('GET /api/connect/github/callback', () => { const res = await app.inject({ method: 'GET', url: '/api/connect/github/autodiscover', - headers: authHeader(app), }); const expected = [ @@ -238,7 +308,7 @@ describe('GET /api/connect/github/callback', () => { expect(res.statusCode).toBe(200); expect(res.json()).toEqual(expected); - expect(mockRedis.set).toHaveBeenCalledWith('github:autodiscover:user-1', JSON.stringify(expected), 'EX', 3600); + expect(mockRedis.set).toHaveBeenCalledWith(`github:autodiscover:${USER_ID}`, JSON.stringify(expected), 'EX', 3600); }); it('returns unauthorized when GitHub API returns 401', async () => { @@ -254,12 +324,11 @@ describe('GET /api/connect/github/callback', () => { const res = await app.inject({ method: 'GET', url: '/api/connect/github/autodiscover', - headers: authHeader(app), }); expect(res.statusCode).toBe(401); expect(res.json()).toEqual({ error: 'GitHub token expired or revoked', requiresAuth: true }); - expect(mockRedis.del).toHaveBeenCalledWith('github:autodiscover:user-1'); + expect(mockRedis.del).toHaveBeenCalledWith(`github:autodiscover:${USER_ID}`); }); it('returns an error when the GitHub follow token is missing', async () => { @@ -270,7 +339,6 @@ describe('GET /api/connect/github/callback', () => { const res = await app.inject({ method: 'GET', url: '/api/connect/github/autodiscover', - headers: authHeader(app), }); expect(res.statusCode).toBe(400); @@ -297,7 +365,6 @@ describe('GET /api/connect/github/callback', () => { const res = await app.inject({ method: 'GET', url: '/api/connect/github/autodiscover', - headers: authHeader(app), }); expect(res.statusCode).toBe(200); @@ -307,4 +374,4 @@ describe('GET /api/connect/github/callback', () => { ]); expect(global.fetch).toHaveBeenCalled(); }); -}); \ No newline at end of file +}); diff --git a/apps/backend/src/__tests__/oauth-scope.test.ts b/apps/backend/src/__tests__/oauth-scope.test.ts index 0985dfa7..df2dd026 100644 --- a/apps/backend/src/__tests__/oauth-scope.test.ts +++ b/apps/backend/src/__tests__/oauth-scope.test.ts @@ -11,10 +11,12 @@ * flow so the two records are independent and can never overwrite each other. */ -import { describe, it, expect, beforeEach, vi } from 'vitest'; import Fastify from 'fastify'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + import { connectRoutes } from '../routes/connect.js'; import { followRoutes } from '../routes/follow.js'; + import type { PrismaClient } from '@prisma/client'; // ── Mocks ───────────────────────────────────────────────────────────────────── @@ -38,14 +40,26 @@ const USER_ID = 'user-scope-test'; // ── 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) { +function buildConnectApp(mockPrisma: Partial, userId = USER_ID) { + const mockRedis = { + get: vi.fn().mockResolvedValue(JSON.stringify({ userId })), + getdel: 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 (request: any) => { request.user = { id: userId }; }); app.register(connectRoutes, { prefix: '/api/connect' }); return app.ready().then(() => app); } @@ -55,7 +69,7 @@ function buildConnectApp(mockPrisma: Partial) { function buildFollowApp(mockPrisma: Partial) { const app = Fastify({ logger: false }); app.decorate('prisma', mockPrisma as PrismaClient); - app.decorate('authenticate', async (req: any) => { req.user = { id: USER_ID }; }); + app.decorate('authenticate', async (request: any) => { request.user = { id: USER_ID }; }); app.register(followRoutes, { prefix: '/api/follow' }); return app.ready().then(() => app); } diff --git a/apps/backend/src/routes/connect.ts b/apps/backend/src/routes/connect.ts index e7e1a2be..3a56e228 100644 --- a/apps/backend/src/routes/connect.ts +++ b/apps/backend/src/routes/connect.ts @@ -15,15 +15,15 @@ const GITHUB_TOKEN_URL = 'https://github.com/login/oauth/access_token'; // silently overwrite the other's access token. const GITHUB_FOLLOW_PLATFORM = 'github_follow'; const GITHUB_AUTODISCOVER_CACHE_TTL = 3600; +const NONCE_TTL = 600; // 10 minutes interface OAuthCallbackQuery { code: string; state?: string; } -interface ParsedOAuthState { +interface StoredConnectNonce { userId: string; - nonce: string; } export async function connectRoutes(app: FastifyInstance): Promise { @@ -58,27 +58,24 @@ export async function connectRoutes(app: FastifyInstance): Promise { }], }, async (request: FastifyRequest, reply: FastifyReply) => { const userId = (request.user as any).id; - const nonce = generateState(); + const nonce = randomBytes(32).toString('hex'); - // Store nonce in Redis with 10-minute TTL. - // The callback verifies this to prevent CSRF attacks. await app.redis.set( - `oauth:nonce:${nonce}`, - userId, + `oauth:connect-nonce:${nonce}`, + JSON.stringify({ userId } satisfies StoredConnectNonce), 'EX', - 600 + NONCE_TTL ); - const state = JSON.stringify({ userId, nonce }); - const redirectUri = `${process.env.BACKEND_URL}/api/connect/github/callback`; const params = new URLSearchParams({ - client_id: process.env.GITHUB_CLIENT_ID || '', + client_id: (process.env.GITHUB_CLIENT_ID || '').trim(), redirect_uri: redirectUri, scope: 'user:follow', - state: Buffer.from(state).toString('base64'), + state: nonce, }); + app.log.debug({ provider: 'github', userId }, 'OAuth connect redirect initiated'); return reply.redirect(`${GITHUB_AUTH_URL}?${params}`); }); @@ -89,30 +86,42 @@ export async function connectRoutes(app: FastifyInstance): Promise { return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=missing_params`); } - try { - // Decode state to find which user requested the connect - const decodedState = parseOAuthState(state); + const nonce = parseNonce(state); + if (!nonce) { + app.log.warn({}, 'OAuth connect state validation failed: malformed state'); + return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=connect_failed`); + } - if (!decodedState) { - return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=connect_failed`); - } + // Atomically consume the nonce — closes the replay TOCTOU window + let storedRaw: string | null; + try { + storedRaw = await app.redis.getdel(`oauth:connect-nonce:${nonce}`); + } catch (err: unknown) { + app.log.error({ err: getErrorMessage(err) }, 'Redis unavailable during OAuth callback'); + return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=server_error`); + } - // Verify nonce was issued by this server -- prevents CSRF - const storedUserId = app.redis ? await app.redis.get(`oauth:nonce:${decodedState.nonce}`) : null; + if (!storedRaw) { + app.log.warn({}, 'OAuth connect state validation failed: unknown or expired nonce'); + return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=connect_failed`); + } - if (app.redis && (!storedUserId || storedUserId !== decodedState.userId)) { - app.log.warn({ nonce: decodedState.nonce }, 'OAuth CSRF check failed: nonce mismatch'); - return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=invalid_state`); - } + let stored: StoredConnectNonce; + try { + stored = JSON.parse(storedRaw); + } catch { + app.log.warn({}, 'OAuth connect state validation failed: corrupt nonce data'); + return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=connect_failed`); + } - // Consume the nonce -- one-time use only (if redis configured) - if (app.redis) { - await app.redis.del(`oauth:nonce:${decodedState.nonce}`); - } + if (typeof stored.userId !== 'string' || !stored.userId) { + app.log.warn({}, 'OAuth connect state validation failed: invalid stored nonce data'); + return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=connect_failed`); + } - const userId = decodedState.userId; + const userId = stored.userId; - // Exchange code for token + try { const tokenRes = await fetch(GITHUB_TOKEN_URL, { method: 'POST', headers: { @@ -120,8 +129,8 @@ export async function connectRoutes(app: FastifyInstance): Promise { Accept: 'application/json', }, body: JSON.stringify({ - client_id: process.env.GITHUB_CLIENT_ID, - client_secret: process.env.GITHUB_CLIENT_SECRET, + client_id: (process.env.GITHUB_CLIENT_ID || '').trim(), + client_secret: (process.env.GITHUB_CLIENT_SECRET || '').trim(), code, redirect_uri: `${process.env.BACKEND_URL}/api/connect/github/callback`, }), @@ -130,7 +139,7 @@ export async function connectRoutes(app: FastifyInstance): Promise { const tokenData = (await tokenRes.json()) as any; if (tokenData.error) { - app.log.error('GitHub connect token error:', tokenData); + app.log.error({ tokenError: tokenData.error }, 'GitHub connect token error'); return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?error=connect_failed`); } @@ -158,12 +167,6 @@ export async function connectRoutes(app: FastifyInstance): Promise { }, }); - // Redirect back to app settings - // If mobile, use custom scheme - if (decodedState.nonce.startsWith('mobile_')) { - return reply.redirect(`${process.env.MOBILE_REDIRECT_URI}?connected=github`); - } - return reply.redirect(`${process.env.PUBLIC_APP_URL}/settings?connected=github`); } catch (error) { @@ -294,18 +297,16 @@ export async function connectRoutes(app: FastifyInstance): Promise { }); } -function parseOAuthState(state: string): ParsedOAuthState | null { - try { - const decoded = JSON.parse(Buffer.from(state, 'base64').toString('utf-8')); - - // validating the OAuth state structure which is expected - if (typeof decoded.userId !== "string" || typeof decoded.nonce !== "string") { - return null; - } - return decoded; - } catch { +// State must be exactly 64 lowercase hex chars (32 random bytes). +// Returns null for any malformed input — caller must reject. +function parseNonce(state: string): string | null { + if (!state) { return null; } + if (!/^[0-9a-f]{64}$/.test(state)) { + return null; + } + return state; } function buildGitHubDiscoverySuggestions(user: { @@ -377,7 +378,3 @@ function parseBlogUrl(value: string): URL | null { } } } - -function generateState(): string { - return randomBytes(32).toString('hex'); -}