fix(security): warn on Discord name-based allowlists in audit
This commit is contained in:
@@ -34,6 +34,7 @@ Docs: https://docs.openclaw.ai
|
||||
|
||||
- Chat/Usage/TUI: strip synthetic inbound metadata blocks (including `Conversation info` and trailing `Untrusted context` channel metadata wrappers) from displayed conversation history so internal prompt context no longer leaks into user-visible logs.
|
||||
- Security/Exec: in non-default setups that manually add `sort` to `tools.exec.safeBins`, block `sort --compress-program` so allowlist-mode safe-bin checks cannot bypass approval. Thanks @tdjackey for reporting.
|
||||
- Security/Discord: add `openclaw security audit` warnings for name/tag-based Discord allowlist entries (DM allowlists, guild/channel `users`, and pairing-store entries), highlighting slug-collision risk while keeping name-based matching supported. Thanks @tdjackey for reporting.
|
||||
- Doctor/State integrity: only require/create the OAuth credentials directory when WhatsApp or pairing-backed channels are configured, and downgrade fresh-install missing-dir noise to an informational warning.
|
||||
- Agents/Sanitization: stop rewriting billing-shaped assistant text outside explicit error context so normal replies about billing/credits/payment are preserved across messaging channels. (#17834, fixes #11359)
|
||||
- Security/Agents: cap embedded Pi runner outer retry loop with a higher profile-aware dynamic limit (32-160 attempts) and return an explicit `retry_limit` error payload when retries never converge, preventing unbounded internal retry cycles (`GHSA-76m6-pj3w-v7mf`).
|
||||
|
||||
@@ -398,6 +398,7 @@ Example:
|
||||
|
||||
- guild must match `channels.discord.guilds` (`id` preferred, slug accepted)
|
||||
- optional sender allowlists: `users` (IDs or names) and `roles` (role IDs only); if either is configured, senders are allowed when they match `users` OR `roles`
|
||||
- names/tags are supported for `users`, but IDs are safer; `openclaw security audit` warns when name/tag entries are used
|
||||
- if a guild has `channels` configured, non-listed channels are denied
|
||||
- if a guild has no `channels` block, all channels in that allowlisted guild are allowed
|
||||
|
||||
|
||||
@@ -31,6 +31,7 @@ It also warns when sandbox Docker settings are configured while sandbox mode is
|
||||
It also warns when sandbox browser uses Docker `bridge` network without `sandbox.browser.cdpSourceRange`.
|
||||
It also warns when existing sandbox browser Docker containers have missing/stale hash labels (for example pre-migration containers missing `openclaw.browserConfigEpoch`) and recommends `openclaw sandbox recreate --browser --all`.
|
||||
It also warns when npm-based plugin/hook install records are unpinned, missing integrity metadata, or drift from currently installed package versions.
|
||||
It warns when Discord allowlists (`channels.discord.allowFrom`, `channels.discord.guilds.*.users`, pairing store) use name or tag entries instead of stable IDs.
|
||||
It warns when `gateway.auth.mode="none"` leaves Gateway HTTP APIs reachable without a shared secret (`/tools/invoke` plus any enabled `/v1/*` endpoint).
|
||||
|
||||
## JSON output
|
||||
|
||||
@@ -17,6 +17,47 @@ function normalizeAllowFromList(list: Array<string | number> | undefined | null)
|
||||
return normalizeStringEntries(Array.isArray(list) ? list : undefined);
|
||||
}
|
||||
|
||||
const DISCORD_ALLOWLIST_ID_PREFIXES = ["discord:", "user:", "pk:"] as const;
|
||||
|
||||
function isDiscordNameBasedAllowEntry(raw: string | number): boolean {
|
||||
const text = String(raw).trim();
|
||||
if (!text || text === "*") {
|
||||
return false;
|
||||
}
|
||||
const maybeId = text.replace(/^<@!?/, "").replace(/>$/, "");
|
||||
if (/^\d+$/.test(maybeId)) {
|
||||
return false;
|
||||
}
|
||||
const prefixed = DISCORD_ALLOWLIST_ID_PREFIXES.find((prefix) => text.startsWith(prefix));
|
||||
if (prefixed) {
|
||||
const candidate = text.slice(prefixed.length);
|
||||
if (candidate) {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
function addDiscordNameBasedEntries(params: {
|
||||
target: Set<string>;
|
||||
values: unknown;
|
||||
source: string;
|
||||
}): void {
|
||||
if (!Array.isArray(params.values)) {
|
||||
return;
|
||||
}
|
||||
for (const value of params.values) {
|
||||
if (!isDiscordNameBasedAllowEntry(value as string | number)) {
|
||||
continue;
|
||||
}
|
||||
const text = String(value).trim();
|
||||
if (!text) {
|
||||
continue;
|
||||
}
|
||||
params.target.add(`${params.source}:${text}`);
|
||||
}
|
||||
}
|
||||
|
||||
function classifyChannelWarningSeverity(message: string): SecurityAuditSeverity {
|
||||
const s = message.toLowerCase();
|
||||
if (
|
||||
@@ -141,6 +182,69 @@ export async function collectChannelSecurityFindings(params: {
|
||||
const discordCfg =
|
||||
(account as { config?: Record<string, unknown> } | null)?.config ??
|
||||
({} as Record<string, unknown>);
|
||||
const storeAllowFrom = await readChannelAllowFromStore("discord").catch(() => []);
|
||||
const discordNameBasedAllowEntries = new Set<string>();
|
||||
addDiscordNameBasedEntries({
|
||||
target: discordNameBasedAllowEntries,
|
||||
values: discordCfg.allowFrom,
|
||||
source: "channels.discord.allowFrom",
|
||||
});
|
||||
addDiscordNameBasedEntries({
|
||||
target: discordNameBasedAllowEntries,
|
||||
values: (discordCfg.dm as { allowFrom?: unknown } | undefined)?.allowFrom,
|
||||
source: "channels.discord.dm.allowFrom",
|
||||
});
|
||||
addDiscordNameBasedEntries({
|
||||
target: discordNameBasedAllowEntries,
|
||||
values: storeAllowFrom,
|
||||
source: "~/.openclaw/credentials/discord-allowFrom.json",
|
||||
});
|
||||
const discordGuildEntries = (discordCfg.guilds as Record<string, unknown> | undefined) ?? {};
|
||||
for (const [guildKey, guildValue] of Object.entries(discordGuildEntries)) {
|
||||
if (!guildValue || typeof guildValue !== "object") {
|
||||
continue;
|
||||
}
|
||||
const guild = guildValue as Record<string, unknown>;
|
||||
addDiscordNameBasedEntries({
|
||||
target: discordNameBasedAllowEntries,
|
||||
values: guild.users,
|
||||
source: `channels.discord.guilds.${guildKey}.users`,
|
||||
});
|
||||
const channels = guild.channels;
|
||||
if (!channels || typeof channels !== "object") {
|
||||
continue;
|
||||
}
|
||||
for (const [channelKey, channelValue] of Object.entries(
|
||||
channels as Record<string, unknown>,
|
||||
)) {
|
||||
if (!channelValue || typeof channelValue !== "object") {
|
||||
continue;
|
||||
}
|
||||
const channel = channelValue as Record<string, unknown>;
|
||||
addDiscordNameBasedEntries({
|
||||
target: discordNameBasedAllowEntries,
|
||||
values: channel.users,
|
||||
source: `channels.discord.guilds.${guildKey}.channels.${channelKey}.users`,
|
||||
});
|
||||
}
|
||||
}
|
||||
if (discordNameBasedAllowEntries.size > 0) {
|
||||
const examples = Array.from(discordNameBasedAllowEntries).slice(0, 5);
|
||||
const more =
|
||||
discordNameBasedAllowEntries.size > examples.length
|
||||
? ` (+${discordNameBasedAllowEntries.size - examples.length} more)`
|
||||
: "";
|
||||
findings.push({
|
||||
checkId: "channels.discord.allowFrom.name_based_entries",
|
||||
severity: "warn",
|
||||
title: "Discord allowlist contains name or tag entries",
|
||||
detail:
|
||||
"Discord name/tag allowlist matching uses normalized slugs and can collide across users. " +
|
||||
`Found: ${examples.join(", ")}${more}.`,
|
||||
remediation:
|
||||
"Prefer stable Discord IDs (or <@id>/user:<id>/pk:<id>) in channels.discord.allowFrom and channels.discord.guilds.*.users.",
|
||||
});
|
||||
}
|
||||
const nativeEnabled = resolveNativeCommandsEnabled({
|
||||
providerId: "discord",
|
||||
providerSetting: coerceNativeSetting(
|
||||
@@ -160,7 +264,7 @@ export async function collectChannelSecurityFindings(params: {
|
||||
const defaultGroupPolicy = params.cfg.channels?.defaults?.groupPolicy;
|
||||
const groupPolicy =
|
||||
(discordCfg.groupPolicy as string | undefined) ?? defaultGroupPolicy ?? "allowlist";
|
||||
const guildEntries = (discordCfg.guilds as Record<string, unknown> | undefined) ?? {};
|
||||
const guildEntries = discordGuildEntries;
|
||||
const guildsConfigured = Object.keys(guildEntries).length > 0;
|
||||
const hasAnyUserAllowlist = Object.values(guildEntries).some((guild) => {
|
||||
if (!guild || typeof guild !== "object") {
|
||||
@@ -184,7 +288,6 @@ export async function collectChannelSecurityFindings(params: {
|
||||
});
|
||||
const dmAllowFromRaw = (discordCfg.dm as { allowFrom?: unknown } | undefined)?.allowFrom;
|
||||
const dmAllowFrom = Array.isArray(dmAllowFromRaw) ? dmAllowFromRaw : [];
|
||||
const storeAllowFrom = await readChannelAllowFromStore("discord").catch(() => []);
|
||||
const ownerAllowFromConfigured =
|
||||
normalizeAllowFromList([...dmAllowFrom, ...storeAllowFrom]).length > 0;
|
||||
|
||||
|
||||
@@ -1166,6 +1166,99 @@ describe("security audit", () => {
|
||||
});
|
||||
});
|
||||
|
||||
it("warns when Discord allowlists contain name-based entries", async () => {
|
||||
await withStateDir("discord-name-based-allowlist", async (tmp) => {
|
||||
await fs.writeFile(
|
||||
path.join(tmp, "credentials", "discord-allowFrom.json"),
|
||||
JSON.stringify({ version: 1, allowFrom: ["team.owner"] }),
|
||||
);
|
||||
const cfg: OpenClawConfig = {
|
||||
channels: {
|
||||
discord: {
|
||||
enabled: true,
|
||||
token: "t",
|
||||
allowFrom: ["Alice#1234", "<@123456789012345678>"],
|
||||
guilds: {
|
||||
"123": {
|
||||
users: ["trusted.operator"],
|
||||
channels: {
|
||||
general: {
|
||||
users: ["987654321098765432", "security-team"],
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const res = await runSecurityAudit({
|
||||
config: cfg,
|
||||
includeFilesystem: false,
|
||||
includeChannelSecurity: true,
|
||||
plugins: [discordPlugin],
|
||||
});
|
||||
|
||||
const finding = res.findings.find(
|
||||
(entry) => entry.checkId === "channels.discord.allowFrom.name_based_entries",
|
||||
);
|
||||
expect(finding).toBeDefined();
|
||||
expect(finding?.severity).toBe("warn");
|
||||
expect(finding?.detail).toContain("channels.discord.allowFrom:Alice#1234");
|
||||
expect(finding?.detail).toContain("channels.discord.guilds.123.users:trusted.operator");
|
||||
expect(finding?.detail).toContain(
|
||||
"channels.discord.guilds.123.channels.general.users:security-team",
|
||||
);
|
||||
expect(finding?.detail).toContain(
|
||||
"~/.openclaw/credentials/discord-allowFrom.json:team.owner",
|
||||
);
|
||||
expect(finding?.detail).not.toContain("<@123456789012345678>");
|
||||
});
|
||||
});
|
||||
|
||||
it("does not warn when Discord allowlists use ID-style entries only", async () => {
|
||||
await withStateDir("discord-id-only-allowlist", async () => {
|
||||
const cfg: OpenClawConfig = {
|
||||
channels: {
|
||||
discord: {
|
||||
enabled: true,
|
||||
token: "t",
|
||||
allowFrom: [
|
||||
"123456789012345678",
|
||||
"<@223456789012345678>",
|
||||
"user:323456789012345678",
|
||||
"discord:423456789012345678",
|
||||
"pk:member-123",
|
||||
],
|
||||
guilds: {
|
||||
"123": {
|
||||
users: ["523456789012345678", "<@623456789012345678>", "pk:member-456"],
|
||||
channels: {
|
||||
general: {
|
||||
users: ["723456789012345678", "user:823456789012345678"],
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const res = await runSecurityAudit({
|
||||
config: cfg,
|
||||
includeFilesystem: false,
|
||||
includeChannelSecurity: true,
|
||||
plugins: [discordPlugin],
|
||||
});
|
||||
|
||||
expect(res.findings).not.toEqual(
|
||||
expect.arrayContaining([
|
||||
expect.objectContaining({ checkId: "channels.discord.allowFrom.name_based_entries" }),
|
||||
]),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
it("flags Discord slash commands when access-group enforcement is disabled and no users allowlist exists", async () => {
|
||||
await withStateDir("discord-open", async () => {
|
||||
const cfg: OpenClawConfig = {
|
||||
|
||||
Reference in New Issue
Block a user