From 1be4eb20f97a76289ab06b71f11a4216f3e6e263 Mon Sep 17 00:00:00 2001 From: Rohan Poudel Date: Mon, 3 Aug 2026 01:16:14 -0600 Subject: [PATCH 1/2] fix(runtime): reclaim credential-home locks whose owner names no process `recoverStaleCredentialHomeLock` consulted `process.kill(pid, 0)` for any `owner.json` whose `pid` was a number. POSIX gives two of those numbers special meanings: 0 signals the caller's own process group and -1 every process it may signal, so both always succeed and always report a live owner. A fractional or out-of-range value makes `process.kill` throw `ERR_INVALID_ARG_TYPE`, which is neither ESRCH nor EPERM and was rethrown raw out of a public API. Because the age check sits in the `else` branch, a lock naming any of these values was also exempt from it, so `acquireCodexSecurityCredentialHomeLock` waited on it forever at a 25 ms poll with no message and no timeout. Only consult `process.kill` for a positive safe integer. Anything else is an owner that cannot be identified, and is now treated like a missing one, so the existing 30 s age check reclaims the lock. This does not address a genuinely reused pid, which needs a heartbeat rather than a liveness probe and is a larger design change. That part is described in the issue. Refs #228 --- sdk/typescript/src/runtime.ts | 15 ++++++++-- sdk/typescript/tests-ts/runtime.test.ts | 39 +++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/sdk/typescript/src/runtime.ts b/sdk/typescript/src/runtime.ts index 4a09a894..133c364a 100644 --- a/sdk/typescript/src/runtime.ts +++ b/sdk/typescript/src/runtime.ts @@ -418,9 +418,20 @@ async function recoverStaleCredentialHomeLock(lock: string): Promise { } } - if (isRecord(owner) && typeof owner["pid"] === "number") { + // Only a positive integer names a process. `process.kill` reads 0 as the caller's own + // process group and -1 as every process it may signal, so both always report a live + // owner and would hold the lock open forever, and a fractional or out-of-range value + // makes it throw an argument error that is neither ESRCH nor EPERM and escapes raw. + // An owner that cannot be identified is treated like a missing one, so the age check + // below still reclaims the lock. + const ownerPid = isRecord(owner) ? owner["pid"] : undefined; + if ( + typeof ownerPid === "number" && + Number.isSafeInteger(ownerPid) && + ownerPid > 0 + ) { try { - process.kill(owner["pid"], 0); + process.kill(ownerPid, 0); return false; } catch (error) { if (nodeErrorCode(error) !== "ESRCH") { diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index 7cb55da3..0b2cd8b5 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -15,6 +15,7 @@ import { stat, symlink, truncate, + utimes, writeFile, } from "node:fs/promises"; import * as fsPromises from "node:fs/promises"; @@ -1632,6 +1633,44 @@ describe("runtime directories and plugin Python boundary", () => { expect(existsSync(lock)).toBe(false); }); + test("recovers credential-home locks whose owner names no process", async () => { + const root = await temporaryDirectory(); + const home = await prepareCodexSecurityCredentialHome({ + CODEX_SECURITY_STATE_DIR: join(root, "state"), + }); + const lock = join(home, ".codex-security-scan.lock"); + // `process.kill` reads 0 as the caller's own process group and -1 as every process + // it may signal, so both report a live owner forever, and a fractional pid makes it + // throw an argument error instead. None of them identifies a process holding this + // lock, so an aged lock naming one has to be reclaimed like any other stale lock. + for (const pid of [0, -1, 0.5, 2 ** 53]) { + await mkdir(lock, { mode: 0o700 }); + await writeFile( + join(lock, "owner.json"), + `${JSON.stringify({ pid, token: "unidentifiable-owner" })}\n`, + { mode: 0o600 }, + ); + const aged = new Date(Date.now() - 10 * 60_000); + await utimes(lock, aged, aged); + + // An owner that is treated as live is waited on forever, so the acquisition is + // bounded here to fail the test rather than hang it. + const abort = new AbortController(); + const timer = setTimeout(() => abort.abort(), 5_000); + try { + const release = await acquireCodexSecurityCredentialHomeLock( + home, + abort.signal, + ); + expect(existsSync(lock)).toBe(true); + await release(); + } finally { + clearTimeout(timer); + } + expect(existsSync(lock)).toBe(false); + } + }); + test("prevents ambient credential imports after an explicit logout", async () => { const root = await temporaryDirectory(); const home = await prepareCodexSecurityCredentialHome({ From 31ea90afc7e82910acd3892e5957ae71c953ea16 Mon Sep 17 00:00:00 2001 From: Rohan Poudel Date: Mon, 3 Aug 2026 17:56:33 -0600 Subject: [PATCH 2/2] fix(runtime): bound owner pids to the range process.kill accepts `process.kill` narrows its pid argument to a 32-bit signed integer and throws ERR_INVALID_ARG_TYPE when the value does not survive that round trip. `Number.isSafeInteger` still admits positive integers above that range, such as 2147483648, so an aged `owner.json` naming one reached `process.kill`, and the argument error - being neither ESRCH nor EPERM - escaped `recoverStaleCredentialHomeLock` and failed the acquisition outright instead of reclaiming the malformed lock. Reject any owner pid above the maximum `process.kill` accepts so it is treated as unidentifiable, letting the age check reclaim the lock, and cover the gap between the pid range and the safe-integer range in the stale-lock test. --- sdk/typescript/src/runtime.ts | 20 ++++++++++++-------- sdk/typescript/tests-ts/runtime.test.ts | 5 +++-- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/sdk/typescript/src/runtime.ts b/sdk/typescript/src/runtime.ts index 133c364a..66f6baff 100644 --- a/sdk/typescript/src/runtime.ts +++ b/sdk/typescript/src/runtime.ts @@ -64,6 +64,9 @@ const CREDENTIAL_LOCK_NAME = ".codex-security-scan.lock"; const CREDENTIAL_LOGOUT_MARKER = ".codex-security-logged-out"; const CREDENTIAL_LOCK_POLL_MILLISECONDS = 25; const INCOMPLETE_CREDENTIAL_LOCK_MILLISECONDS = 30_000; +// `process.kill` narrows its pid to a 32-bit signed integer and rejects anything that +// does not survive the round trip, so a larger value can never name a process. +const MAX_PROCESS_ID = 2_147_483_647; export interface PluginInstall { pluginRoot: string; @@ -418,17 +421,18 @@ async function recoverStaleCredentialHomeLock(lock: string): Promise { } } - // Only a positive integer names a process. `process.kill` reads 0 as the caller's own - // process group and -1 as every process it may signal, so both always report a live - // owner and would hold the lock open forever, and a fractional or out-of-range value - // makes it throw an argument error that is neither ESRCH nor EPERM and escapes raw. - // An owner that cannot be identified is treated like a missing one, so the age check - // below still reclaims the lock. + // Only a positive integer within the pid range names a process. `process.kill` reads 0 + // as the caller's own process group and -1 as every process it may signal, so both + // always report a live owner and would hold the lock open forever, and a fractional or + // out-of-range value makes it throw an argument error that is neither ESRCH nor EPERM + // and escapes raw. An owner that cannot be identified is treated like a missing one, so + // the age check below still reclaims the lock. const ownerPid = isRecord(owner) ? owner["pid"] : undefined; if ( typeof ownerPid === "number" && - Number.isSafeInteger(ownerPid) && - ownerPid > 0 + Number.isInteger(ownerPid) && + ownerPid > 0 && + ownerPid <= MAX_PROCESS_ID ) { try { process.kill(ownerPid, 0); diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index 0b2cd8b5..1624dca2 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -1640,10 +1640,11 @@ describe("runtime directories and plugin Python boundary", () => { }); const lock = join(home, ".codex-security-scan.lock"); // `process.kill` reads 0 as the caller's own process group and -1 as every process - // it may signal, so both report a live owner forever, and a fractional pid makes it + // it may signal, so both report a live owner forever, and a fractional pid, a pid + // just past the signed 32-bit range, or one past the safe-integer range makes it // throw an argument error instead. None of them identifies a process holding this // lock, so an aged lock naming one has to be reclaimed like any other stale lock. - for (const pid of [0, -1, 0.5, 2 ** 53]) { + for (const pid of [0, -1, 0.5, 2 ** 31, 2 ** 53]) { await mkdir(lock, { mode: 0o700 }); await writeFile( join(lock, "owner.json"),