From 8114ff016e5c29c869692a9812b3187f2842ccae Mon Sep 17 00:00:00 2001 From: "Kai (Tam Nhu) Tran" <61256810+kaitranntt@users.noreply.github.com> Date: Sat, 30 May 2026 13:21:01 -0400 Subject: [PATCH] fix: validate browser flags before proxy side effects --- .../__tests__/executor-option-value.test.ts | 58 +++++++++++++++++++ src/cliproxy/executor/browser-launch-setup.ts | 5 +- src/cliproxy/executor/index.ts | 20 +++++-- src/cliproxy/executor/proxy-resolver.ts | 38 ++++++++---- src/cliproxy/quota/quota-manager.ts | 10 ++-- src/management/checks/image-analysis-check.ts | 5 +- 6 files changed, 109 insertions(+), 27 deletions(-) diff --git a/src/cliproxy/executor/__tests__/executor-option-value.test.ts b/src/cliproxy/executor/__tests__/executor-option-value.test.ts index 9315f3f7..52323e9c 100644 --- a/src/cliproxy/executor/__tests__/executor-option-value.test.ts +++ b/src/cliproxy/executor/__tests__/executor-option-value.test.ts @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, expect, it, jest } from 'bun:test'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; +import * as http from 'http'; import { execClaudeWithCLIProxy, hasGitLabTokenLoginFlag, readOptionValue } from '../index'; describe('readOptionValue', () => { @@ -57,6 +58,7 @@ describe('execClaudeWithCLIProxy browser flag validation', () => { fs.writeFileSync(fakeClaudePath, '#!/bin/sh\nexit 0\n', { mode: 0o755 }); fs.chmodSync(fakeClaudePath, 0o755); originalCcsHome = process.env.CCS_HOME; + process.exitCode = 0; process.env.CCS_HOME = tmpHome; }); @@ -66,9 +68,65 @@ describe('execClaudeWithCLIProxy browser flag validation', () => { } else { delete process.env.CCS_HOME; } + process.exitCode = 0; fs.rmSync(tmpHome, { recursive: true, force: true }); }); + it('validates conflicting browser launch flags before remote proxy checks', async () => { + let requestCount = 0; + const server = http.createServer((_req, res) => { + requestCount += 1; + res.writeHead(200, { 'content-type': 'application/json' }); + res.end('{"ok":true}'); + }); + + await new Promise((resolve) => { + server.listen(0, '127.0.0.1', resolve); + }); + + const address = server.address(); + if (!address || typeof address === 'string') { + server.close(); + throw new Error('Test server did not bind to a TCP port'); + } + + const exitSpy = jest + .spyOn(process, 'exit') + .mockImplementation((() => undefined as never) as typeof process.exit); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + + try { + await execClaudeWithCLIProxy( + fakeClaudePath, + 'gemini', + [ + '--proxy-host', + '127.0.0.1', + '--proxy-port', + String(address.port), + '--proxy-auth-token', + 'SECRET_TOKEN_FOR_VALIDATION', + '--remote-only', + '--browser', + '--no-browser', + ], + {} + ); + + expect(exitSpy).toHaveBeenCalledWith(1); + expect(errorSpy).toHaveBeenCalledWith( + '[X] Use either `--browser` or `--no-browser`, not both.' + ); + expect(requestCount).toBe(0); + } finally { + exitSpy.mockRestore(); + errorSpy.mockRestore(); + await new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())); + }); + } + }); + it('exits cleanly when conflicting browser launch flags are provided', async () => { const exitSpy = jest .spyOn(process, 'exit') diff --git a/src/cliproxy/executor/browser-launch-setup.ts b/src/cliproxy/executor/browser-launch-setup.ts index 94405a33..89a8d0ef 100644 --- a/src/cliproxy/executor/browser-launch-setup.ts +++ b/src/cliproxy/executor/browser-launch-setup.ts @@ -9,7 +9,7 @@ * 4. Browser MCP ensure + sync-to-config-dir */ -import { warn } from '../../utils/ui'; +import { fail, warn } from '../../utils/ui'; import { type BrowserLaunchOverride, ensureBrowserMcpOrThrow, @@ -49,7 +49,8 @@ export function resolveBrowserLaunchFlags(argsWithoutProxy: string[]): { browserLaunchOverride = browserLaunchFlags.override; argsWithoutBrowserFlags = browserLaunchFlags.argsWithoutFlags; } catch (error) { - console.error(warn((error as Error).message)); + console.error(fail((error as Error).message)); + process.exitCode = 1; process.exit(1); return { browserLaunchOverride: undefined, argsWithoutBrowserFlags }; } diff --git a/src/cliproxy/executor/index.ts b/src/cliproxy/executor/index.ts index d88ff83e..f72408be 100644 --- a/src/cliproxy/executor/index.ts +++ b/src/cliproxy/executor/index.ts @@ -63,7 +63,7 @@ import { } from './thinking-override-resolver'; import { shouldStartHttpsTunnel } from './https-tunnel-policy'; import { filterCcsFlags, parseExecutorFlags, validateFlagCombinations } from './arg-parser'; -import { resolveExecutorProxy } from './proxy-resolver'; +import { resolveExecutorProxy, resolveExecutorProxyConfig } from './proxy-resolver'; import { buildProxyChain } from './proxy-chain-builder'; import { warnBrokenModels } from './model-warnings'; import { launchClaude } from './claude-launcher'; @@ -129,8 +129,21 @@ export async function execClaudeWithCLIProxy( // Collect all providers to validate (default + composite tiers) const allProviders = [provider, ...compositeProviders]; + const proxyResolution = resolveExecutorProxyConfig(args, { + unifiedConfig, + allProviders, + verbose, + cfg, + log, + }); + + const { browserLaunchOverride, argsWithoutBrowserFlags } = resolveBrowserLaunchFlags( + proxyResolution.argsWithoutProxy + ); + if (process.exitCode === 1) return; + const { proxyConfig, useRemoteProxy, localBackend, binaryPath, argsWithoutProxy } = - await resolveExecutorProxy(args, { + await resolveExecutorProxy(proxyResolution, { unifiedConfig, allProviders, verbose, @@ -138,9 +151,6 @@ export async function execClaudeWithCLIProxy( log, }); - const { browserLaunchOverride, argsWithoutBrowserFlags } = - resolveBrowserLaunchFlags(argsWithoutProxy); - // Setup first-class CCS WebSearch runtime ensureWebSearchMcpOrThrow(); const imageAnalysisMcpReady = ensureImageAnalysisMcpOrThrow(); diff --git a/src/cliproxy/executor/proxy-resolver.ts b/src/cliproxy/executor/proxy-resolver.ts index eae39abb..bade6f32 100644 --- a/src/cliproxy/executor/proxy-resolver.ts +++ b/src/cliproxy/executor/proxy-resolver.ts @@ -24,20 +24,23 @@ import type { ResolvedProxyConfig } from '../types'; import type { UnifiedConfig } from '../../config/schemas/unified-config'; import { isNetworkError, handleNetworkError } from './retry-handler'; -/** Result returned from resolveExecutorProxy */ -export interface ResolvedProxy { +export interface ResolvedExecutorProxyConfig { /** Resolved proxy config after merging CLI > ENV > config.yaml > defaults */ proxyConfig: ResolvedProxyConfig; + /** Args after proxy-related flags are stripped out */ + argsWithoutProxy: string[]; + /** Mutated executor config (port resolved and validated) */ + cfg: ExecutorConfig; +} + +/** Result returned from resolveExecutorProxy */ +export interface ResolvedProxy extends ResolvedExecutorProxyConfig { /** Whether to use the remote proxy (vs spawning a local one) */ useRemoteProxy: boolean; /** Which local backend binary to use ('original' | 'plus') */ localBackend: CLIProxyBackend; /** Absolute path to CLIProxy binary; undefined when useRemoteProxy=true */ binaryPath: string | undefined; - /** Args after proxy-related flags are stripped out */ - argsWithoutProxy: string[]; - /** Mutated executor config (port resolved and validated) */ - cfg: ExecutorConfig; } /** Dependencies injected by the orchestrator */ @@ -50,16 +53,15 @@ export interface ResolveExecutorProxyContext { } /** - * Resolves proxy configuration, checks remote reachability, selects the local - * backend, and ensures the CLIProxy binary is present when running locally. + * Resolves side-effect-free proxy configuration and strips proxy flags. * * Mutates `context.cfg.port` in-place (same as original orchestrator behaviour). */ -export async function resolveExecutorProxy( +export function resolveExecutorProxyConfig( args: string[], context: ResolveExecutorProxyContext -): Promise { - const { unifiedConfig, allProviders, verbose: _verbose, cfg, log } = context; +): ResolvedExecutorProxyConfig { + const { unifiedConfig, cfg, log } = context; // Resolve proxy config from CLI flags > ENV > config.yaml > defaults const cliproxyServerConfig = unifiedConfig.cliproxy_server; @@ -98,6 +100,20 @@ export async function resolveExecutorProxy( log(`Remote host: ${proxyConfig.host}:${proxyConfig.port} (${proxyConfig.protocol})`); } + return { proxyConfig, argsWithoutProxy, cfg }; +} + +/** + * Resolves proxy configuration, checks remote reachability, selects the local + * backend, and ensures the CLIProxy binary is present when running locally. + */ +export async function resolveExecutorProxy( + resolvedConfig: ResolvedExecutorProxyConfig, + context: ResolveExecutorProxyContext +): Promise { + const { allProviders, verbose: _verbose } = context; + const { proxyConfig, argsWithoutProxy, cfg } = resolvedConfig; + // Check remote proxy reachability let useRemoteProxy = false; let localBackend: CLIProxyBackend = 'original'; diff --git a/src/cliproxy/quota/quota-manager.ts b/src/cliproxy/quota/quota-manager.ts index 8574b079..041e2748 100644 --- a/src/cliproxy/quota/quota-manager.ts +++ b/src/cliproxy/quota/quota-manager.ts @@ -483,9 +483,8 @@ export async function reconcileExhaustedRotationAccounts( return []; } - const { pauseAccountForQuotaCooldown, restoreExpiredQuotaPauses } = await import( - '../accounts/account-safety' - ); + const { pauseAccountForQuotaCooldown, restoreExpiredQuotaPauses } = + await import('../accounts/account-safety'); restoreExpiredQuotaPauses(); const config = loadOrCreateUnifiedConfig(); @@ -593,9 +592,8 @@ export async function preflightCheck(provider: CLIProxyProvider): Promise * Fix image analysis configuration issues */ export async function fixImageAnalysisConfig(): Promise { - const { updateConfig, loadOrCreateUnifiedConfig } = await import( - '../../config/config-loader-facade' - ); + const { updateConfig, loadOrCreateUnifiedConfig } = + await import('../../config/config-loader-facade'); const config = loadOrCreateUnifiedConfig(); let fixed = false;