mirror of
https://github.com/openclaw/openclaw.git
synced 2026-07-09 15:59:30 +00:00
fix(security): fail closed on approval persistence errors
This commit is contained in:
parent
c779185bd9
commit
4e14978f8c
6 changed files with 82 additions and 26 deletions
|
|
@ -28,6 +28,7 @@ Docs: https://docs.openclaw.ai
|
|||
- **Lean local model shell access:** keep `exec` directly visible beside the default structured Tool Search controls so coding-tuned local models can use their shell fallback instead of searching for missing domain tools. (#87587) Thanks @vincentkoc.
|
||||
- **OAuth refresh contention diagnostics:** keep local lock paths out of user-facing refresh failures and avoid duplicate failure prefixes while preserving structured provider and profile classification. (#83383) Thanks @vincentkoc.
|
||||
- **Exec approval prompts:** keep background-disabled fallback warnings out of pending gateway/node approvals and show them only after a command actually runs in the foreground. (#78184) Thanks @vincentkoc.
|
||||
- **Exec approval revocation races:** serialize CLI, runtime, and macOS approval mutations against the current on-disk policy so stale metadata or settings snapshots cannot restore revoked execution permissions. (#78225, #78226) Thanks @coygeek.
|
||||
- **Direct poll delivery:** route direct and hybrid channel polls through the owning outbound adapter while preserving gateway-mode routing and channel option checks. (#99950) Thanks @NianJiuZst.
|
||||
- **Agent wait hard-timeout snapshots:** preserve canonical hard-timeout phase and timestamps when the outer `agent.wait` timer wins the retry-grace race, while leaving queue, draining, and restart-cancelled waits correctable. (#89367) Thanks @Pick-cat.
|
||||
- **Control UI typed approvals:** send `/approve` commands immediately through the authorized Gateway command path while an agent run is blocked instead of queueing the command behind that run. (#77672) Thanks @vincentkoc.
|
||||
|
|
|
|||
|
|
@ -1471,6 +1471,33 @@ EOF`,
|
|||
expect(requireSentFollowupText(0)).toContain("done");
|
||||
});
|
||||
|
||||
it("fails closed when detached approval metadata cannot be persisted", async () => {
|
||||
resolveApprovalDecisionOrUndefinedMock.mockResolvedValue("allow-once");
|
||||
createExecApprovalDecisionStateMock.mockReturnValue({
|
||||
baseDecision: { timedOut: false },
|
||||
approvedByAsk: true,
|
||||
deniedReason: null,
|
||||
});
|
||||
recordAllowlistMatchesUseMock.mockRejectedValueOnce(new Error("approval lock unavailable"));
|
||||
buildExecApprovalFollowupTargetMock.mockImplementation((value) => value);
|
||||
|
||||
const result = await runGatewayAllowlist({ command: "echo approved" });
|
||||
|
||||
expect(result.pendingResult?.details.status).toBe("approval-pending");
|
||||
await vi.waitFor(() => {
|
||||
expect(sendExecApprovalFollowupResultMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
expect(requireSentFollowupText(0)).toContain("approval-state-write-failed");
|
||||
expect(runExecProcessMock).not.toHaveBeenCalled();
|
||||
expect(emitTrustedSecurityEventMock).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
action: "exec.approval.denied",
|
||||
outcome: "error",
|
||||
policy: expect.objectContaining({ reason: "approval-state-write-failed" }),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("waits inline for webchat approval so the exec tool can return real output to the model", async () => {
|
||||
resolveApprovalDecisionOrUndefinedMock.mockResolvedValue("allow-once");
|
||||
createExecApprovalDecisionStateMock.mockReturnValue({
|
||||
|
|
|
|||
|
|
@ -925,15 +925,39 @@ export async function processGatewayAllowlist(
|
|||
turnSourceThreadId: params.turnSourceThreadId,
|
||||
direct: params.approvalFollowupMode === "direct",
|
||||
});
|
||||
const denyApprovalStateWriteFailure = async () => {
|
||||
emitGatewayExecApprovalSecurityEvent({
|
||||
action: "exec.approval.denied",
|
||||
outcome: "error",
|
||||
severity: "high",
|
||||
agentId: params.agentId,
|
||||
reason: "approval-state-write-failed",
|
||||
hostSecurity,
|
||||
hostAsk,
|
||||
host: "gateway",
|
||||
segmentCount: allowlistEval.segments.length,
|
||||
trigger: params.trigger,
|
||||
});
|
||||
await sendExecApprovalFollowupResult(
|
||||
followupTarget,
|
||||
`Exec denied (gateway id=${approvalId}, approval-state-write-failed): ${params.command}`,
|
||||
);
|
||||
};
|
||||
|
||||
void (async () => {
|
||||
const approvalDecision = await resolveApprovalForExecution(
|
||||
() =>
|
||||
void sendExecApprovalFollowupResult(
|
||||
followupTarget,
|
||||
`Exec denied (gateway id=${approvalId}, approval-request-failed): ${params.command}`,
|
||||
),
|
||||
);
|
||||
let approvalDecision: Awaited<ReturnType<typeof resolveApprovalForExecution>>;
|
||||
try {
|
||||
approvalDecision = await resolveApprovalForExecution(
|
||||
() =>
|
||||
void sendExecApprovalFollowupResult(
|
||||
followupTarget,
|
||||
`Exec denied (gateway id=${approvalId}, approval-request-failed): ${params.command}`,
|
||||
),
|
||||
);
|
||||
} catch {
|
||||
await denyApprovalStateWriteFailure();
|
||||
return;
|
||||
}
|
||||
if (approvalDecision.requestFailed) {
|
||||
return;
|
||||
}
|
||||
|
|
@ -946,7 +970,12 @@ export async function processGatewayAllowlist(
|
|||
return;
|
||||
}
|
||||
|
||||
await recordMatchedAllowlistUse(resolvedPath ?? undefined);
|
||||
try {
|
||||
await recordMatchedAllowlistUse(resolvedPath ?? undefined);
|
||||
} catch {
|
||||
await denyApprovalStateWriteFailure();
|
||||
return;
|
||||
}
|
||||
|
||||
let run: Awaited<ReturnType<typeof runExecProcess>> | null;
|
||||
try {
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ import {
|
|||
} from "./exec-approvals.js";
|
||||
|
||||
describe("exec approvals wildcard agent", () => {
|
||||
it("merges wildcard allowlist entries with agent entries", async () => {
|
||||
it("merges wildcard allowlist entries with agent entries", () => {
|
||||
const dir = makeTempDir();
|
||||
const prevOpenClawHome = process.env.OPENCLAW_HOME;
|
||||
|
||||
|
|
@ -39,7 +39,7 @@ describe("exec approvals wildcard agent", () => {
|
|||
),
|
||||
);
|
||||
|
||||
const resolved = await resolveExecApprovals("main");
|
||||
const resolved = resolveExecApprovals("main");
|
||||
expect(resolved.allowlist.map((entry) => entry.pattern)).toEqual([
|
||||
"/bin/hostname",
|
||||
"/usr/bin/uname",
|
||||
|
|
|
|||
|
|
@ -137,7 +137,7 @@ describe("exec approvals store helpers", () => {
|
|||
expect(resolveExecApprovalsDisplayPath()).toBe("~/.openclaw/exec-approvals.json");
|
||||
});
|
||||
|
||||
it("uses OPENCLAW_STATE_DIR for default file and socket paths", async () => {
|
||||
it("uses OPENCLAW_STATE_DIR for default file and socket paths", () => {
|
||||
const dir = createHomeDir();
|
||||
const stateDir = path.join(dir, "custom-state");
|
||||
setTestEnvValue("OPENCLAW_STATE_DIR", stateDir);
|
||||
|
|
@ -151,7 +151,7 @@ describe("exec approvals store helpers", () => {
|
|||
expect(resolveExecApprovalsDisplayPath()).toBe(stateApprovalsFilePath(stateDir));
|
||||
expect(resolveExecApprovalsTranscriptPath()).toBe("$OPENCLAW_STATE_DIR/exec-approvals.json");
|
||||
|
||||
const ensured = await ensureExecApprovals();
|
||||
const ensured = ensureExecApprovals();
|
||||
|
||||
expect(ensured.socket?.path).toBe(resolveExecApprovalsSocketPath());
|
||||
expect(fs.existsSync(stateApprovalsFilePath(stateDir))).toBe(true);
|
||||
|
|
@ -183,7 +183,7 @@ describe("exec approvals store helpers", () => {
|
|||
|
||||
it("persists synchronous compatibility writes without restoring revoked policy", async () => {
|
||||
const dir = createHomeDir();
|
||||
await ensureExecApprovals();
|
||||
ensureExecApprovals();
|
||||
await updateExecApprovals({
|
||||
update: (file) => ({
|
||||
...file,
|
||||
|
|
@ -320,7 +320,7 @@ describe("exec approvals store helpers", () => {
|
|||
expect(fs.existsSync(stateApprovalsFilePath(stateDir))).toBe(false);
|
||||
expect(fs.existsSync(approvalsFilePath(dir))).toBe(true);
|
||||
|
||||
const ensured = await ensureExecApprovals();
|
||||
const ensured = ensureExecApprovals();
|
||||
|
||||
expect(ensured.defaults).toEqual({
|
||||
security: "deny",
|
||||
|
|
@ -338,13 +338,13 @@ describe("exec approvals store helpers", () => {
|
|||
expect(fs.existsSync(stateApprovalsFilePath(stateDir))).toBe(false);
|
||||
});
|
||||
|
||||
it("keeps the default approvals path when only legacy state exists", async () => {
|
||||
it("keeps the default approvals path when only legacy state exists", () => {
|
||||
const dir = createHomeDir();
|
||||
fs.mkdirSync(path.join(dir, ".clawdbot"), { recursive: true });
|
||||
|
||||
expect(path.normalize(resolveExecApprovalsPath())).toBe(path.normalize(approvalsFilePath(dir)));
|
||||
|
||||
await ensureExecApprovals();
|
||||
ensureExecApprovals();
|
||||
|
||||
expect(fs.existsSync(approvalsFilePath(dir))).toBe(true);
|
||||
expect(fs.existsSync(path.join(dir, ".clawdbot", "exec-approvals.json"))).toBe(false);
|
||||
|
|
@ -405,10 +405,10 @@ describe("exec approvals store helpers", () => {
|
|||
expect(invalid.file).toEqual(normalizeExecApprovals({ version: 1, agents: {} }));
|
||||
});
|
||||
|
||||
it("ensures approvals file with default socket path and generated token", async () => {
|
||||
it("ensures approvals file with default socket path and generated token", () => {
|
||||
const dir = createHomeDir();
|
||||
|
||||
const ensured = await ensureExecApprovals();
|
||||
const ensured = ensureExecApprovals();
|
||||
const raw = fs.readFileSync(approvalsFilePath(dir), "utf8");
|
||||
|
||||
expect(ensured.socket?.path).toBe(resolveExecApprovalsSocketPath());
|
||||
|
|
@ -843,7 +843,7 @@ describe("exec approvals store helpers", () => {
|
|||
const dir = createHomeDir();
|
||||
vi.spyOn(Date, "now").mockReturnValue(321_000);
|
||||
|
||||
await ensureExecApprovals();
|
||||
ensureExecApprovals();
|
||||
await persistAllowAlwaysDecision({
|
||||
agentId: "worker",
|
||||
decision: {
|
||||
|
|
@ -864,7 +864,7 @@ describe("exec approvals store helpers", () => {
|
|||
|
||||
it("applies allow-always grants to the latest file after a concurrent policy write", async () => {
|
||||
const dir = createHomeDir();
|
||||
await ensureExecApprovals();
|
||||
ensureExecApprovals();
|
||||
const snapshot = readExecApprovalsSnapshot();
|
||||
|
||||
const policyWrite = updateExecApprovals({
|
||||
|
|
@ -912,7 +912,7 @@ describe("exec approvals store helpers", () => {
|
|||
const dir = createHomeDir();
|
||||
vi.spyOn(Date, "now").mockReturnValue(321_000);
|
||||
|
||||
await ensureExecApprovals();
|
||||
ensureExecApprovals();
|
||||
await persistAllowAlwaysDecision({
|
||||
agentId: "worker",
|
||||
decision: {
|
||||
|
|
@ -1070,7 +1070,7 @@ describe("exec approvals store helpers", () => {
|
|||
const dir = createHomeDir();
|
||||
vi.spyOn(Date, "now").mockReturnValue(654_321);
|
||||
|
||||
await ensureExecApprovals();
|
||||
ensureExecApprovals();
|
||||
const patterns = await persistAllowAlwaysPatterns({
|
||||
agentId: "worker",
|
||||
platform: "win32",
|
||||
|
|
@ -1114,7 +1114,7 @@ describe("exec approvals store helpers", () => {
|
|||
const dir = createHomeDir();
|
||||
vi.spyOn(Date, "now").mockReturnValue(654_322);
|
||||
|
||||
await ensureExecApprovals();
|
||||
ensureExecApprovals();
|
||||
const completePatterns = await persistAllowAlwaysPatterns({
|
||||
agentId: "worker",
|
||||
commandText: "/usr/bin/tool ok",
|
||||
|
|
|
|||
|
|
@ -1843,13 +1843,12 @@ function applyRecordedAllowlistUse(params: {
|
|||
return entry;
|
||||
}
|
||||
changed = true;
|
||||
return {
|
||||
...entry,
|
||||
return Object.assign({}, entry, {
|
||||
id: entry.id ?? crypto.randomUUID(),
|
||||
lastUsedAt: Date.now(),
|
||||
lastUsedCommand: params.command,
|
||||
lastResolvedPath: params.resolvedPath,
|
||||
};
|
||||
});
|
||||
});
|
||||
return changed
|
||||
? {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue