mirror of
https://github.com/tiennm99/ccs.git
synced 2026-10-12 20:15:40 +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:
1 parent
0557f93f2f
commit
472497fb03
3 files changed
+55
-10
No files matched your search
@@ -8,6 +8,9 @@
|
|||||||
import { EventEmitter } from 'events';
|
import { EventEmitter } from 'events';
|
||||||
import { ChildProcess } from 'child_process';
|
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 {
|
export interface ActiveAuthSession {
|
||||||
sessionId: string;
|
sessionId: string;
|
||||||
provider: string;
|
provider: string;
|
||||||
@@ -19,6 +22,31 @@ export const authSessionEvents = new EventEmitter();
|
|||||||
|
|
||||||
const activeSessions = new Map<string, ActiveAuthSession>();
|
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
|
* Register an active OAuth session
|
||||||
*/
|
*/
|
||||||
@@ -33,6 +61,8 @@ export function registerAuthSession(
|
|||||||
startedAt: Date.now(),
|
startedAt: Date.now(),
|
||||||
process,
|
process,
|
||||||
});
|
});
|
||||||
|
// H8: Start TTL cleanup when first session registered
|
||||||
|
startCleanupInterval();
|
||||||
authSessionEvents.emit('session:started', sessionId, provider);
|
authSessionEvents.emit('session:started', sessionId, provider);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -17,13 +17,18 @@ import {
|
|||||||
isProjectList,
|
isProjectList,
|
||||||
generateSessionId,
|
generateSessionId,
|
||||||
requestProjectSelection,
|
requestProjectSelection,
|
||||||
|
cancelProjectSelection,
|
||||||
type GCloudProject,
|
type GCloudProject,
|
||||||
type ProjectSelectionPrompt,
|
type ProjectSelectionPrompt,
|
||||||
} from '../project-selection-handler';
|
} from '../project-selection-handler';
|
||||||
import { ProviderOAuthConfig } from './auth-types';
|
import { ProviderOAuthConfig } from './auth-types';
|
||||||
import { getTimeoutTroubleshooting, showStep } from './environment-detector';
|
import { getTimeoutTroubleshooting, showStep } from './environment-detector';
|
||||||
import { isAuthenticated, registerAccountFromToken } from './token-manager';
|
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 { OAUTH_FLOW_TYPES } from '../../management';
|
||||||
import {
|
import {
|
||||||
registerAuthSession,
|
registerAuthSession,
|
||||||
@@ -150,7 +155,7 @@ async function handleStdout(
|
|||||||
provider: options.provider,
|
provider: options.provider,
|
||||||
userCode: state.userCode,
|
userCode: state.userCode,
|
||||||
verificationUrl,
|
verificationUrl,
|
||||||
expiresAt: Date.now() + 900000, // 15 minutes
|
expiresAt: Date.now() + DEVICE_CODE_TIMEOUT_MS,
|
||||||
};
|
};
|
||||||
deviceCodeEvents.emit('deviceCode:received', deviceCodePrompt);
|
deviceCodeEvents.emit('deviceCode:received', deviceCodePrompt);
|
||||||
|
|
||||||
@@ -311,8 +316,13 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
|
|||||||
env: { ...process.env, CLI_PROXY_AUTH_DIR: tokenDir },
|
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
|
// H5: Signal handling - properly kill child process on SIGINT/SIGTERM
|
||||||
|
// H8: Also clear stdinKeepalive interval to prevent memory leak
|
||||||
const cleanup = () => {
|
const cleanup = () => {
|
||||||
|
if (stdinKeepalive) clearInterval(stdinKeepalive);
|
||||||
if (authProcess && !authProcess.killed) {
|
if (authProcess && !authProcess.killed) {
|
||||||
authProcess.kill('SIGTERM');
|
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
|
// If the user completes browser auth after this timer fires but before the
|
||||||
// non-blocking check, the prompt blocks forever on stdin.
|
// non-blocking check, the prompt blocks forever on stdin.
|
||||||
// Workaround: Send newline every 16s to skip the manual prompt and continue polling.
|
// Workaround: Send newline every 16s to skip the manual prompt and continue polling.
|
||||||
let stdinKeepalive: ReturnType<typeof setInterval> | null = null;
|
|
||||||
if (!isDeviceCodeFlow && stdinMode === 'pipe') {
|
if (!isDeviceCodeFlow && stdinMode === 'pipe') {
|
||||||
stdinKeepalive = setInterval(() => {
|
stdinKeepalive = setInterval(() => {
|
||||||
if (authProcess.stdin && !authProcess.stdin.destroyed) {
|
if (authProcess.stdin && !authProcess.stdin.destroyed) {
|
||||||
@@ -415,6 +424,7 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
|
|||||||
process.removeListener('SIGTERM', cleanup);
|
process.removeListener('SIGTERM', cleanup);
|
||||||
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
||||||
unregisterAuthSession(state.sessionId);
|
unregisterAuthSession(state.sessionId);
|
||||||
|
cancelProjectSelection(state.sessionId);
|
||||||
authProcess.kill();
|
authProcess.kill();
|
||||||
console.log('');
|
console.log('');
|
||||||
console.log(fail(`OAuth timed out after ${headless ? 5 : 2} minutes`));
|
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);
|
process.removeListener('SIGTERM', cleanup);
|
||||||
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
||||||
unregisterAuthSession(state.sessionId);
|
unregisterAuthSession(state.sessionId);
|
||||||
|
cancelProjectSelection(state.sessionId);
|
||||||
const elapsed = ((Date.now() - startTime) / 1000).toFixed(1);
|
const elapsed = ((Date.now() - startTime) / 1000).toFixed(1);
|
||||||
|
|
||||||
if (code === 0) {
|
if (code === 0) {
|
||||||
@@ -488,6 +499,7 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise<Accou
|
|||||||
process.removeListener('SIGTERM', cleanup);
|
process.removeListener('SIGTERM', cleanup);
|
||||||
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
authSessionEvents.removeListener('session:cancelled', handleCancel);
|
||||||
unregisterAuthSession(state.sessionId);
|
unregisterAuthSession(state.sessionId);
|
||||||
|
cancelProjectSelection(state.sessionId);
|
||||||
console.log('');
|
console.log('');
|
||||||
console.log(fail(`Failed to start auth process: ${error.message}`));
|
console.log(fail(`Failed to start auth process: ${error.message}`));
|
||||||
resolve(null);
|
resolve(null);
|
||||||
|
|||||||
+10
-7
@@ -218,13 +218,16 @@ export async function testLocalhostBinding(port: number): Promise<BindingTestRes
|
|||||||
const server = net.createServer();
|
const server = net.createServer();
|
||||||
|
|
||||||
server.once('error', (err: NodeJS.ErrnoException) => {
|
server.once('error', (err: NodeJS.ErrnoException) => {
|
||||||
if (err.code === 'EADDRINUSE') {
|
// H8: Close server to prevent fd leak on error path
|
||||||
resolve({ success: false, message: `Port ${port} is already in use` });
|
server.close(() => {
|
||||||
} else if (err.code === 'EACCES') {
|
if (err.code === 'EADDRINUSE') {
|
||||||
resolve({ success: false, message: `Permission denied for port ${port}` });
|
resolve({ success: false, message: `Port ${port} is already in use` });
|
||||||
} else {
|
} else if (err.code === 'EACCES') {
|
||||||
resolve({ success: false, message: `Cannot bind to port ${port}: ${err.message}` });
|
resolve({ success: false, message: `Permission denied for port ${port}` });
|
||||||
}
|
} else {
|
||||||
|
resolve({ success: false, message: `Cannot bind to port ${port}: ${err.message}` });
|
||||||
|
}
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
server.once('listening', () => {
|
server.once('listening', () => {
|
||||||
|
|||||||
Reference in new issue
Block a user