From ddd2bfc39cebe7c5e529b4a37afc3923fa90c0d3 Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Thu, 20 Aug 2026 17:14:10 -0700 Subject: [PATCH] fix(release): bound plugin security scanning --- scripts/lib/plugin-npm-security-scan.mts | 113 ++++++++++++++---- scripts/plugin-npm-security-scan.mts | 25 +++- test/scripts/plugin-npm-security-scan.test.ts | 96 ++++++++++++++- 3 files changed, 204 insertions(+), 30 deletions(-) diff --git a/scripts/lib/plugin-npm-security-scan.mts b/scripts/lib/plugin-npm-security-scan.mts index 2eb81e5d04af..a95335ecd7a5 100644 --- a/scripts/lib/plugin-npm-security-scan.mts +++ b/scripts/lib/plugin-npm-security-scan.mts @@ -17,6 +17,7 @@ import { scanDirectoryWithSummary, type SkillScanFinding, } from "../../src/skills/security/scanner.js"; +import { runTasksWithConcurrency } from "../../src/utils/run-with-concurrency.js"; type NpmPackFile = { path?: unknown; @@ -31,12 +32,18 @@ type PublishablePluginPackage = { packageName: string; }; +type CriticalFindingRecord = { + line: number; + path: string; + ruleId: string; +}; + type ScanPackageResult = { expectedReviewedCriticalFindings: string[]; packageName: string; packedFileCount: number; reviewedCriticalFindings: string[]; - unexpectedCriticalFindings: string[]; + unexpectedCriticalFindings: CriticalFindingRecord[]; }; export type PluginNpmSecurityScanReport = { @@ -55,19 +62,37 @@ export type PluginNpmSecurityScanReport = { }; const execFileAsync = promisify(execFile); +const MAX_SCANNABLE_FILES_PER_PACKAGE = 10_000; +const MAX_SCANNABLE_FILE_BYTES = 1024 * 1024; +const MAX_SCANNABLE_TOTAL_BYTES_PER_PACKAGE = 64 * 1024 * 1024; +const PACKAGE_SCAN_CONCURRENCY = 4; +const DEFAULT_SCANNER_INPUT_LIMITS = { + maxFileBytes: MAX_SCANNABLE_FILE_BYTES, + maxFiles: MAX_SCANNABLE_FILES_PER_PACKAGE, + maxTotalBytes: MAX_SCANNABLE_TOTAL_BYTES_PER_PACKAGE, +}; const COMMON_REVIEWED_CRITICAL_FINDING_COUNTS = new Map([ ["@openclaw/acpx:dangerous-exec:src/codex-auth-bridge.ts", 1], ["@openclaw/acpx:dangerous-exec:src/runtime-internals/mcp-proxy.mjs", 1], + ["@openclaw/acpx:dangerous-exec:src/runtime-internals/mcp-proxy.test.ts", 3], ["@openclaw/codex:dangerous-exec:src/app-server/transport-stdio.ts", 1], ["@openclaw/codex:dangerous-exec:src/doctor.ts", 1], ["@openclaw/discord:dangerous-exec:src/voice/audio.ts", 1], ["@openclaw/imessage:dangerous-exec:src/client.ts", 1], + ["@openclaw/imessage:dangerous-exec:src/client.test.ts", 3], ["@openclaw/llama-cpp-provider:dangerous-exec:src/llama-server-install.ts", 1], ["@openclaw/mxc-sandbox:dangerous-exec:src/readiness.ts", 2], ["@openclaw/raft:dangerous-exec:src/gateway.ts", 1], ["@openclaw/signal:dangerous-exec:src/daemon.ts", 1], ["@openclaw/voice-call:dangerous-exec:src/tunnel.ts", 1], + ["@openclaw/diagnostics-prometheus:dangerous-exec:src/install-runtime.e2e.test.ts", 2], + ["@openclaw/google-meet:dangerous-exec:src/cli-artifacts.test.ts", 1], + ["@openclaw/google-meet:dangerous-exec:src/realtime.process.test.ts", 1], + ["@openclaw/memory-lancedb:dangerous-exec:memory-lancedb.concurrent.test.ts", 1], + ["@openclaw/opencode-go-provider:env-harvesting:opencode-go.live.test.ts", 1], + ["@openclaw/openshell-sandbox:dangerous-exec:src/backend.e2e.test.ts", 1], + ["@openclaw/openshell-sandbox:dangerous-exec:src/openshell-core.test.ts", 1], ]); const REVIEWED_RELEASE_LAYOUTS = Object.freeze([ @@ -78,6 +103,7 @@ const REVIEWED_RELEASE_LAYOUTS = Object.freeze([ ["@openclaw/codex:dangerous-exec:src/app-server/sandbox-exec-server/processes.ts", 1], ["@openclaw/codex:dangerous-exec:src/node-cli-sessions.ts", 1], ["@openclaw/opencode-provider:dangerous-exec:session-catalog.ts", 1], + ["@openclaw/opencode-provider:dangerous-exec:session-catalog.test.ts", 1], ]), }, { @@ -85,6 +111,7 @@ const REVIEWED_RELEASE_LAYOUTS = Object.freeze([ findings: new Map([ ["@openclaw/codex:dangerous-exec:src/app-server/sandbox-exec-server/sandbox-child.ts", 1], ["@openclaw/codex:dangerous-exec:src/app-server/transport-process-containment.ts", 1], + ["@openclaw/codex:dangerous-exec:src/app-server/transport.process.test.ts", 13], ]), }, ]); @@ -171,7 +198,7 @@ export async function collectNpmPackedFiles( return parseNpmPackFiles(stdout, packageName); } -function normalizePackedFindingPath(packedPath: string): string { +export function normalizePackedFindingPath(packedPath: string): string { for (const prefix of [ "dynamic-tools", "outbound-payload.test-harness", @@ -181,7 +208,7 @@ function normalizePackedFindingPath(packedPath: string): string { "session-catalog", "transport-stdio", ]) { - if (packedPath.startsWith(`dist/${prefix}-`) && packedPath.endsWith(".js")) { + if (new RegExp(`^dist/${prefix}-[A-Za-z0-9_-]{8}\\.js$`, "u").test(packedPath)) { return `dist/${prefix}-.js`; } } @@ -235,9 +262,12 @@ function assertPathInside(parentPath: string, childPath: string): void { export function stageScannerRelevantPackedFiles( packageDir: string, packedFiles: readonly string[], -): string { + limits = DEFAULT_SCANNER_INPUT_LIMITS, +): { fileCount: number; stageDir: string; totalBytes: number } { const stageDir = mkdtempSync(join(tmpdir(), "openclaw-plugin-npm-scan-")); const realPackageDir = realpathSync(packageDir); + let fileCount = 0; + let totalBytes = 0; try { for (const packedPath of packedFiles) { @@ -253,6 +283,17 @@ export function stageScannerRelevantPackedFiles( if (!sourceStat.isFile()) { throw new Error(`Packed scanner input is not a regular file: ${packedPath}`); } + if (sourceStat.size > limits.maxFileBytes) { + throw new Error(`Packed scanner input exceeds the per-file byte limit: ${packedPath}`); + } + fileCount += 1; + totalBytes += sourceStat.size; + if (fileCount > limits.maxFiles) { + throw new Error("Packed scanner input exceeds the file-count limit."); + } + if (totalBytes > limits.maxTotalBytes) { + throw new Error("Packed scanner input exceeds the total-byte limit."); + } const realSource = realpathSync(source); assertPathInside(realPackageDir, realSource); @@ -260,7 +301,7 @@ export function stageScannerRelevantPackedFiles( mkdirSync(dirname(target), { recursive: true }); copyFileSync(realSource, target); } - return stageDir; + return { fileCount, stageDir, totalBytes }; } catch (error) { rmSync(stageDir, { recursive: true, force: true }); throw error; @@ -313,11 +354,15 @@ async function listPublishablePluginPackages( }); } -function findingKey(packageName: string, stageDir: string, finding: SkillScanFinding): string { +function findingRecord(stageDir: string, finding: SkillScanFinding): CriticalFindingRecord { const packedPath = normalizePackedFindingPath( relative(stageDir, finding.file).split(sep).join("/"), ); - return `${packageName}:${finding.ruleId}:${packedPath}`; + return { line: finding.line, path: packedPath, ruleId: finding.ruleId }; +} + +function findingKey(packageName: string, finding: CriticalFindingRecord): string { + return `${packageName}:${finding.ruleId}:${finding.path}`; } export function assertCompleteScannerSummary( @@ -342,34 +387,43 @@ async function scanPublishablePluginPackage( ); } - const stageDir = stageScannerRelevantPackedFiles(plugin.packageDir, packedFiles); + const staged = stageScannerRelevantPackedFiles(plugin.packageDir, packedFiles); try { - const summary = await scanDirectoryWithSummary(stageDir, { - excludeTestFiles: true, - maxFiles: 10_000, + const summary = await scanDirectoryWithSummary(staged.stageDir, { + excludeTestFiles: false, + maxFileBytes: MAX_SCANNABLE_FILE_BYTES, + maxFiles: MAX_SCANNABLE_FILES_PER_PACKAGE, }); assertCompleteScannerSummary(plugin.packageName, summary); + if (summary.scannedFiles !== staged.fileCount) { + throw new Error( + `${plugin.packageName}: security scan processed ${summary.scannedFiles} of ${staged.fileCount} staged files.`, + ); + } for (const finding of summary.findings) { if (finding.severity !== "critical") { continue; } - const key = findingKey(plugin.packageName, stageDir, finding); + const record = findingRecord(staged.stageDir, finding); + const key = findingKey(plugin.packageName, record); if (isReviewedCriticalFinding(key)) { reviewedCriticalFindings.push(key); } else { - unexpectedCriticalFindings.push([key, `${finding.line}`, finding.evidence].join(":")); + unexpectedCriticalFindings.push(record); } } } finally { - rmSync(stageDir, { recursive: true, force: true }); + rmSync(staged.stageDir, { recursive: true, force: true }); } return { - expectedReviewedCriticalFindings, + expectedReviewedCriticalFindings: sortStrings(expectedReviewedCriticalFindings), packageName: plugin.packageName, packedFileCount: packedFiles.length, - reviewedCriticalFindings, - unexpectedCriticalFindings, + reviewedCriticalFindings: sortStrings(reviewedCriticalFindings), + unexpectedCriticalFindings: unexpectedCriticalFindings.toSorted((left, right) => + JSON.stringify(left).localeCompare(JSON.stringify(right)), + ), }; } @@ -416,7 +470,7 @@ export function buildPluginNpmSecurityScanReport(params: { for (const result of packageResults) { if (result.unexpectedCriticalFindings.length > 0) { errors.push( - `${result.packageName}: unexpected critical findings: ${result.unexpectedCriticalFindings.join(" | ")}`, + `${result.packageName}: unexpected critical findings: ${JSON.stringify(result.unexpectedCriticalFindings)}`, ); } if (!layout) { @@ -438,13 +492,21 @@ export function buildPluginNpmSecurityScanReport(params: { (total, result) => total + result.unexpectedCriticalFindings.length, 0, ); + const sortedPackages = packageResults + .map((result) => ({ + ...result, + expectedReviewedCriticalFindings: sortStrings(result.expectedReviewedCriticalFindings), + reviewedCriticalFindings: sortStrings(result.reviewedCriticalFindings), + unexpectedCriticalFindings: result.unexpectedCriticalFindings.toSorted((left, right) => + JSON.stringify(left).localeCompare(JSON.stringify(right)), + ), + })) + .toSorted((left, right) => left.packageName.localeCompare(right.packageName)); return { candidateSha, - errors, + errors: sortStrings(errors), layout: layout?.id ?? null, - packages: packageResults.toSorted((left, right) => - left.packageName.localeCompare(right.packageName), - ), + packages: sortedPackages, schemaVersion: 1, status: errors.length === 0 ? "pass" : "fail", summary: { @@ -467,6 +529,11 @@ export async function runPluginNpmSecurityScan(params: { gitOutput(toolingDir, ["rev-parse", "HEAD"]), listPublishablePluginPackages(candidateDir), ]); - const packageResults = await Promise.all(packages.map(scanPublishablePluginPackage)); + const { results: packageResults } = await runTasksWithConcurrency({ + errorMode: "stop", + limit: PACKAGE_SCAN_CONCURRENCY, + tasks: packages.map((plugin) => () => scanPublishablePluginPackage(plugin)), + throwOnError: true, + }); return buildPluginNpmSecurityScanReport({ candidateSha, packageResults, toolingSha }); } diff --git a/scripts/plugin-npm-security-scan.mts b/scripts/plugin-npm-security-scan.mts index 08de171c8129..ec6d08f0aec3 100644 --- a/scripts/plugin-npm-security-scan.mts +++ b/scripts/plugin-npm-security-scan.mts @@ -59,6 +59,25 @@ async function writeReport(outputPath: string, report: PluginNpmSecurityScanRepo await writeFile(outputPath, `${JSON.stringify(report, null, 2)}\n`, "utf8"); } +function sanitizeErrorMessage( + error: unknown, + args: ReturnType | undefined, +): string { + let message = error instanceof Error ? error.message : String(error); + for (const [path, replacement] of [ + [args?.candidateRoot, ""], + [process.cwd(), ""], + ] as const) { + if (path) { + message = message.replaceAll(path, replacement); + } + } + return message.replaceAll( + /\/(?:private\/)?tmp\/openclaw-plugin-npm-scan-[^/\s:]+/gu, + "", + ); +} + async function main(argv = process.argv.slice(2)): Promise { let args: ReturnType | undefined; try { @@ -88,11 +107,11 @@ async function main(argv = process.argv.slice(2)): Promise { } return report.status === "pass" ? 0 : 1; } catch (error) { - const message = error instanceof Error ? error.message : String(error); + const message = sanitizeErrorMessage(error, args); console.error(`Plugin npm security scan failed: ${message}`); if (args) { await writeReport(args.outputPath, { - candidateSha: "", + candidateSha: args.candidateSha, errors: [message], layout: null, packages: [], @@ -103,7 +122,7 @@ async function main(argv = process.argv.slice(2)): Promise { reviewedCriticalFindingCount: 0, unexpectedCriticalFindingCount: 0, }, - toolingSha: "", + toolingSha: args.toolingSha, }); } return 1; diff --git a/test/scripts/plugin-npm-security-scan.test.ts b/test/scripts/plugin-npm-security-scan.test.ts index 0699b495145a..3b9dedfbab0e 100644 --- a/test/scripts/plugin-npm-security-scan.test.ts +++ b/test/scripts/plugin-npm-security-scan.test.ts @@ -1,10 +1,12 @@ -import { existsSync, symlinkSync, writeFileSync } from "node:fs"; +import { spawnSync } from "node:child_process"; +import { existsSync, readFileSync, symlinkSync, writeFileSync } from "node:fs"; import { join } from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import { assertCompleteScannerSummary, buildPluginNpmSecurityScanReport, collectNpmPackedFiles, + normalizePackedFindingPath, resolveReviewedSourceLayout, runPluginNpmSecurityScan, stageScannerRelevantPackedFiles, @@ -18,12 +20,17 @@ describe("scripts/lib/plugin-npm-security-scan.mts", () => { const current = [ "@openclaw/codex:dangerous-exec:src/app-server/sandbox-exec-server/sandbox-child.ts", "@openclaw/codex:dangerous-exec:src/app-server/transport-process-containment.ts", + ...Array.from( + { length: 13 }, + () => "@openclaw/codex:dangerous-exec:src/app-server/transport.process.test.ts", + ), ]; const frozenLegacy = [ "@openclaw/codex:dangerous-exec:src/app-server/sandbox-exec-server/http.ts", "@openclaw/codex:dangerous-exec:src/app-server/sandbox-exec-server/processes.ts", "@openclaw/codex:dangerous-exec:src/node-cli-sessions.ts", "@openclaw/opencode-provider:dangerous-exec:session-catalog.ts", + "@openclaw/opencode-provider:dangerous-exec:session-catalog.test.ts", ]; expect(resolveReviewedSourceLayout(current)?.id).toBe("current"); @@ -81,6 +88,70 @@ describe("scripts/lib/plugin-npm-security-scan.mts", () => { expect(() => stageScannerRelevantPackedFiles(packageDir, ["escape.ts"])).toThrow( "not a regular file", ); + + const oversizeDir = tempDirs.make("openclaw-plugin-npm-security-oversize-"); + writeFileSync(join(oversizeDir, "oversize.ts"), Buffer.alloc(1024 * 1024 + 1)); + expect(() => stageScannerRelevantPackedFiles(oversizeDir, ["oversize.ts"])).toThrow( + "per-file byte limit", + ); + + const boundedDir = tempDirs.make("openclaw-plugin-npm-security-bounds-"); + writeFileSync(join(boundedDir, "one.ts"), "1", "utf8"); + writeFileSync(join(boundedDir, "two.ts"), "22", "utf8"); + expect(() => + stageScannerRelevantPackedFiles(boundedDir, ["one.ts", "two.ts"], { + maxFileBytes: 10, + maxFiles: 1, + maxTotalBytes: 10, + }), + ).toThrow("file-count limit"); + expect(() => + stageScannerRelevantPackedFiles(boundedDir, ["two.ts"], { + maxFileBytes: 10, + maxFiles: 10, + maxTotalBytes: 1, + }), + ).toThrow("total-byte limit"); + }); + + it("normalizes only exact bundler hash filenames", () => { + expect(normalizePackedFindingPath("dist/service-BaCqPs_5.js")).toBe("dist/service-.js"); + expect(normalizePackedFindingPath("dist/service-malware.js")).toBe("dist/service-malware.js"); + }); + + it("retains expected SHAs and redacts candidate paths in failure reports", () => { + const root = tempDirs.make("openclaw-plugin-npm-security-failure-"); + const candidateRoot = join(root, "missing-candidate"); + const reportPath = join(root, "report.json"); + const candidateSha = "1".repeat(40); + const toolingSha = "2".repeat(40); + const result = spawnSync( + process.execPath, + [ + "--import", + "tsx", + "scripts/plugin-npm-security-scan.mts", + "--candidate-root", + candidateRoot, + "--candidate-sha", + candidateSha, + "--tooling-sha", + toolingSha, + "--report", + reportPath, + ], + { cwd: process.cwd(), encoding: "utf8" }, + ); + const report = JSON.parse(readFileSync(reportPath, "utf8")) as { + candidateSha: string; + toolingSha: string; + }; + + expect(result.status).toBe(1); + expect(report.candidateSha).toBe(candidateSha); + expect(report.toolingSha).toBe(toolingSha); + expect(JSON.stringify(report)).not.toContain(candidateRoot); + expect(result.stderr).not.toContain(candidateRoot); }); it("scans the complete current-root publishable plugin inventory", async () => { @@ -98,11 +169,18 @@ describe("scripts/lib/plugin-npm-security-scan.mts", () => { }); expect(report.summary.packageCount).toBe(report.packages.length); expect(report.summary.packageCount).toBeGreaterThan(0); + expect( + report.packages + .find((entry) => entry.packageName === "@openclaw/acpx") + ?.reviewedCriticalFindings.some((finding) => finding.endsWith(".test.ts")), + ).toBe(true); const packageResults = structuredClone(report.packages); - packageResults[0]!.unexpectedCriticalFindings.push( - `${packageResults[0]!.packageName}:dangerous-exec:src/candidate-owned-scanner.ts:1:exec`, - ); + packageResults[0]!.unexpectedCriticalFindings.push({ + line: 1, + path: "src/candidate-owned-scanner.ts", + ruleId: "dangerous-exec", + }); const rejectedReport = buildPluginNpmSecurityScanReport({ candidateSha: report.candidateSha, packageResults, @@ -112,5 +190,15 @@ describe("scripts/lib/plugin-npm-security-scan.mts", () => { expect(rejectedReport.errors).toContainEqual( expect.stringContaining("unexpected critical findings"), ); + expect(JSON.stringify(rejectedReport)).not.toContain("exec("); + expect(JSON.stringify(rejectedReport)).toBe( + JSON.stringify( + buildPluginNpmSecurityScanReport({ + candidateSha: report.candidateSha, + packageResults: structuredClone(packageResults).reverse(), + toolingSha: report.toolingSha, + }), + ), + ); }, 120_000); });