Block coordinate traffic on configured event channels

[ayysasha]

Summary

This PR adds an opt-in, compile-time event-channel privacy policy that prevents POSITION_APP, WAYPOINT_APP, and MAP_REPORT_APP packets from being transmitted, relayed, acknowledged, logged, or forwarded to MQTT on channels whose effective key matches USERPREFS_CHANNEL_0_PSK.

Enable it explicitly in userPrefs.jsonc:

"USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL": "1"

The policy defaults off, emits no replacement or empty position packet, and leaves private-channel and eligible PKI traffic unaffected.

• Identifies configured event channels by matching their full effective AES key against USERPREFS_CHANNEL_0_PSK, independent of channel index or name.
• Suppresses event-channel coordinates across phone ingress, local send, receive, duplicate/reliable relay, acknowledgments, payload logging, and MQTT paths.
• Prevents blocked attempts from consuming retry, duplicate-history, or per-port cooldown state.
• Leaves behavior unchanged when USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL is undefined or 0.

Testing

• Policy-enabled behavioral matrix: 128/128 tests passed.
• Policy-disabled behavioral matrix: 128/128 tests passed.
• pio run -e native: passed.
• Trunk checks on changed files: passed.
• git diff --check: clean.

Hardware 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 acknowledgment received no ACK/NAK.

The exact develop forward port was validated through the native unit-test and build matrices rather than reflashed to hardware.

Scope

The Burning Mesh event configuration does not use ATAK or lost-and-found position reporting. This policy covers POSITION_APP, WAYPOINT_APP, and MAP_REPORT_APP; the generated MapReport path is MQTT-only, while classifying port 73 also prevents generic packet ingress from routing a manually constructed map report over LoRa.

Attestations

• I have tested that my proposed changes behave as described.
• I have tested that my proposed changes do not cause any obvious regressions on all listed reference devices.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds configurable event-channel coordinate privacy enforcement across mesh routing, phone ingress, reliable delivery, precision handling, MQTT reporting, and next-hop forwarding. Native tests and a dedicated PlatformIO coverage workflow validate the policy.

Changes

Event channel policy

