mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-28 05:16:23 -06:00
fix(security): bound install policy notices
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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) => {
|
||||
|
||||
@@ -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<InstallPolicyRequestKind, "skill-install">;
|
||||
|
||||
@@ -103,6 +109,34 @@ export type InstallSecurityScanResult = {
|
||||
};
|
||||
};
|
||||
|
||||
function failOversizedInstallPolicyWarning(params: {
|
||||
result: Awaited<ReturnType<typeof runInstallPolicy>>;
|
||||
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<InstallSecurityScanResult["blocked"]>;
|
||||
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,
|
||||
|
||||
Reference in New Issue
Block a user