From 6e0e19c3b853334c9168f02b6a32563c9f41d5c8 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Sat, 15 Aug 2026 12:08:29 -0700 Subject: [PATCH 1/3] fix(sdk): keep restored scan artifacts within their output directory --- sdk/typescript/src/api.ts | 11 +++++++ sdk/typescript/tests-ts/api-post-scan.test.ts | 30 +++++++++++++++---- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/sdk/typescript/src/api.ts b/sdk/typescript/src/api.ts index 3173ce202..319486093 100644 --- a/sdk/typescript/src/api.ts +++ b/sdk/typescript/src/api.ts @@ -1238,6 +1238,17 @@ export class CodexSecurity { if (signal.aborted || this.#closed) throw error; for (const artifact of completedArtifacts) { const path = join(scanDir, artifact.name); + const parent = await realpath(dirname(path)); + const relativeParent = relative(scanDir, parent); + if ( + relativeParent === ".." || + relativeParent.startsWith(`..${sep}`) || + isAbsolute(relativeParent) + ) { + throw new OutputDirectoryError( + "Cannot restore an artifact outside the scan directory.", + ); + } const current = await readFile(path, { signal }).catch( (readError: NodeJS.ErrnoException) => { if (readError.code !== "ENOENT") throw readError; diff --git a/sdk/typescript/tests-ts/api-post-scan.test.ts b/sdk/typescript/tests-ts/api-post-scan.test.ts index c60563558..0dbb0ddf8 100644 --- a/sdk/typescript/tests-ts/api-post-scan.test.ts +++ b/sdk/typescript/tests-ts/api-post-scan.test.ts @@ -1,5 +1,5 @@ import { createHash } from "node:crypto"; -import { mkdir, readFile, rm, writeFile } from "node:fs/promises"; +import { mkdir, readFile, rm, symlink, writeFile } from "node:fs/promises"; import { dirname, join } from "node:path"; import type { ThreadEvent } from "@openai/codex-sdk"; import { afterEach, describe, expect, test } from "bun:test"; @@ -46,13 +46,15 @@ describe("completed scan follow-up instructions", () => { ["partial report", "report.md", "# Incomplete draft\n"], ["invalid findings", "findings.json", "{invalid"], ["sealed nested artifact", "artifacts/worker.json", '{"partial":true}'], + ["replaced artifact parent", "artifacts/worker.json", null], ] as const)( - "restores completed scan artifacts damaged by post-scan instructions: %s", + "handles completed scan artifacts after post-scan instructions: %s", async (_scenario, artifact, replacement) => { const root = await temporaryDirectory(); const repository = join(root, "repository"); const codexHome = join(root, "codex-home"); const scanDir = join(root, "scan"); + const outside = join(root, "outside"); await mkdir(repository); await mkdir(codexHome); await mkdir(scanDir, { mode: 0o700 }); @@ -95,7 +97,16 @@ describe("completed scan follow-up instructions", () => { return { events: completedEvents() }; } const artifactPath = join(scanDir, artifact); - if (replacement === undefined) await rm(artifactPath); + if (replacement === null) { + await mkdir(outside); + await writeFile(join(outside, "worker.json"), "untouched\n"); + await rm(dirname(artifactPath), { recursive: true }); + await symlink( + outside, + dirname(artifactPath), + process.platform === "win32" ? "junction" : "dir", + ); + } else if (replacement === undefined) await rm(artifactPath); else await writeFile(artifactPath, replacement); async function* failedEvents(): AsyncGenerator { yield { @@ -110,11 +121,18 @@ describe("completed scan follow-up instructions", () => { }, ); - const result = await client.run(repository, { + const scan = client.run(repository, { postScanPrompt: "Draft confirmed fixes.", }); - expect(result).toMatchObject({ scanDir }); - expect(await readFile(join(scanDir, artifact))).toEqual(original); + if (replacement === null) { + await expect(scan).rejects.toThrow("scan directory"); + expect(await readFile(join(outside, "worker.json"), "utf8")).toBe( + "untouched\n", + ); + } else { + expect(await scan).toMatchObject({ scanDir }); + expect(await readFile(join(scanDir, artifact))).toEqual(original); + } await client.close(); }, ); From 4ca4c82b4f14ede84e666742c52639391a127384 Mon Sep 17 00:00:00 2001 From: mldangelo-oai <269034524+mldangelo-oai@users.noreply.github.com> Date: Sun, 16 Aug 2026 00:03:03 -0700 Subject: [PATCH 2/3] fix(sdk): validate restored artifact directory chains --- sdk/typescript/src/api.ts | 20 ++++++---- sdk/typescript/src/contract.ts | 69 +++++++++++++++++++++++++++++----- 2 files changed, 71 insertions(+), 18 deletions(-) diff --git a/sdk/typescript/src/api.ts b/sdk/typescript/src/api.ts index 0bdd2ba34..16664e5ad 100644 --- a/sdk/typescript/src/api.ts +++ b/sdk/typescript/src/api.ts @@ -40,6 +40,7 @@ import { import { estimateScanCost, ScanCostTracker, type ScanCost } from "./cost.js"; import { loadContract, + requireScanArtifactPath, requireScanFile, type ScanExpectation, } from "./contract.js"; @@ -1240,16 +1241,19 @@ export class CodexSecurity { } catch (error) { if (signal.aborted || this.#closed) throw error; for (const artifact of completedArtifacts) { - const path = join(scanDir, artifact.name); - const parent = await realpath(dirname(path)); - const relativeParent = relative(scanDir, parent); - if ( - relativeParent === ".." || - relativeParent.startsWith(`..${sep}`) || - isAbsolute(relativeParent) - ) { + let path: string; + try { + path = await requireScanArtifactPath( + scanDir, + artifact.name, + artifact.name, + signal, + ); + } catch (cause) { + if (signal.aborted || this.#closed) throw cause; throw new OutputDirectoryError( "Cannot restore an artifact outside the scan directory.", + { cause }, ); } const current = await readFile(path, { signal }).catch( diff --git a/sdk/typescript/src/contract.ts b/sdk/typescript/src/contract.ts index 559f17a1e..94bfd4245 100644 --- a/sdk/typescript/src/contract.ts +++ b/sdk/typescript/src/contract.ts @@ -38,12 +38,15 @@ const SAFE_SCHEMA_ERROR_PROPERTIES = new Set([ "coverage", "scope", ]); -interface CheckedScanFile { +interface CheckedScanPath { path: string; - metadata: Stats; parents: Array<{ path: string; metadata: Stats }>; } +interface CheckedScanFile extends CheckedScanPath { + metadata: Stats; +} + interface ScanRoot { path: string; metadata: Stats; @@ -291,14 +294,31 @@ export async function requireScanFile( ).path; } -async function requireCheckedScanFile( +/** Check parents under a canonical scan root; the artifact may be missing. */ +export async function requireScanArtifactPath( scanDirectory: string, relativePath: string, context: string, signal?: AbortSignal, - expectedRoot?: ScanRoot, -): Promise { +): Promise { const checkedRoot = await requireScanRoot(scanDirectory, signal); + if (checkedRoot.path !== scanDirectory) { + throw new ContractValidationError( + "Scan directory changed before artifact restoration.", + ); + } + return ( + await requireCheckedScanPath(checkedRoot, relativePath, context, signal) + ).path; +} + +async function requireCheckedScanPath( + checkedRoot: ScanRoot, + relativePath: string, + context: string, + signal?: AbortSignal, + expectedRoot?: ScanRoot, +): Promise { const scanDir = checkedRoot.path; throwIfAborted(signal); const safePath = safeRelativePath(relativePath, context); @@ -331,20 +351,49 @@ async function requireCheckedScanFile( } parents.push({ path: current, metadata }); } - const path = join(scanDir, ...parts); - const metadata = await lstat(path); + return { path: join(scanDir, ...parts), parents }; + } catch (error) { + throwIfAborted(signal); + if (error instanceof ContractValidationError) { + throw error; + } + throw new ContractValidationError( + `${context}: expected a file inside the scan directory.`, + { + cause: error, + }, + ); + } +} + +async function requireCheckedScanFile( + scanDirectory: string, + relativePath: string, + context: string, + signal?: AbortSignal, + expectedRoot?: ScanRoot, +): Promise { + const checked = await requireCheckedScanPath( + await requireScanRoot(scanDirectory, signal), + relativePath, + context, + signal, + expectedRoot, + ); + try { + const metadata = await lstat(checked.path); throwIfAborted(signal); if (metadata.isSymbolicLink() || !metadata.isFile()) { throw new ContractValidationError( `${context}: expected a regular non-symlink file.`, ); } - const canonical = await realpath(path); + const canonical = await realpath(checked.path); throwIfAborted(signal); - if (!isContained(scanDir, canonical)) { + if (!isContained(checked.parents[0]!.path, canonical)) { throw new Error("outside scan directory"); } - return { path, metadata, parents }; + return { ...checked, metadata }; } catch (error) { throwIfAborted(signal); if (error instanceof ContractValidationError) { From e8fc845591364b41ac8da88462098e90852df680 Mon Sep 17 00:00:00 2001 From: mldangelo-oai <269034524+mldangelo-oai@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:48:43 -0700 Subject: [PATCH 3/3] fix(runtime): handle Windows credential lock contention --- sdk/typescript/src/runtime.ts | 42 ++- sdk/typescript/tests-ts/api.test.ts | 3 +- sdk/typescript/tests-ts/runtime.test.ts | 347 +++++++++++++++++++++++- 3 files changed, 385 insertions(+), 7 deletions(-) diff --git a/sdk/typescript/src/runtime.ts b/sdk/typescript/src/runtime.ts index 13c41f2c2..1646628b8 100644 --- a/sdk/typescript/src/runtime.ts +++ b/sdk/typescript/src/runtime.ts @@ -996,6 +996,10 @@ async function secureWindowsCredentialHome(path: string): Promise { } } +interface CredentialLockReadRetry { + deadline?: number; +} + export async function acquireCodexSecurityCredentialHomeLock( codexHome: string, signal?: AbortSignal, @@ -1010,9 +1014,11 @@ export async function acquireCodexSecurityCredentialHomeLock( ); const expectedDevice = homeMetadata.dev; const expectedInode = homeMetadata.ino; + const platform = securityOptions.platform ?? process.platform; const lock = join(codexHome, CREDENTIAL_LOCK_NAME); const ownerPath = join(lock, "owner.json"); const token = randomUUID(); + const readRetry: CredentialLockReadRetry = {}; while (true) { throwIfSignalAborted(signal); @@ -1027,10 +1033,12 @@ export async function acquireCodexSecurityCredentialHomeLock( throw error; }); if (existingLock !== null) { - if (await recoverStaleCredentialHomeLock(lock)) continue; + if (await recoverStaleCredentialHomeLock(lock, platform, readRetry)) + continue; await delay(CREDENTIAL_LOCK_POLL_MILLISECONDS, undefined, { signal }); continue; } + delete readRetry.deadline; await requireSecureCredentialHome(codexHome, { ...securityOptions, expectedDevice, @@ -1040,7 +1048,8 @@ export async function acquireCodexSecurityCredentialHomeLock( await mkdir(lock, { mode: 0o700 }); } catch (error) { if (nodeErrorCode(error) !== "EEXIST") throw error; - if (await recoverStaleCredentialHomeLock(lock)) continue; + if (await recoverStaleCredentialHomeLock(lock, platform, readRetry)) + continue; await delay(CREDENTIAL_LOCK_POLL_MILLISECONDS, undefined, { signal }); continue; } @@ -1078,12 +1087,19 @@ export async function acquireCodexSecurityCredentialHomeLock( } } -async function recoverStaleCredentialHomeLock(lock: string): Promise { +async function recoverStaleCredentialHomeLock( + lock: string, + platform: NodeJS.Platform, + readRetry: CredentialLockReadRetry, +): Promise { const metadata = await lstat(lock).catch((error: unknown) => { if (nodeErrorCode(error) === "ENOENT") return null; throw error; }); - if (metadata === null) return true; + if (metadata === null) { + delete readRetry.deadline; + return true; + } if (!metadata.isDirectory() || metadata.isSymbolicLink()) { throw new OutputDirectoryError( `Codex Security credential-home lock is not a directory: ${lock}`, @@ -1091,10 +1107,25 @@ async function recoverStaleCredentialHomeLock(lock: string): Promise { } let owner: unknown; + const deadline = + platform === "win32" + ? (readRetry.deadline ??= + Date.now() + INCOMPLETE_CREDENTIAL_LOCK_MILLISECONDS) + : undefined; try { owner = JSON.parse(await readFile(join(lock, "owner.json"), "utf8")); } catch (error) { - if (nodeErrorCode(error) !== "ENOENT" && !(error instanceof SyntaxError)) { + const code = nodeErrorCode(error); + // Re-enter the acquisition loop so a Windows read retry rechecks the lock. + if ( + deadline !== undefined && + (code === "EPERM" || code === "EBUSY") && + Date.now() < deadline + ) { + return false; + } + delete readRetry.deadline; + if (code !== "ENOENT" && !(error instanceof SyntaxError)) { throw error; } if ( @@ -1104,6 +1135,7 @@ async function recoverStaleCredentialHomeLock(lock: string): Promise { return false; } } + delete readRetry.deadline; if (isRecord(owner) && typeof owner["pid"] === "number") { try { diff --git a/sdk/typescript/tests-ts/api.test.ts b/sdk/typescript/tests-ts/api.test.ts index 37896482a..3a07f3e19 100644 --- a/sdk/typescript/tests-ts/api.test.ts +++ b/sdk/typescript/tests-ts/api.test.ts @@ -4488,7 +4488,7 @@ describe("CodexSecurity orchestration", () => { try { const results = await Promise.allSettled( - clients.map((client) => client.run(repository)), + clients.map((client) => client.run(repository).finally(releaseScans)), ); for (const result of results) { expect(result).toMatchObject({ @@ -4500,6 +4500,7 @@ describe("CodexSecurity orchestration", () => { } expect(scansStarted).toBe(2); } finally { + releaseScans(); await Promise.all(clients.map(async (client) => await client.close())); } }); diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index 16280e70e..fd50d384c 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -30,7 +30,7 @@ import { import { fileURLToPath, pathToFileURL } from "node:url"; import { promisify } from "node:util"; import { brotliDecompressSync } from "node:zlib"; -import { afterEach, describe, expect, mock, test } from "bun:test"; +import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; import { strToU8, zipSync } from "fflate"; import { BUNDLED_PLUGIN_VERSION, @@ -2134,6 +2134,351 @@ describe("runtime directories and plugin Python boundary", () => { } }); + test.each(["EPERM", "EBUSY"])( + "retries temporarily unreadable Windows credential-lock owners with %s", + async (code) => { + if ( + runMockInSubprocess( + import.meta.path, + `retries temporarily unreadable Windows credential-lock owners with ${code}`, + ) + ) { + return; + } + const root = await temporaryDirectory(); + const home = join(root, "credential-home"); + const lock = join(home, ".codex-security-scan.lock"); + const ownerPath = join(lock, "owner.json"); + await mkdir(home, { mode: 0o700 }); + const securityOptions = { + platform: "win32" as const, + secureWindowsHome: async () => {}, + }; + const releaseFirst = await acquireCodexSecurityCredentialHomeLock( + home, + undefined, + securityOptions, + ); + const originalReadFile = fsPromises.readFile; + let ownerReads = 0; + let retryObserved!: () => void; + const retried = new Promise((resolve) => { + retryObserved = resolve; + }); + mock.module("node:fs/promises", () => ({ + ...fsPromises, + readFile: async (...args: Parameters) => { + if (String(args[0]) === ownerPath) { + ownerReads += 1; + if (ownerReads === 1) { + throw Object.assign(new Error("temporarily unreadable owner"), { + code, + }); + } + if (ownerReads === 2) retryObserved(); + } + return originalReadFile(...args); + }, + })); + const controller = new AbortController(); + const waiting = acquireCodexSecurityCredentialHomeLock( + home, + controller.signal, + securityOptions, + ); + void waiting.catch(() => undefined); + + try { + await Promise.race([ + retried, + waiting.then(() => { + throw new Error("The held credential lock was acquired early."); + }), + ]); + expect(ownerReads).toBeGreaterThanOrEqual(2); + expect(existsSync(ownerPath)).toBe(true); + await releaseFirst(); + const releaseSecond = await waiting; + await releaseSecond(); + expect(existsSync(lock)).toBe(false); + } finally { + controller.abort(); + mock.module("node:fs/promises", () => ({ + ...fsPromises, + readFile: originalReadFile, + })); + await releaseFirst(); + await waiting.then( + (release) => release(), + () => undefined, + ); + } + }, + ); + + test.each(["EPERM", "EBUSY"])( + "reports persistent Windows credential-lock read failures with %s", + async (code) => { + if ( + runMockInSubprocess( + import.meta.path, + `reports persistent Windows credential-lock read failures with ${code}`, + ) + ) { + return; + } + const root = await temporaryDirectory(); + const home = join(root, "credential-home"); + const ownerPath = join(home, ".codex-security-scan.lock", "owner.json"); + await mkdir(home, { mode: 0o700 }); + const securityOptions = { + platform: "win32" as const, + secureWindowsHome: async () => {}, + }; + const release = await acquireCodexSecurityCredentialHomeLock( + home, + undefined, + securityOptions, + ); + const originalReadFile = fsPromises.readFile; + const failure = Object.assign( + new Error("persistent owner read failure"), + { + code, + }, + ); + let now = Date.now(); + let ownerReads = 0; + const clock = spyOn(Date, "now").mockImplementation(() => now); + mock.module("node:fs/promises", () => ({ + ...fsPromises, + readFile: async (...args: Parameters) => { + if (String(args[0]) === ownerPath) { + ownerReads += 1; + now += 15_000; + throw failure; + } + return originalReadFile(...args); + }, + })); + const controller = new AbortController(); + const watchdog = setTimeout(() => controller.abort(), 1_000); + try { + await expect( + acquireCodexSecurityCredentialHomeLock( + home, + controller.signal, + securityOptions, + ), + ).rejects.toBe(failure); + expect(ownerReads).toBe(2); + expect(existsSync(ownerPath)).toBe(true); + } finally { + clearTimeout(watchdog); + controller.abort(); + clock.mockRestore(); + mock.module("node:fs/promises", () => ({ + ...fsPromises, + readFile: originalReadFile, + })); + await release(); + } + }, + ); + + test("rechecks a replaced Windows credential lock after a read retry", async () => { + if ( + runMockInSubprocess( + import.meta.path, + "rechecks a replaced Windows credential lock after a read retry", + ) + ) { + return; + } + const root = await temporaryDirectory(); + const home = join(root, "credential-home"); + const lock = join(home, ".codex-security-scan.lock"); + const ownerPath = join(lock, "owner.json"); + await mkdir(home, { mode: 0o700 }); + const securityOptions = { + platform: "win32" as const, + secureWindowsHome: async () => {}, + }; + const releaseFirst = await acquireCodexSecurityCredentialHomeLock( + home, + undefined, + securityOptions, + ); + const old = new Date(Date.now() - 60_000); + await fsPromises.utimes(lock, old, old); + const originalReadFile = fsPromises.readFile; + const originalLstat = fsPromises.lstat; + let replaceOnRead = true; + let replacementCreated = false; + let checkedReplacement!: () => void; + const rechecked = new Promise((resolve) => { + checkedReplacement = resolve; + }); + mock.module("node:fs/promises", () => ({ + ...fsPromises, + lstat: async (...args: Parameters) => { + const metadata = await originalLstat(...args); + if (replacementCreated && String(args[0]) === lock) { + checkedReplacement(); + } + return metadata; + }, + readFile: async (...args: Parameters) => { + if (replaceOnRead && String(args[0]) === ownerPath) { + replaceOnRead = false; + await releaseFirst(); + await mkdir(lock, { mode: 0o700 }); + replacementCreated = true; + throw Object.assign(new Error("owner removed during read"), { + code: "EPERM", + }); + } + return originalReadFile(...args); + }, + })); + const controller = new AbortController(); + const waiting = acquireCodexSecurityCredentialHomeLock( + home, + controller.signal, + securityOptions, + ); + void waiting.catch(() => undefined); + try { + await Promise.race([ + rechecked, + waiting.then(() => { + throw new Error( + "The replacement lock was acquired before its owner was ready.", + ); + }), + ]); + controller.abort(); + await expect(waiting).rejects.toMatchObject({ name: "AbortError" }); + expect(existsSync(lock)).toBe(true); + expect(existsSync(ownerPath)).toBe(false); + } finally { + controller.abort(); + mock.module("node:fs/promises", () => ({ + ...fsPromises, + lstat: originalLstat, + readFile: originalReadFile, + })); + await waiting.then( + (release) => release(), + () => undefined, + ); + await releaseFirst(); + await rm(lock, { recursive: true, force: true }); + } + }); + + test("cancels unreadable Windows credential-lock retries", async () => { + if ( + runMockInSubprocess( + import.meta.path, + "cancels unreadable Windows credential-lock retries", + ) + ) { + return; + } + const root = await temporaryDirectory(); + const home = join(root, "credential-home"); + const ownerPath = join(home, ".codex-security-scan.lock", "owner.json"); + await mkdir(home, { mode: 0o700 }); + const securityOptions = { + platform: "win32" as const, + secureWindowsHome: async () => {}, + }; + const release = await acquireCodexSecurityCredentialHomeLock( + home, + undefined, + securityOptions, + ); + const originalReadFile = fsPromises.readFile; + const controller = new AbortController(); + mock.module("node:fs/promises", () => ({ + ...fsPromises, + readFile: async (...args: Parameters) => { + if (String(args[0]) === ownerPath) { + controller.abort(); + throw Object.assign(new Error("unreadable owner"), { code: "EPERM" }); + } + return originalReadFile(...args); + }, + })); + try { + await expect( + acquireCodexSecurityCredentialHomeLock( + home, + controller.signal, + securityOptions, + ), + ).rejects.toMatchObject({ name: "AbortError" }); + expect(existsSync(ownerPath)).toBe(true); + } finally { + controller.abort(); + mock.module("node:fs/promises", () => ({ + ...fsPromises, + readFile: originalReadFile, + })); + await release(); + } + }); + + test("preserves other Windows credential-lock read errors", async () => { + if ( + runMockInSubprocess( + import.meta.path, + "preserves other Windows credential-lock read errors", + ) + ) { + return; + } + const root = await temporaryDirectory(); + const home = join(root, "credential-home"); + const ownerPath = join(home, ".codex-security-scan.lock", "owner.json"); + await mkdir(home, { mode: 0o700 }); + const securityOptions = { + platform: "win32" as const, + secureWindowsHome: async () => {}, + }; + const release = await acquireCodexSecurityCredentialHomeLock( + home, + undefined, + securityOptions, + ); + const originalReadFile = fsPromises.readFile; + mock.module("node:fs/promises", () => ({ + ...fsPromises, + readFile: async (...args: Parameters) => { + if (String(args[0]) === ownerPath) { + throw Object.assign(new Error("access denied"), { code: "EACCES" }); + } + return originalReadFile(...args); + }, + })); + try { + await expect( + acquireCodexSecurityCredentialHomeLock( + home, + undefined, + securityOptions, + ), + ).rejects.toMatchObject({ code: "EACCES" }); + } finally { + mock.module("node:fs/promises", () => ({ + ...fsPromises, + readFile: originalReadFile, + })); + await release(); + } + }); + test("does not rewrite Windows credential ACLs while polling a held lock", async () => { const root = await temporaryDirectory(); const home = join(root, "credential-home");