mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-19 09:44:07 +00:00
fix(security): namespace sessions Map by keyId — close cross-key conversation collision (#86)
**Bug**: the sessions Map used the raw client-supplied conversationId string as
its key. Two callers with different API keys (or one anonymous + one
authenticated) using the same session_id="default" collided in the Map,
sharing a cli.js subprocess and conversation history — a cross-tenant context leak.
**Fix**: introduce _sessionKey(conversationId, keyName) → "${keyName}|${conversationId}".
Replace every sessions Map site (has / set / get / delete) with the namespaced key.
keyName comes from req._authKeyName (set by auth middleware). Anonymous callers
produce "anon|<id>"; admin produces "admin|<id>"; per-key callers produce
"<keyName>|<id>" — matching the convention used by cacheHash() for per-key cache
isolation (D1, v3.13.0).
**cli.js citation — not applicable**: This PR changes OCP-internal session lifecycle
bookkeeping (the sessions Map that OCP maintains to thread --resume flags across
requests). There is no corresponding cli.js operation to cite. ALIGNMENT.md Rule 2
(limiting OCP to operations cli.js performs) does not constrain in-process state
representation. Same pattern as #75.
**Scope of changes (server.mjs only)**:
- New helper: _sessionKey() — 3 lines after sessions Map declaration
- spawnClaudeProcess(): accepts keyName param; all 4 sessions Map calls → _sessionKey
- handleSessionFailure(): uses sessionKey (not raw conversationId) for sessions.delete
- callClaude(): accepts and forwards keyName to spawnClaudeProcess
- callClaudeStreaming(): forwards authInfo.keyName to spawnClaudeProcess
- Request handler: both callClaude() call sites pass req._authKeyName
- Cleanup interval + GET /health + GET /sessions: strip "keyName|" prefix before log/display
**Smoke test**:
- node --check server.mjs → SYNTAX OK
- npm test → 43/43 passed, 0 failed
- Hand-traced has/set/get/delete paths: cross-key collision absent ✓
Identified during privacy/security positioning brainstorm — `.claude/research/ocp-security-audit.md`
Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
+40
-22
@@ -202,25 +202,36 @@ const MODEL_MAP = Object.fromEntries([
|
|||||||
const MODELS = modelsConfig.models.map(m => ({ id: m.id, name: m.displayName }));
|
const MODELS = modelsConfig.models.map(m => ({ id: m.id, name: m.displayName }));
|
||||||
|
|
||||||
// ── Session management ──────────────────────────────────────────────────
|
// ── Session management ──────────────────────────────────────────────────
|
||||||
// Maps conversation IDs (from caller) to Claude CLI session UUIDs.
|
// Maps namespaced session keys to Claude CLI session UUIDs.
|
||||||
|
// Key format: "${keyName}|${conversationId}" — prevents cross-key collision
|
||||||
|
// when two callers (different API keys or anon + authenticated) use the same
|
||||||
|
// session_id string. Anonymous callers use "anon"; admin uses "admin".
|
||||||
// Enables --resume for multi-turn conversations, reducing token waste.
|
// Enables --resume for multi-turn conversations, reducing token waste.
|
||||||
const sessions = new Map(); // conversationId → { uuid, messageCount, lastUsed, model }
|
const sessions = new Map(); // `${keyName}|${conversationId}` → { uuid, messageCount, lastUsed, model }
|
||||||
|
|
||||||
|
// Build the namespaced key used for all sessions Map operations.
|
||||||
|
// Returns null when conversationId is falsy (one-off requests bypass session tracking).
|
||||||
|
function _sessionKey(conversationId, keyName) {
|
||||||
|
return conversationId ? `${keyName || "anon"}|${conversationId}` : null;
|
||||||
|
}
|
||||||
|
|
||||||
const sessionCleanupInterval = setInterval(() => {
|
const sessionCleanupInterval = setInterval(() => {
|
||||||
const now = Date.now();
|
const now = Date.now();
|
||||||
for (const [id, s] of sessions) {
|
for (const [id, s] of sessions) {
|
||||||
const idleMs = now - s.lastUsed;
|
const idleMs = now - s.lastUsed;
|
||||||
const ageMs = s.firstSeen ? now - s.firstSeen : null;
|
const ageMs = s.firstSeen ? now - s.firstSeen : null;
|
||||||
|
// id is "${keyName}|${conversationId}"; strip prefix for log output
|
||||||
|
const convIdShort = id.includes("|") ? id.slice(id.indexOf("|") + 1, id.indexOf("|") + 13) : id.slice(0, 12);
|
||||||
if (idleMs > SESSION_TTL) {
|
if (idleMs > SESSION_TTL) {
|
||||||
sessions.delete(id);
|
sessions.delete(id);
|
||||||
console.log(`[session] expired ${id.slice(0, 12)}... (idle ${Math.round(idleMs / 60000)}m)`);
|
console.log(`[session] expired ${convIdShort}... (idle ${Math.round(idleMs / 60000)}m)`);
|
||||||
logEvent("info", "session_expired", { conversationId: id.slice(0, 12) + "...", idleMs, ageMs });
|
logEvent("info", "session_expired", { conversationId: convIdShort + "...", idleMs, ageMs });
|
||||||
} else if (ageMs !== null && ageMs > 4 * SESSION_TTL) {
|
} else if (ageMs !== null && ageMs > 4 * SESSION_TTL) {
|
||||||
// #42 evidence-gathering: a session whose firstSeen is more than 4× TTL old
|
// #42 evidence-gathering: a session whose firstSeen is more than 4× TTL old
|
||||||
// but whose lastUsed keeps getting bumped (never idle long enough to expire)
|
// but whose lastUsed keeps getting bumped (never idle long enough to expire)
|
||||||
// is the suspected bug. Log without action so the pattern can be confirmed
|
// is the suspected bug. Log without action so the pattern can be confirmed
|
||||||
// in /logs. Do NOT enforce an absolute age cap here speculatively.
|
// in /logs. Do NOT enforce an absolute age cap here speculatively.
|
||||||
logEvent("warn", "session_long_lived", { conversationId: id.slice(0, 12) + "...", idleMs, ageMs });
|
logEvent("warn", "session_long_lived", { conversationId: convIdShort + "...", idleMs, ageMs });
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}, 60000);
|
}, 60000);
|
||||||
@@ -427,7 +438,7 @@ function getModelTier(cliModel) {
|
|||||||
// ── Spawn claude CLI (shared setup) ─────────────────────────────────────
|
// ── Spawn claude CLI (shared setup) ─────────────────────────────────────
|
||||||
// Resolves session logic, builds CLI args, spawns the process, and sets up
|
// Resolves session logic, builds CLI args, spawns the process, and sets up
|
||||||
// timeouts. Returns context object or throws synchronously.
|
// timeouts. Returns context object or throws synchronously.
|
||||||
function spawnClaudeProcess(model, messages, conversationId) {
|
function spawnClaudeProcess(model, messages, conversationId, keyName) {
|
||||||
if (stats.activeRequests >= MAX_CONCURRENT) {
|
if (stats.activeRequests >= MAX_CONCURRENT) {
|
||||||
throw new Error(`concurrency limit reached (${stats.activeRequests}/${MAX_CONCURRENT})`);
|
throw new Error(`concurrency limit reached (${stats.activeRequests}/${MAX_CONCURRENT})`);
|
||||||
}
|
}
|
||||||
@@ -443,8 +454,11 @@ function spawnClaudeProcess(model, messages, conversationId) {
|
|||||||
let prompt;
|
let prompt;
|
||||||
|
|
||||||
// ── Session logic ──
|
// ── Session logic ──
|
||||||
if (conversationId && sessions.has(conversationId)) {
|
// sessionKey namespaces the Map key by keyName to prevent cross-caller collision
|
||||||
const session = sessions.get(conversationId);
|
// when two callers with different API keys share the same conversationId string.
|
||||||
|
const sessionKey = _sessionKey(conversationId, keyName);
|
||||||
|
if (sessionKey && sessions.has(sessionKey)) {
|
||||||
|
const session = sessions.get(sessionKey);
|
||||||
session.lastUsed = Date.now();
|
session.lastUsed = Date.now();
|
||||||
sessionInfo = { uuid: session.uuid, resume: true };
|
sessionInfo = { uuid: session.uuid, resume: true };
|
||||||
stats.sessionHits++;
|
stats.sessionHits++;
|
||||||
@@ -455,17 +469,17 @@ function spawnClaudeProcess(model, messages, conversationId) {
|
|||||||
: "";
|
: "";
|
||||||
session.messageCount = messages.length;
|
session.messageCount = messages.length;
|
||||||
|
|
||||||
console.log(`[session] resume conv=${conversationId.slice(0, 12)}... uuid=${session.uuid.slice(0, 8)}... msgs=${messages.length} prompt_chars=${prompt.length}`);
|
console.log(`[session] resume conv=${conversationId.slice(0, 12)}... key=${keyName || "anon"} uuid=${session.uuid.slice(0, 8)}... msgs=${messages.length} prompt_chars=${prompt.length}`);
|
||||||
|
|
||||||
} else if (conversationId) {
|
} else if (sessionKey) {
|
||||||
const uuid = randomUUID();
|
const uuid = randomUUID();
|
||||||
const now = Date.now();
|
const now = Date.now();
|
||||||
sessions.set(conversationId, { uuid, messageCount: messages.length, firstSeen: now, lastUsed: now, model: cliModel });
|
sessions.set(sessionKey, { uuid, messageCount: messages.length, firstSeen: now, lastUsed: now, model: cliModel });
|
||||||
sessionInfo = { uuid, resume: false };
|
sessionInfo = { uuid, resume: false };
|
||||||
stats.sessionMisses++;
|
stats.sessionMisses++;
|
||||||
prompt = messagesToPrompt(messages);
|
prompt = messagesToPrompt(messages);
|
||||||
|
|
||||||
console.log(`[session] new conv=${conversationId.slice(0, 12)}... uuid=${uuid.slice(0, 8)}... msgs=${messages.length}`);
|
console.log(`[session] new conv=${conversationId.slice(0, 12)}... key=${keyName || "anon"} uuid=${uuid.slice(0, 8)}... msgs=${messages.length}`);
|
||||||
|
|
||||||
} else {
|
} else {
|
||||||
stats.oneOffRequests++;
|
stats.oneOffRequests++;
|
||||||
@@ -510,11 +524,11 @@ function spawnClaudeProcess(model, messages, conversationId) {
|
|||||||
proc.once("exit", cleanup);
|
proc.once("exit", cleanup);
|
||||||
|
|
||||||
function handleSessionFailure() {
|
function handleSessionFailure() {
|
||||||
if (sessionInfo?.resume && conversationId) {
|
if (sessionInfo?.resume && sessionKey) {
|
||||||
console.warn(`[session] resume failed for ${conversationId.slice(0, 12)}..., removing stale session`);
|
console.warn(`[session] resume failed for ${conversationId.slice(0, 12)}..., removing stale session`);
|
||||||
logEvent("warn", "session_failure", { mode: "resume", conversationId: conversationId.slice(0, 12) + "...", action: "deleted" });
|
logEvent("warn", "session_failure", { mode: "resume", conversationId: conversationId.slice(0, 12) + "...", action: "deleted" });
|
||||||
sessions.delete(conversationId);
|
sessions.delete(sessionKey);
|
||||||
} else if (sessionInfo && !sessionInfo.resume && conversationId) {
|
} else if (sessionInfo && !sessionInfo.resume && sessionKey) {
|
||||||
// #41 evidence-gathering: session-create failures currently leave a stale entry
|
// #41 evidence-gathering: session-create failures currently leave a stale entry
|
||||||
// in the sessions map. Log without action so the staleness pattern can be
|
// in the sessions map. Log without action so the staleness pattern can be
|
||||||
// confirmed in /logs before any code change. Do NOT delete here speculatively.
|
// confirmed in /logs before any code change. Do NOT delete here speculatively.
|
||||||
@@ -557,11 +571,11 @@ function spawnClaudeProcess(model, messages, conversationId) {
|
|||||||
// On-demand spawning: each request spawns a fresh `claude -p` process.
|
// On-demand spawning: each request spawns a fresh `claude -p` process.
|
||||||
// No pool = no crash loops, no stale workers, no degraded states.
|
// No pool = no crash loops, no stale workers, no degraded states.
|
||||||
// Stdin is written immediately so there's no 3s stdin timeout issue.
|
// Stdin is written immediately so there's no 3s stdin timeout issue.
|
||||||
function callClaude(model, messages, conversationId) {
|
function callClaude(model, messages, conversationId, keyName) {
|
||||||
return new Promise((resolve, reject) => {
|
return new Promise((resolve, reject) => {
|
||||||
let ctx;
|
let ctx;
|
||||||
try {
|
try {
|
||||||
ctx = spawnClaudeProcess(model, messages, conversationId);
|
ctx = spawnClaudeProcess(model, messages, conversationId, keyName);
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
return reject(err);
|
return reject(err);
|
||||||
}
|
}
|
||||||
@@ -641,7 +655,7 @@ function callClaudeStreaming(model, messages, conversationId, res, authInfo = {}
|
|||||||
|
|
||||||
let ctx;
|
let ctx;
|
||||||
try {
|
try {
|
||||||
ctx = spawnClaudeProcess(model, messages, conversationId);
|
ctx = spawnClaudeProcess(model, messages, conversationId, authInfo.keyName);
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
return jsonResponse(res, 500, { error: { message: err.message, type: "proxy_error" } });
|
return jsonResponse(res, 500, { error: { message: err.message, type: "proxy_error" } });
|
||||||
}
|
}
|
||||||
@@ -1324,7 +1338,7 @@ async function handleChatCompletions(req, res) {
|
|||||||
// will re-read the freshly-populated cache entry here rather than spawning.
|
// will re-read the freshly-populated cache entry here rather than spawning.
|
||||||
const recheck = getCachedResponse(req._cacheHash, CACHE_TTL);
|
const recheck = getCachedResponse(req._cacheHash, CACHE_TTL);
|
||||||
if (recheck) return recheck.response;
|
if (recheck) return recheck.response;
|
||||||
const c = await callClaude(model, messages, conversationId);
|
const c = await callClaude(model, messages, conversationId, req._authKeyName);
|
||||||
try { setCachedResponse(req._cacheHash, model, c); } catch (e) { logEvent("error", "cache_write_failed", { error: e.message }); }
|
try { setCachedResponse(req._cacheHash, model, c); } catch (e) { logEvent("error", "cache_write_failed", { error: e.message }); }
|
||||||
return c;
|
return c;
|
||||||
});
|
});
|
||||||
@@ -1346,7 +1360,7 @@ async function handleChatCompletions(req, res) {
|
|||||||
|
|
||||||
// Fallback: cache disabled (CACHE_TTL=0) or no _cacheHash — original path untouched.
|
// Fallback: cache disabled (CACHE_TTL=0) or no _cacheHash — original path untouched.
|
||||||
try {
|
try {
|
||||||
const content = await callClaude(model, messages, conversationId);
|
const content = await callClaude(model, messages, conversationId, req._authKeyName);
|
||||||
const id = `chatcmpl-${randomUUID()}`;
|
const id = `chatcmpl-${randomUUID()}`;
|
||||||
completionResponse(res, id, model, content);
|
completionResponse(res, id, model, content);
|
||||||
try { recordUsage({ keyId: req._authKeyId, keyName: req._authKeyName, model, promptChars, responseChars: content.length, elapsedMs: Date.now() - t0Usage, success: true }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); }
|
try { recordUsage({ keyId: req._authKeyId, keyName: req._authKeyName, model, promptChars, responseChars: content.length, elapsedMs: Date.now() - t0Usage, success: true }); } catch (e) { logEvent("error", "usage_record_failed", { error: e.message }); }
|
||||||
@@ -1480,8 +1494,10 @@ const server = createServer(async (req, res) => {
|
|||||||
const uptimeMs = Date.now() - START_TIME;
|
const uptimeMs = Date.now() - START_TIME;
|
||||||
const sessionList = [];
|
const sessionList = [];
|
||||||
for (const [id, s] of sessions) {
|
for (const [id, s] of sessions) {
|
||||||
|
// id is "${keyName}|${conversationId}"; expose only the public-facing conversationId
|
||||||
|
const convId = id.includes("|") ? id.slice(id.indexOf("|") + 1) : id;
|
||||||
sessionList.push({
|
sessionList.push({
|
||||||
id: id.slice(0, 12) + "...",
|
id: convId.slice(0, 12) + "...",
|
||||||
model: s.model,
|
model: s.model,
|
||||||
messages: s.messageCount,
|
messages: s.messageCount,
|
||||||
idleMs: Date.now() - s.lastUsed,
|
idleMs: Date.now() - s.lastUsed,
|
||||||
@@ -1526,7 +1542,9 @@ const server = createServer(async (req, res) => {
|
|||||||
if (req.url === "/sessions" && req.method === "GET") {
|
if (req.url === "/sessions" && req.method === "GET") {
|
||||||
const list = [];
|
const list = [];
|
||||||
for (const [id, s] of sessions) {
|
for (const [id, s] of sessions) {
|
||||||
list.push({ id, uuid: s.uuid, model: s.model, messages: s.messageCount, lastUsed: new Date(s.lastUsed).toISOString() });
|
// id is "${keyName}|${conversationId}"; expose only the public-facing conversationId
|
||||||
|
const convId = id.includes("|") ? id.slice(id.indexOf("|") + 1) : id;
|
||||||
|
list.push({ id: convId, uuid: s.uuid, model: s.model, messages: s.messageCount, lastUsed: new Date(s.lastUsed).toISOString() });
|
||||||
}
|
}
|
||||||
return jsonResponse(res, 200, { sessions: list });
|
return jsonResponse(res, 200, { sessions: list });
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user