Block coordinate traffic on configured event channels#11045
Open
ayysasha wants to merge 14 commits into
Open
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Contributor
📝 WalkthroughWalkthroughAdds compile-time event-channel coordinate privacy enforcement across mesh routing, phone ingress, reliable delivery, position precision, and MQTT reporting. It also centralizes channel/PKI decisions, updates routing reprocessing, and adds focused native test coverage and CI execution. ChangesEvent channel policy
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PhoneAPI
participant Router
participant Channels
participant Radio
PhoneAPI->>Router: submit coordinate packet
Router->>Channels: resolve effective channel and event status
Channels-->>Router: event-channel policy result
alt blocked coordinate
Router-->>PhoneAPI: return NOT_AUTHORIZED and notification
else allowed coordinate
Router->>Radio: send packet
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Author
|
CI follow-up: the branch-caused Native PlatformIO failure has been fixed in 6c941f3. The remaining check-label job requires an accepted PR label; I do not have permission to add labels. Could a maintainer please add the enhancement label? |
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Author
|
Added behavioral unit coverage in 9d35291..470c40b: real Router TX/RX paths, NextHop/Flooding/Reliable relay-retry-ACK paths, StreamAPI/PhoneAPI retry-state handling, and MQTT publication/map reporting. The policy-enabled and gate-off five-suite matrices each pass 121/121 cases; native build and targeted Trunk checks pass. The new suites are registered in the policy CI environment and canonical native suite count. |
ayysasha
marked this pull request as ready for review
July 18, 2026 04:56
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/mesh/FloodingRouter.cpp (1)
95-104: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRestrict policy-driven decoding to event-channel candidates.
Setting
shouldDecode = truedecrypts every private and PKI duplicate/upgraded packet when the policy is enabled. Resolve the effective channel first, while independently retaining traceroute decoding.Proposed change
bool shouldDecode = false; `#if` USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL - shouldDecode = true; -#elif !MESHTASTIC_EXCLUDE_TRACEROUTE - shouldDecode = traceRouteModule != nullptr; + shouldDecode = !p->pki_encrypted && channels.isEventChannel(getEffectiveChannelIndex(p)); +#endif +#if !MESHTASTIC_EXCLUDE_TRACEROUTE + shouldDecode = shouldDecode || traceRouteModule != nullptr; `#endif`🤖 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/mesh/FloodingRouter.cpp` around lines 95 - 104, Update the decoding decision around shouldDecode in the duplicate handling path to resolve the packet’s effective channel first and enable policy-driven decoding only for event-channel candidates, rather than every packet. Keep traceroute-driven decoding independently enabled when traceRouteModule is available, including builds where traceroute is not excluded, and preserve the existing perhapsDecode flow.test/test_position_precision/test_main.cpp (1)
274-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep changed comments within the two-line limit.
test/test_position_precision/test_main.cpp#L274-L276: condense the gate-behavior explanation.test/test_position_precision/test_main.cpp#L408-L410: reflow the private-key explanation.test/test_nexthop_routing/test_main.cpp#L610-L612: remove or shorten the decorative heading.As per coding guidelines, “Keep code comments minimal—one or two lines maximum.”
🤖 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 `@test/test_position_precision/test_main.cpp` around lines 274 - 276, Shorten the gate-behavior comment at test/test_position_precision/test_main.cpp lines 274-276 to no more than two lines while retaining its meaning; reflow the private-key explanation at lines 408-410 to the same limit; and remove or shorten the decorative heading at test/test_nexthop_routing/test_main.cpp lines 610-612. No code changes are required.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.
Inline comments:
In `@src/mesh/FloodingRouter.cpp`:
- Around line 109-112: Update the LOG_WARN call in
FloodingRouter::reprocessPacket to format the getFrom(p) node ID with the
hexadecimal 0x%08x specifier instead of decimal %u, keeping the existing
arguments and message context unchanged.
---
Nitpick comments:
In `@src/mesh/FloodingRouter.cpp`:
- Around line 95-104: Update the decoding decision around shouldDecode in the
duplicate handling path to resolve the packet’s effective channel first and
enable policy-driven decoding only for event-channel candidates, rather than
every packet. Keep traceroute-driven decoding independently enabled when
traceRouteModule is available, including builds where traceroute is not
excluded, and preserve the existing perhapsDecode flow.
In `@test/test_position_precision/test_main.cpp`:
- Around line 274-276: Shorten the gate-behavior comment at
test/test_position_precision/test_main.cpp lines 274-276 to no more than two
lines while retaining its meaning; reflow the private-key explanation at lines
408-410 to the same limit; and remove or shorten the decorative heading at
test/test_nexthop_routing/test_main.cpp lines 610-612. No code changes are
required.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f43b6709-be09-429e-a95c-e81d2bdc9e8c
📒 Files selected for processing (20)
.github/workflows/test_native.ymlsrc/mesh/Channels.cppsrc/mesh/Channels.hsrc/mesh/FloodingRouter.cppsrc/mesh/FloodingRouter.hsrc/mesh/NextHopRouter.cppsrc/mesh/PhoneAPI.cppsrc/mesh/PositionPrecision.cppsrc/mesh/ReliableRouter.cppsrc/mesh/Router.cppsrc/mesh/Router.hsrc/mqtt/MQTT.cpptest/native-suite-counttest/test_event_channel_phone_api/test_main.cpptest/test_event_channel_router/test_main.cpptest/test_mqtt/MQTT.cpptest/test_nexthop_routing/test_main.cpptest/test_position_precision/test_main.cppuserPrefs.jsoncvariants/native/portduino/platformio.ini
Comment on lines
109
to
+112
| // Fatal decoding error, we can't do anything with this packet | ||
| LOG_WARN( | ||
| "FloodingRouter::reprocessPacket: Fatal decode error (state=%d, id=0x%08x, from=%u), can't check for traceroute", | ||
| static_cast<int>(decodedState), p->id, getFrom(p)); | ||
| return; | ||
| LOG_WARN("FloodingRouter::reprocessPacket: decode error (state=%d, id=0x%08x, from=%u)", | ||
| static_cast<int>(decodedState), p->id, getFrom(p)); | ||
| return true; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Format from as a node ID.
Use 0x%08x rather than decimal %u.
- LOG_WARN("FloodingRouter::reprocessPacket: decode error (state=%d, id=0x%08x, from=%u)",
+ LOG_WARN("FloodingRouter::reprocessPacket: decode error (state=%d, id=0x%08x, from=0x%08x)",As per coding guidelines, “Format node and packet IDs as 0x%08x.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Fatal decoding error, we can't do anything with this packet | |
| LOG_WARN( | |
| "FloodingRouter::reprocessPacket: Fatal decode error (state=%d, id=0x%08x, from=%u), can't check for traceroute", | |
| static_cast<int>(decodedState), p->id, getFrom(p)); | |
| return; | |
| LOG_WARN("FloodingRouter::reprocessPacket: decode error (state=%d, id=0x%08x, from=%u)", | |
| static_cast<int>(decodedState), p->id, getFrom(p)); | |
| return true; | |
| // Fatal decoding error, we can't do anything with this packet | |
| LOG_WARN("FloodingRouter::reprocessPacket: decode error (state=%d, id=0x%08x, from=0x%08x)", | |
| static_cast<int>(decodedState), p->id, getFrom(p)); | |
| return true; |
🤖 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/mesh/FloodingRouter.cpp` around lines 109 - 112, Update the LOG_WARN call
in FloodingRouter::reprocessPacket to format the getFrom(p) node ID with the
hexadecimal 0x%08x specifier instead of decimal %u, keeping the existing
arguments and message context unchanged.
Source: Coding guidelines
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
2 participants
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.
Summary
This PR adds a compile-time event-channel privacy policy that prevents
POSITION_APP,WAYPOINT_APP, andMAP_REPORT_APPpackets from being transmitted, relayed, acknowledged, logged, or forwarded to MQTT on channels whose effective key matchesUSERPREFS_CHANNEL_0_PSK. Enable it by settingUSERPREFS_BLOCK_POSITION_ON_EVENT_CHANNELtotrueinuserPrefs.jsonc; it defaults toUSERPREFS_EVENT_MODE, emits no replacement packet, and leaves private-channel and PKI traffic unaffected.USERPREFS_CHANNEL_0_PSK, independent of channel index or namePOSITION_APP,WAYPOINT_APP, andMAP_REPORT_APPon event channels across phone ingress, local send, receive, duplicate/reliable relay, ACK, logging, and MQTT pathsUSERPREFS_BLOCK_POSITION_ON_EVENT_CHANNELtoUSERPREFS_EVENT_MODE, while allowing an explicit overrideThe policy emits no replacement or empty position packet; blocked coordinate packets are not queued for transmission.
Testing
pio test -e coverage-event-policy -f test_position_precision— 23/23 passedpio test -e coverage -f test_position_precision— 23/23 passedpio run -e native— passedMESHTASTIC_EXCLUDE_TRACEROUTE=1— passedgit diff --check origin/develop...HEAD— cleanHardware validation
Equivalent policy behavior was exercised on two ThinkNode M2 devices using the earlier master-based implementation: event-channel position sends were suppressed, private-channel sends succeeded, inbound coordinate packets were not stored or relayed, retries produced no relay, and direct coordinate packets requesting ACK received no ACK/NAK.
This exact
developforward port has not been reflashed to hardware; its validation is the native test/build matrix above. Additional device regression testing is welcome.Scope
The Burning Mesh event configuration does not use ATAK or lost-and-found position reporting. This policy therefore covers the deployed coordinate-bearing port numbers listed above rather than changing those unrelated modules.
🤝 Attestations
Summary by CodeRabbit
New Features
Bug Fixes
Tests