Skip to content

Commit f1bf558

Browse files
tonydehnkemukhtharcm
tonydehnke
and
mukhtharcm
authored
fix(doctor): detect groupPolicy=allowlist with empty groupAllowFrom (openclaw#28477)
* fix(doctor): detect groupPolicy=allowlist with empty groupAllowFrom The existing `detectEmptyAllowlistPolicy` check only covers `dmPolicy="allowlist"` with empty `allowFrom`. After the .26 security hardening (`resolveDmGroupAccessDecision` fails closed on empty allowlists), `groupPolicy="allowlist"` without `groupAllowFrom` or `allowFrom` silently drops all group/channel messages with only a verbose-level log. Add a parallel check: when `groupPolicy` is `"allowlist"` and neither `groupAllowFrom` nor `allowFrom` has entries, surface a doctor warning with remediation steps. Closes openclaw#27552 * fix: align empty-array semantics with runtime resolveGroupAllowFromSources The runtime treats groupAllowFrom: [] as unset and falls back to allowFrom, but the doctor check used ?? which treats [] as authoritative. This caused a false warning when groupAllowFrom was explicitly empty but allowFrom had entries. Match runtime behavior: treat empty groupAllowFrom arrays as unset before falling back to allowFrom. * fix: scope group allowlist check to sender-based channels only * fix: align doctor group allowlist semantics (openclaw#28477) (thanks @tonydehnke) --------- Co-authored-by: mukhtharcm <[email protected]>
1 parent 5d51e99 commit f1bf558

File tree

3 files changed

+133
-32
lines changed

3 files changed

+133
-32
lines changed

‎CHANGELOG.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,12 @@ Docs: https://docs.openclaw.ai
6969
- Plugins/Install: clear stale install errors when an npm package is not found so follow-up install attempts report current state correctly. (#25073) Thanks @dalefrieswthat.
7070
- OpenAI Responses/Compaction: rewrite and unify the OpenAI Responses store patches to treat empty `baseUrl` as non-direct, honor `compat.supportsStore=false`, and auto-inject server-side compaction `context_management` for compatible direct OpenAI models (with per-model opt-out/threshold overrides). Landed from contributor PRs #16930 (@OiPunk), #22441 (@EdwardWu7), and #25088 (@MoerAI). Thanks @OiPunk, @EdwardWu7, and @MoerAI.
7171

72+
## Unreleased
73+
74+
### Fixes
75+
76+
- Config/Doctor group allowlist diagnostics: align `groupPolicy: "allowlist"` warnings with per-channel runtime semantics by excluding Google Chat sender-list checks and by warning when no-fallback channels (for example iMessage) omit `groupAllowFrom`, with regression coverage. (#28477) Thanks @tonydehnke.
77+
7278
## 2026.2.26
7379

7480
### Changes

‎src/commands/doctor-config-flow.test.ts‎

Lines changed: 64 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,21 @@ function expectGoogleChatDmAllowFromRepaired(cfg: unknown) {
1919
expect(typed.channels.googlechat.allowFrom).toBeUndefined();
2020
}
2121

22+
async function collectDoctorWarnings(config: Record<string, unknown>): Promise<string[]> {
23+
const noteSpy = vi.spyOn(noteModule, "note").mockImplementation(() => {});
24+
try {
25+
await runDoctorConfigWithInput({
26+
config,
27+
run: loadAndMaybeMigrateDoctorConfig,
28+
});
29+
return noteSpy.mock.calls
30+
.filter((call) => call[1] === "Doctor warnings")
31+
.map((call) => String(call[0]));
32+
} finally {
33+
noteSpy.mockRestore();
34+
}
35+
}
36+
2237
type DiscordGuildRule = {
2338
users: string[];
2439
roles: string[];
@@ -56,31 +71,59 @@ describe("doctor config flow", () => {
5671
});
5772

5873
it("does not warn on mutable account allowlists when dangerous name matching is inherited", async () => {
59-
const noteSpy = vi.spyOn(noteModule, "note").mockImplementation(() => {});
60-
try {
61-
await runDoctorConfigWithInput({
62-
config: {
63-
channels: {
64-
slack: {
65-
dangerouslyAllowNameMatching: true,
66-
accounts: {
67-
work: {
68-
allowFrom: ["alice"],
69-
},
70-
},
74+
const doctorWarnings = await collectDoctorWarnings({
75+
channels: {
76+
slack: {
77+
dangerouslyAllowNameMatching: true,
78+
accounts: {
79+
work: {
80+
allowFrom: ["alice"],
7181
},
7282
},
7383
},
74-
run: loadAndMaybeMigrateDoctorConfig,
75-
});
84+
},
85+
});
86+
expect(doctorWarnings.some((line) => line.includes("mutable allowlist"))).toBe(false);
87+
});
7688

77-
const doctorWarnings = noteSpy.mock.calls
78-
.filter((call) => call[1] === "Doctor warnings")
79-
.map((call) => String(call[0]));
80-
expect(doctorWarnings.some((line) => line.includes("mutable allowlist"))).toBe(false);
81-
} finally {
82-
noteSpy.mockRestore();
83-
}
89+
it("does not warn about sender-based group allowlist for googlechat", async () => {
90+
const doctorWarnings = await collectDoctorWarnings({
91+
channels: {
92+
googlechat: {
93+
groupPolicy: "allowlist",
94+
accounts: {
95+
work: {
96+
groupPolicy: "allowlist",
97+
},
98+
},
99+
},
100+
},
101+
});
102+
103+
expect(
104+
doctorWarnings.some(
105+
(line) => line.includes('groupPolicy is "allowlist"') && line.includes("groupAllowFrom"),
106+
),
107+
).toBe(false);
108+
});
109+
110+
it("warns when imessage group allowlist is empty even if allowFrom is set", async () => {
111+
const doctorWarnings = await collectDoctorWarnings({
112+
channels: {
113+
imessage: {
114+
groupPolicy: "allowlist",
115+
allowFrom: ["+15551234567"],
116+
},
117+
},
118+
});
119+
120+
expect(
121+
doctorWarnings.some(
122+
(line) =>
123+
line.includes('channels.imessage.groupPolicy is "allowlist"') &&
124+
line.includes("does not fall back to allowFrom"),
125+
),
126+
).toBe(true);
84127
});
85128

86129
it("drops unknown keys on repair", async () => {

‎src/commands/doctor-config-flow.ts‎

Lines changed: 63 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1267,10 +1267,34 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] {
12671267

12681268
const warnings: string[] = [];
12691269

1270+
const usesSenderBasedGroupAllowlist = (channelName?: string): boolean => {
1271+
if (!channelName) {
1272+
return true;
1273+
}
1274+
// These channels enforce group access via channel/space config, not sender-based
1275+
// groupAllowFrom lists.
1276+
return !(channelName === "discord" || channelName === "slack" || channelName === "googlechat");
1277+
};
1278+
1279+
const allowsGroupAllowFromFallback = (channelName?: string): boolean => {
1280+
if (!channelName) {
1281+
return true;
1282+
}
1283+
// Keep doctor warnings aligned with runtime access semantics.
1284+
return !(
1285+
channelName === "googlechat" ||
1286+
channelName === "imessage" ||
1287+
channelName === "matrix" ||
1288+
channelName === "msteams" ||
1289+
channelName === "irc"
1290+
);
1291+
};
1292+
12701293
const checkAccount = (
12711294
account: Record<string, unknown>,
12721295
prefix: string,
12731296
parent?: Record<string, unknown>,
1297+
channelName?: string,
12741298
) => {
12751299
const dmEntry = account.dm;
12761300
const dm =
@@ -1289,24 +1313,47 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] {
12891313
(parentDm?.policy as string | undefined) ??
12901314
undefined;
12911315

1292-
if (dmPolicy !== "allowlist") {
1293-
return;
1294-
}
1295-
12961316
const topAllowFrom =
12971317
(account.allowFrom as Array<string | number> | undefined) ??
12981318
(parent?.allowFrom as Array<string | number> | undefined);
12991319
const nestedAllowFrom = dm?.allowFrom as Array<string | number> | undefined;
13001320
const parentNestedAllowFrom = parentDm?.allowFrom as Array<string | number> | undefined;
13011321
const effectiveAllowFrom = topAllowFrom ?? nestedAllowFrom ?? parentNestedAllowFrom;
13021322

1303-
if (hasAllowFromEntries(effectiveAllowFrom)) {
1304-
return;
1323+
if (dmPolicy === "allowlist" && !hasAllowFromEntries(effectiveAllowFrom)) {
1324+
warnings.push(
1325+
`- ${prefix}.dmPolicy is "allowlist" but allowFrom is empty — all DMs will be blocked. Add sender IDs to ${prefix}.allowFrom, or run "${formatCliCommand("openclaw doctor --fix")}" to auto-migrate from pairing store when entries exist.`,
1326+
);
13051327
}
13061328

1307-
warnings.push(
1308-
`- ${prefix}.dmPolicy is "allowlist" but allowFrom is empty — all DMs will be blocked. Add sender IDs to ${prefix}.allowFrom, or run "${formatCliCommand("openclaw doctor --fix")}" to auto-migrate from pairing store when entries exist.`,
1309-
);
1329+
const groupPolicy =
1330+
(account.groupPolicy as string | undefined) ??
1331+
(parent?.groupPolicy as string | undefined) ??
1332+
undefined;
1333+
1334+
if (groupPolicy === "allowlist" && usesSenderBasedGroupAllowlist(channelName)) {
1335+
const rawGroupAllowFrom =
1336+
(account.groupAllowFrom as Array<string | number> | undefined) ??
1337+
(parent?.groupAllowFrom as Array<string | number> | undefined);
1338+
// Match runtime semantics: resolveGroupAllowFromSources treats
1339+
// empty arrays as unset and falls back to allowFrom.
1340+
const groupAllowFrom = hasAllowFromEntries(rawGroupAllowFrom) ? rawGroupAllowFrom : undefined;
1341+
const fallbackToAllowFrom = allowsGroupAllowFromFallback(channelName);
1342+
const effectiveGroupAllowFrom =
1343+
groupAllowFrom ?? (fallbackToAllowFrom ? effectiveAllowFrom : undefined);
1344+
1345+
if (!hasAllowFromEntries(effectiveGroupAllowFrom)) {
1346+
if (fallbackToAllowFrom) {
1347+
warnings.push(
1348+
`- ${prefix}.groupPolicy is "allowlist" but groupAllowFrom (and allowFrom) is empty — all group messages will be silently dropped. Add sender IDs to ${prefix}.groupAllowFrom or ${prefix}.allowFrom, or set groupPolicy to "open".`,
1349+
);
1350+
} else {
1351+
warnings.push(
1352+
`- ${prefix}.groupPolicy is "allowlist" but groupAllowFrom is empty — this channel does not fall back to allowFrom, so all group messages will be silently dropped. Add sender IDs to ${prefix}.groupAllowFrom, or set groupPolicy to "open".`,
1353+
);
1354+
}
1355+
}
1356+
}
13101357
};
13111358

13121359
for (const [channelName, channelConfig] of Object.entries(
@@ -1315,7 +1362,7 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] {
13151362
if (!channelConfig || typeof channelConfig !== "object") {
13161363
continue;
13171364
}
1318-
checkAccount(channelConfig, `channels.${channelName}`);
1365+
checkAccount(channelConfig, `channels.${channelName}`, undefined, channelName);
13191366

13201367
const accounts = channelConfig.accounts;
13211368
if (accounts && typeof accounts === "object") {
@@ -1325,7 +1372,12 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] {
13251372
if (!account || typeof account !== "object") {
13261373
continue;
13271374
}
1328-
checkAccount(account, `channels.${channelName}.accounts.${accountId}`, channelConfig);
1375+
checkAccount(
1376+
account,
1377+
`channels.${channelName}.accounts.${accountId}`,
1378+
channelConfig,
1379+
channelName,
1380+
);
13291381
}
13301382
}
13311383
}

0 commit comments

Comments
 (0)