fix: reject dmPolicy="allowlist" with empty allowFrom across all channels#27936
Conversation
openclaw-barnacle
bot
added
commands
Greptile SummaryPrevents silent DM drops when Key changes:
How it works: Confidence Score: 5/5
Last reviewed commit: 8989baf |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8989baf3cc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| requireAllowlistAllowFrom({ | ||
| policy: value.dmPolicy, | ||
| allowFrom: value.allowFrom, | ||
| ctx, |
There was a problem hiding this comment.
Account configs inherit channel-level fields at runtime (for example, resolveTelegramAccount shallow-merges top-level and account values in src/telegram/accounts.ts), so an account can set dmPolicy: "allowlist" and rely on top-level allowFrom. This new refinement validates value.allowFrom on the account object alone, which now rejects previously working multi-account configs unless every account duplicates the allowlist; the same regression pattern is repeated for other account schemas in this change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — removed account-level allowFrom validation from all 6 channel schemas (Telegram, Signal, IRC, iMessage, BlueBubbles, WhatsApp). Accounts now correctly inherit from parent. Top-level config schemas still enforce validation.
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936.
widingmarcus-cyber
force-pushed
the
fix/allowlist-requires-allowfrom
branch
from
8989baf to
d0cb8c0
Compare
…nels When dmPolicy is set to "allowlist" but allowFrom is missing or empty, all DMs are silently dropped because no sender can match the empty allowlist. This is a common pitfall after upgrades that change how allowlist files are handled (e.g., external allowlist-dm.json files being deprecated in favor of inline allowFrom arrays). Changes: - Add requireAllowlistAllowFrom schema refinement (zod-schema.core.ts) - Apply validation to all channel schemas: Telegram, Discord, Slack, Signal, IRC, iMessage, BlueBubbles, MS Teams, Google Chat, WhatsApp - Add detectEmptyAllowlistPolicy to doctor-config-flow.ts so "openclaw doctor" surfaces a clear warning with remediation steps - Add 12 test cases covering reject/accept for multiple channels Fixes openclaw#27892
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0cb8c0657
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| enforceAllowlistDmPolicyAllowFrom({ | ||
| dmPolicy: value.dmPolicy, | ||
| allowFrom: value.allowFrom, | ||
| ctx, |
There was a problem hiding this comment.
resolveWhatsAppAccount falls back to rootCfg.allowFrom when an account omits its own list (src/web/accounts.ts), so configs like channels.whatsapp.allowFrom=[...] with channels.whatsapp.accounts.work.dmPolicy="allowlist" are valid at runtime and worked before this commit. The new enforceAllowlistDmPolicyAllowFrom check validates the account object in isolation, so it now rejects those inherited multi-account setups unless every account duplicates allowFrom, causing unnecessary config-load failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in latest push — WhatsApp account schema no longer validates allowFrom independently. Parent channel allowFrom is inherited at runtime via resolveWhatsAppAccount.
src/commands/doctor-config-flow.ts
Outdated
| if (!account || typeof account !== "object") { | ||
| continue; | ||
| } | ||
| checkAccount(account, `channels.${channelName}.accounts.${accountId}`); |
There was a problem hiding this comment.
detectEmptyAllowlistPolicy evaluates each account using only the account object’s allowFrom fields, so accounts that intentionally inherit channel-level allowlists are flagged as “all DMs will be silently dropped.” This is a false positive for merged-account channels (for example Telegram merges {...base, ...account} in src/telegram/accounts.ts), and it can mislead openclaw doctor users into changing already-correct configs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed — detectEmptyAllowlistPolicy now resolves parent-level allowFrom (both top-level and dm.allowFrom) before scanning accounts. No more false positives for inherited allowlists.
- Remove account-level allowFrom validation from WhatsAppAccountSchema (accounts inherit from parent via resolveWhatsAppAccount) - Update detectEmptyAllowlistPolicy in doctor to pass parent allowFrom context when scanning accounts, preventing false positives Addresses Codex P1+P2 review feedback on openclaw#27936.
steipete
force-pushed
the
fix/allowlist-requires-allowfrom
branch
from
a8fa463 to
1a46ce1
Compare
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on #27936.
|
Landed via temp rebase onto main.
Thanks @widingmarcus-cyber! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a46ce195d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const effectiveAllowFrom = | ||
| account.allowFrom ?? account.dm?.allowFrom ?? value.allowFrom ?? value.dm?.allowFrom; |
There was a problem hiding this comment.
This fallback treats value.dm.allowFrom as inherited for account checks, but Discord/Slack account resolution is a shallow merge (mergeDiscordAccountConfig/mergeSlackAccountConfig), so an account-level dm object replaces the parent dm object entirely. In configs like parent dm.allowFrom + account dm.policy: "allowlist" (without account allowFrom), validation passes here, but runtime resolves an empty allowlist in the provider (discordCfg/slackCfg.allowFrom ?? dmConfig?.allowFrom) and silently blocks all DMs; this regression comes from the new effective-allowlist logic.
Useful? React with 👍 / 👎.
| const parentNestedAllowFrom = parentDm?.allowFrom as Array<string | number> | undefined; | ||
| const effectiveAllowFrom = topAllowFrom ?? nestedAllowFrom ?? parentNestedAllowFrom; |
There was a problem hiding this comment.
The doctor scan always falls back to parentDm.allowFrom, but for Slack/Discord an account-level dm object shadows the parent dm object at runtime (shallow merge), so parent nested allowlists are not actually inherited in that case. This produces false negatives (no warning) for configs where account dm.policy is allowlist but account allowFrom is effectively empty, which leaves the same silent DM-drop behavior the doctor warning is supposed to catch.
Useful? React with 👍 / 👎.
* main: docs(cli): improve secrets command guide fix: repair Telegram allowlist DM migrations (openclaw#27936) (thanks @widingmarcus-cyber) Changelog: include Gemini OAuth PRs openclaw#16683 and openclaw#16684 (openclaw#27987) fix(ci): align sync boundary realpath canonicalization fix: unblock CI minimatch audit and host policy check fix(ios): eliminate Swift warnings and clean build logs chore(release): point appcast to beta tag
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936.
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936. (cherry picked from commit 8df5999)
…aw#27936) (thanks @widingmarcus-cyber) (cherry picked from commit d4f60bf)
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936.
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936. (cherry picked from commit 8df5999)
…aw#27936) (thanks @widingmarcus-cyber) (cherry picked from commit d4f60bf)
…widingmarcus-cyber) (cherry picked from commit 4ef47d1)
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936. (cherry picked from commit 8df5999)
…aw#27936) (thanks @widingmarcus-cyber) (cherry picked from commit d4f60bf)
…widingmarcus-cyber) (cherry picked from commit 4ef47d1)
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936. (cherry picked from commit 8df5999)
…aw#27936) (thanks @widingmarcus-cyber) (cherry picked from commit d4f60bf)
…widingmarcus-cyber) (cherry picked from commit 4ef47d1)
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936. (cherry picked from commit 8df5999)
…aw#27936) (thanks @widingmarcus-cyber) (cherry picked from commit d4f60bf)
…widingmarcus-cyber) (cherry picked from commit 4ef47d1)
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936.
Account configs inherit channel-level fields at runtime (e.g., resolveTelegramAccount shallow-merges top-level and account values). An account can set dmPolicy='allowlist' and rely on the parent's allowFrom, so validating allowFrom on the account object alone incorrectly rejects valid multi-account configs. Removes requireAllowlistAllowFrom and requireOpenAllowFrom from all account-level schemas (Telegram, Signal, IRC, iMessage, BlueBubbles). Top-level config schemas still enforce the validation. Addresses Codex review feedback on openclaw#27936.
Groups configured with requireMention: 'monitor' are passive (hooks fire, bot never responds), so sender-level filtering is unnecessary. Uses a simple early return in WhatsApp access-control when a group is in monitor mode — no changes to the shared dm-policy-shared layer. This keeps the fix local and avoids adding parameters to the centralized access control functions refactored in openclaw#24670 and openclaw#27936. Each channel can add the same early return pattern to support monitor groups, using the existing resolveChannelGroupRequireMention helper.
Groups configured with requireMention: 'monitor' are passive (hooks fire, bot never responds), so sender-level filtering is unnecessary. Uses a simple early return in WhatsApp access-control when a group is in monitor mode — no changes to the shared dm-policy-shared layer. This keeps the fix local and avoids adding parameters to the centralized access control functions refactored in openclaw#24670 and openclaw#27936. Each channel can add the same early return pattern to support monitor groups, using the existing resolveChannelGroupRequireMention helper.
2 participants
Problem
When
dmPolicyis set to"allowlist"butallowFromis missing or empty, all DMs are silently dropped. No error appears in journalctl. The only indication is a validation warning buried in/tmp/openclaw/openclaw-*.log.This is a common pitfall after upgrading from versions that supported external allowlist files (e.g.,
~/.openclaw/telegram/allowlist-dm.json) to newer versions whereallowFrommust be inline inopenclaw.json.Who is affected: Any user with
dmPolicy: "allowlist"who upgrades and does not manually addallowFromentries. Reported in #27892 as taking "several hours to debug" due to the silent failure.Root Cause
The Zod schema validates
dmPolicy="open"(requiresallowFrom: ["*"]) but has no validation fordmPolicy="allowlist"with an empty/missingallowFrom. The config passes validation, the gateway starts, and messages are silently dropped at runtime inisSenderIdAllowed()because the allowlist is empty.Fix
Schema validation (
src/config/zod-schema.core.ts):requireAllowlistAllowFrom()refinement helper (mirrors existingrequireOpenAllowFrom)dmPolicy="allowlist"andallowFromis empty/missing, config validation now fails with a clear error messageApplied to all channels (
src/config/zod-schema.providers-core.ts,zod-schema.providers-whatsapp.ts):Doctor detection (
src/commands/doctor-config-flow.ts):detectEmptyAllowlistPolicy()that scans all channel configs and warns:channels.telegram.dmPolicy is "allowlist" but allowFrom is empty - all DMs will be silently droppedopenclaw doctorandopenclaw doctor --fixflowsTesting
12 new test cases in
config.allowlist-requires-allowfrom.test.ts:dmPolicy="allowlist"withoutallowFrom(Telegram, Signal, Discord, WhatsApp)dmPolicy="allowlist"with emptyallowFrom: []dmPolicy="allowlist"with validallowFromentriesdmPolicy="pairing"withoutallowFrom(no false positives)All 613 existing config tests + 117 doctor tests continue to pass.
@greptile-apps
Fixes #27892