fix(security): harden scan runtime boundaries

This commit is contained in:
Kyle McCleary
2026-07-29 18:47:51 -07:00
parent 80bdfdcbd4
commit 9314e200fe
10 changed files with 174 additions and 43 deletions
@@ -6,10 +6,9 @@ import type { SecurityFinding } from "../security/contracts";
import { createSecurityResource } from "../security/resource-output";
import type { SecurityScanSummary } from "../security/store";
import { SecurityStore } from "../security/store";
import * as git from "../utils/git";
import type { InternalResource, InternalUrl, ProtocolHandler, ResolveContext, UrlCompletion } from "./types";
export type SecurityStoreResolver = (repositoryRoot: string) => Promise<SecurityStore>;
export type SecurityStoreResolver = (cwd: string, signal?: AbortSignal) => Promise<SecurityStore>;
export function isSecurityEnabled(): boolean {
if (!isSettingsInitialized()) return getDefault("security.enabled");
@@ -105,7 +104,7 @@ export class SecurityProtocolHandler implements ProtocolHandler {
readonly #enabled: () => boolean;
constructor(
resolveStore: SecurityStoreResolver = repositoryRoot => SecurityStore.open(repositoryRoot),
resolveStore: SecurityStoreResolver = (cwd, signal) => SecurityStore.openForCwd(cwd, { signal }),
enabled: () => boolean = isSecurityEnabled,
) {
this.#resolveStore = resolveStore;
@@ -113,9 +112,7 @@ export class SecurityProtocolHandler implements ProtocolHandler {
}
async #store(context?: ResolveContext): Promise<SecurityStore> {
const cwd = path.resolve(context?.cwd ?? process.cwd());
const repositoryRoot = (await git.repo.root(cwd, context?.signal)) ?? cwd;
return this.#resolveStore(repositoryRoot);
return this.#resolveStore(path.resolve(context?.cwd ?? process.cwd()), context?.signal);
}
async resolve(url: InternalUrl, context?: ResolveContext): Promise<InternalResource> {
@@ -90,7 +90,6 @@ export interface SecurityPreflightInput {
credentialId?: number;
model?: Model;
thinkingLevel?: string;
config?: Record<string, unknown>;
signal?: AbortSignal;
}
@@ -134,10 +133,14 @@ interface SecurityOperationRecord {
abortController?: AbortController;
}
function iso(now: () => Date): string {
function toIsoTimestamp(now: () => Date): string {
return now().toISOString();
}
function securityConfigSnapshot(settings: Settings): Record<string, boolean> {
return { securityEnabled: settings.get("security.enabled") };
}
function createOperationId(): string {
return `secop_${Bun.randomUUIDv7().replaceAll("-", "")}`;
}
@@ -322,7 +325,7 @@ export class SecurityCoordinator {
constructor(host: SecurityCoordinatorHost, dependencies: SecurityCoordinatorDependencies = {}) {
this.#host = host;
this.#createSession = dependencies.createSession ?? createDefaultSecuritySession;
this.#openStore = dependencies.openStore ?? (repositoryRoot => SecurityStore.open(repositoryRoot));
this.#openStore = dependencies.openStore ?? (cwd => SecurityStore.openForCwd(cwd));
this.#gitAdapter = dependencies.gitAdapter ?? DEFAULT_SECURITY_GIT_ADAPTER;
this.#now = dependencies.now ?? (() => new Date());
this.#createOperationId = dependencies.createOperationId ?? createOperationId;
@@ -350,7 +353,7 @@ export class SecurityCoordinator {
archiveExisting: input.archiveExisting,
model: modelRef,
account,
config: input.config ?? { securityEnabled: true },
config: securityConfigSnapshot(this.#host.settings),
workflowFingerprint: SECURITY_WORKFLOW_FINGERPRINT,
signal: input.signal,
},
@@ -370,14 +373,14 @@ export class SecurityCoordinator {
await assertSecurityScanPlanFresh(
plan,
{
config: { securityEnabled: true },
config: securityConfigSnapshot(this.#host.settings),
workflowFingerprint: SECURITY_WORKFLOW_FINGERPRINT,
},
this.#gitAdapter,
);
const operationId = this.#createOperationId();
const scanId = createSecurityScanId();
const createdAt = iso(this.#now);
const createdAt = toIsoTimestamp(this.#now);
const snapshot: SecurityOperationSnapshot = {
operationId,
planId: plan.id,
@@ -442,9 +445,9 @@ export class SecurityCoordinator {
return { ...record.snapshot };
}
async #update(record: SecurityOperationRecord, phase: SecurityOperationPhase, error?: string): Promise<void> {
#update(record: SecurityOperationRecord, phase: SecurityOperationPhase, error?: string): void {
record.snapshot.phase = phase;
record.snapshot.updatedAt = iso(this.#now);
record.snapshot.updatedAt = toIsoTimestamp(this.#now);
record.snapshot.error = error;
}
@@ -455,14 +458,14 @@ export class SecurityCoordinator {
signal: AbortSignal,
reportProgress?: (text: string) => Promise<void>,
): Promise<void> {
const startedAt = iso(this.#now);
const startedAt = toIsoTimestamp(this.#now);
let session: SecurityScanSession | undefined;
let publishedBundle: SecurityScanBundle | undefined;
await store.putBundle(initialBundle(store, plan, record.snapshot.scanId, startedAt));
try {
await store.putBundle(initialBundle(store, plan, record.snapshot.scanId, startedAt));
if (signal.aborted) throw signal.reason ?? new Error("Security scan cancelled");
await prepareSecurityOutputDirectory(plan.output, record.snapshot.scanId);
await this.#update(record, "preparing");
this.#update(record, "preparing");
await reportProgress?.("Preparing OMP-native security scan");
const activeModel = this.#host.activeModel;
const model =
@@ -483,7 +486,7 @@ export class SecurityCoordinator {
onPublished: async bundle => {
publishedBundle = bundle;
record.snapshot.findingCount = bundle.findings.length;
await this.#update(record, "publishing");
this.#update(record, "publishing");
},
});
session = await this.#createSession({
@@ -503,7 +506,7 @@ export class SecurityCoordinator {
signal.addEventListener("abort", abortSession, { once: true });
try {
if (signal.aborted) throw signal.reason ?? new Error("Security scan cancelled");
await this.#update(record, "reviewing");
this.#update(record, "reviewing");
await reportProgress?.("Reviewing repository with OMP security workers");
await session.prompt(requestText(plan), {
expandPromptTemplates: false,
@@ -517,19 +520,19 @@ export class SecurityCoordinator {
}
if (signal.aborted) throw signal.reason ?? new Error("Security scan cancelled");
if (publishedBundle) {
await this.#update(record, "completed");
this.#update(record, "completed");
await reportProgress?.(`Published ${publishedBundle.findings.length} security finding(s)`);
return;
}
const partial = initialBundle(store, plan, record.snapshot.scanId, startedAt, "partial");
partial.scan.completedAt = iso(this.#now);
partial.scan.completedAt = toIsoTimestamp(this.#now);
partial.scan.error = "The scan session ended without publishing a canonical result";
this.#update(record, "partial", partial.scan.error);
await store.putBundle(partial);
await this.#update(record, "partial", partial.scan.error);
} catch (error) {
if (publishedBundle) {
record.snapshot.findingCount = publishedBundle.findings.length;
await this.#update(record, "completed");
this.#update(record, "completed");
return;
}
const message = error instanceof Error ? error.message : String(error);
@@ -541,10 +544,10 @@ export class SecurityCoordinator {
startedAt,
cancelled ? "cancelled" : "failed",
);
terminal.scan.completedAt = iso(this.#now);
terminal.scan.completedAt = toIsoTimestamp(this.#now);
terminal.scan.error = message;
this.#update(record, cancelled ? "cancelled" : "failed", message);
await store.putBundle(terminal);
await this.#update(record, cancelled ? "cancelled" : "failed", message);
} finally {
await session?.dispose().catch(() => undefined);
}
@@ -81,7 +81,8 @@ function normalizeRelativePath(input: string): string {
if (slashed.includes("\0")) throw new Error(`Security scope path contains a null byte: ${input}`);
const rawSegments = slashed.split("/");
const normalized = path.posix.normalize(slashed).replace(/^\.\//, "").replace(/\/$/, "");
if (!normalized || normalized === ".") return "";
if (!slashed) return "";
if (normalized === ".") return ".";
if (
rawSegments.includes("..") ||
normalized.startsWith("../") ||
@@ -98,6 +99,15 @@ function normalizeScopePaths(values: readonly string[] | undefined): string[] {
return [...new Set((values ?? []).map(normalizeRelativePath))].sort();
}
function scopeContainsPath(candidate: string, normalizedPath: string): boolean {
return (
candidate === "" ||
candidate === "." ||
normalizedPath === candidate ||
normalizedPath.startsWith(`${candidate}/`)
);
}
function pathMatchesScope(
relativePath: string,
includePaths: readonly string[],
@@ -105,9 +115,8 @@ function pathMatchesScope(
): boolean {
const normalized = normalizeRelativePath(relativePath);
const included =
includePaths.length === 0 ||
includePaths.some(candidate => normalized === candidate || normalized.startsWith(`${candidate}/`));
const excluded = excludePaths.some(candidate => normalized === candidate || normalized.startsWith(`${candidate}/`));
includePaths.length === 0 || includePaths.some(candidate => scopeContainsPath(candidate, normalized));
const excluded = excludePaths.some(candidate => scopeContainsPath(candidate, normalized));
return included && !excluded;
}
@@ -169,6 +178,9 @@ async function normalizeTarget(
adapter: SecurityGitAdapter,
signal?: AbortSignal,
): Promise<SecurityTarget> {
if (request.kind === "scoped_path" && !request.includePaths?.some(value => value.trim().length > 0)) {
throw new Error("scoped_path security scans require at least one include path");
}
const includePaths = normalizeScopePaths(request.includePaths);
const excludePaths = normalizeScopePaths(request.excludePaths);
await validateScopePaths(repositoryRoot, includePaths);
@@ -2,6 +2,7 @@ import * as fs from "node:fs/promises";
import * as path from "node:path";
import { getSecurityProjectDir, isEnoent } from "@oh-my-pi/pi-utils";
import { withFileLock } from "../config/file-lock";
import * as git from "../utils/git";
import { compareSecurityLineage } from "./comparison";
import type {
SecurityComparisonReport,
@@ -19,6 +20,7 @@ import {
parseSecurityScanPlan,
securitySha256,
} from "./contracts";
import { exportSecurityBundleToSarif } from "./sarif";
const STORE_SCHEMA_VERSION = 1;
const PRIVATE_DIRECTORY_MODE = 0o700;
@@ -62,6 +64,7 @@ export interface SecurityScanSummary {
export interface SecurityStoreOptions {
stateRoot?: string;
signal?: AbortSignal;
}
async function ensurePrivateDirectory(directory: string): Promise<void> {
@@ -85,6 +88,7 @@ export async function writeSecurityFileAtomic(
}
const temporaryPath = `${filePath}.${process.pid}.${Bun.randomUUIDv7()}.tmp`;
try {
// Bun.write cannot create exclusively; `wx` keeps concurrent atomic writers from sharing a temp file.
await fs.writeFile(temporaryPath, content, { encoding: "utf-8", mode: PRIVATE_FILE_MODE, flag: "wx" });
if (process.platform !== "win32") await fs.chmod(temporaryPath, PRIVATE_FILE_MODE);
try {
@@ -136,6 +140,12 @@ export class SecurityStore {
return store;
}
static async openForCwd(cwd: string, options: SecurityStoreOptions = {}): Promise<SecurityStore> {
const resolvedCwd = path.resolve(cwd);
const repositoryRoot = (await git.repo.root(resolvedCwd, options.signal)) ?? resolvedCwd;
return SecurityStore.open(repositoryRoot, options);
}
get repositoryRoot(): string {
return this.#repositoryRoot;
}
@@ -336,6 +346,7 @@ export class SecurityStore {
if (disposition.actor !== undefined) canonicalDisposition.actor = disposition.actor;
const updated = { ...bundle.findings[index], disposition: canonicalDisposition };
bundle.findings[index] = parseSecurityFinding(updated);
if (bundle.sarif !== undefined) bundle.sarif = exportSecurityBundleToSarif(bundle);
await this.#putBundleUnlocked(bundle);
return bundle.findings[index];
});
@@ -156,12 +156,12 @@ async function showResource(runtime: SlashCommandRuntime, rest: string): Promise
async function importResults(runtime: SlashCommandRuntime, rest: string): Promise<void> {
const [source] = parseCommandArgs(rest);
if (!source) throw new Error("import requires a SARIF file or Codex Security bundle directory");
const store = await SecurityStore.openForCwd(runtime.cwd);
const absolute = path.resolve(runtime.cwd, source);
const stats = await fs.stat(absolute);
const bundle = stats.isDirectory()
? await importCodexSecurityBundle(absolute, { repositoryRoot: runtime.cwd })
: await importSarifFile(absolute, { repositoryRoot: runtime.cwd });
const store = await SecurityStore.open(runtime.cwd);
? await importCodexSecurityBundle(absolute, { repositoryRoot: store.repositoryRoot })
: await importSarifFile(absolute, { repositoryRoot: store.repositoryRoot });
await store.putBundle(bundle);
await runtime.output(`Imported ${bundle.findings.length} finding(s) as security scan ${bundle.scan.id}.`);
}
@@ -184,7 +184,7 @@ async function exportResults(runtime: SlashCommandRuntime, rest: string): Promis
} else throw new Error(`Unknown export option: ${token}`);
}
if (!outputPath) throw new Error("export requires --output <path>");
const store = await SecurityStore.open(runtime.cwd);
const store = await SecurityStore.openForCwd(runtime.cwd);
const bundle = await store.getBundle(scanIdFromInput(scanId));
if (!bundle) throw new Error(`Unknown security scan: ${scanId}`);
let content: string;
@@ -210,7 +210,7 @@ async function updateDisposition(runtime: SlashCommandRuntime, rest: string): Pr
if (!DISPOSITIONS.has(status as SecurityDispositionStatus)) throw new Error(`Unknown disposition: ${status}`);
const rationale = rationaleParts.join(" ").trim();
if (status !== "open" && !rationale) throw new Error(`${status} requires a rationale`);
const store = await SecurityStore.open(runtime.cwd);
const store = await SecurityStore.openForCwd(runtime.cwd);
const finding = await store.updateDisposition(scanId, findingId, {
status: status as SecurityDispositionStatus,
rationale: rationale || undefined,
@@ -265,7 +265,7 @@ export async function handleSecurityCommand(
return commandConsumed();
}
case "scans": {
const scans = await (await SecurityStore.open(runtime.cwd)).listScans();
const scans = await (await SecurityStore.openForCwd(runtime.cwd)).listScans();
await runtime.output(
scans.length === 0
? "No security scans are stored for this project."
@@ -289,7 +289,7 @@ export async function handleSecurityCommand(
case "compare": {
const [beforeScanId, afterScanId] = parseCommandArgs(rest);
if (!beforeScanId || !afterScanId) throw new Error("compare requires <before-scan-id> <after-scan-id>");
const report = await (await SecurityStore.open(runtime.cwd)).compare(beforeScanId, afterScanId);
const report = await (await SecurityStore.openForCwd(runtime.cwd)).compare(beforeScanId, afterScanId);
await runtime.output(JSON.stringify(report, null, 2));
return commandConsumed();
}
@@ -34,8 +34,12 @@ export interface SecurityScanToolDetails {
function targetFromParams(params: SecurityScanParams): SecurityTargetRequest {
const common = { includePaths: params.include_paths, excludePaths: params.exclude_paths };
switch (params.target_kind ?? "repository") {
case "scoped_path":
return { kind: "scoped_path", includePaths: params.include_paths ?? [], excludePaths: params.exclude_paths };
case "scoped_path": {
if (!params.include_paths?.some(value => value.trim().length > 0)) {
throw new ToolError("scoped_path security scans require at least one include path");
}
return { kind: "scoped_path", includePaths: params.include_paths, excludePaths: params.exclude_paths };
}
case "working_tree":
return { kind: "working_tree", ...common };
case "ref_diff":
@@ -45,11 +45,27 @@ afterEach(async () => {
describe("security://", () => {
test("both producers render through every stable URI level", async () => {
const router = InternalUrlRouter.instance();
const expectations: Record<string, { contentType: "application/json" | "text/markdown"; marker: string }> = {
"": { contentType: "text/markdown", marker: "# Security" },
"/manifest": { contentType: "application/json", marker: `"id"` },
"/findings": { contentType: "text/markdown", marker: "# Findings for" },
"/coverage": { contentType: "application/json", marker: `"mode"` },
"/report": { contentType: "text/markdown", marker: "#" },
"/sarif": { contentType: "application/json", marker: `"version"` },
"/provenance": { contentType: "application/json", marker: `"producer"` },
};
for (const scanId of ["secscan_codexfixture", "secscan_sariffixture"]) {
for (const suffix of ["", "/manifest", "/findings", "/coverage", "/report", "/sarif", "/provenance"]) {
for (const [suffix, expectation] of Object.entries(expectations)) {
const resource = await router.resolve(`security://scans/${scanId}${suffix}`, { cwd: repositoryRoot });
expect(resource.immutable).toBeTrue();
expect(resource.content.length).toBeGreaterThan(0);
expect(resource.contentType).toBe(expectation.contentType);
const marker =
suffix === "/report"
? scanId === "secscan_codexfixture"
? "# Codex Security"
: "# Imported SARIF"
: expectation.marker;
expect(resource.content).toContain(marker);
}
}
});
@@ -1,4 +1,4 @@
import { afterEach, beforeEach, describe, expect, test } from "bun:test";
import { afterEach, beforeEach, describe, expect, test, vi } from "bun:test";
import * as fs from "node:fs/promises";
import * as os from "node:os";
import * as path from "node:path";
@@ -55,6 +55,7 @@ beforeEach(async () => {
});
afterEach(async () => {
vi.restoreAllMocks();
unregisterCustomApis(MOCK_SOURCE_ID);
settings.cancelPendingSaves();
credentialStore?.close();
@@ -124,7 +125,6 @@ describe("native security coordinator", () => {
const terminal = await coordinator.wait(started.operationId);
expect(terminal.phase).toBe("completed");
expect(terminal.findingCount).toBe(1);
expect(mock.calls.length).toBeGreaterThan(0);
const bundle = await (await storeFactory()).getBundle(terminal.scanId);
expect(bundle?.scan.status).toBe("completed");
expect(bundle?.findings).toHaveLength(1);
@@ -136,6 +136,35 @@ describe("native security coordinator", () => {
expect(reopened.getSessionId()).toBeTruthy();
});
test("records a terminal failure when initial scan persistence fails", async () => {
const mock = createMockModel({ id: "security-mock", provider: "openai-codex" });
const store = await storeFactory();
const coordinator = new SecurityCoordinator(
{
cwd: repositoryRoot,
settings,
authStorage,
modelRegistry: new ModelRegistry(authStorage, path.join(temporaryRoot, "models.yml")),
activeModel: mock.model,
},
{
openStore: async () => store,
gitAdapter,
createSession: async () => {
throw new Error("session must not launch when persistence fails");
},
},
);
const plan = await coordinator.preflight({ credentialId, model: mock.model });
vi.spyOn(store, "putBundle").mockRejectedValue(new Error("security store unavailable"));
const started = await coordinator.start({ planId: plan.id });
await expect(coordinator.wait(started.operationId)).rejects.toThrow("security store unavailable");
expect(coordinator.status(started.operationId)).toMatchObject({
phase: "failed",
error: "security store unavailable",
});
});
test("cancellation before session launch has no inference side effects", async () => {
let sessionCreations = 0;
const mock = createMockModel({ id: "security-mock", provider: "openai-codex" });
@@ -2,7 +2,8 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test";
import * as fs from "node:fs/promises";
import * as os from "node:os";
import * as path from "node:path";
import { importSarifFile, SecurityStore } from "../../src/security";
import { pathToFileURL } from "node:url";
import { exportSecurityBundleToSarif, importSarif, importSarifFile, SecurityStore } from "../../src/security";
const FIXTURE = path.join(import.meta.dir, "..", "fixtures", "security", "generic-results.sarif");
let temporaryRoot = "";
@@ -65,5 +66,50 @@ describe("security history and dispositions", () => {
actor: "test-operator",
});
expect((await store.getFinding(bundle.scan.id, original.id))?.disposition).toEqual(updated.disposition);
const persisted = await store.getBundle(bundle.scan.id);
const persistedResult = (
persisted?.sarif?.runs as Array<{ results: Array<{ properties?: Record<string, unknown> }> }> | undefined
)?.[0]?.results[0];
expect(persistedResult?.properties?.disposition).toBe("false_positive");
});
test("SARIF disposition round-trips without changing its finding identity", async () => {
const bundle = await importSarif(
{
version: "2.1.0",
runs: [
{
tool: { driver: { name: "Fixture scanner" } },
results: [
{
ruleId: "fixture.rule",
message: { text: "fixture finding" },
properties: { disposition: "false_positive" },
},
],
},
],
},
{ repositoryRoot, createScanId: () => "secscan_sarifdisposition" },
);
const finding = bundle.findings[0];
if (!finding) throw new Error("expected imported finding");
expect(finding.disposition.status).toBe("false_positive");
const exported = exportSecurityBundleToSarif(bundle);
const result = (exported.runs as Array<{ results: Array<{ properties?: Record<string, unknown> }> }>)[0]
?.results[0];
expect(result?.properties?.disposition).toBe("false_positive");
expect(finding.id).toBe(bundle.scan.findingIds[0]);
});
test("SARIF base URI escapes repository path characters", async () => {
const specialRoot = path.join(temporaryRoot, "repo with #hash");
await fs.mkdir(specialRoot);
const bundle = await importSarif({ version: "2.1.0", runs: [] }, { repositoryRoot: specialRoot });
const exported = exportSecurityBundleToSarif(bundle);
const run = (exported.runs as Array<{ originalUriBaseIds: Record<string, { uri: string }> }>)[0];
expect(run?.originalUriBaseIds["%SRCROOT%"]?.uri).toBe(
pathToFileURL(`${await fs.realpath(specialRoot)}${path.sep}`).href,
);
});
});
@@ -214,6 +214,19 @@ describe("security preflight", () => {
await expect(plan()).rejects.toThrow("symbolic link");
});
test("a root-dot scoped target includes repository descendants", async () => {
const scoped = await plan({ kind: "scoped_path", includePaths: ["."] });
const repository = await plan();
expect(scoped.target.includePaths).toEqual(["."]);
expect(scoped.target.treeDigest).toBe(repository.target.treeDigest);
});
test("an empty scoped target is rejected before planning", async () => {
await expect(plan({ kind: "scoped_path", includePaths: [] })).rejects.toThrow(
"scoped_path security scans require at least one include path",
);
});
test("scope traversal is rejected", async () => {
for (const candidate of ["../outside", "src/../outside", "C:\\outside", "src\\..\\outside"]) {
await expect(plan({ kind: "scoped_path", includePaths: [candidate] })).rejects.toThrow("repository-relative");