mirror of
https://github.com/tiennm99/ccs.git
synced 2026-07-16 08:17:11 +00:00
fix(oauth): harden cleanup for edge cases in auth process
- Clear stdinKeepalive interval on SIGINT/SIGTERM signal handlers - Add cancelProjectSelection() to timeout/exit/error handlers - Close server on error path in testLocalhostBinding to prevent fd leak - Add TTL-based cleanup for stale auth sessions (10 min expiry) - Use DEVICE_CODE_TIMEOUT_MS constant instead of hardcoded value (DRY)
This commit is contained in:
@@ -8,6 +8,9 @@
|
||||
import { EventEmitter } from 'events';
|
||||
import { ChildProcess } from 'child_process';
|
||||
|
||||
// H8: TTL for stale session cleanup (10 minutes - generous for OAuth flows)
|
||||
const SESSION_TTL_MS = 10 * 60 * 1000;
|
||||
|
||||
export interface ActiveAuthSession {
|
||||
sessionId: string;
|
||||
provider: string;
|
||||
@@ -19,6 +22,31 @@ export const authSessionEvents = new EventEmitter();
|
||||
|
||||
const activeSessions = new Map<string, ActiveAuthSession>();
|
||||
|
||||
// H8: Periodic cleanup of stale sessions (prevents memory leak from orphaned sessions)
|
||||
let cleanupInterval: ReturnType<typeof setInterval> | null = null;
|
||||
|
||||
function startCleanupInterval(): void {
|
||||
if (cleanupInterval) return;
|
||||
cleanupInterval = setInterval(() => {
|
||||
const now = Date.now();
|
||||
for (const [sessionId, session] of activeSessions.entries()) {
|
||||
if (now - session.startedAt > SESSION_TTL_MS) {
|
||||
// Stale session - kill process if still running, then remove
|
||||
if (session.process && !session.process.killed) {
|
||||
session.process.kill('SIGTERM');
|
||||
}
|
||||
activeSessions.delete(sessionId);
|
||||
authSessionEvents.emit('session:expired', sessionId);
|
||||
}
|
||||
}
|
||||
// Stop interval if no active sessions
|
||||
if (activeSessions.size === 0 && cleanupInterval) {
|
||||
clearInterval(cleanupInterval);
|
||||
cleanupInterval = null;
|
||||
}
|
||||
}, 60000); // Check every minute
|
||||
}
|
||||
|
||||
/**
|
||||
* Register an active OAuth session
|
||||
*/
|
||||
@@ -33,6 +61,8 @@ export function registerAuthSession(
|
||||
startedAt: Date.now(),
|
||||
process,
|
||||
});
|
||||
// H8: Start TTL cleanup when first session registered
|
||||
startCleanupInterval();
|
||||
authSessionEvents.emit('session:started', sessionId, provider);
|
||||
}
|
||||
|
||||
|
||||
@@ -17,13 +17,18 @@ import {
|
||||
isProjectList,
|
||||
generateSessionId,
|
||||
requestProjectSelection,
|
||||
cancelProjectSelection,
|
||||
type GCloudProject,
|
||||
type ProjectSelectionPrompt,
|
||||
} from '../project-selection-handler';
|
||||
import { ProviderOAuthConfig } from './auth-types';
|
||||
import { getTimeoutTroubleshooting, showStep } from './environment-detector';
|
||||
import { isAuthenticated, registerAccountFromToken } from './token-manager';
|
||||
import { deviceCodeEvents, type DeviceCodePrompt } from '../device-code-handler';
|
||||
import {
|
||||
deviceCodeEvents,
|
||||
DEVICE_CODE_TIMEOUT_MS,
|
||||
type DeviceCodePrompt,
|
||||
} from '../device-code-handler';
|
||||
import { OAUTH_FLOW_TYPES } from '../../management';
|
||||
import {
|
||||
registerAuthSession,
|
||||
@@ -150,7 +155,7 @@ async function handleStdout(
|
||||
provider: options.provider,
|
||||
userCode: state.userCode,
|
||||
verificationUrl,
|
||||
expiresAt: Date.now() + 900000, // 15 minutes
|
||||
expiresAt: Date.now() + DEVICE_CODE_TIMEOUT_MS,
|
||||
};
|
||||
deviceCodeEvents.emit('deviceCode:received', deviceCodePrompt);
|
||||
|
||||
@@ -311,8 +316,13 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
|
||||
env: { ...process.env, CLI_PROXY_AUTH_DIR: tokenDir },
|
||||
});
|
||||
|
||||
// H7: Mutable ref for stdin keepalive interval (set later, needed in cleanup)
|
||||
let stdinKeepalive: ReturnType<typeof setInterval> | null = null;
|
||||
|
||||
// H5: Signal handling - properly kill child process on SIGINT/SIGTERM
|
||||
// H8: Also clear stdinKeepalive interval to prevent memory leak
|
||||
const cleanup = () => {
|
||||
if (stdinKeepalive) clearInterval(stdinKeepalive);
|
||||
if (authProcess && !authProcess.killed) {
|
||||
authProcess.kill('SIGTERM');
|
||||
}
|
||||
@@ -352,7 +362,6 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
|
||||
// If the user completes browser auth after this timer fires but before the
|
||||
// non-blocking check, the prompt blocks forever on stdin.
|
||||
// Workaround: Send newline every 16s to skip the manual prompt and continue polling.
|
||||
let stdinKeepalive: ReturnType<typeof setInterval> | null = null;
|
||||
if (!isDeviceCodeFlow && stdinMode === 'pipe') {
|
||||
stdinKeepalive = setInterval(() => {
|
||||
if (authProcess.stdin && !authProcess.stdin.destroyed) {
|
||||
@@ -415,6 +424,7 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
|
||||
process.removeListener('SIGTERM', cleanup);
|
||||
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
||||
unregisterAuthSession(state.sessionId);
|
||||
cancelProjectSelection(state.sessionId);
|
||||
authProcess.kill();
|
||||
console.log('');
|
||||
console.log(fail(`OAuth timed out after ${headless ? 5 : 2} minutes`));
|
||||
@@ -433,6 +443,7 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
|
||||
process.removeListener('SIGTERM', cleanup);
|
||||
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
||||
unregisterAuthSession(state.sessionId);
|
||||
cancelProjectSelection(state.sessionId);
|
||||
const elapsed = ((Date.now() - startTime) / 1000).toFixed(1);
|
||||
|
||||
if (code === 0) {
|
||||
@@ -488,6 +499,7 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
|
||||
process.removeListener('SIGTERM', cleanup);
|
||||
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
||||
unregisterAuthSession(state.sessionId);
|
||||
cancelProjectSelection(state.sessionId);
|
||||
console.log('');
|
||||
console.log(fail(`Failed to start auth process: ${error.message}`));
|
||||
resolve(null);
|
||||
|
||||
+10
-7
@@ -218,13 +218,16 @@ export async function testLocalhostBinding(port: number): Promise<BindingTestRes
|
||||
const server = net.createServer();
|
||||
|
||||
server.once('error', (err: NodeJS.ErrnoException) => {
|
||||
if (err.code === 'EADDRINUSE') {
|
||||
resolve({ success: false, message: `Port ${port} is already in use` });
|
||||
} else if (err.code === 'EACCES') {
|
||||
resolve({ success: false, message: `Permission denied for port ${port}` });
|
||||
} else {
|
||||
resolve({ success: false, message: `Cannot bind to port ${port}: ${err.message}` });
|
||||
}
|
||||
// H8: Close server to prevent fd leak on error path
|
||||
server.close(() => {
|
||||
if (err.code === 'EADDRINUSE') {
|
||||
resolve({ success: false, message: `Port ${port} is already in use` });
|
||||
} else if (err.code === 'EACCES') {
|
||||
resolve({ success: false, message: `Permission denied for port ${port}` });
|
||||
} else {
|
||||
resolve({ success: false, message: `Cannot bind to port ${port}: ${err.message}` });
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
server.once('listening', () => {
|
||||
|
||||
Reference in New Issue
Block a user