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

@@ -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<string> {
+  // 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<v
     }
 
     // Build callback URL
-    const baseUrl =
-      req.headers['x-forwarded-proto'] && req.headers['x-forwarded-host']
-        ? `${req.headers['x-forwarded-proto']}://${req.headers['x-forwarded-host']}`
-        : `${req.protocol}://${req.get('host')}`;
+    // Note: Use configured callback base URL from oauthSso config if available
+    // This avoids relying on potentially untrusted forwarded headers
+    const baseUrl = await getCallbackBaseUrl(req);
 
     const callbackUrl = `${baseUrl}${config.basePath}/api/auth/sso/${providerId}/callback`;
 
@@ -121,10 +148,7 @@ export const handleOAuthCallback = async (req: Request, res: Response): Promise<
     }
 
     // Build callback URL (same as used in initiate)
-    const baseUrl =
-      req.headers['x-forwarded-proto'] && req.headers['x-forwarded-host']
-        ? `${req.headers['x-forwarded-proto']}://${req.headers['x-forwarded-host']}`
-        : `${req.protocol}://${req.get('host')}`;
+    const baseUrl = await getCallbackBaseUrl(req);
 
     const callbackUrl = `${baseUrl}${config.basePath}/api/auth/sso/${providerId}/callback`;
 

src/services/oauthSsoService.ts

@@ -11,7 +11,9 @@ import { getSystemConfigDao, getUserDao } from '../dao/index.js';
 import { IUser, OAuthSsoProviderConfig, OAuthSsoConfig } from '../types/index.js';
 
 // In-memory store for OAuth state (code verifier, state, etc.)
-// In production, consider using Redis or database for multi-instance deployments
+// NOTE: This implementation uses in-memory storage which is suitable for single-instance deployments.
+// For multi-instance/scaled deployments, implement Redis or database-backed state storage
+// to ensure OAuth callbacks reach the correct instance where the state was stored.
 interface OAuthStateEntry {
   codeVerifier: string;
   providerId: string;
@@ -23,14 +25,48 @@ const stateStore = new Map<string, OAuthStateEntry>();
 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<typeof setInterval> | 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;
       }
     }

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<typeof daoModule.getUserDao>;
 
+  // Stop the cleanup interval to prevent Jest from hanging
+  afterAll(() => {
+    stopStateCleanup();
+  });
+
   const defaultSsoConfig = {
     enabled: true,
     allowLocalAuth: true,