From b708b3991551af46c002fe2f5a430d793b672ba4 Mon Sep 17 00:00:00 2001 From: Kyle McCleary Date: Wed, 29 Jul 2026 18:47:51 -0700 Subject: [PATCH] fix(security): harden native scan contracts --- .../coding-agent/scripts/security-compare.ts | 8 +- .../src/internal-urls/security-protocol.ts | 64 +++-- packages/coding-agent/src/sdk.ts | 4 +- packages/coding-agent/src/security/auth.ts | 10 +- .../coding-agent/src/security/comparison.ts | 11 +- .../src/security/contracts/validation.ts | 10 +- .../coding-agent/src/security/coordinator.ts | 89 ++++--- .../src/security/importers/codex-security.ts | 221 ++++++++++-------- .../src/security/importers/sarif.ts | 62 ++--- packages/coding-agent/src/security/index.ts | 2 +- .../coding-agent/src/security/preflight.ts | 68 ++++-- .../coding-agent/src/security/provenance.ts | 30 +-- .../coding-agent/src/security/publication.ts | 207 ++++++++-------- .../coding-agent/src/security/remediation.ts | 6 +- .../src/security/resource-output.ts | 2 +- packages/coding-agent/src/security/store.ts | 44 ++-- .../src/slash-commands/builtin-registry.ts | 2 +- .../src/slash-commands/helpers/security.ts | 16 +- packages/coding-agent/src/tools/index.ts | 4 +- .../coding-agent/src/tools/security-scan.ts | 17 +- .../internal-urls/security-protocol.test.ts | 44 +++- .../coding-agent/test/security/auth.test.ts | 12 +- .../test/security/comparison.test.ts | 2 +- .../test/security/contracts.test.ts | 16 +- .../test/security/coordinator.test.ts | 4 +- .../test/security/history.test.ts | 5 +- .../test/security/preflight.test.ts | 50 +++- .../test/security/publication.test.ts | 71 ++++-- .../test/security/remediation.test.ts | 5 +- .../test/security/slash-command.test.ts | 18 ++ .../test/task/structured-subagent.test.ts | 4 +- 31 files changed, 677 insertions(+), 431 deletions(-) diff --git a/packages/coding-agent/scripts/security-compare.ts b/packages/coding-agent/scripts/security-compare.ts index 2e13838cf..969d1f4ae 100755 --- a/packages/coding-agent/scripts/security-compare.ts +++ b/packages/coding-agent/scripts/security-compare.ts @@ -6,8 +6,12 @@ async function readBundle(directory: string) { const root = path.resolve(directory); const scan = JSON.parse(await Bun.file(path.join(root, "scan.json")).text()) as unknown; const findings = JSON.parse(await Bun.file(path.join(root, "findings.json")).text()) as unknown; - const report = await Bun.file(path.join(root, "report.md")).text().catch(() => undefined); - const sarifText = await Bun.file(path.join(root, "results.sarif")).text().catch(() => undefined); + const report = await Bun.file(path.join(root, "report.md")) + .text() + .catch(() => undefined); + const sarifText = await Bun.file(path.join(root, "results.sarif")) + .text() + .catch(() => undefined); return parseSecurityScanBundle({ scan, findings, diff --git a/packages/coding-agent/src/internal-urls/security-protocol.ts b/packages/coding-agent/src/internal-urls/security-protocol.ts index 1b37860eb..d9be6133e 100644 --- a/packages/coding-agent/src/internal-urls/security-protocol.ts +++ b/packages/coding-agent/src/internal-urls/security-protocol.ts @@ -1,11 +1,11 @@ import * as path from "node:path"; import { sanitizeText } from "@oh-my-pi/pi-utils"; -import { getDefault } from "../config/settings-schema"; import { isSettingsInitialized, settings } from "../config/settings"; -import { createSecurityResource } from "../security/resource-output"; -import { SecurityStore } from "../security/store"; -import type { SecurityScanSummary } from "../security/store"; +import { getDefault } from "../config/settings-schema"; 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"; @@ -20,6 +20,18 @@ export function isSecurityEnabled(): boolean { } } +function securityEnabledFromContext(context?: ResolveContext): boolean | undefined { + if (!context?.settings || typeof context.settings !== "object") return undefined; + try { + const get = Reflect.get(context.settings, "get"); + if (typeof get !== "function") return undefined; + const enabled = Reflect.apply(get, context.settings, ["security.enabled"]); + return typeof enabled === "boolean" ? enabled : undefined; + } catch { + return undefined; + } +} + const SECURITY_DISABLED_MESSAGE = "security:// is disabled. Enable it by setting `security.enabled = true` (Settings → Tools → Security)."; @@ -54,7 +66,9 @@ function formatFinding(finding: SecurityFinding): string { location.role ? ` (${sanitizeText(location.role)})` : "", ].join(""); }); - const evidence = finding.evidence.map(item => `- **${sanitizeText(item.label)}** — ${sanitizeText(item.explanation)}`); + const evidence = finding.evidence.map( + item => `- **${sanitizeText(item.label)}** — ${sanitizeText(item.explanation)}`, + ); return [ `# ${sanitizeText(finding.title)}`, "", @@ -105,21 +119,20 @@ export class SecurityProtocolHandler implements ProtocolHandler { } async resolve(url: InternalUrl, context?: ResolveContext): Promise { - if (!this.#enabled()) throw new SecurityDisabledError(); + if (!(securityEnabledFromContext(context) ?? this.#enabled())) throw new SecurityDisabledError(); const parts = splitSecurityPath(url); const store = await this.#store(context); if (parts.length === 0) { return createSecurityResource({ url: "security://", - content: - [ - "# Security", - "", - "OMP-owned software-security analysis resources. The namespace is read-only; use explicit security commands or tools for mutations.", - "", - "- `security://scans` — list scans", - "", - ].join("\n"), + content: [ + "# Security", + "", + "OMP-owned software-security analysis resources. The namespace is read-only; use explicit security commands or tools for mutations.", + "", + "- `security://scans` — list scans", + "", + ].join("\n"), contentType: "text/markdown", isDirectory: true, }); @@ -165,12 +178,11 @@ export class SecurityProtocolHandler implements ProtocolHandler { }); case "findings": { if (parts.length === 3) { - const listing = bundle.findings.map( - finding => - [ - `- \`${finding.id}\` **${finding.severity.level}** — ${sanitizeText(finding.title)}`, - ` (\`${sanitizeText(finding.ruleId)}\`)`, - ].join(""), + const listing = bundle.findings.map(finding => + [ + `- \`${finding.id}\` **${finding.severity.level}** — ${sanitizeText(finding.title)}`, + ` (\`${sanitizeText(finding.ruleId)}\`)`, + ].join(""), ); return createSecurityResource({ url: `security://scans/${scanId}/findings`, @@ -225,13 +237,17 @@ export class SecurityProtocolHandler implements ProtocolHandler { } async complete(query = "", context?: ResolveContext): Promise { - if (!this.#enabled()) return []; + if (!(securityEnabledFromContext(context) ?? this.#enabled())) return []; const store = await this.#store(context); const scans = await store.listScans(); const candidates: UrlCompletion[] = [{ value: "scans", label: "Scans", description: "Stored security scans" }]; for (const scan of scans.slice(0, 50)) { const prefix = `scans/${scan.id}`; - candidates.push({ value: prefix, label: scan.id, description: `${scan.status}; ${scan.findingCount} findings` }); + candidates.push({ + value: prefix, + label: scan.id, + description: `${scan.status}; ${scan.findingCount} findings`, + }); for (const child of ["manifest", "findings", "coverage", "report", "sarif", "provenance"]) { candidates.push({ value: `${prefix}/${child}`, label: `${scan.id}/${child}` }); } @@ -239,7 +255,7 @@ export class SecurityProtocolHandler implements ProtocolHandler { const normalizedQuery = query.trim().toLowerCase(); if (!normalizedQuery) return candidates; return candidates.filter(candidate => - [candidate.value, candidate.label, candidate.description ?? ""].some(value => + [candidate.value, candidate.label ?? "", candidate.description ?? ""].some(value => value.toLowerCase().includes(normalizedQuery), ), ); diff --git a/packages/coding-agent/src/sdk.ts b/packages/coding-agent/src/sdk.ts index ee66df888..9bff135bf 100644 --- a/packages/coding-agent/src/sdk.ts +++ b/packages/coding-agent/src/sdk.ts @@ -3317,7 +3317,9 @@ export async function createAgentSession(options: CreateAgentSessionOptions = {} void (async () => { try { const codexPrewarmApiKey = options.getApiKey - ? await resolveApiKeyOnce(options.getApiKey(codexModel)) + ? // `getApiKey` returns a value-or-promise union; unwrap the promise, + // then resolve the result if it is itself an ApiKeyResolver. + await resolveApiKeyOnce(await options.getApiKey(codexModel)) : await modelRegistry.getApiKey(codexModel, providerSessionId); if (!codexPrewarmApiKey) return; await logger.time("prewarmOpenAICodexResponses", prewarmOpenAICodexResponses, codexModel, { diff --git a/packages/coding-agent/src/security/auth.ts b/packages/coding-agent/src/security/auth.ts index fc96c5de6..e16a9eadd 100644 --- a/packages/coding-agent/src/security/auth.ts +++ b/packages/coding-agent/src/security/auth.ts @@ -10,9 +10,7 @@ export interface ExactSecurityOAuthOptions { function assertIdentityMatches(account: SecurityAccountRef, resolvedAccountId: string | undefined): void { if (account.accountId !== undefined && account.accountId !== resolvedAccountId) { - throw new Error( - `Security scan account mismatch: expected workspace ${account.accountId}, resolved ${resolvedAccountId}`, - ); + throw new Error("Security scan account mismatch: the pinned workspace identity changed during authentication"); } } @@ -40,12 +38,10 @@ export function createExactSecurityOAuthResolver( signal: context.signal, }); if (!resolution) { - throw new Error(`Security scan OAuth credential ${account.credentialId} is unavailable`); + throw new Error("The pinned security OAuth credential is unavailable"); } if (!resolution.ok) { - throw new Error( - `Security scan OAuth credential ${account.credentialId} could not be resolved: ${resolution.error}`, - ); + throw new Error("The pinned security OAuth credential could not be resolved"); } assertIdentityMatches(account, resolution.accountId); return resolution.accessToken; diff --git a/packages/coding-agent/src/security/comparison.ts b/packages/coding-agent/src/security/comparison.ts index 5d4900be4..51081e770 100644 --- a/packages/coding-agent/src/security/comparison.ts +++ b/packages/coding-agent/src/security/comparison.ts @@ -1,9 +1,4 @@ -import type { - SecurityComparisonReport, - SecurityFinding, - SecurityFindingMatch, - SecurityScanBundle, -} from "./contracts"; +import type { SecurityComparisonReport, SecurityFinding, SecurityFindingMatch, SecurityScanBundle } from "./contracts"; export interface SecurityDifferentialFindingMatch { referenceFindingId: string; @@ -68,7 +63,9 @@ export function compareSecurityProducers( continue; } const key = fallbackKey(referenceFinding); - const fallback = key ? candidateByFallback.get(key)?.find(finding => !usedCandidateIds.has(finding.id)) : undefined; + const fallback = key + ? candidateByFallback.get(key)?.find(finding => !usedCandidateIds.has(finding.id)) + : undefined; if (!fallback) continue; usedCandidateIds.add(fallback.id); matches.push({ diff --git a/packages/coding-agent/src/security/contracts/validation.ts b/packages/coding-agent/src/security/contracts/validation.ts index b37ede271..c4bf30188 100644 --- a/packages/coding-agent/src/security/contracts/validation.ts +++ b/packages/coding-agent/src/security/contracts/validation.ts @@ -1,10 +1,5 @@ import { type } from "arktype"; -import { - securityFindingSchema, - securityScanBundleSchema, - securityScanPlanSchema, - securityScanSchema, -} from "./schemas"; +import { securityFindingSchema, securityScanBundleSchema, securityScanPlanSchema, securityScanSchema } from "./schemas"; import type { SecurityFinding, SecurityScan, SecurityScanBundle, SecurityScanPlan } from "./types"; function schemaError(label: string, errors: type.errors): Error { @@ -43,7 +38,8 @@ export function parseSecurityScanBundle(value: unknown): SecurityScanBundle { if (!findingIds.has(findingId)) throw new Error(`Security scan references missing finding: ${findingId}`); } for (const findingId of findingIds) { - if (!referencedFindingIds.has(findingId)) throw new Error(`Security scan omits finding from manifest: ${findingId}`); + if (!referencedFindingIds.has(findingId)) + throw new Error(`Security scan omits finding from manifest: ${findingId}`); } for (const finding of bundle.findings) { if (finding.scanId !== bundle.scan.id) { diff --git a/packages/coding-agent/src/security/coordinator.ts b/packages/coding-agent/src/security/coordinator.ts index 980fc8990..b45294972 100644 --- a/packages/coding-agent/src/security/coordinator.ts +++ b/packages/coding-agent/src/security/coordinator.ts @@ -10,27 +10,28 @@ import securityReviewerPrompt from "../prompts/agents/security-reviewer.md" with import securityCoordinatorPrompt from "../prompts/security/scan-coordinator.md" with { type: "text" }; import securityRequestPrompt from "../prompts/security/scan-request.md" with { type: "text" }; import securityPublishDescription from "../prompts/tools/security-publish.md" with { type: "text" }; +import { createAgentSession } from "../sdk"; import type { AgentSession } from "../session/agent-session"; import type { AuthStorage } from "../session/auth-storage"; import { SessionManager } from "../session/session-manager"; -import { createAgentSession } from "../sdk"; import { createExactSecurityOAuthResolver } from "./auth"; -import { createSecurityScanId } from "./contracts"; import type { SecurityAccountRef, SecurityCoverage, + SecurityModelRef, SecurityScan, SecurityScanBundle, SecurityScanPlan, SecurityTargetKind, } from "./contracts"; +import { createSecurityScanId } from "./contracts"; +import type { SecurityGitAdapter, SecurityTargetRequest } from "./preflight"; import { assertSecurityScanPlanFresh, createSecurityScanPlan, DEFAULT_SECURITY_GIT_ADAPTER, prepareSecurityOutputDirectory, } from "./preflight"; -import type { SecurityGitAdapter, SecurityTargetRequest } from "./preflight"; import { createNativeSecurityProducer, createNativeSecurityProvenance, @@ -213,7 +214,7 @@ function resolveAccount( const selected = requestedCredentialId !== undefined ? accounts.find(account => account.credentialId === requestedCredentialId) - : accounts.find(account => account.active) ?? (accounts.length === 1 ? accounts[0] : undefined); + : (accounts.find(account => account.active) ?? (accounts.length === 1 ? accounts[0] : undefined)); if (!selected) { if (accounts.length === 0) { throw new Error(`Security scans require a stored OAuth account for ${model.provider}`); @@ -225,14 +226,15 @@ function resolveAccount( `Multiple OAuth accounts are available for ${model.provider}; supply credentialId to pin one exact account`, ); } - return { + const account: SecurityAccountRef = { provider: model.provider, credentialId: selected.credentialId, - accountId: selected.accountId, - email: selected.email, - organizationId: selected.orgId, - organizationName: selected.orgName, }; + if (selected.accountId !== undefined) account.accountId = selected.accountId; + if (selected.email !== undefined) account.email = selected.email; + if (selected.orgId !== undefined) account.organizationId = selected.orgId; + if (selected.orgName !== undefined) account.organizationName = selected.orgName; + return account; } async function createDefaultSecuritySession(input: SecurityScanSessionFactoryInput): Promise { @@ -280,17 +282,20 @@ async function createDefaultSecuritySession(input: SecurityScanSessionFactoryInp } function requestText(plan: SecurityScanPlan): string { - return prompt.render(securityRequestPrompt, { - repositoryRoot: plan.repositoryRoot, - targetKind: plan.target.kind, - revision: plan.target.revision ?? "", - baseRevision: plan.target.baseRevision ?? "", - headRevision: plan.target.headRevision ?? "", - includePaths: plan.target.includePaths.length > 0 ? plan.target.includePaths.join(", ") : "all in-scope paths", - excludePaths: plan.target.excludePaths.length > 0 ? plan.target.excludePaths.join(", ") : "none", - knowledgeBases: plan.knowledgeBases.length > 0 ? plan.knowledgeBases.map(item => item.path).join(", ") : "none", - planFingerprint: plan.fingerprint, - }).trim(); + return prompt + .render(securityRequestPrompt, { + repositoryRoot: plan.repositoryRoot, + targetKind: plan.target.kind, + revision: plan.target.revision ?? "", + baseRevision: plan.target.baseRevision ?? "", + headRevision: plan.target.headRevision ?? "", + includePaths: plan.target.includePaths.length > 0 ? plan.target.includePaths.join(", ") : "all in-scope paths", + excludePaths: plan.target.excludePaths.length > 0 ? plan.target.excludePaths.join(", ") : "none", + knowledgeBases: + plan.knowledgeBases.length > 0 ? plan.knowledgeBases.map(item => item.path).join(", ") : "none", + planFingerprint: plan.fingerprint, + }) + .trim(); } function terminalText(snapshot: SecurityOperationSnapshot): string { @@ -334,18 +339,23 @@ export class SecurityCoordinator { const workRoot = path.join(store.projectDirectory, "work"); await fs.mkdir(workRoot, { recursive: true, mode: 0o700 }); if (process.platform !== "win32") await fs.chmod(workRoot, 0o700); - const plan = await createSecurityScanPlan({ - cwd: this.#host.cwd, - target: input.target ?? { kind: "repository" }, - knowledgeBasePaths: input.knowledgeBasePaths, - outputRoot: input.outputRoot ?? path.join(workRoot, Bun.randomUUIDv7()), - archiveExisting: input.archiveExisting, - model: { provider: model.provider, modelId: model.id, thinkingLevel: input.thinkingLevel }, - account, - config: input.config ?? { securityEnabled: true }, - workflowFingerprint: SECURITY_WORKFLOW_FINGERPRINT, - signal: input.signal, - }, this.#gitAdapter); + const modelRef: SecurityModelRef = { provider: model.provider, modelId: model.id }; + if (input.thinkingLevel !== undefined) modelRef.thinkingLevel = input.thinkingLevel; + const plan = await createSecurityScanPlan( + { + cwd: this.#host.cwd, + target: input.target ?? { kind: "repository" }, + knowledgeBasePaths: input.knowledgeBasePaths, + outputRoot: input.outputRoot ?? path.join(workRoot, Bun.randomUUIDv7()), + archiveExisting: input.archiveExisting, + model: modelRef, + account, + config: input.config ?? { securityEnabled: true }, + workflowFingerprint: SECURITY_WORKFLOW_FINGERPRINT, + signal: input.signal, + }, + this.#gitAdapter, + ); await store.putPlan(plan); return plan; } @@ -459,7 +469,8 @@ export class SecurityCoordinator { activeModel?.provider === plan.model.provider && activeModel.id === plan.model.modelId ? activeModel : this.#host.modelRegistry.find(plan.model.provider, plan.model.modelId); - if (!model) throw new Error(`Security scan model is unavailable: ${plan.model.provider}/${plan.model.modelId}`); + if (!model) + throw new Error(`Security scan model is unavailable: ${plan.model.provider}/${plan.model.modelId}`); const sessionsDirectory = path.join(store.projectDirectory, "sessions"); await fs.mkdir(sessionsDirectory, { recursive: true, mode: 0o700 }); const sessionManager = SessionManager.create(plan.repositoryRoot, sessionsDirectory); @@ -480,7 +491,9 @@ export class SecurityCoordinator { plan, scanId: record.snapshot.scanId, model, - publicationTool, + // Bare `ToolDefinition` erases the concrete schema; the sdk.ts + // `as unknown as CustomTool` precedent applies to the same variance wall. + publicationTool: publicationTool as unknown as ToolDefinition, sessionManager, }); record.snapshot.sessionFile = session.sessionFile; @@ -521,7 +534,13 @@ export class SecurityCoordinator { } const message = error instanceof Error ? error.message : String(error); const cancelled = signal.aborted; - const terminal = initialBundle(store, plan, record.snapshot.scanId, startedAt, cancelled ? "cancelled" : "failed"); + const terminal = initialBundle( + store, + plan, + record.snapshot.scanId, + startedAt, + cancelled ? "cancelled" : "failed", + ); terminal.scan.completedAt = iso(this.#now); terminal.scan.error = message; await store.putBundle(terminal); diff --git a/packages/coding-agent/src/security/importers/codex-security.ts b/packages/coding-agent/src/security/importers/codex-security.ts index a30c3840b..05110a6ad 100644 --- a/packages/coding-agent/src/security/importers/codex-security.ts +++ b/packages/coding-agent/src/security/importers/codex-security.ts @@ -1,4 +1,17 @@ +import * as fs from "node:fs/promises"; import * as path from "node:path"; +import type { + SecurityCoverage, + SecurityEvidence, + SecurityFinding, + SecurityLocation, + SecurityProducer, + SecurityProvenance, + SecurityScan, + SecurityScanBundle, + SecurityTarget, + SecurityUpstreamProvenance, +} from "../contracts"; import { createSecurityEvidenceId, createSecurityFindingFingerprint, @@ -9,14 +22,6 @@ import { parseSecurityScanBundle, securitySha256, } from "../contracts"; -import type { - SecurityCoverage, - SecurityEvidence, - SecurityFinding, - SecurityLocation, - SecurityProvenance, - SecurityScanBundle, -} from "../contracts"; interface CodexManifest { documentType?: string; @@ -105,14 +110,17 @@ function stringArray(value: unknown): string[] { } function locationsForFinding(finding: CodexFinding): SecurityLocation[] { - const locations = (finding.locations ?? []) - .filter(location => typeof location.path === "string" && typeof location.startLine === "number") - .map(location => ({ - path: location.path as string, - startLine: location.startLine as number, - endLine: location.endLine, - role: location.role, - })); + const locations: SecurityLocation[] = []; + for (const location of finding.locations ?? []) { + if (typeof location.path !== "string" || typeof location.startLine !== "number") continue; + const normalized: SecurityLocation = { + path: location.path, + startLine: location.startLine, + }; + if (location.endLine !== undefined) normalized.endLine = location.endLine; + if (location.role !== undefined) normalized.role = location.role; + locations.push(normalized); + } return locations.length > 0 ? locations : [{ path: "unknown", startLine: 1, role: "unknown" }]; } @@ -131,7 +139,7 @@ function mapCoverage(document: CodexCoverageDocument): SecurityCoverage { ) ? (document.inventoryStrategy as SecurityCoverage["inventoryStrategy"]) : "imported"; - return { + const coverage: SecurityCoverage = { mode, completeness, inventoryStrategy, @@ -142,10 +150,11 @@ function mapCoverage(document: CodexCoverageDocument): SecurityCoverage { ? (document.explicitExclusions as SecurityCoverage["explicitExclusions"]) : [], deferred: Array.isArray(document.deferred) ? (document.deferred as SecurityCoverage["deferred"]) : [], - openQuestions: Array.isArray(document.openQuestions) - ? (document.openQuestions as SecurityCoverage["openQuestions"]) - : undefined, }; + if (Array.isArray(document.openQuestions)) { + coverage.openQuestions = document.openQuestions as SecurityCoverage["openQuestions"]; + } + return coverage; } export async function importCodexSecurityBundle( @@ -172,25 +181,26 @@ export async function importCodexSecurityBundle( ) { throw new Error("Codex Security bundle scan IDs do not agree"); } - const fixtureProvenance = await readJson(path.join(root, "PROVENANCE.json")).catch(() => ({})); + const fixtureProvenance = await readJson(path.join(root, "PROVENANCE.json")).catch( + (): CodexFixtureProvenance => ({}), + ); const scanId = options.createScanId?.() ?? createSecurityScanId(); const createdAt = options.createdAt ?? manifest.scan.startedAt ?? new Date().toISOString(); - const canonicalRoot = path.resolve(options.repositoryRoot); - const producer = { - kind: "codex-security-bundle" as const, + const canonicalRoot = await fs.realpath(path.resolve(options.repositoryRoot)); + const producer: SecurityProducer = { + kind: "codex-security-bundle", name: manifest.scan.producer?.name || "codex-security", - version: manifest.scan.producer?.version, vendor: "openai", - revision: fixtureProvenance.revision, - pluginVersion: fixtureProvenance.pluginVersion, - }; - const upstream = { - repository: fixtureProvenance.repository, - revision: fixtureProvenance.revision, - packageVersion: fixtureProvenance.packageVersion, - pluginVersion: fixtureProvenance.pluginVersion, - archiveSha256: fixtureProvenance.archiveSha256, }; + if (manifest.scan.producer?.version !== undefined) producer.version = manifest.scan.producer.version; + if (fixtureProvenance.revision !== undefined) producer.revision = fixtureProvenance.revision; + if (fixtureProvenance.pluginVersion !== undefined) producer.pluginVersion = fixtureProvenance.pluginVersion; + const upstream: SecurityUpstreamProvenance = {}; + if (fixtureProvenance.repository !== undefined) upstream.repository = fixtureProvenance.repository; + if (fixtureProvenance.revision !== undefined) upstream.revision = fixtureProvenance.revision; + if (fixtureProvenance.packageVersion !== undefined) upstream.packageVersion = fixtureProvenance.packageVersion; + if (fixtureProvenance.pluginVersion !== undefined) upstream.pluginVersion = fixtureProvenance.pluginVersion; + if (fixtureProvenance.archiveSha256 !== undefined) upstream.archiveSha256 = fixtureProvenance.archiveSha256; const findings: SecurityFinding[] = []; for (const source of findingsDocument.findings ?? []) { const ruleId = source.ruleId || "codex-security.unknown"; @@ -202,22 +212,22 @@ export async function importCodexSecurityBundle( anchor: source.identity?.anchor, locations, }); - const evidence: SecurityEvidence[] = (source.codeEvidence ?? []).map((item, index) => ({ - id: createSecurityEvidenceId(fingerprint, item.label || item.id || "code evidence", index), - kind: "code", - label: item.label || item.id || `Evidence ${index + 1}`, - explanation: item.explanation || "", - location: - typeof item.path === "string" && typeof item.startLine === "number" - ? { - path: item.path, - startLine: item.startLine, - endLine: item.endLine, - role: item.role, - } - : undefined, - excerpt: item.code, - })); + const evidence: SecurityEvidence[] = (source.codeEvidence ?? []).map((item, index) => { + const entry: SecurityEvidence = { + id: createSecurityEvidenceId(fingerprint, item.label || item.id || "code evidence", index), + kind: "code", + label: item.label || item.id || `Evidence ${index + 1}`, + explanation: item.explanation || "", + }; + if (typeof item.path === "string" && typeof item.startLine === "number") { + const location: SecurityLocation = { path: item.path, startLine: item.startLine }; + if (item.endLine !== undefined) location.endLine = item.endLine; + if (item.role !== undefined) location.role = item.role; + entry.location = location; + } + if (item.code !== undefined) entry.excerpt = item.code; + return entry; + }); const provenance: SecurityProvenance = { producer, createdAt, @@ -227,34 +237,30 @@ export async function importCodexSecurityBundle( ...(source.findingId ? { findingId: source.findingId } : {}), ...(source.occurrenceId ? { occurrenceId: source.occurrenceId } : {}), }, - vendorFingerprints: source.fingerprints?.primary - ? { [source.fingerprints.algorithm || "codex-security/v1"]: source.fingerprints.primary } - : undefined, upstream, - metadata: source.provenance, }; - findings.push({ + if (source.fingerprints?.primary) { + provenance.vendorFingerprints = { + [source.fingerprints.algorithm || "codex-security/v1"]: source.fingerprints.primary, + }; + } + if (source.provenance !== undefined) provenance.metadata = source.provenance; + const finding: SecurityFinding = { id: createSecurityFindingId(fingerprint), scanId, fingerprint, ruleId, - anchor: source.identity?.anchor, title: source.title || ruleId, summary: source.summary || "", severity: { level: ["critical", "high", "medium", "low", "informational"].includes(source.severity?.level ?? "") ? (source.severity?.level as SecurityFinding["severity"]["level"]) : "informational", - score: source.severity?.score, - scoringSystem: source.severity?.scoringSystem, - vector: source.severity?.vector, - rationale: source.severity?.rationale, }, confidence: { level: ["high", "medium", "low"].includes(source.confidence?.level ?? "") ? (source.confidence?.level as SecurityFinding["confidence"]["level"]) : "medium", - rationale: source.confidence?.rationale, }, taxonomy: { category, cwe: stringArray(source.taxonomy?.cwe) }, occurrences: [ @@ -265,16 +271,23 @@ export async function importCodexSecurityBundle( }, ], evidence, - remediation: source.remediation, validation: { status: source.validation ? "validated" : "unvalidated", - summary: source.validation ? JSON.stringify(source.validation) : undefined, evidenceIds: [], }, disposition: { status: "open" }, provenance, - extensions: source.extensions, - }); + }; + if (source.identity?.anchor !== undefined) finding.anchor = source.identity.anchor; + if (source.severity?.score !== undefined) finding.severity.score = source.severity.score; + if (source.severity?.scoringSystem !== undefined) finding.severity.scoringSystem = source.severity.scoringSystem; + if (source.severity?.vector !== undefined) finding.severity.vector = source.severity.vector; + if (source.severity?.rationale !== undefined) finding.severity.rationale = source.severity.rationale; + if (source.confidence?.rationale !== undefined) finding.confidence.rationale = source.confidence.rationale; + if (source.remediation !== undefined) finding.remediation = source.remediation; + if (source.validation) finding.validation.summary = JSON.stringify(source.validation); + if (source.extensions !== undefined) finding.extensions = source.extensions; + findings.push(finding); } const target = manifest.scan.target ?? {}; @@ -283,8 +296,12 @@ export async function importCodexSecurityBundle( sourceKind === "git_diff" ? "ref_diff" : sourceKind === "git_worktree" ? "working_tree" : "imported"; const reportPath = path.join(root, "report.md"); const sarifPath = path.join(root, "exports", "results.sarif"); - const report = await Bun.file(reportPath).text().catch(() => undefined); - const sarifText = await Bun.file(sarifPath).text().catch(() => undefined); + const report = await Bun.file(reportPath) + .text() + .catch(() => undefined); + const sarifText = await Bun.file(sarifPath) + .text() + .catch(() => undefined); const scanProvenance: SecurityProvenance = { producer, createdAt, @@ -293,39 +310,39 @@ export async function importCodexSecurityBundle( upstream, metadata: { bundleDirectory: root }, }; - return parseSecurityScanBundle({ - scan: { - documentType: "omp-security.scan", - schemaVersion: "1.0", - id: scanId, - projectKey: encodeSecurityProjectKey(canonicalRoot), - status: "completed", - createdAt, - startedAt: manifest.scan.startedAt, - completedAt: manifest.scan.completedAt ?? createdAt, - target: { - kind: targetKind, - repositoryRoot: canonicalRoot, - displayName: String(target.displayName ?? path.basename(canonicalRoot)), - revision: typeof target.revision === "string" ? target.revision : undefined, - baseRevision: typeof target.baseRevision === "string" ? target.baseRevision : undefined, - headRevision: typeof target.headRevision === "string" ? target.headRevision : undefined, - includePaths: stringArray(manifest.scan.scope?.includePaths), - excludePaths: stringArray(manifest.scan.scope?.excludePaths), - treeDigest: - typeof target.snapshotDigest === "string" - ? target.snapshotDigest - : securitySha256(JSON.stringify({ manifest, findingsDocument, coverageDocument })), - }, - producer, - provenance: scanProvenance, - findingIds: findings.map(finding => finding.id), - coverage: mapCoverage(coverageDocument), - reportRef: report ? "report.md" : undefined, - sarifRef: sarifText ? "results.sarif" : undefined, - }, - findings, - report, - sarif: sarifText ? (JSON.parse(sarifText) as Record) : undefined, - }); + const canonicalTarget: SecurityTarget = { + kind: targetKind, + repositoryRoot: canonicalRoot, + displayName: String(target.displayName ?? path.basename(canonicalRoot)), + includePaths: stringArray(manifest.scan.scope?.includePaths), + excludePaths: stringArray(manifest.scan.scope?.excludePaths), + treeDigest: + typeof target.snapshotDigest === "string" + ? target.snapshotDigest + : securitySha256(JSON.stringify({ manifest, findingsDocument, coverageDocument })), + }; + if (typeof target.revision === "string") canonicalTarget.revision = target.revision; + if (typeof target.baseRevision === "string") canonicalTarget.baseRevision = target.baseRevision; + if (typeof target.headRevision === "string") canonicalTarget.headRevision = target.headRevision; + const scan: SecurityScan = { + documentType: "omp-security.scan", + schemaVersion: "1.0", + id: scanId, + projectKey: encodeSecurityProjectKey(canonicalRoot), + status: "completed", + createdAt, + completedAt: manifest.scan.completedAt ?? createdAt, + target: canonicalTarget, + producer, + provenance: scanProvenance, + findingIds: findings.map(finding => finding.id), + coverage: mapCoverage(coverageDocument), + }; + if (manifest.scan.startedAt !== undefined) scan.startedAt = manifest.scan.startedAt; + if (report !== undefined) scan.reportRef = "report.md"; + if (sarifText !== undefined) scan.sarifRef = "results.sarif"; + const bundle: SecurityScanBundle = { scan, findings }; + if (report !== undefined) bundle.report = report; + if (sarifText !== undefined) bundle.sarif = JSON.parse(sarifText) as Record; + return parseSecurityScanBundle(bundle); } diff --git a/packages/coding-agent/src/security/importers/sarif.ts b/packages/coding-agent/src/security/importers/sarif.ts index 7d485595f..ce612f38e 100644 --- a/packages/coding-agent/src/security/importers/sarif.ts +++ b/packages/coding-agent/src/security/importers/sarif.ts @@ -1,4 +1,14 @@ +import * as fs from "node:fs/promises"; import * as path from "node:path"; +import type { + SecurityCoverage, + SecurityFinding, + SecurityLocation, + SecurityProducer, + SecurityProvenance, + SecurityScanBundle, + SecuritySeverityLevel, +} from "../contracts"; import { createSecurityFindingFingerprint, createSecurityFindingId, @@ -8,14 +18,6 @@ import { parseSecurityScanBundle, securitySha256, } from "../contracts"; -import type { - SecurityCoverage, - SecurityFinding, - SecurityLocation, - SecurityProvenance, - SecurityScanBundle, - SecuritySeverityLevel, -} from "../contracts"; interface SarifRegion { startLine?: number; @@ -88,16 +90,18 @@ function normalizeSarifLocations(result: SarifResult): SecurityLocation[] { for (const item of result.locations ?? []) { const physical = item.physicalLocation; const uri = physical?.artifactLocation?.uri; - const startLine = physical?.region?.startLine; + const region = physical?.region; + const startLine = region?.startLine; if (!uri || !startLine || startLine < 1) continue; - locations.push({ + const location: SecurityLocation = { path: uri.replaceAll("\\", "/").replace(/^\.\//, ""), startLine, - endLine: physical.region?.endLine, - startColumn: physical.region?.startColumn, - endColumn: physical.region?.endColumn, role: "primary", - }); + }; + if (region.endLine !== undefined) location.endLine = region.endLine; + if (region.startColumn !== undefined) location.startColumn = region.startColumn; + if (region.endColumn !== undefined) location.endColumn = region.endColumn; + locations.push(location); } return locations.length > 0 ? locations : [{ path: "unknown", startLine: 1, role: "unknown" }]; } @@ -119,9 +123,9 @@ function tagsForRule(rule: SarifRule | undefined): string[] { export async function importSarif(input: unknown, options: SarifImportOptions): Promise { const sarif = input as SarifLog; if (sarif.version !== "2.1.0" || !Array.isArray(sarif.runs)) { - throw new Error("Expected a SARIF 2.1.0 log with a runs array"); + throw new Error("Expected SARIF 2.1.0 input"); } - const canonicalRoot = path.resolve(options.repositoryRoot); + const canonicalRoot = await fs.realpath(path.resolve(options.repositoryRoot)); const scanId = options.createScanId?.() ?? createSecurityScanId(); const createdAt = options.createdAt ?? new Date().toISOString(); const findings: SecurityFinding[] = []; @@ -154,27 +158,22 @@ export async function importSarif(input: unknown, options: SarifImportOptions): }); const tags = tagsForRule(rule); const provenance: SecurityProvenance = { - producer: { kind: "sarif-import", name: producerName, version: producerVersion }, + producer: { kind: "sarif-import", name: producerName }, createdAt, importedAt: new Date().toISOString(), vendorFingerprints, - metadata: options.sourcePath ? { sourcePath: options.sourcePath } : undefined, }; + if (producerVersion !== undefined) provenance.producer.version = producerVersion; + if (options.sourcePath) provenance.metadata = { sourcePath: options.sourcePath }; const message = result.message?.text ?? result.message?.markdown ?? rule?.shortDescription?.text ?? ruleId; - findings.push({ + const finding: SecurityFinding = { id: createSecurityFindingId(fingerprint), scanId, fingerprint, ruleId, - anchor: firstVendorFingerprint, title: rule?.shortDescription?.text ?? rule?.name ?? ruleId, summary: message, - severity: { - level: severityFromSarif(result), - score: Number.isFinite(Number(result.properties?.["security-severity"])) - ? Number(result.properties?.["security-severity"]) - : undefined, - }, + severity: { level: severityFromSarif(result) }, confidence: { level: "medium", rationale: "Imported from a SARIF producer" }, taxonomy: { category, @@ -186,7 +185,11 @@ export async function importSarif(input: unknown, options: SarifImportOptions): validation: { status: "unvalidated", evidenceIds: [] }, disposition: { status: "open" }, provenance, - }); + }; + if (firstVendorFingerprint !== undefined) finding.anchor = firstVendorFingerprint; + const score = Number(result.properties?.["security-severity"]); + if (Number.isFinite(score)) finding.severity.score = score; + findings.push(finding); } } @@ -200,13 +203,14 @@ export async function importSarif(input: unknown, options: SarifImportOptions): explicitExclusions: [], deferred: [{ id: "sarif-coverage", reason: "SARIF does not define repository coverage" }], }; - const producer = { kind: "sarif-import" as const, name: producerName, version: producerVersion }; + const producer: SecurityProducer = { kind: "sarif-import", name: producerName }; + if (producerVersion !== undefined) producer.version = producerVersion; const scanProvenance: SecurityProvenance = { producer, createdAt, importedAt: new Date().toISOString(), - metadata: options.sourcePath ? { sourcePath: options.sourcePath } : undefined, }; + if (options.sourcePath) scanProvenance.metadata = { sourcePath: options.sourcePath }; return parseSecurityScanBundle({ scan: { documentType: "omp-security.scan", diff --git a/packages/coding-agent/src/security/index.ts b/packages/coding-agent/src/security/index.ts index 0a8bf6e34..8e2665c64 100644 --- a/packages/coding-agent/src/security/index.ts +++ b/packages/coding-agent/src/security/index.ts @@ -6,7 +6,7 @@ export * from "./importers"; export * from "./preflight"; export * from "./provenance"; export * from "./publication"; +export * from "./remediation"; export * from "./resource-output"; export * from "./sarif"; export * from "./store"; -export * from "./remediation"; diff --git a/packages/coding-agent/src/security/preflight.ts b/packages/coding-agent/src/security/preflight.ts index 83355a896..c169c34f4 100644 --- a/packages/coding-agent/src/security/preflight.ts +++ b/packages/coding-agent/src/security/preflight.ts @@ -1,11 +1,6 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; -import { - canonicalSecurityJson, - createSecurityPlanId, - parseSecurityScanPlan, - securitySha256, -} from "./contracts"; +import * as git from "../utils/git"; import type { SecurityAccountRef, SecurityKnowledgeBaseRef, @@ -14,7 +9,7 @@ import type { SecurityScanPlan, SecurityTarget, } from "./contracts"; -import * as git from "../utils/git"; +import { canonicalSecurityJson, createSecurityPlanId, parseSecurityScanPlan, securitySha256 } from "./contracts"; export type SecurityTargetRequest = | { kind: "repository"; includePaths?: string[]; excludePaths?: string[] } @@ -63,7 +58,10 @@ export const DEFAULT_SECURITY_GIT_ADAPTER: SecurityGitAdapter = { }; export class StaleSecurityScanPlanError extends Error { - constructor(readonly expected: string, readonly actual: string) { + constructor( + readonly expected: string, + readonly actual: string, + ) { super(`Security scan plan is stale: expected ${expected}, got ${actual}. Run security preflight again.`); this.name = "StaleSecurityScanPlanError"; } @@ -73,10 +71,6 @@ function pathIsWithin(candidate: string, root: string): boolean { return candidate === root || candidate.startsWith(`${root}${path.sep}`); } -async function canonicalExistingPath(input: string): Promise { - return fs.realpath(path.resolve(input)); -} - async function hashFile(filePath: string): Promise<{ sha256: string; size: number }> { const bytes = new Uint8Array(await Bun.file(filePath).arrayBuffer()); return { sha256: securitySha256(bytes), size: bytes.byteLength }; @@ -148,11 +142,17 @@ async function digestWorkingTree( const absolutePath = path.resolve(repositoryRoot, relativePath); if (!pathIsWithin(absolutePath, repositoryRoot)) throw new Error(`Git path escapes repository: ${relativePath}`); const stats = await fs.lstat(absolutePath).catch(() => null); - if (!stats?.isFile() || stats.isSymbolicLink()) continue; - const bytes = new Uint8Array(await Bun.file(absolutePath).arrayBuffer()); + if (!stats) continue; hasher.update(relativePath); hasher.update("\0"); - hasher.update(bytes); + if (stats.isSymbolicLink()) { + hasher.update("symlink\0"); + hasher.update(await fs.readlink(absolutePath)); + } else if (stats.isFile()) { + hasher.update(new Uint8Array(await Bun.file(absolutePath).arrayBuffer())); + } else { + continue; + } hasher.update("\0"); } const head = (await adapter.headSha(repositoryRoot, signal)) ?? "unborn"; @@ -194,21 +194,25 @@ async function normalizeTarget( }; } const revision = await adapter.headSha(repositoryRoot, signal); - return { + const target: SecurityTarget = { kind: request.kind, repositoryRoot, displayName, - revision: revision ?? undefined, includePaths, excludePaths, treeDigest: await digestWorkingTree(repositoryRoot, includePaths, excludePaths, adapter, signal), }; + if (revision !== null) target.revision = revision; + return target; } -async function normalizeKnowledgeBases(paths: readonly string[] | undefined): Promise { +async function normalizeKnowledgeBases( + paths: readonly string[] | undefined, + baseDirectory: string, +): Promise { const results: SecurityKnowledgeBaseRef[] = []; for (const input of paths ?? []) { - const canonical = await canonicalExistingPath(input); + const canonical = await fs.realpath(path.resolve(baseDirectory, input)); const stats = await fs.stat(canonical); if (!stats.isFile()) throw new Error(`Security knowledge base is not a file: ${input}`); const digest = await hashFile(canonical); @@ -249,7 +253,6 @@ async function normalizeOutput( return { root: canonicalCandidate, archiveExisting, existingState }; } - export interface PreparedSecurityOutput { root: string; archivedTo?: string; @@ -299,15 +302,28 @@ async function buildPlanMaterial( if (!repositoryRoot) throw new Error(`Security scans require a Git repository: ${request.cwd}`); const canonicalRoot = await fs.realpath(repositoryRoot); const target = await normalizeTarget(canonicalRoot, request.target, adapter, request.signal); - const knowledgeBases = await normalizeKnowledgeBases(request.knowledgeBasePaths); + const knowledgeBases = await normalizeKnowledgeBases(request.knowledgeBasePaths, canonicalRoot); const output = await normalizeOutput(canonicalRoot, request.outputRoot, request.archiveExisting ?? false); + const model: SecurityModelRef = { + provider: request.model.provider, + modelId: request.model.modelId, + }; + if (request.model.thinkingLevel !== undefined) model.thinkingLevel = request.model.thinkingLevel; + const account: SecurityAccountRef = { + provider: request.account.provider, + credentialId: request.account.credentialId, + }; + if (request.account.accountId !== undefined) account.accountId = request.account.accountId; + if (request.account.email !== undefined) account.email = request.account.email; + if (request.account.organizationId !== undefined) account.organizationId = request.account.organizationId; + if (request.account.organizationName !== undefined) account.organizationName = request.account.organizationName; return { repositoryRoot: canonicalRoot, target, knowledgeBases, output, - model: request.model, - account: request.account, + model, + account, configFingerprint: `omp-security-config/v1:sha256:${securitySha256(canonicalSecurityJson(request.config))}`, workflowFingerprint: request.workflowFingerprint, }; @@ -342,7 +358,11 @@ function requestFromPlan(plan: SecurityScanPlan, freshness: SecurityPlanFreshnes : plan.target.kind === "scoped_path" ? { kind: "scoped_path", includePaths: plan.target.includePaths, excludePaths: plan.target.excludePaths } : plan.target.kind === "working_tree" - ? { kind: "working_tree", includePaths: plan.target.includePaths, excludePaths: plan.target.excludePaths } + ? { + kind: "working_tree", + includePaths: plan.target.includePaths, + excludePaths: plan.target.excludePaths, + } : { kind: "repository", includePaths: plan.target.includePaths, excludePaths: plan.target.excludePaths }; return { cwd: plan.repositoryRoot, diff --git a/packages/coding-agent/src/security/provenance.ts b/packages/coding-agent/src/security/provenance.ts index 91d408dc7..60630d8aa 100644 --- a/packages/coding-agent/src/security/provenance.ts +++ b/packages/coding-agent/src/security/provenance.ts @@ -1,5 +1,5 @@ -import { canonicalSecurityJson, securitySha256 } from "./contracts"; import type { SecurityAccountRef, SecurityProducer, SecurityProvenance } from "./contracts"; +import { canonicalSecurityJson, securitySha256 } from "./contracts"; export const CODEX_SECURITY_UPSTREAM = { repository: "https://github.com/openai/codex-security", @@ -27,23 +27,25 @@ export function createNativeSecurityProvenance(options: { sessionId?: string; }): SecurityProvenance { const producer = createNativeSecurityProducer(); + const account: Record = { + provider: options.account.provider, + credentialId: options.account.credentialId, + }; + if (options.account.accountId !== undefined) account.accountId = options.account.accountId; + if (options.account.email !== undefined) account.email = options.account.email; + if (options.account.organizationId !== undefined) account.organizationId = options.account.organizationId; + if (options.account.organizationName !== undefined) account.organizationName = options.account.organizationName; + const metadata: Record = { + planFingerprint: options.planFingerprint, + workflowFingerprint: options.workflowFingerprint, + account, + }; + if (options.sessionId !== undefined) metadata.sessionId = options.sessionId; return { producer, createdAt: options.createdAt, upstream: { ...CODEX_SECURITY_UPSTREAM }, - metadata: { - planFingerprint: options.planFingerprint, - workflowFingerprint: options.workflowFingerprint, - sessionId: options.sessionId, - account: { - provider: options.account.provider, - credentialId: options.account.credentialId, - accountId: options.account.accountId, - email: options.account.email, - organizationId: options.account.organizationId, - organizationName: options.account.organizationName, - }, - }, + metadata, }; } diff --git a/packages/coding-agent/src/security/publication.ts b/packages/coding-agent/src/security/publication.ts index 4e8b005e1..b0f8ef0d7 100644 --- a/packages/coding-agent/src/security/publication.ts +++ b/packages/coding-agent/src/security/publication.ts @@ -1,12 +1,6 @@ +import { type } from "arktype"; import type { ToolDefinition } from "../extensibility/extensions"; import securityPublishDescription from "../prompts/tools/security-publish.md" with { type: "text" }; -import { type } from "arktype"; -import { - createSecurityEvidenceId, - createSecurityFindingFingerprint, - createSecurityFindingId, - createSecurityOccurrenceId, -} from "./contracts"; import type { SecurityCoverage, SecurityEvidence, @@ -16,6 +10,12 @@ import type { SecurityScanBundle, SecurityScanPlan, } from "./contracts"; +import { + createSecurityEvidenceId, + createSecurityFindingFingerprint, + createSecurityFindingId, + createSecurityOccurrenceId, +} from "./contracts"; import { createNativeSecurityProducer, createNativeSecurityProvenance } from "./provenance"; import { exportSecurityBundleToSarif } from "./sarif"; import type { SecurityStore } from "./store"; @@ -109,14 +109,15 @@ function normalizePublishedPath(input: string): string { } function toLocation(input: SecurityPublishParams["findings"][number]["locations"][number]): SecurityLocation { - return { + const location: SecurityLocation = { path: normalizePublishedPath(input.path), startLine: input.start_line, - endLine: input.end_line, - startColumn: input.start_column, - endColumn: input.end_column, - role: input.role, }; + if (input.end_line !== undefined) location.endLine = input.end_line; + if (input.start_column !== undefined) location.startColumn = input.start_column; + if (input.end_column !== undefined) location.endColumn = input.end_column; + if (input.role !== undefined) location.role = input.role; + return location; } function coverageMode(plan: SecurityScanPlan): SecurityCoverage["mode"] { @@ -155,20 +156,22 @@ function buildFinding( anchor: input.anchor, locations, }); - const evidence: SecurityEvidence[] = (input.evidence ?? []).map((item, index) => ({ - id: createSecurityEvidenceId(fingerprint, item.label, index), - kind: "code", - label: item.label, - explanation: item.explanation, - excerpt: item.excerpt, - location: item.location ? toLocation(item.location) : undefined, - })); - return { + const evidence: SecurityEvidence[] = (input.evidence ?? []).map((item, index) => { + const entry: SecurityEvidence = { + id: createSecurityEvidenceId(fingerprint, item.label, index), + kind: "code", + label: item.label, + explanation: item.explanation, + }; + if (item.excerpt !== undefined) entry.excerpt = item.excerpt; + if (item.location !== undefined) entry.location = toLocation(item.location); + return entry; + }); + const finding: SecurityFinding = { id: createSecurityFindingId(fingerprint), scanId: options.scanId, fingerprint, ruleId: input.rule_id, - anchor: input.anchor, title: input.title, summary: input.summary, severity: { level: input.severity }, @@ -182,7 +185,6 @@ function buildFinding( }, ], evidence, - remediation: input.remediation, validation: { status: input.validation ?? "unvalidated", evidenceIds: [] }, disposition: { status: "open" }, provenance: createNativeSecurityProvenance({ @@ -193,38 +195,50 @@ function buildFinding( sessionId: options.sessionId, }), }; + if (input.anchor !== undefined) finding.anchor = input.anchor; + if (input.remediation !== undefined) finding.remediation = input.remediation; + return finding; } function buildCoverage(params: SecurityPublishParams, plan: SecurityScanPlan): SecurityCoverage { - return { - mode: coverageMode(plan), - completeness: params.coverage.completeness, - inventoryStrategy: inventoryStrategy(plan), - includePaths: plan.target.includePaths, - excludePaths: plan.target.excludePaths, - surfaces: (params.coverage.surfaces ?? []).map((surface, index) => ({ + const surfaces: SecurityCoverage["surfaces"] = (params.coverage.surfaces ?? []).map((surface, index) => { + const entry: SecurityCoverage["surfaces"][number] = { id: `surface-${index + 1}`, label: surface.label, disposition: surface.disposition, receiptRefs: surface.receipt_refs ?? [], - riskArea: surface.risk_area, - notes: surface.notes, - })), - explicitExclusions: (params.coverage.explicit_exclusions ?? []).map(item => ({ - pattern: item.pattern, - reason: item.reason, - })), - deferred: (params.coverage.deferred ?? []).map((item, index) => ({ + }; + if (surface.risk_area !== undefined) entry.riskArea = surface.risk_area; + if (surface.notes !== undefined) entry.notes = surface.notes; + return entry; + }); + const deferred: SecurityCoverage["deferred"] = (params.coverage.deferred ?? []).map((item, index) => { + const entry: SecurityCoverage["deferred"][number] = { id: `deferred-${index + 1}`, reason: item.reason, - paths: item.paths, - surfaceIds: item.surface_ids, - })), - openQuestions: (params.coverage.open_questions ?? []).map(item => ({ - question: item.question, - followUpPrompt: item.follow_up_prompt, - })), + }; + if (item.paths !== undefined) entry.paths = item.paths; + if (item.surface_ids !== undefined) entry.surfaceIds = item.surface_ids; + return entry; + }); + const coverage: SecurityCoverage = { + mode: coverageMode(plan), + completeness: params.coverage.completeness, + inventoryStrategy: inventoryStrategy(plan), + includePaths: [...plan.target.includePaths], + excludePaths: [...plan.target.excludePaths], + surfaces, + explicitExclusions: params.coverage.explicit_exclusions ?? [], + deferred, }; + if (params.coverage.open_questions !== undefined) { + coverage.openQuestions = params.coverage.open_questions.map(item => { + const question: NonNullable[number] = { question: item.question }; + if (item.follow_up_prompt !== undefined) question.followUpPrompt = item.follow_up_prompt; + return question; + }); + } + return coverage; } export function createSecurityPublicationTool( @@ -240,55 +254,62 @@ export function createSecurityPublicationTool( strict: true, async execute(_toolCallId, params) { if (published) throw new Error(`Security scan ${options.scanId} has already been published`); - const completedAt = new Date().toISOString(); - const findingsByFingerprint = new Map(); - for (const input of params.findings) { - const finding = buildFinding(input, options, completedAt); - if (!findingsByFingerprint.has(finding.fingerprint)) { - findingsByFingerprint.set(finding.fingerprint, finding); - } - } - const findings = [...findingsByFingerprint.values()]; - const producer = createNativeSecurityProducer(); - const provenance = createNativeSecurityProvenance({ - createdAt: options.startedAt, - account: options.plan.account, - planFingerprint: options.plan.fingerprint, - workflowFingerprint: options.plan.workflowFingerprint, - sessionId: options.sessionId, - }); - const scan: SecurityScan = { - documentType: "omp-security.scan", - schemaVersion: "1.0", - id: options.scanId, - projectKey: options.store.projectKey, - status: "completed", - createdAt: options.plan.createdAt, - startedAt: options.startedAt, - completedAt, - plan: options.plan, - target: options.plan.target, - producer, - provenance, - findingIds: findings.map(finding => finding.id), - coverage: buildCoverage(params, options.plan), - reportRef: "report.md", - sarifRef: "results.sarif", - }; - const provisional: SecurityScanBundle = { scan, findings, report: params.report }; - const bundle: SecurityScanBundle = { ...provisional, sarif: exportSecurityBundleToSarif(provisional) }; - await options.store.putBundle(bundle); published = true; - await options.onPublished?.(bundle); - return { - content: [ - { - type: "text", - text: `Published security scan ${options.scanId} with ${findings.length} finding(s).`, - }, - ], - details: { scanId: options.scanId, findingCount: findings.length, status: "completed" }, - }; + let persisted = false; + try { + const completedAt = new Date().toISOString(); + const findingsByFingerprint = new Map(); + for (const input of params.findings) { + const finding = buildFinding(input, options, completedAt); + if (!findingsByFingerprint.has(finding.fingerprint)) { + findingsByFingerprint.set(finding.fingerprint, finding); + } + } + const findings = [...findingsByFingerprint.values()]; + const producer = createNativeSecurityProducer(); + const provenance = createNativeSecurityProvenance({ + createdAt: options.startedAt, + account: options.plan.account, + planFingerprint: options.plan.fingerprint, + workflowFingerprint: options.plan.workflowFingerprint, + sessionId: options.sessionId, + }); + const scan: SecurityScan = { + documentType: "omp-security.scan", + schemaVersion: "1.0", + id: options.scanId, + projectKey: options.store.projectKey, + status: "completed", + createdAt: options.plan.createdAt, + startedAt: options.startedAt, + completedAt, + plan: options.plan, + target: options.plan.target, + producer, + provenance, + findingIds: findings.map(finding => finding.id), + coverage: buildCoverage(params, options.plan), + reportRef: "report.md", + sarifRef: "results.sarif", + }; + const provisional: SecurityScanBundle = { scan, findings, report: params.report }; + const bundle: SecurityScanBundle = { ...provisional, sarif: exportSecurityBundleToSarif(provisional) }; + await options.store.putBundle(bundle); + persisted = true; + await options.onPublished?.(bundle); + return { + content: [ + { + type: "text", + text: `Published security scan ${options.scanId} with ${findings.length} finding(s).`, + }, + ], + details: { scanId: options.scanId, findingCount: findings.length, status: "completed" }, + }; + } catch (error) { + if (!persisted) published = false; + throw error; + } }, }; } diff --git a/packages/coding-agent/src/security/remediation.ts b/packages/coding-agent/src/security/remediation.ts index 89079e49f..5441efd86 100644 --- a/packages/coding-agent/src/security/remediation.ts +++ b/packages/coding-agent/src/security/remediation.ts @@ -1,8 +1,8 @@ import type { IsoBackendKind } from "@oh-my-pi/pi-natives"; -import { cleanupIsolation, ensureIsolation } from "../task/worktree"; -import type { IsolationHandle, WorktreeBaseline } from "../task/worktree"; -import { prepareIsolationContext } from "../task/isolation-runner"; import type { IsolationContext } from "../task/isolation-runner"; +import { prepareIsolationContext } from "../task/isolation-runner"; +import type { IsolationHandle, WorktreeBaseline } from "../task/worktree"; +import { cleanupIsolation, ensureIsolation } from "../task/worktree"; export interface SecurityRemediationRequest { cwd: string; diff --git a/packages/coding-agent/src/security/resource-output.ts b/packages/coding-agent/src/security/resource-output.ts index 13ded6f96..83c2a9578 100644 --- a/packages/coding-agent/src/security/resource-output.ts +++ b/packages/coding-agent/src/security/resource-output.ts @@ -1,6 +1,6 @@ import { sanitizeText } from "@oh-my-pi/pi-utils"; -import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, truncateHead } from "../session/streaming-output"; import type { InternalResource } from "../internal-urls"; +import { DEFAULT_MAX_BYTES, DEFAULT_MAX_LINES, truncateHead } from "../session/streaming-output"; export interface SecurityResourceOptions { url: string; diff --git a/packages/coding-agent/src/security/store.ts b/packages/coding-agent/src/security/store.ts index 652010a9b..a4b48f488 100644 --- a/packages/coding-agent/src/security/store.ts +++ b/packages/coding-agent/src/security/store.ts @@ -2,14 +2,6 @@ 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 { - encodeSecurityProjectKey, - parseSecurityFinding, - parseSecurityScan, - parseSecurityScanBundle, - parseSecurityScanPlan, - securitySha256, -} from "./contracts"; import { compareSecurityLineage } from "./comparison"; import type { SecurityComparisonReport, @@ -19,6 +11,14 @@ import type { SecurityScanBundle, SecurityScanPlan, } from "./contracts"; +import { + encodeSecurityProjectKey, + parseSecurityFinding, + parseSecurityScan, + parseSecurityScanBundle, + parseSecurityScanPlan, + securitySha256, +} from "./contracts"; const STORE_SCHEMA_VERSION = 1; const PRIVATE_DIRECTORY_MODE = 0o700; @@ -69,8 +69,20 @@ async function ensurePrivateDirectory(directory: string): Promise { if (process.platform !== "win32") await fs.chmod(directory, PRIVATE_DIRECTORY_MODE); } -export async function writeSecurityFileAtomic(filePath: string, content: string): Promise { - await ensurePrivateDirectory(path.dirname(filePath)); +export interface SecurityFileWriteOptions { + hardenParent?: boolean; +} + +export async function writeSecurityFileAtomic( + filePath: string, + content: string, + options: SecurityFileWriteOptions = {}, +): Promise { + if (options.hardenParent ?? true) { + await ensurePrivateDirectory(path.dirname(filePath)); + } else { + await fs.mkdir(path.dirname(filePath), { recursive: true, mode: PRIVATE_DIRECTORY_MODE }); + } const temporaryPath = `${filePath}.${process.pid}.${Bun.randomUUIDv7()}.tmp`; try { await fs.writeFile(temporaryPath, content, { encoding: "utf-8", mode: PRIVATE_FILE_MODE, flag: "wx" }); @@ -274,8 +286,10 @@ export class SecurityStore { const findings = rawFindings.map(parseSecurityFinding); const report = await readOptionalText(path.join(this.#scanDirectory(scanId), "report.md")); const sarifText = await readOptionalText(path.join(this.#scanDirectory(scanId), "results.sarif")); - const sarif = sarifText ? (JSON.parse(sarifText) as Record) : undefined; - return parseSecurityScanBundle({ scan, findings, report, sarif }); + const bundle: SecurityScanBundle = { scan, findings }; + if (report !== undefined) bundle.report = report; + if (sarifText !== undefined) bundle.sarif = JSON.parse(sarifText) as Record; + return parseSecurityScanBundle(bundle); } async getBundle(scanId: string): Promise { @@ -316,7 +330,11 @@ export class SecurityStore { if (!bundle) throw new Error(`Unknown security scan: ${scanId}`); const index = bundle.findings.findIndex(finding => finding.id === findingId); if (index < 0) throw new Error(`Unknown security finding: ${findingId}`); - const updated = { ...bundle.findings[index], disposition }; + const canonicalDisposition: SecurityDisposition = { status: disposition.status }; + if (disposition.rationale !== undefined) canonicalDisposition.rationale = disposition.rationale; + if (disposition.updatedAt !== undefined) canonicalDisposition.updatedAt = disposition.updatedAt; + if (disposition.actor !== undefined) canonicalDisposition.actor = disposition.actor; + const updated = { ...bundle.findings[index], disposition: canonicalDisposition }; bundle.findings[index] = parseSecurityFinding(updated); await this.#putBundleUnlocked(bundle); return bundle.findings[index]; diff --git a/packages/coding-agent/src/slash-commands/builtin-registry.ts b/packages/coding-agent/src/slash-commands/builtin-registry.ts index 285438bce..d904bdd2e 100644 --- a/packages/coding-agent/src/slash-commands/builtin-registry.ts +++ b/packages/coding-agent/src/slash-commands/builtin-registry.ts @@ -63,8 +63,8 @@ import { createMarketplaceManager } from "./helpers/marketplace-manager"; import { handleMcpAcp } from "./helpers/mcp"; import { commandConsumed, errorMessage, parseSlashCommand, parseSubcommand, usage } from "./helpers/parse"; import { describeRedeemOutcome, type ResetUsageAccount, toResetUsageAccounts } from "./helpers/reset-usage"; -import { matchSessionPinAccounts, toSessionPinAccounts } from "./helpers/session-pin"; import { handleSecurityCommand } from "./helpers/security"; +import { matchSessionPinAccounts, toSessionPinAccounts } from "./helpers/session-pin"; import { handleSshAcp } from "./helpers/ssh"; import { launchStatsDashboard, parseStatsDashboardArgs } from "./helpers/stats-dashboard"; import { handleTodoAcp } from "./helpers/todo"; diff --git a/packages/coding-agent/src/slash-commands/helpers/security.ts b/packages/coding-agent/src/slash-commands/helpers/security.ts index 695f23fac..ca35181a0 100644 --- a/packages/coding-agent/src/slash-commands/helpers/security.ts +++ b/packages/coding-agent/src/slash-commands/helpers/security.ts @@ -1,15 +1,15 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; import { prompt } from "@oh-my-pi/pi-utils"; -import { getSecurityCoordinator } from "../../security/coordinator"; -import type { SecurityPreflightInput } from "../../security/coordinator"; +import { parseInternalUrl } from "../../internal-urls/parse"; +import { SecurityProtocolHandler } from "../../internal-urls/security-protocol"; +import validationRequestPrompt from "../../prompts/security/validate-request.md" with { type: "text" }; import type { SecurityDispositionStatus } from "../../security/contracts"; +import type { SecurityPreflightInput } from "../../security/coordinator"; +import { getSecurityCoordinator } from "../../security/coordinator"; import { importCodexSecurityBundle, importSarifFile } from "../../security/importers"; import type { SecurityTargetRequest } from "../../security/preflight"; -import { SecurityProtocolHandler } from "../../internal-urls/security-protocol"; -import { parseInternalUrl } from "../../internal-urls/parse"; import { SecurityStore, writeSecurityFileAtomic } from "../../security/store"; -import validationRequestPrompt from "../../prompts/security/validate-request.md" with { type: "text" }; import { parseCommandArgs } from "../../utils/command-args"; import type { ParsedSlashCommand, SlashCommandResult, SlashCommandRuntime } from "../types"; import { commandConsumed, errorMessage, parseSubcommand, usage } from "./parse"; @@ -198,7 +198,7 @@ async function exportResults(runtime: SlashCommandRuntime, rest: string): Promis content = `${JSON.stringify(bundle, null, 2)}\n`; } const absolute = path.resolve(runtime.cwd, outputPath); - await writeSecurityFileAtomic(absolute, content); + await writeSecurityFileAtomic(absolute, content, { hardenParent: false }); await runtime.output(`Exported security scan ${scanId} to ${absolute}.`); } @@ -269,7 +269,9 @@ export async function handleSecurityCommand( await runtime.output( scans.length === 0 ? "No security scans are stored for this project." - : scans.map(scan => `${scan.id} ${scan.status} ${scan.findingCount} finding(s) ${scan.producer.name}`).join("\n"), + : scans + .map(scan => `${scan.id} ${scan.status} ${scan.findingCount} finding(s) ${scan.producer.name}`) + .join("\n"), ); return commandConsumed(); } diff --git a/packages/coding-agent/src/tools/index.ts b/packages/coding-agent/src/tools/index.ts index 4057f69a5..892a65cf4 100644 --- a/packages/coding-agent/src/tools/index.ts +++ b/packages/coding-agent/src/tools/index.ts @@ -59,8 +59,8 @@ import { MemoryReflectTool } from "./memory-reflect"; import { MemoryRetainTool } from "./memory-retain"; import { wrapToolWithMetaNotice } from "./output-meta"; import { ReadTool } from "./read"; -import { SecurityScanTool } from "./security-scan"; import type { PlanProposalHandler } from "./resolve"; +import { SecurityScanTool } from "./security-scan"; import { type TodoPhase, TodoTool } from "./todo"; import { WriteTool } from "./write"; import { isMountableUnderXdev, type XdevState } from "./xdev"; @@ -97,10 +97,10 @@ export * from "./memory-recall"; export * from "./memory-reflect"; export * from "./memory-retain"; export * from "./read"; -export * from "./security-scan"; export * from "./report-tool-issue"; export * from "./resolve"; export * from "./review"; +export * from "./security-scan"; export * from "./todo"; export * from "./tts"; export * from "./vibe"; diff --git a/packages/coding-agent/src/tools/security-scan.ts b/packages/coding-agent/src/tools/security-scan.ts index 7f967663c..c7ad1eb47 100644 --- a/packages/coding-agent/src/tools/security-scan.ts +++ b/packages/coding-agent/src/tools/security-scan.ts @@ -1,9 +1,8 @@ import type { AgentTool, AgentToolResult, ToolTier } from "@oh-my-pi/pi-agent-core"; import { type } from "arktype"; import securityScanDescription from "../prompts/tools/security-scan.md" with { type: "text" }; -import { getSecurityCoordinator } from "../security/coordinator"; import type { SecurityOperationSnapshot } from "../security/coordinator"; -import type { SecurityScanPlan } from "../security/contracts"; +import { getSecurityCoordinator } from "../security/coordinator"; import type { SecurityTargetRequest } from "../security/preflight"; import type { ToolSession } from "./index"; import { ToolError } from "./tool-errors"; @@ -27,7 +26,7 @@ type SecurityScanParams = typeof securityScanSchema.infer; export interface SecurityScanToolDetails { action: SecurityScanParams["action"]; - plan?: SecurityScanPlan; + plan?: { id: string; fingerprint: string }; operation?: SecurityOperationSnapshot; cancelled?: boolean; } @@ -36,7 +35,7 @@ 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", ...common }; + return { kind: "scoped_path", includePaths: params.include_paths ?? [], excludePaths: params.exclude_paths }; case "working_tree": return { kind: "working_tree", ...common }; case "ref_diff": @@ -114,15 +113,15 @@ export class SecurityScanTool implements AgentTool { }), ); InternalUrlRouter.resetForTests(); - InternalUrlRouter.instance().register(new SecurityProtocolHandler(async () => store, () => true)); + InternalUrlRouter.instance().register( + new SecurityProtocolHandler( + async () => store, + () => true, + ), + ); }); afterEach(async () => { @@ -74,6 +80,35 @@ describe("security://", () => { expect(completions?.some(item => item.value === "scans/secscan_sariffixture/findings")).toBeTrue(); }); + test("session settings override the process-global feature gate", async () => { + const enabledForSession = new SecurityProtocolHandler( + async () => store, + () => false, + ); + const resource = await enabledForSession.resolve(parseInternalUrl("security://scans"), { + cwd: repositoryRoot, + settings: { get: () => true }, + }); + expect(resource.content).toContain("Security scans"); + + const disabledForSession = new SecurityProtocolHandler( + async () => store, + () => true, + ); + await expect( + disabledForSession.resolve(parseInternalUrl("security://scans"), { + cwd: repositoryRoot, + settings: { get: () => false }, + }), + ).rejects.toThrow("disabled"); + expect( + await disabledForSession.complete("", { + cwd: repositoryRoot, + settings: { get: () => false }, + }), + ).toEqual([]); + }); + test("rejects surplus path segments instead of aliasing a canonical resource", async () => { await expect( InternalUrlRouter.instance().resolve("security://scans/secscan_codexfixture/manifest/extra", { @@ -85,10 +120,9 @@ describe("security://", () => { expect(findingId).toBeDefined(); if (!findingId) return; await expect( - InternalUrlRouter.instance().resolve( - `security://scans/secscan_sariffixture/findings/${findingId}/extra`, - { cwd: repositoryRoot }, - ), + InternalUrlRouter.instance().resolve(`security://scans/secscan_sariffixture/findings/${findingId}/extra`, { + cwd: repositoryRoot, + }), ).rejects.toThrow("Unknown security resource"); }); diff --git a/packages/coding-agent/test/security/auth.test.ts b/packages/coding-agent/test/security/auth.test.ts index c281d14ff..1e066e70a 100644 --- a/packages/coding-agent/test/security/auth.test.ts +++ b/packages/coding-agent/test/security/auth.test.ts @@ -46,6 +46,16 @@ describe("exact security OAuth resolver", () => { account: { provider: "openai-codex", credentialId: 42, accountId: "workspace-a" }, }); const exact = resolver(model()) as ApiKeyResolver; - await expect(exact({ lastChance: false, error: undefined })).rejects.toThrow("account mismatch"); + let caught: unknown; + try { + await exact({ lastChance: false, error: undefined }); + } catch (error) { + caught = error; + } + expect(caught).toBeInstanceOf(Error); + if (!(caught instanceof Error)) throw new Error("expected account mismatch"); + expect(caught.message).toContain("account mismatch"); + expect(caught.message).not.toContain("workspace-a"); + expect(caught.message).not.toContain("undefined"); }); }); diff --git a/packages/coding-agent/test/security/comparison.test.ts b/packages/coding-agent/test/security/comparison.test.ts index 26f4f0866..f387fed86 100644 --- a/packages/coding-agent/test/security/comparison.test.ts +++ b/packages/coding-agent/test/security/comparison.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; -import { compareSecurityLineage, compareSecurityProducers } from "../../src/security"; import type { SecurityFinding, SecurityScanBundle } from "../../src/security"; +import { compareSecurityLineage, compareSecurityProducers } from "../../src/security"; function finding(id: string, fingerprint: string, ruleId: string, path: string, startLine: number): SecurityFinding { return { diff --git a/packages/coding-agent/test/security/contracts.test.ts b/packages/coding-agent/test/security/contracts.test.ts index 851a3e05c..1ebe3b68a 100644 --- a/packages/coding-agent/test/security/contracts.test.ts +++ b/packages/coding-agent/test/security/contracts.test.ts @@ -1,4 +1,5 @@ import { describe, expect, test } from "bun:test"; +import type { SecurityFinding, SecurityScanBundle } from "../../src/security/contracts"; import { createSecurityFindingFingerprint, createSecurityFindingId, @@ -8,7 +9,6 @@ import { parseSecurityScanBundle, securitySha256, } from "../../src/security/contracts"; -import type { SecurityFinding, SecurityScanBundle } from "../../src/security/contracts"; const LOCATION = { path: "src/archive.ts", startLine: 10, endLine: 12, role: "sink" } as const; @@ -30,7 +30,9 @@ function fixtureFinding(): SecurityFinding { severity: { level: "high", score: 8.1, scoringSystem: "CVSS:3.1" }, confidence: { level: "high", rationale: "Direct source trace" }, taxonomy: { category: "path-traversal", cwe: ["CWE-22"] }, - occurrences: [{ id: createSecurityOccurrenceId(fingerprint, [LOCATION]), locations: [LOCATION], evidenceIds: [] }], + occurrences: [ + { id: createSecurityOccurrenceId(fingerprint, [LOCATION]), locations: [LOCATION], evidenceIds: [] }, + ], evidence: [], remediation: "Reject paths outside the extraction root.", validation: { status: "unvalidated", evidenceIds: [] }, @@ -116,13 +118,15 @@ describe("security contracts", () => { }; expect(parseSecurityScanBundle(bundle).findings).toHaveLength(1); expect(() => parseSecurityScanBundle({ ...bundle, findings: [{ ...finding, scanId: "other" }] })).toThrow(); - expect(() => parseSecurityScanBundle({ ...bundle, findings: [finding, finding] })).toThrow("duplicate finding ids"); + expect(() => parseSecurityScanBundle({ ...bundle, findings: [finding, finding] })).toThrow( + "duplicate finding ids", + ); expect(() => parseSecurityScanBundle({ ...bundle, scan: { ...bundle.scan, findingIds: [finding.id, finding.id] } }), ).toThrow("duplicate finding references"); - expect(() => - parseSecurityScanBundle({ ...bundle, scan: { ...bundle.scan, findingIds: [] } }), - ).toThrow("omits finding"); + expect(() => parseSecurityScanBundle({ ...bundle, scan: { ...bundle.scan, findingIds: [] } })).toThrow( + "omits finding", + ); const missingEvidence = { ...finding, occurrences: [{ ...finding.occurrences[0], evidenceIds: ["sece_missing"] }], diff --git a/packages/coding-agent/test/security/coordinator.test.ts b/packages/coding-agent/test/security/coordinator.test.ts index dcc7f4faa..f948d2c98 100644 --- a/packages/coding-agent/test/security/coordinator.test.ts +++ b/packages/coding-agent/test/security/coordinator.test.ts @@ -3,8 +3,8 @@ import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; import { unregisterCustomApis } from "@oh-my-pi/pi-ai/api-registry"; -import { AuthStorage, type AuthCredentialStore, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai/auth-storage"; -import { createMockModel, registerMockApi, type MockResponseSource } from "@oh-my-pi/pi-ai/providers/mock"; +import { type AuthCredentialStore, AuthStorage, SqliteAuthCredentialStore } from "@oh-my-pi/pi-ai/auth-storage"; +import { createMockModel, type MockResponseSource, registerMockApi } from "@oh-my-pi/pi-ai/providers/mock"; import { ModelRegistry } from "../../src/config/model-registry"; import { Settings } from "../../src/config/settings"; import { SecurityCoordinator, type SecurityGitAdapter, SecurityStore } from "../../src/security"; diff --git a/packages/coding-agent/test/security/history.test.ts b/packages/coding-agent/test/security/history.test.ts index a22ebbe5b..fc54c06fe 100644 --- a/packages/coding-agent/test/security/history.test.ts +++ b/packages/coding-agent/test/security/history.test.ts @@ -34,10 +34,7 @@ describe("security history and dispositions", () => { }); await store.putBundle(before); await store.putBundle(after); - expect((await store.listScans()).map(scan => scan.id)).toEqual([ - "secscan_historyafter", - "secscan_historybefore", - ]); + expect((await store.listScans()).map(scan => scan.id)).toEqual(["secscan_historyafter", "secscan_historybefore"]); const comparison = await store.compare(before.scan.id, after.scan.id); expect(comparison.unchanged).toBe(2); expect(comparison.introduced).toBe(0); diff --git a/packages/coding-agent/test/security/preflight.test.ts b/packages/coding-agent/test/security/preflight.test.ts index 84faa81bb..1099cc43c 100644 --- a/packages/coding-agent/test/security/preflight.test.ts +++ b/packages/coding-agent/test/security/preflight.test.ts @@ -6,9 +6,9 @@ import { assertSecurityScanPlanFresh, createSecurityScanPlan, prepareSecurityOutputDirectory, - StaleSecurityScanPlanError, type SecurityGitAdapter, type SecurityTargetRequest, + StaleSecurityScanPlanError, } from "../../src/security"; let temporaryRoot = ""; @@ -106,6 +106,42 @@ describe("security preflight", () => { ).rejects.toBeInstanceOf(StaleSecurityScanPlanError); }); + test("relative knowledge-base paths resolve from the repository", async () => { + await Bun.write(path.join(repositoryRoot, "policy.md"), "policy v1\n"); + const created = await createSecurityScanPlan( + { + cwd: repositoryRoot, + target: { kind: "repository" }, + knowledgeBasePaths: ["policy.md"], + outputRoot: stateRoot, + model: { provider: "openai-codex", modelId: "fixture" }, + account: { provider: "openai-codex", credentialId: 17 }, + config: {}, + workflowFingerprint: "fixture", + }, + adapter, + ); + expect(created.knowledgeBases[0]?.path).toBe(await fs.realpath(path.join(repositoryRoot, "policy.md"))); + }); + + test("symlink target mutation makes a plan stale", async () => { + if (process.platform === "win32") return; + const linkedPath = path.join(repositoryRoot, "src", "a.ts"); + await fs.rm(linkedPath); + await fs.symlink("first-target.ts", linkedPath); + statusText = " M src/a.ts"; + const created = await plan(); + await fs.rm(linkedPath); + await fs.symlink("second-target.ts", linkedPath); + await expect( + assertSecurityScanPlanFresh( + created, + { config: { security: { enabled: true } }, workflowFingerprint: "security-reviewer@fixture" }, + adapter, + ), + ).rejects.toBeInstanceOf(StaleSecurityScanPlanError); + }); + test("configuration mutation makes a plan stale", async () => { const created = await plan(); await expect( @@ -163,9 +199,11 @@ describe("security preflight", () => { adapter, ); const prepared = await prepareSecurityOutputDirectory(created.output, "fixture"); - expect(prepared.archivedTo).toBe(`${stateRoot}.archive-fixture`); - expect(await fs.readdir(stateRoot)).toEqual([]); - expect(await Bun.file(path.join(`${stateRoot}.archive-fixture`, "existing.txt")).text()).toBe("existing"); + expect(prepared.archivedTo).toBe(`${created.output.root}.archive-fixture`); + expect(await fs.readdir(created.output.root)).toEqual([]); + expect(await Bun.file(path.join(`${created.output.root}.archive-fixture`, "existing.txt")).text()).toBe( + "existing", + ); }); test("symlink output is rejected", async () => { @@ -178,9 +216,7 @@ describe("security preflight", () => { 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", - ); + await expect(plan({ kind: "scoped_path", includePaths: [candidate] })).rejects.toThrow("repository-relative"); } }); }); diff --git a/packages/coding-agent/test/security/publication.test.ts b/packages/coding-agent/test/security/publication.test.ts index 589450e7b..4845a0736 100644 --- a/packages/coding-agent/test/security/publication.test.ts +++ b/packages/coding-agent/test/security/publication.test.ts @@ -2,8 +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 { createSecurityPublicationTool, SecurityStore } from "../../src/security"; import type { SecurityScanPlan } from "../../src/security"; +import { createSecurityPublicationTool, SecurityStore } from "../../src/security"; let temporaryRoot = ""; let repositoryRoot = ""; @@ -54,22 +54,61 @@ describe("security publication", () => { startedAt: "2026-07-29T00:00:00.000Z", }); await expect( - tool.execute("tool-call", { - findings: [ - { - rule_id: "fixture.rule", - title: "Fixture finding", - summary: "Fixture summary", - severity: "high", - confidence: "high", - category: "fixture", - locations: [{ path: invalidPath, start_line: 1 }], - }, - ], - coverage: { completeness: "partial" }, - report: "# Fixture\n", - }), + tool.execute( + "tool-call", + { + findings: [ + { + rule_id: "fixture.rule", + title: "Fixture finding", + summary: "Fixture summary", + severity: "high", + confidence: "high", + category: "fixture", + locations: [{ path: invalidPath, start_line: 1 }], + }, + ], + coverage: { completeness: "partial" }, + report: "# Fixture\n", + }, + undefined, + undefined, + undefined as never, + ), ).rejects.toThrow("repository-relative"); } }); + + test("allows only one publication while persistence is in flight", async () => { + const putStarted = Promise.withResolvers(); + const releasePut = Promise.withResolvers(); + let putCalls = 0; + const delayedStore = { + projectKey: store.projectKey, + putBundle: async () => { + putCalls++; + putStarted.resolve(); + await releasePut.promise; + }, + } as unknown as SecurityStore; + const tool = createSecurityPublicationTool({ + plan, + scanId: "secscan_fixture", + store: delayedStore, + startedAt: "2026-07-29T00:00:00.000Z", + }); + const params = { + findings: [], + coverage: { completeness: "complete" as const }, + report: "# Fixture\n", + }; + const first = tool.execute("first", params, undefined, undefined, undefined as never); + await putStarted.promise; + await expect(tool.execute("second", params, undefined, undefined, undefined as never)).rejects.toThrow( + "already been published", + ); + expect(putCalls).toBe(1); + releasePut.resolve(); + await first; + }); }); diff --git a/packages/coding-agent/test/security/remediation.test.ts b/packages/coding-agent/test/security/remediation.test.ts index 403084071..636c707be 100644 --- a/packages/coding-agent/test/security/remediation.test.ts +++ b/packages/coding-agent/test/security/remediation.test.ts @@ -1,9 +1,6 @@ import { describe, expect, test } from "bun:test"; import { IsoBackendKind } from "@oh-my-pi/pi-natives"; -import { - assertSecurityRemediationBaselineClean, - prepareSecurityRemediationWorkspace, -} from "../../src/security"; +import { assertSecurityRemediationBaselineClean, prepareSecurityRemediationWorkspace } from "../../src/security"; import type { IsolationContext } from "../../src/task/isolation-runner"; import type { IsolationHandle, WorktreeBaseline } from "../../src/task/worktree"; diff --git a/packages/coding-agent/test/security/slash-command.test.ts b/packages/coding-agent/test/security/slash-command.test.ts index e53f60caf..626abbbcd 100644 --- a/packages/coding-agent/test/security/slash-command.test.ts +++ b/packages/coding-agent/test/security/slash-command.test.ts @@ -73,6 +73,24 @@ describe("/security", () => { status: "false_positive", rationale: "fixture rationale", }); + + await command(`disposition ${scanId} ${finding.id} open`); + expect((await store.getFinding(scanId, finding.id))?.disposition).toMatchObject({ + status: "open", + actor: "operator", + }); + expect((await store.getFinding(scanId, finding.id))?.disposition.rationale).toBeUndefined(); + }); + + test("export preserves permissions on an existing destination directory", async () => { + if (process.platform === "win32") return; + await fs.chmod(repositoryRoot, 0o755); + await command(`import ${JSON.stringify(SARIF_FIXTURE)}`); + const [scan] = await (await SecurityStore.open(repositoryRoot)).listScans(); + if (!scan) throw new Error("expected imported scan"); + await command(`export ${scan.id} --output exported.sarif --format sarif`); + expect((await fs.stat(repositoryRoot)).mode & 0o777).toBe(0o755); + expect(JSON.parse(await Bun.file(path.join(repositoryRoot, "exported.sarif")).text())).toHaveProperty("version"); }); test("validate returns a static OMP-native residual prompt", async () => { diff --git a/packages/coding-agent/test/task/structured-subagent.test.ts b/packages/coding-agent/test/task/structured-subagent.test.ts index 18719e2c5..371a4d2e6 100644 --- a/packages/coding-agent/test/task/structured-subagent.test.ts +++ b/packages/coding-agent/test/task/structured-subagent.test.ts @@ -327,9 +327,7 @@ describe("structured subagent primitive", () => { const mcpDisabledRun = await runStructuredSubagent( request({ session: mcpDisabledSession, retainArtifacts: true }), ); - const restrictedRun = await runStructuredSubagent( - request({ session: restrictedSession, retainArtifacts: true }), - ); + const restrictedRun = await runStructuredSubagent(request({ session: restrictedSession, retainArtifacts: true })); expect(options[0]).toMatchObject({ enableMCP: false,