From f2a4200625e13754c7f79738dba0562e8ff27895 Mon Sep 17 00:00:00 2001 From: kaitranntt Date: Wed, 24 Dec 2025 20:18:57 -0500 Subject: [PATCH] fix(core): address all code review issues from PR #199 Critical fixes: - CRIT-1: Fix placeholder detection case sensitivity in api-key-validator - CRIT-2: Add TOCTOU protection via loadMigrationCheckData() for atomic config reads - CRIT-3: Restore fs.existsSync mock in profile-detector tests High priority fixes: - H1-H3: API validator HTTP/HTTPS support, timeout abort, debug log - H4-H6: OAuth explicit flow type, signal handling, TTY detection - H7-H8: Profile collision warning, config-first save order - H9-H11: expandPath consistency, preset sync CLI/UI --- src/cliproxy/auth/oauth-process.ts | 33 ++++++- src/cliproxy/config-generator.ts | 5 +- src/config/migration-manager.ts | 121 ++++++++++++++--------- src/utils/api-key-validator.ts | 33 +++++-- tests/unit/auth/profile-detector.test.ts | 3 +- tests/unit/utils/expand-path.test.ts | 35 +++++++ ui/src/lib/provider-presets.ts | 6 +- 7 files changed, 175 insertions(+), 61 deletions(-) diff --git a/src/cliproxy/auth/oauth-process.ts b/src/cliproxy/auth/oauth-process.ts index 60396c07..dbfa9ad4 100644 --- a/src/cliproxy/auth/oauth-process.ts +++ b/src/cliproxy/auth/oauth-process.ts @@ -23,6 +23,7 @@ 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 { OAUTH_FLOW_TYPES } from '../../management'; /** Options for OAuth process execution */ export interface OAuthProcessOptions { @@ -104,7 +105,9 @@ async function handleStdout( log(`stdout: ${output.trim()}`); state.accumulatedOutput += output; - const isDeviceCodeFlow = options.callbackPort === null; + // H4: Use explicit flow type from OAUTH_FLOW_TYPES instead of null port check + const flowType = OAUTH_FLOW_TYPES[options.provider] || 'authorization_code'; + const isDeviceCodeFlow = flowType === 'device_code'; // Parse project list when available if (isProjectList(state.accumulatedOutput) && state.parsedProjects.length === 0) { @@ -258,16 +261,29 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise((resolve) => { - // Device Code flows (Qwen, GHCP) may need interactive stdin for email/prompts + // H4: Use explicit flow type from OAUTH_FLOW_TYPES instead of null port check + const flowType = OAUTH_FLOW_TYPES[provider] || 'authorization_code'; + const isDeviceCodeFlow = flowType === 'device_code'; + + // H6: TTY detection - only inherit stdin if TTY available (prevents issues in CI/piped scripts) + // Device Code flows may need interactive stdin for email/prompts // Authorization Code flows need piped stdin for project selection - const isDeviceCodeFlow = callbackPort === null; - const stdinMode = isDeviceCodeFlow ? 'inherit' : 'pipe'; + const stdinMode = isDeviceCodeFlow && process.stdin.isTTY ? 'inherit' : 'pipe'; const authProcess = spawn(binaryPath, args, { stdio: [stdinMode, 'pipe', 'pipe'], env: { ...process.env, CLI_PROXY_AUTH_DIR: tokenDir }, }); + // H5: Signal handling - properly kill child process on SIGINT/SIGTERM + const cleanup = () => { + if (authProcess && !authProcess.killed) { + authProcess.kill('SIGTERM'); + } + }; + process.on('SIGINT', cleanup); + process.on('SIGTERM', cleanup); + const state: ProcessState = { stderrData: '', urlDisplayed: false, @@ -328,6 +344,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise { + // H5: Remove signal handlers before killing process + process.removeListener('SIGINT', cleanup); + process.removeListener('SIGTERM', cleanup); authProcess.kill(); console.log(''); console.log(fail(`OAuth timed out after ${headless ? 5 : 2} minutes`)); @@ -339,6 +358,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise { clearTimeout(timeout); + // H5: Remove signal handlers to prevent memory leaks + process.removeListener('SIGINT', cleanup); + process.removeListener('SIGTERM', cleanup); const elapsed = ((Date.now() - startTime) / 1000).toFixed(1); if (code === 0) { @@ -380,6 +402,9 @@ export function executeOAuthProcess(options: OAuthProcessOptions): Promise { clearTimeout(timeout); + // H5: Remove signal handlers to prevent memory leaks + process.removeListener('SIGINT', cleanup); + process.removeListener('SIGTERM', cleanup); console.log(''); console.log(fail(`Failed to start auth process: ${error.message}`)); resolve(null); diff --git a/src/cliproxy/config-generator.ts b/src/cliproxy/config-generator.ts index 7e9f940e..c9d44c94 100644 --- a/src/cliproxy/config-generator.ts +++ b/src/cliproxy/config-generator.ts @@ -11,6 +11,7 @@ import * as fs from 'fs'; import * as path from 'path'; import { getCcsDir } from '../utils/config-manager'; +import { expandPath } from '../utils/helpers'; import { warn } from '../utils/ui'; import { CLIProxyProvider, ProviderConfig, ProviderModelMapping } from './types'; import { getModelMappingFromConfig, getEnvVarsFromConfig } from './base-config-loader'; @@ -463,7 +464,7 @@ export function getEffectiveEnvVars( // Priority 1: Custom settings path (for user-defined variants) if (customSettingsPath) { - const expandedPath = customSettingsPath.replace(/^~/, require('os').homedir()); + const expandedPath = expandPath(customSettingsPath); if (fs.existsSync(expandedPath)) { try { const content = fs.readFileSync(expandedPath, 'utf-8'); @@ -569,7 +570,7 @@ export function getRemoteEnvVars( // Priority 1: Custom settings path (for user-defined variants) if (customSettingsPath) { - const expandedPath = customSettingsPath.replace(/^~/, require('os').homedir()); + const expandedPath = expandPath(customSettingsPath); if (fs.existsSync(expandedPath)) { try { const content = fs.readFileSync(expandedPath, 'utf-8'); diff --git a/src/config/migration-manager.ts b/src/config/migration-manager.ts index 9cc4cb43..9fa347d4 100644 --- a/src/config/migration-manager.ts +++ b/src/config/migration-manager.ts @@ -45,6 +45,43 @@ export function needsMigration(): boolean { return hasOldConfig && !hasNewConfig; } +/** + * Pre-loaded config data for TOCTOU-safe migration checks. + * Read once, use for both check and migration. + */ +export interface MigrationCheckData { + legacyConfig: Record | null; + unifiedConfig: ReturnType; + needsMigration: boolean; +} + +/** + * Load config files once for TOCTOU-safe migration operations. + * Returns data that can be passed to migration functions. + */ +export function loadMigrationCheckData(): MigrationCheckData { + const ccsDir = getCcsDir(); + const configJsonPath = path.join(ccsDir, 'config.json'); + + const legacyConfig = fs.existsSync(configJsonPath) ? readJsonSafe(configJsonPath) : null; + const unifiedConfig = loadUnifiedConfig(); + + // Determine if migration is needed + let needsMigration = false; + + if (legacyConfig?.profiles && typeof legacyConfig.profiles === 'object' && unifiedConfig) { + const legacyProfiles = legacyConfig.profiles as Record; + for (const profileName of Object.keys(legacyProfiles)) { + if (!unifiedConfig.profiles[profileName]) { + needsMigration = true; + break; + } + } + } + + return { legacyConfig, unifiedConfig, needsMigration }; +} + /** * Check if there are legacy profiles that haven't been migrated to config.yaml. * This catches the case where config.yaml exists but is empty/missing profiles @@ -52,38 +89,12 @@ export function needsMigration(): boolean { * * Used by autoMigrate() to trigger inline migration when needed. * (Fix for issue #195 - GLM auth persistence regression) + * + * Note: For TOCTOU-safe operations, use loadMigrationCheckData() instead. */ export function needsProfileMigration(): boolean { - const ccsDir = getCcsDir(); - const configJsonPath = path.join(ccsDir, 'config.json'); - - // No legacy config → nothing to migrate - if (!fs.existsSync(configJsonPath)) { - return false; - } - - const legacyConfig = readJsonSafe(configJsonPath); - const unifiedConfig = loadUnifiedConfig(); - - // No legacy profiles → nothing to migrate - if (!legacyConfig?.profiles || typeof legacyConfig.profiles !== 'object') { - return false; - } - - // No unified config → needs full migration (handled by needsMigration) - if (!unifiedConfig) { - return false; - } - - // Check if any legacy profile is missing from unified config - const legacyProfiles = legacyConfig.profiles as Record; - for (const profileName of Object.keys(legacyProfiles)) { - if (!unifiedConfig.profiles[profileName]) { - return true; // Found unmigrated profile - } - } - - return false; + const data = loadMigrationCheckData(); + return data.needsMigration; } /** @@ -209,7 +220,13 @@ export async function migrate(dryRun = false): Promise { } } - // 7. Migrate cache files + // 7. Write new config FIRST (before moving files - H8 fix) + // Note: Settings remain in *.settings.json files, config.yaml only stores references + if (!dryRun) { + saveUnifiedConfig(unifiedConfig); + } + + // 8. Migrate cache files AFTER config is saved (H8: prevents inconsistent state) if (!dryRun) { const cacheDir = path.join(ccsDir, 'cache'); if (!fs.existsSync(cacheDir)) { @@ -230,12 +247,6 @@ export async function migrate(dryRun = false): Promise { } } - // 8. Write new config (unless dry run) - // Note: Settings remain in *.settings.json files, config.yaml only stores references - if (!dryRun) { - saveUnifiedConfig(unifiedConfig); - } - return { success: true, backupPath: dryRun ? undefined : backupDir, @@ -398,12 +409,20 @@ export async function autoMigrate(): Promise { } // Check if inline profile migration is needed (config.yaml exists but missing profiles) - if (needsProfileMigration()) { - const result = await migrateProfilesToUnified(); + // CRIT-2: Load data once and pass to migrateProfilesToUnified to avoid TOCTOU race + const migrationData = loadMigrationCheckData(); + if (migrationData.needsMigration) { + const result = await migrateProfilesToUnified(migrationData); if (result.success && result.migratedFiles.length > 0) { console.log(''); console.log(infoBox('Migrated legacy profiles to config.yaml', 'SUCCESS')); console.log(` Profiles: ${result.migratedFiles.join(', ')}`); + // H7: Show collision warnings if any + if (result.warnings.length > 0) { + for (const warning of result.warnings) { + console.log(warn(warning)); + } + } console.log(''); } } @@ -412,15 +431,21 @@ export async function autoMigrate(): Promise { /** * Migrate only profiles from config.json to existing config.yaml. * Used when config.yaml exists but is missing profiles. + * + * @param preloadedData - Optional pre-loaded config data from loadMigrationCheckData() + * for TOCTOU-safe operations. If not provided, reads configs fresh. */ -async function migrateProfilesToUnified(): Promise { +async function migrateProfilesToUnified( + preloadedData?: MigrationCheckData +): Promise { const ccsDir = getCcsDir(); const migratedFiles: string[] = []; const warnings: string[] = []; try { - const oldConfig = readJsonSafe(path.join(ccsDir, 'config.json')); - const unifiedConfig = loadUnifiedConfig(); + // Use preloaded data if available (CRIT-2: TOCTOU fix) + const oldConfig = preloadedData?.legacyConfig ?? readJsonSafe(path.join(ccsDir, 'config.json')); + const unifiedConfig = preloadedData?.unifiedConfig ?? loadUnifiedConfig(); if (!oldConfig?.profiles || !unifiedConfig) { return { success: true, migratedFiles, warnings }; @@ -430,12 +455,20 @@ async function migrateProfilesToUnified(): Promise { // Migrate API profiles from config.json for (const [name, settingsPath] of Object.entries(oldConfig.profiles)) { - // Skip if already in unified config + const pathStr = settingsPath as string; + + // H7: Detect collision - profile exists in both configs if (unifiedConfig.profiles[name]) { + // Check if settings differ (potential data loss) + const existingSettings = unifiedConfig.profiles[name].settings; + if (existingSettings && existingSettings !== pathStr) { + warnings.push( + `Profile "${name}" exists in both configs with different settings - keeping existing (${existingSettings}), skipping legacy (${pathStr})` + ); + } continue; } - const pathStr = settingsPath as string; const expandedPath = expandPath(pathStr); // Verify settings file exists diff --git a/src/utils/api-key-validator.ts b/src/utils/api-key-validator.ts index f49060f2..cb732566 100644 --- a/src/utils/api-key-validator.ts +++ b/src/utils/api-key-validator.ts @@ -5,6 +5,7 @@ * Catches expired keys early with actionable error messages. */ +import * as http from 'http'; import * as https from 'https'; import { URL } from 'url'; @@ -19,7 +20,7 @@ const DEFAULT_PLACEHOLDERS = [ 'YOUR_GLM_API_KEY_HERE', 'YOUR_KIMI_API_KEY_HERE', 'YOUR_API_KEY_HERE', - 'your-api-key-here', + 'YOUR-API-KEY-HERE', 'PLACEHOLDER', '', ]; @@ -64,14 +65,14 @@ export async function validateGlmKey( } return new Promise((resolve) => { - const timeout = setTimeout(() => { - // Fail-open on timeout - let Claude CLI handle it - resolve({ valid: true }); - }, timeoutMs); + // Determine protocol - use http module for http:// URLs + const isHttps = url.protocol === 'https:'; + const httpModule = isHttps ? https : http; + const defaultPort = isHttps ? 443 : 80; const options: https.RequestOptions = { hostname: url.hostname, - port: url.port || 443, + port: url.port || defaultPort, path: url.pathname, method: 'GET', headers: { @@ -80,8 +81,8 @@ export async function validateGlmKey( }, }; - const req = https.request(options, (res) => { - clearTimeout(timeout); + const req = httpModule.request(options, (res) => { + clearTimeout(timeoutId); if (res.statusCode === 200) { resolve({ valid: true }); @@ -97,6 +98,12 @@ export async function validateGlmKey( }); } else { // Other errors (404, 500, etc.) - fail-open, let Claude CLI handle + // Debug log for diagnostics when CCS_DEBUG is set + if (process.env.CCS_DEBUG === '1') { + console.error( + `[CCS-Preflight] Unexpected status ${res.statusCode} from ${url.href} - fail-open` + ); + } resolve({ valid: true }); } @@ -105,11 +112,19 @@ export async function validateGlmKey( }); req.on('error', () => { - clearTimeout(timeout); + clearTimeout(timeoutId); // Network error - fail-open resolve({ valid: true }); }); + // Set timeout after request is created so we can destroy it on timeout + const timeoutId = setTimeout(() => { + // Abort request to prevent TCP connection leak + req.destroy(); + // Fail-open on timeout - let Claude CLI handle it + resolve({ valid: true }); + }, timeoutMs); + req.end(); }); } diff --git a/tests/unit/auth/profile-detector.test.ts b/tests/unit/auth/profile-detector.test.ts index fb29028d..52f2ae68 100644 --- a/tests/unit/auth/profile-detector.test.ts +++ b/tests/unit/auth/profile-detector.test.ts @@ -144,12 +144,13 @@ describe('ProfileDetector', () => { it('should return null for unknown profile (throws error)', () => { const isUnifiedModeSpy = spyOn(unifiedConfigLoader, 'isUnifiedMode').mockReturnValue(false); // Mock readConfig/readProfiles to return empty - spyOn(fs, 'existsSync').mockReturnValue(false); + const existsSyncSpy = spyOn(fs, 'existsSync').mockReturnValue(false); try { expect(() => detector.detectProfileType('unknown')).toThrow(/Profile not found/); } finally { isUnifiedModeSpy.mockRestore(); + existsSyncSpy.mockRestore(); } }); }); diff --git a/tests/unit/utils/expand-path.test.ts b/tests/unit/utils/expand-path.test.ts index 9764dca1..42dab0f5 100644 --- a/tests/unit/utils/expand-path.test.ts +++ b/tests/unit/utils/expand-path.test.ts @@ -66,4 +66,39 @@ describe("expandPath", () => { expect(expandPath("${UNDEFINED_VAR}/file.txt")).toBe(path.normalize("/file.txt")); expect(expandPath("$UNDEFINED_VAR/file.txt")).toBe(path.normalize("/file.txt")); }); + + test("11. Windows drive letters stay intact", () => { + // Windows drive letter paths should be preserved + const result = expandPath("C:\\Users\\test\\file.txt"); + expect(result).toContain("Users"); + expect(result).toContain("test"); + }); + + test("12. Windows UNC paths handled", () => { + // UNC paths start with \\ + const uncPath = "\\\\server\\share\\folder"; + const result = expandPath(uncPath); + // Should normalize but preserve the structure + expect(result).toContain("server"); + expect(result).toContain("share"); + }); + + test("13. Null-like input throws TypeError", () => { + // Function requires string input - documents current behavior + // @ts-ignore - testing runtime edge case + expect(() => expandPath(undefined as unknown as string)).toThrow(TypeError); + // @ts-ignore - testing runtime edge case + expect(() => expandPath(null as unknown as string)).toThrow(TypeError); + }); + + test("14. Path with spaces preserved", () => { + const pathWithSpaces = "~/My Documents/file.txt"; + const result = expandPath(pathWithSpaces); + expect(result).toContain("My Documents"); + }); + + test("15. Multiple consecutive slashes normalized", () => { + const result = expandPath("path//to///file.txt"); + expect(result).toBe(path.normalize("path/to/file.txt")); + }); }); diff --git a/ui/src/lib/provider-presets.ts b/ui/src/lib/provider-presets.ts index facac46d..638aca0b 100644 --- a/ui/src/lib/provider-presets.ts +++ b/ui/src/lib/provider-presets.ts @@ -19,6 +19,10 @@ export interface ProviderPreset { apiKeyPlaceholder: string; apiKeyHint?: string; category: PresetCategory; + /** Additional env vars for thinking mode, etc. */ + extraEnv?: Record; + /** Enable always thinking mode */ + alwaysThinkingEnabled?: boolean; } export const OPENROUTER_BASE_URL = 'https://openrouter.ai/api'; @@ -141,7 +145,7 @@ export function getPresetsByCategory(category: PresetCategory): ProviderPreset[] /** Get preset by ID */ export function getPresetById(id: string): ProviderPreset | undefined { - return PROVIDER_PRESETS.find((p) => p.id === id); + return PROVIDER_PRESETS.find((p) => p.id.toLowerCase() === id.toLowerCase()); } /** Check if a URL matches a known preset */