mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-21 21:15:09 +00:00
Three audit findings in the -p spawn-token resolution + HOME-isolation layer. All are infra/ process changes to how OCP READS and GATES an OAuth token it already holds; none touch the OAuth wire machinery. F3 (MEDIUM) — expiry-window fallback herds concurrent -p spawns into real HOME. When the keychain token is within 5 min of expiry, resolveSpawnToken() returns null and every concurrent spawn simultaneously falls back to the real HOME; each spawned claude then races a refresh_token grant against the SAME single-use refresh token — rotating it out from under the others and the operator's real claude (the credential-fork hazard, #112/#146 class). Fix: a promise-chain mutex (createSerialMutex) serializes ONLY the real-HOME fallback — one such spawn at a time. When a serialized waiter is admitted (prior holder torn down → its claude has refreshed the keychain), it re-runs resolveSpawnToken(): a now-fresh token means it proceeds ISOLATED instead of real-HOME, so the queue drains to the fast path. Isolated spawns never touch the mutex. F5 (LOW-MED) — per-spawn double keychain exec on the hot path. getOAuthCredentials() sync-exec'd `security find-generic-password` up to twice (wrong label first), worst case 5s×2, blocking the event loop and stalling in-flight SSE streams. Fix: (a) memoize the last-good keychain label and try it first (orderLabelsLastGoodFirst); (b) a 30s TTL cache of the read (createTtlCache). This does NOT reintroduce the #146 forever-memoized regression: the TTL bounds only how often we re-READ the keychain; resolveSpawnToken() still applies the 5-min expiry gate (isTokenExpiring) to the CACHED creds on EVERY use, so a token expiring within the window is still rejected → real-HOME fallback. Call sites stay synchronous (no async conversion). F6 (LOW) — memoized isolation decision goes stale; /health could misreport. getSpawnHomeMode() memoized the isolated/real-home decision forever: credentials appearing after startup never enabled isolation; deleting ~/.ocp/spawn-home at runtime ENOENT'd every isolated spawn until restart; during an expiry stint /health reported isolated:true while spawns ran real- HOME. Fix: re-evaluate the decision per spawn (cheap now that F5 caches the keychain read); ensureSpawnHome() re-verifies + re-prepares the scratch dir per isolated spawn; and /health now reports the EFFECTIVE decision (token presence AND expiry gate). The /health field set is UNCHANGED — no field added/removed/renamed — only the values are made truthful. Alignment: - Class: Not a wire/endpoint change for the spawn-token layer + Class B (B.2) for /health. - cli.js citation: DECLARED ABSENT. cli.js does NOT perform OCP's spawn-token resolution, HOME isolation, keychain caching, or fallback serialization — these are proxy-internal process concerns with no cli.js analogue, so no Class A cli.js:NNNN citation exists or is required (ALIGNMENT.md Rule 2: this is proxy infra, not an invented forwarded endpoint/header/body). - The /health change is authorized by ADR 0006 (grandfathered B.2 as of v3.16.4) and is a behaviour-preserving contract change: same fields, truthful values. - OCP still NEVER performs a refresh_token grant itself — that property is preserved; the fix only serializes/gates reads of a token refreshed by the spawned or real claude (#112). - No new endpoint, header, or request/response field. alignment.yml blacklist unaffected. Tests: extracted the pure primitives to lib/spawn-auth.mjs and added 11 unit tests (mutex serialization order + idempotent release; TTL cache freshness + null-miss; expiry gate; label ordering; and the combined invariant that the TTL cache respects the expiry gate). node --check clean; 249 tests pass (238 + 11). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
68 lines
3.2 KiB
JavaScript
68 lines
3.2 KiB
JavaScript
// Pure, dependency-injected primitives for the `-p` spawn-token resolution + HOME-isolation
|
|
// layer. Extracted from server.mjs (findings F3 / F5 / F6, 2026-07-07) so the concurrency,
|
|
// caching and expiry logic is unit-testable WITHOUT booting the server or mocking execFileSync /
|
|
// child_process.spawn / fs. server.mjs owns all I/O (macOS keychain exec, process spawn, fs);
|
|
// this module owns only pure decision logic.
|
|
//
|
|
// ALIGNMENT NOTE: none of this touches the OAuth wire machinery (no endpoint / header / body).
|
|
// OCP still NEVER performs a refresh_token grant itself — these helpers only READ + GATE a token
|
|
// that some other process (the operator's real claude, or a spawned claude under the real HOME)
|
|
// refreshes. That property is load-bearing (issue #112) and preserved.
|
|
|
|
// Promise-chain mutex. `acquire()` resolves to a `release()` fn; the NEXT `acquire()` does not
|
|
// resolve until the current holder calls its `release()`. Serializes async critical sections
|
|
// without busy-waiting. release() is idempotent.
|
|
export function createSerialMutex() {
|
|
let tail = Promise.resolve();
|
|
return {
|
|
acquire() {
|
|
let release;
|
|
const gate = new Promise((r) => { release = r; });
|
|
const prev = tail;
|
|
tail = tail.then(() => gate);
|
|
// Hand the caller its release fn only after the previous holder has released.
|
|
return prev.then(() => {
|
|
let released = false;
|
|
return function releaseMutex() { if (!released) { released = true; release(); } };
|
|
});
|
|
},
|
|
};
|
|
}
|
|
|
|
// Short-TTL memo. `get(produce, now)` returns the cached value while `now - storedAt < ttlMs`,
|
|
// otherwise calls `produce()` and re-stores. A miss that produces null/undefined is STILL stored
|
|
// (so a genuinely-absent source is not re-probed on every call within the TTL window). `now` is
|
|
// injectable for testing.
|
|
export function createTtlCache({ ttlMs }) {
|
|
let value;
|
|
let at = -Infinity;
|
|
let has = false;
|
|
return {
|
|
get(produce, now = Date.now()) {
|
|
if (has && now - at < ttlMs) return value;
|
|
value = produce();
|
|
at = now;
|
|
has = true;
|
|
return value;
|
|
},
|
|
clear() { has = false; value = undefined; at = -Infinity; },
|
|
};
|
|
}
|
|
|
|
// Pure expiry gate. Returns true when `creds` carries a known expiry that is at/within `bufferMs`
|
|
// of `now`. Creds WITHOUT `expiresAt` (e.g. long-lived env tokens) are never treated as expiring.
|
|
// This gate is applied to the CACHED creds on EVERY use — which is precisely why a short-TTL
|
|
// keychain cache (createTtlCache) cannot reintroduce the #146 forever-stale-token regression: the
|
|
// cache bounds how often we re-READ the keychain, but the expiry decision is recomputed per use.
|
|
export function isTokenExpiring(creds, now = Date.now(), bufferMs = 300000) {
|
|
return !!(creds && creds.expiresAt && now + bufferMs >= creds.expiresAt);
|
|
}
|
|
|
|
// Order candidate keychain labels so the last-known-good label is tried first (avoids the
|
|
// wrong-label miss that doubles the `security` exec count on the hot path). Pure: performs no
|
|
// read. Returns a fresh array; input is not mutated.
|
|
export function orderLabelsLastGoodFirst(labels, lastGood) {
|
|
if (!lastGood || !labels.includes(lastGood)) return labels.slice();
|
|
return [lastGood, ...labels.filter((l) => l !== lastGood)];
|
|
}
|