From 0cda1be7cade4742f0c23694b8f3512b19b0c90e Mon Sep 17 00:00:00 2001 From: Olumide-01 Date: Sun, 30 Aug 2026 18:52:48 +0100 Subject: [PATCH] fix(security): validate GitHub OAuth state nonce to prevent login CSRF Generate the OAuth state value from the CSPRNG instead of Math.random() and validate it in constant time on the callback, so an attacker can no longer predict or reuse the nonce to mount a login CSRF attack. - security: add generateOAuthState/validateOAuthState helpers - github/oauth: generateState now uses the CSPRNG; add validateState - github callback: verify state via validateState before exchanging code - tests for the security helpers, GitHub OAuth utils and callback route Closes #1158 --- .../github/callback/__tests__/route.test.ts | 129 ++++++++++++++++++ src/app/api/auth/github/callback/route.ts | 11 +- src/lib/github/__tests__/oauth.test.ts | 108 +++++++++++++++ src/lib/github/oauth.ts | 15 +- src/middleware/__tests__/security.test.ts | 40 +++++- src/middleware/security.ts | 37 +++++ 6 files changed, 335 insertions(+), 5 deletions(-) create mode 100644 src/app/api/auth/github/callback/__tests__/route.test.ts create mode 100644 src/lib/github/__tests__/oauth.test.ts diff --git a/src/app/api/auth/github/callback/__tests__/route.test.ts b/src/app/api/auth/github/callback/__tests__/route.test.ts new file mode 100644 index 00000000..4441e487 --- /dev/null +++ b/src/app/api/auth/github/callback/__tests__/route.test.ts @@ -0,0 +1,129 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { GET } from '../route'; + +vi.mock('@/lib/ratelimit', () => ({ + withRateLimit: vi.fn(() => ({ + addHeaders: (response: Response) => response, + rateLimitResponse: null, + })), +})); + +vi.mock('@/lib/github/oauth', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + exchangeCodeForToken: vi.fn(), + getGitHubUser: vi.fn(), + getGitHubAvatarUrl: vi.fn(() => 'https://example.com/avatar.png'), + }; +}); + +vi.mock('@/../infra/edge-config', () => ({ + edgeLog: vi.fn(), +})); + +function createRequest(url: string) { + return { + nextUrl: new URL(url), + headers: new Headers(), + cookies: { + get: vi.fn((name: string) => { + if (name === 'github_oauth_state') return { value: 'test_state' }; + return undefined; + }), + delete: vi.fn(), + }, + } as any; +} + +const CALLBACK_URL = 'http://localhost:3000/api/auth/github/callback'; + +describe('GitHub OAuth Callback API Route', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + describe('GET /api/auth/github/callback', () => { + it('should handle a successful GitHub OAuth callback', async () => { + const { exchangeCodeForToken, getGitHubUser } = await import('@/lib/github/oauth'); + + (exchangeCodeForToken as any).mockResolvedValueOnce({ + access_token: 'test_access_token', + token_type: 'Bearer', + scope: 'read:user user:email', + }); + + (getGitHubUser as any).mockResolvedValueOnce({ + id: 123456789, + login: 'testuser', + name: 'Test User', + email: 'test@example.com', + }); + + const response = await GET(createRequest(`${CALLBACK_URL}?code=test_code&state=test_state`)); + + expect(response.status).toBe(200); + const json = await response.json(); + expect(json.message).toBe('GitHub authentication successful'); + expect(json.user).toBeTruthy(); + expect(json.user.email).toBe('test@example.com'); + expect(json.user.provider).toBe('github'); + expect(json.token).toBeTruthy(); + }); + + it('should reject a callback with a mismatched state parameter', async () => { + const response = await GET(createRequest(`${CALLBACK_URL}?code=test_code&state=wrong_state`)); + + expect(response.status).toBe(400); + const json = await response.json(); + expect(json.message).toBe('Invalid state parameter'); + }); + + it('should reject a callback with a missing state parameter', async () => { + const response = await GET(createRequest(`${CALLBACK_URL}?code=test_code`)); + + expect(response.status).toBe(400); + const json = await response.json(); + expect(json.message).toBe('Invalid state parameter'); + }); + + it('should handle an OAuth error from GitHub', async () => { + const response = await GET(createRequest(`${CALLBACK_URL}?error=access_denied`)); + + expect(response.status).toBe(400); + const json = await response.json(); + expect(json.message).toContain('GitHub OAuth error'); + }); + + it('should handle a missing authorization code', async () => { + const response = await GET(createRequest(`${CALLBACK_URL}?state=test_state`)); + + expect(response.status).toBe(400); + const json = await response.json(); + expect(json.message).toBe('Authorization code is required'); + }); + + it('should reject a GitHub account without a verified email', async () => { + const { exchangeCodeForToken, getGitHubUser } = await import('@/lib/github/oauth'); + + (exchangeCodeForToken as any).mockResolvedValueOnce({ + access_token: 'test_access_token', + token_type: 'Bearer', + scope: 'read:user user:email', + }); + + (getGitHubUser as any).mockResolvedValueOnce({ + id: 123456789, + login: 'testuser', + name: 'Test User', + email: null, + }); + + const response = await GET(createRequest(`${CALLBACK_URL}?code=test_code&state=test_state`)); + + expect(response.status).toBe(400); + const json = await response.json(); + expect(json.message).toBe('GitHub account must have a verified email'); + }); + }); +}); diff --git a/src/app/api/auth/github/callback/route.ts b/src/app/api/auth/github/callback/route.ts index 77fcf01e..26c16c68 100644 --- a/src/app/api/auth/github/callback/route.ts +++ b/src/app/api/auth/github/callback/route.ts @@ -1,6 +1,11 @@ import { NextRequest, NextResponse } from 'next/server'; import { withRateLimit } from '@/lib/ratelimit'; -import { exchangeCodeForToken, getGitHubUser, getGitHubAvatarUrl } from '@/lib/github/oauth'; +import { + exchangeCodeForToken, + getGitHubUser, + getGitHubAvatarUrl, + validateState, +} from '@/lib/github/oauth'; import type { AuthResponseDTO, AuthErrorDTO } from '@/types/api/auth.dto'; import { edgeLog } from '@/../infra/edge-config'; @@ -21,7 +26,7 @@ export async function GET( try { const searchParams = request.nextUrl.searchParams; const code = searchParams.get('code'); - const state = searchParams.get('state'); + const state = searchParams.get('state') ?? undefined; const error = searchParams.get('error'); // Check for OAuth errors @@ -40,7 +45,7 @@ export async function GET( // Verify state parameter to prevent CSRF attacks const storedState = request.cookies.get('github_oauth_state')?.value; - if (!state || state !== storedState) { + if (!validateState(state, storedState)) { edgeLog('error', '/api/auth/github/callback', 'Invalid state parameter'); return addHeaders( NextResponse.json({ message: 'Invalid state parameter' }, { status: 400 }), diff --git a/src/lib/github/__tests__/oauth.test.ts b/src/lib/github/__tests__/oauth.test.ts new file mode 100644 index 00000000..cc4e69a5 --- /dev/null +++ b/src/lib/github/__tests__/oauth.test.ts @@ -0,0 +1,108 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import { getGitHubAuthUrl, generateState, validateState, getGitHubAvatarUrl } from '../oauth'; + +const mockEnv = { + GITHUB_CLIENT_ID: 'test_client_id', + GITHUB_REDIRECT_URI: 'http://localhost:3000/api/auth/github/callback', +}; + +describe('GitHub OAuth Utilities', () => { + beforeEach(() => { + process.env.GITHUB_CLIENT_ID = mockEnv.GITHUB_CLIENT_ID; + process.env.GITHUB_REDIRECT_URI = mockEnv.GITHUB_REDIRECT_URI; + }); + + describe('generateState', () => { + it('should generate a random state string', () => { + const state1 = generateState(); + const state2 = generateState(); + + expect(state1).toBeTruthy(); + expect(state2).toBeTruthy(); + expect(state1).not.toBe(state2); + expect(state1.length).toBeGreaterThan(10); + }); + + it('should produce an unpredictable, hex-encoded nonce with 256 bits of entropy', () => { + expect(generateState()).toMatch(/^[0-9a-f]{64}$/); + }); + }); + + describe('validateState', () => { + it('should accept a state matching the stored value', () => { + expect(validateState('abc123', 'abc123')).toBe(true); + }); + + it('should reject a state that does not match the stored value', () => { + expect(validateState('wrong', 'right')).toBe(false); + }); + + it('should reject missing state or stored value', () => { + expect(validateState(undefined, 'stored')).toBe(false); + expect(validateState('present', undefined)).toBe(false); + expect(validateState(undefined, undefined)).toBe(false); + }); + }); + + describe('getGitHubAuthUrl', () => { + it('should generate correct GitHub authorization URL', () => { + const state = 'test_state_123'; + const url = getGitHubAuthUrl(state); + const params = new URL(url).searchParams; + + expect(url).toContain('https://github.com/login/oauth/authorize'); + expect(params.get('client_id')).toBe(mockEnv.GITHUB_CLIENT_ID); + expect(params.get('redirect_uri')).toBe(mockEnv.GITHUB_REDIRECT_URI); + expect(params.get('scope')).toBe('read:user user:email'); + expect(params.get('state')).toBe(state); + }); + + it('should throw error when GITHUB_CLIENT_ID is missing', () => { + process.env.GITHUB_CLIENT_ID = ''; + expect(() => getGitHubAuthUrl('test_state')).toThrow('GitHub OAuth configuration is missing'); + }); + + it('should throw error when GITHUB_REDIRECT_URI is missing', () => { + process.env.GITHUB_REDIRECT_URI = ''; + expect(() => getGitHubAuthUrl('test_state')).toThrow('GitHub OAuth configuration is missing'); + }); + }); + + describe('getGitHubAvatarUrl', () => { + it('should return the avatar URL when present', () => { + const user = { + id: 1, + login: 'testuser', + name: 'Test User', + email: 'test@example.com', + avatar_url: 'https://avatars.githubusercontent.com/u/1?v=4', + html_url: 'https://github.com/testuser', + bio: null, + location: null, + blog: null, + twitter_username: null, + company: null, + }; + + expect(getGitHubAvatarUrl(user)).toBe('https://avatars.githubusercontent.com/u/1?v=4'); + }); + + it('should return an empty string when the user has no avatar', () => { + const user = { + id: 1, + login: 'testuser', + name: 'Test User', + email: 'test@example.com', + avatar_url: '', + html_url: 'https://github.com/testuser', + bio: null, + location: null, + blog: null, + twitter_username: null, + company: null, + }; + + expect(getGitHubAvatarUrl(user)).toBe(''); + }); + }); +}); diff --git a/src/lib/github/oauth.ts b/src/lib/github/oauth.ts index 05de6569..505a8a47 100644 --- a/src/lib/github/oauth.ts +++ b/src/lib/github/oauth.ts @@ -3,6 +3,8 @@ * Handles GitHub OAuth2 flow for authentication */ +import { generateOAuthState, validateOAuthState } from '@/middleware/security'; + export interface GitHubUser { id: number; login: string; @@ -123,9 +125,20 @@ export async function getGitHubUser(accessToken: string): Promise { /** * Generate a random state parameter for OAuth + * + * The value is drawn from the CSPRNG so it cannot be predicted or replayed by + * an attacker, which is the guarantee the OAuth `state` parameter exists for. */ export function generateState(): string { - return Math.random().toString(36).substring(2, 15) + Math.random().toString(36).substring(2, 15); + return generateOAuthState(); +} + +/** + * Verify an OAuth `state` supplied on the callback against the value stored + * when the flow started. Guards the exchange against login CSRF. + */ +export function validateState(actual?: string, expected?: string): boolean { + return validateOAuthState(actual, expected); } /** diff --git a/src/middleware/__tests__/security.test.ts b/src/middleware/__tests__/security.test.ts index 04be0b5e..7b74b0b6 100644 --- a/src/middleware/__tests__/security.test.ts +++ b/src/middleware/__tests__/security.test.ts @@ -1,6 +1,11 @@ import { describe, expect, it } from 'vitest'; import type { NextRequest, NextResponse } from 'next/server'; -import { applySecurityHeaders, buildSecurityHeaders } from '../security'; +import { + applySecurityHeaders, + buildSecurityHeaders, + generateOAuthState, + validateOAuthState, +} from '../security'; import { CSP_NONCE_REQUEST_HEADER, CSP_NONCE_RESPONSE_HEADER, containsUnsafeSources } from '../csp'; function createRequest(options: { nonce?: string; protocol?: string } = {}): NextRequest { @@ -105,3 +110,36 @@ describe('applySecurityHeaders', () => { ); }); }); + +describe('generateOAuthState', () => { + it('returns distinct, high-entropy hex nonces', () => { + const state1 = generateOAuthState(); + const state2 = generateOAuthState(); + + expect(state1).toMatch(/^[0-9a-f]{64}$/); + expect(state2).toMatch(/^[0-9a-f]{64}$/); + expect(state1).not.toBe(state2); + }); + + it('derives entropy from the configured byte length', () => { + expect(generateOAuthState(16)).toMatch(/^[0-9a-f]{32}$/); + expect(generateOAuthState(1)).toMatch(/^[0-9a-f]{2}$/); + }); +}); + +describe('validateOAuthState', () => { + it('accepts a matching value', () => { + expect(validateOAuthState('deadbeef', 'deadbeef')).toBe(true); + }); + + it('rejects mismatched values', () => { + expect(validateOAuthState('deadbeef', 'deadbeec')).toBe(false); + expect(validateOAuthState('deadbeef', 'deadbeef1')).toBe(false); + }); + + it('rejects missing values', () => { + expect(validateOAuthState(undefined, 'deadbeef')).toBe(false); + expect(validateOAuthState('deadbeef', undefined)).toBe(false); + expect(validateOAuthState(undefined, undefined)).toBe(false); + }); +}); diff --git a/src/middleware/security.ts b/src/middleware/security.ts index c9261ebc..1a4149a7 100644 --- a/src/middleware/security.ts +++ b/src/middleware/security.ts @@ -85,3 +85,40 @@ export function applySecurityHeaders( return response; } + +/** Entropy (in bytes) for a generated OAuth state nonce — 256 bits. */ +const OAUTH_STATE_BYTES = 32; + +/** + * Generate a cryptographically random OAuth `state` nonce. + * + * The OAuth `state` parameter must be unpredictable. A guessable value lets an + * attacker prefix the flow with a state they control, then submit the victim's + * authorization code back to the callback as a login CSRF. `Math.random()` is + * seeded by time and is not cryptographically strong, so the value is drawn + * from the CSPRNG (`crypto.getRandomValues`) available on the edge runtime. + */ +export function generateOAuthState(byteLength: number = OAUTH_STATE_BYTES): string { + const bytes = new Uint8Array(byteLength); + crypto.getRandomValues(bytes); + + return Array.from(bytes, (byte) => byte.toString(16).padStart(2, '0')).join(''); +} + +/** + * Constant-time comparison of an OAuth `state` nonce against the stored value. + * The value supplied on the callback (`actual`) must match the value stored + * when the flow started (`expected`); any mismatch aborts the exchange. + */ +export function validateOAuthState(actual?: string, expected?: string): boolean { + if (!actual || !expected || actual.length !== expected.length) { + return false; + } + + let diff = 0; + for (let i = 0; i < expected.length; i += 1) { + diff |= actual.charCodeAt(i) ^ expected.charCodeAt(i); + } + + return diff === 0; +}