Conversation
Root cause: assigning const char* from a stack-allocated UDPMessage to a JsonVariant stores a linked pointer (ArduinoJson v7 zero-copy policy for const char*). When the message goes out of scope after updateDevices() returns, _state.data holds dangling pointers. The next doc.set(_state.data) reads garbage from the reused stack frame, corrupting all string fields and causing the devices array to grow with duplicate garbled entries each cycle. Fix: wrap all string fields from message structs in String() at assignment time, forcing ArduinoJson to copy the bytes into its pool immediately. Additional hardening applied alongside the root fix: - Validate hostname [a-zA-Z0-9-] in updateDevices() to guard all entry paths (including the sendUDP self-update that bypasses receiveUDP) - Validate hostname in receiveUDP() as defence-in-depth with logging - Add packageSize self-describing check to reject struct-layout mismatches - begin() override clears any persisted garbled state loaded from LittleFS - Always use doc.set(_state.data) (remove the activeClients branch)
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR updates the firmware build date and strengthens device discovery in ModuleDevices by rebuilding the device table on startup, ensuring JSON state is deep-copied to avoid pointer issues, and validating incoming UDP packets with stricter header and hostname checks. ChangesBuild Version Update
Device Discovery Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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/MoonBase/Modules/ModuleDevices.h (1)
217-230: ⚡ Quick winExtract the duplicate hostname validation into a private helper.
The identical charset loop (
[a-zA-Z0-9-]) appears verbatim again inreceiveUDP()(lines 373–378). A future charset change (e.g., allowing dots) must be applied in two places.♻️ Proposed refactor — private `isValidHostname` helper
Add to the
private:section (near the other helpers, around line 447+):+ static bool isValidHostname(const char* name, size_t fieldLen) { + if (name[0] == '\0') return false; + // [a-zA-Z0-9-] only; isprint() NOT used — ESP32 newlib treats 0xA0-0xFF as printable + for (size_t j = 0; j < fieldLen - 1 && name[j]; j++) { + uint8_t c = (uint8_t)name[j]; + if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || c == '-')) + return false; + } + return true; + }Then in
updateDevices():- bool nameOk = message.header.name[0] != '\0'; - for (size_t j = 0; nameOk && j < sizeof(message.header.name) - 1 && message.header.name[j]; j++) { - uint8_t c = (uint8_t)message.header.name[j]; - if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-')) - nameOk = false; - } - if (!nameOk) { + if (!isValidHostname(message.header.name, sizeof(message.header.name))) { EXT_LOGW(MB_TAG, "Skipping device update with invalid name from ...%d", ip[3]); return; }And in
receiveUDP()(lines 373–382):- bool nameOk = message.header.name[0] != '\0'; - for (size_t j = 0; nameOk && j < sizeof(message.header.name) - 1 && message.header.name[j]; j++) { - uint8_t c = (uint8_t)message.header.name[j]; - if (!((c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-')) - nameOk = false; - } - if (nameOk) + if (isValidHostname(message.header.name, sizeof(message.header.name))) updateDevices(message, deviceUDP.remoteIP()); else EXT_LOGW(MB_TAG, "Garbled name in packet from ...%d, rejecting", deviceUDP.remoteIP()[3]);🤖 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/MoonBase/Modules/ModuleDevices.h` around lines 217 - 230, Extract the duplicate hostname validation into a single private helper named isValidHostname that accepts a const char* (or reference to the name buffer) and implements the current logic: non-empty first char, iterate up to sizeof(name)-1 or until NUL, and allow only [a-zA-Z0-9-]; then replace the inline loop in updateDevices() and the identical loop in receiveUDP() to call isValidHostname(...) and preserve the current behavior (log & return when false). Ensure the helper is declared in the class private: section and used in both updateDevices() and receiveUDP().
🤖 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/MoonBase/Modules/ModuleDevices.h`:
- Around line 217-230: Extract the duplicate hostname validation into a single
private helper named isValidHostname that accepts a const char* (or reference to
the name buffer) and implements the current logic: non-empty first char, iterate
up to sizeof(name)-1 or until NUL, and allow only [a-zA-Z0-9-]; then replace the
inline loop in updateDevices() and the identical loop in receiveUDP() to call
isValidHostname(...) and preserve the current behavior (log & return when
false). Ensure the helper is declared in the class private: section and used in
both updateDevices() and receiveUDP().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 69f6fab1-68c9-4eb5-9479-716200064716
📒 Files selected for processing (2)
platformio.inisrc/MoonBase/Modules/ModuleDevices.h
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores