From 80bdfdcbd46100b8d4180632d3af8b885f3e9af1 Mon Sep 17 00:00:00 2001 From: Kyle McCleary Date: Wed, 29 Jul 2026 18:47:51 -0700 Subject: [PATCH] fix(security): stabilize imported finding identities --- .../coding-agent/scripts/security-compare.ts | 11 ++- packages/coding-agent/src/security/auth.ts | 27 ++++++-- .../src/security/contracts/ids.ts | 26 ++++++- .../src/security/importers/codex-security.ts | 29 +++++++- .../src/security/importers/sarif.ts | 68 +++++++++++++++++-- packages/coding-agent/src/security/sarif.ts | 3 +- .../coding-agent/test/security/auth.test.ts | 58 +++++++++++++++- .../test/security/contracts.test.ts | 23 +++++++ .../test/security/importers-store.test.ts | 34 +++++++++- .../test/security/seeded-fixture.test.ts | 14 +++- 10 files changed, 266 insertions(+), 27 deletions(-) diff --git a/packages/coding-agent/scripts/security-compare.ts b/packages/coding-agent/scripts/security-compare.ts index 969d1f4ae..99f248616 100755 --- a/packages/coding-agent/scripts/security-compare.ts +++ b/packages/coding-agent/scripts/security-compare.ts @@ -1,10 +1,17 @@ #!/usr/bin/env bun import * as path from "node:path"; -import { compareSecurityProducers, parseSecurityScanBundle } from "../src/security"; +import { isEnoent } from "@oh-my-pi/pi-utils"; +import { compareSecurityProducers, importCodexSecurityBundle, parseSecurityScanBundle } from "../src/security"; 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; + let scan: unknown; + try { + scan = JSON.parse(await Bun.file(path.join(root, "scan.json")).text()) as unknown; + } catch (error) { + if (!isEnoent(error)) throw error; + return importCodexSecurityBundle(root, { repositoryRoot: root }); + } 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() diff --git a/packages/coding-agent/src/security/auth.ts b/packages/coding-agent/src/security/auth.ts index e16a9eadd..d5dd85a5f 100644 --- a/packages/coding-agent/src/security/auth.ts +++ b/packages/coding-agent/src/security/auth.ts @@ -8,9 +8,24 @@ export interface ExactSecurityOAuthOptions { account: SecurityAccountRef; } -function assertIdentityMatches(account: SecurityAccountRef, resolvedAccountId: string | undefined): void { - if (account.accountId !== undefined && account.accountId !== resolvedAccountId) { - throw new Error("Security scan account mismatch: the pinned workspace identity changed during authentication"); +function assertIdentityMatches( + account: SecurityAccountRef, + resolution: { + credentialId?: number; + accountId?: string; + email?: string; + orgId?: string; + orgName?: string; + }, +): void { + if ( + account.credentialId !== resolution.credentialId || + (account.accountId !== undefined && account.accountId !== resolution.accountId) || + (account.email !== undefined && account.email !== resolution.email) || + (account.organizationId !== undefined && account.organizationId !== resolution.orgId) || + (account.organizationName !== undefined && account.organizationName !== resolution.orgName) + ) { + throw new Error("Security scan authentication identity mismatch"); } } @@ -27,9 +42,7 @@ export function createExactSecurityOAuthResolver( const { account, authStorage } = options; return model => { if (model.provider !== account.provider) { - throw new Error( - `Security scan model provider ${model.provider} does not match pinned account provider ${account.provider}`, - ); + throw new Error("Security scan authentication provider mismatch"); } const resolver: ApiKeyResolver = async context => { if (context.lastChance) return undefined; @@ -40,10 +53,10 @@ export function createExactSecurityOAuthResolver( if (!resolution) { throw new Error("The pinned security OAuth credential is unavailable"); } + assertIdentityMatches(account, resolution); if (!resolution.ok) { throw new Error("The pinned security OAuth credential could not be resolved"); } - assertIdentityMatches(account, resolution.accountId); return resolution.accessToken; }; return resolver; diff --git a/packages/coding-agent/src/security/contracts/ids.ts b/packages/coding-agent/src/security/contracts/ids.ts index 8f2e85a19..bebbed99b 100644 --- a/packages/coding-agent/src/security/contracts/ids.ts +++ b/packages/coding-agent/src/security/contracts/ids.ts @@ -24,6 +24,24 @@ function normalizeFingerprintPath(value: string): string { return value.replaceAll("\\", "/").replace(/^\.\//, ""); } +function compareNormalizedLocationValues( + left: string | number | undefined, + right: string | number | undefined, +): number { + if (left === right) return 0; + if (left === undefined) return -1; + if (right === undefined) return 1; + if (typeof left === "number" && typeof right === "number") { + const leftNaN = Number.isNaN(left); + const rightNaN = Number.isNaN(right); + if (leftNaN || rightNaN) return leftNaN ? (rightNaN ? 0 : -1) : 1; + return left - right; + } + return String(left) < String(right) ? -1 : 1; +} + +const NORMALIZED_LOCATION_SORT_KEYS = ["path", "startLine", "endLine", "startColumn", "endColumn", "role"] as const; + function normalizedLocations( locations: readonly SecurityLocation[], ): Array> { @@ -37,9 +55,11 @@ function normalizedLocations( role: location.role, })) .sort((left, right) => { - const byPath = String(left.path).localeCompare(String(right.path)); - if (byPath !== 0) return byPath; - return Number(left.startLine) - Number(right.startLine); + for (const key of NORMALIZED_LOCATION_SORT_KEYS) { + const comparison = compareNormalizedLocationValues(left[key], right[key]); + if (comparison !== 0) return comparison; + } + return 0; }); } diff --git a/packages/coding-agent/src/security/importers/codex-security.ts b/packages/coding-agent/src/security/importers/codex-security.ts index 05110a6ad..4600caddc 100644 --- a/packages/coding-agent/src/security/importers/codex-security.ts +++ b/packages/coding-agent/src/security/importers/codex-security.ts @@ -123,6 +123,22 @@ function locationsForFinding(finding: CodexFinding): SecurityLocation[] { } return locations.length > 0 ? locations : [{ path: "unknown", startLine: 1, role: "unknown" }]; } +const canonicalValidationStatuses: Record = { + unvalidated: true, + validated: true, + rejected: true, + partial: true, + error: true, +}; + +function validationStatus( + validation: Record | null | undefined, +): SecurityFinding["validation"]["status"] { + const status = validation?.status; + return typeof status === "string" && canonicalValidationStatuses[status] === true + ? (status as SecurityFinding["validation"]["status"]) + : "unvalidated"; +} function mapCoverage(document: CodexCoverageDocument): SecurityCoverage { const allowedModes = new Set(["repository", "scoped_path", "diff", "working_tree", "deep_repository"]); @@ -205,11 +221,18 @@ export async function importCodexSecurityBundle( for (const source of findingsDocument.findings ?? []) { const ruleId = source.ruleId || "codex-security.unknown"; const category = source.taxonomy?.category || ruleId.split(/[./-]/)[0] || "security"; + const hasSourceLocations = (source.locations ?? []).some( + location => typeof location.path === "string" && typeof location.startLine === "number", + ); const locations = locationsForFinding(source); + const anchor = + source.identity?.anchor || + source.fingerprints?.primary || + (!hasSourceLocations ? source.findingId : undefined); const fingerprint = createSecurityFindingFingerprint({ ruleId, category, - anchor: source.identity?.anchor, + anchor, locations, }); const evidence: SecurityEvidence[] = (source.codeEvidence ?? []).map((item, index) => { @@ -272,13 +295,13 @@ export async function importCodexSecurityBundle( ], evidence, validation: { - status: source.validation ? "validated" : "unvalidated", + status: validationStatus(source.validation), evidenceIds: [], }, disposition: { status: "open" }, provenance, }; - if (source.identity?.anchor !== undefined) finding.anchor = source.identity.anchor; + if (anchor !== undefined) finding.anchor = 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; diff --git a/packages/coding-agent/src/security/importers/sarif.ts b/packages/coding-agent/src/security/importers/sarif.ts index ce612f38e..1a8cf770d 100644 --- a/packages/coding-agent/src/security/importers/sarif.ts +++ b/packages/coding-agent/src/security/importers/sarif.ts @@ -10,6 +10,7 @@ import type { SecuritySeverityLevel, } from "../contracts"; import { + canonicalSecurityJson, createSecurityFindingFingerprint, createSecurityFindingId, createSecurityOccurrenceId, @@ -119,6 +120,57 @@ function tagsForRule(rule: SarifRule | undefined): string[] { const tags = rule?.properties?.tags; return Array.isArray(tags) ? tags.filter((tag): tag is string => typeof tag === "string") : []; } +function selectFirstVendorFingerprint(vendorFingerprints: Record): string | undefined { + for (const [, value] of Object.entries(vendorFingerprints).sort(([left], [right]) => + left < right ? -1 : left > right ? 1 : 0, + )) { + if (value) return value; + } + return undefined; +} + +function semanticResultAnchor( + ruleId: string, + category: string, + message: string, + locations: readonly SecurityLocation[], +): string { + return `sarif-result/v1:sha256:${securitySha256( + canonicalSecurityJson({ + ruleId, + category, + message, + locations, + }), + )}`; +} +const canonicalValidationStatuses: Record = { + unvalidated: true, + validated: true, + rejected: true, + partial: true, + error: true, +}; + +const canonicalDispositionStatuses: Record = { + open: true, + false_positive: true, + accepted_risk: true, + fixed: true, + wont_fix: true, +}; + +function importedValidationStatus(value: unknown): SecurityFinding["validation"]["status"] { + return typeof value === "string" && canonicalValidationStatuses[value] === true + ? (value as SecurityFinding["validation"]["status"]) + : "unvalidated"; +} + +function importedDispositionStatus(value: unknown): SecurityFinding["disposition"]["status"] { + return typeof value === "string" && canonicalDispositionStatuses[value] === true + ? (value as SecurityFinding["disposition"]["status"]) + : "open"; +} export async function importSarif(input: unknown, options: SarifImportOptions): Promise { const sarif = input as SarifLog; @@ -129,6 +181,7 @@ export async function importSarif(input: unknown, options: SarifImportOptions): const scanId = options.createScanId?.() ?? createSecurityScanId(); const createdAt = options.createdAt ?? new Date().toISOString(); const findings: SecurityFinding[] = []; + const seenFingerprints = new Set(); let producerName = "SARIF importer"; let producerVersion: string | undefined; @@ -145,17 +198,21 @@ export async function importSarif(input: unknown, options: SarifImportOptions): ...stringRecord(result.fingerprints), ...stringRecord(result.partialFingerprints), }; - const firstVendorFingerprint = Object.values(vendorFingerprints)[0]; + const firstVendorFingerprint = selectFirstVendorFingerprint(vendorFingerprints); const category = typeof result.properties?.category === "string" ? result.properties.category : ruleId.split(/[./-]/)[0] || "security"; + const message = result.message?.text ?? result.message?.markdown ?? rule?.shortDescription?.text ?? ruleId; + const anchor = firstVendorFingerprint ?? semanticResultAnchor(ruleId, category, message, locations); const fingerprint = createSecurityFindingFingerprint({ ruleId, category, - anchor: firstVendorFingerprint, + anchor, locations, }); + if (seenFingerprints.has(fingerprint)) continue; + seenFingerprints.add(fingerprint); const tags = tagsForRule(rule); const provenance: SecurityProvenance = { producer: { kind: "sarif-import", name: producerName }, @@ -165,7 +222,6 @@ export async function importSarif(input: unknown, options: SarifImportOptions): }; 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; const finding: SecurityFinding = { id: createSecurityFindingId(fingerprint), scanId, @@ -182,11 +238,11 @@ export async function importSarif(input: unknown, options: SarifImportOptions): }, occurrences: [{ id: createSecurityOccurrenceId(fingerprint, locations), locations, evidenceIds: [] }], evidence: [], - validation: { status: "unvalidated", evidenceIds: [] }, - disposition: { status: "open" }, + validation: { status: importedValidationStatus(result.properties?.validation), evidenceIds: [] }, + disposition: { status: importedDispositionStatus(result.properties?.disposition) }, provenance, }; - if (firstVendorFingerprint !== undefined) finding.anchor = firstVendorFingerprint; + finding.anchor = anchor; const score = Number(result.properties?.["security-severity"]); if (Number.isFinite(score)) finding.severity.score = score; findings.push(finding); diff --git a/packages/coding-agent/src/security/sarif.ts b/packages/coding-agent/src/security/sarif.ts index cba750dc3..1719229c2 100644 --- a/packages/coding-agent/src/security/sarif.ts +++ b/packages/coding-agent/src/security/sarif.ts @@ -1,3 +1,4 @@ +import { pathToFileURL } from "node:url"; import type { SecurityFinding, SecurityScanBundle } from "./contracts"; function sarifLevel(finding: SecurityFinding): "error" | "warning" | "note" | "none" { @@ -69,7 +70,7 @@ export function exportSecurityBundleToSarif(bundle: SecurityScanBundle): Record< }, })), originalUriBaseIds: { - "%SRCROOT%": { uri: `file://${bundle.scan.target.repositoryRoot.replaceAll("\\", "/")}/` }, + "%SRCROOT%": { uri: pathToFileURL(bundle.scan.target.repositoryRoot).href.replace(/\/?$/, "/") }, }, }, ], diff --git a/packages/coding-agent/test/security/auth.test.ts b/packages/coding-agent/test/security/auth.test.ts index 1e066e70a..0874de8a4 100644 --- a/packages/coding-agent/test/security/auth.test.ts +++ b/packages/coding-agent/test/security/auth.test.ts @@ -32,6 +32,60 @@ describe("exact security OAuth resolver", () => { expect(getOAuthAccessByCredentialId.mock.calls.map(call => call[1])).toEqual([42, 42]); }); + test("rejects a model whose provider crosses the pinned OAuth boundary", async () => { + const getOAuthAccessByCredentialId = vi.fn(async () => ({ + ok: true as const, + accessToken: "must-not-be-requested", + credentialId: 42, + accountId: "workspace-a", + })); + const authStorage = { getOAuthAccessByCredentialId } as unknown as AuthStorage; + const resolver = createExactSecurityOAuthResolver({ + authStorage, + account: { provider: "openai-codex", credentialId: 42, accountId: "workspace-a" }, + }); + const wrongProviderModel = { ...model(), provider: "anthropic" } as unknown as Parameters[0]; + expect(() => resolver(wrongProviderModel)).toThrow("provider mismatch"); + expect(getOAuthAccessByCredentialId).not.toHaveBeenCalled(); + }); + + test("fails closed when any durable account identity changes", async () => { + const account = { + provider: "openai-codex", + credentialId: 42, + accountId: "workspace-a", + email: "owner@example.com", + organizationId: "org-a", + organizationName: "Workspace A", + }; + const resolved = { + credentialId: 42, + accountId: "workspace-a", + email: "owner@example.com", + orgId: "org-a", + orgName: "Workspace A", + }; + for (const mismatch of [ + { credentialId: 99 }, + { accountId: "workspace-b" }, + { email: "other@example.com" }, + { orgId: "org-b" }, + { orgName: "Workspace B" }, + ]) { + const authStorage = { + getOAuthAccessByCredentialId: async () => ({ + ok: true as const, + accessToken: "token", + ...resolved, + ...mismatch, + }), + } as unknown as AuthStorage; + const resolver = createExactSecurityOAuthResolver({ authStorage, account }); + const exact = resolver(model()) as ApiKeyResolver; + await expect(exact({ lastChance: false, error: undefined })).rejects.toThrow("identity mismatch"); + } + }); + test("fails closed when the refreshed row loses its workspace identity", async () => { const authStorage = { getOAuthAccessByCredentialId: async () => ({ @@ -53,8 +107,8 @@ describe("exact security OAuth resolver", () => { caught = error; } expect(caught).toBeInstanceOf(Error); - if (!(caught instanceof Error)) throw new Error("expected account mismatch"); - expect(caught.message).toContain("account mismatch"); + if (!(caught instanceof Error)) throw new Error("expected identity mismatch"); + expect(caught.message).toContain("identity mismatch"); expect(caught.message).not.toContain("workspace-a"); expect(caught.message).not.toContain("undefined"); }); diff --git a/packages/coding-agent/test/security/contracts.test.ts b/packages/coding-agent/test/security/contracts.test.ts index 1ebe3b68a..e9dcc473c 100644 --- a/packages/coding-agent/test/security/contracts.test.ts +++ b/packages/coding-agent/test/security/contracts.test.ts @@ -66,6 +66,29 @@ describe("security contracts", () => { expect(createSecurityFindingId(first)).toBe(createSecurityFindingId(second)); }); + test("finding fingerprints are stable across every location ordering", () => { + const locations = [ + { path: "src/entry.ts", startLine: 4, endLine: 8, startColumn: 2, endColumn: 4, role: "source" }, + { path: "src/entry.ts", startLine: 4, endLine: 8, startColumn: 2, endColumn: 4, role: "sink" }, + { path: "src/entry.ts", startLine: 4, endLine: 9, startColumn: 1, endColumn: 3, role: "propagation" }, + { path: "src/entry.ts", startLine: 4, endLine: 10, startColumn: 1, endColumn: 3, role: "source" }, + ] as const; + const baseline = createSecurityFindingFingerprint({ + ruleId: "fixture.rule", + category: "fixture", + locations, + }); + for (const ordered of [locations.toReversed(), [locations[2], locations[0], locations[3], locations[1]]]) { + expect( + createSecurityFindingFingerprint({ + ruleId: "fixture.rule", + category: "fixture", + locations: ordered, + }), + ).toBe(baseline); + } + }); + test("scan IDs remain OMP-owned", () => { expect(createSecurityScanId(() => "018f0000-0000-7000-8000-000000000001")).toBe( "secscan_018f0000000070008000000000000001", diff --git a/packages/coding-agent/test/security/importers-store.test.ts b/packages/coding-agent/test/security/importers-store.test.ts index 8beac4d6b..f676f57df 100644 --- a/packages/coding-agent/test/security/importers-store.test.ts +++ b/packages/coding-agent/test/security/importers-store.test.ts @@ -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 { importCodexSecurityBundle, importSarifFile, SecurityStore } from "../../src/security"; +import { $ } from "bun"; +import { importCodexSecurityBundle, importSarif, importSarifFile, SecurityStore } from "../../src/security"; const FIXTURE_ROOT = path.join(import.meta.dir, "..", "fixtures", "security"); let temporaryRoot = ""; @@ -41,6 +42,37 @@ describe("security importers and store", () => { expect(sarif.scan.producer.kind).toBe("sarif-import"); }); + test("resolves one canonical store for a nested repository cwd", async () => { + const nestedCwd = path.join(repositoryRoot, "packages", "app"); + await fs.mkdir(nestedCwd, { recursive: true }); + const initialized = await $`git init --initial-branch=main`.cwd(repositoryRoot).quiet().nothrow(); + if (initialized.exitCode !== 0) throw new Error("git init failed"); + const store = await SecurityStore.openForCwd(nestedCwd, { stateRoot: path.join(temporaryRoot, "state") }); + expect(store.repositoryRoot).toBe(await fs.realpath(repositoryRoot)); + }); + + test("locationless SARIF keeps distinct results while deduplicating repeats", async () => { + const input = { + version: "2.1.0", + runs: [ + { + tool: { driver: { name: "Fixture scanner" } }, + results: [ + { ruleId: "fixture.rule", message: { text: "first result" } }, + { ruleId: "fixture.rule", message: { text: "second result" } }, + { ruleId: "fixture.rule", message: { text: "second result" } }, + ], + }, + ], + }; + const bundle = await importSarif(input, { + repositoryRoot, + createScanId: () => "secscan_locationless", + }); + expect(bundle.findings.map(finding => finding.summary)).toEqual(["first result", "second result"]); + expect(new Set(bundle.findings.map(finding => finding.id)).size).toBe(2); + }); + test("serializes concurrent index updates without losing scans", async () => { const store = await SecurityStore.open(repositoryRoot, { stateRoot: path.join(temporaryRoot, "state") }); const bundles = await Promise.all( diff --git a/packages/coding-agent/test/security/seeded-fixture.test.ts b/packages/coding-agent/test/security/seeded-fixture.test.ts index 52ea6a851..ef647429a 100644 --- a/packages/coding-agent/test/security/seeded-fixture.test.ts +++ b/packages/coding-agent/test/security/seeded-fixture.test.ts @@ -15,9 +15,19 @@ describe("security seeded validation repository", () => { expect(manifest.schemaVersion).toBe(1); expect(manifest.nonProduction).toBeTrue(); expect(manifest.seeds).toHaveLength(8); + const expectedClasses: Record = { + "command-injection": "command-injection", + "path-traversal": "path-traversal", + "sql-injection": "sql-injection", + ssrf: "ssrf", + "authorization-bypass": "authorization", + "unsafe-deserialization": "unsafe-deserialization", + "fake-secret": "hard-coded-secret", + "safe-lookalike": "path-traversal", + }; for (const seed of manifest.seeds) { - expect(seed.id.length).toBeGreaterThan(0); - expect(seed.expectedClass.length).toBeGreaterThan(0); + expect(seed.id in expectedClasses).toBeTrue(); + expect(expectedClasses[seed.id]).toBe(seed.expectedClass); expect(["finding", "no-finding"]).toContain(seed.expectedDisposition); expect(await Bun.file(path.join(ROOT, seed.path)).exists()).toBeTrue(); }