fix(core): harden zodSocket protocol detection against payload field ambiguity#2898
Conversation
…rruption Fixed a critical bug where user messages containing a 'payload' property were incorrectly identified as protocol-wrapped messages, causing data loss. Changes: - Enhanced hasValidPayload check to require both 'payload' AND 'version' - Added isObject() helper to exclude arrays and null values - Refactored message normalization into dedicated normalizeMessage() method - Added comprehensive test suite with 11 test cases covering all edge cases The fix ensures that only messages with the full protocol signature (version + payload as object) are treated as wrapped messages. Fixes: Protocol ambiguity where user data with 'payload' field was misinterpreted Tests: 11/11 passing, including edge cases for non-object payloads and missing version
|
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request refactors message handling in ZodSocketMessageHandler by introducing an internal normalization layer. The change consolidates how different incoming message formats are processed into a consistent internal representation (NormalizedMessage), replacing previous conditional logic with a unified normalization function. A comprehensive test suite is added covering protocol detection, payload handling, validation, error scenarios, and callback integration. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@ShivaReddyVanja appreciate the PR but this is the type of change that we can't really accept. The amount of risk changing something like this is too high, and would require extensive testing |
|
ok, i completely understand . Is there any other area that i can help, happy to fix ! |
Review CompleteYour review story is ready! Comment !reviewfast on this PR to re-generate the story. |
Problem
Fixed a critical bug where user messages containing a
payloadproperty were incorrectly identified as protocol-wrapped messages, causing data corruption.The original implementation only checked for the presence of
payloadto determine if a message was wrapped, leading to false positives when user data legitimately contained apayloadfield.Solution
Enhanced protocol detection to require both
payloadANDversionproperties, plus validation thatpayloadis a plain object (not array/null/primitive).Changes:
The fix ensures that only messages with the full protocol signature (version + payload as object) are treated as wrapped messages.
✅ Checklist
Testing
✅ Added a new test file covering 11 cases
✅ All 11 new tests passing
✅ All 392 existing core tests passing
Changelog
Fixes: Protocol ambiguity where user data with 'payload' field was misinterpreted
Added Tests: 11/11 passing, including edge cases for non-object payloads and missing version
Screenshots
💯