Repository navigation
Conversation
ClientACL::load() and the companion's DataStore::loadContacts() keep the stored out_path_len as read. Any value but OUT_PATH_UNKNOWN counts as a known path, so replies to that peer go DIRECT. 136 of the 256 values fail Packet::isValidPathLen(): hash size 4 is reserved, and the path may not exceed MAX_PATH_SIZE. Mesh::sendDirect() keeps the value as the header's path_len, and Dispatcher::sendPacket() then frees the packet as invalid, so the peer gets no reply and nothing is logged outside a debug build. The firmware stores only paths it has validated, so this takes a damaged file. Reset an invalid value to OUT_PATH_UNKNOWN on load, so replies to that peer flood until the path is learned again. ZephCore made the same check in liquidraver/ZephCore@fcfee67. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
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 |
There was a problem hiding this comment.
🟡 Changes recommended
The secure-chat contact loader still accepts invalid stored path lengths that can cause ACKs and replies to be dropped.
1 open finding
What changed in this PR
This PR prevents corrupted stored path lengths from causing silent packet drops in the ACL and companion-radio contact loaders.
Changes:
- Reset invalid stored path lengths to “unknown” so replies can flood until a path is learned again.
- Preserve valid lengths and the unknown-path sentinel.
| File | Description |
|---|---|
src/helpers/ClientACL.cpp |
Validates stored client path lengths on load. |
examples/companion_radio/DataStore.cpp |
Validates stored companion contact path lengths on load. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
simple_secure_chat reads /contacts with the same unchecked out_path_len byte before addContact(). Found by Copilot review on fork PR #28. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Iteration branch for #27. Keep it open; never merge it. A clean single-commit branch for upstream is cut from it once it has baked. Pieter's decision on 2026-10-10: nothing goes upstream until a maintainer engages with one of our open PRs.
References:
dev2dbd463e89f732fa, which adds the secure-chat loader that Copilot found on top of8d85ab5cstored-path-len-harness.cpp, a host transcription of the load,Mesh::sendDirect()andDispatcher::sendPacket(), run over all 256 byte values.The defect
ClientACL::load(), the companion'sDataStore::loadContacts()andsimple_secure_chat's contact loader keep the storedout_path_lenexactly as read. Any value other thanOUT_PATH_UNKNOWNcounts as a known path, so replies to that peer go DIRECT.Mesh::sendDirect()copies an invalid value into the header unchanged, andDispatcher::sendPacket()then frees the packet without sending it. #27 has the permalinks.The fix
ClientACL.cppL42-L44 andDataStore.cppL283-L285 reset anout_path_lenthat failsPacket::isValidPathLen()toOUT_PATH_UNKNOWNonce the record is read, and so doessimple_secure_chat/main.cppL119-L121. No other loader readsout_path_lenfrom a file. Replies to that peer then flood until the path is learned again.Evidence
From the harness, for all 256 values the stored byte could hold:
out_path_lendevValid values (119) and FFh behave the same as before.
Testing
RAK_4631_repeater,RAK_4631_sensor,RAK_4631_companion_radio_usbandheltec_v4_r8_companion_radio_usball build at8d85ab5c.RAK_4631_terminal_chatandheltec_v4_r8_terminal_chatbuild at89f732fa./s_contacts,/contacts3or/contactsin flash, and the only test board is held on the RV3028 acceptance build.Notes
CMD_ADD_UPDATE_CONTACTtakesout_path_lenfrom the app's frame unchecked. That value comes from the connected app, not from flash, so it's left for a separate decision.ClientACLin liquidraver/ZephCore@fcfee67. ZephCore's companion contact loader and its add-contact frame have the same gap. They are tracked on the ZephCore fork as ptr727/liquidraver-ZephCore#56 and ptr727/liquidraver-ZephCore#57.Refs #27
🤖 Generated with Claude Code