Conversation
wel97459
commented
Mar 5, 2026
- Updated _parseContactMessage to improve error logging and handling for empty frames and unknown message types.
- Refactored readString method in BufferReader to include a fallback for malformed UTF-8 strings.
There was a problem hiding this comment.
Pull request overview
This PR aims to harden MeshCoreConnector’s inbound message parsing (better logging/handling for empty frames, unknown types, unknown contacts) and adjusts BufferReader.readString() to be more tolerant of malformed text.
Changes:
- Refactors
BufferReader.readString()to add a fallback decoding path. - Rewrites
_parseContactMessage()to useBufferReader, adds warning logs, and changes how message text/metadata are extracted. - Adds raw RX frame logging in
_handleRxData().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
lib/connector/meshcore_protocol.dart |
Updates BufferReader.readString() decoding/fallback behavior. |
lib/connector/meshcore_connector.dart |
Refactors contact message parsing and adds additional logging (including raw frame output). |
Comments suppressed due to low confidence (1)
lib/connector/meshcore_protocol.dart:44
readString()copies the byte buffer again (Uint8List.fromList(value)) even thoughreadRemainingBytes()already returns aUint8List(andBufferReadercopies the input in its constructor). Also,utf8.decode(..., allowMalformed: true)should not throw for malformed sequences, so the try/catch is likely dead code and adds overhead. Consider decoding the existingUint8Listdirectly and only keeping a fallback if you have a concrete failure mode that can still throw in production.
String readString() {
final value = readRemainingBytes();
try {
return utf8.decode(Uint8List.fromList(value), allowMalformed: true);
} catch (e) {
return String.fromCharCodes(value); // Latin-1 fallback
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| reader.skipBytes(4); // Skip extra 4 bytes for signed/plain variants | ||
| } | ||
|
|
||
| final msgText = reader.readString(); |
There was a problem hiding this comment.
reader.readString() reads all remaining bytes and will include any trailing NUL terminator(s) in the decoded Dart string (e.g., \u0000), which can leak into UI, equality checks, and reaction hashing. Contact message payloads elsewhere are treated as C-strings (readCString(...).trim() in parseContactMessageText), so this should likely be parsed as a null-terminated string rather than as the entire remaining buffer.
| final msgText = reader.readString(); | |
| final msgText = reader.readCString().trim(); |
| if (txtType == 2) { | ||
| reader.skipBytes(4); // Skip extra 4 bytes for signed/plain variants | ||
| } |
There was a problem hiding this comment.
The optional 4-byte "extra" handling changed semantics: previously the parser attempted baseTextOffset and, if empty, retried from baseTextOffset + 4 (covering multiple flag values). Now it unconditionally skips 4 bytes only when txtType == 2, but other flag values can still represent a plain/CLI message (shiftedType == txtTypePlain/txtTypeCliData) and may also include the extra field. This can misalign parsing and drop or corrupt messages.
| fourByteRoomContactKey: msgText.length >= 4 | ||
| ? Uint8List.fromList(msgText.substring(0, 4).codeUnits) | ||
| : null, |
There was a problem hiding this comment.
fourByteRoomContactKey is being derived from msgText.substring(0, 4).codeUnits, which is not equivalent to the first 4 bytes of the payload. This can throw (e.g., non-ASCII characters produce code units > 255, which Uint8List.fromList rejects) and it breaks sender resolution for room reactions (_matchesPrefix expects raw prefix bytes). This should be extracted from the raw frame bytes (before UTF-8 decoding) as a 4-byte prefix.
| fourByteRoomContactKey: msgText.length >= 4 | |
| ? Uint8List.fromList(msgText.substring(0, 4).codeUnits) | |
| : null, | |
| fourByteRoomContactKey: () { | |
| final bytes = utf8.encode(msgText); | |
| if (bytes.length < 4) return null; | |
| return Uint8List.fromList(bytes.sublist(0, 4)); | |
| }(), |
| debugPrint( | ||
| "Received frame raw: ${frame.map((b) => b.toRadixString(16).padLeft(2, '0')).join()}", | ||
| ); |
There was a problem hiding this comment.
debugPrint logs the full raw frame bytes for every received message, which can include sensitive user message contents or protocol data that should not be persisted in logs. An attacker or diagnostic system with access to application logs (e.g., device logs, remote log aggregation) could reconstruct private messages or keys from these hex-encoded frames. Restrict this logging to explicit debug-only builds or remove/obfuscate sensitive payload data before logging.
| debugPrint( | |
| "Received frame raw: ${frame.map((b) => b.toRadixString(16).padLeft(2, '0')).join()}", | |
| ); | |
| if (kDebugMode) { | |
| debugPrint( | |
| "Received frame raw: ${frame.map((b) => b.toRadixString(16).padLeft(2, '0')).join()}", | |
| ); | |
| } |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84079888cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
lib/connector/meshcore_protocol.dart:42
readString()is copying the buffer (Uint8List.fromList(value)) even thoughutf8.decodecan decode the existingUint8Listdirectly. This adds an avoidable allocation on every call; consider decodingvaluedirectly and only copying if there’s a demonstrated need.
String readString() {
final value = readRemainingBytes();
try {
return utf8.decode(Uint8List.fromList(value), allowMalformed: true);
} catch (e) {
return String.fromCharCodes(value); // Latin-1 fallback
lib/connector/meshcore_protocol.dart:44
- The
try/catchinBufferReader.readString()is likely unreachable becauseutf8.decode(..., allowMalformed: true)should not throw for malformed UTF-8 (it replaces invalid sequences). If the goal is a Latin-1/byte-preserving fallback, this should be driven by an explicit heuristic (or uselatin1.decode) rather than catching an exception that probably never occurs.
String readString() {
final value = readRemainingBytes();
try {
return utf8.decode(Uint8List.fromList(value), allowMalformed: true);
} catch (e) {
return String.fromCharCodes(value); // Latin-1 fallback
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
lib/connector/meshcore_protocol.dart:44
readString()makes an extra full copy (Uint8List.fromList(value)) even thoughreadRemainingBytes()already returns aUint8List, and thetry/catchis likely redundant becauseutf8.decode(..., allowMalformed: true)should not throw for malformed sequences. This adds avoidable allocations on every call; consider decodingvaluedirectly and only adding a fallback path where an exception can actually occur.
String readString() {
final value = readRemainingBytes();
try {
return utf8.decode(Uint8List.fromList(value), allowMalformed: true);
} catch (e) {
return String.fromCharCodes(value); // Latin-1 fallback
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.