From a4c5bb7421748e744c07eb7ab5c7cfb3a55b8081 Mon Sep 17 00:00:00 2001 From: Tam Nhu Tran Date: Wed, 4 Mar 2026 01:11:07 +0700 Subject: [PATCH] fix(security): harden API endpoints and resolve PR #674 review findings - Fix X-Forwarded-For header spoofing in requireSensitiveLocalAccess (use socket-level address only) - Add model ID length validation (max 256 chars) in PUT /models/:provider - Add provider name validation (alphanumeric, max 64 chars) to prevent path traversal - Add stale entry eviction for unbounded quotaRateLimits Map (>1000 entries) - Fix concurrent refresh race in usage aggregator (wait for in-flight before forced refresh) - Sanitize internal URLs/paths from OAuth failure diagnostics - Add ValidationError class for denylist violations (distinct from system errors) - Make CLIProxy sync interval configurable via CCS_CLIPROXY_SYNC_INTERVAL env var - Improve legacy continuity config error logging (unconditional warn) - Add CACHE_VERSION history comments --- src/auth/profile-continuity-inheritance.ts | 12 +++++----- src/cliproxy/auth/oauth-process.ts | 6 ++++- src/errors/error-types.ts | 14 ++++++++++++ .../routes/cliproxy-stats-routes.ts | 22 +++++++++++++++++++ src/web-server/routes/route-helpers.ts | 5 +++-- src/web-server/routes/settings-routes.ts | 8 +++---- src/web-server/usage/aggregator.ts | 11 +++++++++- src/web-server/usage/cliproxy-usage-syncer.ts | 18 +++++++++------ src/web-server/usage/disk-cache.ts | 2 ++ 9 files changed, 76 insertions(+), 22 deletions(-) diff --git a/src/auth/profile-continuity-inheritance.ts b/src/auth/profile-continuity-inheritance.ts index 91fc253f..7503c053 100644 --- a/src/auth/profile-continuity-inheritance.ts +++ b/src/auth/profile-continuity-inheritance.ts @@ -52,13 +52,11 @@ function loadLegacyContinuityInheritanceMap(): Record { return normalized; } catch (error) { - if (process.env.CCS_DEBUG) { - console.error( - warn( - `Failed to parse legacy continuity mapping at "${configJsonPath}": ${(error as Error).message}` - ) - ); - } + console.error( + warn( + `Failed to parse legacy continuity mapping at "${configJsonPath}": ${(error as Error).message}` + ) + ); return {}; } } diff --git a/src/cliproxy/auth/oauth-process.ts b/src/cliproxy/auth/oauth-process.ts index 829d911c..d3458a18 100644 --- a/src/cliproxy/auth/oauth-process.ts +++ b/src/cliproxy/auth/oauth-process.ts @@ -304,8 +304,12 @@ async function handleTokenNotFound( console.log(''); if (failureReason) { + // Sanitize internal URLs/paths from failure reason to avoid leaking infrastructure details + const sanitizedReason = failureReason + .replace(/https?:\/\/(?:localhost|127\.0\.0\.1|0\.0\.0\.0)[^\s]*/gi, '[internal-url]') + .replace(/\/(?:root|home|opt|tmp|var)\/[^\s]*/g, '[path]'); console.log(fail('Authentication completed but token was not persisted')); - console.log(` ${failureReason}`); + console.log(` ${sanitizedReason}`); console.log(''); console.log('This usually means provider-side authorization was accepted,'); console.log('but CLIProxy failed a post-auth verification or token save step.'); diff --git a/src/errors/error-types.ts b/src/errors/error-types.ts index 92cf2cd7..08424dfe 100644 --- a/src/errors/error-types.ts +++ b/src/errors/error-types.ts @@ -155,6 +155,20 @@ export class UserAbortError extends CCSError { } } +/** + * Input validation errors (model denylist, invalid format, length limits) + * Distinguishes user-input validation failures from system errors + */ +export class ValidationError extends CCSError { + constructor( + message: string, + public readonly field?: string + ) { + super(message, ExitCode.GENERAL_ERROR, false); + this.name = 'ValidationError'; + } +} + /** * Type guard to check if an error is a CCSError */ diff --git a/src/web-server/routes/cliproxy-stats-routes.ts b/src/web-server/routes/cliproxy-stats-routes.ts index 15db9f5c..99bfb23b 100644 --- a/src/web-server/routes/cliproxy-stats-routes.ts +++ b/src/web-server/routes/cliproxy-stats-routes.ts @@ -77,6 +77,16 @@ function buildQuotaRateLimitKey(req: Request, provider: string): string { function isQuotaRouteRateLimited(req: Request, provider: string): boolean { const key = buildQuotaRateLimitKey(req, provider); const now = Date.now(); + + // Evict stale entries to prevent unbounded memory growth + if (quotaRateLimits.size > 1000) { + for (const [k, v] of quotaRateLimits) { + if (now - v.windowStart >= QUOTA_RATE_LIMIT_WINDOW_MS * 2) { + quotaRateLimits.delete(k); + } + } + } + const current = quotaRateLimits.get(key); if (!current || now - current.windowStart >= QUOTA_RATE_LIMIT_WINDOW_MS) { @@ -584,6 +594,13 @@ router.get('/auth-files/download', async (req: Request, res: Response): Promise< router.put('/models/:provider', async (req: Request, res: Response): Promise => { try { const { provider } = req.params; + + // Validate provider name to prevent path traversal via crafted provider param + if (!provider || provider.length > 64 || !/^[a-zA-Z0-9_-]+$/.test(provider)) { + res.status(400).json({ error: 'Invalid provider name' }); + return; + } + const { model } = req.body; if (!model || typeof model !== 'string') { @@ -591,6 +608,11 @@ router.put('/models/:provider', async (req: Request, res: Response): Promise 256) { + res.status(400).json({ error: 'Model ID exceeds maximum length (256 characters)' }); + return; + } + const deniedReason = getDeniedModelIdReasonForProvider(model, provider); if (deniedReason) { res.status(400).json({ error: deniedReason }); diff --git a/src/web-server/routes/route-helpers.ts b/src/web-server/routes/route-helpers.ts index c5b79a25..4f5c048e 100644 --- a/src/web-server/routes/route-helpers.ts +++ b/src/web-server/routes/route-helpers.ts @@ -16,6 +16,7 @@ import { } from '../../cliproxy/model-id-normalizer'; import type { CLIProxyProvider } from '../../cliproxy/types'; import type { Config, Settings } from '../../types/config'; +import { ValidationError } from '../../errors/error-types'; /** Model mapping for API profiles */ export interface ModelMapping { @@ -155,7 +156,7 @@ export function createSettingsFile( canonicalHaikuModel, ]); if (deniedReason) { - throw new Error(deniedReason); + throw new ValidationError(deniedReason, 'model'); } const droidProvider = resolveDroidProvider({ provider, @@ -237,7 +238,7 @@ export function updateSettingsFile( : settings.env?.ANTHROPIC_DEFAULT_HAIKU_MODEL, ]); if (deniedReason) { - throw new Error(deniedReason); + throw new ValidationError(deniedReason, 'model'); } if (updates.baseUrl) { diff --git a/src/web-server/routes/settings-routes.ts b/src/web-server/routes/settings-routes.ts index 8e3c198e..4d6a7af7 100644 --- a/src/web-server/routes/settings-routes.ts +++ b/src/web-server/routes/settings-routes.ts @@ -78,10 +78,10 @@ function requireSensitiveLocalAccess(req: Request, res: Response): boolean { return true; } - const forwarded = req.headers['x-forwarded-for']; - const firstForwarded = - typeof forwarded === 'string' ? forwarded.split(',')[0]?.trim() : undefined; - const candidateAddress = firstForwarded || req.socket.remoteAddress || req.ip; + // Use only socket-level address for security decisions. + // X-Forwarded-For is trivially spoofable and must NOT be trusted + // without an explicit trust proxy configuration. + const candidateAddress = req.socket.remoteAddress; if (isLoopbackAddress(candidateAddress)) { return true; diff --git a/src/web-server/usage/aggregator.ts b/src/web-server/usage/aggregator.ts index daca52ef..8e9fa3c0 100644 --- a/src/web-server/usage/aggregator.ts +++ b/src/web-server/usage/aggregator.ts @@ -378,8 +378,17 @@ async function refreshFromSourceCoalesced(force = false): Promise<{ monthly: MonthlyUsage[]; session: SessionUsage[]; }> { + // Wait for any in-flight refresh to finish before starting a forced one + // to prevent concurrent refreshes competing for the same resources + if (force && pendingFullRefresh) { + await pendingFullRefresh.catch(() => {}); + } + if (force) { - return refreshFromSource(); + pendingFullRefresh = refreshFromSource().finally(() => { + pendingFullRefresh = null; + }); + return pendingFullRefresh; } if (pendingFullRefresh) { diff --git a/src/web-server/usage/cliproxy-usage-syncer.ts b/src/web-server/usage/cliproxy-usage-syncer.ts index cc36763f..64e564ad 100644 --- a/src/web-server/usage/cliproxy-usage-syncer.ts +++ b/src/web-server/usage/cliproxy-usage-syncer.ts @@ -34,6 +34,12 @@ interface CliproxyUsageSnapshot { const SNAPSHOT_VERSION = 1; +/** Sync interval in ms, configurable via CCS_CLIPROXY_SYNC_INTERVAL env var (default: 5 min) */ +const SYNC_INTERVAL_MS = Math.max( + 30_000, + parseInt(process.env.CCS_CLIPROXY_SYNC_INTERVAL ?? '300000', 10) || 300_000 +); + // Module-level interval ID let syncIntervalId: ReturnType | null = null; @@ -149,17 +155,15 @@ export function startCliproxySync(): void { return; } - console.log(info('Starting CLIProxy usage sync (interval: 5 min)')); + const intervalMin = Math.round(SYNC_INTERVAL_MS / 60_000); + console.log(info(`Starting CLIProxy usage sync (interval: ${intervalMin} min)`)); // Fire-and-forget initial sync void syncCliproxyUsage(); - syncIntervalId = setInterval( - () => { - void syncCliproxyUsage(); - }, - 5 * 60 * 1000 - ); + syncIntervalId = setInterval(() => { + void syncCliproxyUsage(); + }, SYNC_INTERVAL_MS); } /** diff --git a/src/web-server/usage/disk-cache.ts b/src/web-server/usage/disk-cache.ts index c97d1632..a2bc1e86 100644 --- a/src/web-server/usage/disk-cache.ts +++ b/src/web-server/usage/disk-cache.ts @@ -35,6 +35,8 @@ export interface UsageDiskCache { } // Current cache version - increment to invalidate old caches +// v1: Initial cache format (daily, monthly, session) +// v2: Added multi-instance aggregation // v3: Added hourly data to cache // v4: Pricing fix for Claude 4.6 models (invalidate stale costs) const CACHE_VERSION = 4;