fix(messages): address Copilot/Codex PR #75 CRITICAL review - JID normalization race condition

This commit addresses CRITICAL vulnerabilities identified in Copilot/Codex code review
that could cause message ordering violations and parallel processing of the same conversation.

CRITICAL ISSUE IDENTIFIED:
- KeyedMutex was using raw msg.key.remoteJid BEFORE normalizeMessageJids() runs
- Messages from the SAME chat arriving with different JID formats (LID vs PN) would
  acquire DIFFERENT mutex keys, causing parallel processing instead of sequential
- This breaks message ordering guarantees within a conversation

Example of the bug:
  Message 1: remoteJid="123456789.0:1@lid" (LID format)
  Message 2: remoteJid="5511999999999@s.whatsapp.net" (PN format, SAME chat)

  Before fix: Different mutex keys → parallel processing → ORDERING VIOLATION
  After fix: Normalized to same PN → same mutex key → sequential processing 

Changes:
1. **messages-recv.ts (CRITICAL FIX):**
   - Move normalizeMessageJids() BEFORE mutex acquisition (line 1273)
   - Ensures all messages from same chat use identical normalized JID as mutex key
   - Remove duplicate normalizeMessageJids() call inside mutex
   - Add detailed comments explaining the criticality

2. **messages-send.ts (CODE QUALITY):**
   - Refactor IIFE pattern to conditional for better readability (4 locations)
   - Change from: const mutexKey = x || (() => { ... })()
   - Change to: let mutexKey = x; if (!mutexKey) { ... }
   - Improves code maintainability per Copilot review

Impact Analysis:
-  Fixes race condition that could reorder messages from same conversation
-  Maintains parallel processing across DIFFERENT conversations (performance preserved)
-  Zero impact on message delivery (only affects internal processing order)
- ⚠️ Adds ~1-2ms latency per message (async normalization before mutex)
  - ACCEPTABLE: Correctness > Micro-optimization

Testing Recommendations:
- Test concurrent messages from same chat arriving with mixed LID/PN formats
- Verify message ordering is preserved
- Monitor performance impact on high-traffic scenarios

Addresses:
- Copilot PR #75 Comment 1 (Critical: JID normalization)
- Codex PR #75 Comment 1 (P2: Same issue)
- Copilot PR #75 Comments 2-5 (Medium: IIFE readability)

https://claude.ai/code/session_0149ZKk2ygmKCJTGu39Mr8oH
This commit is contained in:
Claude
2026-02-03 14:12:25 +00:00
parent d6830c957e
commit 9d4442f053
2 changed files with 30 additions and 17 deletions
+18 -8
View File
@@ -1265,14 +1265,23 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => {
)
}
// CRITICAL: Normalize JIDs BEFORE acquiring mutex to ensure messages from the same
// chat (arriving with different JID formats - LID vs PN) use the SAME mutex key.
// This prevents parallel processing of messages from the same conversation which
// would break message ordering guarantees.
// Addresses Copilot/Codex PR #75 critical review: JID normalization vulnerability
await normalizeMessageJids(msg, signalRepository, logger)
try {
// Use KeyedMutex with remoteJid to allow parallel processing of messages from different chats
// while maintaining order for messages within the same chat
// Fallback: msg.key.id (unique per message) > 'unknown' (serializes all unknown messages)
const mutexKey = msg.key.remoteJid || (() => {
logger.warn({ msgId: msg.key.id, fromMe: msg.key.fromMe }, 'Missing remoteJid in message key, using msg.key.id as fallback')
return msg.key.id || 'unknown'
})()
// Use KeyedMutex with NORMALIZED remoteJid for parallel processing across different chats
// while maintaining sequential order within the same chat
// Fallback chain: remoteJid (normalized) > msg.key.id (unique) > 'unknown' (serializes all)
let mutexKey = msg.key.remoteJid
if (!mutexKey) {
logger.warn({ msgId: msg.key.id, fromMe: msg.key.fromMe }, 'Missing remoteJid after normalization, using msg.key.id as fallback')
mutexKey = msg.key.id || 'unknown'
}
await messageMutex.mutex(mutexKey, async () => {
await decrypt()
// message failed to decrypt
@@ -1487,7 +1496,8 @@ export const makeMessagesRecvSocket = (config: SocketConfig) => {
}
}
await normalizeMessageJids(msg, signalRepository, logger)
// JID normalization moved BEFORE mutex acquisition (line 1273) to prevent race conditions
// cleanMessage still runs inside mutex to ensure atomic message processing
cleanMessage(msg, authState.creds.me!.id, authState.creds.me!.lid!)
await upsertMessage(msg, node.attrs.offline ? 'append' : 'notify')