From f63c61db659f8b6392d6ed21c783ba4fca0ae18f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 31 Dec 2025 15:23:14 +0000 Subject: [PATCH] fix: Address code review feedback for OAuth SSO - Add proper lifecycle management for state cleanup interval - Fix host header injection vulnerability by validating forwarded headers - Add type safety for GitHub API responses - Add stopStateCleanup function for test cleanup - Document scaling limitations of in-memory state store Co-authored-by: samanhappy <2755122+samanhappy@users.noreply.github.com> --- src/controllers/oauthSsoController.ts | 40 ++++++++++++++---- src/services/oauthSsoService.ts | 56 +++++++++++++++++++++----- tests/services/oauthSsoService.test.ts | 6 +++ 3 files changed, 84 insertions(+), 18 deletions(-) diff --git a/src/controllers/oauthSsoController.ts b/src/controllers/oauthSsoController.ts index a417f6d..4cf381e 100644 --- a/src/controllers/oauthSsoController.ts +++ b/src/controllers/oauthSsoController.ts @@ -12,12 +12,40 @@ import { getPublicProviderInfo, isLocalAuthAllowed, isOAuthSsoEnabled, + getOAuthSsoConfig as getSsoConfigFromService, } from '../services/oauthSsoService.js'; import { JWT_SECRET } from '../config/jwt.js'; import config from '../config/index.js'; const TOKEN_EXPIRY = '24h'; +/** + * Get the base URL for OAuth callbacks + * Uses configured callbackBaseUrl if available, otherwise derives from request + * This approach is more secure than blindly trusting forwarded headers + */ +async function getCallbackBaseUrl(req: Request): Promise { + // First, check if a callback base URL is configured (most secure option) + const ssoConfig = await getSsoConfigFromService(); + if (ssoConfig?.callbackBaseUrl) { + return ssoConfig.callbackBaseUrl; + } + + // Fall back to deriving from request (less secure, but works in simpler setups) + // Only trust forwarded headers if app is configured to trust proxy + if (req.app.get('trust proxy') && req.headers['x-forwarded-proto'] && req.headers['x-forwarded-host']) { + const proto = Array.isArray(req.headers['x-forwarded-proto']) + ? req.headers['x-forwarded-proto'][0] + : req.headers['x-forwarded-proto']; + const host = Array.isArray(req.headers['x-forwarded-host']) + ? req.headers['x-forwarded-host'][0] + : req.headers['x-forwarded-host']; + return `${proto}://${host}`; + } + + return `${req.protocol}://${req.get('host')}`; +} + /** * Get OAuth SSO configuration for frontend * Returns enabled providers and whether local auth is allowed @@ -65,10 +93,9 @@ export const initiateOAuthLogin = async (req: Request, res: Response): Promise(); const STATE_TTL_MS = 10 * 60 * 1000; // 10 minutes // Cleanup old state entries periodically -setInterval(() => { - const now = Date.now(); - for (const [state, entry] of stateStore.entries()) { - if (now - entry.createdAt > STATE_TTL_MS) { - stateStore.delete(state); +let cleanupInterval: ReturnType | null = null; + +function startStateCleanup(): void { + if (cleanupInterval) return; + cleanupInterval = setInterval(() => { + const now = Date.now(); + for (const [state, entry] of stateStore.entries()) { + if (now - entry.createdAt > STATE_TTL_MS) { + stateStore.delete(state); + } } + }, 60 * 1000); // Cleanup every minute +} + +// Start cleanup on module load +startStateCleanup(); + +/** + * Stop the state cleanup interval (useful for tests and graceful shutdown) + */ +export function stopStateCleanup(): void { + if (cleanupInterval) { + clearInterval(cleanupInterval); + cleanupInterval = null; } -}, 60 * 1000); // Cleanup every minute +} + +// GitHub API response types for type safety +interface GitHubUserResponse { + id: number; + login: string; + name?: string; + email?: string; + avatar_url?: string; +} + +interface GitHubEmailResponse { + email: string; + primary: boolean; + verified: boolean; + visibility?: string; +} // Provider configurations cache const providerConfigsCache = new Map< @@ -326,7 +362,7 @@ async function getUserInfo( throw new Error(`Failed to fetch GitHub user info: ${response.statusText}`); } - const data = await response.json(); + const data = (await response.json()) as GitHubUserResponse; // Fetch email separately if not public let email = data.email; @@ -339,8 +375,8 @@ async function getUserInfo( }); if (emailResponse.ok) { - const emails = await emailResponse.json(); - const primaryEmail = emails.find((e: any) => e.primary); + const emails = (await emailResponse.json()) as GitHubEmailResponse[]; + const primaryEmail = emails.find((e) => e.primary); email = primaryEmail?.email || emails[0]?.email; } } diff --git a/tests/services/oauthSsoService.test.ts b/tests/services/oauthSsoService.test.ts index 7db846e..592d81f 100644 --- a/tests/services/oauthSsoService.test.ts +++ b/tests/services/oauthSsoService.test.ts @@ -24,6 +24,7 @@ import { isLocalAuthAllowed, getPublicProviderInfo, clearProviderCache, + stopStateCleanup, } from '../../src/services/oauthSsoService.js'; describe('OAuth SSO Service', () => { @@ -32,6 +33,11 @@ describe('OAuth SSO Service', () => { >; const mockGetUserDao = daoModule.getUserDao as jest.MockedFunction; + // Stop the cleanup interval to prevent Jest from hanging + afterAll(() => { + stopStateCleanup(); + }); + const defaultSsoConfig = { enabled: true, allowLocalAuth: true,