From 009be3dd20ab96bc97c73c6a04b842d0b0ca2edc Mon Sep 17 00:00:00 2001 From: jesse-merhi <79823012+jesse-merhi@users.noreply.github.com> Date: Wed, 12 Aug 2026 17:38:37 +1000 Subject: [PATCH] fix(security): bound install policy notices --- docs/tools/skills-config.md | 6 +- .../install-security-scan.runtime.test.ts | 80 +++++++++++++ src/plugins/install-security-scan.runtime.ts | 112 +++++++++++++++--- 3 files changed, 182 insertions(+), 16 deletions(-) diff --git a/docs/tools/skills-config.md b/docs/tools/skills-config.md index 174ee5d7ecca..117400cf522e 100644 --- a/docs/tools/skills-config.md +++ b/docs/tools/skills-config.md @@ -187,7 +187,11 @@ absent. Operator-facing reason and finding text are limited to 1,000 characters. OpenClaw retains at most 100 normalized findings for display. Only a `warn` response with more than 100 valid findings fails closed and cannot be acknowledged; `allow` and `block` retain the first 100. A warning stops the -install before commit. Interactive CLI +install before commit. A `warn` review whose sanitized reason and findings +exceed the 4,000-character aggregate display limit fails closed without +presenting a partial review. An over-budget `block` remains terminal with a +bounded denial, while over-budget findings on `allow` are summarized in bounded +diagnostic output. Interactive CLI plugin and skill commands ask the operator to type the target name using the same `install anyway` or `update anyway` copy as suspicious ClawHub releases, then run policy again before continuing. Declined and non-interactive commands diff --git a/src/plugins/install-security-scan.runtime.test.ts b/src/plugins/install-security-scan.runtime.test.ts index bc2aafc25879..8682b3e1e01c 100644 --- a/src/plugins/install-security-scan.runtime.test.ts +++ b/src/plugins/install-security-scan.runtime.test.ts @@ -574,6 +574,86 @@ describe("legacy file install scan compatibility", () => { ); }); + function createMaximumPolicyFindings() { + const maxText = "x".repeat(1_000); + return { + maxText, + findings: Array.from({ length: 100 }, (_, index) => ({ + ruleId: `${String(index)}-${maxText}`, + severity: "critical" as const, + message: maxText, + file: maxText, + evidence: maxText, + })), + }; + } + + it("fails closed when a maximum-size warning exceeds the aggregate display limit", async () => { + const { findings, maxText } = createMaximumPolicyFindings(); + runInstallPolicyMock.mockResolvedValue({ + warning: { reason: maxText, fingerprint: "oversized-warning" }, + findings, + }); + const onInstallPolicyWarning = vi.fn().mockResolvedValue({ status: "approved" }); + const warnings: string[] = []; + + const result = await scanFileInstallSourceRuntime({ + filePath: "/tmp/payload.js", + logger: { warn: (message) => warnings.push(message) }, + onInstallPolicyWarning, + pluginId: "payload", + }); + + expect(result?.blocked).toEqual({ + code: "security_scan_failed", + reason: + "install policy failed closed: policy review exceeds the 4,000-character display limit; reduce or coalesce the reason and findings", + }); + expect(result?.blocked?.reason.length).toBeLessThan(200); + expect(onInstallPolicyWarning).not.toHaveBeenCalled(); + expect(warnings).toEqual([]); + }); + + it("keeps a maximum-size block terminal with a bounded denial", async () => { + const { findings, maxText } = createMaximumPolicyFindings(); + runInstallPolicyMock.mockResolvedValue({ + blocked: { + code: "security_scan_blocked", + reason: `blocked by install policy: ${maxText}`, + }, + findings, + }); + const onInstallPolicyWarning = vi.fn().mockResolvedValue({ status: "approved" }); + + const result = await scanFileInstallSourceRuntime({ + filePath: "/tmp/payload.js", + logger: {}, + onInstallPolicyWarning, + pluginId: "payload", + }); + + expect(result?.blocked?.code).toBe("security_scan_blocked"); + expect(result?.blocked?.reason).toContain("Findings omitted"); + expect(result?.blocked?.reason.length).toBeLessThanOrEqual(4_000); + expect(onInstallPolicyWarning).not.toHaveBeenCalled(); + }); + + it("bounds maximum-size allow findings without blocking the install", async () => { + const { findings } = createMaximumPolicyFindings(); + runInstallPolicyMock.mockResolvedValue({ findings }); + const warnings: string[] = []; + + const result = await scanFileInstallSourceRuntime({ + filePath: "/tmp/payload.js", + logger: { warn: (message) => warnings.push(message) }, + pluginId: "payload", + }); + + expect(result).toBeUndefined(); + expect(warnings.join("\n")).toContain("additional findings omitted"); + expect(warnings.join("\n").length).toBeLessThanOrEqual(4_000); + }); + it.each(["security_scan_blocked", "security_scan_failed"] as const)( "does not let acknowledgement override %s", async (code) => { diff --git a/src/plugins/install-security-scan.runtime.ts b/src/plugins/install-security-scan.runtime.ts index 64bdccc47b07..1e2754511118 100644 --- a/src/plugins/install-security-scan.runtime.ts +++ b/src/plugins/install-security-scan.runtime.ts @@ -26,6 +26,12 @@ type InstallScanLogger = { const FULL_GIT_COMMIT_PATTERN = /^[0-9a-f]{40}$/i; const INSTALL_POLICY_BLOCK_REASON_PREFIX = "blocked by install policy: "; const INSTALL_POLICY_ACKNOWLEDGEMENT_FLAG = "--acknowledge-install-policy-warning"; +const MAX_INSTALL_POLICY_NOTICE_CHARS = 4_000; +const INSTALL_POLICY_REVIEW_GUIDANCE = [ + "To continue:", + " • Rerun interactively and approve the warning.", + ` • For reviewed automation, add ${INSTALL_POLICY_ACKNOWLEDGEMENT_FLAG}.`, +]; type PluginInstallRequestKind = Exclude; @@ -103,6 +109,34 @@ export type InstallSecurityScanResult = { }; }; +function failOversizedInstallPolicyWarning(params: { + result: Awaited>; + targetName: string; + targetType: "skill" | "plugin"; +}): InstallSecurityScanResult | undefined { + if (!params.result?.warning) { + return undefined; + } + const notice = formatInstallPolicyNotice({ + decision: "warn", + findings: params.result.findings, + guidance: INSTALL_POLICY_REVIEW_GUIDANCE, + reason: params.result.warning.reason, + targetName: params.targetName, + targetType: params.targetType, + }); + if (notice.length <= MAX_INSTALL_POLICY_NOTICE_CHARS) { + return undefined; + } + return { + blocked: { + code: "security_scan_failed", + reason: + "install policy failed closed: policy review exceeds the 4,000-character display limit; reduce or coalesce the reason and findings", + }, + }; +} + function formatBlockedInstallPolicyResult(params: { blocked: NonNullable; findings?: InstallPolicyFinding[]; @@ -116,16 +150,34 @@ function formatBlockedInstallPolicyResult(params: { return { blocked: params.blocked }; } const reason = params.blocked.reason.slice(INSTALL_POLICY_BLOCK_REASON_PREFIX.length); + const notice = formatInstallPolicyNotice({ + decision: "block", + findings: params.findings, + reason, + targetName: params.targetName, + targetType: params.targetType, + }); + if (notice.length > MAX_INSTALL_POLICY_NOTICE_CHARS) { + const compactNotice = `${formatInstallPolicyNotice({ + decision: "block", + reason, + targetName: params.targetName, + targetType: params.targetType, + })}\n Findings omitted: policy review exceeds the 4,000-character display limit.`; + return { + blocked: { + ...params.blocked, + reason: + compactNotice.length <= MAX_INSTALL_POLICY_NOTICE_CHARS + ? compactNotice + : "Install blocked by policy: review exceeds the 4,000-character display limit.", + }, + }; + } return { blocked: { ...params.blocked, - reason: formatInstallPolicyNotice({ - decision: "block", - findings: params.findings, - reason, - targetName: params.targetName, - targetType: params.targetType, - }), + reason: notice, }, }; } @@ -720,14 +772,40 @@ async function runOperatorInstallPolicy(params: { ); return; } - for (const finding of result?.findings ?? []) { - if (finding.severity === "critical" || finding.severity === "warn") { - params.logger.warn?.(`Install policy: ${formatInstallPolicyFinding(finding)}`); + const messages = (result?.findings ?? []) + .filter((finding) => finding.severity === "critical" || finding.severity === "warn") + .map((finding) => `Install policy: ${formatInstallPolicyFinding(finding)}`); + if ( + messages.reduce((length, message) => length + message.length + 1, 0) <= + MAX_INSTALL_POLICY_NOTICE_CHARS + ) { + for (const message of messages) { + params.logger.warn?.(message); } + return; } + const omittedMessage = + "Install policy: additional findings omitted because the 4,000-character log limit was reached."; + let remaining = MAX_INSTALL_POLICY_NOTICE_CHARS - omittedMessage.length - 1; + for (const message of messages) { + if (message.length + 1 > remaining) { + continue; + } + params.logger.warn?.(message); + remaining -= message.length + 1; + } + params.logger.warn?.(omittedMessage); }; const result = await evaluatePolicy(); + const presentationFailure = failOversizedInstallPolicyWarning({ + result, + targetName: params.targetName, + targetType: params.targetType, + }); + if (presentationFailure) { + return presentationFailure; + } if (result?.blocked) { return formatBlockedInstallPolicyResult({ blocked: result.blocked, @@ -747,11 +825,7 @@ async function runOperatorInstallPolicy(params: { reason: formatInstallPolicyNotice({ decision: "warn", findings: result.findings, - guidance: [ - "To continue:", - " • Rerun interactively and approve the warning.", - ` • For reviewed automation, add ${INSTALL_POLICY_ACKNOWLEDGEMENT_FLAG}.`, - ], + guidance: INSTALL_POLICY_REVIEW_GUIDANCE, reason: result.warning.reason, targetName: params.targetName, targetType: params.targetType, @@ -767,6 +841,14 @@ async function runOperatorInstallPolicy(params: { }); if (acknowledgement.status === "approved") { const reevaluated = await evaluatePolicy(); + const reevaluatedPresentationFailure = failOversizedInstallPolicyWarning({ + result: reevaluated, + targetName: params.targetName, + targetType: params.targetType, + }); + if (reevaluatedPresentationFailure) { + return reevaluatedPresentationFailure; + } if (reevaluated?.blocked) { return formatBlockedInstallPolicyResult({ blocked: reevaluated.blocked,