Layer / File(s)	Summary
Policy contracts and core routing src/mesh/Channels.*, src/mesh/Router.*, src/mesh/PositionPrecision.cpp	Defines event-channel identity and coordinate classification, derives effective channels, and suppresses blocked coordinate transmission, decoding, reception, and precision.
Routing and external delivery gates src/mesh/NextHopRouter.cpp, src/mesh/ReliableRouter.cpp, src/mesh/PhoneAPI.cpp, src/mqtt/MQTT.cpp	Prevents blocked coordinates from rebroadcasting, reliable delivery, phone forwarding, MQTT forwarding, and map publication.
Router enforcement and lifecycle tests test/test_event_channel_router/*	Tests event/private channel behavior, PKI exceptions, opaque packets, reception, transmission, and packet-pool ownership.
Phone, precision, and MQTT validation test/test_event_channel_phone_api/*, test/test_position_precision/*, test/test_mqtt/MQTT.cpp	Adds retry-state, precision, position-publication, and map-reporting coverage.
Next-hop and reliable-routing validation test/test_nexthop_routing/*	Tests suppression across flooding, duplicates, retries, ACK/NAK handling, and reliable pending state.
Policy build and test wiring userPrefs.jsonc, variants/native/portduino/platformio.ini, .github/workflows/test_native.yml, test/native-suite-count	Adds preference documentation, the event-policy test environment, JUnit artifact collection, and updated suite count.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PhoneAPI
  participant Router
  participant Channels
  participant NextHopRouter
  participant MQTT
  PhoneAPI->>Router: submit coordinate packet
  Router->>Channels: resolve effective channel and event-channel identity
  Channels-->>Router: policy result
  Router->>NextHopRouter: allow or suppress routing
  Router->>MQTT: allow or suppress reporting

Loading

Possibly related PRs

• meshtastic/firmware#9749: Both use Channels::getKey and channel PSK handling.
• meshtastic/firmware#11322: Both modify native test infrastructure and overlapping MQTT test setup.

Suggested reviewers: jp-bennett, vidplace7, thebentern

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: blocking coordinate traffic on configured event channels.
Description check	✅ Passed	The description covers the implementation, scope, testing, hardware validation limits, and required attestations.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[ayysasha]

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?

[ayysasha]

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.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/mesh/FloodingRouter.cpp (1)

95-104: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Restrict policy-driven decoding to event-channel candidates.

Setting shouldDecode = true decrypts 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 win

Keep 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

📥 Commits

Reviewing files that changed from the base of the PR and between cfecef5 and 470c40b.

📒 Files selected for processing (20)

• .github/workflows/test_native.yml
• src/mesh/Channels.cpp
• src/mesh/Channels.h
• src/mesh/FloodingRouter.cpp
• src/mesh/FloodingRouter.h
• src/mesh/NextHopRouter.cpp
• src/mesh/PhoneAPI.cpp
• src/mesh/PositionPrecision.cpp
• src/mesh/ReliableRouter.cpp
• src/mesh/Router.cpp
• src/mesh/Router.h
• src/mqtt/MQTT.cpp
• test/native-suite-count
• test/test_event_channel_phone_api/test_main.cpp
• test/test_event_channel_router/test_main.cpp
• test/test_mqtt/MQTT.cpp
• test/test_nexthop_routing/test_main.cpp
• test/test_position_precision/test_main.cpp
• userPrefs.jsonc
• variants/native/portduino/platformio.ini

[thebentern]

@ayysasha can you rebase this

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

.github/workflows/test_native.yml (1)

39-40: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Reject count files containing multiple tokens.

tr -d '[:space:]' collapses values such as 1 2 or two numeric lines into 12, allowing malformed files to pass the guard. Validate the raw content as exactly one integer with optional surrounding whitespace.

Proposed fix

-          canonical_count=$(tr -d '[:space:]' <"$count_file")
-          if ! [[ $canonical_count =~ ^[0-9]+$ ]]; then
+          raw_count=$(<"$count_file")
+          if ! [[ $raw_count =~ ^[[:space:]]*([0-9]+)[[:space:]]*$ ]]; then
             echo "::error title=Invalid native-suite-count::$count_file must contain a single integer, got '$canonical_count'."
             exit 1
           fi
+          canonical_count="${BASH_REMATCH[1]}"

🤖 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 @.github/workflows/test_native.yml around lines 39 - 40, Update the
count-file validation around canonical_count to validate the raw file content as
exactly one non-negative integer with optional surrounding whitespace, rather
than removing all whitespace before matching. Ensure values containing multiple
numeric tokens, such as “1 2” or separate numeric lines, are rejected while
preserving acceptance of a single integer surrounded by whitespace.

🧹 Nitpick comments (2)

.github/workflows/test_native.yml (1)

270-270: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Disable credential persistence for the report job.

This checkout leaves GITHUB_TOKEN in .git/config while later steps execute repository tooling and process artifacts. Add persist-credentials: false to avoid unnecessary token exposure.

Proposed fix

       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
+        with:
+          persist-credentials: false

🤖 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 @.github/workflows/test_native.yml at line 270, Update the actions/checkout
step in the report job to set persist-credentials to false, ensuring the
checkout does not retain GITHUB_TOKEN in .git/config for subsequent tooling and
artifact-processing steps.

Source: Linters/SAST tools

src/mesh/Router.cpp (1)

579-633: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Multiple new comment blocks exceed the repo's comment-length convention.

Several newly added comments run 3-8 lines (e.g. the canonicalSignableSize header at 579-583, the Balanced-mode rationale at 720-727, the admin-key persistence note at 932-936), restating rationale in multi-paragraph form. As per coding guidelines, "Keep code comments minimal—one or two lines maximum—and comment only when the reason is not obvious; do not restate straightforward code or add multi-paragraph explanatory blocks."

Also applies to: 720-727, 932-937

🤖 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/Router.cpp` around lines 579 - 633, The newly added comments around
canonicalSignableSize, the Balanced-mode logic, and admin-key persistence exceed
the repository’s one- or two-line comment convention. Condense each comment
block to at most two lines, retaining only non-obvious rationale and removing
restatements of the surrounding code.

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.

Outside diff comments:
In @.github/workflows/test_native.yml:
- Around line 39-40: Update the count-file validation around canonical_count to
validate the raw file content as exactly one non-negative integer with optional
surrounding whitespace, rather than removing all whitespace before matching.
Ensure values containing multiple numeric tokens, such as “1 2” or separate
numeric lines, are rejected while preserving acceptance of a single integer
surrounded by whitespace.

---

Nitpick comments:
In @.github/workflows/test_native.yml:
- Line 270: Update the actions/checkout step in the report job to set
persist-credentials to false, ensuring the checkout does not retain GITHUB_TOKEN
in .git/config for subsequent tooling and artifact-processing steps.

In `@src/mesh/Router.cpp`:
- Around line 579-633: The newly added comments around canonicalSignableSize,
the Balanced-mode logic, and admin-key persistence exceed the repository’s one-
or two-line comment convention. Condense each comment block to at most two
lines, retaining only non-obvious rationale and removing restatements of the
surrounding code.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c1161fcd-0926-4393-9e1b-f2bb5aa7a5d2

📥 Commits

Reviewing files that changed from the base of the PR and between 470c40b and 6d8943a.

📒 Files selected for processing (12)

• .github/workflows/test_native.yml
• src/mesh/NextHopRouter.cpp
• src/mesh/PhoneAPI.cpp
• src/mesh/Router.cpp
• src/mesh/Router.h
• src/mqtt/MQTT.cpp
• test/native-suite-count
• test/test_event_channel_router/test_main.cpp
• test/test_mqtt/MQTT.cpp
• test/test_nexthop_routing/test_main.cpp
• userPrefs.jsonc
• variants/native/portduino/platformio.ini

🚧 Files skipped from review as they are similar to previous changes (10)

• test/native-suite-count
• userPrefs.jsonc
• src/mesh/Router.h
• src/mqtt/MQTT.cpp
• src/mesh/PhoneAPI.cpp
• variants/native/portduino/platformio.ini
• test/test_mqtt/MQTT.cpp
• test/test_event_channel_router/test_main.cpp
• src/mesh/NextHopRouter.cpp
• test/test_nexthop_routing/test_main.cpp

[ayysasha]

Why MAP_REPORT_APP is included

The firmware-generated MapReport path is MQTT-only: MQTT::perhapsReportToMap() wraps the report in a ServiceEnvelope and publishes it directly to the map topic; it does not call the mesh Router or transmit over LoRa. The MeshPacket with a broadcast destination is part of the envelope's protobuf data shape, not evidence of RF transmission.

We included port 73 for two separate reasons. First, the generic phone and MQTT packet-ingress APIs can still construct a decoded MAP_REPORT_APP packet and pass it to the Router, so classifying it as coordinate-bearing prevents a manually injected port-73 packet from reaching LoRa on the configured event channel. Second, we interpreted the event privacy policy as preventing location egress through MQTT as well as LoRa, so the internally generated direct map report is independently suppressed when the primary channel uses the event key.

If maintainers want the scope to be strictly LoRa airtime, these layers can be separated: retain the Router-side defensive classification while removing the direct perhapsReportToMap() MQTT suppression. The current implementation intentionally takes the broader privacy-oriented interpretation.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (1)

test/test_position_precision/test_main.cpp (1)

274-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Reduce these comments to two lines.

Each new comment has three lines. Keep each comment to one or two lines.

As per coding guidelines, “Keep code comments minimal—normally one or two lines.”

Also applies to: 408-410

🤖 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 comments near the event-channel precision checks, including the
corresponding comment around the later referenced section, to no more than two
lines each while preserving their essential intent.

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/Router.cpp`:
- Around line 1498-1505: The blocked coordinate-packet check must run before
Router logging in dispatchReceived(), immediately after successful decoding and
before printPacket(). Move the isBlockedEventCoordinatePacket() handling there
while preserving the LOG_DEBUG, cancelSending(), and skipHandle behavior.

In `@test/test_event_channel_phone_api/test_main.cpp`:
- Around line 89-104: Extend GlobalState and the setup/teardown flow around
MockRouter to save both cryptLock and myNodeInfo, set cryptLock to nullptr
before constructing MockRouter, and restore the saved cryptLock and node info
during teardown alongside the existing global state restoration.

In `@test/test_nexthop_routing/test_main.cpp`:
- Around line 302-310: Replace the unbounded strcpy call in makeBehaviorChannel
with the bounded string-copy helper from src/meshUtils.h, ensuring
channel.settings.name is always null-terminated and overlong names are safely
truncated.

---

Nitpick comments:
In `@test/test_position_precision/test_main.cpp`:
- Around line 274-276: Shorten the comments near the event-channel precision
checks, including the corresponding comment around the later referenced section,
to no more than two lines each while preserving their essential intent.

🪄 Autofix

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: f739c865-7056-444f-8caa-fb19f6e4c0e6

📥 Commits

Reviewing files that changed from the base of the PR and between 5692518 and a50bfc1.

📒 Files selected for processing (18)

• .github/workflows/test_native.yml
• src/mesh/Channels.cpp
• src/mesh/Channels.h
• src/mesh/NextHopRouter.cpp
• src/mesh/PhoneAPI.cpp
• src/mesh/PositionPrecision.cpp
• src/mesh/ReliableRouter.cpp
• src/mesh/Router.cpp
• src/mesh/Router.h
• src/mqtt/MQTT.cpp
• test/native-suite-count
• test/test_event_channel_phone_api/test_main.cpp
• test/test_event_channel_router/test_main.cpp
• test/test_mqtt/MQTT.cpp
• test/test_nexthop_routing/test_main.cpp
• test/test_position_precision/test_main.cpp
• userPrefs.jsonc
• variants/native/portduino/platformio.ini

🚧 Files skipped from review as they are similar to previous changes (12)

• test/native-suite-count
• variants/native/portduino/platformio.ini
• userPrefs.jsonc
• src/mesh/PositionPrecision.cpp
• src/mesh/NextHopRouter.cpp
• src/mesh/Channels.cpp
• src/mesh/Channels.h
• src/mqtt/MQTT.cpp
• src/mesh/PhoneAPI.cpp
• src/mesh/ReliableRouter.cpp
• .github/workflows/test_native.yml
• src/mesh/Router.h

[jp-bennett]

Have you thought through whether this could cause problems with emergency response? This is always my hesitation with blocking precise position: the case where somebody needs help, and location over the mesh would get help there faster.