From 181cac4051f54e68b8c2b020b69d7490af670fd6 Mon Sep 17 00:00:00 2001 From: "yilkimezakka@gmail.com" Date: Mon, 28 Sep 2026 11:26:22 +0000 Subject: [PATCH] =?UTF-8?q?feat(auth):=20OAuth2=20PKCE=20flow=20for=20publ?= =?UTF-8?q?ic=20clients=20=E2=80=94=20Issue=20#806?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add PKCE (RFC 7636) support to oauth-service.ts: generateCodeVerifier() — 43-char URL-safe random verifier generateCodeChallenge(verifier, method) — S256/plain methods validateCodeChallenge(verifier, challenge, method) createPKCEAuthorizationUrl(provider, callbackUrl) — convenience fn - Extend createOAuthAuthorizationUrl with optional pkce param - Extend exchangeOAuthCode with optional codeVerifier validation - Add GET /:provider/pkce route returning { url, codeVerifier } - Update callback route to pass PKCE params through - 22 tests covering all PKCE paths (S256, plain, validation errors) Closes #806 --- backend/src/routes/oauth.ts | 40 ++- .../src/services/__tests__/oauth-pkce.test.ts | 242 ++++++++++++++++++ backend/src/services/oauth-service.ts | 188 +++++++++++++- 3 files changed, 456 insertions(+), 14 deletions(-) create mode 100644 backend/src/services/__tests__/oauth-pkce.test.ts diff --git a/backend/src/routes/oauth.ts b/backend/src/routes/oauth.ts index 965a5ff6..203f9b0b 100644 --- a/backend/src/routes/oauth.ts +++ b/backend/src/routes/oauth.ts @@ -5,6 +5,7 @@ import { consumeOAuthState, createOAuthAuthorizationUrl, createOAuthSessionToken, + createPKCEAuthorizationUrl, exchangeOAuthCode, } from '../services/oauth-service.js'; @@ -18,6 +19,10 @@ function callbackUrl(req: Request, provider: OAuthProvider): string { return `${req.protocol}://${req.get('host')}/api/v1/auth/oauth/${provider}/callback`; } +// --------------------------------------------------------------------------- +// Standard OAuth flow +// --------------------------------------------------------------------------- + oauthRouter.get('/:provider', (req: Request, res: Response) => { try { const provider = providerSchema.parse(req.params.provider); @@ -28,13 +33,44 @@ oauthRouter.get('/:provider', (req: Request, res: Response) => { } }); +// --------------------------------------------------------------------------- +// PKCE flow — GET /:provider/pkce +// Returns { url, codeVerifier } so the public client can initiate PKCE. +// --------------------------------------------------------------------------- + +oauthRouter.get('/:provider/pkce', (req: Request, res: Response) => { + try { + const provider = providerSchema.parse(req.params.provider); + const redirectTo = typeof req.query.redirectTo === 'string' ? req.query.redirectTo : undefined; + const { url, codeVerifier } = createPKCEAuthorizationUrl(provider, callbackUrl(req, provider), redirectTo); + res.json({ url, codeVerifier }); + } catch (error) { + res.status(400).json({ error: error instanceof Error ? error.message : 'Invalid OAuth PKCE request' }); + } +}); + +// --------------------------------------------------------------------------- +// Callback — handles both standard and PKCE flows +// --------------------------------------------------------------------------- + oauthRouter.get('/:provider/callback', async (req: Request, res: Response) => { try { const provider = providerSchema.parse(req.params.provider); const code = z.string().min(1).parse(req.query.code); const state = z.string().min(1).parse(req.query.state); - const { redirectTo } = consumeOAuthState(provider, state); - const profile = await exchangeOAuthCode(provider, code, callbackUrl(req, provider)); + + // codeVerifier may be passed as a query param from the client (PKCE flow) + const codeVerifier = + typeof req.query.code_verifier === 'string' ? req.query.code_verifier : undefined; + + const { redirectTo, codeChallenge, codeChallengeMethod } = consumeOAuthState(provider, state); + + const profile = await exchangeOAuthCode(provider, code, callbackUrl(req, provider), { + codeVerifier, + codeChallenge, + codeChallengeMethod, + }); + const token = createOAuthSessionToken(profile); const frontendCallback = new URL( diff --git a/backend/src/services/__tests__/oauth-pkce.test.ts b/backend/src/services/__tests__/oauth-pkce.test.ts new file mode 100644 index 00000000..aea3ef6a --- /dev/null +++ b/backend/src/services/__tests__/oauth-pkce.test.ts @@ -0,0 +1,242 @@ +/** + * OAuth2 PKCE flow tests (RFC 7636) + * + * Covers: + * - generateCodeVerifier + * - generateCodeChallenge (S256 and plain) + * - validateCodeChallenge + * - createPKCEAuthorizationUrl + * - exchangeOAuthCode PKCE validation + */ + +import { createHash } from 'node:crypto'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { + generateCodeVerifier, + generateCodeChallenge, + validateCodeChallenge, + createPKCEAuthorizationUrl, + createOAuthAuthorizationUrl, + exchangeOAuthCode, +} from '../oauth-service.js'; + +// --------------------------------------------------------------------------- +// Environment setup — provide minimal OAuth provider credentials so the +// functions that call `requireConfig` don't throw. +// --------------------------------------------------------------------------- +beforeEach(() => { + process.env.GOOGLE_OAUTH_CLIENT_ID = 'test-google-client-id'; + process.env.GOOGLE_OAUTH_CLIENT_SECRET = 'test-google-client-secret'; + process.env.GITHUB_OAUTH_CLIENT_ID = 'test-github-client-id'; + process.env.GITHUB_OAUTH_CLIENT_SECRET = 'test-github-client-secret'; +}); + +// --------------------------------------------------------------------------- +// generateCodeVerifier +// --------------------------------------------------------------------------- +describe('generateCodeVerifier', () => { + it('returns a string with length between 43 and 128 characters', () => { + const verifier = generateCodeVerifier(); + expect(typeof verifier).toBe('string'); + expect(verifier.length).toBeGreaterThanOrEqual(43); + expect(verifier.length).toBeLessThanOrEqual(128); + }); + + it('contains only URL-safe base64url characters (A-Z a-z 0-9 - _)', () => { + const verifier = generateCodeVerifier(); + expect(verifier).toMatch(/^[A-Za-z0-9\-_]+$/); + }); + + it('generates unique verifiers on each call', () => { + const v1 = generateCodeVerifier(); + const v2 = generateCodeVerifier(); + expect(v1).not.toBe(v2); + }); +}); + +// --------------------------------------------------------------------------- +// generateCodeChallenge +// --------------------------------------------------------------------------- +describe('generateCodeChallenge', () => { + const verifier = 'dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk'; + + describe('S256 method', () => { + it('returns the base64url-encoded SHA-256 hash of the verifier', () => { + const challenge = generateCodeChallenge(verifier, 'S256'); + const expected = createHash('sha256').update(verifier).digest('base64url'); + expect(challenge).toBe(expected); + }); + + it('contains only URL-safe characters', () => { + const challenge = generateCodeChallenge(verifier, 'S256'); + expect(challenge).toMatch(/^[A-Za-z0-9\-_]+$/); + }); + + it('does not contain base64 padding characters', () => { + const challenge = generateCodeChallenge(verifier, 'S256'); + expect(challenge).not.toContain('='); + expect(challenge).not.toContain('+'); + expect(challenge).not.toContain('/'); + }); + }); + + describe('plain method', () => { + it('returns the verifier unchanged', () => { + const challenge = generateCodeChallenge(verifier, 'plain'); + expect(challenge).toBe(verifier); + }); + }); +}); + +// --------------------------------------------------------------------------- +// validateCodeChallenge +// --------------------------------------------------------------------------- +describe('validateCodeChallenge', () => { + it('returns true when verifier matches challenge via S256', () => { + const verifier = generateCodeVerifier(); + const challenge = generateCodeChallenge(verifier, 'S256'); + expect(validateCodeChallenge(verifier, challenge, 'S256')).toBe(true); + }); + + it('returns false when a wrong verifier is supplied (S256)', () => { + const verifier = generateCodeVerifier(); + const challenge = generateCodeChallenge(verifier, 'S256'); + const wrongVerifier = generateCodeVerifier(); + expect(validateCodeChallenge(wrongVerifier, challenge, 'S256')).toBe(false); + }); + + it('returns true when verifier matches challenge via plain', () => { + const verifier = generateCodeVerifier(); + const challenge = generateCodeChallenge(verifier, 'plain'); + expect(validateCodeChallenge(verifier, challenge, 'plain')).toBe(true); + }); + + it('returns false for plain method when verifiers differ', () => { + const verifier = generateCodeVerifier(); + const challenge = generateCodeChallenge(verifier, 'plain'); + expect(validateCodeChallenge('wrong-verifier', challenge, 'plain')).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// createPKCEAuthorizationUrl +// --------------------------------------------------------------------------- +describe('createPKCEAuthorizationUrl', () => { + const CALLBACK = 'https://example.com/callback'; + + it('returns an object with url and codeVerifier', () => { + const result = createPKCEAuthorizationUrl('google', CALLBACK); + expect(result).toHaveProperty('url'); + expect(result).toHaveProperty('codeVerifier'); + expect(typeof result.url).toBe('string'); + expect(typeof result.codeVerifier).toBe('string'); + }); + + it('includes code_challenge in the authorization URL', () => { + const { url } = createPKCEAuthorizationUrl('google', CALLBACK); + const parsed = new URL(url); + expect(parsed.searchParams.has('code_challenge')).toBe(true); + expect(parsed.searchParams.get('code_challenge')).not.toBe(''); + }); + + it('includes code_challenge_method=S256 in the authorization URL', () => { + const { url } = createPKCEAuthorizationUrl('google', CALLBACK); + const parsed = new URL(url); + expect(parsed.searchParams.get('code_challenge_method')).toBe('S256'); + }); + + it('the code_challenge is the S256 hash of the returned codeVerifier', () => { + const { url, codeVerifier } = createPKCEAuthorizationUrl('google', CALLBACK); + const parsed = new URL(url); + const challenge = parsed.searchParams.get('code_challenge')!; + const expected = createHash('sha256').update(codeVerifier).digest('base64url'); + expect(challenge).toBe(expected); + }); + + it('codeVerifier is URL-safe with length 43-128', () => { + const { codeVerifier } = createPKCEAuthorizationUrl('google', CALLBACK); + expect(codeVerifier).toMatch(/^[A-Za-z0-9\-_]+$/); + expect(codeVerifier.length).toBeGreaterThanOrEqual(43); + expect(codeVerifier.length).toBeLessThanOrEqual(128); + }); + + it('works for github provider', () => { + const { url } = createPKCEAuthorizationUrl('github', CALLBACK); + expect(url).toContain('github.com'); + expect(url).toContain('code_challenge'); + }); +}); + +// --------------------------------------------------------------------------- +// createOAuthAuthorizationUrl with PKCE options (state store persistence) +// --------------------------------------------------------------------------- +describe('createOAuthAuthorizationUrl with PKCE options', () => { + const CALLBACK = 'https://example.com/callback'; + + it('embeds code_challenge and code_challenge_method when pkce is supplied', () => { + const verifier = generateCodeVerifier(); + const challenge = generateCodeChallenge(verifier, 'S256'); + const url = createOAuthAuthorizationUrl('google', CALLBACK, undefined, { + codeChallenge: challenge, + codeChallengeMethod: 'S256', + codeVerifier: verifier, + }); + const parsed = new URL(url); + expect(parsed.searchParams.get('code_challenge')).toBe(challenge); + expect(parsed.searchParams.get('code_challenge_method')).toBe('S256'); + }); + + it('does NOT add code_challenge when pkce is omitted', () => { + const url = createOAuthAuthorizationUrl('google', CALLBACK); + const parsed = new URL(url); + expect(parsed.searchParams.has('code_challenge')).toBe(false); + expect(parsed.searchParams.has('code_challenge_method')).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// exchangeOAuthCode PKCE validation +// --------------------------------------------------------------------------- +describe('exchangeOAuthCode PKCE validation', () => { + it('throws when codeChallenge was stored but codeVerifier is not supplied', async () => { + const verifier = generateCodeVerifier(); + const challenge = generateCodeChallenge(verifier, 'S256'); + + await expect( + exchangeOAuthCode('google', 'auth-code', 'https://example.com/cb', { + // codeVerifier intentionally omitted + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }), + ).rejects.toThrow('PKCE code_verifier is required'); + }); + + it('throws when the supplied codeVerifier does not match the stored challenge', async () => { + const verifier = generateCodeVerifier(); + const challenge = generateCodeChallenge(verifier, 'S256'); + const wrongVerifier = generateCodeVerifier(); + + await expect( + exchangeOAuthCode('google', 'auth-code', 'https://example.com/cb', { + codeVerifier: wrongVerifier, // intentionally wrong + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }), + ).rejects.toThrow('PKCE code_verifier does not match stored code_challenge'); + }); + + it('proceeds to the network call when verifier matches (fetch is not available in test env — expect network error, not PKCE error)', async () => { + const verifier = generateCodeVerifier(); + const challenge = generateCodeChallenge(verifier, 'S256'); + + // PKCE validation itself should pass; the rejection comes from the network + // (no real provider reachable in tests), not from our PKCE check. + const result = exchangeOAuthCode('google', 'auth-code', 'https://example.com/cb', { + codeVerifier: verifier, + codeChallenge: challenge, + codeChallengeMethod: 'S256', + }); + + await expect(result).rejects.not.toThrow('PKCE code_verifier does not match'); + }); +}); diff --git a/backend/src/services/oauth-service.ts b/backend/src/services/oauth-service.ts index b28ec8f1..3fd60a05 100644 --- a/backend/src/services/oauth-service.ts +++ b/backend/src/services/oauth-service.ts @@ -20,7 +20,15 @@ interface OAuthProviderConfig { scopes: string[]; } -const stateStore = new Map(); +interface StateEntry { + provider: OAuthProvider; + expiresAt: number; + redirectTo?: string; + codeChallenge?: string; + codeChallengeMethod?: 'S256' | 'plain'; +} + +const stateStore = new Map(); const STATE_TTL_MS = 10 * 60 * 1000; function getProviderConfig(provider: OAuthProvider): OAuthProviderConfig { @@ -54,10 +62,88 @@ function requireConfig(provider: OAuthProvider): OAuthProviderConfig { return config; } -export function createOAuthAuthorizationUrl(provider: OAuthProvider, callbackUrl: string, redirectTo?: string): string { +// --------------------------------------------------------------------------- +// PKCE utilities (RFC 7636) +// --------------------------------------------------------------------------- + +/** + * Generates a cryptographically secure code verifier (43-128 URL-safe chars). + */ +export function generateCodeVerifier(): string { + // 32 random bytes → 43-char base64url string (well within 43–128 range) + return randomBytes(32).toString('base64url'); +} + +/** + * Derives the code challenge from the verifier. + * - S256: BASE64URL(SHA256(ASCII(code_verifier))) + * - plain: code_verifier unchanged + */ +export function generateCodeChallenge(verifier: string, method: 'S256' | 'plain'): string { + if (method === 'plain') { + return verifier; + } + return createHash('sha256').update(verifier).digest('base64url'); +} + +/** + * Validates that the supplied verifier matches the stored challenge. + */ +export function validateCodeChallenge( + verifier: string, + challenge: string, + method: 'S256' | 'plain', +): boolean { + const expected = generateCodeChallenge(verifier, method); + return expected === challenge; +} + +// --------------------------------------------------------------------------- +// Authorization URL creation +// --------------------------------------------------------------------------- + +export interface PKCEOptions { + codeChallenge: string; + codeChallengeMethod: 'S256' | 'plain'; + codeVerifier: string; +} + +export interface CreateAuthUrlResult { + url: string; + codeVerifier?: string; +} + +/** + * Creates an OAuth authorization URL. + * When `pkce` is supplied the PKCE parameters are embedded in the URL and the + * challenge is persisted in the state store so it can be validated during the + * token exchange. + * + * Returns the URL string for backwards-compatibility. When PKCE is used the + * returned value is still a plain string – callers that need the codeVerifier + * should use `createPKCEAuthorizationUrl` instead. + */ +export function createOAuthAuthorizationUrl( + provider: OAuthProvider, + callbackUrl: string, + redirectTo?: string, + pkce?: PKCEOptions, +): string { const config = requireConfig(provider); const state = randomBytes(24).toString('base64url'); - stateStore.set(state, { provider, expiresAt: Date.now() + STATE_TTL_MS, redirectTo }); + + const entry: StateEntry = { + provider, + expiresAt: Date.now() + STATE_TTL_MS, + redirectTo, + }; + + if (pkce) { + entry.codeChallenge = pkce.codeChallenge; + entry.codeChallengeMethod = pkce.codeChallengeMethod; + } + + stateStore.set(state, entry); const url = new URL(config.authorizationUrl); url.searchParams.set('client_id', config.clientId!); @@ -65,14 +151,50 @@ export function createOAuthAuthorizationUrl(provider: OAuthProvider, callbackUrl url.searchParams.set('response_type', 'code'); url.searchParams.set('scope', config.scopes.join(' ')); url.searchParams.set('state', state); + if (provider === 'google') { url.searchParams.set('access_type', 'offline'); url.searchParams.set('prompt', 'select_account'); } + + if (pkce) { + url.searchParams.set('code_challenge', pkce.codeChallenge); + url.searchParams.set('code_challenge_method', pkce.codeChallengeMethod); + } + return url.toString(); } -export function consumeOAuthState(provider: OAuthProvider, state: string): { redirectTo?: string } { +/** + * Convenience function that generates a PKCE verifier + S256 challenge, + * builds the authorization URL, and returns both so the caller can pass the + * verifier to the token exchange step. + */ +export function createPKCEAuthorizationUrl( + provider: OAuthProvider, + callbackUrl: string, + redirectTo?: string, +): { url: string; codeVerifier: string } { + const codeVerifier = generateCodeVerifier(); + const codeChallenge = generateCodeChallenge(codeVerifier, 'S256'); + + const url = createOAuthAuthorizationUrl(provider, callbackUrl, redirectTo, { + codeChallenge, + codeChallengeMethod: 'S256', + codeVerifier, + }); + + return { url, codeVerifier }; +} + +// --------------------------------------------------------------------------- +// State consumption +// --------------------------------------------------------------------------- + +export function consumeOAuthState( + provider: OAuthProvider, + state: string, +): { redirectTo?: string; codeChallenge?: string; codeChallengeMethod?: 'S256' | 'plain' } { const stored = stateStore.get(state); stateStore.delete(state); @@ -80,28 +202,62 @@ export function consumeOAuthState(provider: OAuthProvider, state: string): { red throw new Error('Invalid or expired OAuth state'); } - return { redirectTo: stored.redirectTo }; + return { + redirectTo: stored.redirectTo, + codeChallenge: stored.codeChallenge, + codeChallengeMethod: stored.codeChallengeMethod, + }; } +// --------------------------------------------------------------------------- +// Token exchange +// --------------------------------------------------------------------------- + export async function exchangeOAuthCode( provider: OAuthProvider, code: string, callbackUrl: string, + options?: { + codeVerifier?: string; + codeChallenge?: string; + codeChallengeMethod?: 'S256' | 'plain'; + }, ): Promise { const config = requireConfig(provider); + + // PKCE validation: if a challenge was stored for this flow, the verifier + // must be supplied and must match. + if (options?.codeChallenge) { + if (!options.codeVerifier) { + throw new Error('PKCE code_verifier is required when a code_challenge was registered'); + } + const method = options.codeChallengeMethod ?? 'S256'; + const valid = validateCodeChallenge(options.codeVerifier, options.codeChallenge, method); + if (!valid) { + throw new Error('PKCE code_verifier does not match stored code_challenge'); + } + } + + const bodyParams: Record = { + client_id: config.clientId!, + client_secret: config.clientSecret!, + code, + redirect_uri: callbackUrl, + grant_type: 'authorization_code', + }; + + // Pass code_verifier to the provider if this is a PKCE flow. + if (options?.codeVerifier) { + bodyParams.code_verifier = options.codeVerifier; + } + const tokenResponse = await fetch(config.tokenUrl, { method: 'POST', headers: { accept: 'application/json', 'content-type': 'application/x-www-form-urlencoded', }, - body: new URLSearchParams({ - client_id: config.clientId!, - client_secret: config.clientSecret!, - code, - redirect_uri: callbackUrl, - grant_type: 'authorization_code', - }), + body: new URLSearchParams(bodyParams), }); if (!tokenResponse.ok) { @@ -118,6 +274,10 @@ export async function exchangeOAuthCode( : fetchGitHubProfile(config.profileUrl, config.emailUrl!, tokenBody.access_token); } +// --------------------------------------------------------------------------- +// Profile fetchers (internal) +// --------------------------------------------------------------------------- + async function fetchGoogleProfile(profileUrl: string, accessToken: string): Promise { const response = await fetch(profileUrl, { headers: { authorization: `Bearer ${accessToken}` } }); if (!response.ok) throw new Error(`Google profile request failed with ${response.status}`); @@ -160,6 +320,10 @@ async function fetchGitHubProfile( }; } +// --------------------------------------------------------------------------- +// Session token +// --------------------------------------------------------------------------- + export function createOAuthSessionToken(profile: OAuthUserProfile): string { const payload = JSON.stringify({ provider: profile.provider,