fix(whatsapp): honor group allowlist in bridge - #43929
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Fix: Honor group allowlist in WhatsApp bridge
- Problem: Group IDs in the allowlist had to be specified with exact
@g.ussuffix, but the adapter sometimes received them without it, causing mismatches. - Fix:
_is_group_allowednow normalizes IDs by checking with/without@g.ussuffix, plus support for bare group IDs and*wildcard. - Environment variable support: Reads
WHATSAPP_GROUP_ALLOWED_USERSenv var in addition to config file. Passes bothWHATSAPP_GROUP_POLICYandWHATSAPP_GROUP_ALLOWED_USERSto the Node bridge subprocess. - Node bridge: Added
matchesAllowedIdentifier()in allowlist.js that handles the same normalization logic in JavaScript. - Tests cover bare group IDs, wildcard, and env var passthrough to the bridge.
Reviewed by Hermes Agent
|
Hey maintainers, can I be assigned on this? @tonydwb |
dd5cfe2 to
0880473
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the group-scoped authorization gap. The underlying problem is still present on current main: scripts/whatsapp-bridge/bridge.js:637 applies the DM sender allowlist to external group traffic, and gateway/authz_mixin.py:467 only trusts an adapter group policy when no environment allowlist is configured.
Problems
- The adapter changes target deleted
gateway/platforms/whatsapp.py; WhatsApp moved toplugins/platforms/whatsapp/adapter.pyin560010547. - The proposed bridge fallback at
scripts/whatsapp-bridge/bridge.js:350would drop pairing DMs. Current main intentionally bypasses sender-list enforcement forWHATSAPP_DM_POLICY === 'pairing'atscripts/whatsapp-bridge/bridge.js:637. - The proposed auth helper reads
self.adaptersdirectly atgateway/authz_mixin.py:192in this PR. Current main'sgateway/authz_mixin.py:34resolves the adapter bySessionSource.profile; a salvage must preserve that multiplex isolation.
Suggested changes
- Port the adapter/test work to the bundled plugin and preserve the current pairing branch.
- Use
_authorization_adapter(source.platform, source.profile)for the group authorization decision and add a profile-scoped regression test.
Automated hermes-sweeper review.
| } catch {} | ||
| continue; | ||
| } | ||
| } else if (!matchesAllowedUser(senderId, ALLOWED_USERS, SESSION_DIR)) { |
There was a problem hiding this comment.
A current-main port must retain the WHATSAPP_DM_POLICY !== 'pairing' guard before applying the sender allowlist here. scripts/whatsapp-bridge/bridge.js:637 deliberately forwards pairing DMs for the gateway pairing handshake; this unconditional fallback would drop them.
| if not self._adapter_enforces_own_access_policy(source.platform): | ||
| return False | ||
|
|
||
| adapters = getattr(self, "adapters", None) or {} |
There was a problem hiding this comment.
Port this through current main's _authorization_adapter(source.platform, source.profile) rather than self.adapters. The current resolver is profile-aware, so direct lookup can read the default profile's WhatsApp policy for a secondary-profile message.
|
I hit the same bridge-level failure on current I opened #73465 as a narrow current-main salvage of this PR's bridge portion because this branch is now conflict-dirty and also carries gateway changes that have since moved/landed through adjacent work. The replacement is one commit touching only the three bridge policy/test files, and credits Martin Gontovnikas as co-author. |
Summary
Test Plan