diff --git a/plugins/codex-security/mcp-app/helpers-main.ts b/plugins/codex-security/mcp-app/helpers-main.ts index 18c8611be..8aad95453 100644 --- a/plugins/codex-security/mcp-app/helpers-main.ts +++ b/plugins/codex-security/mcp-app/helpers-main.ts @@ -1,6 +1,9 @@ +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"; +import { validatePatchRiskAssessmentCommand } from "./src/helpers/validate-patch-risk-assessment"; let commandLine = process.argv.slice(2); if (process.platform === "win32") { @@ -14,8 +17,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 +31,13 @@ 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 if (command === "validate-patch-risk-assessment") { + process.exitCode = validatePatchRiskAssessmentCommand(args); } 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/contract-schema.ts b/plugins/codex-security/mcp-app/src/helpers/contract-schema.ts new file mode 100644 index 000000000..ba076ac78 --- /dev/null +++ b/plugins/codex-security/mcp-app/src/helpers/contract-schema.ts @@ -0,0 +1,173 @@ +import { JsonFloat, object, objectEntries, pythonRepr } from "./python-json"; + +type SchemaType = + | "array" + | "boolean" + | "integer" + | "number" + | "object" + | "string" + | "null"; +export interface ContractSchema { + $ref?: string; + type?: SchemaType | SchemaType[]; + const?: unknown; + enum?: unknown[]; + minLength?: number; + pattern?: string; + minimum?: number; + maximum?: number; + minItems?: number; + maxItems?: number; + uniqueItems?: boolean; + items?: ContractSchema; + required?: string[]; + minProperties?: number; + properties?: Record; + additionalProperties?: boolean | ContractSchema; + [key: string]: unknown; +} + +const numeric = (value: unknown): value is number | bigint | JsonFloat => + typeof value === "number" || + typeof value === "bigint" || + value instanceof JsonFloat; +const number = (value: number | bigint | JsonFloat) => + value instanceof JsonFloat ? Number(value.source) : value; + +function equal(left: unknown, right: unknown): boolean { + if (numeric(left) && numeric(right)) { + const a = number(left), + b = number(right); + if (typeof a === typeof b) return a === b; + const integer = typeof a === "bigint" ? a : b; + const floating = typeof a === "number" ? a : (b as number); + return ( + Number.isFinite(floating) && + Number.isInteger(floating) && + integer === BigInt(floating) + ); + } + return left === right; +} + +function matches(value: unknown, expected: SchemaType): boolean { + switch (expected) { + case "array": + return Array.isArray(value); + case "boolean": + return typeof value === "boolean"; + case "integer": + return ( + typeof value === "bigint" || + (typeof value === "number" && Number.isInteger(value)) + ); + case "number": + return numeric(value); + case "object": + return object(value); + case "string": + return typeof value === "string"; + case "null": + return value === null; + } +} + +/** The assessment schema uses the same structural rules as the scan contract. */ +export function validateAgainstSchema( + value: unknown, + schema: ContractSchema, + context: string, + root: ContractSchema = schema, +): void { + const fail: (message: string) => never = (message) => { + throw new Error(`${context}: ${message}`); + }; + if (schema.$ref !== undefined) { + const reference = schema.$ref; + if (typeof reference !== "string") + fail("schema reference must be a string"); + let target: unknown = root; + if (reference !== "#") { + if (!reference.startsWith("#/")) + fail(`unsupported schema reference ${pythonRepr(reference)}`); + for (const part of reference.slice(2).split("/")) { + const key = part.replaceAll("~1", "/").replaceAll("~0", "~"); + if (!object(target) || !Object.hasOwn(target, key)) + fail(`unresolved schema reference ${pythonRepr(reference)}`); + target = target[key]; + } + } + if (!object(target)) + fail(`schema reference ${pythonRepr(reference)} is not an object`); + validateAgainstSchema(value, target as ContractSchema, context, root); + } + const expected = schema.type; + if (Array.isArray(expected)) { + if (!expected.some((type) => matches(value, type))) + fail(`does not match schema type ${pythonRepr(expected)}`); + } else if (typeof expected === "string" && !matches(value, expected)) { + fail(`expected schema type ${expected}`); + } + if (Object.hasOwn(schema, "const") && !equal(value, schema.const)) + fail(`expected ${pythonRepr(schema.const)}`); + if (schema.enum && !schema.enum.some((candidate) => equal(value, candidate))) + fail(`unsupported value ${pythonRepr(value)}`); + if (typeof value === "string") { + if (schema.minLength && Array.from(value).length < schema.minLength) + fail("string is too short"); + if ( + schema.pattern !== undefined && + !new RegExp(`^(?:${schema.pattern})$(?![\\s\\S])`, "u").test(value) + ) + fail("string does not match schema pattern"); + } + if (numeric(value)) { + if (schema.minimum !== undefined && number(value) < schema.minimum) + fail("value is below schema minimum"); + if (schema.maximum !== undefined && number(value) > schema.maximum) + fail("value is above schema maximum"); + } + if (Array.isArray(value)) { + if (schema.minItems !== undefined && value.length < schema.minItems) + fail("array has too few items"); + if (schema.maxItems !== undefined && value.length > schema.maxItems) + fail("array has too many items"); + if (schema.items) + value.forEach((item, index) => + validateAgainstSchema( + item, + schema.items!, + `${context}[${index}]`, + root, + ), + ); + if (schema.uniqueItems === true && new Set(value).size !== value.length) + fail("array items must be unique"); + } + if (object(value)) { + for (const key of schema.required ?? []) + if (!Object.hasOwn(value, key)) + throw new Error(`${context}.${key}: missing required schema property`); + if ( + schema.minProperties !== undefined && + Object.keys(value).length < schema.minProperties + ) + fail("object has too few properties"); + for (const [key, item] of objectEntries(value)) { + const child = Object.hasOwn(schema.properties ?? {}, key) + ? schema.properties![key] + : undefined; + if (child) validateAgainstSchema(item, child, `${context}.${key}`, root); + else if (schema.additionalProperties === false) + throw new Error(`${context}.${key}: unexpected schema property`); + else if (object(schema.additionalProperties)) + validateAgainstSchema( + item, + schema.additionalProperties, + `${context}.${key}`, + root, + ); + } + } +} diff --git a/plugins/codex-security/mcp-app/src/helpers/helper-files.ts b/plugins/codex-security/mcp-app/src/helpers/helper-files.ts new file mode 100644 index 000000000..37ddcf4fd --- /dev/null +++ b/plugins/codex-security/mcp-app/src/helpers/helper-files.ts @@ -0,0 +1,11 @@ +import { readFileSync } from "node:fs"; +import { windowsBinding } from "../native"; +import { widePath, windowsFileSystem } from "../../../native/windows-files.mjs"; +import { encodePosixPath } from "./posix-path"; + +export function readFile(path: string | number): Buffer { + if (typeof path === "number") return readFileSync(path); + return process.platform === "win32" + ? windowsFileSystem(windowsBinding()).readFile(widePath(path)) + : readFileSync(encodePosixPath(path)); +} 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..523733325 --- /dev/null +++ b/plugins/codex-security/mcp-app/src/helpers/normalize-candidates.ts @@ -0,0 +1,595 @@ +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"; + +import { object } from "./python-json"; + +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; +} + +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): number { + if (typeof value !== "number" || !Number.isInteger(value) || value < 1) + 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 > 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: start, + end_line: 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) => 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)) + 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: unknown = JSON.parse(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/python-json.ts b/plugins/codex-security/mcp-app/src/helpers/python-json.ts new file mode 100644 index 000000000..0796749df --- /dev/null +++ b/plugins/codex-security/mcp-app/src/helpers/python-json.ts @@ -0,0 +1,164 @@ +// Preserve Python's integer/float distinction and arbitrary-size JSON integers. +export class JsonFloat { + constructor(readonly source: string) {} +} + +type Row = Record; +const keyOrder = new WeakMap(); +export function object(value: unknown): value is Row { + return ( + typeof value === "object" && + value !== null && + !Array.isArray(value) && + !(value instanceof JsonFloat) + ); +} +export function objectEntries(value: Row): [string, unknown][] { + return (keyOrder.get(value) ?? Object.keys(value)).map((key) => [ + key, + value[key], + ]); +} + +export class JsonSyntaxError extends Error {} + +export function parseJson(source: string, rejectDuplicates = false): unknown { + const tokens = [ + ...source.matchAll( + /"(?:\\[\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 position = () => tokens[index]?.index ?? source.length; + function error(message: string, offset = position()): never { + const before = source.slice(0, offset); + const line = before.split("\n").length; + const column = + Array.from(before.slice(before.lastIndexOf("\n") + 1)).length + 1; + throw new JsonSyntaxError( + `${message}: line ${line} column ${column} (char ${Array.from(before).length})`, + ); + } + if (source.startsWith("\ufeff")) + error("Unexpected UTF-8 BOM (decode using utf-8-sig)", 0); + const take = () => tokens[index++]?.[0]; + function expect(token: string): void { + if (tokens[index]?.[0] !== token) error(`Expecting '${token}' delimiter`); + index++; + } + function string(token: string, start: number): string { + try { + return JSON.parse(token) as string; + } catch (cause) { + return error((cause as Error).message, start); + } + } + function value(): unknown { + const start = position(); + const token = take(); + if (token === "{") { + const row = Object.create(null) as Row; + const keys: string[] = []; + function finish(): Row { + index++; + const unique = new Set(); + for (const key of keys) { + if (rejectDuplicates && unique.has(key)) + throw new Error(`duplicate JSON object key: ${key}`); + unique.add(key); + } + keyOrder.set(row, [...unique]); + return row; + } + if (tokens[index]?.[0] === "}") return finish(); + while (true) { + const keyStart = position(); + const key = take(); + if (!key?.startsWith('"')) + error("Expecting property name enclosed in double quotes", keyStart); + const name = string(key, keyStart); + expect(":"); + row[name] = value(); + keys.push(name); + if (tokens[index]?.[0] === "}") return finish(); + expect(","); + } + } + if (token === "[") { + const values: unknown[] = []; + if (tokens[index]?.[0] === "]") { + index++; + return values; + } + while (true) { + values.push(value()); + if (tokens[index]?.[0] === "]") { + index++; + return values; + } + expect(","); + } + } + if (token?.startsWith('"')) return string(token, start); + 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); + return error("Expecting value", start); + } + const result = value(); + if (index !== tokens.length) error("Extra data"); + return result; +} + +export function pythonRepr(value: unknown): string { + if (typeof value === "string") { + const quote = value.includes("'") && !value.includes('"') ? '"' : "'"; + return ( + quote + + Array.from(value, (character) => { + if (character === quote || character === "\\") return `\\${character}`; + if (character === "\n") return "\\n"; + if (character === "\r") return "\\r"; + if (character === "\t") return "\\t"; + if (character !== " " && /[\p{C}\p{Z}]/u.test(character)) { + const point = character.codePointAt(0)!; + return point <= 0xff + ? `\\x${point.toString(16).padStart(2, "0")}` + : point <= 0xffff + ? `\\u${point.toString(16).padStart(4, "0")}` + : `\\U${point.toString(16).padStart(8, "0")}`; + } + return character; + }).join("") + + quote + ); + } + if (value === null) return "None"; + if (value === true) return "True"; + if (value === false) return "False"; + if (value instanceof JsonFloat) { + const number = Number(value.source); + if (Number.isNaN(number)) return "nan"; + if (!Number.isFinite(number)) return number < 0 ? "-inf" : "inf"; + if (Object.is(number, -0)) return "-0.0"; + const magnitude = Math.abs(number); + if (magnitude !== 0 && (magnitude < 0.0001 || magnitude >= 1e16)) + return number.toExponential().replace(/e([+-])([0-9])$/u, "e$10$2"); + return number.toString() + (Number.isInteger(number) ? ".0" : ""); + } + if (Array.isArray(value)) return `[${value.map(pythonRepr).join(", ")}]`; + if (object(value)) + return `{${objectEntries(value) + .map(([key, item]) => `${pythonRepr(key)}: ${pythonRepr(item)}`) + .join(", ")}}`; + return String(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 6e87b6d09..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, @@ -13,7 +14,11 @@ import { homedir } from "node:os"; import { basename, dirname, parse, sep } from "node:path"; import { parseArgs } from "node:util"; import { unixBinding, windowsBinding } from "../native"; -import { windowsFileSystem } from "../../../native/windows-files.mjs"; +import { + windowsFileSystem, + windowsJoin, + windowsParts, +} from "../../../native/windows-files.mjs"; import { decodePosixBytes, encodePosixPath, @@ -22,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) => @@ -36,40 +40,7 @@ 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; -} - -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("/", "\\") @@ -100,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) => @@ -167,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))); @@ -336,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/src/helpers/validate-patch-risk-assessment.ts b/plugins/codex-security/mcp-app/src/helpers/validate-patch-risk-assessment.ts new file mode 100644 index 000000000..4f111d5c5 --- /dev/null +++ b/plugins/codex-security/mcp-app/src/helpers/validate-patch-risk-assessment.ts @@ -0,0 +1,223 @@ +import { decodeUtf8 } from "./utf8"; +import assessmentSchema from "../../../schemas/patch-risk-assessment.schema.json"; +import { validateAgainstSchema, type ContractSchema } from "./contract-schema"; +import { readFile } from "./helper-files"; +import { decodePosixBytes } from "./posix-path"; +import { JsonSyntaxError, object, parseJson, pythonRepr } from "./python-json"; +import { parsedPath } from "./resolve-security-md"; + +interface Assessment { + recommendation: "merge" | "revise" | "no_op" | "block" | "hold_for_evidence"; + workflowLabel: string; + impact: { rating: string }; + regressionLikelihood: { rating: string }; + regressionProtection: { rating: string; exactHeadChecksPassed: boolean }; + recoverability: { rating: string }; + confidence: { rating: string }; + applicability: { status: string }; + statusQuoRisk: { rating: string }; + affectedRuntimeRoots: string[]; + autoMergeExclusions: string[]; + materialBoundaries: { result: string }[]; + validation: { status: string }[]; + unknowns: { decisionCritical: boolean }[]; + evidencePlan: unknown[]; +} + +const nonApplicable = new Set([ + "no_live_effect", + "wrong_owner", + "duplicate", + "superseded", +]); + +export function validatePatchRiskAssessment(value: unknown): string[] { + if (!object(value)) return ["assessment must be a JSON object"]; + try { + validateAgainstSchema( + value, + assessmentSchema as ContractSchema, + "patch-risk-assessment.schema", + ); + } catch (error) { + return [error instanceof Error ? error.message : String(error)]; + } + const assessment = value as unknown as Assessment; + const { + recommendation, + workflowLabel, + unknowns, + evidencePlan, + materialBoundaries: boundaries, + } = assessment; + const applicability = assessment.applicability.status; + const affirmativeFailure = + assessment.regressionLikelihood.rating === "critical" || + boundaries.some((item) => item.result === "contradicted") || + assessment.validation.some((item) => item.status === "failed"); + const errors: string[] = []; + + if (recommendation === "merge") { + if ( + !["auto_merge_candidate", "human_review_required"].includes(workflowLabel) + ) + errors.push( + "merge requires an auto-merge or human-review workflow label", + ); + if (applicability !== "confirmed") + errors.push("merge requires confirmed applicability"); + if (unknowns.some((item) => item.decisionCritical)) + errors.push("merge cannot retain a decision-critical unknown"); + if (boundaries.some((item) => item.result !== "supported")) + errors.push("merge requires every material boundary to be supported"); + if (assessment.validation.some((item) => item.status === "failed")) + errors.push("merge cannot retain a failed validation"); + if (evidencePlan.length) + errors.push("merge cannot retain an evidence plan"); + } else if (workflowLabel !== recommendation) { + errors.push("non-merge workflow label must match the recommendation"); + } + + if (recommendation === "hold_for_evidence") { + if (!unknowns.some((item) => item.decisionCritical)) + errors.push("hold_for_evidence requires a decision-critical unknown"); + if (!evidencePlan.length) + errors.push("hold_for_evidence requires a bounded evidence plan"); + if (affirmativeFailure) + errors.push("hold_for_evidence cannot defer an established defect"); + } else if (evidencePlan.length) { + errors.push("only hold_for_evidence may include an evidence plan"); + } + + if (recommendation === "no_op") { + if (!nonApplicable.has(applicability)) + errors.push("no_op requires an established non-applicable disposition"); + if (unknowns.some((item) => item.decisionCritical)) + errors.push("no_op cannot retain a decision-critical unknown"); + } else if (nonApplicable.has(applicability)) { + errors.push("an established non-applicable disposition requires no_op"); + } + + if (["revise", "block"].includes(recommendation) && !affirmativeFailure) + errors.push(`${recommendation} requires affirmative failure evidence`); + + if (workflowLabel === "auto_merge_candidate") { + const requirements: Record = { + "impact.rating": assessment.impact.rating === "low", + "regressionLikelihood.rating": + assessment.regressionLikelihood.rating === "low", + "regressionProtection.rating": + assessment.regressionProtection.rating === "strong", + "regressionProtection.exactHeadChecksPassed": + assessment.regressionProtection.exactHeadChecksPassed, + "recoverability.rating": assessment.recoverability.rating === "easy", + "confidence.rating": assessment.confidence.rating === "high", + "applicability.status": applicability === "confirmed", + affectedRuntimeRoots: assessment.affectedRuntimeRoots.length > 0, + "statusQuoRisk.rating": assessment.statusQuoRisk.rating !== "unknown", + autoMergeExclusions: assessment.autoMergeExclusions.length === 0, + unknowns: unknowns.length === 0, + validation: assessment.validation.every( + (item) => item.status === "passed", + ), + }; + for (const [field, passed] of Object.entries(requirements)) + if (!passed) errors.push(`auto_merge_candidate gate failed: ${field}`); + } + return errors; +} + +function readAssessment(path: string): unknown { + let contents: Buffer; + try { + contents = readFile(path === "-" ? 0 : parsedPath(path)); + } catch (error) { + throw new Error( + `cannot read assessment: ${error instanceof Error ? error.message : String(error)}`, + ); + } + const text = + path === "-" + ? decodePosixBytes(contents) + : decodeUtf8(contents).replace(/\r\n?/gu, "\n"); + try { + return parseJson(text, true); + } catch (error) { + if (error instanceof JsonSyntaxError) + throw new Error(`cannot read assessment: ${error.message}`); + throw error; + } +} + +function report(message: string): void { + // Match Python's UTF-8 stderr when a diagnostic contains an unpaired surrogate. + console.error( + message.replace( + /[\ud800-\udfff]/gu, + (character) => + `\\u${character.charCodeAt(0).toString(16).padStart(4, "0")}`, + ), + ); +} + +export function validatePatchRiskAssessmentCommand(args: string[]): number { + const usage = + "usage: launch_codex_security_mcp[.cmd] --helper validate-patch-risk-assessment [-h] assessment"; + const argumentError = (message: string) => { + report(`${usage}\nvalidate-patch-risk-assessment: error: ${message}`); + return 2; + }; + let path: string | undefined; + let positional = false; + const extra: string[] = []; + for (const arg of args) { + if (!positional && arg === "--") { + positional = true; + continue; + } + if (!positional) { + const option = arg.split("=", 1)[0]; + if ( + ["--h", "--he", "--hel", "--help"].includes(option) || + arg.startsWith("-h") + ) { + if ( + arg.startsWith(`${option}=`) && + (option === "-h" || option.startsWith("--")) + ) + return argumentError( + `argument -h/--help: ignored explicit argument ${pythonRepr(arg.slice(option.length + 1))}`, + ); + console.log( + `${usage}\n\nValidate a patch-risk assessment.\n\npositional arguments:\n assessment Assessment JSON path, or - for stdin.\n\noptions:\n -h, --help show this help message and exit`, + ); + return 0; + } + if ( + arg.startsWith("-") && + arg !== "-" && + !arg.includes(" ") && + !/^-(?:\p{Decimal_Number}+|\p{Decimal_Number}*\.\p{Decimal_Number}+)\n?$/u.test( + arg, + ) + ) { + extra.push(arg); + continue; + } + } + if (path === undefined) path = arg; + else extra.push(arg); + } + if (path === undefined) + return argumentError("the following arguments are required: assessment"); + if (extra.length) + return argumentError(`unrecognized arguments: ${extra.join(" ")}`); + try { + const errors = validatePatchRiskAssessment(readAssessment(path)); + for (const error of errors) report(error); + return errors.length ? 1 : 0; + } catch (error) { + report(error instanceof Error ? error.message : String(error)); + return 1; + } +} 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 12e50e601..50034bb04 100644 --- a/plugins/codex-security/native/README.md +++ b/plugins/codex-security/native/README.md @@ -39,9 +39,9 @@ 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. -Four 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. +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. Assessment validation shares the typed wide-path reader. -`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. `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. +`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. Build on Windows after compiling the TypeScript tools, then run: diff --git a/plugins/codex-security/native/examples/windows-wide-launcher.rs b/plugins/codex-security/native/examples/windows-wide-launcher.rs index 635875fb9..4f8fd74aa 100644 --- a/plugins/codex-security/native/examples/windows-wide-launcher.rs +++ b/plugins/codex-security/native/examples/windows-wide-launcher.rs @@ -35,6 +35,16 @@ fn main() -> std::io::Result<()> { for (index, name) in names.iter().enumerate() { fs::write(cwd.join(name), format!("sentinel-{index}"))?; } + std::os::windows::fs::symlink_file(&names[0], cwd.join("relative-link"))?; + std::os::windows::fs::symlink_file(raw("missing-", 0xdfff), cwd.join("missing-link"))?; + std::os::windows::fs::symlink_file( + Path::new("..").join(raw("missing-", 0xdfff)), + cwd.join("missing-parent-link"), + )?; + std::os::windows::fs::symlink_file("loop-link", cwd.join("loop-link"))?; + fs::write(cwd.join("missing-tail"), "ordinary sibling")?; + std::os::windows::fs::symlink_file("missing-tail.", cwd.join("dot-target-link"))?; + std::os::windows::fs::symlink_file("missing-tail ", cwd.join("space-target-link"))?; fs::create_dir(cwd.join("empty"))?; fs::create_dir(cwd.join(raw("directory-", 0xdc80)))?; std::os::windows::fs::symlink_file(&names[0], cwd.join("file-link"))?; @@ -223,14 +233,157 @@ 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", + )); + } + } + } + let assessment_name = raw("assessment-", 0xd800); + let assessment_path = repo.join(&assessment_name); + let replacement_assessment = repo.join(raw("assessment-", 0xfffd)); + let assessment = r#"{ + "schemaVersion":1, + "patch":{"repository":"example/project","sourceType":"patch_file","base":"base","head":"head","changedFiles":["src/example.ts"],"sha256":"cccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc"}, + "recommendation":"no_op","workflowLabel":"no_op", + "impact":{"rating":"low","rationale":"No active path changes."}, + "regressionLikelihood":{"rating":"low","rationale":"No live effect."}, + "regressionProtection":{"rating":"strong","rationale":"Fixture validated.","exactHeadChecksPassed":true}, + "recoverability":{"rating":"easy","rationale":"Local change."}, + "confidence":{"rating":"high","rationale":"Known fixture."}, + "applicability":{"status":"no_live_effect","rationale":"Synthetic input."}, + "statusQuoRisk":{"rating":"low","rationale":"No live effect."}, + "autoMergeExclusions":[],"affectedRuntimeRoots":[],"materialBoundaries":[], + "validation":[{"name":"fixture","status":"passed","protects":"Validator input."}], + "unknowns":[],"evidencePlan":[] + }"#; + fs::write(&assessment_path, assessment)?; + fs::write(&replacement_assessment, "replacement assessment sentinel")?; + for input in [assessment_path.clone(), PathBuf::from(&assessment_name)] { + let child = Command::new(&node) + .arg(&script) + .args(["--helper", "validate-patch-risk-assessment"]) + .arg(input) + .current_dir(&repo) + .output()?; + if !child.status.success() || !child.stdout.is_empty() || !child.stderr.is_empty() { + return Err(io::Error::other(format!( + "Wide assessment helper failed: {}", + String::from_utf8_lossy(&child.stderr) + ))); + } + } + if fs::read(&assessment_path)? != assessment.as_bytes() + || fs::read(&replacement_assessment)? != b"replacement assessment sentinel" + { + return Err(io::Error::other("Assessment validation changed its input")); + } + println!("{{\"policyHelperRawPaths\":true,\"candidateHelperRawPaths\":true,\"assessmentHelperRawPaths\":true,\"directoryIdentity\":true}}"); Ok(()) } let mut args = env::args_os().skip(1); let node = args.next().expect("Node executable path"); let script = args.next().expect("Windows wide proof script"); - let root = PathBuf::from(args.next().expect("Proof fixture directory")).join("wide-process"); + let root = PathBuf::from(args.next().expect("Proof fixture directory")).join("wide-İprocess"); fs::create_dir(&root)?; let result = if args.next().is_some_and(|argument| argument == "policy") { policy_proof(node, script, &root) diff --git a/plugins/codex-security/native/proof-windows-wide.mts b/plugins/codex-security/native/proof-windows-wide.mts index 5ec3c1ccb..a36f7f056 100644 --- a/plugins/codex-security/native/proof-windows-wide.mts +++ b/plugins/codex-security/native/proof-windows-wide.mts @@ -85,6 +85,10 @@ function worker(root: string): Record { `${drive}\\rooted-\ud800`, ); samePath(files.realpath(widePath(".")), cwd); + samePath(files.realpath(widePath(" "), false), cwd); + assert.throws(() => files.readFile(widePath(".")), { winerror: 5 }); + for (const name of ["NUL", "nUl", ".\\NUL", "x\\..\\NUL"]) + samePath(files.realpath(widePath(name)), "\\\\.\\NUL"); const names = [ "high-\ud800", @@ -110,6 +114,13 @@ function worker(root: string): Record { "directory-link", "dangling-directory-link", "locked-\udfff", + "relative-link", + "missing-link", + "missing-parent-link", + "loop-link", + "missing-tail", + "dot-target-link", + "space-target-link", ].sort(), ); for (const spelling of [".", `${drive}.`, cwd, win32.toNamespacedPath(cwd)]) { @@ -135,7 +146,17 @@ function worker(root: string): Record { .filter((entry) => entry.isSymbolicLink()) .map((entry) => pathText(entry.name)) .sort(), - ["dangling-directory-link", "directory-link", "file-link"], + [ + "dangling-directory-link", + "directory-link", + "dot-target-link", + "file-link", + "loop-link", + "missing-link", + "missing-parent-link", + "relative-link", + "space-target-link", + ], ); assert.throws( () => files.readInto(widePath("locked-\udfff"), Buffer.alloc(1)), @@ -186,6 +207,75 @@ function worker(root: string): Record { win32.join(root, "parent-\ud800"), ); assert(files.stat(widePath(".")).isDirectory()); + assert.deepEqual( + files.readlink(widePath("relative-link")), + widePath(names[0]!), + ); + assert.deepEqual( + files.readFile(widePath("relative-link")), + Buffer.from("sentinel-0"), + ); + samePath( + files.realpath(widePath("missing-link"), false), + win32.join(cwd, "missing-\udfff"), + ); + assert.equal( + pathText( + files.realpath( + widePath(`${win32.toNamespacedPath(cwd)}\\missing-parent-link`), + false, + ), + ).toLowerCase(), + win32.toNamespacedPath(win32.join(root, "missing-\udfff")).toLowerCase(), + ); + assert.throws(() => files.realpath(widePath("loop-link"), false), { + code: "ELOOP", + }); + for (const siblingExists of [true, false]) { + if (!siblingExists) files.unlink(widePath("missing-tail")); + for (const [link, target] of [ + ["dot-target-link", "missing-tail."], + ["space-target-link", "missing-tail "], + ] as const) { + assert.deepEqual(files.readlink(widePath(link)), widePath(target)); + const resolved = files.realpath(widePath(link), false); + samePath(resolved, win32.join(cwd, target)); + assert.throws(() => files.stat(resolved), { code: "ENOENT" }); + files.writeFile(resolved, Buffer.from("literal link target")); + assert.equal( + files.readFile(widePath(link)).toString(), + "literal link target", + ); + files.unlink(resolved); + if (siblingExists) + assert.equal( + files.readFile(widePath("missing-tail")).toString(), + "ordinary sibling", + ); + else + assert.throws(() => files.stat(widePath("missing-tail")), { + code: "ENOENT", + }); + } + } + const streamFile = win32.join(cwd, "a"); + files.writeFile(widePath(streamFile), Buffer.from("base file")); + for (const parent of [cwd, win32.toNamespacedPath(cwd)]) { + const stream = `${parent}\\a:stream`; + const resolved = files.realpath(widePath(stream), false); + samePath(resolved, stream); + files.writeFile(resolved, Buffer.from("stream payload"), true); + assert.equal(files.readFile(widePath(stream)).toString(), "stream payload"); + files.unlink(resolved); + assert.equal(files.readFile(widePath(streamFile)).toString(), "base file"); + } + files.unlink(widePath(streamFile)); + const missingDeepPath = `${win32.toNamespacedPath(cwd)}\\${"a\\".repeat(8_000)}missing`; + assert.equal( + pathText(files.realpath(widePath(missingDeepPath), false)), + missingDeepPath, + ); + assert.notEqual(native.windowsReadLink(widePath("empty")).error, 0); const bounded = Buffer.alloc(4); assert.equal(files.readInto(widePath(names[0]!), bounded), 4); assert.equal(bounded.toString(), "sent"); @@ -197,12 +287,24 @@ function worker(root: string): Record { Buffer.from("replacement output untouched"), ); files.writeFile(rawOutput, Buffer.from("a longer initial output")); + files.writeFile(rawOutput, Buffer.alloc(128 * 1024 + 1, 7)); + assert.deepEqual(files.readFile(rawOutput), Buffer.alloc(128 * 1024 + 1, 7)); files.writeFile(rawOutput, Buffer.from("short")); const contents = Buffer.alloc(64); assert.equal(files.readInto(rawOutput, contents), 5); assert.equal(contents.subarray(0, 5).toString(), "short"); files.writeFile(rawOutput, Buffer.alloc(0)); assert.equal(files.readInto(rawOutput, contents), 0); + assert.throws(() => + files.writeFile(rawOutput, Buffer.from("no replacement"), true), + ); + assert.equal(files.readFile(rawOutput).length, 0); + const renamed = widePath("renamed-\udc80"); + files.writeFile(renamed, Buffer.from("previous destination")); + files.rename(rawOutput, renamed); + assert.equal(files.readFile(renamed).length, 0); + files.unlink(renamed); + assert.throws(() => files.stat(renamed), { code: "ENOENT" }); const replacementLength = files.readInto(replacementOutput, contents); assert.equal( contents.subarray(0, replacementLength).toString(), @@ -271,6 +373,7 @@ function worker(root: string): Record { assert.throws(() => native.windowsEnvironment(malformed)); assert.throws(() => native.windowsAbsolutePath(malformed)); assert.throws(() => native.windowsDirectoryEntries(malformed)); + assert.throws(() => native.windowsReadLink(malformed)); } return { rawArgumentsAndCrtQuoting: true, diff --git a/plugins/codex-security/native/src/windows.rs b/plugins/codex-security/native/src/windows.rs index 1ab3485e3..820e3a92d 100644 --- a/plugins/codex-security/native/src/windows.rs +++ b/plugins/codex-security/native/src/windows.rs @@ -169,6 +169,23 @@ pub fn windows_directory_entries(path: Buffer) -> napi::Result napi::Result { + Ok(match std::fs::read_link(os_string(path)?) { + Ok(target) => BufferResult { + error: 0, + value: wide_bytes(target.as_os_str().encode_wide()), + }, + Err(error) => BufferResult { + error: error + .raw_os_error() + .ok_or_else(|| napi::Error::from_reason(error.to_string()))? + as u32, + value: Vec::new().into(), + }, + }) +} + fn io_range(buffer: &Buffer, offset: f64, length: f64) -> napi::Result<(usize, u32)> { if !offset.is_finite() || !length.is_finite() diff --git a/plugins/codex-security/native/windows-binding.mts b/plugins/codex-security/native/windows-binding.mts index 7b90d458b..61c346fff 100644 --- a/plugins/codex-security/native/windows-binding.mts +++ b/plugins/codex-security/native/windows-binding.mts @@ -36,6 +36,7 @@ export interface WindowsBinding { ): WindowsResult< { name: Buffer; isDirectory: boolean; isSymbolicLink: boolean }[] >; + windowsReadLink(path: Buffer): WindowsResult; openWindowsFile( path: Buffer, access: number, diff --git a/plugins/codex-security/native/windows-files.mts b/plugins/codex-security/native/windows-files.mts index 37f42ca78..69559fc88 100644 --- a/plugins/codex-security/native/windows-files.mts +++ b/plugins/codex-security/native/windows-files.mts @@ -5,6 +5,39 @@ import { windowsFlags as flags } from "./windows-flags.mjs"; export const widePath = (path: string): Buffer => Buffer.from(path, "utf16le"); export const pathText = (path: Buffer): string => path.toString("utf16le"); +export 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), + ]; +} + +export 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 windowsFileSystem(native: WindowsBinding) { function check(error: number, path: Buffer): void { if (error === 0) return; @@ -49,7 +82,7 @@ export function windowsFileSystem(native: WindowsBinding) { access, flags.FILE_SHARE_READ | flags.FILE_SHARE_WRITE | flags.FILE_SHARE_DELETE, disposition, - flags.FILE_FLAG_BACKUP_SEMANTICS | + (access === flags.GENERIC_READ ? 0 : flags.FILE_FLAG_BACKUP_SEMANTICS) | (follow ? 0 : flags.FILE_FLAG_OPEN_REPARSE_POINT), ); check(result.error, path); @@ -67,24 +100,96 @@ export function windowsFileSystem(native: WindowsBinding) { } } - function realpath(path: Buffer): Buffer { - let normalizedText: string; - if (pathText(path).startsWith("\\\\?\\")) { - // Verbatim paths bypass Win32 dot parsing; normalize only below their root. - const text = pathText(path).replaceAll("/", "\\"); - const root = - /^\\\\\?\\(?:UNC\\[^\\]+\\[^\\]+(?:\\|$)|[^\\]+\\)/iu.exec(text)?.[0] ?? - win32.parse(text).root; - normalizedText = - root + - win32.join("\\", text.slice(root.length)).slice(1).replace(/\\+$/u, ""); - } else { - const text = pathText(absolute(path)); - const root = win32.parse(text).root; - normalizedText = root + text.slice(root.length).replace(/\\+$/u, ""); + function readlink(path: Buffer): Buffer { + const result = native.windowsReadLink(operationPath(path)); + check(result.error, path); + return result.value; + } + + function realpath(path: Buffer, strict = true): Buffer { + function normalize(value: Buffer): Buffer { + let normalizedText: string; + if (pathText(value).startsWith("\\\\?\\")) { + // Verbatim paths bypass Win32 dot parsing; normalize only below their root. + const text = pathText(value).replaceAll("/", "\\"); + const root = + /^\\\\\?\\(?:UNC\\[^\\]+\\[^\\]+(?:\\|$)|[^\\]+\\)/iu.exec( + text, + )?.[0] ?? win32.parse(text).root; + normalizedText = + root + + win32 + .join("\\", text.slice(root.length)) + .slice(1) + .replace(/\\+$/u, ""); + } else { + const text = pathText(absolute(value)); + const root = win32.parse(text).root; + normalizedText = root + text.slice(root.length).replace(/\\+$/u, ""); + } + return widePath(normalizedText); } - const normalized = widePath(normalizedText); - const resolved = finalPath(normalized); + if (win32.normalize(pathText(path)).toLowerCase() === "nul") + return widePath("\\\\.\\NUL"); + if (!win32.isAbsolute(pathText(path))) + path = widePath( + windowsJoin(pathText(absolute(widePath("."))), pathText(path)), + ); + const normalized = normalize(path); + const seen = new Set(); + let initialError: number | undefined; + function resolveMissing(value: Buffer): Buffer { + const tail: string[] = []; + while (true) { + try { + value = finalPath(value); + break; + } catch (error) { + if (strict) throw error; + const winerror = (error as { winerror?: number }).winerror; + // Match pathlib's non-strict Windows resolution errors. + if ( + ![ + 1, 2, 3, 5, 21, 32, 50, 53, 65, 67, 87, 123, 161, 1920, 1921, + ].includes(winerror ?? 0) + ) + throw error; + initialError ??= winerror; + // Unicode lowercasing can merge distinct Windows filenames. + const key = pathText(value); + if ((error as { code?: string }).code === "ELOOP" || seen.has(key)) { + check(1921, value); + } + seen.add(key); + const parent = widePath(win32.dirname(pathText(value))); + if (parent.equals(value)) break; + let target: Buffer | undefined; + try { + target = readlink(value); + } catch { + // Missing and ordinary entries have no link target to follow. + } + if (target !== undefined) { + // Native link targets retain literal trailing dots and spaces. + value = normalize( + widePath( + win32.toNamespacedPath( + windowsJoin(pathText(parent), pathText(target)), + ), + ), + ); + continue; + } + tail.push(win32.basename(pathText(value))); + value = parent; + } + } + if (tail.length === 0) return value; + const base = pathText(value).replace(/\\$/u, ""); + // These are filename components; a:stream must not become drive A. + return widePath(`${base}\\${tail.reverse().join("\\")}`); + } + const resolved = resolveMissing(absolute(normalized)); if (pathText(normalized).startsWith("\\\\?\\")) return resolved; const text = pathText(resolved); const shortened = text.startsWith("\\\\?\\UNC\\") @@ -96,8 +201,14 @@ export function windowsFileSystem(native: WindowsBinding) { const candidate = widePath(shortened); try { if (finalPath(candidate).equals(resolved)) return candidate; - } catch { + } catch (error) { // Extended paths can be valid when their ordinary spelling is not. + if ( + !strict && + (error as { winerror?: number }).winerror === initialError && + operationPath(candidate).equals(resolved) + ) + return candidate; } return resolved; } @@ -174,28 +285,72 @@ export function windowsFileSystem(native: WindowsBinding) { return length; } - function writeFile(path: Buffer, buffer: Buffer): void { - const handle = open(path, flags.GENERIC_WRITE, flags.CREATE_ALWAYS); - let offset = 0; + function readFile(path: Buffer): Buffer { + const handle = open(path, flags.GENERIC_READ); + const chunks: Buffer[] = []; try { - while (offset < buffer.length) { - const result = handle.write( - buffer, - offset, - Math.min(buffer.length - offset, 0xffffffff), - ); + while (true) { + const chunk = Buffer.alloc(64 * 1024); + const result = handle.read(chunk, 0, chunk.length); check(result.error, path); - if (result.value === 0) - throw new Error( - `Windows file write made no progress: ${pathText(path)}`, + if (result.value === 0) return Buffer.concat(chunks); + chunks.push(chunk.subarray(0, result.value)); + } + } finally { + check(handle.close(), path); + } + } + + function writeFile( + path: Buffer, + data: Buffer | Iterable, + exclusive = false, + ): void { + const handle = open( + path, + flags.GENERIC_WRITE, + exclusive ? flags.CREATE_NEW : flags.CREATE_ALWAYS, + ); + try { + for (const buffer of Buffer.isBuffer(data) ? [data] : data) { + let offset = 0; + while (offset < buffer.length) { + const result = handle.write( + buffer, + offset, + Math.min(buffer.length - offset, 0xffffffff), ); - offset += result.value; + check(result.error, path); + if (result.value === 0) + throw new Error( + `Windows file write made no progress: ${pathText(path)}`, + ); + offset += result.value; + } } } finally { check(handle.close(), path); } } + function rename(source: Buffer, destination: Buffer): void { + const handle = open(source, flags.DELETE, flags.OPEN_EXISTING, false); + try { + check(handle.rename(operationPath(destination), true), destination); + } finally { + check(handle.close(), source); + } + } + + function unlink(path: Buffer): void { + const handle = open(path, flags.DELETE, flags.OPEN_EXISTING, false); + try { + check(handle.setDisposition(true), path); + } finally { + check(handle.close(), path); + } + } + return { absolute, realpath, @@ -203,7 +358,11 @@ export function windowsFileSystem(native: WindowsBinding) { identity, entriesWithTypes, mkdir, + readlink, readInto, + readFile, writeFile, + rename, + unlink, }; } diff --git a/plugins/codex-security/native/windows-files.test.mts b/plugins/codex-security/native/windows-files.test.mts index 29291731a..2ad1492db 100644 --- a/plugins/codex-security/native/windows-files.test.mts +++ b/plugins/codex-security/native/windows-files.test.mts @@ -43,14 +43,12 @@ for (const [input, absolute] of [ ["C:\\file\\", "C:\\file\\"], ] as const) { test(`ordinary realpath uses native absolute resolution: ${JSON.stringify(input)}`, () => { - let absoluteCalls = 0; const native = { windowsAbsolutePath(path: Buffer) { - if (absoluteCalls++ === 0) { - assert.equal(pathText(path), input); - return { error: 0, value: widePath(absolute) }; - } - return { error: 0, value: path }; + return { + error: 0, + value: widePath(win32.resolve("C:\\parent\\child", pathText(path))), + }; }, openWindowsFile(path: Buffer) { const root = win32.parse(absolute).root; @@ -65,3 +63,150 @@ for (const [input, absolute] of [ ); }); } + +for (const share of ["\\\\server\\share", "//server/share"]) { + test(`realpath resolves the UNC share root from a directory on that share: ${share}`, () => { + const native = { + windowsAbsolutePath(path: Buffer) { + return { + error: 0, + value: widePath( + win32.resolve("\\\\server\\share\\nested", pathText(path)), + ), + }; + }, + openWindowsFile(path: Buffer) { + assert.equal(pathText(path), "\\\\?\\UNC\\server\\share\\"); + throw opened; + }, + } as unknown as WindowsBinding; + assert.throws( + () => windowsFileSystem(native).realpath(widePath(share)), + (error) => error === opened, + ); + }); +} + +test("non-strict realpath resolves a whitespace-only relative path from cwd", () => { + const native = { + windowsAbsolutePath(path: Buffer) { + return pathText(path).trim() === "" + ? { error: 123, value: Buffer.alloc(0) } + : { + error: 0, + value: widePath( + win32.resolve("C:\\work", pathText(path).replace(/ +$/u, "")), + ), + }; + }, + windowsReadLink: () => ({ error: 2, value: Buffer.alloc(0) }), + openWindowsFile(path: Buffer) { + return pathText(path) === "\\\\?\\C:\\work" + ? { + error: 0, + handle: { finalPath: () => ({ error: 0, path }), close: () => 0 }, + } + : { error: 2, handle: null }; + }, + } as unknown as WindowsBinding; + assert.equal( + pathText(windowsFileSystem(native).realpath(widePath(" "), false)), + "C:\\work", + ); +}); + +for (const target of ["missing.", "missing "]) { + for (const siblingExists of [true, false]) { + test(`dangling link target ${JSON.stringify(target)} with ordinary sibling present=${siblingExists}`, () => { + const native = { + windowsAbsolutePath(path: Buffer) { + const text = pathText(path); + return { + error: 0, + value: widePath( + text.startsWith("\\\\?\\") ? text : text.replace(/[. ]+$/u, ""), + ), + }; + }, + windowsReadLink(path: Buffer) { + return pathText(path) === "\\\\?\\C:\\links\\link" + ? { error: 0, value: widePath(target) } + : { error: 4390, value: Buffer.alloc(0) }; + }, + openWindowsFile(path: Buffer) { + if ( + ![ + "\\\\?\\C:\\links", + ...(siblingExists ? ["\\\\?\\C:\\links\\missing"] : []), + ].includes(pathText(path)) + ) + return { error: 2, handle: null }; + return { + error: 0, + handle: { + finalPath: () => ({ error: 0, path }), + close: () => 0, + }, + }; + }, + } as unknown as WindowsBinding; + assert.equal( + pathText( + windowsFileSystem(native).realpath( + widePath("C:\\links\\link"), + false, + ), + ), + `\\\\?\\C:\\links\\${target}`, + ); + }); + } +} + +for (const parent of ["C:\\dir", "\\\\server\\share\\dir"]) { + for (const namespaced of [false, true]) { + const input = `${namespaced ? win32.toNamespacedPath(parent) : parent}\\a:stream`; + test(`non-strict resolution preserves the stream parent: ${JSON.stringify(input)}`, () => { + const native = { + windowsAbsolutePath: (path: Buffer) => ({ error: 0, value: path }), + windowsReadLink: () => ({ error: 2, value: Buffer.alloc(0) }), + openWindowsFile(path: Buffer) { + return pathText(path) === win32.toNamespacedPath(parent) + ? { + error: 0, + handle: { + finalPath: () => ({ error: 0, path }), + close: () => 0, + }, + } + : { error: 2, handle: null }; + }, + } as unknown as WindowsBinding; + assert.equal( + pathText(windowsFileSystem(native).realpath(widePath(input), false)), + input, + ); + }); + } +} + +test("non-strict resolution handles deeply nested missing paths", () => { + const root = "\\\\?\\C:\\"; + const input = root + "a\\".repeat(8_000) + "missing"; + const native = { + windowsAbsolutePath: (path: Buffer) => ({ error: 0, value: path }), + windowsReadLink: () => ({ error: 3, value: Buffer.alloc(0) }), + openWindowsFile(path: Buffer) { + return pathText(path) === root + ? { + error: 0, + handle: { finalPath: () => ({ error: 0, path }), close: () => 0 }, + } + : { error: 3, handle: null }; + }, + } as unknown as WindowsBinding; + assert.equal( + pathText(windowsFileSystem(native).realpath(widePath(input), false)), + input, + ); +}); diff --git a/plugins/codex-security/plugin-files.json b/plugins/codex-security/plugin-files.json index dbb67cc74..6ca58f7ab 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", @@ -104,7 +103,6 @@ "skills/assess-patch-risk/SKILL.md", "skills/assess-patch-risk/agents/openai.yaml", "skills/assess-patch-risk/references/risk-rubric.md", - "skills/assess-patch-risk/scripts/validate_patch_risk_assessment.py", "skills/deep-security-scan/SKILL.md", "skills/deep-security-scan/agents/openai.yaml", "skills/define-security-policy/SKILL.md", 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/assess-patch-risk/SKILL.md b/plugins/codex-security/skills/assess-patch-risk/SKILL.md index fdc6b23c5..840f7899e 100644 --- a/plugins/codex-security/skills/assess-patch-risk/SKILL.md +++ b/plugins/codex-security/skills/assess-patch-risk/SKILL.md @@ -60,15 +60,15 @@ Return both a concise Markdown report and a JSON object conforming to [`../../sc 7. top risk drivers, protective factors, and status-quo risk; and 8. unknowns plus the bounded evidence plan when held. -This skill lives at `/skills/assess-patch-risk/SKILL.md`, so `` is two directories up. Resolve `` to the configured Python interpreter (`"$PYTHON"` in POSIX shells or `& "$env:PYTHON"` in PowerShell), otherwise use `python` on Windows and `python3` on Unix-like hosts. +This skill lives at `/skills/assess-patch-risk/SKILL.md`, so `` is two directories up. Before returning the result, validate the JSON from any working directory with: ```text - /skills/assess-patch-risk/scripts/validate_patch_risk_assessment.py +/scripts/launch_codex_security_mcp --helper validate-patch-risk-assessment ``` -Pass `-` as `` to read the assessment from standard input without creating a file. +Use the `.cmd` launcher on Windows and prefix its quoted path with `&` in PowerShell. Pass `-` as `` to read the assessment from standard input without creating a file. The validator uses the bundled Node runtime helper and does not require Python. Correct structural or invariant errors by revisiting the evidence; never change a recommendation merely to make validation pass. Return the validated JSON in the response. Write it to disk only when the caller requests an artifact, and keep every assessment-created file outside the subject checkout and its Git directories. diff --git a/plugins/codex-security/skills/assess-patch-risk/scripts/validate_patch_risk_assessment.py b/plugins/codex-security/skills/assess-patch-risk/scripts/validate_patch_risk_assessment.py deleted file mode 100644 index cc4b67195..000000000 --- a/plugins/codex-security/skills/assess-patch-risk/scripts/validate_patch_risk_assessment.py +++ /dev/null @@ -1,163 +0,0 @@ -#!/usr/bin/env python3 -from __future__ import annotations - -import argparse -import importlib.util -import json -import sys -from pathlib import Path -from types import ModuleType -from typing import Any - -PLUGIN_ROOT = Path(__file__).resolve().parents[3] -SCHEMA_PATH = PLUGIN_ROOT / "schemas" / "patch-risk-assessment.schema.json" -NON_APPLICABLE = {"no_live_effect", "wrong_owner", "duplicate", "superseded"} - - -def load_scan_contract_validator() -> ModuleType: - script = PLUGIN_ROOT / "scripts" / "finalize_scan_contract.py" - spec = importlib.util.spec_from_file_location("codex_security_scan_contract", script) - if spec is None or spec.loader is None: - raise RuntimeError(f"could not load scan contract validator: {script}") - module = importlib.util.module_from_spec(spec) - spec.loader.exec_module(module) - return module - - -SCAN_CONTRACT = load_scan_contract_validator() - - -def parse_args() -> argparse.Namespace: - parser = argparse.ArgumentParser(description="Validate a patch-risk assessment.") - parser.add_argument("assessment", help="Assessment JSON path, or - for stdin.") - return parser.parse_args() - - -def reject_duplicate_keys(pairs: list[tuple[str, Any]]) -> dict[str, Any]: - value: dict[str, Any] = {} - for key, item in pairs: - if key in value: - raise ValueError(f"duplicate JSON object key: {key}") - value[key] = item - return value - - -def read_object(path: str) -> dict[str, Any]: - try: - text = sys.stdin.read() if path == "-" else Path(path).read_text(encoding="utf-8") - value = json.loads(text, object_pairs_hook=reject_duplicate_keys) - except (OSError, json.JSONDecodeError) as error: - raise ValueError(f"cannot read assessment: {error}") from error - if not isinstance(value, dict): - raise ValueError("assessment must be a JSON object") - return value - - -def schema_errors(value: dict[str, Any]) -> list[str]: - try: - SCAN_CONTRACT.validate_against_schema(value, SCHEMA_PATH) - except (OSError, ValueError, RecursionError) as error: - return [str(error)] - return [] - - -def semantic_errors(value: dict[str, Any]) -> list[str]: - recommendation = value["recommendation"] - workflow_label = value["workflowLabel"] - unknowns = value["unknowns"] - evidence_plan = value["evidencePlan"] - boundaries = value["materialBoundaries"] - applicability_status = value["applicability"]["status"] - affirmative_failure = ( - value["regressionLikelihood"]["rating"] == "critical" - or any(item["result"] == "contradicted" for item in boundaries) - or any(item["status"] == "failed" for item in value["validation"]) - ) - errors: list[str] = [] - - if recommendation == "merge": - if workflow_label not in {"auto_merge_candidate", "human_review_required"}: - errors.append("merge requires an auto-merge or human-review workflow label") - if value["applicability"]["status"] != "confirmed": - errors.append("merge requires confirmed applicability") - if any(item["decisionCritical"] for item in unknowns): - errors.append("merge cannot retain a decision-critical unknown") - if any(item["result"] != "supported" for item in boundaries): - errors.append("merge requires every material boundary to be supported") - if any(item["status"] == "failed" for item in value["validation"]): - errors.append("merge cannot retain a failed validation") - if evidence_plan: - errors.append("merge cannot retain an evidence plan") - elif workflow_label != recommendation: - errors.append("non-merge workflow label must match the recommendation") - - if recommendation == "hold_for_evidence": - if not any(item["decisionCritical"] for item in unknowns): - errors.append("hold_for_evidence requires a decision-critical unknown") - if not evidence_plan: - errors.append("hold_for_evidence requires a bounded evidence plan") - if affirmative_failure: - errors.append("hold_for_evidence cannot defer an established defect") - elif evidence_plan: - errors.append("only hold_for_evidence may include an evidence plan") - - if recommendation == "no_op": - if applicability_status not in NON_APPLICABLE: - errors.append("no_op requires an established non-applicable disposition") - if any(item["decisionCritical"] for item in unknowns): - errors.append("no_op cannot retain a decision-critical unknown") - elif applicability_status in NON_APPLICABLE: - errors.append("an established non-applicable disposition requires no_op") - - if recommendation in {"revise", "block"}: - if not affirmative_failure: - errors.append(f"{recommendation} requires affirmative failure evidence") - - if workflow_label == "auto_merge_candidate": - auto_merge_requirements = { - "impact.rating": value["impact"]["rating"] == "low", - "regressionLikelihood.rating": value["regressionLikelihood"]["rating"] == "low", - "regressionProtection.rating": value["regressionProtection"]["rating"] == "strong", - "regressionProtection.exactHeadChecksPassed": value["regressionProtection"][ - "exactHeadChecksPassed" - ], - "recoverability.rating": value["recoverability"]["rating"] == "easy", - "confidence.rating": value["confidence"]["rating"] == "high", - "applicability.status": value["applicability"]["status"] == "confirmed", - "affectedRuntimeRoots": bool(value["affectedRuntimeRoots"]), - "statusQuoRisk.rating": value["statusQuoRisk"]["rating"] != "unknown", - "autoMergeExclusions": not value["autoMergeExclusions"], - "unknowns": not unknowns, - "validation": all(item["status"] == "passed" for item in value["validation"]), - } - for field, passed in auto_merge_requirements.items(): - if not passed: - errors.append(f"auto_merge_candidate gate failed: {field}") - - return errors - - -def validate(value: dict[str, Any]) -> list[str]: - errors = schema_errors(value) - if errors: - return errors - return semantic_errors(value) - - -def main() -> int: - args = parse_args() - try: - value = read_object(args.assessment) - errors = validate(value) - except ValueError as error: - print(error, file=sys.stderr) - return 1 - if errors: - for error in errors: - print(error, file=sys.stderr) - return 1 - return 0 - - -if __name__ == "__main__": - raise SystemExit(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/plugins/codex-security/tests/test_patch_risk_contract.py b/plugins/codex-security/tests/test_patch_risk_contract.py deleted file mode 100644 index 88b301e4d..000000000 --- a/plugins/codex-security/tests/test_patch_risk_contract.py +++ /dev/null @@ -1,233 +0,0 @@ -from __future__ import annotations - -import copy -import json -import subprocess -import sys -from pathlib import Path -from typing import Any - -from jsonschema import Draft202012Validator - -PLUGIN_ROOT = Path(__file__).resolve().parents[1] -SCHEMA_PATH = PLUGIN_ROOT / "schemas" / "patch-risk-assessment.schema.json" -VALIDATOR_PATH = ( - PLUGIN_ROOT / "skills" / "assess-patch-risk" / "scripts" / "validate_patch_risk_assessment.py" -) - - -def assessment() -> dict[str, Any]: - return { - "schemaVersion": 1, - "patch": { - "repository": "example/project", - "sourceType": "pull_request_diff", - "base": "a" * 40, - "head": "b" * 40, - "changedFiles": ["src/request.ts"], - "sha256": "c" * 64, - }, - "recommendation": "merge", - "workflowLabel": "human_review_required", - "impact": {"rating": "moderate", "rationale": "A bounded caller can fail."}, - "regressionLikelihood": { - "rating": "low", - "rationale": "The changed path and its caller are covered.", - }, - "regressionProtection": { - "rating": "strong", - "rationale": "Focused and integration checks passed at the exact head.", - "exactHeadChecksPassed": True, - }, - "recoverability": {"rating": "easy", "rationale": "A revert is isolated."}, - "confidence": {"rating": "high", "rationale": "Runtime callers are known."}, - "applicability": {"status": "confirmed", "rationale": "The path is deployed."}, - "statusQuoRisk": {"rating": "moderate", "rationale": "The defect remains."}, - "autoMergeExclusions": [], - "affectedRuntimeRoots": ["service.request"], - "materialBoundaries": [ - { - "id": "request-contract", - "invariant": "Supported requests retain their existing response contract.", - "runtimeRoot": "service.request", - "counterexample": "A supported request takes the changed branch.", - "legitimateControl": "A supported request takes the unchanged branch.", - "result": "supported", - } - ], - "validation": [ - { - "name": "focused request tests", - "status": "passed", - "protects": "Changed behavior through the production caller.", - } - ], - "unknowns": [], - "evidencePlan": [], - } - - -def validate(tmp_path: Path, payload: dict[str, Any]) -> subprocess.CompletedProcess[str]: - assessment_path = tmp_path / "assessment.json" - assessment_path.write_text(json.dumps(payload), encoding="utf-8") - return subprocess.run( - [sys.executable, str(VALIDATOR_PATH), str(assessment_path)], - cwd=PLUGIN_ROOT, - capture_output=True, - text=True, - check=False, - ) - - -def test_schema_is_valid_draft_2020_12() -> None: - schema = json.loads(SCHEMA_PATH.read_text(encoding="utf-8")) - Draft202012Validator.check_schema(schema) - - -def test_supported_human_review_merge_is_valid(tmp_path: Path) -> None: - assert validate(tmp_path, assessment()).returncode == 0 - - -def test_strict_low_risk_assessment_can_be_auto_merge_candidate(tmp_path: Path) -> None: - payload = assessment() - payload["workflowLabel"] = "auto_merge_candidate" - payload["impact"]["rating"] = "low" - - assert validate(tmp_path, payload).returncode == 0 - - -def test_auto_merge_rejects_non_low_impact(tmp_path: Path) -> None: - payload = assessment() - payload["workflowLabel"] = "auto_merge_candidate" - - assert validate(tmp_path, payload).returncode != 0 - - -def test_auto_merge_rejects_materially_excluded_change(tmp_path: Path) -> None: - payload = assessment() - payload["workflowLabel"] = "auto_merge_candidate" - payload["impact"]["rating"] = "low" - payload["autoMergeExclusions"] = ["public_contract"] - - assert validate(tmp_path, payload).returncode != 0 - - -def test_merge_rejects_decision_critical_unknown(tmp_path: Path) -> None: - payload = assessment() - payload["unknowns"] = [ - {"summary": "Deployment ownership is unresolved.", "decisionCritical": True} - ] - - assert validate(tmp_path, payload).returncode != 0 - - -def test_merge_rejects_unknown_applicability(tmp_path: Path) -> None: - payload = assessment() - payload["applicability"] = { - "status": "unknown", - "rationale": "The supported runtime owner is unresolved.", - } - - result = validate(tmp_path, payload) - - assert result.returncode != 0 - assert "merge requires confirmed applicability" in result.stderr - - -def test_auto_merge_requires_affected_runtime_root(tmp_path: Path) -> None: - payload = assessment() - payload["workflowLabel"] = "auto_merge_candidate" - payload["impact"]["rating"] = "low" - payload["affectedRuntimeRoots"] = [] - - result = validate(tmp_path, payload) - - assert result.returncode != 0 - assert "auto_merge_candidate gate failed: affectedRuntimeRoots" in result.stderr - - -def test_hold_requires_bounded_evidence_plan(tmp_path: Path) -> None: - payload = assessment() - payload["recommendation"] = "hold_for_evidence" - payload["workflowLabel"] = "hold_for_evidence" - payload["unknowns"] = [ - {"summary": "The rollout target is unavailable.", "decisionCritical": True} - ] - - assert validate(tmp_path, payload).returncode != 0 - - payload["evidencePlan"] = [ - { - "question": "Does the changed configuration own the rollout target?", - "action": "Inspect the checked-in deployment mapping.", - "outcomes": { - "supported": "merge", - "contradicted": "no_op", - "unavailable": "hold_for_evidence", - }, - } - ] - assert validate(tmp_path, payload).returncode == 0 - - -def test_no_op_requires_non_applicable_disposition(tmp_path: Path) -> None: - payload = assessment() - payload["recommendation"] = "no_op" - payload["workflowLabel"] = "no_op" - - assert validate(tmp_path, payload).returncode != 0 - - payload["applicability"] = { - "status": "superseded", - "rationale": "A narrower patch already landed.", - } - assert validate(tmp_path, payload).returncode == 0 - - -def test_no_op_rejects_decision_critical_unknown(tmp_path: Path) -> None: - payload = assessment() - payload["recommendation"] = "no_op" - payload["workflowLabel"] = "no_op" - payload["applicability"] = { - "status": "superseded", - "rationale": "A sibling patch may cover the affected runtime.", - } - payload["unknowns"] = [ - { - "summary": "Whether the sibling covers the runtime is unresolved.", - "decisionCritical": True, - } - ] - - result = validate(tmp_path, payload) - - assert result.returncode != 0 - assert "no_op cannot retain a decision-critical unknown" in result.stderr - - -def test_raw_worktree_is_not_a_supported_patch_source(tmp_path: Path) -> None: - payload = assessment() - payload["patch"]["sourceType"] = "raw_worktree" - - assert validate(tmp_path, payload).returncode != 0 - - -def test_non_merge_recommendation_requires_matching_workflow_label(tmp_path: Path) -> None: - payload = assessment() - payload["recommendation"] = "revise" - payload["validation"][0]["status"] = "failed" - - assert validate(tmp_path, payload).returncode != 0 - - payload["workflowLabel"] = "revise" - assert validate(tmp_path, payload).returncode == 0 - - -def test_validator_does_not_modify_input(tmp_path: Path) -> None: - payload = assessment() - original = copy.deepcopy(payload) - - result = validate(tmp_path, payload) - - assert result.returncode == 0 - assert payload == original diff --git a/sdk/typescript/src/cli.ts b/sdk/typescript/src/cli.ts index d73570ec4..01afe0a59 100644 --- a/sdk/typescript/src/cli.ts +++ b/sdk/typescript/src/cli.ts @@ -6024,7 +6024,7 @@ async function runSkill( ), "Assess the immutable patch artifact described by this JSON object:", JSON.stringify(options.patchArtifact), - `Validate the JSON assessment with ${JSON.stringify(join(plugin, "skills", skill, "scripts", "validate_patch_risk_assessment.py"))} as required by the skill.`, + `Validate the JSON assessment with ${JSON.stringify(join(plugin, "scripts", process.platform === "win32" ? "launch_codex_security_mcp.cmd" : "launch_codex_security_mcp"))} --helper validate-patch-risk-assessment as required by the skill.`, "Wrap only the concise Markdown report between these exact marker lines:", PATCH_RISK_SUMMARY_START, PATCH_RISK_SUMMARY_END, 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..70d75d302 --- /dev/null +++ b/sdk/typescript/tests-ts/candidate-normalizer.test.ts @@ -0,0 +1,895 @@ +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.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.5", + "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("row 1:"); + 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/cli-patch.test.ts b/sdk/typescript/tests-ts/cli-patch.test.ts index 7493298ba..4f02d0548 100644 --- a/sdk/typescript/tests-ts/cli-patch.test.ts +++ b/sdk/typescript/tests-ts/cli-patch.test.ts @@ -730,6 +730,14 @@ describe("scan and patch workflow", () => { expect(output.appServer?.prompt).toContain( "", ); + expect(output.appServer?.prompt).toContain( + "--helper validate-patch-risk-assessment ", + ); + expect(output.appServer?.prompt).toContain( + process.platform === "win32" + ? "launch_codex_security_mcp.cmd" + : "launch_codex_security_mcp", + ); const artifact = JSON.parse( output .appServer!.prompt.split("\n") 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/patch-risk-contract.test.ts b/sdk/typescript/tests-ts/patch-risk-contract.test.ts index 6f786a909..e71d3cad0 100644 --- a/sdk/typescript/tests-ts/patch-risk-contract.test.ts +++ b/sdk/typescript/tests-ts/patch-risk-contract.test.ts @@ -1,5 +1,5 @@ import { spawnSync } from "node:child_process"; -import { mkdtemp, readFile, rm } from "node:fs/promises"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import Ajv2020 from "ajv/dist/2020.js"; @@ -61,18 +61,8 @@ const schemaPath = join( "schemas", "patch-risk-assessment.schema.json", ); -const validatorPath = join( - PLUGIN_ROOT, - "skills", - "assess-patch-risk", - "scripts", - "validate_patch_risk_assessment.py", -); -const python = - process.env["PYTHON"] ?? - Bun.which("python3") ?? - Bun.which("python") ?? - Bun.which("py"); +const node = Bun.which("node")!; +const helper = join(PLUGIN_ROOT, "mcp", "helpers.mjs"); function assessment(): Assessment { return { @@ -132,13 +122,13 @@ function assessment(): Assessment { }; } -function validateText(input: string, cwd = PLUGIN_ROOT) { - expect(python).toBeDefined(); - expect(python).not.toBeNull(); - return spawnSync(python!, ["-I", "-B", "-S", validatorPath, "-"], { +function validateText(input: string | Buffer, cwd = PLUGIN_ROOT, args = ["-"]) { + return spawnSync(node, [helper, "validate-patch-risk-assessment", ...args], { cwd, encoding: "utf8", input, + env: { ...process.env, PATH: "", PYTHON: join(cwd, "unavailable-python") }, + maxBuffer: Infinity, }); } @@ -146,24 +136,8 @@ function validate(payload: Assessment) { return validateText(JSON.stringify(payload)); } -function validateWithSharedSchema(payload: Assessment) { - expect(python).toBeDefined(); - expect(python).not.toBeNull(); - const program = [ - "import json, pathlib, sys", - "sys.path.insert(0, sys.argv[1])", - "import finalize_scan_contract as finalizer", - "finalizer.validate_against_schema(json.load(sys.stdin), pathlib.Path(sys.argv[2]))", - ].join("\n"); - return spawnSync( - python!, - ["-I", "-B", "-S", "-c", program, join(PLUGIN_ROOT, "scripts"), schemaPath], - { encoding: "utf8", input: JSON.stringify(payload) }, - ); -} - describe("patch risk assessment contract", () => { - test("resolves the validator from the installed skill", async () => { + test("loads the installed helper without Python from another working directory", async () => { const outside = await mkdtemp(join(tmpdir(), "patch-risk-contract-")); try { const result = validateText(JSON.stringify(assessment()), outside); @@ -192,7 +166,7 @@ describe("patch risk assessment contract", () => { }); test("enforces the patch-risk schema through the shared validator", () => { - const valid = validateWithSharedSchema(assessment()); + const valid = validate(assessment()); expect(valid.status, valid.stderr).toBe(0); const duplicateChangedFiles = assessment(); @@ -200,15 +174,15 @@ describe("patch risk assessment contract", () => { "src/request.ts", "src/request.ts", ]; - expect(validateWithSharedSchema(duplicateChangedFiles).status).not.toBe(0); + expect(validate(duplicateChangedFiles).status).not.toBe(0); const emptyRationale = assessment(); emptyRationale.impact.rationale = ""; - expect(validateWithSharedSchema(emptyRationale).status).not.toBe(0); + expect(validate(emptyRationale).status).not.toBe(0); const duplicateItems = assessment(); duplicateItems.autoMergeExclusions = ["migration", "migration"]; - expect(validateWithSharedSchema(duplicateItems).status).not.toBe(0); + expect(validate(duplicateItems).status).not.toBe(0); const tooManyEvidenceSteps = assessment(); tooManyEvidenceSteps.evidencePlan = Array.from( @@ -219,7 +193,9 @@ describe("patch risk assessment contract", () => { outcomes: { supported: "merge", contradicted: "revise" }, }), ); - expect(validateWithSharedSchema(tooManyEvidenceSteps).status).not.toBe(0); + expect(validate(tooManyEvidenceSteps).stderr).toBe( + "patch-risk-assessment.schema.evidencePlan: array has too many items\n", + ); const incompleteOutcomes = assessment(); incompleteOutcomes.evidencePlan = [ @@ -229,7 +205,9 @@ describe("patch risk assessment contract", () => { outcomes: { supported: "merge" }, }, ]; - expect(validateWithSharedSchema(incompleteOutcomes).status).not.toBe(0); + expect(validate(incompleteOutcomes).stderr).toBe( + "patch-risk-assessment.schema.evidencePlan[0].outcomes: object has too few properties\n", + ); const emptyOutcome = assessment(); emptyOutcome.evidencePlan = [ @@ -239,10 +217,12 @@ describe("patch risk assessment contract", () => { outcomes: { supported: "", contradicted: "revise" }, }, ]; - expect(validateWithSharedSchema(emptyOutcome).status).not.toBe(0); + expect(validate(emptyOutcome).stderr).toBe( + "patch-risk-assessment.schema.evidencePlan[0].outcomes.supported: unsupported value ''\n", + ); }); - test("enforces the published schema without site packages", async () => { + test("enforces the published schema without Python", async () => { const schema = JSON.parse(await readFile(schemaPath, "utf8")); const validateSchema = new Ajv2020({ strict: false, @@ -284,7 +264,7 @@ describe("patch risk assessment contract", () => { } }); - test("accepts a supported human-review merge without site packages", () => { + test("accepts a supported human-review merge without Python", () => { const result = validate(assessment()); expect(result.status, result.stderr).toBe(0); }); @@ -425,4 +405,400 @@ describe("patch risk assessment contract", () => { "duplicate JSON object key: recommendation", ); }); + + test("preserves the ordered auto-merge gates", () => { + const cases: Array<[string, (value: Assessment) => void]> = [ + [ + "impact.rating", + (value) => { + value.impact.rating = "moderate"; + }, + ], + [ + "regressionLikelihood.rating", + (value) => { + value.regressionLikelihood.rating = "high"; + }, + ], + [ + "regressionProtection.rating", + (value) => { + value.regressionProtection.rating = "partial"; + }, + ], + [ + "regressionProtection.exactHeadChecksPassed", + (value) => { + value.regressionProtection.exactHeadChecksPassed = false; + }, + ], + [ + "recoverability.rating", + (value) => { + value.recoverability.rating = "managed"; + }, + ], + [ + "confidence.rating", + (value) => { + value.confidence.rating = "moderate"; + }, + ], + [ + "applicability.status", + (value) => { + value.applicability.status = "unknown"; + }, + ], + [ + "affectedRuntimeRoots", + (value) => { + value.affectedRuntimeRoots = []; + }, + ], + [ + "statusQuoRisk.rating", + (value) => { + value.statusQuoRisk.rating = "unknown"; + }, + ], + [ + "autoMergeExclusions", + (value) => { + value.autoMergeExclusions = ["public_contract"]; + }, + ], + [ + "unknowns", + (value) => { + value.unknowns = [ + { summary: "A non-critical detail.", decisionCritical: false }, + ]; + }, + ], + [ + "validation", + (value) => { + value.validation[0]!.status = "skipped"; + }, + ], + ]; + const payload = assessment(); + payload.workflowLabel = "auto_merge_candidate"; + payload.impact.rating = "low"; + expect(validate(payload).status).toBe(0); + for (const [field, mutate] of cases) { + const changed = structuredClone(payload); + mutate(changed); + const result = validate(changed); + expect(result.status).toBe(1); + expect(result.stderr).toContain( + `auto_merge_candidate gate failed: ${field}`, + ); + mutate(payload); + } + expect(validate(payload).stderr.trim().split("\n")).toEqual([ + "merge requires confirmed applicability", + ...cases.map(([field]) => `auto_merge_candidate gate failed: ${field}`), + ]); + }); + + test("keeps all merge errors in their established order", () => { + const payload = assessment(); + payload.workflowLabel = "block"; + payload.applicability.status = "wrong_owner"; + payload.unknowns = [ + { summary: "The owner is unknown.", decisionCritical: true }, + ]; + payload.materialBoundaries[0]!.result = "unresolved"; + payload.validation[0]!.status = "failed"; + payload.evidencePlan = [ + { + question: "Who owns this?", + action: "Inspect the mapping.", + outcomes: { owned: "revise", unowned: "no_op" }, + }, + ]; + const result = validate(payload); + expect(result.status).toBe(1); + expect(result.stdout).toBe(""); + expect(result.stderr.trim().split("\n")).toEqual([ + "merge requires an auto-merge or human-review workflow label", + "merge requires confirmed applicability", + "merge cannot retain a decision-critical unknown", + "merge requires every material boundary to be supported", + "merge cannot retain a failed validation", + "merge cannot retain an evidence plan", + "only hold_for_evidence may include an evidence plan", + "an established non-applicable disposition requires no_op", + ]); + delete (payload as Record)["patch"]; + expect(validate(payload).stderr).toBe( + "patch-risk-assessment.schema.patch: missing required schema property\n", + ); + }); + + test("requires matching non-merge labels and a settled no-op disposition", () => { + const payload = assessment(); + payload.recommendation = "revise"; + payload.validation[0]!.status = "failed"; + expect(validate(payload).stderr).toBe( + "non-merge workflow label must match the recommendation\n", + ); + payload.workflowLabel = "revise"; + expect(validate(payload).status).toBe(0); + for (const status of [ + "no_live_effect", + "wrong_owner", + "duplicate", + "superseded", + ]) { + const noOp = assessment(); + noOp.recommendation = noOp.workflowLabel = "no_op"; + noOp.applicability.status = status; + expect(validate(noOp).status).toBe(0); + noOp.unknowns = [ + { summary: "Coverage is unresolved.", decisionCritical: true }, + ]; + expect(validate(noOp).stderr).toBe( + "no_op cannot retain a decision-critical unknown\n", + ); + } + }); + + test("keeps each established failure out of an evidence hold", () => { + const failures: Array<(value: Assessment) => void> = [ + (value) => { + value.regressionLikelihood.rating = "critical"; + }, + (value) => { + value.materialBoundaries[0]!.result = "contradicted"; + }, + (value) => { + value.validation[0]!.status = "failed"; + }, + ]; + for (const fail of failures) { + const payload = assessment(); + payload.recommendation = payload.workflowLabel = "hold_for_evidence"; + payload.unknowns = [ + { summary: "A rollout detail.", decisionCritical: true }, + ]; + payload.evidencePlan = [ + { + question: "Which rollout?", + action: "Inspect deployment.", + outcomes: { owned: "merge", unowned: "no_op" }, + }, + ]; + fail(payload); + expect(validate(payload).stderr).toBe( + "hold_for_evidence cannot defer an established defect\n", + ); + payload.evidencePlan = []; + for (const recommendation of ["revise", "block"]) { + payload.recommendation = payload.workflowLabel = recommendation; + expect(validate(payload).status).toBe(0); + } + } + }); + + test("distinguishes booleans, exact integers, and floating-point values", () => { + const serialized = JSON.stringify(assessment()); + for (const token of ["1", "1.0", "1e0", "1.00000000000000000000001"]) { + const result = validateText( + serialized.replace('"schemaVersion":1', `"schemaVersion":${token}`), + ); + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toBe(""); + expect(result.stderr).toBe(""); + } + for (const token of [ + "true", + "false", + "0", + "-0.0", + "NaN", + "Infinity", + "-Infinity", + "1e9999", + "9007199254740993", + ]) { + const result = validateText( + serialized.replace('"schemaVersion":1', `"schemaVersion":${token}`), + ); + expect(result.status).toBe(1); + expect(result.stderr).toBe( + "patch-risk-assessment.schema.schemaVersion: expected 1\n", + ); + } + for (const [items, message] of [ + ["1,1.0", "unsupported value 1"], + ["true,1", "unsupported value True"], + [ + "9007199254740992,9007199254740993.0", + "unsupported value 9007199254740992", + ], + [ + "9007199254740993,9007199254740993.0", + "unsupported value 9007199254740993", + ], + ['{"x":1,"y":2},{"y":2.0,"x":1.0}', "unsupported value"], + ["NaN,NaN", "unsupported value nan"], + ]) { + const result = validateText( + serialized.replace( + '"autoMergeExclusions":[]', + `"autoMergeExclusions":[${items}]`, + ), + ); + expect(result.status).toBe(1); + expect(result.stderr).toContain(message!); + } + }); + + test("rejects malformed JSON and preserves duplicate and property order", () => { + for (const [text, message] of [ + ["\ufeff{}", "cannot read assessment: Unexpected UTF-8 BOM"], + ['{"x":1,}', "Expecting property name enclosed in double quotes"], + ['{"x":"\\uZZZZ"}', "cannot read assessment:"], + ['{"x":"\\q"}', "cannot read assessment:"], + ['"\\q', "cannot read assessment:"], + ['"\\u123', "cannot read assessment:"], + ['{"x":"line\n"}', "cannot read assessment:"], + ["{} false", "Extra data"], + ["[]", "assessment must be a JSON object"], + ['{"x":0,"x":1,"nested":{"y":0,"y":1}}', "duplicate JSON object key: y"], + ['{"x":0,"\\u0078":1}', "duplicate JSON object key: x"], + ['{"__proto__":0,"__proto__":1}', "duplicate JSON object key: __proto__"], + ]) { + const result = validateText(text!); + expect(result.status).toBe(1); + expect(result.stderr).toContain(message!); + } + const serialized = JSON.stringify(assessment()); + const reordered = `{"unexpected":0,"1":0,${serialized.slice(1)}`; + expect(validateText(reordered).stderr).toBe( + "patch-risk-assessment.schema.unexpected: unexpected schema property\n", + ); + expect(validateText(`{"\\ud800":0,${serialized.slice(1)}`).stderr).toBe( + "patch-risk-assessment.schema.\\ud800: unexpected schema property\n", + ); + const control = assessment(); + control.patch.sourceType = "can't\u00a0merge\n"; + expect(validate(control).stderr).toBe( + 'patch-risk-assessment.schema.patch.sourceType: unsupported value "can\'t\\xa0merge\\n"\n', + ); + control.patch.repository = "\ud800"; + control.patch.sourceType = "patch_file"; + expect(validate(control).status).toBe(0); + for (const value of [`${"c".repeat(64)}\n`, `${"c".repeat(64)}\r\n`]) { + control.patch.sha256 = value; + expect(validate(control).stderr).toContain( + "string does not match schema pattern", + ); + } + }); + + test("reads file and stdin inputs without rewriting the assessment", async () => { + const outside = await mkdtemp(join(tmpdir(), "patch-risk-files-")); + try { + const original = + JSON.stringify(assessment(), null, 2).replaceAll("\n", "\r\n") + "\r\n"; + for (const name of ["assessment with spaces.json", "-1", "- item", "-"]) { + const path = join(outside, name); + await writeFile(path, original); + const result = validateText("not stdin", outside, [ + name === "-" ? "./-" : name, + ]); + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toBe(""); + expect(result.stderr).toBe(""); + expect(await readFile(path, "utf8")).toBe(original); + expect(validateText("", outside, ["--", name]).status).toBe( + name === "-" ? 1 : 0, + ); + } + const file = join(outside, "assessment with spaces.json"); + if (process.platform !== "win32") + expect(validateText("", outside, [`${file}/.`]).status).toBe(0); + expect(validateText(original, outside).status).toBe(0); + if (process.platform !== "win32") { + const launched = spawnSync( + join(PLUGIN_ROOT, "scripts", "launch_codex_security_mcp"), + ["--helper", "validate-patch-risk-assessment", "-"], + { + cwd: outside, + input: original, + encoding: "utf8", + env: { ...process.env, CODEX_MCP_NODE_PATH: node }, + }, + ); + expect(launched.status, launched.stderr).toBe(0); + expect(launched.stdout).toBe(""); + expect(launched.stderr).toBe(""); + } + const invalid = join(outside, "invalid.json"); + const malformed = '{\r\n"x":1\r\n"y":2}'; + await writeFile(invalid, malformed); + expect(validateText(malformed).stderr).toContain( + "line 3 column 1 (char 10)", + ); + expect(validateText("", outside, [invalid]).stderr).toContain( + "line 3 column 1 (char 8)", + ); + await writeFile(invalid, Buffer.from([0xff])); + expect(validateText("", outside, [invalid]).status).toBe(1); + expect(validateText("", outside, ["missing.json"]).stderr).toContain( + "cannot read assessment:", + ); + } finally { + await rm(outside, { recursive: true, force: true }); + } + }); + + test.skipIf(process.platform === "win32")( + "keeps stdin surrogate escapes distinct from replacement characters", + () => { + const serialized = JSON.stringify(assessment()).replace( + '"changedFiles":["src/request.ts"]', + '"changedFiles":["RAW","\\ufffd"]', + ); + const [before, after] = serialized.split("RAW"); + const result = validateText( + Buffer.concat([ + Buffer.from(before!), + Buffer.from([0xff]), + Buffer.from(after!), + ]), + ); + expect(result.status, result.stderr).toBe(0); + }, + ); + + test("preserves help, positional arguments, and parser exit statuses", () => { + for (const args of [ + ["-h"], + ["--h"], + ["--he"], + ["-hfoo"], + ["--bad", "--help"], + ]) { + const result = validateText("", PLUGIN_ROOT, args); + expect(result.status).toBe(0); + expect(result.stdout).toContain("Validate a patch-risk assessment."); + } + for (const args of [ + [], + ["--"], + ["--bad"], + ["one", "two"], + ["--help=bad"], + ["-h=bad"], + ]) { + expect(validateText("", PLUGIN_ROOT, args).status).toBe(2); + } + expect(validateText("", PLUGIN_ROOT, ["--", "-h"]).status).toBe(1); + expect(validateText("", PLUGIN_ROOT, ["-.5"]).status).toBe(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), - })), + ), ); }, );