From a71496cc3d7db499780c2b257d74bb6cc101f450 Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 18 Feb 2026 04:18:39 +0700 Subject: [PATCH] fix(cliproxy): harden provider alias and refresh edge cases - guard provider alias generation against ambiguous collisions - add account-scoped Gemini token refresh with strict account checks - warn and fallback on invalid management remote ports - align remaining HTTP remote defaults and extend cliproxy tests --- src/cliproxy/auth/gemini-token-refresh.ts | 88 ++++++++++++------- .../auth/provider-refreshers/index.ts | 29 ++++-- src/cliproxy/management-api-client.ts | 23 ++++- src/cliproxy/provider-capabilities.ts | 39 ++++++-- src/commands/setup-command.ts | 2 +- src/config/unified-config-types.ts | 2 +- .../cliproxy/management-api-client.test.ts | 14 ++- .../cliproxy/provider-capabilities.test.ts | 22 ++++- 8 files changed, 167 insertions(+), 52 deletions(-) diff --git a/src/cliproxy/auth/gemini-token-refresh.ts b/src/cliproxy/auth/gemini-token-refresh.ts index 2117a3be..c08799b8 100644 --- a/src/cliproxy/auth/gemini-token-refresh.ts +++ b/src/cliproxy/auth/gemini-token-refresh.ts @@ -108,42 +108,53 @@ function isValidCliproxyToken(data: unknown): data is CliproxyGeminiToken { * Read Gemini token from CLIProxy auth directory * Returns credentials with source path, or null if no valid token found */ -function readCliproxyGeminiCreds(): GeminiCredsWithSource | null { +function readCliproxyGeminiCreds(accountId?: string): GeminiCredsWithSource | null { const authDir = getProviderAuthDir('gemini'); if (!fs.existsSync(authDir)) return null; - // Try to find default account's token file - const defaultAccount = getDefaultAccount('gemini'); let tokenPath: string | null = null; + const normalizedAccountId = accountId?.trim(); + const accounts = getProviderAccounts('gemini'); - if (defaultAccount) { - tokenPath = path.join(authDir, defaultAccount.tokenFile); - if (!fs.existsSync(tokenPath)) tokenPath = null; + // Account-specific refresh path (used by background worker) + if (normalizedAccountId) { + const targetAccount = accounts.find((account) => account.id === normalizedAccountId); + if (!targetAccount) { + return null; + } + + tokenPath = path.join(authDir, targetAccount.tokenFile); } - // Fallback: find any gemini token file by prefix or type - if (!tokenPath) { - const accounts = getProviderAccounts('gemini'); - if (accounts.length > 0) { + if (!normalizedAccountId) { + // Try to find default account's token file + const defaultAccount = getDefaultAccount('gemini'); + if (defaultAccount) { + tokenPath = path.join(authDir, defaultAccount.tokenFile); + if (!fs.existsSync(tokenPath)) tokenPath = null; + } + + // Fallback: find any gemini account token file + if (!tokenPath && accounts.length > 0) { tokenPath = path.join(authDir, accounts[0].tokenFile); if (!fs.existsSync(tokenPath)) tokenPath = null; } - } - // Last fallback: scan directory for gemini token files - if (!tokenPath) { - try { - const files = fs.readdirSync(authDir).filter((f) => f.endsWith('.json')); - for (const file of files) { - const filePath = path.join(authDir, file); - if (file.startsWith('gemini-') || isTokenFileForProvider(filePath, 'gemini')) { - tokenPath = filePath; - break; + // Last fallback: scan directory for gemini token files + if (!tokenPath) { + try { + const files = fs.readdirSync(authDir).filter((f) => f.endsWith('.json')); + for (const file of files) { + const filePath = path.join(authDir, file); + if (file.startsWith('gemini-') || isTokenFileForProvider(filePath, 'gemini')) { + tokenPath = filePath; + break; + } } + } catch { + // Directory read failed - continue to return null + return null; } - } catch { - // Directory read failed - continue to return null - return null; } } @@ -172,13 +183,19 @@ function readCliproxyGeminiCreds(): GeminiCredsWithSource | null { * Priority: CLIProxy auth dir first, then ~/.gemini/oauth_creds.json * Returns credentials with source path for correct write-back */ -function readGeminiCreds(): GeminiCredsWithSource | null { +function readGeminiCreds(accountId?: string): GeminiCredsWithSource | null { // 1. Try CLIProxy auth directory first (CCS-managed tokens) - const cliproxyResult = readCliproxyGeminiCreds(); + const cliproxyResult = readCliproxyGeminiCreds(accountId); if (cliproxyResult) { return cliproxyResult; } + // Account-scoped refresh is only supported for CLIProxy account files. + // Do not fall back to ~/.gemini for a specific accountId. + if (accountId?.trim()) { + return null; + } + // 2. Fall back to standard Gemini CLI location const oauthPath = getGeminiOAuthPath(); if (!fs.existsSync(oauthPath)) { @@ -249,8 +266,8 @@ function writeGeminiCreds(creds: GeminiOAuthCreds, sourcePath: string): string | /** * Check if Gemini token is expired or expiring soon */ -export function isGeminiTokenExpiringSoon(): boolean { - const result = readGeminiCreds(); +export function isGeminiTokenExpiringSoon(accountId?: string): boolean { + const result = readGeminiCreds(accountId); if (!result || !result.creds.access_token) { return true; // No token = needs auth } @@ -263,14 +280,15 @@ export function isGeminiTokenExpiringSoon(): boolean { /** * Refresh Gemini access token using refresh_token + * @param accountId Optional account ID for account-scoped refresh * @returns Result with success status, optional error, and expiry time */ -export async function refreshGeminiToken(): Promise<{ +export async function refreshGeminiToken(accountId?: string): Promise<{ success: boolean; error?: string; expiresAt?: number; }> { - const result = readGeminiCreds(); + const result = readGeminiCreds(accountId); if (!result || !result.creds.refresh_token) { return { success: false, error: 'No refresh token available' }; } @@ -334,19 +352,23 @@ export async function refreshGeminiToken(): Promise<{ /** * Ensure Gemini token is valid, refreshing if needed * @param verbose Log progress if true + * @param accountId Optional account ID for account-scoped refresh * @returns true if token is valid (or was refreshed), false if refresh failed */ -export async function ensureGeminiTokenValid(verbose = false): Promise<{ +export async function ensureGeminiTokenValid( + verbose = false, + accountId?: string +): Promise<{ valid: boolean; refreshed: boolean; error?: string; }> { - const result = readGeminiCreds(); + const result = readGeminiCreds(accountId); if (!result || !result.creds.access_token) { return { valid: false, refreshed: false, error: 'No Gemini credentials found' }; } - if (!isGeminiTokenExpiringSoon()) { + if (!isGeminiTokenExpiringSoon(accountId)) { return { valid: true, refreshed: false }; } @@ -355,7 +377,7 @@ export async function ensureGeminiTokenValid(verbose = false): Promise<{ console.log('[i] Gemini token expired or expiring soon, refreshing...'); } - const refreshResult = await refreshGeminiToken(); + const refreshResult = await refreshGeminiToken(accountId); if (refreshResult.success) { if (verbose) { console.log('[OK] Gemini token refreshed successfully'); diff --git a/src/cliproxy/auth/provider-refreshers/index.ts b/src/cliproxy/auth/provider-refreshers/index.ts index 07ed2bd2..207ac0fa 100644 --- a/src/cliproxy/auth/provider-refreshers/index.ts +++ b/src/cliproxy/auth/provider-refreshers/index.ts @@ -11,6 +11,7 @@ */ import { CLIProxyProvider } from '../../types'; +import { getProviderAccounts } from '../../account-manager'; import { getTokenRefreshOwnership, isRefreshDelegatedToCLIProxy, @@ -40,15 +41,33 @@ export function isRefreshDelegated(provider: CLIProxyProvider): boolean { /** * Refresh token for a specific provider and account * @param provider Provider to refresh - * @param _accountId Account ID (currently unused, multi-account not yet implemented) + * @param accountId Account ID used to refresh the correct provider token * @returns Refresh result with success status and optional error */ export async function refreshToken( provider: CLIProxyProvider, - _accountId: string + accountId: string ): Promise { + const normalizedAccountId = accountId.trim(); + if (!normalizedAccountId) { + return { + success: false, + error: 'Account ID is required for token refresh', + }; + } + + const hasAccount = getProviderAccounts(provider).some( + (account) => account.id === normalizedAccountId + ); + if (!hasAccount) { + return { + success: false, + error: `Account not found for ${provider}: ${normalizedAccountId}`, + }; + } + if (provider === 'gemini') { - return await refreshGeminiTokenWrapper(); + return await refreshGeminiTokenWrapper(normalizedAccountId); } const ownership = getTokenRefreshOwnership(provider); @@ -73,8 +92,8 @@ export async function refreshToken( * Wrapper for Gemini token refresh * Converts gemini-token-refresh.ts format to provider-refreshers format */ -async function refreshGeminiTokenWrapper(): Promise { - const result = await refreshGeminiToken(); +async function refreshGeminiTokenWrapper(accountId: string): Promise { + const result = await refreshGeminiToken(accountId); if (!result.success) { return { diff --git a/src/cliproxy/management-api-client.ts b/src/cliproxy/management-api-client.ts index f89713e1..f33edd21 100644 --- a/src/cliproxy/management-api-client.ts +++ b/src/cliproxy/management-api-client.ts @@ -24,14 +24,33 @@ const DEFAULT_TIMEOUT_MS = 5000; /** Default port for HTTPS protocol */ const DEFAULT_HTTPS_PORT = 443; +/** Avoid duplicate warnings for repeated invalid port inputs */ +const WARNED_INVALID_PORTS = new Set(); + +function isValidPort(port: number | undefined): port is number { + return port !== undefined && Number.isInteger(port) && port > 0 && port <= 65535; +} + /** * Get effective port based on config and protocol. */ function getEffectivePort(port: number | undefined, protocol: 'http' | 'https'): number { - if (port !== undefined && Number.isInteger(port) && port > 0 && port <= 65535) { + if (isValidPort(port)) { return port; } - return protocol === 'https' ? DEFAULT_HTTPS_PORT : CLIPROXY_DEFAULT_PORT; + + const fallbackPort = protocol === 'https' ? DEFAULT_HTTPS_PORT : CLIPROXY_DEFAULT_PORT; + if (port !== undefined) { + const warningKey = `${protocol}:${String(port)}`; + if (!WARNED_INVALID_PORTS.has(warningKey)) { + WARNED_INVALID_PORTS.add(warningKey); + console.warn( + `[management-api-client] Invalid port "${String(port)}", using default ${fallbackPort}` + ); + } + } + + return fallbackPort; } /** diff --git a/src/cliproxy/provider-capabilities.ts b/src/cliproxy/provider-capabilities.ts index 04049bcf..c582cfb7 100644 --- a/src/cliproxy/provider-capabilities.ts +++ b/src/cliproxy/provider-capabilities.ts @@ -154,16 +154,39 @@ export function buildProviderMap( const PROVIDER_ID_SET = new Set(CLIPROXY_PROVIDER_IDS); -const PROVIDER_ALIAS_MAP: ReadonlyMap = (() => { - const entries: Array<[string, CLIProxyProvider]> = []; - for (const provider of CLIPROXY_PROVIDER_IDS) { - entries.push([provider, provider]); - for (const alias of PROVIDER_CAPABILITIES[provider].aliases) { - entries.push([alias.toLowerCase(), provider]); +export function buildProviderAliasMap( + capabilities: Record = PROVIDER_CAPABILITIES +): ReadonlyMap { + const aliasMap = new Map(); + const providers = Object.keys(capabilities) as CLIProxyProvider[]; + + const registerAlias = (alias: string, provider: CLIProxyProvider): void => { + const normalized = alias.trim().toLowerCase(); + if (!normalized) { + return; + } + + const existingProvider = aliasMap.get(normalized); + if (existingProvider && existingProvider !== provider) { + throw new Error( + `Provider alias collision for "${normalized}": ${existingProvider} and ${provider}` + ); + } + + aliasMap.set(normalized, provider); + }; + + for (const provider of providers) { + registerAlias(provider, provider); + for (const alias of capabilities[provider].aliases) { + registerAlias(alias, provider); } } - return new Map(entries); -})(); + + return aliasMap; +} + +const PROVIDER_ALIAS_MAP: ReadonlyMap = buildProviderAliasMap(); export function isCLIProxyProvider(provider: string): provider is CLIProxyProvider { return PROVIDER_ID_SET.has(provider as CLIProxyProvider); diff --git a/src/commands/setup-command.ts b/src/commands/setup-command.ts index 24ea9235..5317774c 100644 --- a/src/commands/setup-command.ts +++ b/src/commands/setup-command.ts @@ -227,7 +227,7 @@ async function configureRemoteProxy(rl: readline.Interface): Promise<{ ])) as 'http' | 'https'; // Port (optional) - with validation - const defaultPort = protocol === 'https' ? '443' : '80'; + const defaultPort = protocol === 'https' ? '443' : String(CLIPROXY_DEFAULT_PORT); const portStr = await prompt(rl, `Port (leave empty for default ${defaultPort})`); let port: number | undefined; if (portStr) { diff --git a/src/config/unified-config-types.ts b/src/config/unified-config-types.ts index 5e5e0d2d..1b1d1d9c 100644 --- a/src/config/unified-config-types.ts +++ b/src/config/unified-config-types.ts @@ -328,7 +328,7 @@ export interface ProxyRemoteConfig { * Remote proxy port. * Optional - defaults based on protocol: * - HTTPS: 443 - * - HTTP: 80 + * - HTTP: 8317 * When empty/undefined, uses protocol default. */ port?: number; diff --git a/tests/unit/cliproxy/management-api-client.test.ts b/tests/unit/cliproxy/management-api-client.test.ts index e3b6b7b4..58edbdb2 100644 --- a/tests/unit/cliproxy/management-api-client.test.ts +++ b/tests/unit/cliproxy/management-api-client.test.ts @@ -1,7 +1,7 @@ /** * Unit tests for management-api-client module */ -import { describe, it, expect, beforeEach, mock } from 'bun:test'; +import { describe, it, expect, beforeEach, mock, spyOn } from 'bun:test'; import { ManagementApiClient } from '../../../src/cliproxy/management-api-client'; import type { ManagementClientConfig, @@ -76,6 +76,18 @@ describe('management-api-client', () => { const client = new ManagementApiClient(configNoPort); expect(client.getBaseUrl()).toBe('https://localhost'); }); + + it('should warn and fall back when configured port is invalid', () => { + const warnSpy = spyOn(console, 'warn').mockImplementation(() => {}); + + const client = new ManagementApiClient({ ...config, port: 99999 }); + expect(client.getBaseUrl()).toBe('http://localhost:8317'); + expect(warnSpy).toHaveBeenCalledWith( + '[management-api-client] Invalid port "99999", using default 8317' + ); + + warnSpy.mockRestore(); + }); }); describe('error code mapping', () => { diff --git a/tests/unit/cliproxy/provider-capabilities.test.ts b/tests/unit/cliproxy/provider-capabilities.test.ts index 13f3e8f7..bbc8ae8b 100644 --- a/tests/unit/cliproxy/provider-capabilities.test.ts +++ b/tests/unit/cliproxy/provider-capabilities.test.ts @@ -1,8 +1,10 @@ import { describe, expect, it } from 'bun:test'; import { + buildProviderAliasMap, CLIPROXY_PROVIDER_IDS, getOAuthCallbackPort, getOAuthFlowType, + PROVIDER_CAPABILITIES, getProviderDisplayName, getProvidersByOAuthFlow, isCLIProxyProvider, @@ -68,7 +70,25 @@ describe('provider-capabilities', () => { expect(getOAuthCallbackPort('qwen')).toBeNull(); expect(getOAuthCallbackPort('kiro')).toBeNull(); expect(getOAuthCallbackPort('gemini')).toBe(8085); - expect(getProviderDisplayName('agy')).toBe('AntiGravity'); + expect(getProviderDisplayName('agy')).toBe('Antigravity'); + }); + + it('throws when provider aliases collide across providers', () => { + const capabilitiesWithCollision = { + ...PROVIDER_CAPABILITIES, + gemini: { + ...PROVIDER_CAPABILITIES.gemini, + aliases: ['shared-alias'], + }, + codex: { + ...PROVIDER_CAPABILITIES.codex, + aliases: ['shared-alias'], + }, + }; + + expect(() => + buildProviderAliasMap(capabilitiesWithCollision as typeof PROVIDER_CAPABILITIES) + ).toThrow(/shared-alias/i); }); it('keeps diagnostics flow metadata in sync with provider capabilities', () => {