Skip to content

Commit 861b99a

Browse files
committed
fix(claude): production hardening — 2 BLOCKERs + 8 MAJORs from sol gap audit
BLOCKER fixes: - outbound.ts: detached pump pattern for SSE streaming — frames now reach the client as they arrive instead of buffering until upstream EOF; cancel calls reader.cancel() instead of the locked stream (#1) - system-env.ts: transactional launchctl injection with incremental tracking and rollback on mid-flight failure (#2) MAJOR fixes: - system-env.ts: POSIX single-quote escaping for all user-controlled values in the generated shell file, preventing command injection (#3) - system-env.ts: exported applySystemEnvToggle for runtime ON/OFF lifecycle (#7) - claude-messages.ts: 120s hard timeout on native passthrough upstream fetch using AbortSignal.timeout, returning Anthropic 504 on expiry (#4) - management-api.ts: reject non-plain-object PUT bodies (null, array, primitive) with 400; strict validation for blockedSkills, tierModels, modelMap, and autoCompactWindow (integer, range 100k-1M) (#5, #6) - management-api.ts: call applySystemEnvToggle after systemEnv config toggle (#7) - cli/claude.ts: override stale loopback ANTHROPIC_BASE_URL with live port; warn on gateway-cache and agent-sync failures instead of swallowing (#8, #10) - gateway-cache.ts: distinguish discovery failure (keep cache) from authoritative empty success (write {models:[]}) (#9) Test fixes: updated assertions for single-quote shell escaping, empty cache semantics, validation messages, incremental tracking writes, and PUT response shape.
1 parent 6638459 commit 861b99a

9 files changed

Lines changed: 259 additions & 144 deletions

src/claude/gateway-cache.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ export function writeGatewayModelCache(baseUrl: string, models: readonly Gateway
3030
try {
3131
// Mirror the CLI's usable-id filter so our file matches what it would cache.
3232
const usable = models.filter(m => /^(claude|anthropic)/i.test(m.id));
33-
if (usable.length === 0) return null;
3433
const cacheDir = join(configDir, "cache");
3534
mkdirSync(cacheDir, { recursive: true });
3635
const path = join(cacheDir, "gateway-models.json");
@@ -56,8 +55,9 @@ export async function refreshGatewayModelCacheFromProxy(port: number, timeoutMs
5655
signal: AbortSignal.timeout(timeoutMs),
5756
});
5857
if (!res.ok) return null;
59-
const body = await res.json() as { data?: Array<Record<string, unknown>> };
60-
const models: GatewayModelRow[] = (Array.isArray(body.data) ? body.data : [])
58+
const body = await res.json() as { data?: unknown };
59+
if (!Array.isArray(body.data)) return null;
60+
const models: GatewayModelRow[] = body.data
6161
.filter(m => typeof m.id === "string" && (m.id as string).length > 0)
6262
.map(m => ({
6363
id: m.id as string,

src/claude/outbound.ts

Lines changed: 40 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,15 @@ export function responsesSseToAnthropicSse(
105105
let buffer = "";
106106
let started = false;
107107
let terminated = false;
108+
let cancelled = false;
108109
let blockIndex = 0;
109110
let open: OpenBlock | null = null;
110111
let sawToolUse = false;
111112
let pingTimer: ReturnType<typeof setInterval> | undefined;
113+
let reader: ReadableStreamDefaultReader<Uint8Array> | undefined;
112114

113115
return new ReadableStream<Uint8Array>({
114-
async start(controller) {
116+
start(controller) {
115117
const emit = (name: string, data: Rec) => controller.enqueue(encoder.encode(sseFrame(name, data)));
116118
const ensureStarted = () => {
117119
if (started) return;
@@ -267,46 +269,49 @@ export function responsesSseToAnthropicSse(
267269
}
268270
};
269271

270-
const reader = upstream.getReader();
271-
try {
272-
for (;;) {
273-
const { done, value } = await reader.read();
274-
if (done) break;
275-
buffer += decoder.decode(value, { stream: true });
276-
let sep: number;
277-
while ((sep = buffer.indexOf("\n\n")) !== -1) {
278-
const rawFrame = buffer.slice(0, sep);
279-
buffer = buffer.slice(sep + 2);
280-
let eventName = "";
281-
let dataLine = "";
282-
for (const line of rawFrame.split("\n")) {
283-
if (line.startsWith("event: ")) eventName = line.slice(7).trim();
284-
else if (line.startsWith("data: ")) dataLine += line.slice(6);
272+
reader = upstream.getReader();
273+
void (async () => {
274+
try {
275+
for (;;) {
276+
const { done, value } = await reader.read();
277+
if (done) break;
278+
buffer += decoder.decode(value, { stream: true });
279+
let sep: number;
280+
while ((sep = buffer.indexOf("\n\n")) !== -1) {
281+
const rawFrame = buffer.slice(0, sep);
282+
buffer = buffer.slice(sep + 2);
283+
let eventName = "";
284+
let dataLine = "";
285+
for (const line of rawFrame.split("\n")) {
286+
if (line.startsWith("event: ")) eventName = line.slice(7).trim();
287+
else if (line.startsWith("data: ")) dataLine += line.slice(6);
288+
}
289+
if (!eventName || !dataLine) continue;
290+
let data: unknown;
291+
try { data = JSON.parse(dataLine); } catch { continue; }
292+
if (!isRec(data)) continue;
293+
if (terminated) continue;
294+
handleFrame(eventName, data);
285295
}
286-
if (!eventName || !dataLine) continue;
287-
let data: unknown;
288-
try { data = JSON.parse(dataLine); } catch { continue; }
289-
if (!isRec(data)) continue;
290-
if (terminated) continue;
291-
handleFrame(eventName, data);
292296
}
297+
// EOF without a terminal frame is a TRUNCATION, not success (devlog 100:
298+
// gateways that close such streams politely hand Claude Code an empty/partial
299+
// turn with no retryable error — CLIProxyAPI#2189 failure pattern). Fail closed
300+
// with a mid-stream Anthropic error event so the client can retry.
301+
if (!cancelled) fail(502, "upstream stream ended before a terminal frame (truncated response)");
302+
} catch (err) {
303+
fail(500, err instanceof Error ? err.message : String(err));
304+
} finally {
305+
if (pingTimer !== undefined) clearInterval(pingTimer);
306+
reader.releaseLock();
307+
if (!cancelled) controller.close();
293308
}
294-
// EOF without a terminal frame is a TRUNCATION, not success (devlog 100:
295-
// gateways that close such streams politely hand Claude Code an empty/partial
296-
// turn with no retryable error — CLIProxyAPI#2189 failure pattern). Fail closed
297-
// with a mid-stream Anthropic error event so the client can retry.
298-
fail(502, "upstream stream ended before a terminal frame (truncated response)");
299-
} catch (err) {
300-
fail(500, err instanceof Error ? err.message : String(err));
301-
} finally {
302-
if (pingTimer !== undefined) clearInterval(pingTimer);
303-
reader.releaseLock();
304-
controller.close();
305-
}
309+
})();
306310
},
307311
cancel(reason) {
312+
cancelled = true;
308313
if (pingTimer !== undefined) clearInterval(pingTimer);
309-
return upstream.cancel(reason);
314+
return reader?.cancel(reason);
310315
},
311316
});
312317
}

src/cli/claude.ts

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
*
44
* Mirrors `ccr code` UX (devlog/260711_claude_inbound/020, 003 E1/E2/E5/G1):
55
* ensures the proxy is running, injects the Anthropic env slots, then execs the
6-
* `claude` CLI with stdio inherited. User-exported env always wins.
6+
* `claude` CLI with stdio inherited. User-exported env wins except when a stale
7+
* loopback opencodex base URL points at a different proxy port.
78
*/
89
import { spawn } from "node:child_process";
910
import { loadConfig } from "../config";
@@ -20,7 +21,8 @@ export interface ClaudeLaunchEnv {
2021
/**
2122
* Pure env assembly (unit-tested): never sets ANTHROPIC_API_KEY (setting both
2223
* token vars triggers Claude Code's auth-conflict warning, 003 E1), and never
23-
* overrides variables the user already exported.
24+
* overrides variables the user already exported, apart from stale loopback
25+
* ANTHROPIC_BASE_URL values owned by a previous opencodex launch.
2426
*/
2527
export function buildClaudeEnv(config: OcxConfig, port: number, base: ClaudeLaunchEnv, contextWindows: Record<string, number> = {}): ClaudeLaunchEnv {
2628
const env: ClaudeLaunchEnv = { ...base };
@@ -30,6 +32,20 @@ export function buildClaudeEnv(config: OcxConfig, port: number, base: ClaudeLaun
3032
env[name] = value;
3133
};
3234
setDefault("ANTHROPIC_BASE_URL", `http://127.0.0.1:${port}`);
35+
const existingBaseUrl = env.ANTHROPIC_BASE_URL;
36+
if (existingBaseUrl) {
37+
try {
38+
const parsed = new URL(existingBaseUrl);
39+
const isLoopback = parsed.hostname === "localhost" || parsed.hostname === "127.0.0.1";
40+
if (isLoopback && parsed.port !== "" && Number(parsed.port) !== port) {
41+
const replacement = `http://127.0.0.1:${port}`;
42+
console.error(`⚠ Replacing stale opencodex ANTHROPIC_BASE_URL ${existingBaseUrl} with ${replacement}.`);
43+
env.ANTHROPIC_BASE_URL = replacement;
44+
}
45+
} catch {
46+
// Preserve user-provided values that are not parseable URLs.
47+
}
48+
}
3349
// Subscription-preserving default (teamclaude --no-mitm / Vercel gateway pattern):
3450
// setting ANTHROPIC_AUTH_TOKEN/API_KEY disables claude.ai connectors and overrides
3551
// the user's Claude login. Only inject a token when the proxy actually requires an
@@ -137,9 +153,25 @@ export async function cmdClaude(args: string[]): Promise<number> {
137153
const env = buildClaudeEnv(config, port, process.env, contextWindows);
138154
// Pre-write the CLI's gateway-model cache (devlog 030): without a token the CLI
139155
// never refreshes it, so the picker would keep showing yesterday's aliases.
140-
await refreshGatewayModelCacheFromProxy(port);
156+
try {
157+
const cachePath = await refreshGatewayModelCacheFromProxy(port);
158+
if (cachePath === null) {
159+
console.error("⚠ Gateway model cache could not be refreshed; the model picker may be stale.");
160+
}
161+
} catch (error) {
162+
const message = error instanceof Error ? error.message : String(error);
163+
console.error(`⚠ Gateway model cache could not be refreshed: ${message}`);
164+
}
141165
// Sync roster agents (devlog 070): subagentModels + self -> ~/.claude/agents/ocx-*.md.
142-
injectClaudeAgentDefs(config, contextWindows);
166+
try {
167+
const written = injectClaudeAgentDefs(config, contextWindows);
168+
if (written === null) {
169+
console.error("⚠ Claude agent definitions could not be synced; check ~/.claude/agents permissions.");
170+
}
171+
} catch (error) {
172+
const message = error instanceof Error ? error.message : String(error);
173+
console.error(`⚠ Claude agent definitions could not be synced: ${message}`);
174+
}
143175
return await new Promise<number>(resolve => {
144176
const child = spawn("claude", args, { stdio: "inherit", env: env as NodeJS.ProcessEnv });
145177
child.on("error", (err: NodeJS.ErrnoException) => {

src/server/claude-messages.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -178,15 +178,21 @@ async function anthropicNativePassthrough(
178178
});
179179
headers.set("content-type", "application/json");
180180

181+
const timeoutSignal = AbortSignal.timeout(config.connectTimeoutMs ?? 120_000);
182+
const upstreamSignal = AbortSignal.any([req.signal, timeoutSignal]);
181183
let upstream: Response;
182184
try {
183185
upstream = await fetch(`${base}${pathname}${search}`, {
184186
method: "POST",
185187
headers,
186188
body: JSON.stringify(body),
187-
signal: req.signal,
189+
signal: upstreamSignal,
188190
});
189191
} catch (err) {
192+
if (timeoutSignal.aborted && upstreamSignal.reason === timeoutSignal.reason) {
193+
finalize(504, { closeReason: "non_stream" });
194+
return anthropicErrorResponse(504, "anthropic passthrough timed out waiting for response headers", "timeout_error");
195+
}
190196
finalize(502, { closeReason: "non_stream" });
191197
return anthropicErrorResponse(502, `anthropic passthrough failed: ${err instanceof Error ? err.message : String(err)}`, "api_error");
192198
}

src/server/management-api.ts

Lines changed: 49 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import type { OcxConfig, OcxProviderConfig } from "../types";
4141
import { drainAndShutdown } from "./lifecycle";
4242
import { filterRequestLogs, getRequestLogEntries } from "./request-log";
4343
import { isAllowedRequestOrigin, jsonResponse, providerManagementConfigError, publicProviderBaseUrl, safeConfigDTO } from "./auth-cors";
44+
import { applySystemEnvToggle } from "./system-env";
4445

4546
// Single source of truth = package.json (../ from src/), so /healthz + the GUI badge match the
4647
// installed npm version instead of a stale hardcode.
@@ -734,8 +735,15 @@ export async function handleManagementAPI(req: Request, url: URL, config: OcxCon
734735
// supersede tiers; auto-context supersedes the max-context pair; effort rides
735736
// regardless on 2.1.207). PUT keeps validating them so hand-written configs
736737
// and older GUIs stay safe; GUI saves omit them and the spread preserves them.
737-
let body: { enabled?: unknown; model?: unknown; smallFastModel?: unknown; modelMap?: unknown; systemEnv?: unknown; fastMode?: unknown; maxContextTokens?: unknown; alwaysEnableEffort?: unknown; tierModels?: unknown; autoContext?: unknown; autoCompactWindow?: unknown; blockedSkills?: unknown; injectAgents?: unknown };
738-
try { body = await req.json(); } catch { return jsonResponse({ error: "invalid JSON body" }, 400); }
738+
let parsedBody: unknown;
739+
try { parsedBody = await req.json(); } catch { return jsonResponse({ error: "invalid JSON body" }, 400); }
740+
const isPlainObject = (value: unknown): value is Record<string, unknown> => {
741+
if (value === null || typeof value !== "object" || Array.isArray(value)) return false;
742+
const prototype = Object.getPrototypeOf(value);
743+
return prototype === Object.prototype || prototype === null;
744+
};
745+
if (!isPlainObject(parsedBody)) return jsonResponse({ error: "body must be an object" }, 400);
746+
const body = parsedBody as { enabled?: unknown; model?: unknown; smallFastModel?: unknown; modelMap?: unknown; systemEnv?: unknown; fastMode?: unknown; maxContextTokens?: unknown; alwaysEnableEffort?: unknown; tierModels?: unknown; autoContext?: unknown; autoCompactWindow?: unknown; blockedSkills?: unknown; injectAgents?: unknown };
739747
const next = { ...(config.claudeCode ?? {}) };
740748
if (body.enabled !== undefined) {
741749
if (typeof body.enabled !== "boolean") return jsonResponse({ error: "enabled must be a boolean" }, 400);
@@ -797,18 +805,23 @@ export async function handleManagementAPI(req: Request, url: URL, config: OcxCon
797805
}
798806
if (body.tierModels !== undefined) {
799807
// CONFIG-ONLY back-compat (GUI pickers removed — roster agents supersede tiers).
800-
if (!body.tierModels || typeof body.tierModels !== "object" || Array.isArray(body.tierModels)) {
801-
return jsonResponse({ error: "tierModels must be an object" }, 400);
802-
}
803-
const tiers: Record<string, string> = {};
804-
for (const tier of ["opus", "sonnet", "haiku", "fable"] as const) {
805-
const value = (body.tierModels as Record<string, unknown>)[tier];
806-
if (value === undefined || value === null) continue;
807-
if (typeof value !== "string") return jsonResponse({ error: `tierModels.${tier} must be a string` }, 400);
808-
if (value.trim() !== "") tiers[tier] = value.trim();
808+
if (body.tierModels === null) {
809+
delete next.tierModels;
810+
} else if (!isPlainObject(body.tierModels)) {
811+
return jsonResponse({ error: "tierModels must be an object with string values, or null" }, 400);
812+
} else {
813+
for (const [tier, value] of Object.entries(body.tierModels)) {
814+
if (typeof value !== "string") return jsonResponse({ error: `tierModels.${tier} must be a string` }, 400);
815+
}
816+
const tierModels = body.tierModels as Record<string, string>;
817+
const tiers: Record<string, string> = {};
818+
for (const tier of ["opus", "sonnet", "haiku", "fable"] as const) {
819+
const value = tierModels[tier];
820+
if (value !== undefined && value.trim() !== "") tiers[tier] = value.trim();
821+
}
822+
if (Object.keys(tiers).length > 0) next.tierModels = tiers;
823+
else delete next.tierModels;
809824
}
810-
if (Object.keys(tiers).length > 0) next.tierModels = tiers;
811-
else delete next.tierModels;
812825
}
813826
if (body.fastMode !== undefined) {
814827
if (body.fastMode !== true && body.fastMode !== false && body.fastMode !== null) {
@@ -824,26 +837,38 @@ export async function handleManagementAPI(req: Request, url: URL, config: OcxCon
824837
else next[field] = value.trim();
825838
}
826839
if (body.modelMap !== undefined) {
827-
if (!body.modelMap || typeof body.modelMap !== "object" || Array.isArray(body.modelMap)) {
828-
return jsonResponse({ error: "modelMap must be an object of string->string" }, 400);
829-
}
830-
const map: Record<string, string> = {};
831-
for (const [k, v] of Object.entries(body.modelMap as Record<string, unknown>)) {
832-
if (typeof v !== "string" || k.trim() === "" || v.trim() === "") {
833-
return jsonResponse({ error: "modelMap entries must be non-empty strings" }, 400);
840+
if (body.modelMap === null) {
841+
delete next.modelMap;
842+
} else {
843+
if (!isPlainObject(body.modelMap)) {
844+
return jsonResponse({ error: "modelMap must be an object of string->string, or null" }, 400);
834845
}
835-
map[k.trim()] = v.trim();
846+
const map: Record<string, string> = {};
847+
for (const [k, v] of Object.entries(body.modelMap)) {
848+
if (typeof v !== "string" || k.trim() === "" || v.trim() === "") {
849+
return jsonResponse({ error: "modelMap entries must be non-empty strings" }, 400);
850+
}
851+
map[k.trim()] = v.trim();
852+
}
853+
if (Object.keys(map).length > 0) next.modelMap = map;
854+
else delete next.modelMap;
836855
}
837-
if (Object.keys(map).length > 0) next.modelMap = map;
838-
else delete next.modelMap;
839856
}
840857
config.claudeCode = next;
841858
const { saveConfig: save } = await import("../config");
842859
save(config);
860+
const warnings: string[] = [];
861+
if (body.systemEnv !== undefined) {
862+
try {
863+
await applySystemEnvToggle(config, config.port);
864+
} catch (err) {
865+
warnings.push(`Failed to apply system environment setting: ${err instanceof Error ? err.message : String(err)}`);
866+
}
867+
}
843868
// Keep the file-backed live registry symmetric: OFF prunes immediately, while
844869
// ON and config changes restore definitions without requiring a restart.
845870
await syncClaudeAgentDefsBestEffort();
846-
return jsonResponse({ ok: true, enabled: next.enabled !== false });
871+
return jsonResponse({ ok: true, enabled: next.enabled !== false, warnings });
847872
}
848873

849874
// Per-provider catalog allowlist (issue #52): when a provider has a non-empty selectedModels list,

0 commit comments

Comments
 (0)