fix(serial): drop blank and line-noise text messages in TEXTMSG mode - #11380
fix(serial): drop blank and line-noise text messages in TEXTMSG mode#11380samirbhattarai135 wants to merge 2 commits into
Conversation
TEXTMSG mode forwarded whatever readBytes() returned straight into a mesh packet, so pressing ENTER in a terminal sent a message whose entire content was CR/LF, and a node booting with a floating RX pin sent a byte of line noise as a text message. sanitizeTextMessagePayload() compacts the payload in place to what is worth sending: control characters other than newline and tab are dropped, as are bytes that are not valid UTF-8, and surrounding whitespace is trimmed. Nothing left means nothing is sent. Only TEXTMSG mode is sanitized, so DEFAULT and SIMPLE stay byte-transparent; the length check applies to every mode, since a zero-length read was never worth a packet either. The UTF-8 validity check sanitizeUtf8() performed inline is extracted as utf8SequenceLength() so both callers share one implementation.
|
|
|
|
@samirbhattarai135, Welcome to Meshtastic!Thanks for opening your first pull request. We really appreciate it. We discuss work as a team in discord, please join us in the #firmware channel. Welcome to the team 😄 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds shared UTF-8 validation and uses it to sanitize serial text-message payloads. Invalid bytes, disallowed controls, and surrounding whitespace are removed. Empty sanitized payloads are not transmitted. Tests cover validation and sanitization behavior. ChangesUTF-8 and serial text sanitization
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/meshUtils.h (1)
73-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConform new comments to the project comment policy.
These comments exceed two lines or repeat information that the code already states.
src/meshUtils.h#L73-L75: reduce the API comment to two lines or less.src/modules/SerialModule.h#L16-L19: reduce the API comment to two lines or less.test/test_serial/SerialModule.cpp#L122-L206: remove comments that only restate each test name or assertion. Keep byte-sequence rationale where needed.As per coding guidelines, “Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/meshUtils.h` around lines 73 - 75, Shorten the API comment in src/meshUtils.h lines 73-75 to no more than two lines while retaining only non-obvious behavior. Similarly reduce the API comment in src/modules/SerialModule.h lines 16-19 to two lines or less. In test/test_serial/SerialModule.cpp lines 122-206, remove comments that merely repeat test names or assertions, preserving only comments explaining necessary byte-sequence rationale.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/meshUtils.h`:
- Around line 73-75: Shorten the API comment in src/meshUtils.h lines 73-75 to
no more than two lines while retaining only non-obvious behavior. Similarly
reduce the API comment in src/modules/SerialModule.h lines 16-19 to two lines or
less. In test/test_serial/SerialModule.cpp lines 122-206, remove comments that
merely repeat test names or assertions, preserving only comments explaining
necessary byte-sequence rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de082574-f158-4bda-965f-cda2f18c20a9
📒 Files selected for processing (6)
src/meshUtils.cppsrc/meshUtils.hsrc/modules/SerialModule.cppsrc/modules/SerialModule.htest/test_serial/SerialModule.cpptest/test_utf8/test_main.cpp
Drop the comments that only restated a test name, and shorten the two API comments. What is left is the byte-sequence rationale and the reason a case exists at all.
3 participants
Fixes #6444.
What was wrong
In TEXTMSG mode the read loop in
SerialModule::runOnce()forwarded whateverreadBytes()returned straight into a mesh packet, with no check on length or content:Both symptoms in the issue follow from that directly:
What changed
sanitizeTextMessagePayload()(src/modules/SerialModule.cpp) compacts a TEXTMSG payload in place and returns the length worth sending. Control characters other than\nand\tare dropped, so are bytes that are not valid UTF-8, then surrounding whitespace is trimmed. Nothing left means nothing is sent. It sits outside the architecture guard next toserialConfigIsValid()for the same reason given there: it touches no serial hardware, so it can be tested on the native target.utf8SequenceLength()(src/meshUtils.cpp) is the UTF-8 validity checksanitizeUtf8()already performed inline, extracted so both callers share one implementation.sanitizeUtf8()behaviour is unchanged: the existingtest_utf8cases (truncated sequences, overlong encodings, surrogate halves, 5-byte forms, buffer-end truncation) all pass unmodified.One deviation from the report
The issue asks for non-ASCII characters to be rejected. This implements "must be valid UTF-8" instead, because a hard ASCII filter would stop people sending text in their own language over serial. Random line noise essentially never forms a valid UTF-8 sequence, so the reported symptom is still covered. Happy to tighten it to ASCII-only if you would rather follow the report literally.
Known limit either way: a noise byte that happens to land in printable ASCII is indistinguishable from a one-character message, so that one still goes out.
Testing
test_serialgains 10 cases for the new helper: CR/LF only, whitespace only, a lone0xFF, surrounding-whitespace trimming, control characters mid-message, interior newline/tab kept, valid multi-byte UTF-8 preserved, invalid UTF-8 dropped, plain text untouched, empty read.test_utf8gains 8 cases for the extracted primitive.test_serial20/20 andtest_utf830/30 under the sanitizedcoverageenv.test_packet_signingtest_B11_normal_unicast_still_uses_pkiandtest_B12_licensed_receiver_does_not_decrypt_pki, which fail identically on an unmodified export of the parent commit, so they are pre-existing and unrelated. One suite (test_traceroute_nexthop) errored on an out-of-memorycc1pluskill inside my 2 GB container and passes 3/3 when run on its own.heltec-v3to check the arch-guarded call site: SUCCESS.🤝 Attestations
I do not have a Meshtastic device, so the hardware boxes are deliberately unchecked: the behaviour is verified by the native unit tests and the
heltec-v3build only, and the real UART path has not been exercised. A check from anyone with a TEXTMSG-configured node and a serial adapter would be very welcome - pressing ENTER should now send nothing, and a cold boot should no longer emit a stray message.Summary by CodeRabbit
New Features
Bug Fixes
Tests