diff --git a/scripts/security/run-codex-scan-worker-clawscan.test.ts b/scripts/security/run-codex-scan-worker-clawscan.test.ts index 5021f04d..24f3be90 100644 --- a/scripts/security/run-codex-scan-worker-clawscan.test.ts +++ b/scripts/security/run-codex-scan-worker-clawscan.test.ts @@ -7,7 +7,12 @@ import { join } from "node:path"; import { promisify } from "node:util"; import { afterEach, describe, expect, it, vi } from "vitest"; import type { ClaimedJob } from "./run-codex-scan-worker"; -import { processJob, runClawScan } from "./run-codex-scan-worker"; +import { + aggregateSkillSpectorAnalyses, + processJob, + resolveBundledSkillSpectorScanInputs, + runClawScan, +} from "./run-codex-scan-worker"; const tempDirs: string[] = []; const execFileAsync = promisify(execFile); @@ -88,7 +93,12 @@ function claimedJob(input: { target: { ...input.target, ...(input.targetKind === "packageRelease" - ? { release: { vtAnalysis: input.vtAnalysis ?? null } } + ? { + release: { + ...(input.target.release ?? {}), + vtAnalysis: input.vtAnalysis ?? null, + }, + } : { version: { vtAnalysis: input.vtAnalysis ?? null } }), }, }; @@ -220,6 +230,216 @@ function clawScanArtifactJson(options?: { } describe("run-codex-scan-worker clawscan authority", () => { + it.each([ + { + name: "zero roots", + roots: [], + expected: [], + }, + { + name: "one root", + roots: ["skills/alpha"], + expected: [{ rootPath: "skills/alpha", scanPath: "artifact/package/skills/alpha" }], + }, + { + name: "multiple roots in deterministic order", + roots: ["skills/zeta/", "./skills/alpha", "skills/zeta"], + expected: [ + { rootPath: "skills/alpha", scanPath: "artifact/package/skills/alpha" }, + { rootPath: "skills/zeta", scanPath: "artifact/package/skills/zeta" }, + ], + }, + { + name: "traversal roots rejected", + roots: ["../outside", "skills/../../outside", "/absolute", ".", "skills/safe"], + expected: [{ rootPath: "skills/safe", scanPath: "artifact/package/skills/safe" }], + }, + ])("selects $name only from the package manifest", async ({ name, roots, expected }) => { + const workspace = await tempDir(); + await mkdir(join(workspace, "artifact", "package"), { recursive: true }); + await writeFile(join(workspace, "artifact", "package", "package.json"), "{}\n"); + const job = claimedJob({ + jobId: `securityScanJobs:${name.replaceAll(" ", "-")}`, + source: "publish", + targetKind: "packageRelease", + target: { + release: { + pluginManifestSummary: { + bundledSkills: roots.map((rootPath) => ({ rootPath })), + }, + }, + }, + }); + + await expect(resolveBundledSkillSpectorScanInputs(workspace, job)).resolves.toEqual(expected); + }); + + it("produces an explicit result when a package has no bundled skills", () => { + expect(aggregateSkillSpectorAnalyses([])).toEqual({ + applicable: false, + status: "clean", + risk_assessment: { + score: 0, + severity: "NONE", + recommendation: "NOT_APPLICABLE", + }, + issue_count: 0, + filtered_findings: [], + metadata: { skillspector_version: "skillspector" }, + summary: "Package declares no bundled skills; SkillSpector was not applicable.", + }); + }); + + it("aggregates bundled SkillSpector reports deterministically", () => { + const analyses = [ + { + status: "clean", + score: 5, + severity: "LOW", + recommendation: "ALLOW", + issueCount: 0, + issues: [], + scannerVersion: "2.0.0", + summary: "alpha clean", + checkedAt: 20, + }, + { + status: "suspicious", + score: 80, + severity: "HIGH", + recommendation: "REVIEW", + issueCount: 1, + issues: [ + { + issueId: "SDI-2", + severity: "HIGH", + explanation: "review beta", + }, + ], + scannerVersion: "1.0.0", + summary: "beta suspicious", + checkedAt: 10, + }, + ]; + + expect(aggregateSkillSpectorAnalyses(analyses)).toEqual( + aggregateSkillSpectorAnalyses([...analyses].reverse()), + ); + expect(aggregateSkillSpectorAnalyses(analyses)).toMatchObject({ + applicable: true, + status: "suspicious", + risk_assessment: { + score: 80, + severity: "HIGH", + recommendation: "ALLOW; REVIEW", + }, + issue_count: 1, + metadata: { skillspector_version: "1.0.0, 2.0.0" }, + summary: "Scanned 2 bundled skills. alpha clean beta suspicious", + }); + }); + + it("never passes a package root to SkillSpector", async () => { + const workspace = await tempDir(); + const packageRoot = join(workspace, "artifact", "package"); + await mkdir(join(packageRoot, "skills", "alpha"), { recursive: true }); + await mkdir(join(packageRoot, "skills", "beta"), { recursive: true }); + await writeFile(join(packageRoot, "package.json"), "{}\n"); + await writeFile(join(packageRoot, "skills", "alpha", "SKILL.md"), "# alpha\n"); + await writeFile(join(packageRoot, "skills", "beta", "SKILL.md"), "# beta\n"); + + const fakeSkillSpector = join(workspace, "skillspector"); + const skillSpectorTargets = join(workspace, "skillspector-targets.log"); + await writeFakeClawScanCommand( + fakeSkillSpector, + `target="$2" +printf '%s\\n' "$target" >> ${JSON.stringify(skillSpectorTargets)} +out="" +while [[ $# -gt 0 ]]; do + case "$1" in + --output) + out="$2" + shift 2 + ;; + *) + shift + ;; + esac +done +cat > "$out" < "$out" <<'JSON' +${clawScanArtifactJson()} +JSON`, + ); + + const previousClawScan = process.env.CODEX_SECURITY_SCAN_CLAWSCAN_COMMAND; + const previousPath = process.env.PATH; + process.env.CODEX_SECURITY_SCAN_CLAWSCAN_COMMAND = fakeClawScan; + process.env.PATH = `${workspace}:${previousPath ?? ""}`; + try { + const job = claimedJob({ + jobId: "securityScanJobs:bundled-roots-only", + source: "publish", + targetKind: "packageRelease", + target: { + release: { + pluginManifestSummary: { + bundledSkills: [{ rootPath: "skills/beta" }, { rootPath: "skills/alpha" }], + }, + }, + }, + }); + + await runClawScan(job, workspace, () => {}); + + expect((await readFile(skillSpectorTargets, "utf8")).trim().split("\n")).toEqual([ + "artifact/package/skills/alpha", + "artifact/package/skills/beta", + ]); + expect(JSON.parse(await readFile(copiedFixture, "utf8"))).toMatchObject({ + applicable: true, + status: "suspicious", + issue_count: 2, + filtered_findings: [{ file: "skills/alpha/SKILL.md" }, { file: "skills/beta/SKILL.md" }], + summary: "Scanned 2 bundled skills. alpha suspicious beta suspicious", + }); + } finally { + if (previousClawScan === undefined) delete process.env.CODEX_SECURITY_SCAN_CLAWSCAN_COMMAND; + else process.env.CODEX_SECURITY_SCAN_CLAWSCAN_COMMAND = previousClawScan; + if (previousPath === undefined) { + delete process.env.PATH; + } else { + process.env.PATH = previousPath; + } + } + }); + it("uses ClawScan as the only skillVersion scan implementation", async () => { const workspace = await tempDir(); const fakeClawScan = join(workspace, "fake-clawscan"); @@ -483,7 +703,12 @@ JSON`, expect.arrayContaining(["--profile", "clawhub", "--output"]), ); expect(invocationArgs).not.toContain("--context"); - expect(invocationArgs).not.toContain("--scanner-result"); + if (targetKind === "packageRelease") { + expect(invocationArgs).toContain("--scanner-result"); + expect(invocationArgs.some((arg) => arg.startsWith("skillspector="))).toBe(true); + } else { + expect(invocationArgs).not.toContain("--scanner-result"); + } expect((await readFile(filesLog, "utf8")).trim().split("\n")).toContain(expectedFile); } finally { if (previousCommand === undefined) delete process.env.CODEX_SECURITY_SCAN_CLAWSCAN_COMMAND; diff --git a/scripts/security/run-codex-scan-worker.source-test.mjs b/scripts/security/run-codex-scan-worker.source-test.mjs new file mode 100644 index 00000000..28b89315 --- /dev/null +++ b/scripts/security/run-codex-scan-worker.source-test.mjs @@ -0,0 +1,27 @@ +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import test from "node:test"; + +const source = readFileSync(new URL("./run-codex-scan-worker.ts", import.meta.url), "utf8"); +const functionStart = source.indexOf("async function runBundledSkillSpector("); +const functionEnd = source.indexOf("\nconst REQUIRED_CLAWHUB_RESULT_KEYS", functionStart); +const functionSource = source.slice(functionStart, functionEnd); + +test("bundled SkillSpector roots share one deadline", () => { + const deadlineIndex = functionSource.indexOf( + "const deadlineMs = Date.now() + clawScanTimeoutMs();", + ); + const loopIndex = functionSource.indexOf("for (const [index, scanInput]"); + assert.ok(deadlineIndex >= 0, "expected a shared deadline"); + assert.ok(loopIndex > deadlineIndex, "deadline must be captured before the per-root loop"); + assert.match(functionSource, /const remainingMs = deadlineMs - Date\.now\(\);/); +}); + +test("expired budgets fail before starting another root", () => { + const expiryIndex = functionSource.indexOf("if (remainingMs <= 0)"); + const commandIndex = functionSource.indexOf('await runCommand("skillspector"'); + assert.ok(expiryIndex >= 0, "expected an exhausted-budget guard"); + assert.ok(commandIndex > expiryIndex, "budget guard must run before SkillSpector"); + assert.match(functionSource, /timeoutMs: remainingMs/); + assert.doesNotMatch(functionSource, /timeoutMs: clawScanTimeoutMs\(\)/); +}); diff --git a/scripts/security/run-codex-scan-worker.ts b/scripts/security/run-codex-scan-worker.ts index d9662d09..f87fc809 100644 --- a/scripts/security/run-codex-scan-worker.ts +++ b/scripts/security/run-codex-scan-worker.ts @@ -2,7 +2,7 @@ import { spawn } from "node:child_process"; import { mkdirSync, readFileSync } from "node:fs"; import { appendFile, mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { basename, dirname, join, resolve } from "node:path"; +import { basename, dirname, join, resolve, sep } from "node:path"; import { pathToFileURL } from "node:url"; import { ConvexHttpClient } from "convex/browser"; import { api } from "../../convex/_generated/api"; @@ -92,6 +92,28 @@ export type SkillSpectorAnalysis = { checkedAt: number; }; +type SkillSpectorScannerResult = { + applicable: boolean; + error?: string; + filtered_findings: SkillSpectorIssue[]; + issue_count: number; + metadata?: { + skillspector_version: string; + }; + risk_assessment: { + recommendation?: string; + score?: number; + severity?: string; + }; + status: string; + summary?: string; +}; + +type BundledSkillSpectorScanInput = { + rootPath: string; + scanPath: string; +}; + type ClawScanCommandDiagnostic = { args?: string[]; artifactPath?: string; @@ -1156,6 +1178,230 @@ function clawScanTimeoutMs() { return Number.isFinite(parsed) && parsed > 0 ? parsed : DEFAULT_CLAWSCAN_TIMEOUT_MS; } +function normalizedBundledSkillRoot(value: unknown) { + if (typeof value !== "string") return null; + const normalized = value + .trim() + .replaceAll("\\", "/") + .replace(/^\.\/+/, "") + .replace(/\/+$/, ""); + if ( + !normalized || + normalized === "." || + normalized.startsWith("/") || + normalized.split("/").some((segment) => segment === "..") + ) { + return null; + } + return normalized; +} + +function bundledSkillRootsForJob(job: ClaimedJob) { + if (job.job.targetKind !== "packageRelease") return []; + const release = asRecord(job.target.release); + const pluginManifestSummary = asRecord(release?.pluginManifestSummary); + const bundledSkills = pluginManifestSummary?.bundledSkills; + if (!Array.isArray(bundledSkills)) return []; + return [ + ...new Set( + bundledSkills + .map((skill) => normalizedBundledSkillRoot(asRecord(skill)?.rootPath)) + .filter((rootPath): rootPath is string => Boolean(rootPath)), + ), + ].sort(); +} + +export async function resolveBundledSkillSpectorScanInputs(workspace: string, job: ClaimedJob) { + if (job.job.targetKind !== "packageRelease") return []; + const packageRoot = await resolveClawScanTarget(workspace, job); + const artifactRoot = resolve(workspace, packageRoot); + return bundledSkillRootsForJob(job) + .map((rootPath) => { + const skillRoot = resolve(artifactRoot, rootPath); + return skillRoot.startsWith(`${artifactRoot}${sep}`) + ? { rootPath, scanPath: join(packageRoot, rootPath) } + : null; + }) + .filter((input): input is BundledSkillSpectorScanInput => Boolean(input)); +} + +function packageRelativeSkillSpectorIssueFile(rootPath: string, file: string) { + const normalized = file + .trim() + .replaceAll("\\", "/") + .replace(/^\.\/+/, ""); + if (normalized === rootPath || normalized.startsWith(`${rootPath}/`)) return normalized; + const rootedSuffix = `/${rootPath}/`; + const rootedIndex = normalized.lastIndexOf(rootedSuffix); + if (rootedIndex >= 0) return normalized.slice(rootedIndex + 1); + const relative = normalized + .split("/") + .filter((segment) => segment && segment !== "." && segment !== "..") + .join("/"); + return relative ? `${rootPath}/${relative}` : rootPath; +} + +function prefixSkillSpectorFindingPaths( + analysis: SkillSpectorAnalysis, + rootPath: string, +): SkillSpectorAnalysis { + return { + ...analysis, + issues: analysis.issues.map((issue) => + issue.file + ? { + ...issue, + file: packageRelativeSkillSpectorIssueFile(rootPath, issue.file), + } + : issue, + ), + }; +} + +function compareSkillSpectorIssues(left: SkillSpectorIssue, right: SkillSpectorIssue) { + return JSON.stringify(left).localeCompare(JSON.stringify(right)); +} + +export function aggregateSkillSpectorAnalyses( + analyses: SkillSpectorAnalysis[], +): SkillSpectorScannerResult { + if (analyses.length === 0) { + return { + applicable: false, + status: "clean", + risk_assessment: { + score: 0, + severity: "NONE", + recommendation: "NOT_APPLICABLE", + }, + issue_count: 0, + filtered_findings: [], + metadata: { skillspector_version: "skillspector" }, + summary: "Package declares no bundled skills; SkillSpector was not applicable.", + }; + } + if (analyses.length === 1) { + const [analysis] = analyses; + return { + applicable: true, + status: analysis.status, + risk_assessment: { + score: analysis.score, + severity: analysis.severity, + recommendation: analysis.recommendation, + }, + issue_count: analysis.issueCount, + filtered_findings: analysis.issues, + metadata: analysis.scannerVersion + ? { skillspector_version: analysis.scannerVersion } + : undefined, + summary: analysis.summary, + error: analysis.error, + }; + } + + const statuses = analyses.map((analysis) => analysis.status); + const status = statuses.some((value) => value === "error" || value === "failed") + ? "error" + : statuses.includes("malicious") + ? "malicious" + : statuses.includes("suspicious") + ? "suspicious" + : "clean"; + const severityRank = ["NONE", "UNKNOWN", "LOW", "MEDIUM", "HIGH", "CRITICAL"]; + const severity = analyses + .map((analysis) => analysis.severity?.toUpperCase()) + .filter((value): value is string => Boolean(value)) + .sort( + (left, right) => + severityRank.indexOf(right) - severityRank.indexOf(left) || left.localeCompare(right), + )[0]; + const recommendations = [ + ...new Set(analyses.map((analysis) => analysis.recommendation).filter(Boolean)), + ].sort(); + const scannerVersions = [ + ...new Set(analyses.map((analysis) => analysis.scannerVersion).filter(Boolean)), + ].sort(); + const summaries = analyses + .map((analysis) => analysis.summary) + .filter((summary): summary is string => Boolean(summary)) + .sort(); + const errors = analyses + .map((analysis) => analysis.error) + .filter((error): error is string => Boolean(error)) + .sort(); + return { + applicable: true, + status, + risk_assessment: { + score: Math.max(...analyses.map((analysis) => analysis.score ?? 0)), + severity, + recommendation: recommendations.length > 0 ? recommendations.join("; ") : undefined, + }, + issue_count: analyses.reduce((total, analysis) => total + analysis.issueCount, 0), + filtered_findings: analyses + .flatMap((analysis) => analysis.issues) + .sort(compareSkillSpectorIssues) + .slice(0, MAX_STORED_SKILLSPECTOR_ISSUES), + metadata: + scannerVersions.length > 0 ? { skillspector_version: scannerVersions.join(", ") } : undefined, + summary: + summaries.length > 0 + ? `Scanned ${analyses.length} bundled skills. ${summaries.join(" ")}` + : `Scanned ${analyses.length} bundled skills.`, + error: errors.length > 0 ? errors.join("; ") : undefined, + }; +} + +async function runBundledSkillSpector( + workspace: string, + scanInputs: BundledSkillSpectorScanInput[], +) { + const analyses: SkillSpectorAnalysis[] = []; + const deadlineMs = Date.now() + clawScanTimeoutMs(); + for (const [index, scanInput] of scanInputs.entries()) { + const remainingMs = deadlineMs - Date.now(); + if (remainingMs <= 0) { + throw new Error("SkillSpector bundled-skill scan deadline exceeded"); + } + const resultPath = join(workspace, `skillspector-report-${index}.json`); + const args = ["scan", scanInput.scanPath, "--format", "json", "--output", resultPath]; + try { + await runCommand("skillspector", args, { + cwd: workspace, + timeoutMs: remainingMs, + }); + analyses.push( + prefixSkillSpectorFindingPaths( + normalizeSkillSpectorAnalysis(await readFile(resultPath, "utf8"), 0), + scanInput.rootPath, + ), + ); + } catch (error) { + if (error instanceof CommandFailure) { + const rawResult = await readFile(resultPath, "utf8").catch(() => undefined); + if (rawResult) { + const analysis = prefixSkillSpectorFindingPaths( + normalizeSkillSpectorAnalysis(rawResult, 0), + scanInput.rootPath, + ); + const validFindingsExit = + error.exitCode === 1 && + (analysis.status === "suspicious" || analysis.status === "malicious") && + analysis.issueCount > 0 && + analysis.issues.length > 0; + if (validFindingsExit) { + analyses.push(analysis); + continue; + } + } + } + throw error; + } + } + return aggregateSkillSpectorAnalyses(analyses); +} + const REQUIRED_CLAWHUB_RESULT_KEYS = [ ...CLAWHUB_OUTPUT_SCHEMA_CONTRACT.requiredResultKeys, "artifact_inspection", @@ -1383,7 +1629,21 @@ export async function runClawScan( const command = process.env.CODEX_SECURITY_SCAN_CLAWSCAN_COMMAND ?? "clawscan"; const artifactPath = join(workspace, "clawscan-artifact.json"); const target = await resolveClawScanTarget(workspace, job); - const args = [target, "--profile", "clawhub", "--output", artifactPath]; + const args = [target, "--profile", "clawhub"]; + if (job.job.targetKind === "packageRelease") { + // SkillSpector only understands skills. ClawHub owns the plugin manifest + // boundary, so never let ClawScan fall back to scanning the whole package. + const scanInputs = await resolveBundledSkillSpectorScanInputs(workspace, job); + const skillSpectorResultPath = join(workspace, "skillspector-aggregate.json"); + const skillSpectorResult = await runBundledSkillSpector(workspace, scanInputs); + await writeFile( + skillSpectorResultPath, + `${JSON.stringify(skillSpectorResult, null, 2)}\n`, + "utf8", + ); + args.push("--scanner-result", `skillspector=${skillSpectorResultPath}`); + } + args.push("--output", artifactPath); onDiagnostic({ args: [command, ...args], artifactPath }); const captureArtifact = async () => { diff --git a/specs/security-moderation.md b/specs/security-moderation.md index 9a5dbfcd..24e61cdf 100644 --- a/specs/security-moderation.md +++ b/specs/security-moderation.md @@ -326,6 +326,11 @@ See also: [acceptable-usage.md](./acceptable-usage.md) for the marketplace polic - ClawScan verdicts come from a GitHub Actions Codex worker, not a single hosted LLM call. Codex reviews the materialized artifact workspace with SkillSpector and static scan evidence as context. +- In the external Codex security worker, package-release SkillSpector runs scan + only normalized bundled-skill roots declared by the stored plugin manifest + summary. The plugin package root is never a fallback SkillSpector target; + packages with no bundled roots provide ClawScan an explicit not-applicable + result. This is not a prepublication-worker contract. - Current skill and plugin scans are queued through `securityScanJobs` and completed by the external Codex worker. - VirusTotal telemetry remains a separate Security audit signal and is not an