Skip to content

Commit 2d53ffd

Browse files
vincentkocscoootscooob
vincentkoc
and
scoootscooob
authored
fix(exec): resolve remote approval regressions (#58792)
* fix(exec): restore remote approval policy defaults * fix(exec): handle headless cron approval conflicts * fix(exec): make allow-always durable * fix(exec): persist exact-command shell trust * fix(doctor): match host exec fallback * fix(exec): preserve blocked and inline approval state * Doctor: surface allow-always ask bypass * Doctor: match effective exec policy * Exec: match node durable command text * Exec: tighten durable approval security * Exec: restore owner approver fallback * Config: refresh Slack approval metadata --------- Co-authored-by: scoootscooob <zhentongfan@gmail.com>
1 parent 4ceb01f commit 2d53ffd

34 files changed

Lines changed: 1608 additions & 225 deletions

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ Docs: https://docs.openclaw.ai
1515

1616
### Fixes
1717

18+
- Exec/approvals: honor `exec-approvals.json` security defaults when inline or configured tool policy is unset, and keep Slack and Discord native approval handling aligned with inferred approvers and real channel enablement so remote exec stops falling into false approval timeouts and disabled states. Thanks @scoootscooob and @vincentkoc.
19+
- Exec/cron: resolve isolated cron no-route approval dead-ends from the effective host fallback policy when trusted automation is allowed, and make `openclaw doctor` warn when `tools.exec` is broader than `~/.openclaw/exec-approvals.json` so stricter host-policy conflicts are explicit. Thanks @scoootscooob and @vincentkoc.
20+
- Exec/approvals: make `allow-always` persist as durable user-approved trust instead of behaving like `allow-once`, reuse exact-command trust on shell-wrapper paths that cannot safely persist an executable allowlist entry, keep static allowlist entries from silently bypassing `ask:"always"`, and require explicit approval when Windows cannot build an allowlist execution plan instead of hard-dead-ending remote exec. Thanks @scoootscooob and @vincentkoc.
1821
- Gateway/reload: ignore startup config writes by persisted hash in the config reloader so generated auth tokens and seeded Control UI origins do not trigger a restart loop, while real `gateway.auth.*` edits still require restart. (#58678) Thanks @yelog
1922
- Discord/inbound media: pass Discord attachment and sticker downloads through the shared idle-timeout and worker-abort path so slow or stuck inbound media fetches stop hanging message processing. (#58593) Thanks @aquaright1
2023
- Telegram/local Bot API: preserve media MIME types for absolute-path downloads so local audio files still trigger transcription and other MIME-based handling. (#54603) Thanks @jzakirov

‎extensions/discord/src/exec-approvals.test.ts‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -22,13 +22,26 @@ function buildConfig(
2222
}
2323

2424
describe("discord exec approvals", () => {
25-
it("requires enablement and an explicit or inferred approver", () => {
25+
it("requires enablement and explicit or owner approvers", () => {
2626
expect(isDiscordExecApprovalClientEnabled({ cfg: buildConfig() })).toBe(false);
2727
expect(isDiscordExecApprovalClientEnabled({ cfg: buildConfig({ enabled: true }) })).toBe(false);
2828
expect(
2929
isDiscordExecApprovalClientEnabled({
3030
cfg: buildConfig({ enabled: true }, { allowFrom: ["123"] }),
3131
}),
32+
).toBe(false);
33+
expect(
34+
isDiscordExecApprovalClientEnabled({
35+
cfg: buildConfig({ enabled: true, approvers: ["123"] }),
36+
}),
37+
).toBe(true);
38+
expect(
39+
isDiscordExecApprovalClientEnabled({
40+
cfg: {
41+
...buildConfig({ enabled: true }),
42+
commands: { ownerAllowFrom: ["discord:789"] },
43+
} as OpenClawConfig,
44+
}),
3245
).toBe(true);
3346
});
3447

@@ -43,7 +56,7 @@ describe("discord exec approvals", () => {
4356
expect(isDiscordExecApprovalApprover({ cfg, senderId: "123" })).toBe(false);
4457
});
4558

46-
it("infers approvers from allowFrom, legacy dm.allowFrom, and explicit DM defaultTo", () => {
59+
it("does not infer approvers from allowFrom or default DM routes", () => {
4760
const cfg = buildConfig(
4861
{ enabled: true },
4962
{
@@ -53,18 +66,17 @@ describe("discord exec approvals", () => {
5366
},
5467
);
5568

56-
expect(getDiscordExecApprovalApprovers({ cfg })).toEqual(["123", "456", "789"]);
57-
expect(isDiscordExecApprovalApprover({ cfg, senderId: "789" })).toBe(true);
69+
expect(getDiscordExecApprovalApprovers({ cfg })).toEqual([]);
70+
expect(isDiscordExecApprovalApprover({ cfg, senderId: "789" })).toBe(false);
5871
});
5972

60-
it("ignores non-user default targets when inferring approvers", () => {
61-
const cfg = buildConfig(
62-
{ enabled: true },
63-
{
64-
defaultTo: "channel:123",
65-
},
66-
);
73+
it("falls back to commands.ownerAllowFrom for exec approvers", () => {
74+
const cfg = {
75+
...buildConfig({ enabled: true }),
76+
commands: { ownerAllowFrom: ["discord:123", "user:456", "789"] },
77+
} as OpenClawConfig;
6778

68-
expect(getDiscordExecApprovalApprovers({ cfg })).toEqual([]);
79+
expect(getDiscordExecApprovalApprovers({ cfg })).toEqual(["123", "456", "789"]);
80+
expect(isDiscordExecApprovalApprover({ cfg, senderId: "456" })).toBe(true);
6981
});
7082
});

‎extensions/discord/src/exec-approvals.ts‎

Lines changed: 15 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -22,26 +22,28 @@ function normalizeDiscordApproverId(value: string): string | undefined {
2222
}
2323
}
2424

25+
function resolveDiscordOwnerApprovers(cfg: OpenClawConfig): string[] {
26+
const ownerAllowFrom = cfg.commands?.ownerAllowFrom;
27+
if (!Array.isArray(ownerAllowFrom) || ownerAllowFrom.length === 0) {
28+
return [];
29+
}
30+
return resolveApprovalApprovers({
31+
explicit: ownerAllowFrom,
32+
normalizeApprover: (value) => normalizeDiscordApproverId(String(value)),
33+
});
34+
}
35+
2536
export function getDiscordExecApprovalApprovers(params: {
2637
cfg: OpenClawConfig;
2738
accountId?: string | null;
2839
configOverride?: DiscordExecApprovalConfig | null;
2940
}): string[] {
30-
const account = resolveDiscordAccount(params).config;
3141
return resolveApprovalApprovers({
32-
explicit: params.configOverride?.approvers ?? account.execApprovals?.approvers,
33-
allowFrom: account.allowFrom,
34-
extraAllowFrom: account.dm?.allowFrom,
35-
defaultTo: account.defaultTo,
42+
explicit:
43+
params.configOverride?.approvers ??
44+
resolveDiscordAccount(params).config.execApprovals?.approvers ??
45+
resolveDiscordOwnerApprovers(params.cfg),
3646
normalizeApprover: (value) => normalizeDiscordApproverId(String(value)),
37-
normalizeDefaultTo: (value) => {
38-
try {
39-
const target = parseDiscordTarget(value);
40-
return target?.kind === "user" ? target.id : undefined;
41-
} catch {
42-
return undefined;
43-
}
44-
},
4547
});
4648
}
4749

‎extensions/discord/src/monitor/exec-approvals.test.ts‎

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -177,12 +177,16 @@ type ExecApprovalButtonContext = import("./exec-approvals.js").ExecApprovalButto
177177

178178
// ─── Helpers ──────────────────────────────────────────────────────────────────
179179

180-
function createHandler(config: DiscordExecApprovalConfig, accountId = "default") {
180+
function createHandler(
181+
config: DiscordExecApprovalConfig,
182+
accountId = "default",
183+
cfgOverrides: Record<string, unknown> = {},
184+
) {
181185
return new DiscordExecApprovalHandler({
182186
token: "test-token",
183187
accountId,
184188
config,
185-
cfg: { session: { store: STORE_PATH } },
189+
cfg: { session: { store: STORE_PATH }, ...cfgOverrides },
186190
});
187191
}
188192

@@ -447,6 +451,18 @@ describe("DiscordExecApprovalHandler.shouldHandle", () => {
447451
expect(handler.shouldHandle(createRequest())).toBe(false);
448452
});
449453

454+
it("does not treat channel allowFrom as approval authority", () => {
455+
const handler = createHandler({ enabled: true }, "default", {
456+
channels: {
457+
discord: {
458+
token: "discord-token",
459+
allowFrom: ["123"],
460+
},
461+
},
462+
});
463+
expect(handler.shouldHandle(createRequest())).toBe(false);
464+
});
465+
450466
it("returns true with minimal config", () => {
451467
const handler = createHandler({ enabled: true, approvers: ["123"] });
452468
expect(handler.shouldHandle(createRequest())).toBe(true);
@@ -617,10 +633,37 @@ describe("DiscordExecApprovalHandler.getApprovers", () => {
617633
config: { enabled: true } as DiscordExecApprovalConfig,
618634
expected: [],
619635
},
636+
{
637+
name: "allowFrom does not grant approver rights",
638+
config: { enabled: true } as DiscordExecApprovalConfig,
639+
cfgOverrides: {
640+
channels: {
641+
discord: {
642+
token: "discord-token",
643+
allowFrom: ["123"],
644+
},
645+
},
646+
},
647+
expected: [],
648+
},
649+
{
650+
name: "ownerAllowFrom still grants exec approver rights",
651+
config: { enabled: true } as DiscordExecApprovalConfig,
652+
cfgOverrides: {
653+
commands: {
654+
ownerAllowFrom: ["discord:123"],
655+
},
656+
},
657+
expected: ["123"],
658+
},
620659
] as const;
621660

622661
for (const testCase of cases) {
623-
const handler = createHandler(testCase.config);
662+
const handler = createHandler(
663+
testCase.config,
664+
"default",
665+
"cfgOverrides" in testCase ? (testCase.cfgOverrides as Record<string, unknown>) : {},
666+
);
624667
expect(handler.getApprovers(), testCase.name).toEqual(testCase.expected);
625668
}
626669
});

‎extensions/discord/src/monitor/exec-approvals.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import type {
3333
import type { RuntimeEnv } from "openclaw/plugin-sdk/runtime-env";
3434
import { logDebug, logError } from "openclaw/plugin-sdk/text-runtime";
3535
import { createDiscordNativeApprovalAdapter } from "../approval-native.js";
36+
import { getDiscordExecApprovalApprovers } from "../exec-approvals.js";
3637
import { createDiscordClient, stripUndefinedFields } from "../send.shared.js";
3738
import { DiscordUiContainer } from "../ui.js";
3839

@@ -454,8 +455,7 @@ export class DiscordExecApprovalHandler {
454455
cfg: this.opts.cfg,
455456
gatewayUrl: this.opts.gatewayUrl,
456457
eventKinds: ["exec", "plugin"],
457-
isConfigured: () =>
458-
Boolean(this.opts.config.enabled && (this.opts.config.approvers?.length ?? 0) > 0),
458+
isConfigured: () => Boolean(this.opts.config.enabled && this.getApprovers().length > 0),
459459
shouldHandle: (request) => this.shouldHandle(request),
460460
deliverRequested: async (request) => await this.deliverRequested(request),
461461
finalizeResolved: async ({ request, resolved, entries }) => {
@@ -472,7 +472,7 @@ export class DiscordExecApprovalHandler {
472472
if (!config.enabled) {
473473
return false;
474474
}
475-
if (!config.approvers || config.approvers.length === 0) {
475+
if (this.getApprovers().length === 0) {
476476
return false;
477477
}
478478

@@ -763,7 +763,11 @@ export class DiscordExecApprovalHandler {
763763

764764
/** Return the list of configured approver IDs. */
765765
getApprovers(): string[] {
766-
return this.opts.config.approvers ?? [];
766+
return getDiscordExecApprovalApprovers({
767+
cfg: this.opts.cfg,
768+
accountId: this.opts.accountId,
769+
configOverride: this.opts.config,
770+
});
767771
}
768772
}
769773

‎extensions/slack/src/config-ui-hints.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ export const slackChannelConfigUiHints = {
5959
},
6060
"execApprovals.approvers": {
6161
label: "Slack Exec Approval Approvers",
62-
help: "Slack user IDs allowed to approve exec requests for this workspace account. Use Slack user IDs or user targets such as `U123`, `user:U123`, or `<@U123>`. If you leave this unset, OpenClaw falls back to owner IDs inferred from channels.slack.allowFrom, channels.slack.dm.allowFrom, and defaultTo when possible.",
62+
help: "Slack user IDs allowed to approve exec requests for this workspace account. Use Slack user IDs or user targets such as `U123`, `user:U123`, or `<@U123>`. If you leave this unset, OpenClaw falls back to commands.ownerAllowFrom when possible.",
6363
},
6464
"execApprovals.agentFilter": {
6565
label: "Slack Exec Approval Agent Filter",

‎extensions/slack/src/exec-approvals.test.ts‎

Lines changed: 20 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,19 +29,27 @@ function buildConfig(
2929
}
3030

3131
describe("slack exec approvals", () => {
32-
it("requires enablement and an explicit or inferred approver", () => {
32+
it("requires enablement and explicit or owner approvers", () => {
3333
expect(isSlackExecApprovalClientEnabled({ cfg: buildConfig() })).toBe(false);
3434
expect(isSlackExecApprovalClientEnabled({ cfg: buildConfig({ enabled: true }) })).toBe(false);
3535
expect(
3636
isSlackExecApprovalClientEnabled({
3737
cfg: buildConfig({ enabled: true }, { allowFrom: ["U123"] }),
3838
}),
39-
).toBe(true);
39+
).toBe(false);
4040
expect(
4141
isSlackExecApprovalClientEnabled({
4242
cfg: buildConfig({ enabled: true, approvers: ["U123"] }),
4343
}),
4444
).toBe(true);
45+
expect(
46+
isSlackExecApprovalClientEnabled({
47+
cfg: {
48+
...buildConfig({ enabled: true }),
49+
commands: { ownerAllowFrom: ["slack:U123OWNER"] },
50+
} as OpenClawConfig,
51+
}),
52+
).toBe(true);
4553
});
4654

4755
it("prefers explicit approvers when configured", () => {
@@ -55,7 +63,7 @@ describe("slack exec approvals", () => {
5563
expect(isSlackExecApprovalApprover({ cfg, senderId: "U123" })).toBe(false);
5664
});
5765

58-
it("infers approvers from allowFrom, dm.allowFrom, and DM defaultTo", () => {
66+
it("does not infer approvers from allowFrom or DM default routes", () => {
5967
const cfg = buildConfig(
6068
{ enabled: true },
6169
{
@@ -65,19 +73,18 @@ describe("slack exec approvals", () => {
6573
},
6674
);
6775

68-
expect(getSlackExecApprovalApprovers({ cfg })).toEqual(["U123", "U456", "U789"]);
69-
expect(isSlackExecApprovalApprover({ cfg, senderId: "U789" })).toBe(true);
76+
expect(getSlackExecApprovalApprovers({ cfg })).toEqual([]);
77+
expect(isSlackExecApprovalApprover({ cfg, senderId: "U789" })).toBe(false);
7078
});
7179

72-
it("ignores non-user default targets when inferring approvers", () => {
73-
const cfg = buildConfig(
74-
{ enabled: true },
75-
{
76-
defaultTo: "channel:C123",
77-
},
78-
);
80+
it("falls back to commands.ownerAllowFrom for exec approvers", () => {
81+
const cfg = {
82+
...buildConfig({ enabled: true }),
83+
commands: { ownerAllowFrom: ["slack:U123", "user:U456", "<@U789>"] },
84+
} as OpenClawConfig;
7985

80-
expect(getSlackExecApprovalApprovers({ cfg })).toEqual([]);
86+
expect(getSlackExecApprovalApprovers({ cfg })).toEqual(["U123", "U456", "U789"]);
87+
expect(isSlackExecApprovalApprover({ cfg, senderId: "U456" })).toBe(true);
8188
});
8289

8390
it("defaults target to dm", () => {

‎extensions/slack/src/exec-approvals.ts‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,17 @@ export function normalizeSlackApproverId(value: string | number): string | undef
2828
return /^[UW][A-Z0-9]+$/i.test(trimmed) ? trimmed : undefined;
2929
}
3030

31+
function resolveSlackOwnerApprovers(cfg: OpenClawConfig): string[] {
32+
const ownerAllowFrom = cfg.commands?.ownerAllowFrom;
33+
if (!Array.isArray(ownerAllowFrom) || ownerAllowFrom.length === 0) {
34+
return [];
35+
}
36+
return resolveApprovalApprovers({
37+
explicit: ownerAllowFrom,
38+
normalizeApprover: normalizeSlackApproverId,
39+
});
40+
}
41+
3142
export function shouldHandleSlackExecApprovalRequest(params: {
3243
cfg: OpenClawConfig;
3344
accountId?: string | null;
@@ -61,14 +72,11 @@ export function getSlackExecApprovalApprovers(params: {
6172
cfg: OpenClawConfig;
6273
accountId?: string | null;
6374
}): string[] {
64-
const account = resolveSlackAccount(params).config;
6575
return resolveApprovalApprovers({
66-
explicit: account.execApprovals?.approvers,
67-
allowFrom: account.allowFrom,
68-
extraAllowFrom: account.dm?.allowFrom,
69-
defaultTo: account.defaultTo,
76+
explicit:
77+
resolveSlackAccount(params).config.execApprovals?.approvers ??
78+
resolveSlackOwnerApprovers(params.cfg),
7079
normalizeApprover: normalizeSlackApproverId,
71-
normalizeDefaultTo: normalizeSlackApproverId,
7280
});
7381
}
7482

0 commit comments

Comments
 (0)