diff --git a/src/plugins/install-security-scan.runtime.test.ts b/src/plugins/install-security-scan.runtime.test.ts index 27c50c1d97c7..32910acba840 100644 --- a/src/plugins/install-security-scan.runtime.test.ts +++ b/src/plugins/install-security-scan.runtime.test.ts @@ -325,8 +325,12 @@ describe("legacy file install scan compatibility", () => { it("continues after one acknowledgement and a fresh evaluation of the same warning", async () => { const onInstallPolicyWarning = vi.fn().mockResolvedValue({ status: "approved" }); runInstallPolicyMock - .mockResolvedValueOnce({ warning: { reason: "review this plugin" } }) - .mockResolvedValueOnce({ warning: { reason: "review this plugin" } }); + .mockResolvedValueOnce({ + warning: { reason: "review this plugin", fingerprint: "warning-a" }, + }) + .mockResolvedValueOnce({ + warning: { reason: "review this plugin", fingerprint: "warning-a" }, + }); const result = await scanFileInstallSourceRuntime({ filePath: "/tmp/payload.js", @@ -347,9 +351,11 @@ describe("legacy file install scan compatibility", () => { it("requires approval again when policy re-evaluation returns a changed warning", async () => { const onInstallPolicyWarning = vi.fn().mockResolvedValue({ status: "approved" }); runInstallPolicyMock - .mockResolvedValueOnce({ warning: { reason: "review this plugin" } }) .mockResolvedValueOnce({ - warning: { reason: "review the new finding" }, + warning: { reason: "review this plugin", fingerprint: "warning-a" }, + }) + .mockResolvedValueOnce({ + warning: { reason: "review the new finding", fingerprint: "warning-b" }, findings: [ { ruleId: "changed-warning", @@ -376,9 +382,41 @@ describe("legacy file install scan compatibility", () => { expect(runInstallPolicyMock).toHaveBeenCalledTimes(2); }); + it("requires approval again when the full warning changes behind identical display values", async () => { + const onInstallPolicyWarning = vi.fn().mockResolvedValue({ status: "approved" }); + const displayedWarning = { + warning: { reason: "same bounded reason", fingerprint: "full-warning-a" }, + findings: [ + { + ruleId: "same-bounded-finding", + severity: "warn" as const, + message: "same bounded message", + }, + ], + }; + runInstallPolicyMock.mockResolvedValueOnce(displayedWarning).mockResolvedValueOnce({ + ...displayedWarning, + warning: { ...displayedWarning.warning, fingerprint: "full-warning-b" }, + }); + + const result = await scanFileInstallSourceRuntime({ + filePath: "/tmp/payload.js", + logger: {}, + onInstallPolicyWarning, + pluginId: "payload", + }); + + expect(result?.blocked?.reason).toContain("The policy warning changed after approval."); + expect(result?.blocked?.reason).toContain("same bounded reason"); + expect(onInstallPolicyWarning).toHaveBeenCalledTimes(1); + expect(runInstallPolicyMock).toHaveBeenCalledTimes(2); + }); + it("keeps a block from policy re-evaluation terminal", async () => { runInstallPolicyMock - .mockResolvedValueOnce({ warning: { reason: "review this plugin" } }) + .mockResolvedValueOnce({ + warning: { reason: "review this plugin", fingerprint: "warning-a" }, + }) .mockResolvedValueOnce({ blocked: { code: "security_scan_blocked", reason: "now blocked" }, }); @@ -395,7 +433,7 @@ describe("legacy file install scan compatibility", () => { it("keeps the deprecated unsafe flag inert when policy warns", async () => { runInstallPolicyMock.mockResolvedValue({ - warning: { reason: "review this plugin" }, + warning: { reason: "review this plugin", fingerprint: "warning-a" }, }); const result = await scanFileInstallSourceRuntime({ @@ -424,7 +462,9 @@ describe("legacy file install scan compatibility", () => { it("keeps a block from acknowledged policy re-evaluation terminal", async () => { runInstallPolicyMock - .mockResolvedValueOnce({ warning: { reason: "review this plugin" } }) + .mockResolvedValueOnce({ + warning: { reason: "review this plugin", fingerprint: "warning-a" }, + }) .mockResolvedValueOnce({ blocked: { code: "security_scan_blocked", reason: "now blocked" }, }); @@ -442,7 +482,7 @@ describe("legacy file install scan compatibility", () => { it("distinguishes an exhausted noninteractive approval from cancellation", async () => { runInstallPolicyMock.mockResolvedValue({ - warning: { reason: "review the dependency warning" }, + warning: { reason: "review the dependency warning", fingerprint: "warning-a" }, findings: [ { ruleId: "dependency-warning", @@ -473,7 +513,7 @@ describe("legacy file install scan compatibility", () => { it("renders warning details as one readable review notice", async () => { const warnings: string[] = []; runInstallPolicyMock.mockResolvedValue({ - warning: { reason: "review this plugin" }, + warning: { reason: "review this plugin", fingerprint: "warning-a" }, findings: [{ ruleId: "context", severity: "info", message: "Informational context." }], }); diff --git a/src/plugins/install-security-scan.runtime.ts b/src/plugins/install-security-scan.runtime.ts index f3a8aad798c4..eec74c94250a 100644 --- a/src/plugins/install-security-scan.runtime.ts +++ b/src/plugins/install-security-scan.runtime.ts @@ -1,7 +1,6 @@ // Runtime bridge for plugin install security scanning. import fs from "node:fs/promises"; import path from "node:path"; -import { isDeepStrictEqual } from "node:util"; import { sanitizeTerminalText } from "../../packages/terminal-core/src/safe-text.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { formatErrorMessage } from "../infra/errors.js"; @@ -776,9 +775,7 @@ async function runOperatorInstallPolicy(params: { }); } if (reevaluated?.warning) { - const warningUnchanged = - reevaluated.warning.reason === result.warning.reason && - isDeepStrictEqual(reevaluated.findings ?? [], result.findings ?? []); + const warningUnchanged = reevaluated.warning.fingerprint === result.warning.fingerprint; if (!warningUnchanged) { return { blocked: { diff --git a/src/security/install-policy.test.ts b/src/security/install-policy.test.ts index 3dcb6a3d340f..e43633c0d097 100644 --- a/src/security/install-policy.test.ts +++ b/src/security/install-policy.test.ts @@ -435,7 +435,10 @@ describe("runInstallPolicy", () => { }); expect(result).toEqual({ - warning: { reason: "review this source" }, + warning: { + reason: "review this source", + fingerprint: expect.stringMatching(/^[a-f0-9]{64}$/), + }, findings: [ { ruleId: "manual-review", @@ -447,6 +450,53 @@ describe("runInstallPolicy", () => { expect(debugLogs.filter((message) => message.endsWith(": warned"))).toHaveLength(1); }); + it("fingerprints warning reason changes beyond the display limit", async () => { + const sharedPrefix = "r".repeat(1000); + const runWarning = async (reason: string) => + await runInstallPolicy({ + config: configWithPolicy(scriptPath, { + POLICY_RESPONSE: JSON.stringify({ protocolVersion: 1, decision: "warn", reason }), + }), + request: baseRequest(sourceDir), + }); + + const first = await runWarning(`${sharedPrefix}-first`); + const second = await runWarning(`${sharedPrefix}-second`); + + expect(first?.warning?.reason).toBe(second?.warning?.reason); + expect(first?.warning?.fingerprint).not.toBe(second?.warning?.fingerprint); + }); + + it("fingerprints warning findings beyond the display limit", async () => { + const visibleFindings = Array.from({ length: 100 }, (_, index) => ({ + ruleId: `visible-${String(index)}`, + severity: "warn", + message: `Visible finding ${String(index)}`, + })); + const runWarning = async (extraMessage: string) => + await runInstallPolicy({ + config: configWithPolicy(scriptPath, { + POLICY_RESPONSE: JSON.stringify({ + protocolVersion: 1, + decision: "warn", + reason: "review all findings", + findings: [ + ...visibleFindings, + { ruleId: "hidden-100", severity: "warn", message: extraMessage }, + ], + }), + }), + request: baseRequest(sourceDir), + }); + + const first = await runWarning("First hidden finding"); + const second = await runWarning("Second hidden finding"); + + expect(first?.findings).toEqual(second?.findings); + expect(first?.findings).toHaveLength(100); + expect(first?.warning?.fingerprint).not.toBe(second?.warning?.fingerprint); + }); + it.each([ { label: "missing", reason: undefined }, { label: "empty", reason: " " }, diff --git a/src/security/install-policy.ts b/src/security/install-policy.ts index 88aca8fd3ee9..549e19f2e9ba 100644 --- a/src/security/install-policy.ts +++ b/src/security/install-policy.ts @@ -1,3 +1,4 @@ +import { createHash } from "node:crypto"; // Checks install policy constraints for package and plugin operations. import fs from "node:fs/promises"; import path from "node:path"; @@ -122,7 +123,7 @@ type InstallPolicyResult = | { blocked?: undefined; warning?: undefined; findings?: InstallPolicyFinding[] } | { blocked?: undefined; - warning: { reason: string }; + warning: { reason: string; fingerprint: string }; findings?: InstallPolicyFinding[]; } | { @@ -359,11 +360,7 @@ const installPolicyResponseEnvelopeSchema = z.object({ const installPolicyReasonSchema = z.string().trim().min(1); -const findingTextSchema = z - .string() - .trim() - .min(1) - .transform((value) => truncateText(value, MAX_FINDING_TEXT_CHARS)); +const findingTextSchema = z.string().trim().min(1); const optionalFindingTextSchema = findingTextSchema.optional().catch(undefined); @@ -421,13 +418,6 @@ function blockedByPolicy(reason: string, findings?: InstallPolicyFinding[]): Ins }; } -function warnedByPolicy(reason: string, findings?: InstallPolicyFinding[]): InstallPolicyResult { - return { - warning: { reason: truncateText(reason, MAX_REASON_CHARS) }, - ...(findings?.length ? { findings } : {}), - }; -} - function isTargetEnabled(params: { policy: NonNullable; targetType: InstallPolicyTarget; @@ -527,6 +517,25 @@ function normalizeFinding(value: unknown): InstallPolicyFinding | null { return parsed.success ? parsed.data : null; } +function truncateFinding(finding: InstallPolicyFinding): InstallPolicyFinding { + return { + ruleId: truncateText(finding.ruleId, MAX_FINDING_TEXT_CHARS), + severity: finding.severity, + message: truncateText(finding.message, MAX_FINDING_TEXT_CHARS), + ...(finding.file ? { file: truncateText(finding.file, MAX_FINDING_TEXT_CHARS) } : {}), + ...(finding.line !== undefined ? { line: finding.line } : {}), + ...(finding.evidence + ? { evidence: truncateText(finding.evidence, MAX_FINDING_TEXT_CHARS) } + : {}), + }; +} + +function fingerprintWarning(reason: string, findings: InstallPolicyFinding[]): string { + // Presentation is truncated and capped; approval must bind the complete + // validated warning so hidden suffix or overflow changes still fail closed. + return createHash("sha256").update(JSON.stringify({ reason, findings })).digest("hex"); +} + function formatPolicyResponseEnvelopeError(error: z.ZodError): string { const invalidPath = error.issues[0]?.path[0]; return invalidPath === undefined @@ -552,10 +561,14 @@ function parsePolicyResponse(stdout: string): InstallPolicyResult { if (!response.success) { return blockedByFailure(formatPolicyResponseEnvelopeError(response.error)); } + const fullFindings = (response.data.findings ?? []) + .map(normalizeFinding) + .filter((finding): finding is InstallPolicyFinding => finding !== null); const normalizedFindings = (response.data.findings ?? []) .slice(0, MAX_FINDINGS) .map(normalizeFinding) - .filter((finding): finding is InstallPolicyFinding => finding !== null); + .filter((finding): finding is InstallPolicyFinding => finding !== null) + .map(truncateFinding); if (response.data.decision === "allow") { return normalizedFindings.length > 0 ? { findings: normalizedFindings } : {}; } @@ -566,7 +579,13 @@ function parsePolicyResponse(stdout: string): InstallPolicyResult { ); } if (response.data.decision === "warn") { - return warnedByPolicy(reason.data, normalizedFindings); + return { + warning: { + reason: truncateText(reason.data, MAX_REASON_CHARS), + fingerprint: fingerprintWarning(reason.data, fullFindings), + }, + ...(normalizedFindings.length > 0 ? { findings: normalizedFindings } : {}), + }; } return blockedByPolicy(reason.data, normalizedFindings); }