Skip to content
Draft
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion sdk/typescript/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ import {
import { estimateScanCost, ScanCostTracker, type ScanCost } from "./cost.js";
import {
loadContract,
requireScanArtifactPath,
requireScanFile,
type ScanExpectation,
} from "./contract.js";
Expand Down Expand Up @@ -1240,7 +1241,21 @@ export class CodexSecurity {
} catch (error) {
if (signal.aborted || this.#closed) throw error;
for (const artifact of completedArtifacts) {
const path = join(scanDir, artifact.name);
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(
(readError: NodeJS.ErrnoException) => {
if (readError.code !== "ENOENT") throw readError;
Expand Down
69 changes: 59 additions & 10 deletions sdk/typescript/src/contract.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<CheckedScanFile> {
): Promise<string> {
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<CheckedScanPath> {
const scanDir = checkedRoot.path;
throwIfAborted(signal);
const safePath = safeRelativePath(relativePath, context);
Expand Down Expand Up @@ -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 };
Comment thread
mldangelo-oai marked this conversation as resolved.
Outdated
} 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<CheckedScanFile> {
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) {
Expand Down
42 changes: 37 additions & 5 deletions sdk/typescript/src/runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -996,6 +996,10 @@ async function secureWindowsCredentialHome(path: string): Promise<void> {
}
}

interface CredentialLockReadRetry {
deadline?: number;
}

export async function acquireCodexSecurityCredentialHomeLock(
codexHome: string,
signal?: AbortSignal,
Expand All @@ -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);
Expand All @@ -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,
Expand All @@ -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;
}
Expand Down Expand Up @@ -1078,23 +1087,45 @@ export async function acquireCodexSecurityCredentialHomeLock(
}
}

async function recoverStaleCredentialHomeLock(lock: string): Promise<boolean> {
async function recoverStaleCredentialHomeLock(
lock: string,
platform: NodeJS.Platform,
readRetry: CredentialLockReadRetry,
): Promise<boolean> {
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}`,
);
}

let owner: unknown;
const deadline =
platform === "win32"
? (readRetry.deadline ??=
Date.now() + INCOMPLETE_CREDENTIAL_LOCK_MILLISECONDS)
: undefined;
Comment thread
mldangelo-oai marked this conversation as resolved.
Outdated
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 (
Expand All @@ -1104,6 +1135,7 @@ async function recoverStaleCredentialHomeLock(lock: string): Promise<boolean> {
return false;
}
}
delete readRetry.deadline;

if (isRecord(owner) && typeof owner["pid"] === "number") {
try {
Expand Down
30 changes: 24 additions & 6 deletions sdk/typescript/tests-ts/api-post-scan.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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 });
Expand Down Expand Up @@ -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<ThreadEvent> {
yield {
Expand All @@ -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();
},
);
Expand Down
3 changes: 2 additions & 1 deletion sdk/typescript/tests-ts/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -4500,6 +4500,7 @@ describe("CodexSecurity orchestration", () => {
}
expect(scansStarted).toBe(2);
} finally {
releaseScans();
await Promise.all(clients.map(async (client) => await client.close()));
}
});
Expand Down
Loading
Loading