mirror of
https://github.com/tiennm99/ccs.git
synced 2026-07-17 08:17:22 +00:00
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
This commit is contained in:
@@ -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})`);
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<ProxyStatus> {
|
||||
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');
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user