fix: request validation + OpenAI-compat correctness (#110) (#117)

Three correctness/compat defects on the request path:

1. Non-array `messages` (e.g. {"messages":"x"}) passed the `!messages?.length`
   guard but threw in `messages.reduce(...)`; since the handler runs without
   await, the rejection silently hung the client until socket timeout. Now
   guarded with `Array.isArray(messages) && length>0` → 400 invalid_request_error,
   placed before the first use of `messages`.

2. OpenAI array `content` ([{type:"text",text},{type:"image_url",...}]) was
   JSON.stringify'd into the prompt as literal noise. New `contentToText()` helper
   flattens text parts and replaces non-text parts with a placeholder; used in
   messagesToPrompt, extractSystemPrompt, and all promptChars char-count sites
   (zero `JSON.stringify(m.content)` remain).

3. A streamed upstream error arriving after eager SSE headers emitted a bare
   finish_reason:"stop" + [DONE] — byte-identical to a successful empty completion.
   Both streaming error paths (the parsed.error result branch AND the non-zero-exit
   close-handler branch — the latter folded in per reviewer) now emit an SSE
   `data:{"error":{...}}` frame so clients can distinguish failure from empty.

Error-text sanitization across all emit sites is intentionally deferred to #110's
sibling issue #111 (security layer).

ALIGNMENT.md: these are OpenAI-compat shim + input-validation behaviors; OCP does
not forward, add, or alter any Anthropic API operation here (cli.js does not speak
the OpenAI wire format), so a cli.js citation is N/A under Rule 2. No blacklisted
tokens or port literals introduced; alignment.yml passes.

Independent fresh-context reviewer (opus): APPROVE WITH MINOR (Iron Rule 10) —
the close-handler sibling fold-in addresses MINOR #1; this commit body addresses
MINOR #2.

Closes #110.

Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dtzp555-max
2026-05-31 22:22:41 +10:00
committed by GitHub
co-authored by taodeng Claude Opus 4.8
parent 36be723198
commit 4458490caa
2 changed files with 90 additions and 15 deletions
+30 -15
View File
@@ -144,7 +144,7 @@ function extractSystemPrompt(messages) {
return OCP_SYSTEM_PROMPT_WRAPPER; return OCP_SYSTEM_PROMPT_WRAPPER;
} }
const clientContent = systemMessages.map(m => const clientContent = systemMessages.map(m =>
typeof m.content === "string" ? m.content : JSON.stringify(m.content) contentToText(m.content)
).join("\n\n"); ).join("\n\n");
return `${OCP_SYSTEM_PROMPT_WRAPPER}\n\n${clientContent}`; return `${OCP_SYSTEM_PROMPT_WRAPPER}\n\n${clientContent}`;
} }
@@ -636,9 +636,22 @@ function buildCliArgs(cliModel, systemPrompt) {
// This prevents runaway context from gateway-side conversation accumulation. // This prevents runaway context from gateway-side conversation accumulation.
let MAX_PROMPT_CHARS = parseInt(process.env.CLAUDE_MAX_PROMPT_CHARS || "150000", 10); let MAX_PROMPT_CHARS = parseInt(process.env.CLAUDE_MAX_PROMPT_CHARS || "150000", 10);
// Flatten OpenAI content (string | array of parts) to plain text for the prompt.
// Array content: concatenate text parts; replace non-text parts (e.g. image_url)
// with a placeholder rather than dumping raw JSON. (issue #110)
function contentToText(content) {
if (typeof content === "string") return content;
if (Array.isArray(content)) {
return content.map(p =>
p && p.type === "text" && typeof p.text === "string" ? p.text : "[non-text content omitted]"
).join("");
}
return content == null ? "" : JSON.stringify(content);
}
function messagesToPrompt(messages) { function messagesToPrompt(messages) {
const full = messages.map((m) => { const full = messages.map((m) => {
const text = typeof m.content === "string" ? m.content : JSON.stringify(m.content); const text = contentToText(m.content);
if (m.role === "system") return `[System] ${text}`; if (m.role === "system") return `[System] ${text}`;
if (m.role === "assistant") return `[Assistant] ${text}`; if (m.role === "assistant") return `[Assistant] ${text}`;
return text; return text;
@@ -1045,10 +1058,10 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {}
if (!headersSent && !res.writableEnded && !res.destroyed) { if (!headersSent && !res.writableEnded && !res.destroyed) {
jsonResponse(res, 500, { error: { message: errStr, type: "provider_error" } }); jsonResponse(res, 500, { error: { message: errStr, type: "provider_error" } });
} else if (!res.writableEnded && !res.destroyed) { } else if (!res.writableEnded && !res.destroyed) {
sendSSE(res, { // Headers already sent (eager ensureHeaders) — can't send a JSON 500. Surface the
id, object: "chat.completion.chunk", created, model, // failure as an SSE error frame so the client can distinguish an upstream error
choices: [{ index: 0, delta: {}, finish_reason: "stop" }], // from a legitimately empty completion, instead of a success-looking finish_reason:"stop". (issue #110)
}, hb); sendSSE(res, { error: { message: errStr, type: "provider_error" } }, hb);
res.write("data: [DONE]\n\n"); res.write("data: [DONE]\n\n");
res.end(); res.end();
} }
@@ -1070,7 +1083,7 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {}
// never record success or write cache for an errored response. // never record success or write cache for an errored response.
if ((code !== 0 && !resultEventSeen) || errored) { if ((code !== 0 && !resultEventSeen) || errored) {
recordModelError(cliModel, false); recordModelError(cliModel, false);
try { recordUsage({ keyId: authInfo.keyId, keyName: authInfo.keyName, model, promptChars: messages.reduce((a, m) => a + (typeof m.content === "string" ? m.content.length : JSON.stringify(m.content).length), 0), responseChars: 0, elapsedMs: elapsed, success: false }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); } try { recordUsage({ keyId: authInfo.keyId, keyName: authInfo.keyName, model, promptChars: messages.reduce((a, m) => a + contentToText(m.content).length, 0), responseChars: 0, elapsedMs: elapsed, success: false }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); }
logEvent("error", "claude_exit", { model: cliModel, code, signal: signal || "none", elapsed, errored, stderr: stderr.slice(0, 300) }); logEvent("error", "claude_exit", { model: cliModel, code, signal: signal || "none", elapsed, errored, stderr: stderr.slice(0, 300) });
trackError(stderr.slice(0, 300) || `claude exit ${code}`); trackError(stderr.slice(0, 300) || `claude exit ${code}`);
handleSessionFailure(); handleSessionFailure();
@@ -1080,17 +1093,17 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {}
if (!headersSent && !res.writableEnded && !res.destroyed) { if (!headersSent && !res.writableEnded && !res.destroyed) {
jsonResponse(res, 500, { error: { message: stderr.slice(0, 300) || `claude exit ${code}`, type: "proxy_error" } }); jsonResponse(res, 500, { error: { message: stderr.slice(0, 300) || `claude exit ${code}`, type: "proxy_error" } });
} else if (!res.writableEnded && !res.destroyed) { } else if (!res.writableEnded && !res.destroyed) {
sendSSE(res, { // Headers already sent — surface the failure as an SSE error frame instead of a
id, object: "chat.completion.chunk", created, model, // success-looking finish_reason:"stop", so the client can tell the upstream crashed
choices: [{ index: 0, delta: {}, finish_reason: "stop" }], // rather than returned empty. (issue #110 — sibling of the parsed.error branch above.)
}, hb); sendSSE(res, { error: { message: stderr.slice(0, 300) || `claude exit ${code}`, type: "proxy_error" } }, hb);
res.write("data: [DONE]\n\n"); res.write("data: [DONE]\n\n");
res.end(); res.end();
} }
} else { } else {
recordModelSuccess(cliModel, elapsed); recordModelSuccess(cliModel, elapsed);
breakerRecordSuccess(cliModel); breakerRecordSuccess(cliModel);
try { recordUsage({ keyId: authInfo.keyId, keyName: authInfo.keyName, model, promptChars: messages.reduce((a, m) => a + (typeof m.content === "string" ? m.content.length : JSON.stringify(m.content).length), 0), responseChars: totalChars, elapsedMs: elapsed, success: true }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); } try { recordUsage({ keyId: authInfo.keyId, keyName: authInfo.keyName, model, promptChars: messages.reduce((a, m) => a + contentToText(m.content).length, 0), responseChars: totalChars, elapsedMs: elapsed, success: true }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); }
logEvent("info", "claude_ok", { model: cliModel, chars: totalChars, elapsed, session: convId ? convId.slice(0, 12) + "..." : "none" }); logEvent("info", "claude_ok", { model: cliModel, chars: totalChars, elapsed, session: convId ? convId.slice(0, 12) + "..." : "none" });
// Cache write-back for streaming — only on true success (not errored) // Cache write-back for streaming — only on true success (not errored)
if (CACHE_TTL > 0 && authInfo.cacheHash) { if (CACHE_TTL > 0 && authInfo.cacheHash) {
@@ -1647,7 +1660,9 @@ async function handleChatCompletions(req, res) {
// Session ID: from request body, header, or null (one-off) // Session ID: from request body, header, or null (one-off)
const conversationId = parsed.session_id || parsed.conversation_id || req.headers["x-session-id"] || req.headers["x-conversation-id"] || null; const conversationId = parsed.session_id || parsed.conversation_id || req.headers["x-session-id"] || req.headers["x-conversation-id"] || null;
if (!messages?.length) return jsonResponse(res, 400, { error: "messages required" }); if (!Array.isArray(messages) || messages.length === 0) {
return jsonResponse(res, 400, { error: { message: "'messages' must be a non-empty array", type: "invalid_request_error" } });
}
// Quota check — only for identified per-key users (not anonymous/admin/local) // Quota check — only for identified per-key users (not anonymous/admin/local)
if (req._authKeyId) { if (req._authKeyId) {
@@ -1703,7 +1718,7 @@ async function handleChatCompletions(req, res) {
// Default path (TUI_MODE===false) falls through to callClaudeStreaming below, // Default path (TUI_MODE===false) falls through to callClaudeStreaming below,
// which is byte-for-byte unchanged from before this gate was added. // which is byte-for-byte unchanged from before this gate was added.
const t0TuiStream = Date.now(); const t0TuiStream = Date.now();
const promptCharsTuiStream = messages.reduce((a, m) => a + (typeof m.content === "string" ? m.content.length : JSON.stringify(m.content).length), 0); const promptCharsTuiStream = messages.reduce((a, m) => a + contentToText(m.content).length, 0);
try { try {
const content = await callClaudeTui(model, messages, conversationId, req._authKeyName); const content = await callClaudeTui(model, messages, conversationId, req._authKeyName);
if (CACHE_TTL > 0 && req._cacheHash) { if (CACHE_TTL > 0 && req._cacheHash) {
@@ -1724,7 +1739,7 @@ async function handleChatCompletions(req, res) {
} }
const t0Usage = Date.now(); const t0Usage = Date.now();
const promptChars = messages.reduce((a, m) => a + (typeof m.content === "string" ? m.content.length : JSON.stringify(m.content).length), 0); const promptChars = messages.reduce((a, m) => a + contentToText(m.content).length, 0);
// Select upstream based on TUI_MODE flag. With TUI_MODE===false (default), // Select upstream based on TUI_MODE flag. With TUI_MODE===false (default),
// upstreamCall===callClaude — identical to the pre-TUI code path. // upstreamCall===callClaude — identical to the pre-TUI code path.
+60
View File
@@ -1563,6 +1563,66 @@ test("(localhost=true, flag=true) → include key (both true)", () => {
assert.equal(shouldAdvertiseAnonKey(true, true), true); assert.equal(shouldAdvertiseAnonKey(true, true), true);
}); });
// ── contentToText helper tests (issue #110) ──────────────────────────────────
// MIRRORS server.mjs contentToText — copied verbatim to avoid importing server.mjs
// (top-level server.listen() would start a live HTTP server).
// Keep in sync with the definition in server.mjs above messagesToPrompt.
console.log("\ncontentToText helper (issue #110):");
function contentToText(content) {
if (typeof content === "string") return content;
if (Array.isArray(content)) {
return content.map(p =>
p && p.type === "text" && typeof p.text === "string" ? p.text : "[non-text content omitted]"
).join("");
}
return content == null ? "" : JSON.stringify(content);
}
test("contentToText: string input returned unchanged", () => {
assert.equal(contentToText("hello"), "hello");
});
test("contentToText: array of text parts concatenated", () => {
assert.equal(
contentToText([{ type: "text", text: "hello" }, { type: "text", text: " world" }]),
"hello world"
);
});
test("contentToText: non-text part (image_url) replaced with placeholder", () => {
assert.equal(
contentToText([{ type: "image_url", image_url: { url: "https://example.com/img.png" } }]),
"[non-text content omitted]"
);
});
test("contentToText: empty array returns empty string", () => {
assert.equal(contentToText([]), "");
});
test("contentToText: null returns empty string", () => {
assert.equal(contentToText(null), "");
});
// ── messages guard predicate truth-table (issue #110) ────────────────────────
// Mirrors the guard at server.mjs line ~1650: Array.isArray(x) && x.length > 0
console.log("\nmessages guard predicate (issue #110):");
function isValidMessages(x) { return Array.isArray(x) && x.length > 0; }
test("messages guard: string 'x' → invalid (non-array)", () => {
assert.equal(isValidMessages("x"), false);
});
test("messages guard: empty array [] → invalid", () => {
assert.equal(isValidMessages([]), false);
});
test("messages guard: [{role:'user',content:'hi'}] → valid", () => {
assert.equal(isValidMessages([{ role: "user", content: "hi" }]), true);
});
// ── Cleanup ── // ── Cleanup ──
closeDb(); closeDb();