From cf0097647c79661cf3af770e9731f13c4e5432d3 Mon Sep 17 00:00:00 2001 From: Kyle Brown Date: Tue, 8 Sep 2026 23:34:01 +0000 Subject: [PATCH 01/14] refactor(plugin): port candidate normalization to TypeScript --- .../codex-security/mcp-app/helpers-main.ts | 10 +- .../mcp-app/src/artifact-discovery.ts | 17 +- .../src/helpers/normalize-candidates.ts | 684 ++++++++++++++ .../mcp-app/src/helpers/posix-path.ts | 85 +- .../src/helpers/resolve-security-md.ts | 36 +- .../mcp-app/src/helpers/utf8.ts | 8 + .../mcp-app/tests/test_artifact_discovery.mjs | 21 +- .../tests/test_compact_artifact_server.mjs | 6 +- plugins/codex-security/native/README.md | 2 +- .../native/examples/windows-wide-launcher.rs | 106 ++- plugins/codex-security/plugin-files.json | 1 - .../scripts/launch_codex_security_mcp | 6 +- .../scripts/normalize_candidates.py | 338 ------- .../skills/security-diff-scan/SKILL.md | 2 +- .../tests/test_normalize_candidates.py | 789 ---------------- .../src/custom-validation-prompt.ts | 2 +- .../tests-ts/candidate-normalizer.test.ts | 880 ++++++++++++++++++ .../tests-ts/compact-diff-scan.test.ts | 24 +- sdk/typescript/tests-ts/runtime.test.ts | 85 +- 19 files changed, 1869 insertions(+), 1233 deletions(-) create mode 100644 plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts create mode 100644 plugins/codex-security/mcp-app/src/helpers/utf8.ts delete mode 100644 plugins/codex-security/scripts/normalize_candidates.py delete mode 100644 plugins/codex-security/tests/test_normalize_candidates.py create mode 100644 sdk/typescript/tests-ts/candidate-normalizer.test.ts diff --git a/plugins/codex-security/mcp-app/helpers-main.ts b/plugins/codex-security/mcp-app/helpers-main.ts index 18c8611be..29d07df72 100644 --- a/plugins/codex-security/mcp-app/helpers-main.ts +++ b/plugins/codex-security/mcp-app/helpers-main.ts @@ -1,6 +1,8 @@ +import { closeSync, readFileSync } from "node:fs"; import { resolveSecurityMdCommand } from "./src/helpers/resolve-security-md"; import { decodePosixBytes } from "./src/helpers/posix-path"; import { windowsBinding } from "./src/native"; +import { normalizeCandidatesCommand } from "./src/helpers/normalize-candidates"; let commandLine = process.argv.slice(2); if (process.platform === "win32") { @@ -14,8 +16,10 @@ if (commandLine[0] === "--helper") { if (process.platform === "win32") { commandLine = commandLine.slice(1); } else { + const encoded = readFileSync(3, "ascii"); + closeSync(3); const [homeSet, home, ...args] = decodePosixBytes( - Buffer.from(commandLine[1] ?? "", "hex"), + Buffer.from(encoded.trim(), "hex"), ) .split("\0") .slice(0, -1); @@ -26,9 +30,11 @@ if (commandLine[0] === "--helper") { const [command, ...args] = commandLine; if (command === "resolve-security-md") { process.exitCode = resolveSecurityMdCommand(args, posixHome); +} else if (command === "normalize-candidates") { + process.exitCode = normalizeCandidatesCommand(args, posixHome); } else { console.error( - "Usage: launch_codex_security_mcp[.cmd] --helper resolve-security-md [options]", + "Usage: launch_codex_security_mcp[.cmd] --helper [options]", ); process.exitCode = 2; } diff --git a/plugins/codex-security/mcp-app/src/artifact-discovery.ts b/plugins/codex-security/mcp-app/src/artifact-discovery.ts index 622a8156e..dd66fdd79 100644 --- a/plugins/codex-security/mcp-app/src/artifact-discovery.ts +++ b/plugins/codex-security/mcp-app/src/artifact-discovery.ts @@ -17,7 +17,6 @@ import { type SchemaDocument } from "./artifact-schema-loader.js"; import { candidateSchemaV1 } from "./deep-scan/artifact-contracts.js"; -import { missingPythonHelperMessage, resolvePythonCommand } from "./python_command.js"; const execFileAsync = promisify(execFile); const discoveryComponents = ["artifacts", "02_discovery"] as const; @@ -137,7 +136,7 @@ export async function recordCodexSecurityDiscoveryCandidates( const inventoryComponents = [...discoveryComponents, "in_scope_files.txt"]; const candidateComponents = [...discoveryComponents, "candidate_ledger.jsonl"]; - // Verify the inventory is a context-bound regular file before passing it to Python. + // Verify the inventory is a context-bound regular file before normalization. await readArtifactText(context, inventoryComponents, "discovery review inventory"); const inventoryPath = await artifactDestination( context, @@ -161,12 +160,12 @@ export async function recordCodexSecurityDiscoveryCandidates( mode: 0o600 }); - const pythonCommand = context.pythonCommand ?? await resolvePythonCommand(); try { await execFileAsync( - pythonCommand, + process.execPath, [ - join(pluginRoot, "scripts", "normalize_candidates.py"), + join(pluginRoot, "mcp", "helpers.mjs"), + "normalize-candidates", "--input", temporaryInput, "--out", @@ -184,7 +183,7 @@ export async function recordCodexSecurityDiscoveryCandidates( } ); } catch (error) { - throw discoveryNormalizationError(error, pythonCommand, [ + throw discoveryNormalizationError(error, [ [temporaryInput, "candidate input"], [temporaryDirectory, "private candidate input"], [inventoryPath, "the assigned review inventory"], @@ -226,14 +225,8 @@ export async function listCodexSecurityCandidates( function discoveryNormalizationError( error: unknown, - pythonCommand: string, privateValues: Array ): Error { - const pythonMessage = missingPythonHelperMessage(error, pythonCommand); - if (pythonMessage) { - return new Error(`${discoveryLabel}: ${pythonMessage}`, { cause: error }); - } - const stderr = error && typeof error === "object" && "stderr" in error ? error.stderr : undefined; diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts new file mode 100644 index 000000000..b0a338476 --- /dev/null +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -0,0 +1,684 @@ +import { decodeUtf8 } from "./utf8"; +import { createHash, randomBytes } from "node:crypto"; +import { + closeSync, + mkdirSync, + openSync, + readFileSync, + renameSync, + statSync, + unlinkSync, + writeFileSync, +} from "node:fs"; +import { basename, dirname, isAbsolute, join, relative, sep } from "node:path"; +import { + decodePosixBytes, + encodePosixPath, + resolvePosixPath, + SymlinkLoopError, +} from "./posix-path"; +import { + expandHome, + HomeExpansionError, + parsedPath, + windowsRelativePath, +} from "./resolve-security-md"; +import { windowsBinding } from "../native"; +import { + pathText, + widePath, + windowsFileSystem, +} from "../../../native/windows-files.mjs"; + +const trim = (value: string) => + value.replace( + /^[\p{White_Space}\u001c-\u001f]+|[\p{White_Space}\u001c-\u001f]+$/gu, + "", + ); +const roles = [ + "entrypoint", + "entrypoint/wrapper", + "source", + "root_control", + "sink", + "concrete_implementation", + "evidence", +]; +const fields = new Set([ + "candidate_id", + "cwe_ids", + "locations", + "summary", + "evidence", + "context", + "instance", +]); +type Row = Record; +interface Location { + path: string; + start_line: number; + end_line: number; + role: string; +} +interface Candidate { + cwe_ids: string[]; + locations: Location[]; + summary: string; + evidence: string; + context?: string; + instance?: string; +} + +function compare(left: string, right: string): number { + const a = Array.from(left, (value) => value.codePointAt(0)!); + const b = Array.from(right, (value) => value.codePointAt(0)!); + for (let index = 0; index < Math.min(a.length, b.length); index++) { + if (a[index] !== b[index]) return a[index]! - b[index]!; + } + return a.length - b.length; +} + +// JSON's numeric spelling matters: Python accepts 1 but rejects 1.0 as a line. +class JsonFloat { + constructor(readonly source: string) {} +} +function parseJson(source: string): unknown { + const tokens = + source.match( + /"(?:\\[\s\S]|[^"\\])*"|[{}\[\]:,]|-?(?:0|[1-9][0-9]*)(?:\.[0-9]+)?(?:[eE][+-]?[0-9]+)?|true|false|null|-?Infinity|NaN|[^ \t\r\n]/gu, + ) ?? []; + let index = 0; + const take = () => tokens[index++]; + function expect(token: string): void { + if (take() !== token) throw new Error(`expected ${token} in JSON`); + } + function value(): unknown { + const token = take(); + if (token === "{") { + const row: Row = Object.create(null) as Row; + if (tokens[index] === "}") { + index++; + return row; + } + while (true) { + const key = take(); + if (!key?.startsWith('"')) + throw new Error("expected a JSON property name"); + expect(":"); + row[JSON.parse(key) as string] = value(); + if (tokens[index] === "}") { + index++; + return row; + } + expect(","); + } + } + if (token === "[") { + const values: unknown[] = []; + if (tokens[index] === "]") { + index++; + return values; + } + while (true) { + values.push(value()); + if (tokens[index] === "]") { + index++; + return values; + } + expect(","); + } + } + if (token?.startsWith('"')) return JSON.parse(token) as string; + if (token === "true") return true; + if (token === "false") return false; + if (token === "null") return null; + if (token !== undefined && /^-?[0-9]+$/u.test(token)) return BigInt(token); + if ( + token !== undefined && + (/^-?(?:[0-9]+(?:\.[0-9]+)?(?:[eE][+-]?[0-9]+)?|Infinity)$/u.test( + token, + ) || + token === "NaN") + ) + return new JsonFloat(token); + throw new Error("expected a JSON value"); + } + const result = value(); + if (index !== tokens.length) throw new Error("extra data after JSON value"); + return result; +} + +function object(value: unknown): value is Row { + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) && + !(value instanceof JsonFloat) + ); +} + +function stableJson(value: unknown): string { + if (typeof value === "string") { + if (/[\ud800-\udfff]/u.test(value)) + throw new Error("UTF-8 cannot encode an unpaired surrogate"); + return JSON.stringify(value); + } + if (Array.isArray(value)) return `[${value.map(stableJson).join(",")}]`; + if (object(value)) + return `{${Object.keys(value) + .sort(compare) + .map((key) => `${stableJson(key)}:${stableJson(value[key])}`) + .join(",")}}`; + return JSON.stringify(value); +} + +const windows = process.platform === "win32"; +const windowsFiles = () => windowsFileSystem(windowsBinding()); +const fsPath = (value: string) => + windows ? widePath(value) : encodePosixPath(value); +const readFile = (path: string) => + windows ? windowsFiles().readFile(fsPath(path)) : readFileSync(fsPath(path)); +const stat = (path: string) => + windows ? windowsFiles().stat(fsPath(path)) : statSync(fsPath(path)); +const pathKey = (value: string) => + process.platform === "win32" ? value.toLowerCase() : value; + +function resolvedPath(value: string, strict = true): string { + if (process.platform !== "win32") + return decodePosixBytes( + resolvePosixPath(encodePosixPath(parsedPath(value)), strict), + ); + try { + return pathText(windowsFiles().realpath(widePath(value), strict)); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ELOOP") + throw new SymlinkLoopError(`Symlink loop from ${value}`); + throw error; + } +} + +function inside(path: string, root: string, allowMissing = false): string { + let result: string | undefined; + if (windows) { + const bytes = windowsRelativePath( + widePath(path), + widePath(root), + allowMissing, + ); + result = bytes === undefined ? undefined : pathText(bytes); + } else { + result = relative(root, path); + } + if ( + result === undefined || + isAbsolute(result) || + result === ".." || + result.startsWith(`..${sep}`) + ) + throw new Error("path: must resolve inside --repo-root"); + return result.split(sep).join("/"); +} + +function relativeFile(value: unknown, root: string): [string, string] { + if (typeof value !== "string" || value === "" || value.includes("\0")) + throw new Error("path: expected a non-empty repository-relative path"); + const raw = + process.platform === "win32" ? value.replaceAll("\\", "/") : value; + if ( + raw.startsWith("/") || + raw.split("/").includes("..") || + (process.platform === "win32" && /^[A-Za-z]:/u.test(raw)) + ) + throw new Error( + "path: expected a repository-relative path without traversal", + ); + const path = resolvedPath(`${root}${sep}${raw}`); + const name = inside(path, root); + if (!stat(path).isFile()) throw new Error("path: expected a regular file"); + return [name, path]; +} + +function readScope( + path: string, + root: string, + allowMissing: boolean, +): Set { + const contents = decodeUtf8(readFile(path)); + const lines = contents.split("\n"); + const listed = new Set(lines); + const isFile = (value: string) => { + try { + relativeFile(value, root); + return true; + } catch (error) { + if (error instanceof SymlinkLoopError) throw error; + return false; + } + }; + const carriage = new Map(); + if (process.platform !== "win32") { + for (const line of lines) { + if (line.endsWith("\r") && line !== "\r") + carriage.set(line, [isFile(line), isFile(line.slice(0, -1))]); + } + } + const crlf = + lines.includes("\r") || + [...carriage.values()].some(([literal, stripped]) => stripped && !literal); + const literalEvidence = [...carriage.values()].some( + ([literal, stripped]) => literal && !stripped, + ); + const scope = new Set(); + for (const [index, original] of lines.entries()) { + let line = original; + if (process.platform === "win32" || line === "\r") { + if (line.endsWith("\r")) line = line.slice(0, -1); + } else if (line.endsWith("\r")) { + const [literal, stripped] = carriage.get(line)!; + if (stripped && !literal) line = line.slice(0, -1); + else if (stripped && literal) { + if ( + (index === lines.length - 1 && !contents.endsWith("\n")) || + listed.has(line.slice(0, -1)) + ) { + // An unterminated row or a separately listed sibling preserves CR. + } else if (crlf && !literalEvidence) line = line.slice(0, -1); + else if (!(literalEvidence && !crlf)) + throw new Error( + `in-scope file row ${index + 1}: ambiguous carriage-return paths`, + ); + } else if (!literal && crlf) line = line.slice(0, -1); + } + if (line === "") continue; + try { + scope.add(relativeFile(line, root)[0]); + } catch (error) { + if (error instanceof SymlinkLoopError) throw error; + if (allowMissing && (error as NodeJS.ErrnoException).code === "ENOENT") { + if ( + line.startsWith("/") || + line.split("/").includes("..") || + line.includes("\0") + ) + throw new Error( + `in-scope file row ${index + 1}: unsafe deleted path`, + ); + try { + scope.add( + inside(resolvedPath(`${root}${sep}${line}`, false), root, true), + ); + } catch (error) { + if (error instanceof SymlinkLoopError) throw error; + throw new Error( + `in-scope file row ${index + 1}: path escapes repository`, + ); + } + } else { + throw new Error( + `in-scope file row ${index + 1}: ${(error as Error).message}`, + ); + } + } + } + return scope; +} + +function textField( + row: Row, + field: string, + required = true, +): string | undefined { + const value = row[field]; + if ((value === undefined || value === null) && !required) return undefined; + if (typeof value !== "string" || trim(value) === "") + throw new Error(`${field}: expected a non-empty string`); + return trim(value); +} + +function cweIds(row: Row): string[] { + const values = row.cwe_ids; + if (!Array.isArray(values)) throw new Error("cwe_ids: expected an array"); + const found = new Set(); + for (const value of values) { + if (typeof value !== "string") + throw new Error("cwe_ids: expected CWE strings"); + const match = /^CWE-(\p{Decimal_Number}+)$/iu.exec(trim(value)); + if (match === null) + throw new Error(`cwe_ids: unsupported value ${JSON.stringify(value)}`); + const digits = Array.from(match[1]!, (digit) => { + let point = digit.codePointAt(0)!; + let offset = 0; + while (/\p{Decimal_Number}/u.test(String.fromCodePoint(point - 1))) { + point--; + offset++; + } + return String(offset % 10); + }).join(""); + const number = BigInt(digits); + if (number < 1n) + throw new Error(`cwe_ids: unsupported value ${JSON.stringify(value)}`); + found.add(number); + } + return [...found] + .sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)) + .map((number) => `CWE-${number}`); +} + +function positiveLine(value: unknown, field: string): bigint { + if (typeof value !== "bigint" || value < 1n) + throw new Error(`${field}: expected a positive integer`); + return value; +} + +function normalizeLocations( + row: Row, + root: string, + lineCounts: Map, +): Location[] { + if (!Array.isArray(row.locations) || row.locations.length === 0) + throw new Error("locations: expected a non-empty array"); + const normalized = new Map(); + for (const item of row.locations) { + if (!object(item)) throw new Error("locations: expected location objects"); + const unknown = Object.keys(item) + .filter( + (key) => !["path", "start_line", "end_line", "role"].includes(key), + ) + .sort(compare); + if (unknown.length) + throw new Error(`locations: unsupported fields ${unknown.join(", ")}`); + const [name, source] = relativeFile(item.path, root); + if ( + trim(name) === "" || + name.includes("\\") || + name.split("/").some((part) => part.includes(":")) + ) + throw new Error("path: expected a safe repository-relative POSIX path"); + const start = positiveLine(item.start_line, "start_line"); + const end = positiveLine( + item.end_line === undefined ? start : item.end_line, + "end_line", + ); + if (end < start) + throw new Error("end_line: must be greater than or equal to start_line"); + const key = pathKey(source); + if (!lineCounts.has(key)) { + const bytes = readFile(source); + const contents = bytes.toString("latin1"); + const lines = + contents.split(/\r\n|[\r\n]/u).length - + (contents === "" || /[\r\n]$/u.test(contents) ? 1 : 0); + lineCounts.set(key, lines); + } + const count = lineCounts.get(key)!; + if (end > BigInt(count)) + throw new Error(`line range ${start}-${end} exceeds ${name}:${count}`); + if (typeof item.role !== "string" || !roles.includes(item.role)) + throw new Error(`role: unsupported value ${String(item.role)}`); + const location = { + path: name, + start_line: Number(start), + end_line: Number(end), + role: item.role, + }; + normalized.set(stableJson(location), location); + } + return [...normalized.values()].sort( + (a, b) => + roles.indexOf(a.role) - roles.indexOf(b.role) || + compare(a.path, b.path) || + a.start_line - b.start_line || + a.end_line - b.end_line, + ); +} + +function normalizeCandidate( + row: Row, + root: string, + scope: Set, + lineCounts: Map, +): Candidate { + const unknown = Object.keys(row) + .filter((key) => !fields.has(key)) + .sort(compare); + if (unknown.length) + throw new Error(`unsupported fields ${unknown.join(", ")}`); + if ("candidate_id" in row) textField(row, "candidate_id"); + const locations = normalizeLocations(row, root, lineCounts); + if (!locations.some((item) => scope.has(item.path))) + throw new Error("locations: expected at least one in-scope file"); + const result: Candidate = { + cwe_ids: cweIds(row), + locations, + summary: textField(row, "summary")!, + evidence: textField(row, "evidence")!, + }; + const context = textField(row, "context", false); + if (context !== undefined) result.context = context; + const instance = textField(row, "instance", false); + if (instance !== undefined) result.instance = instance; + return result; +} + +function combine(rows: Candidate[]): (Candidate & { candidate_id: string })[] { + const groups = new Map(); + for (const row of rows) { + const key = stableJson({ + cwe_ids: row.cwe_ids, + locations: row.locations, + instance: row.instance ?? null, + }); + const group = groups.get(key) ?? []; + group.push(row); + groups.set(key, group); + } + return [...groups.keys()].sort(compare).map((key) => { + const group = groups.get(key)!; + const merged = (field: "summary" | "evidence" | "context") => + [ + ...new Set( + group + .map((row) => row[field]) + .filter((value): value is string => value !== undefined), + ), + ] + .sort(compare) + .join("\n"); + const result = { + ...group[0]!, + candidate_id: `candidate-${createHash("sha256").update(key).digest("hex").slice(0, 16)}`, + summary: merged("summary"), + evidence: merged("evidence"), + }; + const context = merged("context"); + if (context !== "") result.context = context; + return result; + }); +} + +function argumentsFor(args: string[]): Record { + const names = [ + "input", + "out", + "repo-root", + "in-scope-files", + "allow-missing-in-scope", + "help", + ]; + function option(value: string): string | undefined { + if (value === "-h") return "help"; + if (value.startsWith("--") && value !== "--") { + const name = value.slice(2).split("=", 1)[0]!; + const matches = names.filter((item) => item.startsWith(name)); + if (matches.includes(name)) return name; + if (matches.length === 1) return matches[0]; + if (matches.length > 1) throw new Error(`ambiguous option: ${value}`); + } + if ( + !value.startsWith("-") || + value === "-" || + (!value.startsWith("-h") && + (value.includes(" ") || + /^-(?:\p{Decimal_Number}+|\p{Decimal_Number}*\.\p{Decimal_Number}+)\n?$/u.test( + value, + ))) + ) + return undefined; + throw new Error(`unrecognized argument: ${value}`); + } + const values: Record = {}; + for (let index = 0; index < args.length; index++) { + const argument = args[index]!; + const name = option(argument); + if (name === undefined) + throw new Error(`unrecognized argument: ${argument}`); + const equals = argument.indexOf("="); + if (name === "help" || name === "allow-missing-in-scope") { + if (equals !== -1) + throw new Error(`argument --${name} does not take a value`); + values[name] = true; + if (name === "help") return values; + continue; + } + const found: string[] = []; + if (equals !== -1) found.push(argument.slice(equals + 1)); + else { + while ( + index + 1 < args.length && + option(args[index + 1]!) === undefined + ) { + found.push(args[++index]!); + if (name !== "input") break; + } + } + if (found.length === 0) + throw new Error( + `argument --${name}: expected ${name === "input" ? "at least one argument" : "one argument"}`, + ); + values[name] = found; + } + for (const name of ["input", "out", "repo-root", "in-scope-files"]) { + if (values[name] === undefined) throw new Error(`--${name} is required`); + } + return values; +} + +export function normalizeCandidatesCommand( + args: string[], + posixHome = process.env.HOME, +): number { + try { + const values = argumentsFor(args); + if (values.help) { + console.log( + "Validate and combine security-scan candidates into deterministic JSONL.\n", + ); + console.log( + "Usage: launch_codex_security_mcp[.cmd] --helper normalize-candidates --input PATH [PATH ...] --out PATH --repo-root PATH --in-scope-files PATH [--allow-missing-in-scope]", + ); + return 0; + } + const paths = (name: string, strict = true) => + (values[name] as string[]).map((value) => + resolvedPath(expandHome(parsedPath(value), posixHome), strict), + ); + const root = paths("repo-root")[0]!; + if (!stat(root).isDirectory()) + throw new Error("--repo-root: expected a directory"); + const output = paths("out", false)[0]!; + const scopePath = paths("in-scope-files")[0]!; + const inputs = [ + ...new Map(paths("input").map((path) => [pathKey(path), path])).values(), + ].sort((a, b) => { + const left = pathKey(a).split(sep), + right = pathKey(b).split(sep); + for ( + let index = 0; + index < Math.min(left.length, right.length); + index++ + ) { + const order = compare(left[index]!, right[index]!); + if (order !== 0) return order; + } + return left.length - right.length; + }); + if (inputs.some((path) => pathKey(path) === pathKey(output))) + throw new Error("--out: must not also be an input"); + if (pathKey(output) === pathKey(scopePath)) + throw new Error("--out: must not replace --in-scope-files"); + const scope = readScope( + scopePath, + root, + values["allow-missing-in-scope"] === true, + ); + const lineCounts = new Map(); + const rows: Candidate[] = []; + for (const source of inputs) { + const lines = decodeUtf8(readFile(source)).split(/\r\n|[\r\n]/u); + for (const [index, line] of lines.entries()) { + if (trim(line) === "") continue; + try { + const row = parseJson(line); + if (!object(row)) throw new Error("expected a JSON object"); + rows.push(normalizeCandidate(row, root, scope, lineCounts)); + } catch (error) { + if (error instanceof SymlinkLoopError) throw error; + throw new Error( + `${source} row ${index + 1}: ${(error as Error).message}`, + ); + } + } + } + const combined = combine(rows); + if (windows) windowsFiles().mkdir(fsPath(dirname(output))); + else mkdirSync(fsPath(dirname(output)), { recursive: true }); + const temporary = join( + dirname(output), + `.${basename(output)}.${randomBytes(6).toString("base64url")}.tmp`, + ); + let created = false; + try { + if (windows) { + function* contents() { + created = true; + for (const row of combined) + yield Buffer.from(`${stableJson(row)}\r\n`); + } + windowsFiles().writeFile(fsPath(temporary), contents(), true); + windowsFiles().rename(fsPath(temporary), fsPath(output)); + } else { + const descriptor = openSync(fsPath(temporary), "wx", 0o600); + created = true; + try { + for (const row of combined) + writeFileSync(descriptor, `${stableJson(row)}\n`, "utf8"); + } finally { + closeSync(descriptor); + } + renameSync(fsPath(temporary), fsPath(output)); + } + } finally { + try { + if (created) { + if (windows) windowsFiles().unlink(fsPath(temporary)); + else unlinkSync(fsPath(temporary)); + } + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; + } + } + const message = `Combined ${rows.length} candidate rows into ${combined.length} rows in ${output}\n`; + process.stdout.write( + process.platform === "win32" + ? message.replace(/\n/gu, "\r\n") + : encodePosixPath(message), + ); + return 0; + } catch (error) { + console.error(`normalize_candidates: ${(error as Error).message}`); + return error instanceof SymlinkLoopError || + error instanceof HomeExpansionError + ? 1 + : 2; + } +} diff --git a/plugins/codex-security/mcp-app/src/helpers/posix-path.ts b/plugins/codex-security/mcp-app/src/helpers/posix-path.ts index 6eb933ae5..207f4bf16 100644 --- a/plugins/codex-security/mcp-app/src/helpers/posix-path.ts +++ b/plugins/codex-security/mcp-app/src/helpers/posix-path.ts @@ -1,30 +1,30 @@ -const utf8 = new TextDecoder("utf-8", { fatal: true, ignoreBOM: true }); +import { isUtf8 } from "node:buffer"; export function decodePosixBytes(bytes: Buffer): string { - try { - return utf8.decode(bytes); - } catch { - // Match Python's surrogateescape for undecodable POSIX path bytes. - let value = ""; - for (let offset = 0; offset < bytes.length; ) { - let decoded = false; - for (let size = 1; size <= 4 && offset + size <= bytes.length; size++) { - try { - value += utf8.decode(bytes.subarray(offset, offset + size)); - offset += size; - decoded = true; - break; - } catch { - // A UTF-8 character can occupy up to four bytes. - } + // Node 20's fatal TextDecoder can replace invalid bytes in longer inputs. + if (isUtf8(bytes)) return bytes.toString("utf8"); + // Match Python's surrogateescape for undecodable POSIX path bytes. + let value = ""; + for (let offset = 0; offset < bytes.length; ) { + let decoded = false; + for (let size = 1; size <= 4 && offset + size <= bytes.length; size++) { + const part = bytes.subarray(offset, offset + size); + if (isUtf8(part)) { + value += part.toString("utf8"); + offset += size; + decoded = true; + break; } - if (!decoded) value += String.fromCharCode(0xdc00 + bytes[offset++]!); } - return value; + if (!decoded) value += String.fromCharCode(0xdc00 + bytes[offset++]!); } + return value; } export function encodePosixPath(value: string): Buffer { + if (/[\ud800-\udc7f\udd00-\udfff]/u.test(value)) { + throw new Error("UTF-8 cannot encode an unpaired surrogate"); + } return Buffer.concat( value .split(/([\udc80-\udcff])/u) @@ -38,14 +38,17 @@ export function encodePosixPath(value: string): Buffer { export class SymlinkLoopError extends Error {} -export function resolvePosixPath(value: Buffer): Buffer { +export function resolvePosixPath(value: Buffer, strict = true): Buffer { // GNU Linux native realpath rejects file/.. and links targeting it with // ENOTDIR. Retain the shipped pathlib contract for those inputs. const seen = new Map(); // Latin-1 is a lossless internal representation of pathname bytes. - function follow(directory: string, path: string): string { + const append = (directory: string, rest: string) => + rest.startsWith("/") ? rest : `${directory}/${rest}`; + function follow(directory: string, path: string): [string, boolean] { if (path.startsWith("/")) directory = "/"; - for (const name of path.split("/")) { + const parts = path.split("/"); + for (const [index, name] of parts.entries()) { if (name === "" || name === ".") continue; if (name === "..") { directory = directory.slice(0, directory.lastIndexOf("/")) || "/"; @@ -53,12 +56,21 @@ export function resolvePosixPath(value: Buffer): Buffer { } const candidate = `${directory === "/" ? "" : directory}/${name}`; const bytes = Buffer.from(candidate, "latin1"); - if (!lstatSync(bytes).isSymbolicLink()) { + let link: boolean; + try { + link = lstatSync(bytes).isSymbolicLink(); + } catch (error) { + if (strict) throw error; + link = false; + } + if (!link) { directory = candidate; continue; } const cached = seen.get(candidate); if (cached === null) { + if (!strict) + return [append(candidate, parts.slice(index + 1).join("/")), false]; throw new SymlinkLoopError( `Symlink loop from ${decodePosixBytes(bytes)}`, ); @@ -68,21 +80,34 @@ export function resolvePosixPath(value: Buffer): Buffer { continue; } seen.set(candidate, null); - directory = follow( + const [resolved, complete] = follow( directory, readlinkSync(bytes, { encoding: "buffer" }).toString("latin1"), ); + if (!complete) + return [append(resolved, parts.slice(index + 1).join("/")), false]; + directory = resolved; seen.set(candidate, directory); } - return directory; + return [directory, true]; } const cwd = value[0] === 0x2f ? Buffer.from("/") : realpathSync.native(".", { encoding: "buffer" }); - return Buffer.from( - follow(cwd.toString("latin1"), value.toString("latin1")), - "latin1", - ); + const [path] = follow(cwd.toString("latin1"), value.toString("latin1")); + const result = Buffer.from(posix.resolve(path), "latin1"); + if (!strict) { + try { + statSync(result); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ELOOP") + throw new SymlinkLoopError( + `Symlink loop from ${decodePosixBytes(result)}`, + ); + } + } + return result; } -import { lstatSync, readlinkSync, realpathSync } from "node:fs"; +import { lstatSync, readlinkSync, realpathSync, statSync } from "node:fs"; +import { posix } from "node:path"; diff --git a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts index e5b38993a..9e38dd18d 100644 --- a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts +++ b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts @@ -1,3 +1,4 @@ +import { decodeUtf8 } from "./utf8"; import { closeSync, lstatSync, @@ -26,8 +27,7 @@ import { } from "./posix-path"; const MAX_SECURITY_MD_BYTES = 1024 * 1024; -const utf8 = new TextDecoder("utf-8", { fatal: true, ignoreBOM: true }); -class HomeExpansionError extends Error {} +export class HomeExpansionError extends Error {} const windows = process.platform === "win32"; const windowsFiles = () => windowsFileSystem(windowsBinding()); const encodePath = (path: string) => @@ -40,7 +40,7 @@ type FileInfo = Pick & { const statPath = (path: Buffer): FileInfo => windows ? windowsFiles().stat(path) : statSync(path); -function parsedPath(value: string): string { +export function parsedPath(value: string): string { // pathlib removes empty and '.' components while preserving symlink/.. pairs. let root = windows ? windowsParts(value).slice(0, 2).join("").replaceAll("/", "\\") @@ -71,7 +71,10 @@ function resolvedPath(path: Buffer): Buffer { } } -function expandHome(path: string, posixHome: string | undefined): string { +export function expandHome( + path: string, + posixHome: string | undefined, +): string { if (!path.startsWith("~")) return path; if (process.platform === "win32") { const environment = (name: string) => @@ -138,18 +141,27 @@ function parentDirectory(path: Buffer): Buffer { : path.subarray(0, Math.max(1, separator)); } -function windowsRelativePath(path: Buffer, root: Buffer): Buffer | undefined { +export function windowsRelativePath( + path: Buffer, + root: Buffer, + allowMissing = false, +): Buffer | undefined { const files = windowsFiles(); const rootIdentity = files.identity(root); const parts: string[] = []; let current = path; while (true) { - const identity = files.identity(current); - if ( - identity.volume === rootIdentity.volume && - identity.fileId.equals(rootIdentity.fileId) - ) - return encodePath(parts.reverse().join("\\")); + try { + const identity = files.identity(current); + if ( + identity.volume === rootIdentity.volume && + identity.fileId.equals(rootIdentity.fileId) + ) + return encodePath(parts.reverse().join("\\")); + } catch (error) { + if (!allowMissing || (error as NodeJS.ErrnoException).code !== "ENOENT") + throw error; + } const parent = parentDirectory(current); if (parent.equals(current)) return undefined; parts.push(basename(decodePath(current))); @@ -307,7 +319,7 @@ function readPolicy(path: Buffer, displayedPath: Buffer): string { throw new Error(`SECURITY.md exceeds 1 MiB: ${decodePath(displayedPath)}`); } try { - return utf8.decode(buffer.subarray(0, length)); + return decodeUtf8(buffer.subarray(0, length)); } catch { throw new Error( `SECURITY.md is not valid UTF-8: ${decodePath(displayedPath)}`, diff --git a/plugins/codex-security/mcp-app/src/helpers/utf8.ts b/plugins/codex-security/mcp-app/src/helpers/utf8.ts new file mode 100644 index 000000000..5b2e71e4d --- /dev/null +++ b/plugins/codex-security/mcp-app/src/helpers/utf8.ts @@ -0,0 +1,8 @@ +import { isUtf8 } from "node:buffer"; + +export function decodeUtf8(bytes: Buffer): string { + // Node 20's fatal TextDecoder can silently replace invalid input bytes. + if (!isUtf8(bytes)) + throw new TypeError("The encoded data was not valid for encoding utf-8"); + return bytes.toString("utf8"); +} diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs index b1c6753f6..e200be5b9 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs @@ -1,5 +1,6 @@ import assert from "node:assert/strict"; import { + copyFile, mkdir, mkdtemp, readFile, @@ -76,7 +77,24 @@ assert.deepEqual( assert.deepEqual(toolSchemas.$defs.workbenchListCandidatesInput.required, ["scanId"]); const root = await realpath(await mkdtemp(path.join(tmpdir(), "security-artifact-discovery-"))); +const runtimePluginRoot = path.join(root, "plugin"); try { + await build({ + bundle: true, + entryPoints: [path.join(pluginRoot, "mcp-app", "helpers-main.ts")], + outfile: path.join(runtimePluginRoot, "mcp", "helpers.mjs"), + format: "esm", + platform: "node" + }); + if (process.platform === "win32") { + const target = `win32-${process.arch}`; + const destination = path.join(runtimePluginRoot, "mcp", "native", target); + await mkdir(destination, { recursive: true }); + await copyFile( + path.join(pluginRoot, "native", "prebuilt", target, "windows.node"), + path.join(destination, "windows.node") + ); + } const repoRoot = path.join(root, "repository"); await mkdir(path.join(repoRoot, "src"), { recursive: true }); await mkdir(path.join(repoRoot, "support"), { recursive: true }); @@ -388,7 +406,8 @@ async function createContext(root, repoRoot, name, layout) { root: artifactRoot, repoRoot, layout, - pluginRoot + pluginRoot: runtimePluginRoot, + pythonCommand: path.join(root, "python-must-not-run") }; } diff --git a/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs b/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs index 153bfaf9f..485288320 100644 --- a/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs +++ b/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs @@ -1023,7 +1023,7 @@ async function testDiscoveryWorkerToolList(bundle) { CODEX_SECURITY_REPO_ROOT: repoRoot, CODEX_SECURITY_ARTIFACT_LAYOUT: "worker", CODEX_SECURITY_SCAN_ID: scanId, - CODEX_SECURITY_PLUGIN_ROOT: pluginRoot + CODEX_SECURITY_PLUGIN_ROOT: bundledPluginRoot }); try { assert.deepEqual( @@ -1119,7 +1119,7 @@ async function testReducerWorkerToolList(bundle) { CODEX_SECURITY_ARTIFACT_ROOT: artifactRoot, CODEX_SECURITY_REPO_ROOT: repoRoot, CODEX_SECURITY_ARTIFACT_LAYOUT: "reducer", - CODEX_SECURITY_PLUGIN_ROOT: pluginRoot, + CODEX_SECURITY_PLUGIN_ROOT: bundledPluginRoot, CODEX_SECURITY_REDUCER_CONTEXT_JSON: JSON.stringify({ scanRoot, claimedWorkers: [] @@ -1182,7 +1182,7 @@ async function bundleEntrypoint(entrypoint, outfile) { await build({ bundle: true, define: { - __dirname: JSON.stringify(applicationRoot), + __dirname: JSON.stringify(path.join(bundledPluginRoot, "mcp")), "import.meta.url": "__filename" }, entryPoints: [path.join(applicationRoot, entrypoint)], diff --git a/plugins/codex-security/native/README.md b/plugins/codex-security/native/README.md index 97251c1a7..fcdee3b18 100644 --- a/plugins/codex-security/native/README.md +++ b/plugins/codex-security/native/README.md @@ -39,7 +39,7 @@ Windows uses `windows-binding.mts` and the same Rust crate. `WindowsHandle` owns The binding exposes synchronous file and directory creation, attributes and reparse tags, identity and final/opened names, read/write/seek/size/EOF/flush, exact-handle rename and deletion, and exclusive whole-file locking. Rust's `File` supplies ordinary I/O, cursor-preserving truncation, `sync_all` for flush, and locks. Calls return numeric Windows errors, including 6 for closed handles and 33 for nonblocking lock contention. Buffer ranges, path encoding, and 64-bit seek arguments are checked before use. Overlapped handles are unsupported because pending operations could retain native buffers beyond the call. Path authorization, ancestor traversal, and reparse-point policy remain the caller's responsibility. -Five additional operations preserve Windows strings at the Node boundary. `windowsArguments` returns the complete OS argument vector, including the executable and Node options, using Rust's CRT-compatible parser. `windowsEnvironment` reads one wide environment name and distinguishes an absent value (`null`) from an empty buffer. `windowsAbsolutePath` resolves against the native current directory and drive directories without requiring the destination to exist. `windowsDirectoryEntries` uses `std::fs::read_dir` and cached `DirEntry::file_type()` values without opening each child; names remain UTF-16LE, and construction or iteration failures return their numeric Windows error and an empty array. Directory symlinks and junctions have both directory and symbolic-link flags. The typed adapter exposes this enumerator through `entriesWithTypes`, which `resolve-security-md --list` uses on Windows. `windowsReadLink` returns a UTF-16LE link target or its numeric Windows error; the typed adapter uses it to resolve missing paths without losing raw filenames. +Five additional operations preserve Windows strings at the Node boundary. `windowsArguments` returns the complete OS argument vector, including the executable and Node options, using Rust's CRT-compatible parser. `windowsEnvironment` reads one wide environment name and distinguishes an absent value (`null`) from an empty buffer. `windowsAbsolutePath` resolves against the native current directory and drive directories without requiring the destination to exist. `windowsDirectoryEntries` uses `std::fs::read_dir` and cached `DirEntry::file_type()` values without opening each child; names remain UTF-16LE, and construction or iteration failures return their numeric Windows error and an empty array. Directory symlinks and junctions have both directory and symbolic-link flags. The typed adapter exposes this enumerator through `entriesWithTypes`, which `resolve-security-md --list` uses on Windows. `windowsReadLink` returns a UTF-16LE link target or its numeric Windows error; candidate normalization uses it to resolve missing paths without losing raw filenames. `windows-files.mts` leaves ordinary absolute-path resolution and canonicalization to `GetFullPathNameW` and `GetFinalPathNameByHandleW`, trimming trailing separators below the root. Its small verbatim-path normalizer preserves drive and UNC share roots when resolving dot segments, including literal trailing dots and spaces. Non-strict `realpath` can retain unresolved components; callers must check containment independently. It also supports missing output paths. `stat(path, false)` retains exact symbolic-link and reparse-point metadata so callers can reject junction traversal independently of the enumerator's link label. The SDK's public runtime floor remains Node 22.13.0. Node 20.0.0 is an additional native-foundation compatibility proof; it does not change the SDK engine requirement. diff --git a/plugins/codex-security/native/examples/windows-wide-launcher.rs b/plugins/codex-security/native/examples/windows-wide-launcher.rs index 9cf15ea57..6789b9bfb 100644 --- a/plugins/codex-security/native/examples/windows-wide-launcher.rs +++ b/plugins/codex-security/native/examples/windows-wide-launcher.rs @@ -233,7 +233,111 @@ fn main() -> std::io::Result<()> { )); } } - println!("{{\"policyHelperRawPaths\":true,\"directoryIdentity\":true}}"); + let input_name = raw("input-", 0xd800); + let scope_name = raw("scope-files-", 0xdc80); + fs::write(repo.join("source.py"), "source line\n")?; + fs::write(repo.join(&scope_name), "source.py\ndeleted.py\n")?; + fs::write(repo.join(raw("scope-files-", 0xfffd)), "wrong.py\n")?; + fs::write( + repo.join(raw("input-", 0xfffd)), + "invalid replacement input", + )?; + fs::write( + repo.join(&input_name), + concat!( + "{\"cwe_ids\":[\"CWE-89\"],\"locations\":[{\"path\":\"source.py\",", + "\"start_line\":1,\"role\":\"entrypoint\"}],\"summary\":\"wide paths\",", + "\"evidence\":\"source evidence\"}\n", + ), + )?; + let output_link = "i\u{0307}.jsonl"; + std::os::windows::fs::symlink_file(&output_name, repo.join(output_link))?; + std::os::windows::fs::symlink_file(output_link, repo.join("İ.jsonl"))?; + let expected = concat!( + "{\"candidate_id\":\"candidate-a69fa65a28ed4e55\",\"cwe_ids\":[\"CWE-89\"],", + "\"evidence\":\"source evidence\",\"locations\":[{\"end_line\":1,", + "\"path\":\"source.py\",\"role\":\"entrypoint\",\"start_line\":1}],", + "\"summary\":\"wide paths\"}\r\n", + ); + let candidate = |repo_arg: &Path, input: &Path, scope: &Path, output: &Path| { + Command::new(&node) + .arg(&script) + .args(["--helper", "normalize-candidates", "--repo-root"]) + .arg(repo_arg) + .arg("--input") + .arg(input) + .arg("--in-scope-files") + .arg(scope) + .arg("--out") + .arg(output) + .arg("--allow-missing-in-scope") + .current_dir(&repo) + .env("USERPROFILE", &repo) + .output() + }; + for (index, prefix) in [repo.clone(), PathBuf::from("~"), PathBuf::from(".")] + .into_iter() + .enumerate() + { + if index == 0 { + fs::write(&output, "previous output")?; + } + let child = candidate( + &prefix, + &prefix.join(&input_name), + &prefix.join(&scope_name), + &prefix.join(if index == 0 { + output_name.clone() + } else { + OsString::from("İ.jsonl") + }), + )?; + if !child.status.success() + || !child.stderr.is_empty() + || fs::read(&output)? != expected.as_bytes() + { + return Err(io::Error::other(format!( + "Wide candidate helper failed: {}", + String::from_utf8_lossy(&child.stderr) + ))); + } + fs::remove_file(&output)?; + } + fs::create_dir(repo.join("blocked-output"))?; + let child = candidate( + Path::new("."), + Path::new(&input_name), + Path::new(&scope_name), + Path::new("blocked-output"), + )?; + if child.status.code() != Some(2) || !repo.join("blocked-output").is_dir() { + return Err(io::Error::other( + "Candidate replacement failure was not preserved", + )); + } + for entry in fs::read_dir(&repo)? { + if entry? + .file_name() + .to_string_lossy() + .starts_with(".blocked-output.") + { + return Err(io::Error::other( + "Candidate temporary output was not removed", + )); + } + } + for cwd in &cwds { + for repository in &repos { + if fs::read(root.join(cwd).join(repository).join(&replacement_output))? + != b"output sentinel" + { + return Err(io::Error::other( + "Candidate helper changed a replacement output", + )); + } + } + } + println!("{{\"policyHelperRawPaths\":true,\"candidateHelperRawPaths\":true,\"directoryIdentity\":true}}"); Ok(()) } diff --git a/plugins/codex-security/plugin-files.json b/plugins/codex-security/plugin-files.json index dbb67cc74..dbf6ff138 100644 --- a/plugins/codex-security/plugin-files.json +++ b/plugins/codex-security/plugin-files.json @@ -66,7 +66,6 @@ "scripts/generate_rank_input.py", "scripts/launch_codex_security_mcp", "scripts/launch_codex_security_mcp.cmd", - "scripts/normalize_candidates.py", "scripts/rank_preview.py", "scripts/report_projection.py", "scripts/snapshot_sqlite.py", diff --git a/plugins/codex-security/scripts/launch_codex_security_mcp b/plugins/codex-security/scripts/launch_codex_security_mcp index f7739b842..a112a2b9f 100755 --- a/plugins/codex-security/scripts/launch_codex_security_mcp +++ b/plugins/codex-security/scripts/launch_codex_security_mcp @@ -10,7 +10,11 @@ if [ "${1:-}" = "--helper" ]; then server_path=$launcher_dir/../mcp/helpers.mjs shift # Node decodes argv and HOME as UTF-8; preserve POSIX bytes first. - set -- --helper "$(printf '%s\000' "${HOME+x}" "${HOME-}" "$@" | od -An -v -tx1 | tr -d ' \n')" + # A separate descriptor preserves stdin and avoids a single argument limit. + exec 3< str | None: - value = row.get(field) - if value is None and not required: - return None - if not isinstance(value, str) or not value.strip(): - raise ValueError(f"{field}: expected a non-empty string") - return value.strip() - - -def cwe_ids(row: dict[str, Any]) -> list[str]: - value = row.get("cwe_ids") - if not isinstance(value, list): - raise ValueError("cwe_ids: expected an array") - found: set[int] = set() - for item in value: - if not isinstance(item, str): - raise ValueError("cwe_ids: expected CWE strings") - match = CWE.fullmatch(item.strip()) - if match is None or int(match[1]) < 1: - raise ValueError(f"cwe_ids: unsupported value {item!r}") - found.add(int(match[1])) - return [f"CWE-{number}" for number in sorted(found)] - - -def relative_file(value: Any, repo_root: Path) -> tuple[str, Path]: - if not isinstance(value, str) or not value or "\0" in value: - raise ValueError("path: expected a non-empty repository-relative path") - raw = value - if sys.platform == "win32": - raw = raw.replace("\\", "/") - path = PurePosixPath(raw) - if ( - path.is_absolute() - or ".." in path.parts - or (sys.platform == "win32" and re.match(r"^[A-Za-z]:", raw)) - ): - raise ValueError("path: expected a repository-relative path without traversal") - resolved = (repo_root / raw).resolve(strict=True) - try: - relative = resolved.relative_to(repo_root).as_posix() - except ValueError as error: - raise ValueError("path: must resolve inside --repo-root") from error - if not resolved.is_file(): - raise ValueError("path: expected a regular file") - return relative, resolved - - -def positive_line(value: Any, field: str) -> int: - if not isinstance(value, int) or isinstance(value, bool) or value < 1: - raise ValueError(f"{field}: expected a positive integer") - return value - - -def normalize_locations( - row: dict[str, Any], repo_root: Path, line_counts: dict[Path, int] -) -> list[dict[str, Any]]: - value = row.get("locations") - if not isinstance(value, list) or not value: - raise ValueError("locations: expected a non-empty array") - normalized: dict[tuple[str, int, int, str], dict[str, Any]] = {} - for item in value: - if not isinstance(item, dict): - raise ValueError("locations: expected location objects") - unknown = set(item) - {"path", "start_line", "end_line", "role"} - if unknown: - raise ValueError(f"locations: unsupported fields {', '.join(sorted(unknown))}") - relative, source = relative_file(item.get("path"), repo_root) - if ( - not relative.strip() - or "\\" in relative - or any(":" in part for part in PurePosixPath(relative).parts) - ): - raise ValueError("path: expected a safe repository-relative POSIX path") - start = positive_line(item.get("start_line"), "start_line") - end = positive_line(item.get("end_line", start), "end_line") - if end < start: - raise ValueError("end_line: must be greater than or equal to start_line") - if source not in line_counts: - line_counts[source] = len(source.read_bytes().splitlines()) - if end > line_counts[source]: - raise ValueError(f"line range {start}-{end} exceeds {relative}:{line_counts[source]}") - role = item.get("role") - if not isinstance(role, str) or role not in ROLES: - raise ValueError(f"role: unsupported value {role!r}") - key = (relative, start, end, role) - normalized[key] = { - "path": relative, - "start_line": start, - "end_line": end, - "role": role, - } - return sorted( - normalized.values(), - key=lambda item: ( - ROLES[item["role"]], - item["path"], - item["start_line"], - item["end_line"], - ), - ) - - -def read_scope(path: Path, repo_root: Path, *, allow_missing: bool = False) -> set[str]: - contents = path.read_bytes().decode("utf-8") - lines = contents.split("\n") - listed_rows = set(lines) - - def is_scope_file(value: str) -> bool: - try: - relative_file(value, repo_root) - except (OSError, ValueError): - return False - return True - - carriage_rows = { - line: (is_scope_file(line), is_scope_file(line.removesuffix("\r"))) - for line in lines - if line.endswith("\r") and line != "\r" and sys.platform != "win32" - } - crlf_evidence = any(line == "\r" for line in lines) or any( - stripped and not literal for literal, stripped in carriage_rows.values() - ) - literal_evidence = any(literal and not stripped for literal, stripped in carriage_rows.values()) - - scope: set[str] = set() - for number, line in enumerate(lines, 1): - if sys.platform == "win32" or line == "\r": - line = line.removesuffix("\r") - elif line.endswith("\r"): - literal, stripped = carriage_rows[line] - if stripped and not literal: - line = line.removesuffix("\r") - elif stripped and literal: - if number == len(lines) and not contents.endswith("\n"): - pass - elif line.removesuffix("\r") in listed_rows: - pass - elif crlf_evidence and not literal_evidence: - line = line.removesuffix("\r") - elif literal_evidence and not crlf_evidence: - pass - else: - raise ValueError(f"in-scope file row {number}: ambiguous carriage-return paths") - elif not literal and crlf_evidence: - line = line.removesuffix("\r") - if not line: - continue - try: - relative, _ = relative_file(line, repo_root) - except (OSError, ValueError) as error: - if allow_missing and isinstance(error, FileNotFoundError): - candidate = PurePosixPath(line) - if candidate.is_absolute() or ".." in candidate.parts or "\0" in line: - raise ValueError(f"in-scope file row {number}: unsafe deleted path") from error - resolved = (repo_root / line).resolve(strict=False) - try: - relative = resolved.relative_to(repo_root).as_posix() - except ValueError as escaped: - raise ValueError( - f"in-scope file row {number}: path escapes repository" - ) from escaped - scope.add(relative) - continue - raise ValueError(f"in-scope file row {number}: {error}") from error - scope.add(relative) - return scope - - -def normalize_candidate( - row: dict[str, Any], repo_root: Path, scope: set[str], line_counts: dict[Path, int] -) -> dict[str, Any]: - unknown = set(row) - FIELDS - if unknown: - raise ValueError(f"unsupported fields {', '.join(sorted(unknown))}") - if "candidate_id" in row: - text_field(row, "candidate_id") - candidate_locations = normalize_locations(row, repo_root, line_counts) - if not any(item["path"] in scope for item in candidate_locations): - raise ValueError("locations: expected at least one in-scope file") - result: dict[str, Any] = { - "cwe_ids": cwe_ids(row), - "locations": candidate_locations, - "summary": text_field(row, "summary"), - "evidence": text_field(row, "evidence"), - } - context = text_field(row, "context", required=False) - if context is not None: - result["context"] = context - instance = text_field(row, "instance", required=False) - if instance is not None: - result["instance"] = instance - return result - - -def identity(row: dict[str, Any]) -> str: - return json.dumps( - { - "cwe_ids": row["cwe_ids"], - "locations": row["locations"], - "instance": row.get("instance"), - }, - ensure_ascii=False, - separators=(",", ":"), - sort_keys=True, - ) - - -def merged_text(group: list[dict[str, Any]], field: str) -> str: - return "\n".join(sorted({item[field] for item in group if field in item})) - - -def combine(rows: list[dict[str, Any]]) -> list[dict[str, Any]]: - groups: dict[str, list[dict[str, Any]]] = {} - for row in rows: - groups.setdefault(identity(row), []).append(row) - combined: list[dict[str, Any]] = [] - for key, group in sorted(groups.items()): - candidate_id = hashlib.sha256(key.encode()).hexdigest()[:16] - - result = { - "candidate_id": f"candidate-{candidate_id}", - "cwe_ids": group[0]["cwe_ids"], - "locations": group[0]["locations"], - "summary": merged_text(group, "summary"), - "evidence": merged_text(group, "evidence"), - } - context = merged_text(group, "context") - if context: - result["context"] = context - if "instance" in group[0]: - result["instance"] = group[0]["instance"] - combined.append(result) - return combined - - -def main() -> None: - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument("--input", nargs="+", required=True, help="Candidate JSONL inputs.") - parser.add_argument("--out", required=True, help="Combined candidate JSONL output.") - parser.add_argument("--repo-root", required=True, help="Repository root for candidate paths.") - parser.add_argument("--in-scope-files", required=True, help="Repository-relative file list.") - parser.add_argument( - "--allow-missing-in-scope", - action="store_true", - help="Keep deleted Git paths in a diff inventory while validating existing candidate files.", - ) - args = parser.parse_args() - try: - repo_root = Path(args.repo_root).expanduser().resolve(strict=True) - if not repo_root.is_dir(): - raise ValueError("--repo-root: expected a directory") - output = Path(args.out).expanduser().resolve(strict=False) - scope_path = Path(args.in_scope_files).expanduser().resolve(strict=True) - inputs = sorted({Path(value).expanduser().resolve(strict=True) for value in args.input}) - if output in inputs: - raise ValueError("--out: must not also be an input") - if output == scope_path: - raise ValueError("--out: must not replace --in-scope-files") - scope = read_scope(scope_path, repo_root, allow_missing=args.allow_missing_in_scope) - line_counts: dict[Path, int] = {} - rows: list[dict[str, Any]] = [] - for source in inputs: - with source.open(encoding="utf-8") as handle: - for number, line in enumerate(handle, 1): - if not line.strip(): - continue - try: - row = json.loads(line) - if not isinstance(row, dict): - raise ValueError("expected a JSON object") - rows.append(normalize_candidate(row, repo_root, scope, line_counts)) - except (ValueError, TypeError, OSError) as error: - raise ValueError(f"{source} row {number}: {error}") from error - combined = combine(rows) - output.parent.mkdir(parents=True, exist_ok=True) - temporary: Path | None = None - try: - with tempfile.NamedTemporaryFile( - mode="w", - encoding="utf-8", - dir=output.parent, - prefix=f".{output.name}.", - suffix=".tmp", - delete=False, - ) as handle: - temporary = Path(handle.name) - for row in combined: - handle.write( - json.dumps(row, ensure_ascii=False, separators=(",", ":"), sort_keys=True) - + "\n" - ) - temporary.replace(output) - finally: - if temporary is not None: - temporary.unlink(missing_ok=True) - print(f"Combined {len(rows)} candidate rows into {len(combined)} rows in {output}") - except (OSError, ValueError) as error: - print(f"normalize_candidates: {error}", file=sys.stderr) - raise SystemExit(2) from error - - -if __name__ == "__main__": - main() diff --git a/plugins/codex-security/skills/security-diff-scan/SKILL.md b/plugins/codex-security/skills/security-diff-scan/SKILL.md index af59e39c2..245e65aa0 100644 --- a/plugins/codex-security/skills/security-diff-scan/SKILL.md +++ b/plugins/codex-security/skills/security-diff-scan/SKILL.md @@ -34,6 +34,6 @@ For terminal scans without a `scanId`, generate the changed-file list with: /scripts/generate_in_scope_files.py --repo --scope . --diff-base --diff-head --diff-mode --out /in_scope_files.txt ``` -Record candidates with `normalize_candidates.py --input --out /candidate_ledger.jsonl --repo-root --in-scope-files /in_scope_files.txt --allow-missing-in-scope`. Add validation and attack-path decisions to that same file. Following `../../references/final-report.md`, assemble unsealed `scan-manifest.json`, `findings.json`, and `coverage.json` before running `finalize_scan_contract.py --scan-dir --source-root `. +Record candidates with `/scripts/launch_codex_security_mcp --helper normalize-candidates --input --out /candidate_ledger.jsonl --repo-root --in-scope-files /in_scope_files.txt --allow-missing-in-scope`. Use the `.cmd` launcher on Windows. Add validation and attack-path decisions to that same file. Following `../../references/final-report.md`, assemble unsealed `scan-manifest.json`, `findings.json`, and `coverage.json` before running `finalize_scan_contract.py --scan-dir --source-root `. Finish only after every changed file and candidate is accounted for. Return the generated report, actual coverage gaps, and Codex review comments for confirmed findings. diff --git a/plugins/codex-security/tests/test_normalize_candidates.py b/plugins/codex-security/tests/test_normalize_candidates.py deleted file mode 100644 index 94290fcb6..000000000 --- a/plugins/codex-security/tests/test_normalize_candidates.py +++ /dev/null @@ -1,789 +0,0 @@ -from __future__ import annotations - -import json -import subprocess -import sys -from collections.abc import Callable -from pathlib import Path - -import pytest - -SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "normalize_candidates.py" - - -def write_jsonl(path: Path, rows: list[dict[str, object]]) -> None: - path.write_text("".join(json.dumps(row) + "\n" for row in rows), encoding="utf-8") - - -def location(path: str, line: int, role: str) -> dict[str, object]: - return {"path": path, "start_line": line, "role": role} - - -def candidate( - locations: list[dict[str, object]], - *, - cwes: list[str] | None = None, - summary: str = "Request input reaches an unsafe operation", - evidence: str = "The input is passed to the operation without a check", - context: str | None = None, - instance: str | None = None, -) -> dict[str, object]: - row: dict[str, object] = { - "cwe_ids": ["CWE-89"] if cwes is None else cwes, - "locations": locations, - "summary": summary, - "evidence": evidence, - } - if context is not None: - row["context"] = context - if instance is not None: - row["instance"] = instance - return row - - -def setup_repo(tmp_path: Path) -> tuple[Path, Path]: - repo = tmp_path / "repo" - for path in ("app/routes.py", "app/query.py", "app/export.py", "helpers/shared.py"): - source = repo / path - source.parent.mkdir(parents=True, exist_ok=True) - source.write_text("one\ntwo\nthree\nfour\nfive\n", encoding="utf-8") - scope = tmp_path / "in_scope_files.txt" - scope.write_text("app/routes.py\napp/query.py\napp/export.py\n", encoding="utf-8") - return repo, scope - - -def run_combiner( - tmp_path: Path, - inputs: list[list[dict[str, object]]], - *, - repo: Path | None = None, - scope: Path | None = None, - output_name: str = "combined.jsonl", - allow_missing: bool = False, -) -> tuple[subprocess.CompletedProcess[str], Path]: - if repo is None or scope is None: - repo, scope = setup_repo(tmp_path) - sources: list[Path] = [] - for index, rows in enumerate(inputs): - source = tmp_path / f"candidates-{index}.jsonl" - write_jsonl(source, rows) - sources.append(source) - output = tmp_path / output_name - result = subprocess.run( - [ - sys.executable, - str(SCRIPT), - "--input", - *(str(source) for source in sources), - "--out", - str(output), - "--repo-root", - str(repo), - "--in-scope-files", - str(scope), - *(["--allow-missing-in-scope"] if allow_missing else []), - ], - capture_output=True, - text=True, - ) - return result, output - - -def read_jsonl(path: Path) -> list[dict[str, object]]: - return [json.loads(line) for line in path.read_text(encoding="utf-8").split("\n") if line] - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -@pytest.mark.parametrize( - "relative", - [ - "app/ leading.py", - "app/trailing .py", - "app/ .py", - "app/ .py", - "app/carriage\rname.py", - "app/trailing-carriage.py\r", - "app/vertical\vname.py", - "app/form\fname.py", - "app/next\u0085name.py", - "app/line\u2028name.py", - "app/paragraph\u2029name.py", - ], -) -def test_scope_preserves_literal_posix_inventory_paths(tmp_path: Path, relative: str) -> None: - repo, scope = setup_repo(tmp_path) - source = repo / relative - source.write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes((relative + "\n").encode("utf-8")) - - result, output = run_combiner( - tmp_path, - [[candidate([location(relative, 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == relative - - -def test_scope_accepts_crlf_inventory_on_every_platform(tmp_path: Path) -> None: - repo, scope = setup_repo(tmp_path) - scope.write_bytes(b"\r\napp/routes.py\r\napp/query.py\r\n") - - result, output = run_combiner( - tmp_path, - [[candidate([location("app/routes.py", 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == "app/routes.py" - - -def test_diff_scope_accepts_deleted_files_without_weakening_candidate_locations( - tmp_path: Path, -) -> None: - repo, scope = setup_repo(tmp_path) - scope.write_text("app/deleted_guard.py\napp/routes.py\n", encoding="utf-8") - - rejected, _ = run_combiner( - tmp_path, - [[candidate([location("app/routes.py", 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - assert rejected.returncode == 2 - assert "in-scope file row 1" in rejected.stderr - - accepted, output = run_combiner( - tmp_path, - [[candidate([location("app/routes.py", 1, "entrypoint")])]], - repo=repo, - scope=scope, - allow_missing=True, - ) - assert accepted.returncode == 0, accepted.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == "app/routes.py" - - missing_candidate, _ = run_combiner( - tmp_path, - [[candidate([location("app/deleted_guard.py", 1, "root_control")])]], - repo=repo, - scope=scope, - output_name="missing-candidate.jsonl", - allow_missing=True, - ) - assert missing_candidate.returncode == 2 - assert "deleted_guard.py" in missing_candidate.stderr - - -def test_diff_scope_rejects_deleted_path_outside_repository(tmp_path: Path) -> None: - repo, scope = setup_repo(tmp_path) - scope.write_text("../deleted_guard.py\napp/routes.py\n", encoding="utf-8") - - result, output = run_combiner( - tmp_path, - [[candidate([location("app/routes.py", 1, "entrypoint")])]], - repo=repo, - scope=scope, - allow_missing=True, - ) - - assert result.returncode == 2 - assert "in-scope file row 1" in result.stderr - assert not output.exists() - - -@pytest.mark.parametrize( - "inventory", - [ - b"app/routes.py\r\napp/query.py\n", - b"app/routes.py\r\napp/query.py", - ], -) -def test_scope_accepts_mixed_and_unterminated_crlf_inventories( - tmp_path: Path, inventory: bytes -) -> None: - repo, scope = setup_repo(tmp_path) - scope.write_bytes(inventory) - - result, output = run_combiner( - tmp_path, - [[candidate([location("app/routes.py", 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == "app/routes.py" - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -def test_crlf_scope_preserves_paths_when_carriage_return_names_also_exist( - tmp_path: Path, -) -> None: - repo, scope = setup_repo(tmp_path) - (repo / "app/routes.py\r").write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes(b"app/routes.py\r\napp/query.py\r\n") - - result, output = run_combiner( - tmp_path, - [[candidate([location("app/routes.py", 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == "app/routes.py" - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -@pytest.mark.parametrize("candidate_path", ["app/routes.py", "app/routes.py\r"]) -def test_lf_scope_preserves_separately_listed_carriage_return_collisions( - tmp_path: Path, candidate_path: str -) -> None: - repo, scope = setup_repo(tmp_path) - for relative in ("app/routes.py\r", "app/query.py\r"): - (repo / relative).write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes(b"app/query.py\napp/query.py\r\napp/routes.py\napp/routes.py\r\n") - - result, output = run_combiner( - tmp_path, - [[candidate([location(candidate_path, 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == candidate_path - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -def test_scope_preserves_unterminated_final_carriage_return_filename( - tmp_path: Path, -) -> None: - repo, scope = setup_repo(tmp_path) - relative = "app/routes.py\r" - (repo / relative).write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes(b"app/query.py\r\napp/routes.py\r") - - result, output = run_combiner( - tmp_path, - [[candidate([location(relative, 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == relative - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -@pytest.mark.parametrize( - "relative", - ["app/literal\\name.py", "app/C:foo.py", " ", " "], -) -def test_candidate_rejects_paths_incompatible_with_scan_contract( - tmp_path: Path, relative: str -) -> None: - repo, scope = setup_repo(tmp_path) - source = repo / relative - source.write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes((relative + "\n").encode("utf-8")) - - result, output = run_combiner( - tmp_path, - [[candidate([location(relative, 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 2 - assert "safe repository-relative POSIX path" in result.stderr - assert not output.exists() - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -@pytest.mark.parametrize("candidate_path", ["app/routes.py", "app/routes.py\r"]) -def test_mixed_scope_rejects_colliding_carriage_return_file_names( - tmp_path: Path, candidate_path: str -) -> None: - repo, scope = setup_repo(tmp_path) - (repo / "app/routes.py\r").write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes(b"app/routes.py\r\napp/query.py\n") - - result, output = run_combiner( - tmp_path, - [[candidate([location(candidate_path, 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 2 - assert "ambiguous carriage-return paths" in result.stderr - assert not output.exists() - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -def test_lf_scope_uses_independent_literal_carriage_return_evidence( - tmp_path: Path, -) -> None: - repo, scope = setup_repo(tmp_path) - relative = "app/routes.py\r" - evidence = "app/literal-evidence.py\r" - for path in (relative, evidence): - (repo / path).write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes(b"app/routes.py\r\napp/literal-evidence.py\r\napp/query.py\n") - - result, output = run_combiner( - tmp_path, - [[candidate([location(relative, 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == relative - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -def test_scope_rejects_indistinguishable_carriage_return_inventories( - tmp_path: Path, -) -> None: - repo, scope = setup_repo(tmp_path) - for relative in ("app/routes.py\r", "app/query.py\r"): - (repo / relative).write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes(b"app/routes.py\r\napp/query.py\r\n") - - result, output = run_combiner( - tmp_path, - [[candidate([location("app/routes.py", 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 2 - assert "ambiguous carriage-return paths" in result.stderr - assert not output.exists() - - -@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only literal file names") -def test_crlf_blank_line_disambiguates_colliding_carriage_return_paths( - tmp_path: Path, -) -> None: - repo, scope = setup_repo(tmp_path) - for relative in ("app/routes.py\r", "app/query.py\r"): - (repo / relative).write_text("one\ntwo\n", encoding="utf-8") - scope.write_bytes(b"\r\napp/routes.py\r\napp/query.py\r\n") - - result, output = run_combiner( - tmp_path, - [[candidate([location("app/routes.py", 1, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == "app/routes.py" - - -@pytest.mark.skipif(sys.platform != "win32", reason="Windows-native file paths") -def test_windows_scope_normalizes_native_separators(tmp_path: Path) -> None: - repo, scope = setup_repo(tmp_path) - scope.write_bytes(b"app\\routes.py\r\n") - - result, output = run_combiner( - tmp_path, - [[candidate([location("app\\routes.py", 2, "entrypoint")])]], - repo=repo, - scope=scope, - ) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["locations"][0]["path"] == "app/routes.py" - - -@pytest.mark.skipif(sys.platform != "win32", reason="Windows-native drive paths") -def test_windows_candidates_reject_drive_qualified_paths(tmp_path: Path) -> None: - result, output = run_combiner( - tmp_path, - [[candidate([location("C:routes.py", 1, "entrypoint")])]], - ) - - assert result.returncode == 2 - assert "repository-relative path without traversal" in result.stderr - assert not output.exists() - - -def test_matching_code_paths_combine_even_when_the_prose_differs( - tmp_path: Path, -) -> None: - first = candidate( - [ - location("app/query.py", 4, "sink"), - location("app/routes.py", 2, "entrypoint"), - location("app/query.py", 3, "root_control"), - ], - cwes=["cwe-089", "CWE-89"], - summary="The request id reaches an interpolated query", - evidence="The query uses an f-string", - context="Intentional training application", - ) - second = candidate( - [ - location("app/query.py", 3, "root_control"), - location("app/routes.py", 2, "entrypoint"), - location("app/query.py", 4, "sink"), - location("app/query.py", 4, "sink"), - ], - summary="SQL injection is reachable from the request", - evidence="The id is inserted directly into SQL", - context="Runs only on localhost", - ) - - result, output = run_combiner(tmp_path, [[first], [second]]) - - assert result.returncode == 0, result.stderr - rows = read_jsonl(output) - assert len(rows) == 1 - assert rows[0]["candidate_id"].startswith("candidate-") - assert rows[0]["cwe_ids"] == ["CWE-89"] - assert rows[0]["locations"] == [ - {"path": "app/routes.py", "start_line": 2, "end_line": 2, "role": "entrypoint"}, - { - "path": "app/query.py", - "start_line": 3, - "end_line": 3, - "role": "root_control", - }, - {"path": "app/query.py", "start_line": 4, "end_line": 4, "role": "sink"}, - ] - assert rows[0]["summary"] == ( - "SQL injection is reachable from the request\nThe request id reaches an interpolated query" - ) - assert rows[0]["evidence"] == "The id is inserted directly into SQL\nThe query uses an f-string" - assert rows[0]["context"] == "Intentional training application\nRuns only on localhost" - - -@pytest.mark.parametrize( - "first_locations,second_locations", - [ - ( - [ - location("app/routes.py", 2, "entrypoint"), - location("helpers/shared.py", 3, "root_control"), - location("app/query.py", 4, "sink"), - ], - [ - location("app/routes.py", 2, "entrypoint"), - location("helpers/shared.py", 3, "root_control"), - location("app/export.py", 4, "sink"), - ], - ), - ( - [ - location("app/routes.py", 2, "entrypoint"), - location("helpers/shared.py", 3, "root_control"), - location("app/query.py", 4, "sink"), - ], - [ - location("app/export.py", 2, "entrypoint"), - location("helpers/shared.py", 3, "root_control"), - location("app/query.py", 4, "sink"), - ], - ), - ], -) -def test_different_reachable_paths_remain_separate( - tmp_path: Path, - first_locations: list[dict[str, object]], - second_locations: list[dict[str, object]], -) -> None: - result, output = run_combiner( - tmp_path, - [[candidate(first_locations), candidate(second_locations)]], - ) - - assert result.returncode == 0, result.stderr - rows = read_jsonl(output) - assert len(rows) == 2 - assert rows[0]["candidate_id"] != rows[1]["candidate_id"] - - -def test_separate_bugs_at_the_same_locations_can_use_an_instance_label( - tmp_path: Path, -) -> None: - locations = [ - location("app/routes.py", 2, "entrypoint"), - location("app/query.py", 4, "sink"), - ] - first = candidate(locations, instance="id parameter") - second = candidate(locations, instance="sort parameter") - - result, output = run_combiner(tmp_path, [[first, second]]) - - assert result.returncode == 0, result.stderr - rows = read_jsonl(output) - assert len(rows) == 2 - assert {row["instance"] for row in rows} == {"id parameter", "sort parameter"} - assert rows[0]["candidate_id"] != rows[1]["candidate_id"] - - -def test_output_is_stable_when_input_and_row_order_changes(tmp_path: Path) -> None: - duplicate_one = candidate( - [ - location("app/routes.py", 2, "entrypoint"), - location("app/query.py", 4, "sink"), - ], - summary="Request id reaches SQL", - evidence="The query interpolates id", - ) - duplicate_two = candidate( - [ - location("app/query.py", 4, "sink"), - location("app/routes.py", 2, "entrypoint"), - ], - summary="SQL is built from request input", - evidence="The id is part of the query string", - ) - separate = candidate( - [ - location("app/export.py", 2, "entrypoint"), - location("app/export.py", 4, "sink"), - ], - cwes=["CWE-22"], - summary="The export path is unsafe", - evidence="The path is joined without a containment check", - ) - - first, first_output = run_combiner( - tmp_path, - [[duplicate_one, separate], [duplicate_two]], - output_name="first.jsonl", - ) - second, second_output = run_combiner( - tmp_path, - [[duplicate_two], [separate, duplicate_one]], - output_name="second.jsonl", - ) - - assert first.returncode == 0, first.stderr - assert second.returncode == 0, second.stderr - assert first_output.read_bytes() == second_output.read_bytes() - - -def test_combined_output_can_be_combined_again_without_changing_it( - tmp_path: Path, -) -> None: - repo, scope = setup_repo(tmp_path) - locations = [ - location("app/routes.py", 2, "entrypoint"), - location("app/query.py", 4, "sink"), - ] - first, first_output = run_combiner( - tmp_path, - [ - [ - candidate(locations, evidence="First trace"), - candidate(locations, evidence="Second trace"), - ] - ], - repo=repo, - scope=scope, - output_name="first.jsonl", - ) - second_output = tmp_path / "second.jsonl" - second = subprocess.run( - [ - sys.executable, - str(SCRIPT), - "--input", - str(first_output), - "--out", - str(second_output), - "--repo-root", - str(repo), - "--in-scope-files", - str(scope), - ], - capture_output=True, - text=True, - ) - - assert first.returncode == 0, first.stderr - assert second.returncode == 0, second.stderr - assert first_output.read_bytes() == second_output.read_bytes() - - -def test_multiline_candidate_text_keeps_its_original_order(tmp_path: Path) -> None: - summary = "Z: attacker reaches the route.\nA: the route then reaches the sink." - evidence = "Z: read the request.\nA: pass it into the query." - context = "Z: enabled in the demo.\nA: runs only on localhost." - row = candidate( - [location("app/routes.py", 2, "entrypoint")], - summary=summary, - evidence=evidence, - context=context, - ) - - result, output = run_combiner(tmp_path, [[row]]) - - assert result.returncode == 0, result.stderr - combined = read_jsonl(output)[0] - assert combined["summary"] == summary - assert combined["evidence"] == evidence - assert combined["context"] == context - - -def test_unknown_cwe_and_supporting_location_outside_scope_are_allowed( - tmp_path: Path, -) -> None: - row = candidate( - [ - location("app/routes.py", 2, "entrypoint"), - location("helpers/shared.py", 3, "root_control"), - ], - cwes=[], - ) - - result, output = run_combiner(tmp_path, [[row]]) - - assert result.returncode == 0, result.stderr - assert read_jsonl(output)[0]["cwe_ids"] == [] - - -@pytest.mark.parametrize( - ("change", "message"), - [ - (lambda row: row.pop("cwe_ids"), "cwe_ids: expected an array"), - ( - lambda row: row.update(cwe_ids=["SQL injection"]), - "cwe_ids: unsupported value", - ), - (lambda row: row.update(locations=[]), "locations: expected a non-empty array"), - ( - lambda row: row["locations"][0].update(path="app/missing.py"), - "missing.py", - ), - ( - lambda row: row["locations"][0].update(path="../outside.py"), - "repository-relative path without traversal", - ), - ( - lambda row: row["locations"][0].update(start_line=6), - "line range 6-6 exceeds app/routes.py:5", - ), - ( - lambda row: row["locations"][0].update(end_line=1), - "greater than or equal", - ), - ( - lambda row: row["locations"][0].update(role="rootControl"), - "role: unsupported value", - ), - ( - lambda row: row["locations"][0].update(line=2), - "locations: unsupported fields line", - ), - ( - lambda row: row.update(technically_validated=True), - "unsupported fields technically_validated", - ), - ( - lambda row: row.update(disposition="reportable"), - "unsupported fields disposition", - ), - ], -) -def test_invalid_candidates_fail_with_the_input_row( - tmp_path: Path, change: Callable[[dict[str, object]], object], message: str -) -> None: - valid = candidate([location("app/routes.py", 2, "entrypoint")]) - invalid = candidate([location("app/routes.py", 2, "entrypoint")]) - change(invalid) - - result, output = run_combiner(tmp_path, [[valid, invalid]]) - - assert result.returncode == 2 - assert "candidates-0.jsonl row 2:" in result.stderr - assert message in result.stderr - assert not output.exists() - - -def test_candidate_with_no_in_scope_location_fails(tmp_path: Path) -> None: - row = candidate([location("helpers/shared.py", 3, "root_control")]) - - result, output = run_combiner(tmp_path, [[row]]) - - assert result.returncode == 2 - assert "expected at least one in-scope file" in result.stderr - assert not output.exists() - - -def test_malformed_json_does_not_replace_an_existing_output(tmp_path: Path) -> None: - repo, scope = setup_repo(tmp_path) - source = tmp_path / "candidates.jsonl" - source.write_text( - json.dumps(candidate([location("app/routes.py", 2, "entrypoint")])) + "\nnot-json\n", - encoding="utf-8", - ) - output = tmp_path / "combined.jsonl" - output.write_text("previous output\n", encoding="utf-8") - - result = subprocess.run( - [ - sys.executable, - str(SCRIPT), - "--input", - str(source), - "--out", - str(output), - "--repo-root", - str(repo), - "--in-scope-files", - str(scope), - ], - capture_output=True, - text=True, - ) - - assert result.returncode == 2 - assert "candidates.jsonl row 2:" in result.stderr - assert output.read_text(encoding="utf-8") == "previous output\n" - - -def test_empty_candidate_input_produces_an_empty_ledger(tmp_path: Path) -> None: - result, output = run_combiner(tmp_path, [[]]) - - assert result.returncode == 0, result.stderr - assert output.read_text(encoding="utf-8") == "" - - -def test_output_cannot_replace_the_in_scope_file_list(tmp_path: Path) -> None: - repo, scope = setup_repo(tmp_path) - source = tmp_path / "candidates.jsonl" - write_jsonl(source, [candidate([location("app/routes.py", 2, "entrypoint")])]) - original_scope = scope.read_text(encoding="utf-8") - - result = subprocess.run( - [ - sys.executable, - str(SCRIPT), - "--input", - str(source), - "--out", - str(scope), - "--repo-root", - str(repo), - "--in-scope-files", - str(scope), - ], - capture_output=True, - text=True, - ) - - assert result.returncode == 2 - assert "--out: must not replace --in-scope-files" in result.stderr - assert scope.read_text(encoding="utf-8") == original_scope diff --git a/sdk/typescript/src/custom-validation-prompt.ts b/sdk/typescript/src/custom-validation-prompt.ts index 93a1ec42e..dbf3cedc0 100644 --- a/sdk/typescript/src/custom-validation-prompt.ts +++ b/sdk/typescript/src/custom-validation-prompt.ts @@ -13,7 +13,7 @@ const SOURCES = { "skills/security-scan/SKILL.md": "5b8f5d7debeca14c6b37e8e7ba737671362b8eb4b7f49e693c99c6bd04bc8fa0", "skills/security-diff-scan/SKILL.md": - "0a4c519ad713585876ea7eb0a8af4b59892c86746f4c69851db9ab347b7fad2f", + "a158847ce309e37fe367b52b02dbd169dd6c61ac31ee7b7daab06488ac1fef00", } as const; const DISABLED_TOOLS = [ diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts new file mode 100644 index 000000000..217df22b5 --- /dev/null +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -0,0 +1,880 @@ +import { spawnSync } from "node:child_process"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + readdirSync, + realpathSync, + rmSync, + statSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { afterEach, describe, expect, test } from "bun:test"; +import { PLUGIN_ROOT } from "./plugin-root.js"; + +const node = Bun.which("node")!; +const helper = join(PLUGIN_ROOT, "mcp", "helpers.mjs"); +const roots: string[] = []; +type Row = Record; +interface Fixture { + root: string; + repo: string; + scope: string; + output: string; +} +const location = ( + path = "app/routes.py", + start_line = 2, + role = "entrypoint", +) => ({ path, start_line, role }); +const candidate = (locations: Row[] = [location()], extra: Row = {}) => ({ + cwe_ids: ["CWE-89"], + locations, + summary: "Request input reaches an unsafe operation", + evidence: "The input reaches the operation without a check", + ...extra, +}); +function write(path: string, data: string | Buffer): void { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, data); +} +function fixture(): Fixture { + const root = realpathSync( + mkdtempSync(join(tmpdir(), "candidate-normalizer-")), + ); + roots.push(root); + const repo = join(root, "İrepository"); + for (const path of [ + "app/routes.py", + "app/query.py", + "app/export.py", + "helpers/shared.py", + ]) + write(join(repo, path), "one\ntwo\nthree\nfour\nfive\n"); + const scope = join(root, "in-scope.txt"); + write(scope, "app/routes.py\napp/query.py\napp/export.py\n"); + return { root, repo, scope, output: join(root, "combined.jsonl") }; +} +function run( + f: Fixture, + groups: Row[][], + extra: string[] = [], + env = process.env, +) { + const inputs = groups.map((rows, index) => { + const input = join(f.root, `candidates-${index}.jsonl`); + write(input, rows.map((row) => JSON.stringify(row)).join("\n") + "\n"); + return input; + }); + return invoke(f, inputs, extra, env); +} +function invoke( + f: Fixture, + inputs: string[], + extra: string[] = [], + env = process.env, +) { + return spawnSync( + node, + [ + helper, + "normalize-candidates", + "--input", + ...inputs, + "--out", + f.output, + "--repo-root", + f.repo, + "--in-scope-files", + f.scope, + ...extra, + ], + { + encoding: "utf8", + env, + cwd: f.root, + maxBuffer: Infinity, + }, + ); +} +function ledger(f: Fixture): Row[] { + return readFileSync(f.output, "utf8") + .split(/\r?\n/u) + .filter(Boolean) + .map((line) => JSON.parse(line) as Row); +} +afterEach(() => { + for (const root of roots.splice(0)) + rmSync(root, { recursive: true, force: true }); +}); + +describe("built candidate normalizer", () => { + test.skipIf(process.platform !== "linux")( + "accepts many input paths through the packaged launcher", + () => { + const f = fixture(); + const row = `${JSON.stringify(candidate())}\n`; + const inputs = Array.from({ length: 700 }, (_, index) => { + const path = join( + f.root, + `worker-${index}-${"input".repeat(16)}.jsonl`, + ); + write(path, row); + return path; + }); + const result = spawnSync( + join(PLUGIN_ROOT, "scripts", "launch_codex_security_mcp"), + [ + "--helper", + "normalize-candidates", + "--input", + ...inputs, + "--out", + f.output, + "--repo-root", + f.repo, + "--in-scope-files", + f.scope, + ], + { + cwd: f.root, + env: { ...process.env, CODEX_MCP_NODE_PATH: node }, + encoding: "utf8", + }, + ); + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toBe( + `Combined 700 candidate rows into 1 rows in ${f.output}\n`, + ); + expect(ledger(f)).toHaveLength(1); + }, + ); + + test("combines canonical code paths without Python and preserves separate instances", () => { + const f = fixture(); + const locations = [ + location("app/query.py", 4, "sink"), + location(), + location("app/query.py", 3, "root_control"), + ]; + const first = candidate(locations, { + cwe_ids: ["cwe-089", "CWE-89"], + summary: "Z: request reaches SQL\nA: query runs", + evidence: "First trace", + context: "First review", + }); + const second = candidate( + [...locations].reverse().concat(location("app/query.py", 4, "sink")), + { + summary: "A second review", + evidence: "Second trace", + context: "Second review", + }, + ); + const result = run( + f, + [[first], [second, candidate(locations, { instance: "sort parameter" })]], + [], + { ...process.env, PATH: "" }, + ); + expect(result.status, result.stderr).toBe(0); + expect(result.stdout.replaceAll("\r\n", "\n")).toBe( + `Combined 3 candidate rows into 2 rows in ${f.output}\n`, + ); + const rows = ledger(f); + expect(rows).toHaveLength(2); + const merged = rows.find((row) => row["instance"] === undefined)!; + expect(merged["cwe_ids"]).toEqual(["CWE-89"]); + expect(merged["locations"]).toEqual([ + { path: "app/routes.py", start_line: 2, end_line: 2, role: "entrypoint" }, + { + path: "app/query.py", + start_line: 3, + end_line: 3, + role: "root_control", + }, + { path: "app/query.py", start_line: 4, end_line: 4, role: "sink" }, + ]); + expect(merged["summary"]).toBe( + "A second review\nZ: request reaches SQL\nA: query runs", + ); + expect(merged["evidence"]).toBe("First trace\nSecond trace"); + expect(merged["context"]).toBe("First review\nSecond review"); + expect(rows.some((row) => row["instance"] === "sort parameter")).toBe(true); + if (process.platform !== "win32") + expect(statSync(f.output).mode & 0o777).toBe(0o600); + }); + + test("is byte-stable across input order, duplicate aliases, and recombination", () => { + const f = fixture(); + const first = candidate([location(), location("app/query.py", 4, "sink")], { + evidence: "First trace", + }); + const second = candidate([...first["locations"]].reverse(), { + evidence: "Second trace", + }); + const separate = candidate( + [location("app/export.py", 2), location("app/export.py", 4, "sink")], + { cwe_ids: ["CWE-22"] }, + ); + expect(run(f, [[first, separate], [second]]).status).toBe(0); + const expected = readFileSync(f.output); + expect(run(f, [[second], [separate, first]]).status).toBe(0); + expect(readFileSync(f.output)).toEqual(expected); + const input = join(f.root, "previous.jsonl"); + write(input, expected); + expect(invoke(f, [input, input]).status).toBe(0); + expect(readFileSync(f.output)).toEqual(expected); + if (process.platform !== "win32") { + const alias = join(f.root, "input-alias.jsonl"); + symlinkSync(input, alias); + const result = invoke(f, [input, alias]); + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toContain("Combined 2 candidate rows into 2 rows"); + expect(readFileSync(f.output)).toEqual(expected); + } + }); + + test("keeps different entrypoints and sinks and allows supporting files outside scope", () => { + const f = fixture(); + const shared = location("helpers/shared.py", 3, "root_control"); + const rows = [ + candidate([location(), shared, location("app/query.py", 4, "sink")], { + cwe_ids: [], + }), + candidate([location(), shared, location("app/export.py", 4, "sink")], { + cwe_ids: [], + }), + candidate( + [ + location("app/export.py", 2), + shared, + location("app/query.py", 4, "sink"), + ], + { cwe_ids: [] }, + ), + ]; + expect(run(f, [rows]).status).toBe(0); + expect(ledger(f)).toHaveLength(3); + expect( + ledger(f).every((row) => (row["cwe_ids"] as unknown[]).length === 0), + ).toBe(true); + const rejected = run(f, [[candidate([shared])]]); + expect(rejected.status).toBe(2); + expect(rejected.stderr).toContain("expected at least one in-scope file"); + }); + + test("sorts Unicode code points, normalizes Unicode and large CWE integers, and retains BOM text", () => { + const f = fixture(); + const names = ["app/\u{10000}.py", "app/\ue000.py"]; + for (const name of names) write(join(f.repo, name), "line\n"); + write(f.scope, names.join("\n") + "\n"); + const row = candidate( + names.map((name) => location(name, 1, "evidence")), + { + cwe_ids: ["CWE-٢", "cwe-００８９", "CWE-89", "CWE-9007199254740993"], + summary: "\u001c\ufeffSummary\u0085", + evidence: "\u{10000}", + }, + ); + const result = run(f, [[row, { ...row, evidence: "\ue000" }]]); + expect(result.status, result.stderr).toBe(0); + const value = ledger(f)[0]!; + expect(value["cwe_ids"]).toEqual([ + "CWE-2", + "CWE-89", + "CWE-9007199254740993", + ]); + expect((value["locations"] as Row[]).map((item) => item["path"])).toEqual( + [...names].reverse(), + ); + expect(value["summary"]).toBe("\ufeffSummary"); + expect(value["evidence"]).toBe("\ue000\n\u{10000}"); + expect(value["candidate_id"]).toBe("candidate-cc5ebd3ebd732a50"); + expect(readFileSync(f.output, "utf8").startsWith('{"candidate_id":')).toBe( + true, + ); + }); + + test("counts source lines as bytes separated by CR, LF, or CRLF", () => { + const f = fixture(); + for (const [contents, count] of [ + [Buffer.from("a\rb\r\nc\n"), 3], + [Buffer.from("a\vb\fc\u0085d\u2028e"), 1], + [Buffer.from("unterminated"), 1], + [Buffer.alloc(0), 0], + [Buffer.from([0xff, 0x0a, 0xfe]), 2], + ] as const) { + write(join(f.repo, "app/routes.py"), contents); + if (count > 0) + expect( + run(f, [[candidate([location("app/routes.py", count)])]]).status, + ).toBe(0); + const result = run(f, [ + [candidate([location("app/routes.py", count + 1)])], + ]); + expect(result.status).toBe(2); + expect(result.stderr).toContain(`exceeds app/routes.py:${count}`); + } + }); + + test("rejects noninteger numeric spellings, malformed rows, and invalid UTF-8 atomically", () => { + const f = fixture(); + const input = join(f.root, "raw.jsonl"); + for (const number of [ + "1.0", + "1e0", + "true", + "0", + "-1", + "NaN", + "Infinity", + "1e9999", + '"1"', + ]) { + write( + input, + JSON.stringify(candidate([location("app/routes.py", 1)])).replace( + '"start_line":1', + `"start_line":${number}`, + ) + "\n", + ); + write(f.output, "previous output\n"); + const result = invoke(f, [input]); + expect(result.status).toBe(2); + expect(result.stderr).toContain( + "start_line: expected a positive integer", + ); + expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); + } + const prefix = Buffer.from( + JSON.stringify( + candidate(undefined, { cwe_ids: [], summary: "s", evidence: "" }), + ).slice(0, -2), + ); + const invalidLongJson = Buffer.concat([ + prefix, + Buffer.alloc(191 - prefix.length, 0x61), + Buffer.from([0xff]), + Buffer.from('"}'), + ]); + for (const invalid of [ + invalidLongJson, + "not-json", + "[]", + "null", + '{"locations":', + "\ufeff{}", + Buffer.from([0xff]), + JSON.stringify(candidate([], { summary: "\ud800" })), + ]) { + write(input, invalid); + const result = invoke(f, [input]); + expect(result.status).toBe(2); + expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); + } + write(input, JSON.stringify(candidate()) + "\r\n\rnot-json\n"); + expect(invoke(f, [input]).stderr).toContain("raw.jsonl row 3:"); + write( + input, + JSON.stringify(candidate(undefined, { evidence: "\ud800" })) + "\n", + ); + expect(invoke(f, [input]).status).toBe(2); + expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); + expect(readdirSync(f.root).some((name) => name.endsWith(".tmp"))).toBe( + false, + ); + }); + + test("reports invalid fields with the input row and preserves existing output", () => { + const f = fixture(); + const cases: [Row, string][] = [ + [candidate(undefined, { cwe_ids: null }), "cwe_ids: expected an array"], + [ + candidate(undefined, { cwe_ids: [12] }), + "cwe_ids: expected CWE strings", + ], + [ + candidate(undefined, { cwe_ids: ["SQL injection"] }), + "cwe_ids: unsupported value", + ], + [ + candidate(undefined, { cwe_ids: ["CWE-0"] }), + "cwe_ids: unsupported value", + ], + [candidate([]), "locations: expected a non-empty array"], + [ + candidate([null as unknown as Row]), + "locations: expected location objects", + ], + [candidate([location("app/missing.py")]), "missing.py"], + [ + candidate([location("../outside.py")]), + "repository-relative path without traversal", + ], + [ + candidate([location(f.repo)]), + "repository-relative path without traversal", + ], + [candidate([location("app")]), "path: expected a regular file"], + [ + candidate([location("app/routes.py", 6)]), + "line range 6-6 exceeds app/routes.py:5", + ], + [candidate([{ ...location(), end_line: 1 }]), "greater than or equal"], + [ + candidate([location("app/routes.py", 1, "rootControl")]), + "role: unsupported value", + ], + [ + candidate([{ ...location(), line: 2 }]), + "locations: unsupported fields line", + ], + [ + candidate(undefined, { technically_validated: true }), + "unsupported fields technically_validated", + ], + [ + candidate(undefined, { disposition: "reportable" }), + "unsupported fields disposition", + ], + [ + candidate(undefined, { candidate_id: " " }), + "candidate_id: expected a non-empty string", + ], + [ + candidate(undefined, { summary: "\u001c" }), + "summary: expected a non-empty string", + ], + [ + candidate(undefined, { evidence: null }), + "evidence: expected a non-empty string", + ], + [ + candidate(undefined, { context: " " }), + "context: expected a non-empty string", + ], + [ + candidate(undefined, { instance: false }), + "instance: expected a non-empty string", + ], + ]; + for (const [row, message] of cases) { + write(f.output, "previous output\n"); + const result = run(f, [[candidate(), row]]); + expect(result.status).toBe(2); + expect(result.stderr).toContain("candidates-0.jsonl row 2:"); + expect(result.stderr).toContain(message); + expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); + } + }); + + test.skipIf(process.platform === "win32")( + "rejects unsupported surrogate paths before replacement-character sibling lookup", + () => { + const f = fixture(); + write(join(f.repo, "\ufffd.py"), "replacement sibling\n"); + write(f.scope, "\ufffd.py\n"); + for (const name of ["\ud800.py", "\udc00.py"]) { + write(f.output, "previous output\n"); + const result = run(f, [[candidate([location(name, 1)])]]); + expect(result.status).toBe(2); + expect(result.stderr).toContain("unpaired surrogate"); + expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); + } + }, + ); + + test("writes valid long output basenames through a short exclusive temporary name", () => { + const f = fixture(); + f.output = join(f.root, `${"x".repeat(220)}.jsonl`); + const result = run(f, [[candidate()]]); + expect(result.status, result.stderr).toBe(0); + expect(ledger(f)).toHaveLength(1); + expect(readdirSync(f.root).some((name) => name.endsWith(".tmp"))).toBe( + false, + ); + }); + + test.skipIf(process.platform === "win32")( + "preserves scope through repeated separators after an unresolved symlink", + () => { + const f = fixture(); + symlinkSync("self", join(f.repo, "self")); + symlinkSync("missing/../self", join(f.repo, "alias")); + write(f.scope, `alias/${f.repo}/app/routes.py\n`); + const result = run(f, [[candidate()]], ["--allow-missing-in-scope"]); + expect(result.status, result.stderr).toBe(1); + expect(existsSync(f.output)).toBe(false); + }, + ); + + test.skipIf(process.platform === "win32")( + "normalizes remaining output components after a non-strict symlink cycle", + () => { + const f = fixture(); + symlinkSync("loop", join(f.root, "loop")); + const target = join(f.root, "target.jsonl"); + write(target, "target must remain unchanged\n"); + const output = join(f.root, "from-loop.jsonl"); + symlinkSync(target, output); + const result = run( + { ...f, output: `${f.root}/loop/../from-loop.jsonl` }, + [[candidate()]], + ); + expect(result.status, result.stderr).toBe(0); + expect(readFileSync(target, "utf8")).toBe( + "target must remain unchanged\n", + ); + expect(ledger({ ...f, output })).toHaveLength(1); + const alias = join(f.root, "nested-output-alias"); + const nestedTarget = join(f.root, "nested-target.jsonl"); + symlinkSync("loop/../nested-target.jsonl", alias); + const nested = run({ ...f, output: alias }, [[candidate()]]); + expect(nested.status, nested.stderr).toBe(0); + expect(ledger({ ...f, output: nestedTarget })).toHaveLength(1); + expect( + run({ ...f, output: `${f.root}/loop/still-looped.jsonl` }, [ + [candidate()], + ]).status, + ).toBe(1); + }, + ); + + test("handles empty ledgers and protects resolved input and inventory destinations", () => { + const f = fixture(); + expect(run(f, [[]]).status).toBe(0); + expect(readFileSync(f.output, "utf8")).toBe(""); + const source = join(f.root, "candidates-0.jsonl"); + expect(invoke({ ...f, output: source }, [source]).stderr).toContain( + "--out: must not also be an input", + ); + const scopeBefore = readFileSync(f.scope); + expect(invoke({ ...f, output: f.scope }, [source]).stderr).toContain( + "--out: must not replace --in-scope-files", + ); + expect(readFileSync(f.scope)).toEqual(scopeBefore); + if (process.platform !== "win32") { + const alias = join(f.root, "output-alias"); + symlinkSync(source, alias); + expect(invoke({ ...f, output: alias }, [source]).stderr).toContain( + "--out: must not also be an input", + ); + } + const nested = { + ...f, + output: join(f.root, "new", "nested", "ledger.jsonl"), + }; + expect(run(nested, [[candidate()]]).status).toBe(0); + expect(ledger(nested)).toHaveLength(1); + }); + + test("allows deleted inventory paths only when requested and keeps candidate files strict", () => { + const f = fixture(); + write(f.scope, "app/deleted.py\napp/routes.py\n"); + const strict = run(f, [[candidate()]]); + expect(strict.status).toBe(2); + expect(strict.stderr).toContain("in-scope file row 1:"); + expect(run(f, [[candidate()]], ["--allow-missing-in-scope"]).status).toBe( + 0, + ); + expect( + run( + f, + [[candidate([location("app/deleted.py")])]], + ["--allow-missing-in-scope"], + ).status, + ).toBe(2); + write(f.scope, "../deleted.py\napp/routes.py\n"); + expect(run(f, [[candidate()]], ["--allow-missing-in-scope"]).status).toBe( + 2, + ); + if (process.platform !== "win32") { + const outside = join(f.root, "outside"); + mkdirSync(outside); + symlinkSync(outside, join(f.repo, "outside"), "dir"); + write(f.scope, "outside/deleted.py\napp/routes.py\n"); + const escaped = run(f, [[candidate()]], ["--allow-missing-in-scope"]); + expect(escaped.status).toBe(2); + expect(escaped.stderr).toContain("path escapes repository"); + } + }); + + test("accepts CRLF, mixed newline, and unterminated inventories", () => { + const f = fixture(); + for (const inventory of [ + "\r\napp/routes.py\r\napp/query.py\r\n", + "app/routes.py\r\napp/query.py\n", + "app/routes.py\r\napp/query.py", + ]) { + write(f.scope, inventory); + expect(run(f, [[candidate()]]).status).toBe(0); + expect((ledger(f)[0]!["locations"] as Row[])[0]!["path"]).toBe( + "app/routes.py", + ); + } + }); + + test.skipIf(process.platform === "win32")( + "preserves literal POSIX names and rejects incompatible candidate paths", + () => { + const f = fixture(); + for (const name of [ + "app/ leading.py", + "app/trailing .py", + "app/ .py", + "app/ .py", + "app/carriage\rname.py", + "app/trailing.py\r", + "app/vertical\vname.py", + "app/form\fname.py", + "app/next\u0085name.py", + "app/line\u2028name.py", + "app/paragraph\u2029name.py", + ]) { + write(join(f.repo, name), "line\n"); + write(f.scope, name + "\n"); + const result = run(f, [[candidate([location(name, 1)])]]); + expect(result.status, result.stderr).toBe(0); + expect((ledger(f)[0]!["locations"] as Row[])[0]!["path"]).toBe(name); + } + for (const name of ["app/literal\\name.py", "app/C:foo.py", " ", " "]) { + write(join(f.repo, name), "line\n"); + write(f.scope, name + "\n"); + const result = run(f, [[candidate([location(name, 1)])]]); + expect(result.status).toBe(2); + expect(result.stderr).toContain("safe repository-relative POSIX path"); + } + }, + ); + + test.skipIf(process.platform === "win32")( + "disambiguates carriage-return collisions from independent inventory evidence", + () => { + const cases = [ + { + inventory: "app/routes.py\r\napp/query.py\r\n", + files: ["app/routes.py\r"], + selected: "app/routes.py", + }, + { + inventory: + "app/query.py\napp/query.py\r\napp/routes.py\napp/routes.py\r\n", + files: ["app/routes.py\r", "app/query.py\r"], + selected: "app/routes.py\r", + }, + { + inventory: "app/query.py\r\napp/routes.py\r", + files: ["app/routes.py\r"], + selected: "app/routes.py\r", + }, + { + inventory: "app/routes.py\r\napp/literal.py\r\napp/query.py\n", + files: ["app/routes.py\r", "app/literal.py\r"], + selected: "app/routes.py\r", + }, + { + inventory: "\r\napp/routes.py\r\napp/query.py\r\n", + files: ["app/routes.py\r", "app/query.py\r"], + selected: "app/routes.py", + }, + ]; + for (const item of cases) { + const f = fixture(); + for (const name of item.files) write(join(f.repo, name), "line\n"); + write(f.scope, item.inventory); + const result = run(f, [[candidate([location(item.selected, 1)])]]); + expect(result.status, result.stderr).toBe(0); + expect((ledger(f)[0]!["locations"] as Row[])[0]!["path"]).toBe( + item.selected, + ); + } + for (const inventory of [ + "app/routes.py\r\napp/query.py\n", + "app/routes.py\r\napp/query.py\r\n", + ]) { + const f = fixture(); + for (const name of ["app/routes.py\r", "app/query.py\r"]) + write(join(f.repo, name), "line\n"); + write(f.scope, inventory); + for (const selected of ["app/routes.py", "app/routes.py\r"]) { + const result = run(f, [[candidate([location(selected, 1)])]]); + expect(result.status).toBe(2); + expect(result.stderr).toContain("ambiguous carriage-return paths"); + expect(existsSync(f.output)).toBe(false); + } + } + }, + ); + + test.skipIf(process.platform !== "win32")( + "normalizes Windows separators and rejects drive-qualified locations", + () => { + const f = fixture(); + write(f.scope, "app\\routes.py\r\n"); + expect(run(f, [[candidate([location("app\\routes.py")])]]).status).toBe( + 0, + ); + expect((ledger(f)[0]!["locations"] as Row[])[0]!["path"]).toBe( + "app/routes.py", + ); + const result = run(f, [[candidate([location("C:routes.py")])]]); + expect(result.status).toBe(2); + expect(result.stderr).toContain( + "repository-relative path without traversal", + ); + }, + ); + + test.skipIf(process.platform !== "linux")( + "preserves an invalid filename byte near the end of a longer launcher payload", + () => { + const f = fixture(); + const input = Buffer.concat([ + Buffer.from(join(f.root, "candidates-")), + Buffer.from([0xff]), + ]); + writeFileSync(input, JSON.stringify(candidate()) + "\n"); + write(join(f.root, "candidates-\ufffd"), "wrong input\n"); + const args = [ + "normalize-candidates", + "--repo-root", + "İrepository", + "--in-scope-files", + "in-scope.txt", + "--out", + "combined.jsonl", + "--input", + "candidates-", + ]; + // The invalid byte sits at offset 191, where Node 20's decoder lost it. + const padding = + 193 - Buffer.byteLength(["x", "", ...args].join("\0")) - 2; + const result = spawnSync( + "/bin/sh", + [ + "-c", + 'exec "$1" --helper normalize-candidates --repo-root "$2" --in-scope-files in-scope.txt --out combined.jsonl --input "candidates-$(printf \'\\377\')"', + "helper-test", + join(PLUGIN_ROOT, "scripts", "launch_codex_security_mcp"), + "İrepository", + ], + { + cwd: f.root, + env: { + ...process.env, + HOME: "h".repeat(padding), + CODEX_MCP_NODE_PATH: node, + }, + encoding: "utf8", + }, + ); + expect(result.status, result.stderr).toBe(0); + expect(ledger(f)).toHaveLength(1); + expect(readFileSync(join(f.root, "candidates-\ufffd"), "utf8")).toBe( + "wrong input\n", + ); + }, + ); + + test.skipIf(process.platform === "win32")( + "preserves canonical byte paths and rejects links outside the repository", + () => { + const f = fixture(); + const raw = Buffer.concat([ + Buffer.from(f.root + "/repository-"), + process.platform === "darwin" ? Buffer.from("é") : Buffer.from([0xff]), + ]); + mkdirSync(raw); + writeFileSync(Buffer.concat([raw, Buffer.from("/entry.py")]), "one\n"); + const alias = join(f.root, "alias"); + symlinkSync(raw, alias, "dir"); + write(f.scope, "entry.py\n"); + const result = run({ ...f, repo: alias }, [ + [candidate([location("entry.py", 1)])], + ]); + expect(result.status, result.stderr).toBe(0); + expect((ledger(f)[0]!["locations"] as Row[])[0]!["path"]).toBe( + "entry.py", + ); + const outside = join(f.root, "outside.py"); + write(outside, "one\n"); + symlinkSync(outside, join(f.repo, "outside.py")); + write(f.scope, "app/routes.py\n"); + const escaped = run(f, [ + [candidate([location(), location("outside.py", 1, "sink")])], + ]); + expect(escaped.status).toBe(2); + expect(escaped.stderr).toContain("must resolve inside --repo-root"); + }, + ); + + test.skipIf(process.platform !== "win32")( + "keeps Unicode sibling directories outside candidate and deleted-file scope", + () => { + const f = fixture(); + const sibling = join(f.root, "i\u0307repository"); + write(join(sibling, "source.py"), "one\n"); + symlinkSync(sibling, join(f.repo, "outside"), "junction"); + const escaped = run(f, [ + [candidate([location(), location("outside/source.py", 1, "sink")])], + ]); + expect(escaped.status).toBe(2); + expect(escaped.stderr).toContain("must resolve inside --repo-root"); + write(f.scope, "app/routes.py\noutside/deleted.py\n"); + const missing = run(f, [[candidate()]], ["--allow-missing-in-scope"]); + expect(missing.status).toBe(2); + expect(missing.stderr).toContain("path escapes repository"); + expect(existsSync(f.output)).toBe(false); + }, + ); + + test("keeps argument aliases, multiple inputs, home expansion, and literal dash output", () => { + const f = fixture(); + run(f, [[candidate()]]); + const input = join(f.root, "candidates-0.jsonl"); + const result = spawnSync( + node, + [ + helper, + "normalize-candidates", + "--inp", + input, + input, + "--o", + "-", + "--repo-r", + "./~/İrepository", + "--in-s", + "~/in-scope.txt", + ], + { + cwd: f.root, + env: { ...process.env, HOME: f.root, USERPROFILE: f.root }, + encoding: "utf8", + }, + ); + expect(result.status, result.stderr).toBe(0); + expect(JSON.parse(readFileSync(join(f.root, "-"), "utf8"))).toHaveProperty( + "candidate_id", + ); + expect( + spawnSync(node, [helper, "normalize-candidates", "--help"], { + encoding: "utf8", + }).status, + ).toBe(0); + for (const args of [ + [], + ["--input"], + ["--in", input], + ["--allow-missing-in-scope=true"], + ]) + expect( + spawnSync(node, [helper, "normalize-candidates", ...args]).status, + ).toBe(2); + }); +}); diff --git a/sdk/typescript/tests-ts/compact-diff-scan.test.ts b/sdk/typescript/tests-ts/compact-diff-scan.test.ts index f117cb0c3..db0ddf668 100644 --- a/sdk/typescript/tests-ts/compact-diff-scan.test.ts +++ b/sdk/typescript/tests-ts/compact-diff-scan.test.ts @@ -389,12 +389,18 @@ describe("compact diff scan", () => { inventory, ]; - expect(python("normalize_candidates.py", ...args).status).toBe(2); - const accepted = python( - "normalize_candidates.py", - ...args, - "--allow-missing-in-scope", - ); + const normalize = (...options: string[]) => + spawnSync( + process.execPath, + [ + join(PLUGIN_ROOT, "mcp", "helpers.mjs"), + "normalize-candidates", + ...options, + ], + { encoding: "utf8" }, + ); + expect(normalize(...args).status).toBe(2); + const accepted = normalize(...args, "--allow-missing-in-scope"); expect(accepted.status, accepted.stderr).toBe(0); const contents = readFileSync(output, "utf8"); expect(contents).toContain("Résumé: missing guard"); @@ -408,11 +414,7 @@ describe("compact diff scan", () => { ]); writeFileSync(inventory, "../escaped.py\nsrc/handler.py\n"); - const escaped = python( - "normalize_candidates.py", - ...args, - "--allow-missing-in-scope", - ); + const escaped = normalize(...args, "--allow-missing-in-scope"); expect(escaped.status).toBe(2); expect(escaped.stderr).toContain("in-scope file row 1"); }); diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index 8c418e39c..9187c6815 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -867,61 +867,88 @@ describe("plugin runtime preparation", () => { expect(projection.exitCode).toBe(0); bundledPlugin = join(packageRoot, "_bundled_plugin"); } - const normalizer = join( - bundledPlugin, - "scripts", - "normalize_candidates.py", - ); + const normalizer = join(bundledPlugin, "mcp", "helpers.mjs"); expect(await readFile(normalizer, "utf8")).toBe( - await readFile( - join(sourcePlugin, "scripts", "normalize_candidates.py"), - "utf8", - ), + await readFile(join(sourcePlugin, "mcp", "helpers.mjs"), "utf8"), ); + const locations: (Record | null)[] = []; + for (const item of cases) { + const input = join(root, "candidate-input.jsonl"); + const output = join(root, "candidate-output.jsonl"); + await writeFile( + input, + JSON.stringify({ + cwe_ids: ["CWE-89"], + locations: [{ path: item.path, start_line: 1, role: "entrypoint" }], + summary: "Test finding", + evidence: "Test evidence", + }) + "\n", + ); + const normalized = Bun.spawnSync([ + process.execPath, + normalizer, + "normalize-candidates", + "--input", + input, + "--out", + output, + "--repo-root", + root, + "--in-scope-files", + scopePath, + ]); + const safe = + item.path.trim().length > 0 && + !item.path.includes(":") && + !item.path.includes("\\"); + expect(normalized.exitCode).toBe(safe ? 0 : 2); + if (safe) { + const row = JSON.parse(await readFile(output, "utf8")) as { + locations: Record[]; + }; + expect(row.locations[0]?.["path"]).toBe(item.path); + expect( + await readFile( + join(root, row.locations[0]!["path"] as string), + "utf8", + ), + ).toBe(item.contents); + locations.push(row.locations[0]!); + } else locations.push(null); + } const result = Bun.spawnSync([ python!, "-I", "-B", "-c", [ - "import json, pathlib, runpy, sys", - "module = runpy.run_path(sys.argv[1])", - "root = pathlib.Path(sys.argv[2])", - "scope = module['read_scope'](pathlib.Path(sys.argv[3]), root)", - "finalizer = runpy.run_path(sys.argv[5])", + "import json, runpy, sys", + "finalizer = runpy.run_path(sys.argv[1])", "results = []", - "for value in json.loads(sys.argv[4]):", - " path, source = module['relative_file'](value, root)", - " candidate = {'cwe_ids': ['CWE-89'], 'locations': [{'path': value, 'start_line': 1, 'role': 'entrypoint'}], 'summary': 'Test finding', 'evidence': 'Test evidence'}", + "for location in json.loads(sys.argv[2]):", " try:", - " normalized = module['normalize_candidate'](candidate, root, scope, {})", - " location = normalized['locations'][0]", + " if location is None: raise ValueError('rejected candidate')", " finalizer['_validate_location']({'path': location['path'], 'startLine': location['start_line'], 'endLine': location['end_line'], 'role': location['role']}, 'candidate.locations[0]')", " except ValueError:", " contract_valid = False", " else:", " contract_valid = True", - " results.append({'path': path, 'contents': source.read_text(encoding='utf-8'), 'inScope': path in scope, 'contractValid': contract_valid})", + " results.append(contract_valid)", "print(json.dumps(results))", ].join("\n"), - normalizer, - root, - scopePath, - JSON.stringify(cases.map((item) => item.path)), join(bundledPlugin, "scripts", "finalize_scan_contract.py"), + JSON.stringify(locations), ]); expect(result.exitCode).toBe(0); expect(JSON.parse(new TextDecoder().decode(result.stdout))).toEqual( - cases.map((item) => ({ - ...item, - inScope: true, - contractValid: + cases.map( + (item) => item.path.trim().length > 0 && !/^[A-Za-z]:/.test(item.path) && !item.path.includes("\\") && !/[\u0000-\u001f]/u.test(item.path), - })), + ), ); }, ); From 280281d81678bb12f0668cd4f7244616ca5525d6 Mon Sep 17 00:00:00 2001 From: Kyle Brown Date: Thu, 10 Sep 2026 23:15:27 +0000 Subject: [PATCH 02/14] refactor: simplify candidate parsing and input ordering --- .../src/helpers/normalize-candidates.ts | 103 ++---------------- .../tests-ts/candidate-normalizer.test.ts | 27 ++++- 2 files changed, 29 insertions(+), 101 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index b0a338476..f64ad7888 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -78,83 +78,8 @@ function compare(left: string, right: string): number { return a.length - b.length; } -// JSON's numeric spelling matters: Python accepts 1 but rejects 1.0 as a line. -class JsonFloat { - constructor(readonly source: string) {} -} -function parseJson(source: string): unknown { - const tokens = - source.match( - /"(?:\\[\s\S]|[^"\\])*"|[{}\[\]:,]|-?(?:0|[1-9][0-9]*)(?:\.[0-9]+)?(?:[eE][+-]?[0-9]+)?|true|false|null|-?Infinity|NaN|[^ \t\r\n]/gu, - ) ?? []; - let index = 0; - const take = () => tokens[index++]; - function expect(token: string): void { - if (take() !== token) throw new Error(`expected ${token} in JSON`); - } - function value(): unknown { - const token = take(); - if (token === "{") { - const row: Row = Object.create(null) as Row; - if (tokens[index] === "}") { - index++; - return row; - } - while (true) { - const key = take(); - if (!key?.startsWith('"')) - throw new Error("expected a JSON property name"); - expect(":"); - row[JSON.parse(key) as string] = value(); - if (tokens[index] === "}") { - index++; - return row; - } - expect(","); - } - } - if (token === "[") { - const values: unknown[] = []; - if (tokens[index] === "]") { - index++; - return values; - } - while (true) { - values.push(value()); - if (tokens[index] === "]") { - index++; - return values; - } - expect(","); - } - } - if (token?.startsWith('"')) return JSON.parse(token) as string; - if (token === "true") return true; - if (token === "false") return false; - if (token === "null") return null; - if (token !== undefined && /^-?[0-9]+$/u.test(token)) return BigInt(token); - if ( - token !== undefined && - (/^-?(?:[0-9]+(?:\.[0-9]+)?(?:[eE][+-]?[0-9]+)?|Infinity)$/u.test( - token, - ) || - token === "NaN") - ) - return new JsonFloat(token); - throw new Error("expected a JSON value"); - } - const result = value(); - if (index !== tokens.length) throw new Error("extra data after JSON value"); - return result; -} - function object(value: unknown): value is Row { - return ( - typeof value === "object" && - value !== null && - !Array.isArray(value) && - !(value instanceof JsonFloat) - ); + return typeof value === "object" && value !== null && !Array.isArray(value); } function stableJson(value: unknown): string { @@ -364,8 +289,8 @@ function cweIds(row: Row): string[] { .map((number) => `CWE-${number}`); } -function positiveLine(value: unknown, field: string): bigint { - if (typeof value !== "bigint" || value < 1n) +function positiveLine(value: unknown, field: string): number { + if (typeof value !== "number" || !Number.isInteger(value) || value < 1) throw new Error(`${field}: expected a positive integer`); return value; } @@ -411,14 +336,14 @@ function normalizeLocations( lineCounts.set(key, lines); } const count = lineCounts.get(key)!; - if (end > BigInt(count)) + if (end > count) throw new Error(`line range ${start}-${end} exceeds ${name}:${count}`); if (typeof item.role !== "string" || !roles.includes(item.role)) throw new Error(`role: unsupported value ${String(item.role)}`); const location = { path: name, - start_line: Number(start), - end_line: Number(end), + start_line: start, + end_line: end, role: item.role, }; normalized.set(stableJson(location), location); @@ -589,19 +514,7 @@ export function normalizeCandidatesCommand( const scopePath = paths("in-scope-files")[0]!; const inputs = [ ...new Map(paths("input").map((path) => [pathKey(path), path])).values(), - ].sort((a, b) => { - const left = pathKey(a).split(sep), - right = pathKey(b).split(sep); - for ( - let index = 0; - index < Math.min(left.length, right.length); - index++ - ) { - const order = compare(left[index]!, right[index]!); - if (order !== 0) return order; - } - return left.length - right.length; - }); + ].sort((a, b) => compare(pathKey(a), pathKey(b))); if (inputs.some((path) => pathKey(path) === pathKey(output))) throw new Error("--out: must not also be an input"); if (pathKey(output) === pathKey(scopePath)) @@ -618,7 +531,7 @@ export function normalizeCandidatesCommand( for (const [index, line] of lines.entries()) { if (trim(line) === "") continue; try { - const row = parseJson(line); + const row: unknown = JSON.parse(line); if (!object(row)) throw new Error("expected a JSON object"); rows.push(normalizeCandidate(row, root, scope, lineCounts)); } catch (error) { diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts index 217df22b5..70d75d302 100644 --- a/sdk/typescript/tests-ts/candidate-normalizer.test.ts +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -322,12 +322,29 @@ describe("built candidate normalizer", () => { } }); - test("rejects noninteger numeric spellings, malformed rows, and invalid UTF-8 atomically", () => { + test.each(["1.0", "1e0"])( + "normalizes integral line spelling %s", + (spelling) => { + const f = fixture(); + const input = join(f.root, "raw.jsonl"); + const row = JSON.stringify(candidate([location("app/routes.py", 1)])); + write(input, row + "\n"); + expect(invoke(f, [input]).status).toBe(0); + const expected = readFileSync(f.output); + write( + input, + row.replace('"start_line":1', `"start_line":${spelling}`) + "\n", + ); + expect(invoke(f, [input]).status).toBe(0); + expect(readFileSync(f.output)).toEqual(expected); + }, + ); + + test("rejects invalid line values, malformed rows, and invalid UTF-8 atomically", () => { const f = fixture(); const input = join(f.root, "raw.jsonl"); for (const number of [ - "1.0", - "1e0", + "1.5", "true", "0", "-1", @@ -346,9 +363,7 @@ describe("built candidate normalizer", () => { write(f.output, "previous output\n"); const result = invoke(f, [input]); expect(result.status).toBe(2); - expect(result.stderr).toContain( - "start_line: expected a positive integer", - ); + expect(result.stderr).toContain("row 1:"); expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); } const prefix = Buffer.from( From 2f3b519f7ce26fe2c1cead7ee148d2b823584a4b Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 08:36:16 +0000 Subject: [PATCH 03/14] refactor(plugin): simplify candidate normalization checks --- .../mcp-app/src/artifact-discovery.ts | 26 ++++++++++++------- .../src/helpers/normalize-candidates.ts | 22 ++++------------ .../mcp-app/src/helpers/posix-path.ts | 2 +- .../mcp-app/tests/test_artifact_discovery.mjs | 10 ++++--- .../tests/test_compact_artifact_server.mjs | 4 +-- 5 files changed, 30 insertions(+), 34 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/artifact-discovery.ts b/plugins/codex-security/mcp-app/src/artifact-discovery.ts index c59bf7e60..e88de679a 100644 --- a/plugins/codex-security/mcp-app/src/artifact-discovery.ts +++ b/plugins/codex-security/mcp-app/src/artifact-discovery.ts @@ -141,7 +141,11 @@ export async function recordCodexSecurityDiscoveryCandidates( ]; // Verify the inventory is a context-bound regular file before normalization. - await readArtifactText(context, inventoryComponents, "discovery review inventory"); + await readArtifactText( + context, + inventoryComponents, + "discovery review inventory", + ); const inventoryPath = await artifactDestination( context, inventoryComponents, @@ -236,16 +240,18 @@ export async function listCodexSecurityCandidates( function discoveryNormalizationError( error: unknown, - privateValues: Array + privateValues: Array, ): Error { - const stderr = error && typeof error === "object" && "stderr" in error - ? error.stderr - : undefined; - let detail = typeof stderr === "string" - ? stderr.trim() - : Buffer.isBuffer(stderr) - ? stderr.toString("utf8").trim() - : ""; + const stderr = + error && typeof error === "object" && "stderr" in error + ? error.stderr + : undefined; + let detail = + typeof stderr === "string" + ? stderr.trim() + : Buffer.isBuffer(stderr) + ? stderr.toString("utf8").trim() + : ""; if (!detail) { return new Error( diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index f64ad7888..a3319825d 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -220,14 +220,6 @@ function readScope( } catch (error) { if (error instanceof SymlinkLoopError) throw error; if (allowMissing && (error as NodeJS.ErrnoException).code === "ENOENT") { - if ( - line.startsWith("/") || - line.split("/").includes("..") || - line.includes("\0") - ) - throw new Error( - `in-scope file row ${index + 1}: unsafe deleted path`, - ); try { scope.add( inside(resolvedPath(`${root}${sep}${line}`, false), root, true), @@ -313,11 +305,7 @@ function normalizeLocations( if (unknown.length) throw new Error(`locations: unsupported fields ${unknown.join(", ")}`); const [name, source] = relativeFile(item.path, root); - if ( - trim(name) === "" || - name.includes("\\") || - name.split("/").some((part) => part.includes(":")) - ) + if (trim(name) === "" || name.includes("\\") || name.includes(":")) throw new Error("path: expected a safe repository-relative POSIX path"); const start = positiveLine(item.start_line, "start_line"); const end = positiveLine( @@ -378,10 +366,10 @@ function normalizeCandidate( summary: textField(row, "summary")!, evidence: textField(row, "evidence")!, }; - const context = textField(row, "context", false); - if (context !== undefined) result.context = context; - const instance = textField(row, "instance", false); - if (instance !== undefined) result.instance = instance; + for (const field of ["context", "instance"] as const) { + const value = textField(row, field, false); + if (value !== undefined) result[field] = value; + } return result; } diff --git a/plugins/codex-security/mcp-app/src/helpers/posix-path.ts b/plugins/codex-security/mcp-app/src/helpers/posix-path.ts index 63a632c31..76bdf5ce9 100644 --- a/plugins/codex-security/mcp-app/src/helpers/posix-path.ts +++ b/plugins/codex-security/mcp-app/src/helpers/posix-path.ts @@ -7,7 +7,7 @@ export function decodePosixBytes(bytes: Buffer): string { if (isUtf8(bytes)) return bytes.toString("utf8"); // Match Python's surrogateescape for undecodable POSIX path bytes. let value = ""; - for (let offset = 0; offset < bytes.length; ) { + for (let offset = 0; offset < bytes.length;) { let decoded = false; for (let size = 1; size <= 4 && offset + size <= bytes.length; size++) { const part = bytes.subarray(offset, offset + size); diff --git a/plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs b/plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs index fd338ccf7..3c2d956c1 100644 --- a/plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs +++ b/plugins/codex-security/mcp-app/tests/test_artifact_discovery.mjs @@ -95,7 +95,9 @@ assert.deepEqual(toolSchemas.$defs.workbenchListCandidatesInput.required, [ "scanId", ]); -const root = await realpath(await mkdtemp(path.join(tmpdir(), "security-artifact-discovery-"))); +const root = await realpath( + await mkdtemp(path.join(tmpdir(), "security-artifact-discovery-")), +); const runtimePluginRoot = path.join(root, "plugin"); try { await build({ @@ -103,7 +105,7 @@ try { entryPoints: [path.join(pluginRoot, "mcp-app", "helpers-main.ts")], outfile: path.join(runtimePluginRoot, "mcp", "helpers.mjs"), format: "esm", - platform: "node" + platform: "node", }); if (process.platform === "win32") { const target = `win32-${process.arch}`; @@ -111,7 +113,7 @@ try { await mkdir(destination, { recursive: true }); await copyFile( path.join(pluginRoot, "native", "prebuilt", target, "windows.node"), - path.join(destination, "windows.node") + path.join(destination, "windows.node"), ); } const repoRoot = path.join(root, "repository"); @@ -578,7 +580,7 @@ async function createContext(root, repoRoot, name, layout) { repoRoot, layout, pluginRoot: runtimePluginRoot, - pythonCommand: path.join(root, "python-must-not-run") + pythonCommand: path.join(root, "python-must-not-run"), }; } diff --git a/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs b/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs index ceb3af5b3..35599cc67 100644 --- a/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs +++ b/plugins/codex-security/mcp-app/tests/test_compact_artifact_server.mjs @@ -1341,7 +1341,7 @@ async function testDiscoveryWorkerToolList(bundle) { CODEX_SECURITY_REPO_ROOT: repoRoot, CODEX_SECURITY_ARTIFACT_LAYOUT: "worker", CODEX_SECURITY_SCAN_ID: scanId, - CODEX_SECURITY_PLUGIN_ROOT: bundledPluginRoot + CODEX_SECURITY_PLUGIN_ROOT: bundledPluginRoot, }); try { assert.deepEqual( @@ -1723,7 +1723,7 @@ async function bundleEntrypoint(entrypoint, outfile) { bundle: true, define: { __dirname: JSON.stringify(path.join(bundledPluginRoot, "mcp")), - "import.meta.url": "__filename" + "import.meta.url": "__filename", }, entryPoints: [path.join(applicationRoot, entrypoint)], external: ["fsevents"], From c85f508e0f50de644f3dec4a9481f2e4f1b5b813 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 10:16:22 +0000 Subject: [PATCH 04/14] refactor(plugin): streamline candidate normalization flow --- .../src/helpers/normalize-candidates.ts | 202 ++++++++---------- .../src/helpers/resolve-security-md.ts | 6 +- 2 files changed, 97 insertions(+), 111 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index a3319825d..de7e9362b 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -14,13 +14,13 @@ import { basename, dirname, isAbsolute, join, relative, sep } from "node:path"; import { decodePosixBytes, encodePosixPath, - resolvePosixPath, SymlinkLoopError, } from "./posix-path"; import { expandHome, HomeExpansionError, parsedPath, + resolvedPath as resolveFilePath, windowsRelativePath, } from "./resolve-security-md"; import { windowsBinding } from "../native"; @@ -83,18 +83,15 @@ function object(value: unknown): value is Row { } function stableJson(value: unknown): string { - if (typeof value === "string") { - if (/[\ud800-\udfff]/u.test(value)) + return JSON.stringify(value, (_key, item: unknown) => { + if (typeof item === "string" && /[\ud800-\udfff]/u.test(item)) throw new Error("UTF-8 cannot encode an unpaired surrogate"); - return JSON.stringify(value); - } - if (Array.isArray(value)) return `[${value.map(stableJson).join(",")}]`; - if (object(value)) - return `{${Object.keys(value) - .sort(compare) - .map((key) => `${stableJson(key)}:${stableJson(value[key])}`) - .join(",")}}`; - return JSON.stringify(value); + return object(item) + ? Object.fromEntries( + Object.entries(item).sort(([a], [b]) => compare(a, b)), + ) + : item; + }); } const windows = process.platform === "win32"; @@ -105,35 +102,24 @@ const readFile = (path: string) => windows ? windowsFiles().readFile(fsPath(path)) : readFileSync(fsPath(path)); const stat = (path: string) => windows ? windowsFiles().stat(fsPath(path)) : statSync(fsPath(path)); -const pathKey = (value: string) => - process.platform === "win32" ? value.toLowerCase() : value; +const pathKey = (value: string) => (windows ? value.toLowerCase() : value); function resolvedPath(value: string, strict = true): string { - if (process.platform !== "win32") - return decodePosixBytes( - resolvePosixPath(encodePosixPath(parsedPath(value)), strict), - ); - try { - return pathText(windowsFiles().realpath(widePath(value), strict)); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ELOOP") - throw new SymlinkLoopError(`Symlink loop from ${value}`); - throw error; - } + const path = resolveFilePath( + fsPath(windows ? value : parsedPath(value)), + strict, + ); + return windows ? pathText(path) : decodePosixBytes(path); } function inside(path: string, root: string, allowMissing = false): string { - let result: string | undefined; - if (windows) { - const bytes = windowsRelativePath( - widePath(path), - widePath(root), - allowMissing, - ); - result = bytes === undefined ? undefined : pathText(bytes); - } else { - result = relative(root, path); - } + const result = windows + ? windowsRelativePath( + widePath(path), + widePath(root), + allowMissing, + )?.toString("utf16le") + : relative(root, path); if ( result === undefined || isAbsolute(result) || @@ -147,12 +133,11 @@ function inside(path: string, root: string, allowMissing = false): string { function relativeFile(value: unknown, root: string): [string, string] { if (typeof value !== "string" || value === "" || value.includes("\0")) throw new Error("path: expected a non-empty repository-relative path"); - const raw = - process.platform === "win32" ? value.replaceAll("\\", "/") : value; + const raw = windows ? value.replaceAll("\\", "/") : value; if ( raw.startsWith("/") || raw.split("/").includes("..") || - (process.platform === "win32" && /^[A-Za-z]:/u.test(raw)) + (windows && /^[A-Za-z]:/u.test(raw)) ) throw new Error( "path: expected a repository-relative path without traversal", @@ -181,7 +166,7 @@ function readScope( } }; const carriage = new Map(); - if (process.platform !== "win32") { + if (!windows) { for (const line of lines) { if (line.endsWith("\r") && line !== "\r") carriage.set(line, [isFile(line), isFile(line.slice(0, -1))]); @@ -196,7 +181,7 @@ function readScope( const scope = new Set(); for (const [index, original] of lines.entries()) { let line = original; - if (process.platform === "win32" || line === "\r") { + if (windows || line === "\r") { if (line.endsWith("\r")) line = line.slice(0, -1); } else if (line.endsWith("\r")) { const [literal, stripped] = carriage.get(line)!; @@ -373,43 +358,33 @@ function normalizeCandidate( return result; } -function combine(rows: Candidate[]): (Candidate & { candidate_id: string })[] { - const groups = new Map(); - for (const row of rows) { - const key = stableJson({ - cwe_ids: row.cwe_ids, - locations: row.locations, - instance: row.instance ?? null, +function combine(groups: Map) { + return [...groups] + .sort(([a], [b]) => compare(a, b)) + .map(([key, group]) => { + const merged = (field: "summary" | "evidence" | "context") => + [ + ...new Set( + group + .map((row) => row[field]) + .filter((value): value is string => value !== undefined), + ), + ] + .sort(compare) + .join("\n"); + const result = { + ...group[0]!, + candidate_id: `candidate-${createHash("sha256").update(key).digest("hex").slice(0, 16)}`, + summary: merged("summary"), + evidence: merged("evidence"), + }; + const context = merged("context"); + if (context !== "") result.context = context; + return result; }); - const group = groups.get(key) ?? []; - group.push(row); - groups.set(key, group); - } - return [...groups.keys()].sort(compare).map((key) => { - const group = groups.get(key)!; - const merged = (field: "summary" | "evidence" | "context") => - [ - ...new Set( - group - .map((row) => row[field]) - .filter((value): value is string => value !== undefined), - ), - ] - .sort(compare) - .join("\n"); - const result = { - ...group[0]!, - candidate_id: `candidate-${createHash("sha256").update(key).digest("hex").slice(0, 16)}`, - summary: merged("summary"), - evidence: merged("evidence"), - }; - const context = merged("context"); - if (context !== "") result.context = context; - return result; - }); } -function argumentsFor(args: string[]): Record { +function argumentsFor(args: string[]): Map { const names = [ "input", "out", @@ -439,7 +414,7 @@ function argumentsFor(args: string[]): Record { return undefined; throw new Error(`unrecognized argument: ${value}`); } - const values: Record = {}; + const values = new Map(); for (let index = 0; index < args.length; index++) { const argument = args[index]!; const name = option(argument); @@ -449,7 +424,7 @@ function argumentsFor(args: string[]): Record { if (name === "help" || name === "allow-missing-in-scope") { if (equals !== -1) throw new Error(`argument --${name} does not take a value`); - values[name] = true; + values.set(name, []); if (name === "help") return values; continue; } @@ -468,10 +443,10 @@ function argumentsFor(args: string[]): Record { throw new Error( `argument --${name}: expected ${name === "input" ? "at least one argument" : "one argument"}`, ); - values[name] = found; + values.set(name, found); } for (const name of ["input", "out", "repo-root", "in-scope-files"]) { - if (values[name] === undefined) throw new Error(`--${name} is required`); + if (!values.has(name)) throw new Error(`--${name} is required`); } return values; } @@ -482,7 +457,7 @@ export function normalizeCandidatesCommand( ): number { try { const values = argumentsFor(args); - if (values.help) { + if (values.has("help")) { console.log( "Validate and combine security-scan candidates into deterministic JSONL.\n", ); @@ -492,9 +467,11 @@ export function normalizeCandidatesCommand( return 0; } const paths = (name: string, strict = true) => - (values[name] as string[]).map((value) => - resolvedPath(expandHome(parsedPath(value), posixHome), strict), - ); + values + .get(name)! + .map((value) => + resolvedPath(expandHome(parsedPath(value), posixHome), strict), + ); const root = paths("repo-root")[0]!; if (!stat(root).isDirectory()) throw new Error("--repo-root: expected a directory"); @@ -510,69 +487,78 @@ export function normalizeCandidatesCommand( const scope = readScope( scopePath, root, - values["allow-missing-in-scope"] === true, + values.has("allow-missing-in-scope"), ); const lineCounts = new Map(); - const rows: Candidate[] = []; + const groups = new Map(); + let rowCount = 0; for (const source of inputs) { const lines = decodeUtf8(readFile(source)).split(/\r\n|[\r\n]/u); for (const [index, line] of lines.entries()) { if (trim(line) === "") continue; + let candidate: Candidate; try { const row: unknown = JSON.parse(line); if (!object(row)) throw new Error("expected a JSON object"); - rows.push(normalizeCandidate(row, root, scope, lineCounts)); + candidate = normalizeCandidate(row, root, scope, lineCounts); } catch (error) { if (error instanceof SymlinkLoopError) throw error; throw new Error( `${source} row ${index + 1}: ${(error as Error).message}`, ); } + const key = stableJson({ + cwe_ids: candidate.cwe_ids, + locations: candidate.locations, + instance: candidate.instance ?? null, + }); + const group = groups.get(key) ?? []; + group.push(candidate); + groups.set(key, group); + rowCount++; } } - const combined = combine(rows); + const combined = combine(groups); if (windows) windowsFiles().mkdir(fsPath(dirname(output))); else mkdirSync(fsPath(dirname(output)), { recursive: true }); - const temporary = join( - dirname(output), - `.${basename(output)}.${randomBytes(6).toString("base64url")}.tmp`, + const temporary = fsPath( + join( + dirname(output), + `.${basename(output)}.${randomBytes(6).toString("base64url")}.tmp`, + ), ); let created = false; + function* contents() { + created = true; + for (const row of combined) + yield Buffer.from(`${stableJson(row)}${windows ? "\r\n" : "\n"}`); + } try { if (windows) { - function* contents() { - created = true; - for (const row of combined) - yield Buffer.from(`${stableJson(row)}\r\n`); - } - windowsFiles().writeFile(fsPath(temporary), contents(), true); - windowsFiles().rename(fsPath(temporary), fsPath(output)); + windowsFiles().writeFile(temporary, contents(), true); + windowsFiles().rename(temporary, fsPath(output)); } else { - const descriptor = openSync(fsPath(temporary), "wx", 0o600); - created = true; + const descriptor = openSync(temporary, "wx", 0o600); try { - for (const row of combined) - writeFileSync(descriptor, `${stableJson(row)}\n`, "utf8"); + for (const chunk of contents()) writeFileSync(descriptor, chunk); } finally { closeSync(descriptor); } - renameSync(fsPath(temporary), fsPath(output)); + renameSync(temporary, fsPath(output)); } } finally { try { if (created) { - if (windows) windowsFiles().unlink(fsPath(temporary)); - else unlinkSync(fsPath(temporary)); + if (windows) windowsFiles().unlink(temporary); + else unlinkSync(temporary); } } catch (error) { if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; } } - const message = `Combined ${rows.length} candidate rows into ${combined.length} rows in ${output}\n`; + const message = `Combined ${rowCount} candidate rows into ${combined.length} rows in ${output}\n`; process.stdout.write( - process.platform === "win32" - ? message.replace(/\n/gu, "\r\n") - : encodePosixPath(message), + windows ? message.replace(/\n/gu, "\r\n") : encodePosixPath(message), ); return 0; } catch (error) { diff --git a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts index 9e38dd18d..ef3197833 100644 --- a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts +++ b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts @@ -60,10 +60,10 @@ export function parsedPath(value: string): string { return root + parts.join(sep) || "."; } -function resolvedPath(path: Buffer): Buffer { - if (process.platform !== "win32") return resolvePosixPath(path); +export function resolvedPath(path: Buffer, strict = true): Buffer { + if (process.platform !== "win32") return resolvePosixPath(path, strict); try { - return windowsFiles().realpath(path); + return windowsFiles().realpath(path, strict); } catch (error) { if ((error as NodeJS.ErrnoException).code === "ELOOP") throw new SymlinkLoopError(`Symlink loop from ${decodePath(path)}`); From e3e2afccea56f1e89466fa3325e9deb70cc501d7 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 10:46:01 +0000 Subject: [PATCH 05/14] fix(package): scan Brotli contents after decompression --- sdk/typescript/scripts/check-package.mjs | 47 ++-------- .../scripts/package-internal-references.mjs | 50 +++++++++++ .../package-internal-references.test.ts | 86 +++++++++++++++++++ 3 files changed, 142 insertions(+), 41 deletions(-) create mode 100644 sdk/typescript/scripts/package-internal-references.mjs create mode 100644 sdk/typescript/tests-ts/package-internal-references.test.ts diff --git a/sdk/typescript/scripts/check-package.mjs b/sdk/typescript/scripts/check-package.mjs index 4a9471a7f..daef7e9bd 100644 --- a/sdk/typescript/scripts/check-package.mjs +++ b/sdk/typescript/scripts/check-package.mjs @@ -2,7 +2,11 @@ import { spawnSync } from "node:child_process"; import { createHash } from "node:crypto"; import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; -import { brotliDecompressSync, gunzipSync } from "node:zlib"; +import { gunzipSync } from "node:zlib"; +import { + assertPublicPackageContents, + MAX_EXPANDED_ASSET_BYTES, +} from "./package-internal-references.mjs"; import { assertExpectedGitHead } from "./package-provenance.mjs"; import { packageSmokeTimeouts } from "./package-smoke-timeouts.mjs"; import { regularTarListingLines } from "./package-tar-listing.mjs"; @@ -25,7 +29,6 @@ if (archive === undefined || args.length > 2) { ); } -const MAX_EXPANDED_ASSET_BYTES = 32 * 1024 * 1024; const archiveBytes = gunzipSync(readFileSync(archive), { maxOutputLength: MAX_EXPANDED_ASSET_BYTES, }); @@ -335,40 +338,6 @@ assertExpectedGitHead( process.env.CODEX_SECURITY_EXPECTED_GIT_HEAD, ); -const internalMarker = - /(?:internal\.api\.openai\.org|gateway\.[a-z0-9.-]*internal|\.openai\.org|openai\.firewall\.socket\.dev|socket\x2dfirewall\x2dregistry|openai\.(?:enterprise\.)?slack\.com|app\.slack\.com\/client|(?:app\.notion\.com\/p|notion\.so)\/openai|linear\.app\/openai|(?:github\.com[:/]|api\.github\.com\/repos\/|raw\.githubusercontent\.com\/)openai\/openai(?:\.git)?(?:[^a-z0-9_-]|$)|LicenseRef\x2dProprietary|\/Users\/|\/home\/dev-user|flow\.apps\.openai\.org|(?:^|[^a-z0-9_-])go\/[a-z0-9_-]+)/iu; - -const payloads = [archiveBytes.toString("utf8")]; -const compressedFiles = [...files].filter((file) => /\.br$/iu.test(file)); -const compressedParts = new Map(); -for (const file of files) { - const match = /^(.*\.br)\.part-([0-9]+)$/iu.exec(file); - if (match === null) continue; - const [, name, part] = match; - const parts = compressedParts.get(name) ?? []; - parts.push({ file, part: Number(part) }); - compressedParts.set(name, parts); -} - -function brotliPayload(bytes, file) { - const result = brotliDecompressSync(bytes, { - info: true, - maxOutputLength: MAX_EXPANDED_ASSET_BYTES, - }); - if (result.engine.bytesWritten !== bytes.length) { - throw new Error(`npm tarball contains trailing Brotli data: ${file}.`); - } - return result.buffer; -} - -for (const file of compressedFiles) { - payloads.push(brotliPayload(archiveFile(file), file).toString("utf8")); -} -for (const parts of compressedParts.values()) { - parts.sort((left, right) => left.part - right.part); - const bytes = Buffer.concat(parts.map(({ file }) => archiveFile(file))); - payloads.push(brotliPayload(bytes, parts[0].file).toString("utf8")); -} for (const file of files) { if (/\.png$/iu.test(file)) { const digest = createHash("sha256").update(archiveFile(file)).digest("hex"); @@ -378,11 +347,7 @@ for (const file of files) { } } -for (const contents of payloads) { - if (internalMarker.test(contents)) { - throw new Error("npm tarball contains an internal reference."); - } -} +assertPublicPackageContents(archiveBytes, archiveFiles); if (args.length === 1) { const smoke = spawnSync( diff --git a/sdk/typescript/scripts/package-internal-references.mjs b/sdk/typescript/scripts/package-internal-references.mjs new file mode 100644 index 000000000..76a2a5be6 --- /dev/null +++ b/sdk/typescript/scripts/package-internal-references.mjs @@ -0,0 +1,50 @@ +import { brotliDecompressSync } from "node:zlib"; + +export const MAX_EXPANDED_ASSET_BYTES = 32 * 1024 * 1024; + +const internalMarker = + /(?:internal\.api\.openai\.org|gateway\.[a-z0-9.-]*internal|\.openai\.org|openai\.firewall\.socket\.dev|socket\x2dfirewall\x2dregistry|openai\.(?:enterprise\.)?slack\.com|app\.slack\.com\/client|(?:app\.notion\.com\/p|notion\.so)\/openai|linear\.app\/openai|(?:github\.com[:/]|api\.github\.com\/repos\/|raw\.githubusercontent\.com\/)openai\/openai(?:\.git)?(?:[^a-z0-9_-]|$)|LicenseRef\x2dProprietary|\/Users\/|\/home\/dev-user|flow\.apps\.openai\.org|(?:^|[^a-z0-9_-])go\/[a-z0-9_-]+)/iu; + +export function assertPublicPackageContents(archiveBytes, archiveFiles) { + const uncompressed = Buffer.from(archiveBytes); + const payloads = [uncompressed]; + const compressedParts = new Map(); + + function brotliPayload(bytes, file) { + const result = brotliDecompressSync(bytes, { + info: true, + maxOutputLength: MAX_EXPANDED_ASSET_BYTES, + }); + if (result.engine.bytesWritten !== bytes.length) { + throw new Error(`npm tarball contains trailing Brotli data: ${file}.`); + } + return result.buffer; + } + + for (const [file, bytes] of archiveFiles) { + const match = /^(.*\.br)\.part-([0-9]+)$/iu.exec(file); + if (match !== null) { + const [, name, part] = match; + const parts = compressedParts.get(name) ?? []; + parts.push({ file, part: Number(part), bytes }); + compressedParts.set(name, parts); + } else if (/\.br$/iu.test(file)) { + payloads.push(brotliPayload(bytes, file)); + } else { + continue; + } + // Scan compressed members after decoding; retain tar headers and other bytes. + const start = bytes.byteOffset - archiveBytes.byteOffset; + uncompressed.fill(0, start, start + bytes.length); + } + for (const parts of compressedParts.values()) { + parts.sort((left, right) => left.part - right.part); + const bytes = Buffer.concat(parts.map((part) => part.bytes)); + payloads.push(brotliPayload(bytes, parts[0].file)); + } + for (const contents of payloads) { + if (internalMarker.test(contents.toString("utf8"))) { + throw new Error("npm tarball contains an internal reference."); + } + } +} diff --git a/sdk/typescript/tests-ts/package-internal-references.test.ts b/sdk/typescript/tests-ts/package-internal-references.test.ts new file mode 100644 index 000000000..983e36bb3 --- /dev/null +++ b/sdk/typescript/tests-ts/package-internal-references.test.ts @@ -0,0 +1,86 @@ +import { brotliCompressSync, brotliDecompressSync } from "node:zlib"; +import { describe, expect, test } from "bun:test"; + +const { assertPublicPackageContents } = (await import( + new URL("../scripts/package-internal-references.mjs", import.meta.url).href +)) as { + assertPublicPackageContents: ( + archiveBytes: Buffer, + archiveFiles: Map, + ) => void; +}; + +function inspect(entries: [string, Buffer][], metadata = "") { + const chunks = entries.flatMap(([path, contents]) => [ + Buffer.from(`${path}\0${metadata}\0`), + contents, + ]); + const archive = Buffer.concat(chunks); + const files = new Map(); + let offset = 0; + for (const [index, [path, contents]] of entries.entries()) { + offset += chunks[index * 2]!.length; + files.set(path, archive.subarray(offset, offset + contents.length)); + offset += contents.length; + } + assertPublicPackageContents(archive, files); +} + +// Valid compressed bytes contain a marker-like sequence absent from the text. +const compressedFixture = Buffer.from( + "G58AAGRgnikP5mWEcAF4L/70rY0DMLgq+du/Il7KM3pABrBwrD1HTy9f2iPXD9nLS+eeyfBS9jDPuLehk60dpTuQ79FQP7p/aAf/wTf6JztZL0zOmf5MxsTNWvJn24c4O36Wn683/mQze6hJHFvvx3oTW0UrVZo7fnHstXN8INaut32GZmyr3snPxr3yZ7ggGbghAw==", + "base64", +); + +function split(contents: Buffer): [string, Buffer][] { + const midpoint = Math.floor(contents.length / 2); + return [ + ["package/runtime.br.part-001", contents.subarray(midpoint)], + ["package/runtime.br.part-000", contents.subarray(0, midpoint)], + ]; +} + +describe("npm package internal references", () => { + test("accepts marker-like compressed bytes in standalone and split Brotli", () => { + expect(compressedFixture.toString("utf8")).toContain("=GO/_"); + expect( + brotliDecompressSync(compressedFixture).toString("utf8"), + ).not.toContain("/"); + expect(() => + inspect([["package/runtime.br", compressedFixture]]), + ).not.toThrow(); + expect(() => inspect(split(compressedFixture))).not.toThrow(); + }); + + test("rejects references in decoded standalone and split Brotli", () => { + const compressed = brotliCompressSync(Buffer.from("go/example")); + for (const entries of [ + [["package/runtime.br", compressed]] as [string, Buffer][], + split(compressed), + ]) { + expect(() => inspect(entries)).toThrow("internal reference"); + } + }); + + test("keeps scanning uncompressed source, native bytes, paths, and tar metadata", () => { + for (const path of ["package/index.js", "package/native/runtime.node"]) { + expect(() => inspect([[path, Buffer.from("go/example")]])).toThrow( + "internal reference", + ); + } + expect(() => + inspect([["package/go/example.br", compressedFixture]]), + ).toThrow("internal reference"); + expect(() => + inspect([["package/runtime.br", compressedFixture]], "go/example"), + ).toThrow("internal reference"); + }); + + test("rejects malformed and trailing Brotli in standalone and split members", () => { + const trailing = Buffer.concat([compressedFixture, Buffer.from("tail")]); + for (const bytes of [compressedFixture.subarray(0, -1), trailing]) { + expect(() => inspect([["package/runtime.br", bytes]])).toThrow(); + expect(() => inspect(split(bytes))).toThrow(); + } + }); +}); From 0fd8e0a6d0ad8d187f612b89dbaa7922b6fb63c4 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 14:44:37 +0000 Subject: [PATCH 06/14] refactor(plugin): share Unicode ordering and simplify migration fixtures --- .../mcp-app/src/artifact-discovery.ts | 7 ++-- .../src/helpers/normalize-candidates.ts | 11 +----- .../src/helpers/resolve-security-md.ts | 19 ++-------- .../mcp-app/src/helpers/utf8.ts | 13 +++++++ .../native/examples/windows-wide-launcher.rs | 36 ++++++++---------- .../tests-ts/candidate-normalizer.test.ts | 15 +++----- .../tests-ts/compact-diff-scan.test.ts | 25 ++++++------ sdk/typescript/tests-ts/runtime.test.ts | 38 +++++++------------ 8 files changed, 67 insertions(+), 97 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/artifact-discovery.ts b/plugins/codex-security/mcp-app/src/artifact-discovery.ts index e88de679a..86a9f190c 100644 --- a/plugins/codex-security/mcp-app/src/artifact-discovery.ts +++ b/plugins/codex-security/mcp-app/src/artifact-discovery.ts @@ -163,10 +163,9 @@ export async function recordCodexSecurityDiscoveryCandidates( try { await fs.chmod(temporaryDirectory, 0o700); - const content = - candidates.length === 0 - ? "" - : `${candidates.map((candidate) => JSON.stringify(candidate)).join("\n")}\n`; + const content = candidates + .map((candidate) => `${JSON.stringify(candidate)}\n`) + .join(""); await fs.writeFile(temporaryInput, content, { encoding: "utf8", flag: "wx", diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index de7e9362b..f85ded340 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -1,4 +1,4 @@ -import { decodeUtf8 } from "./utf8"; +import { compareUnicode as compare, decodeUtf8 } from "./utf8"; import { createHash, randomBytes } from "node:crypto"; import { closeSync, @@ -69,15 +69,6 @@ interface Candidate { instance?: string; } -function compare(left: string, right: string): number { - const a = Array.from(left, (value) => value.codePointAt(0)!); - const b = Array.from(right, (value) => value.codePointAt(0)!); - for (let index = 0; index < Math.min(a.length, b.length); index++) { - if (a[index] !== b[index]) return a[index]! - b[index]!; - } - return a.length - b.length; -} - function object(value: unknown): value is Row { return typeof value === "object" && value !== null && !Array.isArray(value); } diff --git a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts index ef3197833..6dcbd552d 100644 --- a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts +++ b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts @@ -1,4 +1,4 @@ -import { decodeUtf8 } from "./utf8"; +import { compareUnicode, decodeUtf8 } from "./utf8"; import { closeSync, lstatSync, @@ -226,19 +226,6 @@ function asciiJson(value: string): string { ); } -function comparePaths(left: string, right: string): number { - let leftIndex = 0; - let rightIndex = 0; - while (leftIndex < left.length && rightIndex < right.length) { - const leftPoint = left.codePointAt(leftIndex)!; - const rightPoint = right.codePointAt(rightIndex)!; - if (leftPoint !== rightPoint) return leftPoint - rightPoint; - leftIndex += leftPoint > 0xffff ? 2 : 1; - rightIndex += rightPoint > 0xffff ? 2 : 1; - } - return left.length - right.length; -} - function listSecurityMd(repo: string, posixHome: string | undefined): string[] { const root = resolveRoot(repo, posixHome); const policies: string[] = []; @@ -252,7 +239,7 @@ function listSecurityMd(repo: string, posixHome: string | undefined): string[] { name: decodePath(entry.name), entry, })); - entries.sort((left, right) => comparePaths(left.name, right.name)); + entries.sort((left, right) => compareUnicode(left.name, right.name)); for (const { bytes, name, entry: listedEntry } of entries) { if (name === ".git") continue; const path = appendPath(directory, bytes); @@ -289,7 +276,7 @@ function listSecurityMd(repo: string, posixHome: string | undefined): string[] { } } walk(root, ""); - return policies.sort(comparePaths); + return policies.sort(compareUnicode); } function readPolicy(path: Buffer, displayedPath: Buffer): string { diff --git a/plugins/codex-security/mcp-app/src/helpers/utf8.ts b/plugins/codex-security/mcp-app/src/helpers/utf8.ts index 5b2e71e4d..e80c47974 100644 --- a/plugins/codex-security/mcp-app/src/helpers/utf8.ts +++ b/plugins/codex-security/mcp-app/src/helpers/utf8.ts @@ -1,5 +1,18 @@ import { isUtf8 } from "node:buffer"; +export function compareUnicode(left: string, right: string): number { + let leftIndex = 0; + let rightIndex = 0; + while (leftIndex < left.length && rightIndex < right.length) { + const leftPoint = left.codePointAt(leftIndex)!; + const rightPoint = right.codePointAt(rightIndex)!; + if (leftPoint !== rightPoint) return leftPoint - rightPoint; + leftIndex += leftPoint > 0xffff ? 2 : 1; + rightIndex += rightPoint > 0xffff ? 2 : 1; + } + return left.length - right.length; +} + export function decodeUtf8(bytes: Buffer): string { // Node 20's fatal TextDecoder can silently replace invalid input bytes. if (!isUtf8(bytes)) diff --git a/plugins/codex-security/native/examples/windows-wide-launcher.rs b/plugins/codex-security/native/examples/windows-wide-launcher.rs index aaf451eab..f0b182259 100644 --- a/plugins/codex-security/native/examples/windows-wide-launcher.rs +++ b/plugins/codex-security/native/examples/windows-wide-launcher.rs @@ -221,7 +221,7 @@ fn main() -> std::io::Result<()> { "Windows policy helper lost directory names", )); } - for sentinel in sentinels { + for sentinel in &sentinels { if fs::read(sentinel)? != b"output sentinel" { return Err(io::Error::other( "Windows policy helper changed a replacement output", @@ -272,9 +272,10 @@ fn main() -> std::io::Result<()> { fs::write( repo.join(&input_name), concat!( - "{\"cwe_ids\":[\"CWE-89\"],\"locations\":[{\"path\":\"source.py\",", - "\"start_line\":1,\"role\":\"entrypoint\"}],\"summary\":\"wide paths\",", - "\"evidence\":\"source evidence\"}\n", + r#"{"cwe_ids":["CWE-89"],"locations":[{"path":"source.py","#, + r#""start_line":1,"role":"entrypoint"}],"summary":"wide paths","#, + r#""evidence":"source evidence"}"#, + "\n", ), )?; let output_link = "i\u{0307}.jsonl"; @@ -283,10 +284,11 @@ fn main() -> std::io::Result<()> { std::os::windows::fs::symlink_file(output_link, repo.join("İ.jsonl"))?; } let expected = concat!( - "{\"candidate_id\":\"candidate-a69fa65a28ed4e55\",\"cwe_ids\":[\"CWE-89\"],", - "\"evidence\":\"source evidence\",\"locations\":[{\"end_line\":1,", - "\"path\":\"source.py\",\"role\":\"entrypoint\",\"start_line\":1}],", - "\"summary\":\"wide paths\"}\r\n", + r#"{"candidate_id":"candidate-a69fa65a28ed4e55","cwe_ids":["CWE-89"],"#, + r#""evidence":"source evidence","locations":[{"end_line":1,"#, + r#""path":"source.py","role":"entrypoint","start_line":1}],"#, + r#""summary":"wide paths"}"#, + "\r\n", ); let candidate = |repo_arg: &Path, input: &Path, scope: &Path, output: &Path| { Command::new(&node) @@ -304,13 +306,11 @@ fn main() -> std::io::Result<()> { .env("USERPROFILE", &repo) .output() }; + fs::write(&output, "previous output")?; for (index, prefix) in [repo.clone(), PathBuf::from("~"), PathBuf::from(".")] .into_iter() .enumerate() { - if index == 0 { - fs::write(&output, "previous output")?; - } let child = candidate( &prefix, &prefix.join(&input_name), @@ -355,15 +355,11 @@ fn main() -> std::io::Result<()> { )); } } - for cwd in &cwds { - for repository in &repos { - if fs::read(root.join(cwd).join(repository).join(&replacement_output))? - != b"output sentinel" - { - return Err(io::Error::other( - "Candidate helper changed a replacement output", - )); - } + for sentinel in &sentinels { + if fs::read(sentinel)? != b"output sentinel" { + return Err(io::Error::other( + "Candidate helper changed a replacement output", + )); } } println!( diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts index 70d75d302..6923ab0dc 100644 --- a/sdk/typescript/tests-ts/candidate-normalizer.test.ts +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -343,6 +343,7 @@ describe("built candidate normalizer", () => { test("rejects invalid line values, malformed rows, and invalid UTF-8 atomically", () => { const f = fixture(); const input = join(f.root, "raw.jsonl"); + write(f.output, "previous output\n"); for (const number of [ "1.5", "true", @@ -360,7 +361,6 @@ describe("built candidate normalizer", () => { `"start_line":${number}`, ) + "\n", ); - write(f.output, "previous output\n"); const result = invoke(f, [input]); expect(result.status).toBe(2); expect(result.stderr).toContain("row 1:"); @@ -385,7 +385,9 @@ describe("built candidate normalizer", () => { '{"locations":', "\ufeff{}", Buffer.from([0xff]), - JSON.stringify(candidate([], { summary: "\ud800" })), + ...["summary", "evidence"].map((field) => + JSON.stringify(candidate(undefined, { [field]: "\ud800" })), + ), ]) { write(input, invalid); const result = invoke(f, [input]); @@ -394,11 +396,6 @@ describe("built candidate normalizer", () => { } write(input, JSON.stringify(candidate()) + "\r\n\rnot-json\n"); expect(invoke(f, [input]).stderr).toContain("raw.jsonl row 3:"); - write( - input, - JSON.stringify(candidate(undefined, { evidence: "\ud800" })) + "\n", - ); - expect(invoke(f, [input]).status).toBe(2); expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); expect(readdirSync(f.root).some((name) => name.endsWith(".tmp"))).toBe( false, @@ -478,8 +475,8 @@ describe("built candidate normalizer", () => { "instance: expected a non-empty string", ], ]; + write(f.output, "previous output\n"); for (const [row, message] of cases) { - write(f.output, "previous output\n"); const result = run(f, [[candidate(), row]]); expect(result.status).toBe(2); expect(result.stderr).toContain("candidates-0.jsonl row 2:"); @@ -494,8 +491,8 @@ describe("built candidate normalizer", () => { const f = fixture(); write(join(f.repo, "\ufffd.py"), "replacement sibling\n"); write(f.scope, "\ufffd.py\n"); + write(f.output, "previous output\n"); for (const name of ["\ud800.py", "\udc00.py"]) { - write(f.output, "previous output\n"); const result = run(f, [[candidate([location(name, 1)])]]); expect(result.status).toBe(2); expect(result.stderr).toContain("unpaired surrogate"); diff --git a/sdk/typescript/tests-ts/compact-diff-scan.test.ts b/sdk/typescript/tests-ts/compact-diff-scan.test.ts index db0ddf668..087eeda64 100644 --- a/sdk/typescript/tests-ts/compact-diff-scan.test.ts +++ b/sdk/typescript/tests-ts/compact-diff-scan.test.ts @@ -378,29 +378,26 @@ describe("compact diff scan", () => { .map((entry) => JSON.stringify(entry)) .join("\n") + "\n", ); - const args = [ - "--input", - input, - "--out", - output, - "--repo-root", - repository, - "--in-scope-files", - inventory, - ]; - const normalize = (...options: string[]) => spawnSync( process.execPath, [ join(PLUGIN_ROOT, "mcp", "helpers.mjs"), "normalize-candidates", + "--input", + input, + "--out", + output, + "--repo-root", + repository, + "--in-scope-files", + inventory, ...options, ], { encoding: "utf8" }, ); - expect(normalize(...args).status).toBe(2); - const accepted = normalize(...args, "--allow-missing-in-scope"); + expect(normalize().status).toBe(2); + const accepted = normalize("--allow-missing-in-scope"); expect(accepted.status, accepted.stderr).toBe(0); const contents = readFileSync(output, "utf8"); expect(contents).toContain("Résumé: missing guard"); @@ -414,7 +411,7 @@ describe("compact diff scan", () => { ]); writeFileSync(inventory, "../escaped.py\nsrc/handler.py\n"); - const escaped = normalize(...args, "--allow-missing-in-scope"); + const escaped = normalize("--allow-missing-in-scope"); expect(escaped.status).toBe(2); expect(escaped.stderr).toContain("in-scope file row 1"); }); diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index cbb9d504c..53d710c6f 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -834,10 +834,10 @@ describe("plugin runtime preparation", () => { expect(await readFile(normalizer, "utf8")).toBe( await readFile(join(sourcePlugin, "mcp", "helpers.mjs"), "utf8"), ); - const locations: (Record | null)[] = []; + const locations: { path: string }[] = []; + const input = join(root, "candidate-input.jsonl"); + const output = join(root, "candidate-output.jsonl"); for (const item of cases) { - const input = join(root, "candidate-input.jsonl"); - const output = join(root, "candidate-output.jsonl"); await writeFile( input, JSON.stringify({ @@ -867,17 +867,15 @@ describe("plugin runtime preparation", () => { expect(normalized.exitCode).toBe(safe ? 0 : 2); if (safe) { const row = JSON.parse(await readFile(output, "utf8")) as { - locations: Record[]; + locations: typeof locations; }; - expect(row.locations[0]?.["path"]).toBe(item.path); - expect( - await readFile( - join(root, row.locations[0]!["path"] as string), - "utf8", - ), - ).toBe(item.contents); - locations.push(row.locations[0]!); - } else locations.push(null); + const location = row.locations[0]!; + expect(location.path).toBe(item.path); + expect(await readFile(join(root, location.path), "utf8")).toBe( + item.contents, + ); + locations.push(location); + } } const result = Bun.spawnSync([ python!, @@ -890,13 +888,11 @@ describe("plugin runtime preparation", () => { "results = []", "for location in json.loads(sys.argv[2]):", " try:", - " if location is None: raise ValueError('rejected candidate')", " finalizer['_validate_location']({'path': location['path'], 'startLine': location['start_line'], 'endLine': location['end_line'], 'role': location['role']}, 'candidate.locations[0]')", " except ValueError:", - " contract_valid = False", + " results.append(False)", " else:", - " contract_valid = True", - " results.append(contract_valid)", + " results.append(True)", "print(json.dumps(results))", ].join("\n"), join(bundledPlugin, "scripts", "finalize_scan_contract.py"), @@ -905,13 +901,7 @@ describe("plugin runtime preparation", () => { expect(result.exitCode).toBe(0); expect(JSON.parse(new TextDecoder().decode(result.stdout))).toEqual( - cases.map( - (item) => - item.path.trim().length > 0 && - !/^[A-Za-z]:/.test(item.path) && - !item.path.includes("\\") && - !/[\u0000-\u001f]/u.test(item.path), - ), + locations.map((item) => !/[\u0000-\u001f]/u.test(item.path)), ); }, ); From 2c1fbb98d11906d999527f9163e647b140ba7d22 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 15:25:24 +0000 Subject: [PATCH 07/14] fix(plugin): ignore unused symlink loops in inventory probes --- .../src/helpers/normalize-candidates.ts | 3 +-- .../tests-ts/candidate-normalizer.test.ts | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index f85ded340..32e162c0a 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -151,8 +151,7 @@ function readScope( try { relativeFile(value, root); return true; - } catch (error) { - if (error instanceof SymlinkLoopError) throw error; + } catch { return false; } }; diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts index 6923ab0dc..6f312976a 100644 --- a/sdk/typescript/tests-ts/candidate-normalizer.test.ts +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -664,6 +664,31 @@ describe("built candidate normalizer", () => { }, ); + test.skipIf(process.platform === "win32")( + "ignores unused symlink loops when disambiguating carriage-return paths", + () => { + const f = fixture(); + for (const name of ["app/plain.py", "app/literal.py\r"]) { + const sibling = name.endsWith("\r") ? name.slice(0, -1) : `${name}\r`; + write(join(f.repo, name), "one\n"); + const loop = join(f.repo, sibling); + symlinkSync(loop, loop); + write(f.scope, name.replace(/\r$/u, "") + "\r\n"); + const result = run(f, [[candidate([location(name, 1)])]]); + expect(result.status, result.stderr).toBe(0); + expect((ledger(f)[0]!["locations"] as Row[])[0]!["path"]).toBe(name); + } + const loop = join(f.repo, "app/loop.py"); + symlinkSync(loop, loop); + write(f.scope, "app/loop.py\n"); + const previous = readFileSync(f.output); + const result = run(f, [[candidate()]]); + expect(result.status).toBe(1); + expect(result.stderr).toContain("Symlink loop"); + expect(readFileSync(f.output)).toEqual(previous); + }, + ); + test.skipIf(process.platform === "win32")( "disambiguates carriage-return collisions from independent inventory evidence", () => { From c51ef3dc137ca2608c630ae53f1ef0613fa99f46 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 15:37:20 +0000 Subject: [PATCH 08/14] refactor(plugin): replace Python compatibility emulation with native behavior --- .../src/helpers/normalize-candidates.ts | 224 +++++------------- .../mcp-app/src/helpers/posix-path.ts | 91 ++----- .../src/helpers/resolve-security-md.ts | 99 ++++---- .../native/examples/windows-wide-launcher.rs | 3 +- .../tests-ts/candidate-normalizer.test.ts | 199 +++------------- sdk/typescript/tests-ts/runtime.test.ts | 52 +--- .../tests-ts/security-policy-helper.test.ts | 86 +++---- 7 files changed, 197 insertions(+), 557 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index 32e162c0a..8bcaff133 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -11,15 +11,10 @@ import { writeFileSync, } from "node:fs"; import { basename, dirname, isAbsolute, join, relative, sep } from "node:path"; -import { - decodePosixBytes, - encodePosixPath, - SymlinkLoopError, -} from "./posix-path"; +import { parseArgs } from "node:util"; +import { decodePosixBytes, encodePosixPath } from "./posix-path"; import { expandHome, - HomeExpansionError, - parsedPath, resolvedPath as resolveFilePath, windowsRelativePath, } from "./resolve-security-md"; @@ -30,11 +25,6 @@ import { windowsFileSystem, } from "../../../native/windows-files.mjs"; -const trim = (value: string) => - value.replace( - /^[\p{White_Space}\u001c-\u001f]+|[\p{White_Space}\u001c-\u001f]+$/gu, - "", - ); const roles = [ "entrypoint", "entrypoint/wrapper", @@ -74,15 +64,13 @@ function object(value: unknown): value is Row { } function stableJson(value: unknown): string { - return JSON.stringify(value, (_key, item: unknown) => { - if (typeof item === "string" && /[\ud800-\udfff]/u.test(item)) - throw new Error("UTF-8 cannot encode an unpaired surrogate"); - return object(item) + return JSON.stringify(value, (_key, item: unknown) => + object(item) ? Object.fromEntries( Object.entries(item).sort(([a], [b]) => compare(a, b)), ) - : item; - }); + : item, + ); } const windows = process.platform === "win32"; @@ -96,10 +84,7 @@ const stat = (path: string) => const pathKey = (value: string) => (windows ? value.toLowerCase() : value); function resolvedPath(value: string, strict = true): string { - const path = resolveFilePath( - fsPath(windows ? value : parsedPath(value)), - strict, - ); + const path = resolveFilePath(fsPath(value), strict); return windows ? pathText(path) : decodePosixBytes(path); } @@ -144,63 +129,19 @@ function readScope( root: string, allowMissing: boolean, ): Set { - const contents = decodeUtf8(readFile(path)); - const lines = contents.split("\n"); - const listed = new Set(lines); - const isFile = (value: string) => { - try { - relativeFile(value, root); - return true; - } catch { - return false; - } - }; - const carriage = new Map(); - if (!windows) { - for (const line of lines) { - if (line.endsWith("\r") && line !== "\r") - carriage.set(line, [isFile(line), isFile(line.slice(0, -1))]); - } - } - const crlf = - lines.includes("\r") || - [...carriage.values()].some(([literal, stripped]) => stripped && !literal); - const literalEvidence = [...carriage.values()].some( - ([literal, stripped]) => literal && !stripped, - ); + const lines = decodeUtf8(readFile(path)).split(/\r?\n/u); const scope = new Set(); - for (const [index, original] of lines.entries()) { - let line = original; - if (windows || line === "\r") { - if (line.endsWith("\r")) line = line.slice(0, -1); - } else if (line.endsWith("\r")) { - const [literal, stripped] = carriage.get(line)!; - if (stripped && !literal) line = line.slice(0, -1); - else if (stripped && literal) { - if ( - (index === lines.length - 1 && !contents.endsWith("\n")) || - listed.has(line.slice(0, -1)) - ) { - // An unterminated row or a separately listed sibling preserves CR. - } else if (crlf && !literalEvidence) line = line.slice(0, -1); - else if (!(literalEvidence && !crlf)) - throw new Error( - `in-scope file row ${index + 1}: ambiguous carriage-return paths`, - ); - } else if (!literal && crlf) line = line.slice(0, -1); - } + for (const [index, line] of lines.entries()) { if (line === "") continue; try { scope.add(relativeFile(line, root)[0]); } catch (error) { - if (error instanceof SymlinkLoopError) throw error; if (allowMissing && (error as NodeJS.ErrnoException).code === "ENOENT") { try { scope.add( inside(resolvedPath(`${root}${sep}${line}`, false), root, true), ); } catch (error) { - if (error instanceof SymlinkLoopError) throw error; throw new Error( `in-scope file row ${index + 1}: path escapes repository`, ); @@ -222,9 +163,9 @@ function textField( ): string | undefined { const value = row[field]; if ((value === undefined || value === null) && !required) return undefined; - if (typeof value !== "string" || trim(value) === "") + if (typeof value !== "string" || value.trim() === "") throw new Error(`${field}: expected a non-empty string`); - return trim(value); + return value.trim(); } function cweIds(row: Row): string[] { @@ -234,19 +175,10 @@ function cweIds(row: Row): string[] { for (const value of values) { if (typeof value !== "string") throw new Error("cwe_ids: expected CWE strings"); - const match = /^CWE-(\p{Decimal_Number}+)$/iu.exec(trim(value)); + const match = /^CWE-(\d+)$/iu.exec(value.trim()); if (match === null) throw new Error(`cwe_ids: unsupported value ${JSON.stringify(value)}`); - const digits = Array.from(match[1]!, (digit) => { - let point = digit.codePointAt(0)!; - let offset = 0; - while (/\p{Decimal_Number}/u.test(String.fromCodePoint(point - 1))) { - point--; - offset++; - } - return String(offset % 10); - }).join(""); - const number = BigInt(digits); + const number = BigInt(match[1]!); if (number < 1n) throw new Error(`cwe_ids: unsupported value ${JSON.stringify(value)}`); found.add(number); @@ -280,7 +212,7 @@ function normalizeLocations( if (unknown.length) throw new Error(`locations: unsupported fields ${unknown.join(", ")}`); const [name, source] = relativeFile(item.path, root); - if (trim(name) === "" || name.includes("\\") || name.includes(":")) + if (name.trim() === "" || name.includes("\\") || name.includes(":")) throw new Error("path: expected a safe repository-relative POSIX path"); const start = positiveLine(item.start_line, "start_line"); const end = positiveLine( @@ -374,69 +306,32 @@ function combine(groups: Map) { }); } -function argumentsFor(args: string[]): Map { - const names = [ - "input", - "out", - "repo-root", - "in-scope-files", - "allow-missing-in-scope", - "help", - ]; - function option(value: string): string | undefined { - if (value === "-h") return "help"; - if (value.startsWith("--") && value !== "--") { - const name = value.slice(2).split("=", 1)[0]!; - const matches = names.filter((item) => item.startsWith(name)); - if (matches.includes(name)) return name; - if (matches.length === 1) return matches[0]; - if (matches.length > 1) throw new Error(`ambiguous option: ${value}`); - } - if ( - !value.startsWith("-") || - value === "-" || - (!value.startsWith("-h") && - (value.includes(" ") || - /^-(?:\p{Decimal_Number}+|\p{Decimal_Number}*\.\p{Decimal_Number}+)\n?$/u.test( - value, - ))) - ) - return undefined; - throw new Error(`unrecognized argument: ${value}`); - } - const values = new Map(); - for (let index = 0; index < args.length; index++) { - const argument = args[index]!; - const name = option(argument); - if (name === undefined) - throw new Error(`unrecognized argument: ${argument}`); - const equals = argument.indexOf("="); - if (name === "help" || name === "allow-missing-in-scope") { - if (equals !== -1) - throw new Error(`argument --${name} does not take a value`); - values.set(name, []); - if (name === "help") return values; - continue; - } - const found: string[] = []; - if (equals !== -1) found.push(argument.slice(equals + 1)); - else { - while ( - index + 1 < args.length && - option(args[index + 1]!) === undefined - ) { - found.push(args[++index]!); - if (name !== "input") break; - } +function argumentsFor(args: string[]) { + const { values, tokens } = parseArgs({ + args, + allowPositionals: true, + tokens: true, + options: { + input: { type: "string", multiple: true }, + out: { type: "string" }, + "repo-root": { type: "string" }, + "in-scope-files": { type: "string" }, + "allow-missing-in-scope": { type: "boolean" }, + help: { type: "boolean", short: "h" }, + }, + }); + let collectingInputs = false; + for (const token of tokens) { + if (token.kind === "option") collectingInputs = token.name === "input"; + else if (token.kind === "positional") { + if (!collectingInputs) + throw new Error(`unrecognized argument: ${token.value}`); + values.input!.push(token.value); } - if (found.length === 0) - throw new Error( - `argument --${name}: expected ${name === "input" ? "at least one argument" : "one argument"}`, - ); - values.set(name, found); } - for (const name of ["input", "out", "repo-root", "in-scope-files"]) { - if (!values.has(name)) throw new Error(`--${name} is required`); + if (!values.help) { + for (const name of ["input", "out", "repo-root", "in-scope-files"] as const) + if (values[name] === undefined) throw new Error(`--${name} is required`); } return values; } @@ -447,7 +342,7 @@ export function normalizeCandidatesCommand( ): number { try { const values = argumentsFor(args); - if (values.has("help")) { + if (values.help) { console.log( "Validate and combine security-scan candidates into deterministic JSONL.\n", ); @@ -456,19 +351,20 @@ export function normalizeCandidatesCommand( ); return 0; } - const paths = (name: string, strict = true) => - values - .get(name)! - .map((value) => - resolvedPath(expandHome(parsedPath(value), posixHome), strict), - ); - const root = paths("repo-root")[0]!; + const resolve = (value: string, strict = true) => + resolvedPath(expandHome(value, posixHome), strict); + const root = resolve(values["repo-root"]!); if (!stat(root).isDirectory()) throw new Error("--repo-root: expected a directory"); - const output = paths("out", false)[0]!; - const scopePath = paths("in-scope-files")[0]!; + const output = resolve(values.out!, false); + const scopePath = resolve(values["in-scope-files"]!); const inputs = [ - ...new Map(paths("input").map((path) => [pathKey(path), path])).values(), + ...new Map( + values.input!.map((value) => { + const path = resolve(value); + return [pathKey(path), path]; + }), + ).values(), ].sort((a, b) => compare(pathKey(a), pathKey(b))); if (inputs.some((path) => pathKey(path) === pathKey(output))) throw new Error("--out: must not also be an input"); @@ -477,22 +373,21 @@ export function normalizeCandidatesCommand( const scope = readScope( scopePath, root, - values.has("allow-missing-in-scope"), + values["allow-missing-in-scope"] ?? false, ); const lineCounts = new Map(); const groups = new Map(); let rowCount = 0; for (const source of inputs) { - const lines = decodeUtf8(readFile(source)).split(/\r\n|[\r\n]/u); + const lines = decodeUtf8(readFile(source)).split(/\r?\n/u); for (const [index, line] of lines.entries()) { - if (trim(line) === "") continue; + if (line.trim() === "") continue; let candidate: Candidate; try { const row: unknown = JSON.parse(line); if (!object(row)) throw new Error("expected a JSON object"); candidate = normalizeCandidate(row, root, scope, lineCounts); } catch (error) { - if (error instanceof SymlinkLoopError) throw error; throw new Error( `${source} row ${index + 1}: ${(error as Error).message}`, ); @@ -520,8 +415,7 @@ export function normalizeCandidatesCommand( let created = false; function* contents() { created = true; - for (const row of combined) - yield Buffer.from(`${stableJson(row)}${windows ? "\r\n" : "\n"}`); + for (const row of combined) yield Buffer.from(`${stableJson(row)}\n`); } try { if (windows) { @@ -546,16 +440,12 @@ export function normalizeCandidatesCommand( if ((error as NodeJS.ErrnoException).code !== "ENOENT") throw error; } } - const message = `Combined ${rowCount} candidate rows into ${combined.length} rows in ${output}\n`; - process.stdout.write( - windows ? message.replace(/\n/gu, "\r\n") : encodePosixPath(message), + console.log( + `Combined ${rowCount} candidate rows into ${combined.length} rows in ${output}`, ); return 0; } catch (error) { console.error(`normalize_candidates: ${(error as Error).message}`); - return error instanceof SymlinkLoopError || - error instanceof HomeExpansionError - ? 1 - : 2; + return 2; } } diff --git a/plugins/codex-security/mcp-app/src/helpers/posix-path.ts b/plugins/codex-security/mcp-app/src/helpers/posix-path.ts index 76bdf5ce9..6eb6d0d4f 100644 --- a/plugins/codex-security/mcp-app/src/helpers/posix-path.ts +++ b/plugins/codex-security/mcp-app/src/helpers/posix-path.ts @@ -1,11 +1,11 @@ import { isUtf8 } from "node:buffer"; -import { lstatSync, readlinkSync, realpathSync, statSync } from "node:fs"; +import { lstatSync, realpathSync } from "node:fs"; import { posix } from "node:path"; export function decodePosixBytes(bytes: Buffer): string { // Node 20's fatal TextDecoder can replace invalid bytes in longer inputs. if (isUtf8(bytes)) return bytes.toString("utf8"); - // Match Python's surrogateescape for undecodable POSIX path bytes. + // Preserve undecodable POSIX bytes as lone low surrogates. let value = ""; for (let offset = 0; offset < bytes.length;) { let decoded = false; @@ -38,76 +38,29 @@ export function encodePosixPath(value: string): Buffer { ); } -export class SymlinkLoopError extends Error {} - export function resolvePosixPath(value: Buffer, strict = true): Buffer { - // GNU Linux native realpath rejects file/.. and links targeting it with - // ENOTDIR. Retain the shipped pathlib contract for those inputs. - const seen = new Map(); - // Latin-1 is a lossless internal representation of pathname bytes. - const append = (directory: string, rest: string) => - rest.startsWith("/") ? rest : `${directory}/${rest}`; - function follow(directory: string, path: string): [string, boolean] { - if (path.startsWith("/")) directory = "/"; - const parts = path.split("/"); - for (const [index, name] of parts.entries()) { - if (name === "" || name === ".") continue; - if (name === "..") { - directory = directory.slice(0, directory.lastIndexOf("/")) || "/"; - continue; - } - const candidate = `${directory === "/" ? "" : directory}/${name}`; - const bytes = Buffer.from(candidate, "latin1"); - let link: boolean; - try { - link = lstatSync(bytes).isSymbolicLink(); - } catch (error) { - if (strict) throw error; - link = false; - } - if (!link) { - directory = candidate; - continue; - } - const cached = seen.get(candidate); - if (cached === null) { - if (!strict) - return [append(candidate, parts.slice(index + 1).join("/")), false]; - throw new SymlinkLoopError( - `Symlink loop from ${decodePosixBytes(bytes)}`, - ); - } - if (cached !== undefined) { - directory = cached; - continue; - } - seen.set(candidate, null); - const [resolved, complete] = follow( - directory, - readlinkSync(bytes, { encoding: "buffer" }).toString("latin1"), - ); - if (!complete) - return [append(resolved, parts.slice(index + 1).join("/")), false]; - directory = resolved; - seen.set(candidate, directory); - } - return [directory, true]; - } - const cwd = - value[0] === 0x2f - ? Buffer.from("/") - : realpathSync.native(".", { encoding: "buffer" }); - const [path] = follow(cwd.toString("latin1"), value.toString("latin1")); - const result = Buffer.from(posix.resolve(path), "latin1"); - if (!strict) { + const missing: string[] = []; + let current = value; + while (true) { try { - statSync(result); + const resolved = realpathSync.native(current, { encoding: "buffer" }); + // Latin-1 preserves raw path bytes while joining missing components. + return Buffer.from( + posix.join(resolved.toString("latin1"), ...missing.reverse()), + "latin1", + ); } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ELOOP") - throw new SymlinkLoopError( - `Symlink loop from ${decodePosixBytes(result)}`, - ); + if ( + strict || + (error as NodeJS.ErrnoException).code !== "ENOENT" || + lstatSync(current, { throwIfNoEntry: false }) !== undefined + ) + throw error; + // Existing dangling links are rejected above; only absent components + // may be appended to a canonical existing ancestor. + const path = current.toString("latin1"); + missing.push(posix.basename(path)); + current = Buffer.from(posix.dirname(path), "latin1"); } } - return result; } diff --git a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts index 6dcbd552d..0d71478a0 100644 --- a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts +++ b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts @@ -11,23 +11,17 @@ import { type Stats, } from "node:fs"; import { homedir } from "node:os"; -import { basename, dirname, parse, sep } from "node:path"; +import { basename, dirname, sep } from "node:path"; import { parseArgs } from "node:util"; import { unixBinding, windowsBinding } from "../native"; -import { - windowsFileSystem, - windowsJoin, - windowsParts, -} from "../../../native/windows-files.mjs"; +import { windowsFileSystem } from "../../../native/windows-files.mjs"; import { decodePosixBytes, encodePosixPath, - SymlinkLoopError, resolvePosixPath, } from "./posix-path"; const MAX_SECURITY_MD_BYTES = 1024 * 1024; -export class HomeExpansionError extends Error {} const windows = process.platform === "win32"; const windowsFiles = () => windowsFileSystem(windowsBinding()); const encodePath = (path: string) => @@ -40,35 +34,59 @@ type FileInfo = Pick & { const statPath = (path: Buffer): FileInfo => windows ? windowsFiles().stat(path) : statSync(path); +function windowsParts(value: string): [string, string, string] { + const path = value.replaceAll("/", "\\"); + if (path.startsWith("\\\\")) { + const start = path.slice(0, 8).toUpperCase() === "\\\\?\\UNC\\" ? 8 : 2; + const server = path.indexOf("\\", start); + const share = server === -1 ? -1 : path.indexOf("\\", server + 1); + return share === -1 + ? [value, "", ""] + : [value.slice(0, share), value[share]!, value.slice(share + 1)]; + } + const drive = path[1] === ":" ? 2 : 0; + const root = path[drive] === "\\" ? 1 : 0; + return [ + value.slice(0, drive), + value.slice(drive, drive + root), + value.slice(drive + root), + ]; +} + +function windowsJoin(left: string, right: string): string { + const [leftDrive, leftRoot, leftPath] = windowsParts(left); + const [rightDrive, rightRoot, rightPath] = windowsParts(right); + if (rightRoot) return (rightDrive || leftDrive) + rightRoot + rightPath; + if (rightDrive && rightDrive.toLowerCase() !== leftDrive.toLowerCase()) + return right; + const drive = rightDrive || leftDrive; + const path = + leftPath + (leftPath && !/[/\\]$/u.test(leftPath) ? "\\" : "") + rightPath; + const root = + leftRoot || (path && drive && !/[:/\\]$/u.test(drive) ? "\\" : ""); + return drive + root + path; +} + export function parsedPath(value: string): string { - // pathlib removes empty and '.' components while preserving symlink/.. pairs. - let root = windows - ? windowsParts(value).slice(0, 2).join("").replaceAll("/", "\\") - : value.startsWith("//") && !value.startsWith("///") - ? "//" - : parse(value).root; - if (windows && root.startsWith("\\\\") && !root.endsWith("\\")) { + if (!windows) return value || "."; + let root = windowsParts(value).slice(0, 2).join("").replaceAll("/", "\\"); + if (root.startsWith("\\\\") && !root.endsWith("\\")) { const parts = root.split("\\"); if ((parts.length === 4 && !"?.".includes(parts[2]!)) || parts.length === 6) root += "\\"; } const parts = value .slice(root.length) - .split(process.platform === "win32" ? /[/\\]/u : /\//u) + .split(/[/\\]/u) .filter((part) => part !== "" && part !== "."); - if (windows && !root && windowsParts(parts[0] ?? "")[0]) parts.unshift("."); + if (!root && windowsParts(parts[0] ?? "")[0]) parts.unshift("."); return root + parts.join(sep) || "."; } export function resolvedPath(path: Buffer, strict = true): Buffer { - if (process.platform !== "win32") return resolvePosixPath(path, strict); - try { - return windowsFiles().realpath(path, strict); - } catch (error) { - if ((error as NodeJS.ErrnoException).code === "ELOOP") - throw new SymlinkLoopError(`Symlink loop from ${decodePath(path)}`); - throw error; - } + return windows + ? windowsFiles().realpath(path, strict) + : resolvePosixPath(path, strict); } export function expandHome( @@ -91,35 +109,35 @@ export function expandHome( home = windowsJoin(environment("HOMEDRIVE") ?? "", homePath); } if (home === undefined) - throw new HomeExpansionError("Could not determine home directory."); + throw new Error("Could not determine home directory."); if (username !== "" && username !== currentUsername) { const [drive, root, tail] = windowsParts(home); const separator = Math.max(tail.lastIndexOf("/"), tail.lastIndexOf("\\")); if (currentUsername !== tail.slice(separator + 1)) { - throw new HomeExpansionError("Could not determine home directory."); + throw new Error("Could not determine home directory."); } const parent = drive + root + tail.slice(0, separator + 1).replace(/[/\\]+$/u, ""); home = windowsJoin(parent, username); } if (home.startsWith("~")) - throw new HomeExpansionError("Could not determine home directory."); + throw new Error("Could not determine home directory."); return windowsJoin(home, separator === -1 ? "" : path.slice(end + 1)); } if (path === "~" || path.startsWith("~/")) { const home = posixHome ?? homedir(); if (home.startsWith("~")) - throw new HomeExpansionError("Could not determine home directory."); + throw new Error("Could not determine home directory."); return home + path.slice(1) || "/"; } const separator = path.indexOf("/"); const end = separator === -1 ? path.length : separator; const result = unixBinding().userHome(encodePosixPath(path.slice(1, end))); if (result.value === null) - throw new HomeExpansionError("Could not determine home directory."); + throw new Error("Could not determine home directory."); const home = decodePosixBytes(result.value).replace(/\/+$/u, ""); if (home.startsWith("~")) - throw new HomeExpansionError("Could not determine home directory."); + throw new Error("Could not determine home directory."); return home + path.slice(end) || "/"; } @@ -184,15 +202,11 @@ function inside(path: Buffer, root: Buffer, label: string): Buffer { } function resolveRoot(repo: string, posixHome: string | undefined): Buffer { + const expanded = encodePath(parsedPath(expandHome(repo, posixHome))); let root: Buffer; try { - root = resolvedPath(encodePath(parsedPath(expandHome(repo, posixHome)))); - } catch (error) { - if ( - error instanceof SymlinkLoopError || - error instanceof HomeExpansionError - ) - throw error; + root = resolvedPath(expanded); + } catch { throw new Error(`scan root does not exist: ${repo}`); } if (!statPath(root).isDirectory()) { @@ -331,10 +345,8 @@ function resolveSecurityMd( : appendPath(root, encodePosixPath(expandedScope)); let resolvedScope: Buffer; try { - // Resolve links before '..', including Python's accepted file/.. paths. resolvedScope = resolvedPath(requestedScope); - } catch (error) { - if (error instanceof SymlinkLoopError) throw error; + } catch { throw new Error(`scan scope does not exist: ${decodePath(requestedScope)}`); } inside(resolvedScope, root, "scan scope"); @@ -471,10 +483,7 @@ export function resolveSecurityMdCommand( } } catch (error) { console.error(`resolve-security-md: error: ${(error as Error).message}`); - return error instanceof SymlinkLoopError || - error instanceof HomeExpansionError - ? 1 - : 2; + return 2; } return 0; } diff --git a/plugins/codex-security/native/examples/windows-wide-launcher.rs b/plugins/codex-security/native/examples/windows-wide-launcher.rs index 118de35db..a00a9447e 100644 --- a/plugins/codex-security/native/examples/windows-wide-launcher.rs +++ b/plugins/codex-security/native/examples/windows-wide-launcher.rs @@ -287,7 +287,7 @@ fn main() -> std::io::Result<()> { r#""evidence":"source evidence","locations":[{"end_line":1,"#, r#""path":"source.py","role":"entrypoint","start_line":1}],"#, r#""summary":"wide paths"}"#, - "\r\n", + "\n", ); let candidate = |repo_arg: &Path, input: &Path, scope: &Path, output: &Path| { Command::new(&node) @@ -329,7 +329,6 @@ fn main() -> std::io::Result<()> { String::from_utf8_lossy(&child.stderr) ))); } - fs::remove_file(&output)?; } fs::create_dir(repo.join("blocked-output"))?; let child = candidate( diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts index 6f312976a..5afda082c 100644 --- a/sdk/typescript/tests-ts/candidate-normalizer.test.ts +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -182,7 +182,7 @@ describe("built candidate normalizer", () => { { ...process.env, PATH: "" }, ); expect(result.status, result.stderr).toBe(0); - expect(result.stdout.replaceAll("\r\n", "\n")).toBe( + expect(result.stdout).toBe( `Combined 3 candidate rows into 2 rows in ${f.output}\n`, ); const rows = ledger(f); @@ -268,7 +268,7 @@ describe("built candidate normalizer", () => { expect(rejected.stderr).toContain("expected at least one in-scope file"); }); - test("sorts Unicode code points, normalizes Unicode and large CWE integers, and retains BOM text", () => { + test("sorts Unicode code points, normalizes CWE IDs, and trims text", () => { const f = fixture(); const names = ["app/\u{10000}.py", "app/\ue000.py"]; for (const name of names) write(join(f.repo, name), "line\n"); @@ -276,8 +276,8 @@ describe("built candidate normalizer", () => { const row = candidate( names.map((name) => location(name, 1, "evidence")), { - cwe_ids: ["CWE-٢", "cwe-００８９", "CWE-89", "CWE-9007199254740993"], - summary: "\u001c\ufeffSummary\u0085", + cwe_ids: ["CWE-002", "cwe-0089", "CWE-89", "CWE-9007199254740993"], + summary: " \ufeffSummary\u00a0", evidence: "\u{10000}", }, ); @@ -292,7 +292,7 @@ describe("built candidate normalizer", () => { expect((value["locations"] as Row[]).map((item) => item["path"])).toEqual( [...names].reverse(), ); - expect(value["summary"]).toBe("\ufeffSummary"); + expect(value["summary"]).toBe("Summary"); expect(value["evidence"]).toBe("\ue000\n\u{10000}"); expect(value["candidate_id"]).toBe("candidate-cc5ebd3ebd732a50"); expect(readFileSync(f.output, "utf8").startsWith('{"candidate_id":')).toBe( @@ -322,24 +322,6 @@ describe("built candidate normalizer", () => { } }); - test.each(["1.0", "1e0"])( - "normalizes integral line spelling %s", - (spelling) => { - const f = fixture(); - const input = join(f.root, "raw.jsonl"); - const row = JSON.stringify(candidate([location("app/routes.py", 1)])); - write(input, row + "\n"); - expect(invoke(f, [input]).status).toBe(0); - const expected = readFileSync(f.output); - write( - input, - row.replace('"start_line":1', `"start_line":${spelling}`) + "\n", - ); - expect(invoke(f, [input]).status).toBe(0); - expect(readFileSync(f.output)).toEqual(expected); - }, - ); - test("rejects invalid line values, malformed rows, and invalid UTF-8 atomically", () => { const f = fixture(); const input = join(f.root, "raw.jsonl"); @@ -385,16 +367,13 @@ describe("built candidate normalizer", () => { '{"locations":', "\ufeff{}", Buffer.from([0xff]), - ...["summary", "evidence"].map((field) => - JSON.stringify(candidate(undefined, { [field]: "\ud800" })), - ), ]) { write(input, invalid); const result = invoke(f, [input]); expect(result.status).toBe(2); expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); } - write(input, JSON.stringify(candidate()) + "\r\n\rnot-json\n"); + write(input, JSON.stringify(candidate()) + "\r\n\nnot-json\n"); expect(invoke(f, [input]).stderr).toContain("raw.jsonl row 3:"); expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); expect(readdirSync(f.root).some((name) => name.endsWith(".tmp"))).toBe( @@ -459,7 +438,7 @@ describe("built candidate normalizer", () => { "candidate_id: expected a non-empty string", ], [ - candidate(undefined, { summary: "\u001c" }), + candidate(undefined, { summary: " \t\r\n" }), "summary: expected a non-empty string", ], [ @@ -513,47 +492,17 @@ describe("built candidate normalizer", () => { }); test.skipIf(process.platform === "win32")( - "preserves scope through repeated separators after an unresolved symlink", - () => { - const f = fixture(); - symlinkSync("self", join(f.repo, "self")); - symlinkSync("missing/../self", join(f.repo, "alias")); - write(f.scope, `alias/${f.repo}/app/routes.py\n`); - const result = run(f, [[candidate()]], ["--allow-missing-in-scope"]); - expect(result.status, result.stderr).toBe(1); - expect(existsSync(f.output)).toBe(false); - }, - ); - - test.skipIf(process.platform === "win32")( - "normalizes remaining output components after a non-strict symlink cycle", + "rejects scope symlink loops without replacing existing output", () => { const f = fixture(); - symlinkSync("loop", join(f.root, "loop")); - const target = join(f.root, "target.jsonl"); - write(target, "target must remain unchanged\n"); - const output = join(f.root, "from-loop.jsonl"); - symlinkSync(target, output); - const result = run( - { ...f, output: `${f.root}/loop/../from-loop.jsonl` }, - [[candidate()]], - ); - expect(result.status, result.stderr).toBe(0); - expect(readFileSync(target, "utf8")).toBe( - "target must remain unchanged\n", - ); - expect(ledger({ ...f, output })).toHaveLength(1); - const alias = join(f.root, "nested-output-alias"); - const nestedTarget = join(f.root, "nested-target.jsonl"); - symlinkSync("loop/../nested-target.jsonl", alias); - const nested = run({ ...f, output: alias }, [[candidate()]]); - expect(nested.status, nested.stderr).toBe(0); - expect(ledger({ ...f, output: nestedTarget })).toHaveLength(1); - expect( - run({ ...f, output: `${f.root}/loop/still-looped.jsonl` }, [ - [candidate()], - ]).status, - ).toBe(1); + const loop = join(f.repo, "app/loop.py"); + symlinkSync(loop, loop); + write(f.scope, "app/loop.py\n"); + write(f.output, "previous output\n"); + const result = run(f, [[candidate()]]); + expect(result.status).toBe(2); + expect(result.stderr).toContain("in-scope file row 1:"); + expect(readFileSync(f.output, "utf8")).toBe("previous output\n"); }, ); @@ -576,6 +525,11 @@ describe("built candidate normalizer", () => { expect(invoke({ ...f, output: alias }, [source]).stderr).toContain( "--out: must not also be an input", ); + const missing = join(f.root, "missing.jsonl"); + const dangling = join(f.root, "dangling-output"); + symlinkSync(missing, dangling); + expect(run({ ...f, output: dangling }, [[candidate()]]).status).toBe(2); + expect(existsSync(missing)).toBe(false); } const nested = { ...f, @@ -638,15 +592,8 @@ describe("built candidate normalizer", () => { for (const name of [ "app/ leading.py", "app/trailing .py", - "app/ .py", - "app/ .py", - "app/carriage\rname.py", - "app/trailing.py\r", - "app/vertical\vname.py", - "app/form\fname.py", - "app/next\u0085name.py", - "app/line\u2028name.py", - "app/paragraph\u2029name.py", + "app/résumé.py", + "app/路径.py", ]) { write(join(f.repo, name), "line\n"); write(f.scope, name + "\n"); @@ -664,90 +611,6 @@ describe("built candidate normalizer", () => { }, ); - test.skipIf(process.platform === "win32")( - "ignores unused symlink loops when disambiguating carriage-return paths", - () => { - const f = fixture(); - for (const name of ["app/plain.py", "app/literal.py\r"]) { - const sibling = name.endsWith("\r") ? name.slice(0, -1) : `${name}\r`; - write(join(f.repo, name), "one\n"); - const loop = join(f.repo, sibling); - symlinkSync(loop, loop); - write(f.scope, name.replace(/\r$/u, "") + "\r\n"); - const result = run(f, [[candidate([location(name, 1)])]]); - expect(result.status, result.stderr).toBe(0); - expect((ledger(f)[0]!["locations"] as Row[])[0]!["path"]).toBe(name); - } - const loop = join(f.repo, "app/loop.py"); - symlinkSync(loop, loop); - write(f.scope, "app/loop.py\n"); - const previous = readFileSync(f.output); - const result = run(f, [[candidate()]]); - expect(result.status).toBe(1); - expect(result.stderr).toContain("Symlink loop"); - expect(readFileSync(f.output)).toEqual(previous); - }, - ); - - test.skipIf(process.platform === "win32")( - "disambiguates carriage-return collisions from independent inventory evidence", - () => { - const cases = [ - { - inventory: "app/routes.py\r\napp/query.py\r\n", - files: ["app/routes.py\r"], - selected: "app/routes.py", - }, - { - inventory: - "app/query.py\napp/query.py\r\napp/routes.py\napp/routes.py\r\n", - files: ["app/routes.py\r", "app/query.py\r"], - selected: "app/routes.py\r", - }, - { - inventory: "app/query.py\r\napp/routes.py\r", - files: ["app/routes.py\r"], - selected: "app/routes.py\r", - }, - { - inventory: "app/routes.py\r\napp/literal.py\r\napp/query.py\n", - files: ["app/routes.py\r", "app/literal.py\r"], - selected: "app/routes.py\r", - }, - { - inventory: "\r\napp/routes.py\r\napp/query.py\r\n", - files: ["app/routes.py\r", "app/query.py\r"], - selected: "app/routes.py", - }, - ]; - for (const item of cases) { - const f = fixture(); - for (const name of item.files) write(join(f.repo, name), "line\n"); - write(f.scope, item.inventory); - const result = run(f, [[candidate([location(item.selected, 1)])]]); - expect(result.status, result.stderr).toBe(0); - expect((ledger(f)[0]!["locations"] as Row[])[0]!["path"]).toBe( - item.selected, - ); - } - for (const inventory of [ - "app/routes.py\r\napp/query.py\n", - "app/routes.py\r\napp/query.py\r\n", - ]) { - const f = fixture(); - for (const name of ["app/routes.py\r", "app/query.py\r"]) - write(join(f.repo, name), "line\n"); - write(f.scope, inventory); - for (const selected of ["app/routes.py", "app/routes.py\r"]) { - const result = run(f, [[candidate([location(selected, 1)])]]); - expect(result.status).toBe(2); - expect(result.stderr).toContain("ambiguous carriage-return paths"); - expect(existsSync(f.output)).toBe(false); - } - } - }, - ); - test.skipIf(process.platform !== "win32")( "normalizes Windows separators and rejects drive-qualified locations", () => { @@ -870,7 +733,7 @@ describe("built candidate normalizer", () => { }, ); - test("keeps argument aliases, multiple inputs, home expansion, and literal dash output", () => { + test("accepts documented options, multiple inputs, home expansion, and literal dash output", () => { const f = fixture(); run(f, [[candidate()]]); const input = join(f.root, "candidates-0.jsonl"); @@ -879,15 +742,13 @@ describe("built candidate normalizer", () => { [ helper, "normalize-candidates", - "--inp", + "--input", input, input, - "--o", - "-", - "--repo-r", - "./~/İrepository", - "--in-s", - "~/in-scope.txt", + "--out=-", + "--repo-root", + "~/İrepository", + "--in-scope-files=~/in-scope.txt", ], { cwd: f.root, @@ -907,7 +768,7 @@ describe("built candidate normalizer", () => { for (const args of [ [], ["--input"], - ["--in", input], + ["--unknown", input], ["--allow-missing-in-scope=true"], ]) expect( diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index 53d710c6f..8186bc4a9 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -782,58 +782,8 @@ describe("plugin runtime preparation", () => { const python = Bun.which("python3") ?? Bun.which("python"); expect(python).not.toBeNull(); - const sourcePlugin = await bundledPluginRoot(); - const projector = new URL( - "../scripts/project-plugin.mjs", - import.meta.url, - ); - const publicManifest = new URL( - "../public-repo/sdk/typescript/plugin.public.json", - import.meta.url, - ); - let bundledPlugin = sourcePlugin; - if (existsSync(projector) && existsSync(publicManifest)) { - const packageRoot = join(root, "package"); - const isolatedProjector = join( - packageRoot, - "scripts", - "project-plugin.mjs", - ); - const isolatedManifest = join( - packageRoot, - "public-repo", - "sdk", - "typescript", - "plugin.public.json", - ); - await Promise.all([ - mkdir(dirname(isolatedProjector), { recursive: true }), - mkdir(dirname(isolatedManifest), { recursive: true }), - ]); - await Promise.all([ - copyFile(projector, isolatedProjector), - copyFile(publicManifest, isolatedManifest), - ]); - const projection = Bun.spawnSync( - [process.execPath, isolatedProjector], - { - cwd: packageRoot, - env: { - ...process.env, - CODEX_SECURITY_PLUGIN_ROOT: sourcePlugin, - }, - stdout: "pipe", - stderr: "pipe", - }, - ); - expect(new TextDecoder().decode(projection.stderr)).toBe(""); - expect(projection.exitCode).toBe(0); - bundledPlugin = join(packageRoot, "_bundled_plugin"); - } + const bundledPlugin = await bundledPluginRoot(); const normalizer = join(bundledPlugin, "mcp", "helpers.mjs"); - expect(await readFile(normalizer, "utf8")).toBe( - await readFile(join(sourcePlugin, "mcp", "helpers.mjs"), "utf8"), - ); const locations: { path: string }[] = []; const input = join(root, "candidate-input.jsonl"); const output = join(root, "candidate-output.jsonl"); diff --git a/sdk/typescript/tests-ts/security-policy-helper.test.ts b/sdk/typescript/tests-ts/security-policy-helper.test.ts index 74d2b78e0..b6d1e642f 100644 --- a/sdk/typescript/tests-ts/security-policy-helper.test.ts +++ b/sdk/typescript/tests-ts/security-policy-helper.test.ts @@ -174,9 +174,13 @@ describe("built SECURITY.md helper", () => { expect(result.status, result.stderr).toBe(0); expect(result.stdout).toContain("home-variable policy"); } - expect(run(["--repo", root, "--scope", "~"], homeEnv({})).status).toBe(1); + expect( + run(["--repo", root, "--scope", "~"], homeEnv({})).status, + ).not.toBe(0); const other = homeEnv({ USERPROFILE: `${home}\\`, USERNAME: "current" }); - expect(run(["--repo", "~other", "--scope", "."], other).status).toBe(1); + expect(run(["--repo", "~other", "--scope", "."], other).status).not.toBe( + 0, + ); }, ); @@ -509,63 +513,37 @@ describe("built SECURITY.md helper", () => { expectGuidance(result.stdout, [["SECURITY.md", "\ufeff"]]); }); - test("preserves path parsing for file scopes and output destinations", () => { - const { root, output } = fixture(); - write(root, "src/SECURITY.md", "source policy\n"); - write(root, "src/app.ts", "export {};\n"); - const expected: [string, string][] = [["src/SECURITY.md", "source policy"]]; - for (const scope of ["src/app.ts/", "./src//app.ts/./"]) { - const result = resolve(`${root}/./`, scope, "./-/"); - expect(result.status, result.stderr).toBe(0); - expectGuidance(result.stdout, expected); - } - const destination = `${output}/guidance.md/./`; - const result = resolve(root, "src/app.ts", destination); - expect(result.status, result.stderr).toBe(0); - expect(result.stdout).toBe(""); - expectGuidance(readFileSync(join(output, "guidance.md"), "utf8"), expected); - }); - - test("resolves parent components after existing files and symbolic links", () => { - const { root } = fixture(); - write(root, "nested/SECURITY.md", "nested policy\n"); - write(root, "nested/file.ts", "export {};\n"); - const expected: [string, string][] = [ - ["nested/SECURITY.md", "nested policy"], - ]; - for (const scope of ["nested/file.ts/..", "nested/SECURITY.md/../."]) { - const result = resolve(root, scope); - expect(result.status, result.stderr).toBe(0); - expectGuidance(result.stdout, expected); - } - const result = resolve(`${root}/nested/file.ts/..`, "."); - expect(result.status, result.stderr).toBe(0); - expectGuidance(result.stdout, [["SECURITY.md", "nested policy"]]); - symlinkSync("nested/file.ts/..", join(root, "alias"), "dir"); - const linked = resolve(root, "alias"); - expect(linked.status, linked.stderr).toBe(0); - expectGuidance(linked.stdout, expected); - const missing = resolve(root, "missing/../nested"); - expect(missing.status, missing.stderr).toBe( - process.platform === "win32" ? 0 : 2, - ); - if (process.platform === "win32") expectGuidance(missing.stdout, expected); - else expect(missing.stdout).toBe(""); - }); - test.skipIf(process.platform === "win32")( - "returns the existing failure status for scope link cycles", + "resolves parent components after directory symlinks", () => { - const { root } = fixture(); - symlinkSync("second", join(root, "first"), "dir"); - symlinkSync("first", join(root, "second"), "dir"); - const result = resolve(root, "first"); - expect(result.status).toBe(1); - expect(result.stdout).toBe(""); - expect(result.stderr).toContain("Symlink loop"); + const { root, output } = fixture(); + write(root, "nested/SECURITY.md", "nested policy\n"); + mkdirSync(join(root, "nested", "child")); + symlinkSync("nested/child", join(root, "alias"), "dir"); + const result = resolve(root, "alias/.."); + expect(result.status, result.stderr).toBe(0); + expectGuidance(result.stdout, [["nested/SECURITY.md", "nested policy"]]); + + write(output, "SECURITY.md", "outside policy\n"); + mkdirSync(join(output, "child")); + symlinkSync(join(output, "child"), join(root, "outside"), "dir"); + const outside = resolve(root, "outside/.."); + expect(outside.status).not.toBe(0); + expect(outside.stdout).toBe(""); + expect(outside.stderr).toContain("outside the scan root"); }, ); + test.skipIf(process.platform === "win32")("rejects scope link cycles", () => { + const { root } = fixture(); + symlinkSync("second", join(root, "first"), "dir"); + symlinkSync("first", join(root, "second"), "dir"); + const result = resolve(root, "first"); + expect(result.status).not.toBe(0); + expect(result.stdout).toBe(""); + expect(result.stderr).toContain("scan scope does not exist"); + }); + test("creates output directories and writes empty guidance when no policy exists", () => { const { root, output } = fixture(); const destination = join(output, "artifacts", "guidance.md"); From f0d931194dbc50e41ded8e23acd9c4b4ee889013 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 15:46:02 +0000 Subject: [PATCH 09/14] test(plugin): run bundled normalizer with Node --- sdk/typescript/tests-ts/runtime.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/sdk/typescript/tests-ts/runtime.test.ts b/sdk/typescript/tests-ts/runtime.test.ts index 8186bc4a9..5506d429a 100644 --- a/sdk/typescript/tests-ts/runtime.test.ts +++ b/sdk/typescript/tests-ts/runtime.test.ts @@ -784,6 +784,8 @@ describe("plugin runtime preparation", () => { expect(python).not.toBeNull(); const bundledPlugin = await bundledPluginRoot(); const normalizer = join(bundledPlugin, "mcp", "helpers.mjs"); + const node = Bun.which("node"); + expect(node).not.toBeNull(); const locations: { path: string }[] = []; const input = join(root, "candidate-input.jsonl"); const output = join(root, "candidate-output.jsonl"); @@ -798,7 +800,7 @@ describe("plugin runtime preparation", () => { }) + "\n", ); const normalized = Bun.spawnSync([ - process.execPath, + node!, normalizer, "normalize-candidates", "--input", From d8a8a76fdf63595f41b6bdfbc541846172f7f4bd Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 15:49:00 +0000 Subject: [PATCH 10/14] fix(plugin): normalize relative candidate paths before native lookup --- .../mcp-app/src/helpers/normalize-candidates.ts | 4 ++-- .../codex-security/native/examples/windows-wide-launcher.rs | 4 ++-- sdk/typescript/tests-ts/candidate-normalizer.test.ts | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index 8bcaff133..d562bdde3 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -118,7 +118,7 @@ function relativeFile(value: unknown, root: string): [string, string] { throw new Error( "path: expected a repository-relative path without traversal", ); - const path = resolvedPath(`${root}${sep}${raw}`); + const path = resolvedPath(join(root, raw)); const name = inside(path, root); if (!stat(path).isFile()) throw new Error("path: expected a regular file"); return [name, path]; @@ -139,7 +139,7 @@ function readScope( if (allowMissing && (error as NodeJS.ErrnoException).code === "ENOENT") { try { scope.add( - inside(resolvedPath(`${root}${sep}${line}`, false), root, true), + inside(resolvedPath(join(root, line), false), root, true), ); } catch (error) { throw new Error( diff --git a/plugins/codex-security/native/examples/windows-wide-launcher.rs b/plugins/codex-security/native/examples/windows-wide-launcher.rs index ebe3b6c96..d45c818e7 100644 --- a/plugins/codex-security/native/examples/windows-wide-launcher.rs +++ b/plugins/codex-security/native/examples/windows-wide-launcher.rs @@ -255,7 +255,7 @@ fn main() -> std::io::Result<()> { let input_name = raw("input-", 0xd800); let scope_name = raw("scope-files-", 0xdc80); fs::write(repo.join("source.py"), "source line\n")?; - fs::write(repo.join(&scope_name), "source.py\ndeleted.py\n")?; + fs::write(repo.join(&scope_name), "./source.py\n./deleted.py\n")?; fs::write(repo.join(raw("scope-files-", 0xfffd)), "wrong.py\n")?; fs::write( repo.join(raw("input-", 0xfffd)), @@ -264,7 +264,7 @@ fn main() -> std::io::Result<()> { fs::write( repo.join(&input_name), concat!( - r#"{"cwe_ids":["CWE-89"],"locations":[{"path":"source.py","#, + r#"{"cwe_ids":["CWE-89"],"locations":[{"path":"./source.py","#, r#""start_line":1,"role":"entrypoint"}],"summary":"wide paths","#, r#""evidence":"source evidence"}"#, "\n", diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts index 5afda082c..01a127dad 100644 --- a/sdk/typescript/tests-ts/candidate-normalizer.test.ts +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -158,7 +158,7 @@ describe("built candidate normalizer", () => { const f = fixture(); const locations = [ location("app/query.py", 4, "sink"), - location(), + location("./app/routes.py"), location("app/query.py", 3, "root_control"), ]; const first = candidate(locations, { From f47cc357a891a5e3c1b0aaa6dffb7554b52f7816 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 16:05:20 +0000 Subject: [PATCH 11/14] fix(plugin): retain native errors for unresolved parent paths --- .../codex-security/mcp-app/src/helpers/posix-path.ts | 5 ++++- sdk/typescript/tests-ts/candidate-normalizer.test.ts | 11 ++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/helpers/posix-path.ts b/plugins/codex-security/mcp-app/src/helpers/posix-path.ts index 6eb6d0d4f..ea636908f 100644 --- a/plugins/codex-security/mcp-app/src/helpers/posix-path.ts +++ b/plugins/codex-security/mcp-app/src/helpers/posix-path.ts @@ -59,7 +59,10 @@ export function resolvePosixPath(value: Buffer, strict = true): Buffer { // Existing dangling links are rejected above; only absent components // may be appended to a canonical existing ancestor. const path = current.toString("latin1"); - missing.push(posix.basename(path)); + const name = posix.basename(path); + // Cancelling an unresolved component can expose an unresolved symlink. + if (name === "..") throw error; + missing.push(name); current = Buffer.from(posix.dirname(path), "latin1"); } } diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts index 01a127dad..d526e846c 100644 --- a/sdk/typescript/tests-ts/candidate-normalizer.test.ts +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -12,7 +12,7 @@ import { writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { dirname, join } from "node:path"; +import { basename, dirname, join } from "node:path"; import { afterEach, describe, expect, test } from "bun:test"; import { PLUGIN_ROOT } from "./plugin-root.js"; @@ -530,6 +530,15 @@ describe("built candidate normalizer", () => { symlinkSync(missing, dangling); expect(run({ ...f, output: dangling }, [[candidate()]]).status).toBe(2); expect(existsSync(missing)).toBe(false); + + symlinkSync(f.root, join(f.root, "directory-alias"), "dir"); + for (const protectedPath of [source, f.scope]) { + const before = readFileSync(protectedPath); + const output = `${f.root}/missing/../directory-alias/${basename(protectedPath)}`; + expect(invoke({ ...f, output }, [source]).status).toBe(2); + expect(readFileSync(protectedPath)).toEqual(before); + } + expect(existsSync(join(f.root, "missing"))).toBe(false); } const nested = { ...f, From f02379b72f753139dbaed7578febda7a78a9cd88 Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 16:51:53 +0000 Subject: [PATCH 12/14] style(plugin): format candidate scope normalization --- .../mcp-app/src/helpers/normalize-candidates.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index d562bdde3..4a791fa10 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -138,9 +138,7 @@ function readScope( } catch (error) { if (allowMissing && (error as NodeJS.ErrnoException).code === "ENOENT") { try { - scope.add( - inside(resolvedPath(join(root, line), false), root, true), - ); + scope.add(inside(resolvedPath(join(root, line), false), root, true)); } catch (error) { throw new Error( `in-scope file row ${index + 1}: path escapes repository`, From e26de2daf9cae2feb76ccc4df56fcc752fddc55e Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 17:03:37 +0000 Subject: [PATCH 13/14] test(plugin): expect native Windows output paths --- sdk/typescript/tests-ts/candidate-normalizer.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts index d526e846c..c491a5172 100644 --- a/sdk/typescript/tests-ts/candidate-normalizer.test.ts +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -12,7 +12,7 @@ import { writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; -import { basename, dirname, join } from "node:path"; +import { basename, dirname, join, toNamespacedPath } from "node:path"; import { afterEach, describe, expect, test } from "bun:test"; import { PLUGIN_ROOT } from "./plugin-root.js"; @@ -183,7 +183,7 @@ describe("built candidate normalizer", () => { ); expect(result.status, result.stderr).toBe(0); expect(result.stdout).toBe( - `Combined 3 candidate rows into 2 rows in ${f.output}\n`, + `Combined 3 candidate rows into 2 rows in ${toNamespacedPath(f.output)}\n`, ); const rows = ledger(f); expect(rows).toHaveLength(2); From a3779036920516beb7d947b12a8a7e6fbf2a3f8d Mon Sep 17 00:00:00 2001 From: Michael D'Angelo Date: Tue, 29 Sep 2026 17:29:58 +0000 Subject: [PATCH 14/14] refactor(plugin): use native argument parsing and JSON ordering --- .../src/helpers/normalize-candidates.ts | 26 +++--- .../src/helpers/resolve-security-md.ts | 89 ++++--------------- .../mcp-app/src/helpers/utf8.ts | 13 --- .../tests-ts/candidate-normalizer.test.ts | 15 +++- .../tests-ts/security-policy-helper.test.ts | 82 ++++------------- 5 files changed, 58 insertions(+), 167 deletions(-) diff --git a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts index 4a791fa10..09aa8ef02 100644 --- a/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -1,4 +1,4 @@ -import { compareUnicode as compare, decodeUtf8 } from "./utf8"; +import { decodeUtf8 } from "./utf8"; import { createHash, randomBytes } from "node:crypto"; import { closeSync, @@ -43,6 +43,8 @@ const fields = new Set([ "context", "instance", ]); +const locationFields = new Set(["path", "start_line", "end_line", "role"]); +const jsonFields = [...fields, ...locationFields].sort(); type Row = Record; interface Location { path: string; @@ -63,14 +65,12 @@ function object(value: unknown): value is Row { return typeof value === "object" && value !== null && !Array.isArray(value); } +function compare(left: string, right: string): number { + return left < right ? -1 : left > right ? 1 : 0; +} + function stableJson(value: unknown): string { - return JSON.stringify(value, (_key, item: unknown) => - object(item) - ? Object.fromEntries( - Object.entries(item).sort(([a], [b]) => compare(a, b)), - ) - : item, - ); + return JSON.stringify(value, jsonFields); } const windows = process.platform === "win32"; @@ -203,10 +203,8 @@ function normalizeLocations( for (const item of row.locations) { if (!object(item)) throw new Error("locations: expected location objects"); const unknown = Object.keys(item) - .filter( - (key) => !["path", "start_line", "end_line", "role"].includes(key), - ) - .sort(compare); + .filter((key) => !locationFields.has(key)) + .sort(); if (unknown.length) throw new Error(`locations: unsupported fields ${unknown.join(", ")}`); const [name, source] = relativeFile(item.path, root); @@ -258,7 +256,7 @@ function normalizeCandidate( ): Candidate { const unknown = Object.keys(row) .filter((key) => !fields.has(key)) - .sort(compare); + .sort(); if (unknown.length) throw new Error(`unsupported fields ${unknown.join(", ")}`); if ("candidate_id" in row) textField(row, "candidate_id"); @@ -290,7 +288,7 @@ function combine(groups: Map) { .filter((value): value is string => value !== undefined), ), ] - .sort(compare) + .sort() .join("\n"); const result = { ...group[0]!, diff --git a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts index 86dc0e8ae..96a8f5c13 100644 --- a/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts +++ b/plugins/codex-security/mcp-app/src/helpers/resolve-security-md.ts @@ -1,4 +1,4 @@ -import { compareUnicode, decodeUtf8 } from "./utf8"; +import { decodeUtf8 } from "./utf8"; import { closeSync, lstatSync, @@ -258,17 +258,12 @@ function listSecurityMd(repo: string, posixHome: string | undefined): string[] { const root = resolveRoot(repo, posixHome); const policies: string[] = []; function walk(directory: Buffer, prefix: string): void { - const entries = ( - windows - ? windowsFiles().entriesWithTypes(directory) - : readdirSync(directory, { encoding: "buffer", withFileTypes: true }) - ).map((entry) => ({ - bytes: entry.name, - name: decodePath(entry.name), - entry, - })); - entries.sort((left, right) => compareUnicode(left.name, right.name)); - for (const { bytes, name, entry: listedEntry } of entries) { + const entries = windows + ? windowsFiles().entriesWithTypes(directory) + : readdirSync(directory, { encoding: "buffer", withFileTypes: true }); + for (const listedEntry of entries) { + const bytes = listedEntry.name; + const name = decodePath(bytes); if (name === ".git") continue; const path = appendPath(directory, bytes); const source = prefix === "" ? name : `${prefix}/${name}`; @@ -304,7 +299,7 @@ function listSecurityMd(repo: string, posixHome: string | undefined): string[] { } } walk(root, ""); - return policies.sort(compareUnicode); + return policies.sort(); } function readPolicy(path: Buffer, displayedPath: Buffer): string { @@ -398,67 +393,15 @@ export function resolveSecurityMdCommand( posixHome = process.env.HOME, ): number { try { - const options = { - repo: { type: "string" }, - list: { type: "boolean" }, - scope: { type: "string" }, - out: { type: "string", default: "-" }, - help: { type: "boolean", short: "h" }, - } as const; - const names = Object.keys(options) as (keyof typeof options)[]; - let parsedArgs: string[] = []; - for (let index = 0; index < args.length; index++) { - let arg = args[index]!; - if (arg === "--") throw new Error("Unexpected argument '--'"); - if (arg.startsWith("-h")) { - if (/^-h+-/u.test(arg)) throw new Error(`Unexpected argument '${arg}'`); - if (/^-h+=/u.test(arg)) parseArgs({ args: [arg], options }); - arg = "--help"; - } - if (arg.startsWith("--") && arg !== "--") { - const equals = arg.indexOf("="); - const name = arg.slice(2, equals === -1 ? undefined : equals); - const matches = names.filter((option) => option.startsWith(name)); - const option = matches.length === 1 ? matches[0] : undefined; - if (option !== undefined) { - // argparse accepts unique long-option prefixes. - arg = `--${option}${equals === -1 ? "" : arg.slice(equals)}`; - const next = args[index + 1]; - if ( - equals === -1 && - options[option].type === "string" && - next !== undefined - ) { - const prefix = next.split("=", 1)[0]!; - const optional = - next.startsWith("-h") || - names.some((name) => `--${name}`.startsWith(prefix)); - // Declared options take precedence over negative numbers and spaces. - if ( - !next.startsWith("-") || - next === "-" || - (!optional && - (next.includes(" ") || - /^-(?:\p{Decimal_Number}+|\p{Decimal_Number}*\.\p{Decimal_Number}+)\n?$/u.test( - next, - ))) - ) { - arg += `=${next}`; - index++; - } - } - } - if (matches.length) parseArgs({ args: [arg], options }); - } - parsedArgs.push(arg); - if (arg === "--help") { - parsedArgs = [arg]; - break; - } - } const { values } = parseArgs({ - args: parsedArgs, - options, + args, + options: { + repo: { type: "string" }, + list: { type: "boolean" }, + scope: { type: "string" }, + out: { type: "string", default: "-" }, + help: { type: "boolean", short: "h" }, + }, }); if (values.help) { console.log( diff --git a/plugins/codex-security/mcp-app/src/helpers/utf8.ts b/plugins/codex-security/mcp-app/src/helpers/utf8.ts index e80c47974..5b2e71e4d 100644 --- a/plugins/codex-security/mcp-app/src/helpers/utf8.ts +++ b/plugins/codex-security/mcp-app/src/helpers/utf8.ts @@ -1,18 +1,5 @@ import { isUtf8 } from "node:buffer"; -export function compareUnicode(left: string, right: string): number { - let leftIndex = 0; - let rightIndex = 0; - while (leftIndex < left.length && rightIndex < right.length) { - const leftPoint = left.codePointAt(leftIndex)!; - const rightPoint = right.codePointAt(rightIndex)!; - if (leftPoint !== rightPoint) return leftPoint - rightPoint; - leftIndex += leftPoint > 0xffff ? 2 : 1; - rightIndex += rightPoint > 0xffff ? 2 : 1; - } - return left.length - right.length; -} - export function decodeUtf8(bytes: Buffer): string { // Node 20's fatal TextDecoder can silently replace invalid input bytes. if (!isUtf8(bytes)) diff --git a/sdk/typescript/tests-ts/candidate-normalizer.test.ts b/sdk/typescript/tests-ts/candidate-normalizer.test.ts index c491a5172..3452542a5 100644 --- a/sdk/typescript/tests-ts/candidate-normalizer.test.ts +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -268,7 +268,7 @@ describe("built candidate normalizer", () => { expect(rejected.stderr).toContain("expected at least one in-scope file"); }); - test("sorts Unicode code points, normalizes CWE IDs, and trims text", () => { + test("normalizes Unicode candidates deterministically with native string order", () => { const f = fixture(); const names = ["app/\u{10000}.py", "app/\ue000.py"]; for (const name of names) write(join(f.repo, name), "line\n"); @@ -290,11 +290,18 @@ describe("built candidate normalizer", () => { "CWE-9007199254740993", ]); expect((value["locations"] as Row[]).map((item) => item["path"])).toEqual( - [...names].reverse(), + names, ); expect(value["summary"]).toBe("Summary"); - expect(value["evidence"]).toBe("\ue000\n\u{10000}"); - expect(value["candidate_id"]).toBe("candidate-cc5ebd3ebd732a50"); + expect(value["evidence"]).toBe("\u{10000}\n\ue000"); + const reordered = { + ...row, + locations: [...(row.locations as Row[])].reverse(), + }; + expect( + run(f, [[{ ...reordered, evidence: "\ue000" }, reordered]]).status, + ).toBe(0); + expect(ledger(f)).toEqual([value]); expect(readFileSync(f.output, "utf8").startsWith('{"candidate_id":')).toBe( true, ); diff --git a/sdk/typescript/tests-ts/security-policy-helper.test.ts b/sdk/typescript/tests-ts/security-policy-helper.test.ts index 9b0e8ce44..a3caa1902 100644 --- a/sdk/typescript/tests-ts/security-policy-helper.test.ts +++ b/sdk/typescript/tests-ts/security-policy-helper.test.ts @@ -74,56 +74,24 @@ afterEach(() => { }); describe("built SECURITY.md helper", () => { - test("accepts negative-number paths and unique long-option prefixes", () => { - const { root } = fixture(); - const cases: [string, string, string][] = [ - ["-1", "-2", "-3"], - ["-١", "-.5", "-1.5"], - ]; - if (process.platform !== "win32") cases.push(["-4", "-1\n", "-3\n"]); - for (const [repo, scope, output] of cases) { - write(root, `${repo}/${scope}/SECURITY.md`, "negative path policy\n"); - const result = run( - ["--r", repo, "--s", scope, "--o", output], - process.env, - root, - ); - expect(result.status, result.stderr).toBe(0); - expect(result.stdout).toBe(""); - expect(readFileSync(join(root, output), "utf8")).toContain( - "negative path policy", - ); - } - }); - - test("accepts dash-prefixed paths containing spaces after option matching", () => { + test("accepts dash-prefixed paths with equals syntax", () => { const { root } = fixture(); for (const [repo, scope, output] of [ - ["- repository", "- archived", "- guidance"], - ["--repo space", "--special space", "--other= output"], + ["-1", "-2", "-3"], + ["--repo space", "--scope space", "--output= guidance"], ] as const) { - write(root, `${repo}/${scope}/SECURITY.md`, "space path policy\n"); + write(root, `${repo}/${scope}/SECURITY.md`, "dash path policy\n"); const result = run( - ["--repo", repo, "--scope", scope, "--out", output], + [`--repo=${repo}`, `--scope=${scope}`, `--out=${output}`], process.env, root, ); expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toBe(""); expect(readFileSync(join(root, output), "utf8")).toContain( - "space path policy", + "dash path policy", ); } - for (const value of [ - "-hello world", - "--help=some text", - "--s=some text", - "--unsupported", - "-tab\tvalue", - ]) { - const result = run(["--repo", root, "--scope", value]); - expect(result.status, result.stderr).toBe(2); - expect(result.stdout).toBe(""); - } }); test.skipIf(process.platform !== "win32")( @@ -275,7 +243,7 @@ describe("built SECURITY.md helper", () => { expect(existsSync(join(root, "temporary.tmp"))).toBe(false); }); - test("frames Unicode paths as ASCII JSON in codepoint order", () => { + test("frames Unicode paths as ASCII JSON in standard string order", () => { const { root } = fixture(); for (const name of ["\u{10000}", "\ue000", "\u0080", "\u007f"]) { write(root, `${name}/SECURITY.md`, "policy\n"); @@ -283,7 +251,7 @@ describe("built SECURITY.md helper", () => { const result = inventory(root); expect(result.status, result.stderr).toBe(0); expect(result.stdout).toBe( - '["\\u007f/SECURITY.md", "\\u0080/SECURITY.md", "\\ue000/SECURITY.md", "\\ud800\\udc00/SECURITY.md"]\n', + '["\\u007f/SECURITY.md", "\\u0080/SECURITY.md", "\\ud800\\udc00/SECURITY.md", "\\ue000/SECURITY.md"]\n', ); const guidance = resolve(root, "\u{10000}").stdout; expectGuidance(guidance, [["\u{10000}/SECURITY.md", "policy"]]); @@ -319,7 +287,7 @@ describe("built SECURITY.md helper", () => { const result = inventory(root); expect(result.status, result.stderr).toBe(0); expect(result.stdout).toBe( - '["SECURITY.md", "\\u00e9\\udcff/SECURITY.md", "\\u00e9\\ue000/SECURITY.md", "\\u00e9\\ud800\\udc00/SECURITY.md"]\n', + '["SECURITY.md", "\\u00e9\\ud800\\udc00/SECURITY.md", "\\u00e9\\udcff/SECURITY.md", "\\u00e9\\ue000/SECURITY.md"]\n', ); expect(result.stderr).toBe(""); }, @@ -840,27 +808,15 @@ describe("built SECURITY.md helper", () => { test("preserves required and mutually exclusive helper arguments", () => { const { root } = fixture(); - for (const [args, status] of [ - [["--help", "--bogus"], 0], - [["-h", "--scope"], 0], - [["--hel", "--scope"], 0], - [["-hh", "--bogus"], 0], - [["--bogus", "--help"], 0], - [["positional", "--help"], 0], - [["-hfoo"], 0], - [["-hfoo-"], 0], - [["--scope", "--help"], 2], - [["--list=value", "--help"], 2], - [["-h=foo"], 2], - [["-h-"], 2], - [["-hh-"], 2], - [["-h--help"], 2], - [["--"], 2], - [["--", "--help"], 2], - ] as const) { - const result = run(["--repo", root, "--scope", ".", ...args]); - expect(result.status, result.stderr).toBe(status); - expect(result.stdout.includes("Usage:")).toBe(status === 0); + for (const flag of ["--help", "-h"]) { + const result = run([flag]); + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toContain("Usage:"); + } + for (const args of [["--unknown"], ["--scope"], ["positional"]]) { + const result = run(["--repo", root, ...args]); + expect(result.status, result.stderr).toBe(2); + expect(result.stdout).toBe(""); } for (const [args, message] of [ [["--list"], "--repo is required"],