feat(whatsapp): add HMAC-SHA256 webhook signature verification (#75)#403
feat(whatsapp): add HMAC-SHA256 webhook signature verification (#75)#403jrevillard wants to merge 1 commit intonearai:stagingfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances webhook security and flexibility by implementing comprehensive verification mechanisms for WhatsApp Cloud API, covering both initial GET request verification and subsequent POST message validation. It introduces a new HMAC-SHA256 signature verification method, alongside a configurable verification mode for GET requests, allowing for more tailored and secure handling of various webhook protocols. The changes ensure robust security practices, including constant-time comparisons, and integrate these new features seamlessly into the existing channel routing and secret management infrastructure. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces end-to-end webhook verification for the WhatsApp channel, including GET request verification via query parameters and POST request verification using HMAC-SHA256 signatures. The implementation is robust, featuring constant-time comparison to prevent timing attacks and a comprehensive set of new tests. The changes are well-structured, adding new fields to schemas and capabilities, and updating the router logic to handle the new verification flows.
My main feedback is a suggestion to refactor some duplicated code in src/extensions/manager.rs to improve maintainability. Overall, this is a solid feature addition that enhances the security and functionality of webhook integrations.
| // Also refresh HMAC secret in the router (WhatsApp/Slack style) | ||
| let hmac_secret_name = { | ||
| let cap_path = self | ||
| .wasm_channels_dir | ||
| .join(format!("{}.capabilities.json", name)); | ||
| match tokio::fs::read(&cap_path).await { | ||
| Ok(bytes) => crate::channels::wasm::ChannelCapabilitiesFile::from_bytes(&bytes) | ||
| .ok() | ||
| .and_then(|f| f.webhook_hmac_secret_name().map(|s| s.to_string())), | ||
| Err(_) => None, | ||
| } | ||
| }; | ||
| if let Some(ref hmac_name) = hmac_secret_name | ||
| && let Ok(hmac_secret) = self | ||
| .secrets | ||
| .get_decrypted(&self.user_id, hmac_name) | ||
| .await | ||
| { | ||
| router | ||
| .register_hmac_secret(name, hmac_secret.expose().to_string()) | ||
| .await; | ||
| } |
There was a problem hiding this comment.
This logic for refreshing the HMAC secret is quite similar to the logic for registering it during initial activation in activate_channel_from_loaded (lines 1889-1899). There's also a similar block in src/main.rs for startup.
To improve maintainability and reduce code duplication, consider refactoring this into a private helper method within ExtensionManager. This helper could encapsulate fetching the secret and registering it with the router.
For example:
async fn register_hmac_secret_from_name(
&self,
router: &WasmChannelRouter,
channel_name: &str,
hmac_secret_name: &Option<String>,
) {
if let Some(ref hmac_name) = hmac_secret_name {
if let Ok(hmac_secret) = self.secrets.get_decrypted(&self.user_id, hmac_name).await {
router
.register_hmac_secret(channel_name, hmac_secret.expose().to_string())
.await;
}
}
}You could then call this helper from both activate_channel_from_loaded and refresh_active_channel.
There was a problem hiding this comment.
The pattern appears in:
activate_channel_from_loaded(lines 1889-1899)refresh_active_channel(lines 2068-2087)
Technical considerations:
-
Scope limitation: A helper method would only help within
ExtensionManager. The code insrc/main.rsuses a different secrets store reference and user_id ("default"), so it cannot use the same helper. -
Consistency: There's equivalent duplication for Ed25519 signature key registration (
register_signature_key) in the same two locations. If we refactor HMAC, we should also refactor Ed25519 for consistency. -
Complexity vs benefit: Each duplicated block is ~10 lines appearing twice. A helper method adds API surface and indirection for minimal code reduction.
Options:
- Accept as-is (working code, minor duplication is acceptable)
- Refactor both HMAC and Ed25519 registration into a unified helper
Open to either approach - let me know your preference.
zmanian
left a comment
There was a problem hiding this comment.
Review: WhatsApp Webhook Channel
Thanks for this substantial contribution — the HMAC-SHA256 signature verification is solid and the overall webhook architecture is well thought out. However, there are several issues that need addressing before this can merge.
Blocker
.serena/ directory committed — There's an AI tool configuration directory (.serena/) checked into the PR. This needs to be removed and added to .gitignore.
High Priority
-
WhatsApp-specific logic in generic router —
src/channels/wasm/router.rshardcodes WhatsApp-specific behavior (message type detection, status filtering, reaction handling) in what should be a channel-agnostic webhook router. The project guideline is "prefer generic/extensible architectures over hardcoding specific integrations." The router should dispatch to WASM modules and let each channel handle its own message format internally. -
Duplicate
WhatsAppMetadatastructs — There are twoWhatsAppMetadatadefinitions with different field types (one usesOption<String>, the other usesString). These will cause confusion and bugs. Consolidate into one canonical definition, likely in the WASM channel module. -
Dedup TOCTOU race condition — The deduplication check (SELECT then INSERT) has a time-of-check-to-time-of-use race under concurrent webhook deliveries. Use
INSERT ... ON CONFLICT DO NOTHINGor equivalent upsert pattern to make it atomic.
Medium Priority
-
Pending ACK leak on timeout — If a webhook request times out, the pending acknowledgment is never cleaned up. Add a cleanup path or TTL-based expiration.
-
verification_modeas raw string — This should be an enum (VerificationMode::Challenge | VerificationMode::Signature | ...) rather than a stringly-typed field. Follows the project convention of "prefer strong types over strings."
Security (Good)
The HMAC-SHA256 signature verification uses constant-time comparison, which is correct. The webhook secret handling through the capabilities system is clean.
Recommendation
Split the WhatsApp-specific routing logic out of the generic router into the WASM channel module, remove the .serena/ directory, fix the dedup race, and this will be in good shape.
- Remove .serena/ directory (committed by mistake) - Remove is_webhook_message_processed (TOCTOU race condition) - Use atomic INSERT-first pattern in record_webhook_message_processed - Return bool to indicate new vs duplicate message - Fix pending ACK leak on timeout (cleanup before removal) - Fix metadata parsing efficiency (from_value vs to_string+from_str) - Consolidate WhatsAppMetadata structs (Option<String> for all fields) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c26acbd to
06ff62c
Compare
|
@zmanian Thanks for the thorough review. I've addressed all the issues: Blocker
High Priority
Medium Priority
Best, |
06ff62c to
d71ac14
Compare
- Remove .serena/ directory (committed by mistake) - Remove is_webhook_message_processed (TOCTOU race condition) - Use atomic INSERT-first pattern in record_webhook_message_processed - Return bool to indicate new vs duplicate message - Fix pending ACK leak on timeout (cleanup before removal) - Fix metadata parsing efficiency (from_value vs to_string+from_str) - Consolidate WhatsAppMetadata structs (Option<String> for all fields) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove .serena/ directory (committed by mistake) - Remove is_webhook_message_processed (TOCTOU race condition) - Use atomic INSERT-first pattern in record_webhook_message_processed - Return bool to indicate new vs duplicate message - Fix pending ACK leak on timeout (cleanup before removal) - Fix metadata parsing efficiency (from_value vs to_string+from_str) - Consolidate WhatsAppMetadata structs (Option<String> for all fields) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
c3d6b96 to
7e68b0f
Compare
|
Hello @zmanian, I finally adressed your feedback regarding WhatsApp-specific logic in the generic router. Main Changes1. New
2. Generic JSON Pointer (RFC 6901) extraction
3. Removed WhatsApp logic from router
Files Changed
Tests
The router is now truly channel-agnostic and can support any channel via JSON pointer configuration and WASM callbacks. |
- Add on-message-persisted callback to WIT interface for post-persistence actions - Implement mark_as_read in WhatsApp WASM instead of router - Add generic message_id_json_pointer extraction using JSON Pointer (RFC 6901) - Remove WhatsAppMetadata struct and trigger_mark_as_read from router - Remove access_tokens and api_versions storage from router (now channel-owned) - Update capabilities files with message_id_json_pointer field - Add 9 new tests for JSON pointer extraction Addresses PR nearai#403 code review feedback about WhatsApp-specific logic in the generic router. The router is now truly channel-agnostic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove .serena/ directory (committed by mistake) - Remove is_webhook_message_processed (TOCTOU race condition) - Use atomic INSERT-first pattern in record_webhook_message_processed - Return bool to indicate new vs duplicate message - Fix pending ACK leak on timeout (cleanup before removal) - Fix metadata parsing efficiency (from_value vs to_string+from_str) - Consolidate WhatsAppMetadata structs (Option<String> for all fields) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
aa30147 to
a55d1d2
Compare
- Add on-message-persisted callback to WIT interface for post-persistence actions - Implement mark_as_read in WhatsApp WASM instead of router - Add generic message_id_json_pointer extraction using JSON Pointer (RFC 6901) - Remove WhatsAppMetadata struct and trigger_mark_as_read from router - Remove access_tokens and api_versions storage from router (now channel-owned) - Update capabilities files with message_id_json_pointer field - Add 9 new tests for JSON pointer extraction Addresses PR nearai#403 code review feedback about WhatsApp-specific logic in the generic router. The router is now truly channel-agnostic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add on-message-persisted callback to WIT interface for post-persistence actions - Implement mark_as_read in WhatsApp WASM instead of router - Add generic message_id_json_pointer extraction using JSON Pointer (RFC 6901) - Remove WhatsAppMetadata struct and trigger_mark_as_read from router - Remove access_tokens and api_versions storage from router (now channel-owned) - Update capabilities files with message_id_json_pointer field - Add 9 new tests for JSON pointer extraction Addresses PR nearai#403 code review feedback about WhatsApp-specific logic in the generic router. The router is now truly channel-agnostic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
a55d1d2 to
7570f03
Compare
19d0d51 to
ed847d2
Compare
- Remove .serena/ directory (committed by mistake) - Remove is_webhook_message_processed (TOCTOU race condition) - Use atomic INSERT-first pattern in record_webhook_message_processed - Return bool to indicate new vs duplicate message - Fix pending ACK leak on timeout (cleanup before removal) - Fix metadata parsing efficiency (from_value vs to_string+from_str) - Consolidate WhatsAppMetadata structs (Option<String> for all fields) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add on-message-persisted callback to WIT interface for post-persistence actions - Implement mark_as_read in WhatsApp WASM instead of router - Add generic message_id_json_pointer extraction using JSON Pointer (RFC 6901) - Remove WhatsAppMetadata struct and trigger_mark_as_read from router - Remove access_tokens and api_versions storage from router (now channel-owned) - Update capabilities files with message_id_json_pointer field - Add 9 new tests for JSON pointer extraction Addresses PR nearai#403 code review feedback about WhatsApp-specific logic in the generic router. The router is now truly channel-agnostic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ed847d2 to
6d5e648
Compare
🔄 Branch Rebased and SquashedThis branch has been rebased onto 📦 Backup of Previous VersionThe previous version of this branch (8 commits) has been saved at: ✅ What Was Done
🔧 Fixes from Code Reviews
✅ Quality
📝 Final Commit |
6d5e648 to
bf67b71
Compare
Implements secure webhook verification for WhatsApp Cloud API alongside
the existing Slack HMAC verification:
- Add verify_hmac_sha256() for X-Hub-Signature-256 header validation
(WhatsApp/GitHub-style, simple body-only HMAC)
- Add webhook deduplication via database to prevent replay attacks
- Add mark_as_read support for blue checkmarks
- Update WhatsApp capabilities.json with hmac_secret_name config
- Add on_message_persisted callback for post-DB persistence actions
- Move WhatsApp-specific logic to WASM channel from host
The router supports both verification styles:
- Slack: X-Slack-Signature + X-Slack-Request-Timestamp (v0:{ts}:{body})
- WhatsApp: X-Hub-Signature-256 (sha256={hex})
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
bf67b71 to
8664ca5
Compare
|
Closing — core HMAC webhook auth was merged in #970. This WhatsApp-specific extension has diverged too far. |
Summary
Implements end-to-end webhook verification for WhatsApp Cloud API with reliable message processing, including GET verification, POST HMAC-SHA256 signature verification, DB-based deduplication, and mark_as_read for blue checkmarks.
Changes
Part 1 - GET Verification (query_param mode)
verification_modefield toWebhookSchema"query_param": Skip host-level secret validation for GET requests"signature": Always require signature validationNone(default): Current behaviorverification_mode: "query_param"hub.verify_token)Part 2 - POST Verification (HMAC-SHA256)
hmac_secret_namefield toWebhookSchemafor WhatsApp/Slack-style signaturesverify_hmac_sha256()function in signature.rsX-Hub-Signature-256header format:sha256=<hex>whatsapp_app_secretPart 3 - mark_as_read (Blue Checkmarks)
Part 4 - DB-based Webhook Deduplication
webhook_message_deduptable to track processed messagesPart 5 - Reliable ACK Mechanism
Configuration
WEBHOOK_ACK_TIMEOUT_SECSWEBHOOK_DEDUP_CLEANUP_INTERVAL_HOURSFiles Changed
migrations/V10__webhook_dedup.sqlsrc/channels/wasm/router.rssrc/channels/wasm/signature.rssrc/channels/wasm/schema.rssrc/config/wasm.rssrc/db/mod.rssrc/db/postgres.rssrc/db/libsql/webhook_dedup.rssrc/main.rschannels-src/whatsapp/Tests Added
Security
Verification
Tested with real WhatsApp webhooks: