From e0a1f8f312c1eb7621b3ea2af2edb4df44a51f64 Mon Sep 17 00:00:00 2001 From: kaitranntt Date: Thu, 1 Jan 2026 02:00:30 -0500 Subject: [PATCH] fix(cliproxy): add comprehensive port validation across proxy system - Export validatePort() from config-generator with full edge case handling - Add YAML port validation in proxy-config-resolver before use - Add final port validation in cliproxy-executor after config merge - Add default port parameter and validation to proxy-detector - Add 38 new tests covering all port validation edge cases Fixes undefined port bug when upgrading from 7.11.1 to 7.12.0 --- src/cliproxy/cliproxy-executor.ts | 4 + src/cliproxy/config-generator.ts | 8 +- src/cliproxy/proxy-config-resolver.ts | 6 +- src/cliproxy/proxy-detector.ts | 27 ++- tests/unit/cliproxy/port-validation.test.ts | 229 ++++++++++++++++++ .../unit/cliproxy/proxy-detector-port.test.ts | 67 +++++ 6 files changed, 327 insertions(+), 14 deletions(-) create mode 100644 tests/unit/cliproxy/port-validation.test.ts create mode 100644 tests/unit/cliproxy/proxy-detector-port.test.ts diff --git a/src/cliproxy/cliproxy-executor.ts b/src/cliproxy/cliproxy-executor.ts index a8e821ed..4b11748d 100644 --- a/src/cliproxy/cliproxy-executor.ts +++ b/src/cliproxy/cliproxy-executor.ts @@ -26,6 +26,7 @@ import { ensureProviderSettings, CLIPROXY_DEFAULT_PORT, getCliproxyWritablePath, + validatePort, } from './config-generator'; import { checkRemoteProxy } from './remote-proxy-client'; import { isAuthenticated } from './auth-handler'; @@ -172,6 +173,9 @@ export async function execClaudeWithCLIProxy( cfg.port = proxyConfig.port; } + // Final port validation - ensure valid port after all resolution + cfg.port = validatePort(cfg.port); + log(`Proxy mode: ${proxyConfig.mode}`); if (proxyConfig.mode === 'remote') { log(`Remote host: ${proxyConfig.host}:${proxyConfig.port} (${proxyConfig.protocol})`); diff --git a/src/cliproxy/config-generator.ts b/src/cliproxy/config-generator.ts index bb4872a7..e2d9c40f 100644 --- a/src/cliproxy/config-generator.ts +++ b/src/cliproxy/config-generator.ts @@ -26,8 +26,14 @@ interface ProviderSettings { /** * Validate port is a valid positive integer (1-65535). * Returns default port if invalid. + * + * @param port - Port number to validate + * @returns Valid port or CLIPROXY_DEFAULT_PORT (8317) if invalid */ -function validatePort(port: number): number { +export function validatePort(port: number | undefined): number { + if (port === undefined || port === null) { + return CLIPROXY_DEFAULT_PORT; + } if (!Number.isFinite(port) || port < 1 || port > 65535 || !Number.isInteger(port)) { return CLIPROXY_DEFAULT_PORT; } diff --git a/src/cliproxy/proxy-config-resolver.ts b/src/cliproxy/proxy-config-resolver.ts index f38bd7cf..c4e382f9 100644 --- a/src/cliproxy/proxy-config-resolver.ts +++ b/src/cliproxy/proxy-config-resolver.ts @@ -8,7 +8,7 @@ */ import { ResolvedProxyConfig } from './types'; -import { CLIPROXY_DEFAULT_PORT } from './config-generator'; +import { CLIPROXY_DEFAULT_PORT, validatePort } from './config-generator'; /** CLI flags for proxy configuration */ export const PROXY_CLI_FLAGS = [ @@ -276,10 +276,12 @@ export function resolveProxyConfig( resolved.host = cliFlags.host ?? envConfig.host ?? yamlConfig.remote?.host; // Merge port: CLI > ENV > config.yaml (remote or local) > default + // Validate YAML-sourced ports before use (CLI/ENV already validated during parse) + const yamlPort = resolved.mode === 'remote' ? yamlConfig.remote?.port : yamlConfig.local?.port; resolved.port = cliFlags.port ?? envConfig.port ?? - (resolved.mode === 'remote' ? yamlConfig.remote?.port : yamlConfig.local?.port) ?? + (yamlPort !== undefined ? validatePort(yamlPort) : undefined) ?? DEFAULT_PROXY_CONFIG.port; // Merge protocol: CLI > ENV > config.yaml > default diff --git a/src/cliproxy/proxy-detector.ts b/src/cliproxy/proxy-detector.ts index d1d6afe6..0e381778 100644 --- a/src/cliproxy/proxy-detector.ts +++ b/src/cliproxy/proxy-detector.ts @@ -18,6 +18,7 @@ import { getExistingProxy, registerSession, getRunningProxyVersion } from './session-tracker'; import { isCliproxyRunning } from './stats-fetcher'; import { getPortProcess, isCLIProxyProcess, PortProcess } from '../utils/port-utils'; +import { CLIPROXY_DEFAULT_PORT } from './config-generator'; /** Detection method used to find the proxy */ export type DetectionMethod = 'http' | 'session-lock' | 'port-process' | 'http-retry'; @@ -61,27 +62,31 @@ const noopLog: LogFn = () => {}; * @returns ProxyStatus with detection details */ export async function detectRunningProxy( - port: number, + port: number = CLIPROXY_DEFAULT_PORT, verbose: boolean = false ): Promise { const log: LogFn = verbose ? (msg) => console.error(`[proxy-detector] ${msg}`) : noopLog; - log(`Detecting proxy on port ${port}...`); + // Validate port - fallback to default if invalid + const validPort = + port && Number.isFinite(port) && port >= 1 && port <= 65535 ? port : CLIPROXY_DEFAULT_PORT; + + log(`Detecting proxy on port ${validPort}...`); // 1. First: HTTP health check (fastest, most reliable) log('Trying HTTP health check...'); - const healthy = await isCliproxyRunning(port); + const healthy = await isCliproxyRunning(validPort); if (healthy) { // Proxy is running and responsive // Try to get PID from session lock if available - const lock = getExistingProxy(port); + const lock = getExistingProxy(validPort); let pid = lock?.pid; // If no PID from session lock, try port-process detection // This handles orphaned proxies (running but no sessions.json) if (!pid) { log('No PID from session lock, checking port process...'); - const portProcess = await getPortProcess(port); + const portProcess = await getPortProcess(validPort); if (portProcess) { pid = portProcess.pid; log(`Got PID from port process: ${pid}`); @@ -89,7 +94,7 @@ export async function detectRunningProxy( } // Get version from session lock - const runningVersion = getRunningProxyVersion(port); + const runningVersion = getRunningProxyVersion(validPort); log( `HTTP check passed, proxy healthy (PID: ${pid ?? 'unknown'}, version: ${runningVersion ?? 'unknown'})` @@ -107,11 +112,11 @@ export async function detectRunningProxy( // 2. Second: Check session lock file log('Checking session lock file...'); - const lock = getExistingProxy(port); + const lock = getExistingProxy(validPort); if (lock) { // Session lock exists - proxy might be starting up // The lock validates PID is running, so proxy exists but not ready - const lockVersion = getRunningProxyVersion(port); + const lockVersion = getRunningProxyVersion(validPort); log( `Session lock found: PID ${lock.pid}, ${lock.sessions.length} sessions, version: ${lockVersion ?? 'unknown'}` ); @@ -128,7 +133,7 @@ export async function detectRunningProxy( // 3. Third: Check port process (fallback for orphaned/untracked proxy) log('Checking port process...'); - const portProcess = await getPortProcess(port); + const portProcess = await getPortProcess(validPort); if (portProcess) { log(`Port occupied by: ${portProcess.processName} (PID ${portProcess.pid})`); @@ -136,7 +141,7 @@ export async function detectRunningProxy( if (isCLIProxyProcess(portProcess)) { log('Process name matches CLIProxy, retrying HTTP...'); // Looks like CLIProxy by name - verify with HTTP - const retryHealth = await isCliproxyRunning(port); + const retryHealth = await isCliproxyRunning(validPort); if (retryHealth) { log('HTTP retry passed, proxy is healthy'); return { @@ -159,7 +164,7 @@ export async function detectRunningProxy( // Process name doesn't match (or Windows returned PID-XXXXX) // Do one more HTTP check to be sure it's not ours log('Process name unrecognized, final HTTP check (Windows PID-XXXXX case)...'); - const finalHealthCheck = await isCliproxyRunning(port); + const finalHealthCheck = await isCliproxyRunning(validPort); if (finalHealthCheck) { // It IS our proxy, just with unrecognized name (Windows case) log('Final HTTP check passed, reclaiming proxy with unrecognized name'); diff --git a/tests/unit/cliproxy/port-validation.test.ts b/tests/unit/cliproxy/port-validation.test.ts new file mode 100644 index 00000000..ac9831c8 --- /dev/null +++ b/tests/unit/cliproxy/port-validation.test.ts @@ -0,0 +1,229 @@ +/** + * Port Validation Tests + * + * Comprehensive test suite for port validation across the CLIProxy system. + * Tests the fix for undefined port bug (7.11.1 → 7.12.0 regression). + * + * Covers: + * - validatePort() function edge cases + * - Object spread undefined filtering in executor + * - YAML port validation in resolver + * - detectRunningProxy with invalid ports + */ + +import { describe, it, expect } from 'bun:test'; +import { + validatePort, + CLIPROXY_DEFAULT_PORT, +} from '../../../dist/cliproxy/config-generator'; +import { resolveProxyConfig } from '../../../dist/cliproxy/proxy-config-resolver'; + +describe('Port Validation', () => { + describe('validatePort()', () => { + describe('returns default port for invalid inputs', () => { + it('handles undefined', () => { + expect(validatePort(undefined)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles null', () => { + expect(validatePort(null as unknown as number)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles NaN', () => { + expect(validatePort(NaN)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles Infinity', () => { + expect(validatePort(Infinity)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles negative Infinity', () => { + expect(validatePort(-Infinity)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles zero', () => { + expect(validatePort(0)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles negative numbers', () => { + expect(validatePort(-1)).toBe(CLIPROXY_DEFAULT_PORT); + expect(validatePort(-8317)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles ports > 65535', () => { + expect(validatePort(65536)).toBe(CLIPROXY_DEFAULT_PORT); + expect(validatePort(100000)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles floating point numbers', () => { + expect(validatePort(8317.5)).toBe(CLIPROXY_DEFAULT_PORT); + expect(validatePort(8317.999)).toBe(CLIPROXY_DEFAULT_PORT); + }); + }); + + describe('returns valid port for valid inputs', () => { + it('handles port 1 (minimum valid)', () => { + expect(validatePort(1)).toBe(1); + }); + + it('handles port 65535 (maximum valid)', () => { + expect(validatePort(65535)).toBe(65535); + }); + + it('handles default port 8317', () => { + expect(validatePort(8317)).toBe(8317); + }); + + it('handles common ports', () => { + expect(validatePort(80)).toBe(80); + expect(validatePort(443)).toBe(443); + expect(validatePort(3000)).toBe(3000); + expect(validatePort(8080)).toBe(8080); + }); + + it('handles variant ports (8318-8417)', () => { + expect(validatePort(8318)).toBe(8318); + expect(validatePort(8350)).toBe(8350); + expect(validatePort(8417)).toBe(8417); + }); + }); + }); + + describe('Object Spread Undefined Filtering', () => { + /** + * Bug: `{ ...DEFAULT_CONFIG, ...{ port: undefined } }` overwrites port with undefined + * Fix: Filter undefined values before spreading + */ + it('demonstrates the original bug pattern', () => { + const DEFAULT_CONFIG = { port: 8317, timeout: 2000 }; + const config = { port: undefined, timeout: undefined }; + + // BUG: Direct spread overwrites with undefined + const bugResult = { ...DEFAULT_CONFIG, ...config }; + expect(bugResult.port).toBe(undefined); // This was the bug! + + // FIX: Filter undefined values before spreading + const filteredConfig = Object.fromEntries( + Object.entries(config).filter(([, v]) => v !== undefined) + ); + const fixResult = { ...DEFAULT_CONFIG, ...filteredConfig }; + expect(fixResult.port).toBe(8317); // Fixed! + }); + + it('preserves explicit values while filtering undefined', () => { + const DEFAULT_CONFIG = { port: 8317, timeout: 2000, protocol: 'http' }; + const config = { port: 9000, timeout: undefined, protocol: undefined }; + + const filteredConfig = Object.fromEntries( + Object.entries(config).filter(([, v]) => v !== undefined) + ); + const result = { ...DEFAULT_CONFIG, ...filteredConfig }; + + expect(result.port).toBe(9000); // Explicit value preserved + expect(result.timeout).toBe(2000); // Default preserved (undefined filtered) + expect(result.protocol).toBe('http'); // Default preserved (undefined filtered) + }); + }); + + describe('YAML Port Validation in resolveProxyConfig', () => { + it('validates YAML remote port', () => { + const { config } = resolveProxyConfig([], { + remote: { host: 'example.com', port: 9000 }, + }); + expect(config.port).toBe(9000); + }); + + it('falls back to default for invalid YAML port (negative)', () => { + const { config } = resolveProxyConfig([], { + remote: { host: 'example.com', port: -1 }, + }); + expect(config.port).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('falls back to default for invalid YAML port (> 65535)', () => { + const { config } = resolveProxyConfig([], { + remote: { host: 'example.com', port: 70000 }, + }); + expect(config.port).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('falls back to default for invalid YAML port (zero)', () => { + const { config } = resolveProxyConfig([], { + remote: { host: 'example.com', port: 0 }, + }); + expect(config.port).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('validates YAML local port', () => { + const { config } = resolveProxyConfig([], { + local: { port: 9000 }, + }); + expect(config.port).toBe(9000); + }); + + it('falls back to default for invalid YAML local port', () => { + const { config } = resolveProxyConfig([], { + local: { port: -100 }, + }); + expect(config.port).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('CLI port overrides invalid YAML port', () => { + const { config } = resolveProxyConfig(['--proxy-port', '8080'], { + remote: { host: 'example.com', port: -1 }, + }); + expect(config.port).toBe(8080); + }); + }); + + describe('Priority Chain with Invalid Values', () => { + it('CLI valid > ENV valid > YAML invalid', () => { + process.env.CCS_PROXY_PORT = '9000'; + const { config } = resolveProxyConfig(['--proxy-port', '8080'], { + remote: { host: 'example.com', port: -1 }, + }); + expect(config.port).toBe(8080); // CLI wins + delete process.env.CCS_PROXY_PORT; + }); + + it('ENV valid > YAML invalid > default', () => { + process.env.CCS_PROXY_PORT = '9000'; + const { config } = resolveProxyConfig([], { + remote: { host: 'example.com', port: -1 }, + }); + expect(config.port).toBe(9000); // ENV wins + delete process.env.CCS_PROXY_PORT; + }); + + it('YAML valid when CLI and ENV not set', () => { + const { config } = resolveProxyConfig([], { + remote: { host: 'example.com', port: 9000 }, + }); + expect(config.port).toBe(9000); + }); + + it('default when all sources invalid/missing', () => { + const { config } = resolveProxyConfig([], {}); + expect(config.port).toBe(CLIPROXY_DEFAULT_PORT); + }); + }); + + describe('Boundary Conditions', () => { + it('handles port exactly at minimum boundary (1)', () => { + expect(validatePort(1)).toBe(1); + expect(validatePort(0)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles port exactly at maximum boundary (65535)', () => { + expect(validatePort(65535)).toBe(65535); + expect(validatePort(65536)).toBe(CLIPROXY_DEFAULT_PORT); + }); + + it('handles reserved ports (< 1024)', () => { + // Reserved ports are still valid from validation perspective + expect(validatePort(22)).toBe(22); + expect(validatePort(80)).toBe(80); + expect(validatePort(443)).toBe(443); + }); + }); +}); diff --git a/tests/unit/cliproxy/proxy-detector-port.test.ts b/tests/unit/cliproxy/proxy-detector-port.test.ts new file mode 100644 index 00000000..6006b328 --- /dev/null +++ b/tests/unit/cliproxy/proxy-detector-port.test.ts @@ -0,0 +1,67 @@ +/** + * Proxy Detector Port Validation Tests + * + * Tests for detectRunningProxy with invalid port inputs. + * Verifies the fix handles undefined, null, and invalid ports gracefully. + */ + +import { describe, it, expect } from 'bun:test'; +import { detectRunningProxy } from '../../../dist/cliproxy/proxy-detector'; +import { CLIPROXY_DEFAULT_PORT } from '../../../dist/cliproxy/config-generator'; + +describe('Proxy Detector Port Validation', () => { + describe('detectRunningProxy with invalid ports', () => { + it('handles undefined port (uses default)', async () => { + // TypeScript allows this at runtime via any/unknown + const result = await detectRunningProxy(undefined as unknown as number); + // Should not throw, should use default port + expect(result).toBeDefined(); + expect(typeof result.running).toBe('boolean'); + }); + + it('handles NaN port (uses default)', async () => { + const result = await detectRunningProxy(NaN); + expect(result).toBeDefined(); + expect(typeof result.running).toBe('boolean'); + }); + + it('handles negative port (uses default)', async () => { + const result = await detectRunningProxy(-1); + expect(result).toBeDefined(); + expect(typeof result.running).toBe('boolean'); + }); + + it('handles zero port (uses default)', async () => { + const result = await detectRunningProxy(0); + expect(result).toBeDefined(); + expect(typeof result.running).toBe('boolean'); + }); + + it('handles port > 65535 (uses default)', async () => { + const result = await detectRunningProxy(70000); + expect(result).toBeDefined(); + expect(typeof result.running).toBe('boolean'); + }); + + it('handles valid port correctly', async () => { + const result = await detectRunningProxy(CLIPROXY_DEFAULT_PORT); + expect(result).toBeDefined(); + expect(typeof result.running).toBe('boolean'); + expect(typeof result.verified).toBe('boolean'); + }); + + it('handles variant port correctly', async () => { + const result = await detectRunningProxy(8318); + expect(result).toBeDefined(); + expect(typeof result.running).toBe('boolean'); + }); + }); + + describe('detectRunningProxy default parameter', () => { + it('works without port argument (uses default)', async () => { + const result = await detectRunningProxy(); + expect(result).toBeDefined(); + expect(typeof result.running).toBe('boolean'); + }); + }); +});