diff --git a/src/agents/skills-install.test.ts b/src/agents/skills-install.test.ts index 52d9173cdd9..6819b6f0ddc 100644 --- a/src/agents/skills-install.test.ts +++ b/src/agents/skills-install.test.ts @@ -86,7 +86,7 @@ describe("installSkill code safety scanning", () => { }); }); - it("blocks install for critical findings without dangerous override", async () => { + it("blocks install when skill has dangerous code patterns", async () => { await withWorkspaceCase(async ({ workspaceDir }) => { const skillDir = await writeInstallableSkill(workspaceDir, "danger-skill"); scanDirectoryWithSummaryMock.mockResolvedValue({ @@ -113,8 +113,7 @@ describe("installSkill code safety scanning", () => { }); expect(result.ok).toBe(false); - expect(result.message).toContain("installation blocked by dangerous code patterns"); - expect(result.message).toContain("dangerouslyForceUnsafeInstall"); + expect(result.message).toContain('Skill "danger-skill" installation blocked'); expect(result.warnings?.some((warning) => warning.includes("dangerous code patterns"))).toBe( true, ); @@ -123,9 +122,9 @@ describe("installSkill code safety scanning", () => { }); }); - it("allows critical findings when dangerous override is set", async () => { + it("allows dangerous skill installs when forced unsafe install is set", async () => { await withWorkspaceCase(async ({ workspaceDir }) => { - const skillDir = await writeInstallableSkill(workspaceDir, "forced-skill"); + const skillDir = await writeInstallableSkill(workspaceDir, "forced-danger-skill"); scanDirectoryWithSummaryMock.mockResolvedValue({ scannedFiles: 1, critical: 1, @@ -145,7 +144,7 @@ describe("installSkill code safety scanning", () => { const result = await installSkill({ workspaceDir, - skillName: "forced-skill", + skillName: "forced-danger-skill", installId: "deps", dangerouslyForceUnsafeInstall: true, }); @@ -154,15 +153,14 @@ describe("installSkill code safety scanning", () => { expect( result.warnings?.some((warning) => warning.includes( - "forced despite dangerous code patterns via dangerouslyForceUnsafeInstall", + "forced despite dangerous code patterns via --dangerously-force-unsafe-install", ), ), ).toBe(true); - expect(runCommandWithTimeoutMock).toHaveBeenCalledTimes(1); }); }); - it("warns and continues when skill scan fails", async () => { + it("blocks install when skill scan fails", async () => { await withWorkspaceCase(async ({ workspaceDir }) => { await writeInstallableSkill(workspaceDir, "scanfail-skill"); scanDirectoryWithSummaryMock.mockRejectedValue(new Error("scanner exploded")); @@ -173,13 +171,9 @@ describe("installSkill code safety scanning", () => { installId: "deps", }); - expect(result.ok).toBe(true); - expect(result.warnings?.some((warning) => warning.includes("code safety scan failed"))).toBe( - true, - ); - expect(result.warnings?.some((warning) => warning.includes("Installation continues"))).toBe( - true, - ); + expect(result.ok).toBe(false); + expect(result.message).toContain("code safety scan failed"); + expect(runCommandWithTimeoutMock).not.toHaveBeenCalled(); }); }); @@ -267,7 +261,7 @@ describe("installSkill code safety scanning", () => { }); }); - it("keeps before_install blocks even with dangerous override", async () => { + it("keeps before_install hook blocks even when forced unsafe install is set", async () => { const handler = vi.fn().mockReturnValue({ block: true, blockReason: "Blocked by enterprise policy", @@ -275,7 +269,7 @@ describe("installSkill code safety scanning", () => { initializeGlobalHookRunner(createMockPluginRegistry([{ hookName: "before_install", handler }])); await withWorkspaceCase(async ({ workspaceDir }) => { - const skillDir = await writeInstallableSkill(workspaceDir, "forced-but-blocked"); + const skillDir = await writeInstallableSkill(workspaceDir, "forced-blocked-skill"); scanDirectoryWithSummaryMock.mockResolvedValue({ scannedFiles: 1, critical: 1, @@ -295,13 +289,20 @@ describe("installSkill code safety scanning", () => { const result = await installSkill({ workspaceDir, - skillName: "forced-but-blocked", + skillName: "forced-blocked-skill", installId: "deps", dangerouslyForceUnsafeInstall: true, }); expect(result.ok).toBe(false); expect(result.message).toBe("Blocked by enterprise policy"); + expect( + result.warnings?.some((warning) => + warning.includes( + "forced despite dangerous code patterns via --dangerously-force-unsafe-install", + ), + ), + ).toBe(true); expect(runCommandWithTimeoutMock).not.toHaveBeenCalled(); }); }); diff --git a/src/agents/skills-install.ts b/src/agents/skills-install.ts index 2aa8b0c8c60..9da4ff86483 100644 --- a/src/agents/skills-install.ts +++ b/src/agents/skills-install.ts @@ -2,11 +2,12 @@ import fs from "node:fs"; import path from "node:path"; import type { OpenClawConfig } from "../config/config.js"; import { resolveBrewExecutable } from "../infra/brew.js"; -import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; -import { createBeforeInstallHookPayload } from "../plugins/install-policy-context.js"; -import type { InstallSafetyOverrides } from "../plugins/install-security-scan.js"; +import { + type InstallSafetyOverrides, + scanSkillInstallSource, + type SkillInstallSpecMetadata, +} from "../plugins/install-security-scan.js"; import { runCommandWithTimeout, type CommandOptions } from "../process/exec.js"; -import { scanDirectoryWithSummary } from "../security/skill-scanner.js"; import { resolveUserPath } from "../utils.js"; import { installDownloadSpec } from "./skills-install-download.js"; import { formatInstallFailureMessage } from "./skills-install-output.js"; @@ -47,124 +48,6 @@ function withWarnings(result: SkillInstallResult, warnings: string[]): SkillInst }; } -function formatScanFindingDetail( - rootDir: string, - finding: { message: string; file: string; line: number }, -): string { - const relativePath = path.relative(rootDir, finding.file); - const filePath = - relativePath && relativePath !== "." && !relativePath.startsWith("..") - ? relativePath - : path.basename(finding.file); - return `${finding.message} (${filePath}:${finding.line})`; -} - -type SkillScanFinding = { - ruleId: string; - severity: "info" | "warn" | "critical"; - file: string; - line: number; - message: string; -}; - -type SkillBuiltinScan = { - status: "ok" | "error"; - scannedFiles: number; - critical: number; - warn: number; - info: number; - findings: SkillScanFinding[]; - error?: string; -}; - -type SkillScanResult = { - warnings: string[]; - builtinScan: SkillBuiltinScan; -}; - -function buildCriticalFindingDetails(rootDir: string, findings: SkillScanFinding[]): string { - return findings - .filter((finding) => finding.severity === "critical") - .map((finding) => formatScanFindingDetail(rootDir, finding)) - .join("; "); -} - -async function collectSkillInstallScanWarnings(entry: SkillEntry): Promise { - const warnings: string[] = []; - const skillName = entry.skill.name; - const skillDir = path.resolve(entry.skill.baseDir); - - try { - const summary = await scanDirectoryWithSummary(skillDir); - const builtinScan: SkillBuiltinScan = { - status: "ok", - scannedFiles: summary.scannedFiles, - critical: summary.critical, - warn: summary.warn, - info: summary.info, - findings: summary.findings, - }; - if (summary.critical > 0) { - const criticalDetails = buildCriticalFindingDetails(skillDir, summary.findings); - warnings.push( - `WARNING: Skill "${skillName}" contains dangerous code patterns: ${criticalDetails}`, - ); - } else if (summary.warn > 0) { - warnings.push( - `Skill "${skillName}" has ${summary.warn} suspicious code pattern(s). Run "openclaw security audit --deep" for details.`, - ); - } - return { warnings, builtinScan }; - } catch (err) { - warnings.push( - `Skill "${skillName}" code safety scan failed (${String(err)}). Installation continues; run "openclaw security audit --deep" after install.`, - ); - return { - warnings, - builtinScan: { - status: "error", - scannedFiles: 0, - critical: 0, - warn: 0, - info: 0, - findings: [], - error: String(err), - }, - }; - } -} - -function resolveBuiltinSkillScanDecision(params: { - builtinScan: SkillBuiltinScan; - dangerouslyForceUnsafeInstall?: boolean; - skillDir: string; - skillName: string; - warnings: string[]; -}): SkillInstallResult | undefined { - if (params.builtinScan.status !== "ok" || params.builtinScan.critical === 0) { - return undefined; - } - - const criticalDetails = buildCriticalFindingDetails(params.skillDir, params.builtinScan.findings); - if (params.dangerouslyForceUnsafeInstall) { - params.warnings.push( - `WARNING: Skill "${params.skillName}" forced despite dangerous code patterns via dangerouslyForceUnsafeInstall: ${criticalDetails}`, - ); - return undefined; - } - - return { - ok: false, - message: [ - `Skill "${params.skillName}" installation blocked by dangerous code patterns: ${criticalDetails}.`, - "Retry only if you trust this skill and set dangerouslyForceUnsafeInstall (CLI flag: --dangerously-force-unsafe-install).", - ].join(" "), - stdout: "", - stderr: "", - code: null, - }; -} - function resolveInstallId(spec: SkillInstallSpec, index: number): string { return (spec.id ?? `${spec.kind}-${index}`).trim(); } @@ -179,7 +62,7 @@ function findInstallSpec(entry: SkillEntry, installId: string): SkillInstallSpec return undefined; } -function normalizeSkillInstallSpec(spec: SkillInstallSpec): SkillInstallSpec { +function normalizeSkillInstallSpec(spec: SkillInstallSpec): SkillInstallSpecMetadata { return { ...(spec.id ? { id: spec.id } : {}), kind: spec.kind, @@ -538,56 +421,31 @@ export async function installSkill(params: SkillInstallRequest): Promise 0 ? warnings.slice() : undefined, - }; - } - if (hookResult?.findings) { - for (const finding of hookResult.findings) { - if (finding.severity === "critical") { - warnings.push( - `WARNING: Plugin scanner: ${finding.message} (${finding.file}:${finding.line})`, - ); - } else if (finding.severity === "warn") { - warnings.push(`Plugin scanner: ${finding.message} (${finding.file}:${finding.line})`); - } - } - } - } catch { - // Hook errors are non-fatal — built-in scanner results still apply. - } + const normalizedSpec = spec ? normalizeSkillInstallSpec(spec) : undefined; + const scanResult = await scanSkillInstallSource({ + dangerouslyForceUnsafeInstall: params.dangerouslyForceUnsafeInstall, + installId: params.installId, + ...(normalizedSpec ? { installSpec: normalizedSpec } : {}), + logger: { + warn: (message) => warnings.push(message), + }, + origin: skillSource, + skillName: params.skillName, + sourceDir: path.resolve(entry.skill.baseDir), + }); + if (scanResult?.blocked) { + return withWarnings( + { + ok: false, + message: scanResult.blocked.reason, + stdout: "", + stderr: "", + code: null, + }, + warnings, + ); } // Warn when install is triggered from a non-bundled source. // Workspace/project/personal agent skills can contain attacker-controlled metadata. @@ -597,16 +455,6 @@ export async function installSkill(params: SkillInstallRequest): Promise { + const builtinScan = await scanDirectoryTarget({ + logger: params.logger, + path: params.sourceDir, + suspiciousMessage: + 'Skill "{target}" has {count} suspicious code pattern(s). Run "openclaw security audit --deep" for details.', + targetName: params.skillName, + warningMessage: `WARNING: Skill "${params.skillName}" contains dangerous code patterns`, + }); + const builtinBlocked = buildBlockedScanResult({ + builtinScan, + dangerouslyForceUnsafeInstall: params.dangerouslyForceUnsafeInstall, + targetLabel: `Skill "${params.skillName}" installation`, + }); + if (params.dangerouslyForceUnsafeInstall && builtinScan.critical > 0) { + logDangerousForceUnsafeInstall({ + findings: builtinScan.findings, + logger: params.logger, + targetLabel: `Skill "${params.skillName}" installation`, + }); + } + + const hookResult = await runBeforeInstallHook({ + logger: params.logger, + installLabel: `Skill "${params.skillName}" installation`, + origin: params.origin, + sourcePath: params.sourceDir, + sourcePathKind: "directory", + targetName: params.skillName, + targetType: "skill", + requestKind: "skill-install", + requestMode: "install", + builtinScan, + skill: { + installId: params.installId, + ...(params.installSpec ? { installSpec: params.installSpec } : {}), + }, + }); + return hookResult?.blocked ? hookResult : builtinBlocked; +} diff --git a/src/plugins/install-security-scan.ts b/src/plugins/install-security-scan.ts index 9446d74a436..1843a295cbe 100644 --- a/src/plugins/install-security-scan.ts +++ b/src/plugins/install-security-scan.ts @@ -19,6 +19,22 @@ export type PluginInstallRequestKind = | "plugin-file" | "plugin-npm"; +export type SkillInstallSpecMetadata = { + id?: string; + kind: "brew" | "node" | "go" | "uv" | "download"; + label?: string; + bins?: string[]; + os?: string[]; + formula?: string; + package?: string; + module?: string; + url?: string; + archive?: string; + extract?: boolean; + stripComponents?: number; + targetDir?: string; +}; + async function loadInstallSecurityScanRuntime() { return await import("./install-security-scan.runtime.js"); } @@ -68,3 +84,16 @@ export async function scanFileInstallSource( const { scanFileInstallSourceRuntime } = await loadInstallSecurityScanRuntime(); return await scanFileInstallSourceRuntime(params); } + +export async function scanSkillInstallSource(params: { + dangerouslyForceUnsafeInstall?: boolean; + installId: string; + installSpec?: SkillInstallSpecMetadata; + logger: InstallScanLogger; + origin: string; + skillName: string; + sourceDir: string; +}): Promise { + const { scanSkillInstallSourceRuntime } = await loadInstallSecurityScanRuntime(); + return await scanSkillInstallSourceRuntime(params); +}