From f36d8c09f177af7fa465af8fdc34cbd379ff6b31 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 13 Mar 2026 01:41:31 +0000 Subject: [PATCH] feat(zalouser): audit mutable group allowlists --- src/commands/doctor-config-flow.test.ts | 34 ++++++++ src/commands/doctor-config-flow.ts | 22 +++++ src/security/audit-channel.ts | 60 +++++++++++++- src/security/audit.test.ts | 92 ++++++++++++++++++++- src/security/mutable-allowlist-detectors.ts | 21 +++++ 5 files changed, 227 insertions(+), 2 deletions(-) diff --git a/src/commands/doctor-config-flow.test.ts b/src/commands/doctor-config-flow.test.ts index 2ce46adeb29..265c90197e2 100644 --- a/src/commands/doctor-config-flow.test.ts +++ b/src/commands/doctor-config-flow.test.ts @@ -107,6 +107,40 @@ describe("doctor config flow", () => { ).toBe(false); }); + it("warns on mutable Zalouser group entries when dangerous name matching is disabled", async () => { + const doctorWarnings = await collectDoctorWarnings({ + channels: { + zalouser: { + groups: { + "Ops Room": { allow: true }, + }, + }, + }, + }); + + expect( + doctorWarnings.some( + (line) => + line.includes("mutable allowlist") && line.includes("channels.zalouser.groups: Ops Room"), + ), + ).toBe(true); + }); + + it("does not warn on mutable Zalouser group entries when dangerous name matching is enabled", async () => { + const doctorWarnings = await collectDoctorWarnings({ + channels: { + zalouser: { + dangerouslyAllowNameMatching: true, + groups: { + "Ops Room": { allow: true }, + }, + }, + }, + }); + + expect(doctorWarnings.some((line) => line.includes("channels.zalouser.groups"))).toBe(false); + }); + it("warns when imessage group allowlist is empty even if allowFrom is set", async () => { const doctorWarnings = await collectDoctorWarnings({ channels: { diff --git a/src/commands/doctor-config-flow.ts b/src/commands/doctor-config-flow.ts index ff97c001f07..71cd6926417 100644 --- a/src/commands/doctor-config-flow.ts +++ b/src/commands/doctor-config-flow.ts @@ -44,6 +44,7 @@ import { isMSTeamsMutableAllowEntry, isMattermostMutableAllowEntry, isSlackMutableAllowEntry, + isZalouserMutableGroupEntry, } from "../security/mutable-allowlist-detectors.js"; import { inspectTelegramAccount } from "../telegram/account-inspect.js"; import { listTelegramAccountIds, resolveTelegramAccount } from "../telegram/accounts.js"; @@ -885,6 +886,27 @@ function scanMutableAllowlistEntries(cfg: OpenClawConfig): MutableAllowlistHit[] } } + for (const scope of collectProviderDangerousNameMatchingScopes(cfg, "zalouser")) { + if (scope.dangerousNameMatchingEnabled) { + continue; + } + const groups = asObjectRecord(scope.account.groups); + if (!groups) { + continue; + } + for (const entry of Object.keys(groups)) { + if (!isZalouserMutableGroupEntry(entry)) { + continue; + } + hits.push({ + channel: "zalouser", + path: `${scope.prefix}.groups`, + entry, + dangerousFlagPath: scope.dangerousFlagPath, + }); + } + } + return hits; } diff --git a/src/security/audit-channel.ts b/src/security/audit-channel.ts index 70a21cf729c..a46db8646a4 100644 --- a/src/security/audit-channel.ts +++ b/src/security/audit-channel.ts @@ -18,7 +18,10 @@ import { readChannelAllowFromStore } from "../pairing/pairing-store.js"; import { normalizeStringEntries } from "../shared/string-normalization.js"; import type { SecurityAuditFinding, SecurityAuditSeverity } from "./audit.js"; import { resolveDmAllowState } from "./dm-policy-shared.js"; -import { isDiscordMutableAllowEntry } from "./mutable-allowlist-detectors.js"; +import { + isDiscordMutableAllowEntry, + isZalouserMutableGroupEntry, +} from "./mutable-allowlist-detectors.js"; function normalizeAllowFromList(list: Array | undefined | null): string[] { return normalizeStringEntries(Array.isArray(list) ? list : undefined); @@ -44,6 +47,22 @@ function addDiscordNameBasedEntries(params: { } } +function addZalouserMutableGroupEntries(params: { + target: Set; + groups: unknown; + source: string; +}): void { + if (!params.groups || typeof params.groups !== "object" || Array.isArray(params.groups)) { + return; + } + for (const key of Object.keys(params.groups as Record)) { + if (!isZalouserMutableGroupEntry(key)) { + continue; + } + params.target.add(`${params.source}:${key}`); + } +} + function collectInvalidTelegramAllowFromEntries(params: { entries: unknown; target: Set; @@ -467,6 +486,45 @@ export async function collectChannelSecurityFindings(params: { } } + if (plugin.id === "zalouser") { + const zalouserCfg = + (account as { config?: Record } | null)?.config ?? + ({} as Record); + const dangerousNameMatchingEnabled = isDangerousNameMatchingEnabled(zalouserCfg); + const zalouserPathPrefix = + orderedAccountIds.length > 1 || hasExplicitAccountPath + ? `channels.zalouser.accounts.${accountId}` + : "channels.zalouser"; + const mutableGroupEntries = new Set(); + addZalouserMutableGroupEntries({ + target: mutableGroupEntries, + groups: zalouserCfg.groups, + source: `${zalouserPathPrefix}.groups`, + }); + if (mutableGroupEntries.size > 0) { + const examples = Array.from(mutableGroupEntries).slice(0, 5); + const more = + mutableGroupEntries.size > examples.length + ? ` (+${mutableGroupEntries.size - examples.length} more)` + : ""; + findings.push({ + checkId: "channels.zalouser.groups.mutable_entries", + severity: dangerousNameMatchingEnabled ? "info" : "warn", + title: dangerousNameMatchingEnabled + ? "Zalouser group routing uses break-glass name matching" + : "Zalouser group routing contains mutable group entries", + detail: dangerousNameMatchingEnabled + ? "Zalouser group-name routing is explicitly enabled via dangerouslyAllowNameMatching. This mutable-identity mode is operator-selected break-glass behavior and out-of-scope for vulnerability reports by itself. " + + `Found: ${examples.join(", ")}${more}.` + : "Zalouser group auth is ID-only by default, so unresolved group-name or slug entries are ignored for auth and can drift from the intended trusted group. " + + `Found: ${examples.join(", ")}${more}.`, + remediation: dangerousNameMatchingEnabled + ? "Prefer stable Zalo group IDs (for example group: or provider-native g- ids), then disable dangerouslyAllowNameMatching." + : "Prefer stable Zalo group IDs in channels.zalouser.groups, or explicitly opt in with dangerouslyAllowNameMatching=true if you accept mutable group-name matching.", + }); + } + } + if (plugin.id === "slack") { const slackCfg = (account as { config?: Record; dm?: Record } | null) diff --git a/src/security/audit.test.ts b/src/security/audit.test.ts index 2546feae947..e757c2970d6 100644 --- a/src/security/audit.test.ts +++ b/src/security/audit.test.ts @@ -27,7 +27,7 @@ const execDockerRawUnavailable: NonNullable unknown; inspectAccount?: (cfg: OpenClawConfig, accountId: string | null | undefined) => unknown; @@ -110,6 +110,27 @@ const telegramPlugin = stubChannelPlugin({ }, }); +const zalouserPlugin = stubChannelPlugin({ + id: "zalouser", + label: "Zalo Personal", + listAccountIds: (cfg) => { + const channel = (cfg.channels as Record | undefined)?.zalouser as + | { accounts?: Record } + | undefined; + const ids = Object.keys(channel?.accounts ?? {}); + return ids.length > 0 ? ids : ["default"]; + }, + resolveAccount: (cfg, accountId) => { + const resolvedAccountId = typeof accountId === "string" && accountId ? accountId : "default"; + const channel = (cfg.channels as Record | undefined)?.zalouser as + | { accounts?: Record } + | undefined; + const base = (channel ?? {}) as Record; + const account = channel?.accounts?.[resolvedAccountId] ?? {}; + return { config: { ...base, ...account } }; + }, +}); + function successfulProbeResult(url: string) { return { ok: true, @@ -2324,6 +2345,75 @@ description: test skill }); }); + it("warns when Zalouser group routing contains mutable group entries", async () => { + await withChannelSecurityStateDir(async () => { + const cfg: OpenClawConfig = { + channels: { + zalouser: { + enabled: true, + groups: { + "Ops Room": { allow: true }, + "group:g-123": { allow: true }, + }, + }, + }, + }; + + const res = await runSecurityAudit({ + config: cfg, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [zalouserPlugin], + }); + + const finding = res.findings.find( + (entry) => entry.checkId === "channels.zalouser.groups.mutable_entries", + ); + expect(finding).toBeDefined(); + expect(finding?.severity).toBe("warn"); + expect(finding?.detail).toContain("channels.zalouser.groups:Ops Room"); + expect(finding?.detail).not.toContain("group:g-123"); + }); + }); + + it("marks Zalouser mutable group routing as break-glass when dangerous matching is enabled", async () => { + await withChannelSecurityStateDir(async () => { + const cfg: OpenClawConfig = { + channels: { + zalouser: { + enabled: true, + dangerouslyAllowNameMatching: true, + groups: { + "Ops Room": { allow: true }, + }, + }, + }, + }; + + const res = await runSecurityAudit({ + config: cfg, + includeFilesystem: false, + includeChannelSecurity: true, + plugins: [zalouserPlugin], + }); + + const finding = res.findings.find( + (entry) => entry.checkId === "channels.zalouser.groups.mutable_entries", + ); + expect(finding).toBeDefined(); + expect(finding?.severity).toBe("info"); + expect(finding?.detail).toContain("out-of-scope"); + expect(res.findings).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + checkId: "channels.zalouser.allowFrom.dangerous_name_matching_enabled", + severity: "info", + }), + ]), + ); + }); + }); + it("does not warn when Discord allowlists use ID-style entries only", async () => { await withChannelSecurityStateDir(async () => { const cfg: OpenClawConfig = { diff --git a/src/security/mutable-allowlist-detectors.ts b/src/security/mutable-allowlist-detectors.ts index af3a8f81eaa..d37e1a7cc9e 100644 --- a/src/security/mutable-allowlist-detectors.ts +++ b/src/security/mutable-allowlist-detectors.ts @@ -99,3 +99,24 @@ export function isIrcMutableAllowEntry(raw: string): boolean { return !normalized.includes("!") && !normalized.includes("@"); } + +export function isZalouserMutableGroupEntry(raw: string): boolean { + const text = raw.trim(); + if (!text || text === "*") { + return false; + } + + const normalized = text + .replace(/^(zalouser|zlu):/i, "") + .replace(/^group:/i, "") + .trim(); + + if (!normalized) { + return false; + } + if (/^\d+$/.test(normalized)) { + return false; + } + + return !/^g-\S+$/i.test(normalized); +}