mirror of https://github.com/openclaw/openclaw.git
fix: align skill install security gate
This commit is contained in:
Peter Steinberger
parent
192484ed0a
commit
bf96c67fd1
|
|
@ -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();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<SkillScanResult> {
|
||||
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<SkillIn
|
|||
}
|
||||
|
||||
const spec = findInstallSpec(entry, params.installId);
|
||||
const scanResult = await collectSkillInstallScanWarnings(entry);
|
||||
const warnings = scanResult.warnings;
|
||||
const skillDir = path.resolve(entry.skill.baseDir);
|
||||
const warnings: string[] = [];
|
||||
const skillSource = resolveSkillSource(entry.skill);
|
||||
|
||||
// Run before_install so external scanners can augment findings or block installs.
|
||||
const hookRunner = getGlobalHookRunner();
|
||||
if (hookRunner?.hasHooks("before_install")) {
|
||||
try {
|
||||
const { event, ctx } = createBeforeInstallHookPayload({
|
||||
targetName: params.skillName,
|
||||
targetType: "skill",
|
||||
origin: skillSource,
|
||||
sourcePath: path.resolve(entry.skill.baseDir),
|
||||
sourcePathKind: "directory",
|
||||
request: {
|
||||
kind: "skill-install",
|
||||
mode: "install",
|
||||
},
|
||||
builtinScan: scanResult.builtinScan,
|
||||
skill: {
|
||||
installId: params.installId,
|
||||
...(spec ? { installSpec: normalizeSkillInstallSpec(spec) } : {}),
|
||||
},
|
||||
});
|
||||
const hookResult = await hookRunner.runBeforeInstall(event, ctx);
|
||||
if (hookResult?.block) {
|
||||
return {
|
||||
ok: false,
|
||||
message: hookResult.blockReason || "Installation blocked by plugin hook",
|
||||
stdout: "",
|
||||
stderr: "",
|
||||
code: null,
|
||||
warnings: warnings.length > 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<SkillIn
|
|||
`WARNING: Skill "${params.skillName}" install triggered from non-bundled source "${skillSource}". Verify the install recipe is trusted.`,
|
||||
);
|
||||
}
|
||||
const builtinBlocked = resolveBuiltinSkillScanDecision({
|
||||
builtinScan: scanResult.builtinScan,
|
||||
dangerouslyForceUnsafeInstall: params.dangerouslyForceUnsafeInstall,
|
||||
skillDir,
|
||||
skillName: params.skillName,
|
||||
warnings,
|
||||
});
|
||||
if (builtinBlocked) {
|
||||
return withWarnings(builtinBlocked, warnings);
|
||||
}
|
||||
if (!spec) {
|
||||
return withWarnings(
|
||||
{
|
||||
|
|
|
|||
|
|
@ -445,3 +445,66 @@ export async function scanFileInstallSourceRuntime(
|
|||
});
|
||||
return hookResult?.blocked ? hookResult : builtinBlocked;
|
||||
}
|
||||
|
||||
export async function scanSkillInstallSourceRuntime(params: {
|
||||
dangerouslyForceUnsafeInstall?: boolean;
|
||||
installId: string;
|
||||
installSpec?: {
|
||||
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;
|
||||
};
|
||||
logger: InstallScanLogger;
|
||||
origin: string;
|
||||
skillName: string;
|
||||
sourceDir: string;
|
||||
}): Promise<InstallSecurityScanResult | undefined> {
|
||||
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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<InstallSecurityScanResult | undefined> {
|
||||
const { scanSkillInstallSourceRuntime } = await loadInstallSecurityScanRuntime();
|
||||
return await scanSkillInstallSourceRuntime(params);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue