fix(whatsapp): read WHATSAPP_ALLOWED_USERS and WHATSAPP_GROUP_ALLOWED_USERS from env - #37452
AhmetArif0 wants to merge 1 commit into
Conversation
…_USERS from env dm_policy honors WHATSAPP_DM_POLICY and group_policy honors WHATSAPP_GROUP_POLICY from the environment, but _allow_from and _group_allow_from were only populated from config.extra — env-only setups were silently left with empty allowlists. The CLI setup wizard (hermes_cli/main.py) saves WHATSAPP_ALLOWED_USERS to .env, .env.example documents it, and gateway/config.py bridges the YAML allow_from field into WHATSAPP_ALLOWED_USERS. Any of these paths followed by dm_policy=allowlist resulted in every authorized DM being dropped at intake. Mirrors the WeCom fix in f7a3509 which resolved the identical gap for WECOM_ALLOWED_USERS. Config extra still takes precedence over env so an explicit allowlist cannot be widened by a stale env var. Adds four regression tests: env-only populates allowlist (DM + group), extra takes precedence over env (DM + group).
Hermes Agent ReviewFound one precedence edge case in the allowlist fallback logic:
I verified on this PR branch that Suggested fix: distinguish missing config keys from explicit empty values, e.g. choose env only when neither snake_case nor camelCase key is present in Local targeted test suite otherwise passes: Reviewed by Hermes Agent hourly commander. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean fix: makes WhatsApp adapter read WHATSAPP_ALLOWED_USERS and WHATSAPP_GROUP_ALLOWED_USERS from environment variables as fallback when config extra doesn't specify them. Config extra properly takes precedence over env vars to prevent accidental widening.
✅ Looks Good
- +4 tests covering DM allowlist env, group allowlist env, and precedence behavior
- Well-documented inline comments
- Fixes silent failure mode where
dm_policy: allowlistwithoutextra.allow_fromwould drop all DMs - No security concerns — env-only setup was the missing piece
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real env-only allowlist gap. The issue remains on current main, but this branch predates the WhatsApp adapter migration.
Problems
- The diff changes
gateway/platforms/whatsapp.py, while current main constructs the live adapter inplugins/platforms/whatsapp/adapter.py:392-412(migrated by560010547). The current defect is still atplugins/platforms/whatsapp/adapter.py:410and:412, so the patch needs relocation. - The proposed
config.extra.get(...) or os.getenv(...)chain makes explicit empty config values fall through to the environment. That can widen an intentionally empty DM or group allowlist; distinguish an absent key from an empty configured value. - The proposed tests import the removed
gateway.platforms.whatsappmodule; current tests importplugins.platforms.whatsapp.adapterattests/gateway/test_whatsapp_group_gating.py:9.
Suggested changes
- Port the fix and tests to the plugin adapter, preserving explicit empty config values, then add regression coverage for empty DM and group lists with populated environment values.
Automated hermes-sweeper review.
| @@ -261,9 +261,17 @@ def __init__(self, config: PlatformConfig): | |||
| )) | |||
| self._reply_prefix: Optional[str] = config.extra.get("reply_prefix") | |||
| self._dm_policy = str(config.extra.get("dm_policy") or os.getenv("WHATSAPP_DM_POLICY", "open")).strip().lower() | |||
There was a problem hiding this comment.
This truthiness chain lets an explicit empty allow_from ([] or "") fall through to WHATSAPP_ALLOWED_USERS, widening access from a stale environment value. Resolve by key presence so an explicitly configured empty list remains authoritative; apply the same rule to the group allowlist.
|
The env read this PR asked for landed on |
Summary
dm_policyhonorsWHATSAPP_DM_POLICYfrom the environment andgroup_policyhonorsWHATSAPP_GROUP_POLICY, but_allow_fromand_group_allow_fromwere only populated fromconfig.extra— env-only setups silently got empty allowlists.Concrete failure path:
save_env_value("WHATSAPP_ALLOWED_USERS", phone)and writes it to.envWHATSAPP_DM_POLICY=allowlist(or the wizard does so)WhatsAppAdapter.__init__readsdm_policyfrom env correctly but_allow_fromstaysset()becauseconfig.extra.get("allow_from")returnsNoneand there was no env fallbackSame issue existed for
WHATSAPP_GROUP_ALLOWED_USERS/group_allow_from.Fix
Add env fallbacks mirroring
dm_policy/group_policy:Config
extrastill takes precedence over env so an explicit allowlist cannot be silently widened.This is the exact same fix applied to WeCom in f7a3509 (
fix(gateway): honor WECOM_ALLOWED_USERS in env-only WeCom DM allowlist), merged today.Evidence that env vars are expected to work
.env.example:352:# WHATSAPP_ALLOWED_USERS=15551234567hermes_cli/main.py: setup wizard writesWHATSAPP_ALLOWED_USERSto.envgateway/config.py:1091-1098: bridges YAMLallow_from/group_allow_fromintoWHATSAPP_ALLOWED_USERS/WHATSAPP_GROUP_ALLOWED_USERSenv vars_coerce_allow_listdocstring: "Parse allow_from / group_allow_from from config or env var" — the "or env var" was unimplementedTest plan
test_dm_allowlist_honors_env_only_whatsapp_allowed_users— env-only DM allowlist workstest_dm_allowlist_extra_takes_precedence_over_env— config.extra wins for DMstest_group_allowlist_honors_env_only_whatsapp_group_allowed_users— env-only group allowlist workstest_group_allowlist_extra_takes_precedence_over_env— config.extra wins for groups