diff --git a/CHANGELOG.md b/CHANGELOG.md index 46fa3c805..e904185b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,12 @@ Docs: https://docs.openclaw.ai - 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. - 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. +## Unreleased + +### Fixes + +- 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. + ## 2026.2.26 ### Changes diff --git a/src/commands/doctor-config-flow.test.ts b/src/commands/doctor-config-flow.test.ts index d4b032739..ff4763987 100644 --- a/src/commands/doctor-config-flow.test.ts +++ b/src/commands/doctor-config-flow.test.ts @@ -19,6 +19,21 @@ function expectGoogleChatDmAllowFromRepaired(cfg: unknown) { expect(typed.channels.googlechat.allowFrom).toBeUndefined(); } +async function collectDoctorWarnings(config: Record): Promise { + const noteSpy = vi.spyOn(noteModule, "note").mockImplementation(() => {}); + try { + await runDoctorConfigWithInput({ + config, + run: loadAndMaybeMigrateDoctorConfig, + }); + return noteSpy.mock.calls + .filter((call) => call[1] === "Doctor warnings") + .map((call) => String(call[0])); + } finally { + noteSpy.mockRestore(); + } +} + type DiscordGuildRule = { users: string[]; roles: string[]; @@ -56,31 +71,59 @@ describe("doctor config flow", () => { }); it("does not warn on mutable account allowlists when dangerous name matching is inherited", async () => { - const noteSpy = vi.spyOn(noteModule, "note").mockImplementation(() => {}); - try { - await runDoctorConfigWithInput({ - config: { - channels: { - slack: { - dangerouslyAllowNameMatching: true, - accounts: { - work: { - allowFrom: ["alice"], - }, - }, + const doctorWarnings = await collectDoctorWarnings({ + channels: { + slack: { + dangerouslyAllowNameMatching: true, + accounts: { + work: { + allowFrom: ["alice"], }, }, }, - run: loadAndMaybeMigrateDoctorConfig, - }); + }, + }); + expect(doctorWarnings.some((line) => line.includes("mutable allowlist"))).toBe(false); + }); - const doctorWarnings = noteSpy.mock.calls - .filter((call) => call[1] === "Doctor warnings") - .map((call) => String(call[0])); - expect(doctorWarnings.some((line) => line.includes("mutable allowlist"))).toBe(false); - } finally { - noteSpy.mockRestore(); - } + it("does not warn about sender-based group allowlist for googlechat", async () => { + const doctorWarnings = await collectDoctorWarnings({ + channels: { + googlechat: { + groupPolicy: "allowlist", + accounts: { + work: { + groupPolicy: "allowlist", + }, + }, + }, + }, + }); + + expect( + doctorWarnings.some( + (line) => line.includes('groupPolicy is "allowlist"') && line.includes("groupAllowFrom"), + ), + ).toBe(false); + }); + + it("warns when imessage group allowlist is empty even if allowFrom is set", async () => { + const doctorWarnings = await collectDoctorWarnings({ + channels: { + imessage: { + groupPolicy: "allowlist", + allowFrom: ["+15551234567"], + }, + }, + }); + + expect( + doctorWarnings.some( + (line) => + line.includes('channels.imessage.groupPolicy is "allowlist"') && + line.includes("does not fall back to allowFrom"), + ), + ).toBe(true); }); it("drops unknown keys on repair", async () => { diff --git a/src/commands/doctor-config-flow.ts b/src/commands/doctor-config-flow.ts index 5c62a8c25..2b02cf45b 100644 --- a/src/commands/doctor-config-flow.ts +++ b/src/commands/doctor-config-flow.ts @@ -1267,10 +1267,34 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] { const warnings: string[] = []; + const usesSenderBasedGroupAllowlist = (channelName?: string): boolean => { + if (!channelName) { + return true; + } + // These channels enforce group access via channel/space config, not sender-based + // groupAllowFrom lists. + return !(channelName === "discord" || channelName === "slack" || channelName === "googlechat"); + }; + + const allowsGroupAllowFromFallback = (channelName?: string): boolean => { + if (!channelName) { + return true; + } + // Keep doctor warnings aligned with runtime access semantics. + return !( + channelName === "googlechat" || + channelName === "imessage" || + channelName === "matrix" || + channelName === "msteams" || + channelName === "irc" + ); + }; + const checkAccount = ( account: Record, prefix: string, parent?: Record, + channelName?: string, ) => { const dmEntry = account.dm; const dm = @@ -1289,10 +1313,6 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] { (parentDm?.policy as string | undefined) ?? undefined; - if (dmPolicy !== "allowlist") { - return; - } - const topAllowFrom = (account.allowFrom as Array | undefined) ?? (parent?.allowFrom as Array | undefined); @@ -1300,13 +1320,40 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] { const parentNestedAllowFrom = parentDm?.allowFrom as Array | undefined; const effectiveAllowFrom = topAllowFrom ?? nestedAllowFrom ?? parentNestedAllowFrom; - if (hasAllowFromEntries(effectiveAllowFrom)) { - return; + if (dmPolicy === "allowlist" && !hasAllowFromEntries(effectiveAllowFrom)) { + warnings.push( + `- ${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.`, + ); } - warnings.push( - `- ${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.`, - ); + const groupPolicy = + (account.groupPolicy as string | undefined) ?? + (parent?.groupPolicy as string | undefined) ?? + undefined; + + if (groupPolicy === "allowlist" && usesSenderBasedGroupAllowlist(channelName)) { + const rawGroupAllowFrom = + (account.groupAllowFrom as Array | undefined) ?? + (parent?.groupAllowFrom as Array | undefined); + // Match runtime semantics: resolveGroupAllowFromSources treats + // empty arrays as unset and falls back to allowFrom. + const groupAllowFrom = hasAllowFromEntries(rawGroupAllowFrom) ? rawGroupAllowFrom : undefined; + const fallbackToAllowFrom = allowsGroupAllowFromFallback(channelName); + const effectiveGroupAllowFrom = + groupAllowFrom ?? (fallbackToAllowFrom ? effectiveAllowFrom : undefined); + + if (!hasAllowFromEntries(effectiveGroupAllowFrom)) { + if (fallbackToAllowFrom) { + warnings.push( + `- ${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".`, + ); + } else { + warnings.push( + `- ${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".`, + ); + } + } + } }; for (const [channelName, channelConfig] of Object.entries( @@ -1315,7 +1362,7 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] { if (!channelConfig || typeof channelConfig !== "object") { continue; } - checkAccount(channelConfig, `channels.${channelName}`); + checkAccount(channelConfig, `channels.${channelName}`, undefined, channelName); const accounts = channelConfig.accounts; if (accounts && typeof accounts === "object") { @@ -1325,7 +1372,12 @@ function detectEmptyAllowlistPolicy(cfg: OpenClawConfig): string[] { if (!account || typeof account !== "object") { continue; } - checkAccount(account, `channels.${channelName}.accounts.${accountId}`, channelConfig); + checkAccount( + account, + `channels.${channelName}.accounts.${accountId}`, + channelConfig, + channelName, + ); } } }