Skip to content

chore(channels/whatsapp-sidecar): 3 nits from post-merge audit#5454

Merged
houko merged 1 commit into
mainfrom
chore/whatsapp-sidecar-audit-nits
May 21, 2026
Merged

chore(channels/whatsapp-sidecar): 3 nits from post-merge audit#5454
houko merged 1 commit into
mainfrom
chore/whatsapp-sidecar-audit-nits

Conversation

@houko

@houko houko commented May 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Surfaces from the post-#5445 audit (6th in the dingtalk/qq/omnibus review chain — first verdict that wasn't BROKEN/NEEDS_HOTFIX). All three are doc/UX cleanups, none are runtime regressions:

  1. Docstring claim Bump docker/login-action from 3 to 4 #3 was wrong — said "429 Retry-After honoured on every outbound POST", but only Cloud API uses `_cloud_post_with_retry`; the gateway path calls `_http_request` directly. Softened to "Cloud-API outbound POSTs only" + explained when gateway-side retry would matter.

  2. WHATSAPP_VERIFY_TOKEN unset failed silently — Meta's subscription handshake returned 403 with no log line, operators saw "subscription failed" in the Meta dashboard with nothing in their daemon logs. Now warns at `init`.

  3. WHATSAPP_GROUP_POLICY is dead config — Cloud API webhook doesn't surface groups (we hardcode `is_group=False`); gateway mode delegates inbound entirely to the Node gateway. So all values are ignored. Schema field kept for forward-compat but labeled `(currently inert)`.

Drive-by

Dropped the misleading `send-voice` docstring claim — promised a `{gateway}/message/send-voice` multipart upload route that doesn't exist in code; rationale comment explains why we don't need it (daemon's `ChannelContent::Voice` only carries a URL at dispatch boundary).

Tests

  • New `test_get_verify_empty_self_token_rejects` asserts BOTH the `init` warn fires AND the handshake fails closed. Regression guard — without this an attacker who sends empty `hub.verify_token` could subscribe their own callback URL.
  • `cd sdk/python && pytest tests/test_whatsapp_adapter.py` — 79 passed (was 78; +1 regression guard).

Audit chain

This closes the 6-review chain. WhatsApp itself is structurally clean — natural routing key (phone) is preserved as `channel_id` both directions, so the bug family from #5417#5449 doesn't apply by construction.

# PR Found
1 #5392 self-review FeishuConfig dangling ref (race)
2 #5417 self-review dingtalk on_send routing
3 #5431 post-hotfix qq same bug class
4 #5439 post-hotfix 6 sidecars same class
5 #5448+#5449+#5450 post-omnibus wechat / wecom restart fragility + SDK trace
6 This whatsapp CLEAN (3 nits only)

Surfaces from the post-#5445 audit (6th in the dingtalk/qq/omnibus
review chain — first verdict that wasn't BROKEN/NEEDS_HOTFIX). All
three nits are doc/UX cleanups, none are runtime regressions:

1. **Docstring claim #3 was wrong** — said "429 Retry-After honoured
   on every outbound POST", but only Cloud API uses
   `_cloud_post_with_retry`; the gateway path calls `_http_request`
   directly and raises on any non-2xx. Soften the claim to
   "Cloud-API outbound POSTs only" + explain when gateway-side
   retry would matter (operators proxying the local Baileys
   gateway behind a rate-limiting reverse-proxy).

2. **WHATSAPP_VERIFY_TOKEN unset failed silently** — Meta's
   subscription handshake returned 403 with no log line pointing
   back to the missing env var; operators saw "subscription failed"
   in the Meta dashboard with nothing in their daemon logs. Now
   warns at __init__, matching the WHATSAPP_APP_SECRET pattern
   already there.

3. **WHATSAPP_GROUP_POLICY is dead config** — Cloud API webhook
   payloads don't surface a group/conversation distinction (we
   hardcode `is_group=False` at `_handle_post_webhook`), and gateway
   mode delegates inbound entirely to the Node Baileys gateway which
   never calls back into the sidecar's `should_handle_message`
   filter. So `WHATSAPP_GROUP_POLICY` has zero effect today. Mark
   the schema field with an explicit "(currently inert)" note in the
   label so operators don't waste time setting it. Kept the field
   itself for forward-compat — Meta has been rolling out group-chat
   support to the Cloud API gradually and we want the schema to be
   ready when it lands.

Drive-by: dropped the misleading send-voice docstring claim (lines
22-23 + 36-39 promised a `{gateway}/message/send-voice` multipart
upload route that doesn't exist in code; rationale comment about
the daemon's ChannelContent::Voice always carrying a URL at the
dispatch boundary explains why we don't need it).

Test:
- New test_get_verify_empty_self_token_rejects asserts BOTH the
  __init__ warn fires AND the handshake fails closed. Regression
  guard for #2 — without this an attacker who guesses the empty
  hub.verify_token could subscribe their own callback URL.
- cd sdk/python && pytest tests/test_whatsapp_adapter.py — 79 passed
  (was 78; +1 regression guard).

This closes the audit chain. WhatsApp itself is structurally clean
— natural routing key (phone) is preserved as channel_id both
directions, the bug family from #5417#5449 doesn't apply.
@houko
houko merged commit bb83423 into main May 21, 2026
15 checks passed
@houko
houko deleted the chore/whatsapp-sidecar-audit-nits branch May 21, 2026 02:22
@github-actions github-actions Bot added area/sdk JavaScript and Python SDKs size/M 50-249 lines changed labels May 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sdk JavaScript and Python SDKs size/M 50-249 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant