Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds validation to ensure seeker fields in reply and forward messages are complete before deserialization, preventing truncated messages from causing errors downstream. It introduces a new assertAvailable helper function to validate buffer boundaries and adds defensive checks in the message lookup logic.
Changes:
- Added
assertAvailablehelper function to validate buffer size before reading message components during deserialization - Added buffer size validation checks at critical points in reply and forward message deserialization (content length, content, and seeker)
- Added defensive validation in
MessageService.findMessageBySeekerto handle empty or invalid seekers gracefully - Added test cases to verify truncated messages throw appropriate errors
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| gossip-sdk/src/utils/messageSerialization.ts | Adds assertAvailable helper function and buffer validation checks in reply/forward message deserialization to ensure seekers are complete |
| gossip-sdk/src/services/message.ts | Adds defensive check to validate seeker is non-empty Uint8Array before database lookup, preventing IndexedDB errors |
| gossip-sdk/test/utils/serialization.spec.ts | Adds test cases for truncated seekers in reply and forward messages to ensure proper error handling |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assertAvailable(buffer, offset, 4, 'forward'); | ||
| const forwardContentLen = Number( | ||
| U32.fromBytes(buffer.slice(offset, offset + 4)) | ||
| ); | ||
| offset += 4; | ||
|
|
||
| // Read forward content | ||
| assertAvailable(buffer, offset, forwardContentLen, 'forward'); |
There was a problem hiding this comment.
The assertAvailable function validates that enough bytes are available for the content length itself, but it does not validate that the content length value is reasonable before attempting to validate the content bytes. If forwardContentLen is an extremely large number (e.g., due to corruption), this could lead to an unhelpful error message or potential performance issues. Consider adding a sanity check on the length value (e.g., maximum message size) before using it to validate content availability.
| if (!(seeker instanceof Uint8Array) || seeker.byteLength === 0) { | ||
| logger | ||
| .forMethod('findMessageBySeeker') | ||
| .warn('skipping invalid seeker lookup', { | ||
| ownerUserId, | ||
| seekerLength: | ||
| seeker instanceof Uint8Array ? seeker.byteLength : 'not-uint8array', | ||
| }); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
The defensive check validates that the seeker is a Uint8Array and has non-zero length, but does not verify that it has the expected length of 34 bytes (SEEKER_SIZE). While the deserialization validation should ensure seekers are always 34 bytes, adding a check for the exact size would make this function more robust and would catch any potential bugs in the deserialization logic or other code paths that might pass seekers to this function. Consider adding a check like: seeker.byteLength !== 34
| // Malformed payloads can carry an empty seeker in reply/forward metadata. | ||
| // fake-indexeddb rejects that as an IndexedDB key and throws DataError. | ||
| if (!(seeker instanceof Uint8Array) || seeker.byteLength === 0) { | ||
| logger | ||
| .forMethod('findMessageBySeeker') | ||
| .warn('skipping invalid seeker lookup', { | ||
| ownerUserId, | ||
| seekerLength: | ||
| seeker instanceof Uint8Array ? seeker.byteLength : 'not-uint8array', | ||
| }); |
There was a problem hiding this comment.
The logging pattern here creates the logger inline rather than storing it in a const variable like other methods in this file (e.g., fetchMessages, storeDecryptedMessages). While this works correctly, for consistency with the rest of the file, consider creating a const log variable at the start of the function: const log = logger.forMethod('findMessageBySeeker'); and then using log.warn(...). This is a minor stylistic preference based on the patterns seen elsewhere in this file.
| // Malformed payloads can carry an empty seeker in reply/forward metadata. | |
| // fake-indexeddb rejects that as an IndexedDB key and throws DataError. | |
| if (!(seeker instanceof Uint8Array) || seeker.byteLength === 0) { | |
| logger | |
| .forMethod('findMessageBySeeker') | |
| .warn('skipping invalid seeker lookup', { | |
| ownerUserId, | |
| seekerLength: | |
| seeker instanceof Uint8Array ? seeker.byteLength : 'not-uint8array', | |
| }); | |
| const log = logger.forMethod('findMessageBySeeker'); | |
| // Malformed payloads can carry an empty seeker in reply/forward metadata. | |
| // fake-indexeddb rejects that as an IndexedDB key and throws DataError. | |
| if (!(seeker instanceof Uint8Array) || seeker.byteLength === 0) { | |
| log.warn('skipping invalid seeker lookup', { | |
| ownerUserId, | |
| seekerLength: | |
| seeker instanceof Uint8Array ? seeker.byteLength : 'not-uint8array', | |
| }); |
| expect(() => deserializeMessage(truncated)).toThrow( | ||
| 'Invalid reply message' | ||
| ); |
There was a problem hiding this comment.
The error message expectation does not match the actual error message thrown by the assertAvailable function. The assertAvailable function throws a more detailed error message that includes the expected bytes, offset, and actual bytes received (e.g., "Invalid reply message: expected 34 bytes at offset X, got Y"). The test expects only "Invalid reply message" which will cause this test to fail. Consider updating the expectation to match the full error message format or use a partial string match.
| expect(() => deserializeMessage(truncated)).toThrow( | ||
| 'Invalid forward message' | ||
| ); |
There was a problem hiding this comment.
The error message expectation does not match the actual error message thrown by the assertAvailable function. The assertAvailable function throws a more detailed error message that includes the expected bytes, offset, and actual bytes received (e.g., "Invalid forward message: expected 34 bytes at offset X, got Y"). The test expects only "Invalid forward message" which will cause this test to fail. Consider updating the expectation to match the full error message format or use a partial string match.
| assertAvailable(buffer, offset, 4, 'reply'); | ||
| const originalContentLen = Number( | ||
| U32.fromBytes(buffer.slice(offset, offset + 4)) | ||
| ); | ||
| offset += 4; | ||
|
|
||
| // Read original content | ||
| assertAvailable(buffer, offset, originalContentLen, 'reply'); |
There was a problem hiding this comment.
The assertAvailable function validates that enough bytes are available for the content length itself, but it does not validate that the content length value is reasonable before attempting to validate the content bytes. If originalContentLen or forwardContentLen is an extremely large number (e.g., due to corruption), this could lead to an unhelpful error message or potential performance issues. Consider adding a sanity check on the length value (e.g., maximum message size) before using it to validate content availability.
1 participant
No description provided.