Skip to content

Commit 9e4eca0

Browse files
kevinslinkevinslin
authored
fix(slack): normalize approval user ids (#84671)
* fix(slack): normalize approval user ids * chore(openrouter): satisfy spread fallback lint * fix(ci): unblock status and secret-file checks
1 parent 404fd6d commit 9e4eca0

5 files changed

Lines changed: 107 additions & 9 deletions

File tree

‎extensions/slack/src/approval-auth.test.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,15 @@ describe("slackApprovalAuth", () => {
2323
}),
2424
).toEqual({ authorized: true });
2525

26+
expect(
27+
slackApprovalAuth.authorizeActorAction({
28+
cfg,
29+
senderId: "u123owner",
30+
action: "approve",
31+
approvalKind: "plugin",
32+
}),
33+
).toEqual({ authorized: true });
34+
2635
expect(
2736
slackApprovalAuth.authorizeActorAction({
2837
cfg,
@@ -32,6 +41,15 @@ describe("slackApprovalAuth", () => {
3241
}),
3342
).toEqual({ authorized: true });
3443

44+
expect(
45+
slackApprovalAuth.authorizeActorAction({
46+
cfg,
47+
senderId: "u345default",
48+
action: "approve",
49+
approvalKind: "plugin",
50+
}),
51+
).toEqual({ authorized: true });
52+
3553
expect(
3654
slackApprovalAuth.authorizeActorAction({
3755
cfg,
@@ -56,4 +74,26 @@ describe("slackApprovalAuth", () => {
5674
reason: "❌ You are not authorized to approve exec requests on Slack.",
5775
});
5876
});
77+
78+
it("canonicalizes configured plugin approver ids before matching uppercase senders", () => {
79+
const cfg = {
80+
channels: {
81+
slack: {
82+
allowFrom: ["slack:u123owner"],
83+
defaultTo: "user:u345default",
84+
},
85+
},
86+
};
87+
88+
for (const senderId of ["U123OWNER", "U345DEFAULT"]) {
89+
expect(
90+
slackApprovalAuth.authorizeActorAction({
91+
cfg,
92+
senderId,
93+
action: "approve",
94+
approvalKind: "plugin",
95+
}),
96+
).toEqual({ authorized: true });
97+
}
98+
});
5999
});

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

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,9 +69,25 @@ describe("slack exec approvals", () => {
6969

7070
expect(getSlackExecApprovalApprovers({ cfg })).toEqual(["U456"]);
7171
expect(isSlackExecApprovalApprover({ cfg, senderId: "U456" })).toBe(true);
72+
expect(isSlackExecApprovalApprover({ cfg, senderId: "u456" })).toBe(true);
7273
expect(isSlackExecApprovalApprover({ cfg, senderId: "U123" })).toBe(false);
7374
});
7475

76+
it("canonicalizes configured exec approver ids before matching uppercase senders", () => {
77+
const explicitCfg = buildConfig({ approvers: ["u456"] });
78+
expect(getSlackExecApprovalApprovers({ cfg: explicitCfg })).toEqual(["U456"]);
79+
expect(isSlackExecApprovalApprover({ cfg: explicitCfg, senderId: "U456" })).toBe(true);
80+
81+
const ownerFallbackCfg = {
82+
...buildConfig({ enabled: true }),
83+
commands: { ownerAllowFrom: ["slack:u123owner"] },
84+
} as OpenClawConfig;
85+
expect(getSlackExecApprovalApprovers({ cfg: ownerFallbackCfg })).toEqual(["U123OWNER"]);
86+
expect(isSlackExecApprovalApprover({ cfg: ownerFallbackCfg, senderId: "U123OWNER" })).toBe(
87+
true,
88+
);
89+
});
90+
7591
it("does not infer approvers from allowFrom or DM default routes", () => {
7692
const cfg = buildConfig(
7793
{ enabled: true },
@@ -115,16 +131,18 @@ describe("slack exec approvals", () => {
115131
enabled: true,
116132
mode: "targets",
117133
targets: [
118-
{ channel: "slack", to: "user:U123TARGET" },
134+
{ channel: "slack", to: "user:u123target" },
119135
{ channel: "slack", to: "channel:C123" },
120136
],
121137
},
122138
},
123139
} as OpenClawConfig;
124140

125141
expect(isSlackExecApprovalTargetRecipient({ cfg, senderId: "U123TARGET" })).toBe(true);
142+
expect(isSlackExecApprovalTargetRecipient({ cfg, senderId: "u123target" })).toBe(true);
126143
expect(isSlackExecApprovalTargetRecipient({ cfg, senderId: "U999OTHER" })).toBe(false);
127144
expect(isSlackExecApprovalAuthorizedSender({ cfg, senderId: "U123TARGET" })).toBe(true);
145+
expect(isSlackExecApprovalAuthorizedSender({ cfg, senderId: "u123target" })).toBe(true);
128146
});
129147

130148
it("keeps the local Slack approval prompt path active", () => {
@@ -154,7 +172,15 @@ describe("slack exec approvals", () => {
154172

155173
it("normalizes wrapped sender ids", () => {
156174
expect(normalizeSlackApproverId("user:U123OWNER")).toBe("U123OWNER");
175+
expect(normalizeSlackApproverId("user:u123owner")).toBe("U123OWNER");
176+
expect(normalizeSlackApproverId("slack:u123owner")).toBe("U123OWNER");
157177
expect(normalizeSlackApproverId("<@U123OWNER>")).toBe("U123OWNER");
178+
expect(normalizeSlackApproverId("<@u123owner>")).toBe("U123OWNER");
179+
expect(normalizeSlackApproverId("u123owner")).toBe("U123OWNER");
180+
expect(normalizeSlackApproverId("C123CHANNEL")).toBeUndefined();
181+
expect(normalizeSlackApproverId("slack:C123CHANNEL")).toBeUndefined();
182+
expect(normalizeSlackApproverId("user:C123CHANNEL")).toBeUndefined();
183+
expect(normalizeSlackApproverId("<@C123CHANNEL>")).toBeUndefined();
158184
});
159185

160186
it("applies agent and session filters to request handling", () => {

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,25 @@ import type { OpenClawConfig } from "openclaw/plugin-sdk/config-contracts";
88
import { normalizeStringifiedOptionalString } from "openclaw/plugin-sdk/string-coerce-runtime";
99
import { resolveSlackAccount } from "./accounts.js";
1010

11+
function normalizeSlackUserLikeId(value: string): string | undefined {
12+
const upper = value.toUpperCase();
13+
return /^[UW][A-Z0-9]+$/.test(upper) ? upper : undefined;
14+
}
15+
1116
export function normalizeSlackApproverId(value: string | number): string | undefined {
1217
const trimmed = normalizeStringifiedOptionalString(value);
1318
if (!trimmed) {
1419
return undefined;
1520
}
1621
const prefixed = trimmed.match(/^(?:slack|user):([A-Z0-9]+)$/i);
1722
if (prefixed?.[1]) {
18-
return prefixed[1];
23+
return normalizeSlackUserLikeId(prefixed[1]);
1924
}
2025
const mention = trimmed.match(/^<@([A-Z0-9]+)>$/i);
2126
if (mention?.[1]) {
22-
return mention[1];
27+
return normalizeSlackUserLikeId(mention[1]);
2328
}
24-
return /^[UW][A-Z0-9]+$/i.test(trimmed) ? trimmed : undefined;
29+
return normalizeSlackUserLikeId(trimmed);
2530
}
2631

2732
function resolveSlackOwnerApprovers(cfg: OpenClawConfig): string[] {

‎extensions/slack/src/monitor/events/interactions.test.ts‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -934,7 +934,20 @@ describe("registerSlackInteractionEvents", () => {
934934
});
935935

936936
it("resolves exec approvals from shared interactive Slack actions", async () => {
937-
const { ctx, app, getHandler } = createContext({ allowFrom: ["U999"] });
937+
const { ctx, app, getHandler } = createContext({
938+
allowFrom: ["U999"],
939+
cfg: {
940+
channels: {
941+
slack: {
942+
execApprovals: {
943+
enabled: true,
944+
approvers: ["u123"],
945+
target: "both",
946+
},
947+
},
948+
},
949+
},
950+
});
938951
registerSlackInteractionEvents({ ctx: ctx as never });
939952

940953
const handler = getHandler();
@@ -997,7 +1010,7 @@ describe("registerSlackInteractionEvents", () => {
9971010
slack: {
9981011
accounts: {
9991012
default: {
1000-
allowFrom: ["U123OWNER"],
1013+
allowFrom: ["u123owner"],
10011014
execApprovals: {
10021015
enabled: true,
10031016
approvers: ["U999EXEC"],
@@ -1071,7 +1084,7 @@ describe("registerSlackInteractionEvents", () => {
10711084
slack: {
10721085
accounts: {
10731086
default: {
1074-
allowFrom: ["U123OWNER"],
1087+
allowFrom: ["u123owner"],
10751088
execApprovals: {
10761089
enabled: true,
10771090
approvers: ["U999EXEC"],

‎src/infra/secret-file.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,31 @@
11
import "./fs-safe-defaults.js";
2-
import { readSecretFileSync as readSecretFileSyncImpl } from "@openclaw/fs-safe/secret";
2+
import {
3+
readSecretFileSync as readSecretFileSyncImpl,
4+
tryReadSecretFileSync as tryReadSecretFileSyncImpl,
5+
} from "@openclaw/fs-safe/secret";
36
import { resolveUserPath } from "../utils.js";
47

58
export {
69
DEFAULT_SECRET_FILE_MAX_BYTES,
710
PRIVATE_SECRET_DIR_MODE,
811
PRIVATE_SECRET_FILE_MODE,
912
readSecretFileSync,
10-
tryReadSecretFileSync,
1113
type SecretFileReadOptions,
1214
} from "@openclaw/fs-safe/secret";
1315
export { writeSecretFileAtomic as writePrivateSecretFileAtomic } from "@openclaw/fs-safe/secret";
1416

17+
export function tryReadSecretFileSync(
18+
filePath: string | undefined,
19+
label: string,
20+
options: Parameters<typeof tryReadSecretFileSyncImpl>[2] = {},
21+
): string | undefined {
22+
try {
23+
return tryReadSecretFileSyncImpl(filePath, label, options);
24+
} catch {
25+
return undefined;
26+
}
27+
}
28+
1529
export type SecretFileReadResult =
1630
| {
1731
ok: true;

0 commit comments

Comments
 (0)