AdminModule: only accept admin responses to requests we sent

[caveman99]

Admin *_response messages skip both the sender-authorization branch and the
session-passkey check that gate every other admin message:

if (messageIsResponse(r)) {
    LOG_DEBUG("Allow admin response message");   // no auth, no session key
}
...
if (mp.from != 0 && !messageIsRequest(r) && !messageIsResponse(r)) { checkPassKey... }

The module is bound to no channel, so on any channel it can decode, a node can deliver
a get_module_config_response it never asked for. handleGetModuleConfigResponse then
writes the responder's node_num and pin list into devicestate.node_remote_hardware_pins.

Accept a response only from a node we sent the matching request to. MeshService::handleToRadio
now records the destination of each outgoing admin request (per remote, with the pinned PKC key
when the client set one), and the response branch checks for a live matching request within the
same window as the session passkey. Local admin (from == 0) is unchanged - PhoneAPI already
gates that connection.

Second, smaller fix in the same path: the response dispatch compared
get_module_config_response.which_payload_variant (a ModuleConfig oneof tag, remote_hardware = 9)
against meshtastic_AdminMessage_ModuleConfigType_REMOTEHARDWARE_CONFIG (an AdminMessage enum, = 8).
They never matched, so the handler was unreachable. It's compared against the oneof tag now, the
same way the request-side switch already builds the response.

Tests (test_admin_session_repro): the pairing gate directly - unsolicited response rejected,
solicited accepted, a request to one node does not admit another, and a setter does not arm the
gate - plus two end-to-end cases through handleReceivedProtobuf asserting the pin table is left
alone for an unsolicited response and updated for a solicited one (the latter also covering the
dispatch fix).

Note: touches AdminModule alongside the in-flight signing series (#10967, #10969, #10972); no
overlap on the response path, but they will need a trivial rebase around the switch.

Summary by CodeRabbit

• Bug Fixes

  • Tightened admin reply handling: admin responses are accepted only when they match a recently sent outgoing admin request (including sender, expected response type, and timing), preventing unsolicited/mismatched responses from altering remote hardware pin ownership.
  • Fixed recognition of remote hardware “module config” responses to use the correct variant tag.

• Tests

  • Added deterministic unit/integration tests validating solicited vs. unsolicited behavior, variant mismatches, and pin update outcomes for admin sessions.

• New Hardware Support

  • Added LoRa configuration profiles for ZebraHatDuo Radio variants, plus SD-card support for the relevant ESP32-S3 variant.

[coderabbitai]

📝 Walkthrough

Walkthrough

Admin requests are tracked by destination, timeout, response variant, and optional PKC key. Outgoing mesh handling records requests, while incoming remote responses are rejected unless solicited. Remote hardware response matching is corrected and covered by expanded tests. BLE restoration, board configuration, release metadata, and workflow updates are also included.

Changes

Admin response solicitation

Layer / File(s)	Summary
Outstanding request tracking src/modules/AdminModule.h, src/modules/AdminModule.cpp	Adds bounded request tracking with timeout eviction, destination and response-variant matching, and optional PKC key validation.
Mesh request and response flow src/mesh/MeshService.cpp, src/modules/AdminModule.cpp	Records outgoing admin getter requests before transmission, gates unsolicited responses, and uses the correct remote-hardware oneof tag.
Solicitation behavior validation test/support/AdminModuleTestShim.h, test/test_admin_session_repro/test_main.cpp	Adds packet builders and tests for unsolicited responses, matching requests, node isolation, variant mismatches, setters, and remote hardware pin updates.

Platform and release updates

Layer / File(s)	Summary
nRF52 Bluetooth state restoration src/platform/nrf52/NRF52Bluetooth.*	Extracts Bluetooth security and TX-power restoration helpers, reuses them when advertising resumes, and removes blocking pairing behavior.
Hardware and source configuration bin/config.d/*, variants/esp32s3/*, src/detect/ScanI2CTwoWire.h, src/graphics/draw/CompassRenderer.h, src/mesh/SX126xInterface.cpp	Adds ZebraHatDuo and SD-card configuration, initializes variant LEDs, adds an I2C default argument, removes an unused include, and relocates unchanged SX126x definitions.
Release and workflow metadata bin/org.meshtastic.meshtasticd.metainfo.xml, debian/changelog, .github/workflows/*	Adds release and Debian changelog entries and pins Trunk actions to version 1.3.1.

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

Sequence Diagram(s)

sequenceDiagram
  participant MeshService
  participant AdminModule
  participant Mesh
  MeshService->>AdminModule: Record outgoing admin request
  MeshService->>Mesh: Forward packet
  Mesh->>AdminModule: Deliver admin response
  AdminModule->>AdminModule: Validate solicitation and key
  AdminModule->>AdminModule: Apply remote hardware response

Loading

Possibly related issues

• meshtastic/design#123 — Adds the outstanding request/response correlation described by the issue.

Suggested labels: bug

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: restricting admin responses to requests we sent.
Description check	✅ Passed	The description is substantive and covers the bug, fix, and tests, but it omits the template's attestation checklist.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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 docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch admin-response-pairing

⚔️ Resolve merge conflicts

• Resolve merge conflict in branch admin-response-pairing

---

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.}

[coderabbitai]

Actionable comments posted: 2

🤖 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/modules/AdminModule.cpp`:
- Around line 1852-1853: Replace the raw sentAtSecs/millis-based expiry
arithmetic in the request response gate around slot->sentAtSecs and the related
lines 1864-1870 with the repository’s Throttle helper. Initialize and check
Throttle using elapsed-time semantics so the 300-second request expiry remains
correct across millis() rollover, while preserving the existing response
handling behavior.

In `@src/modules/AdminModule.h`:
- Around line 90-99: Update OutstandingAdminRequest and responseIsSolicited to
store and validate the expected response variant/tag alongside the destination
and key, so a request only authorizes its matching response type within the
existing timeout. Populate the expected tag when recording each getter request,
reject mismatched response tags, and add a test confirming an otherwise valid
response with the wrong variant is denied.

🪄 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: 8b8d9011-19d7-450e-bd7f-873840dc1d28

📥 Commits

Reviewing files that changed from the base of the PR and between 53a6b5e and 35f1cfe.

📒 Files selected for processing (5)

• src/mesh/MeshService.cpp
• src/modules/AdminModule.cpp
• src/modules/AdminModule.h
• test/support/AdminModuleTestShim.h
• test/test_admin_session_repro/test_main.cpp

[github-actions]

⚡ Try this PR in the Web Flasher

Note

Building this pull request… the flash button, badges and supported-board
list will appear here automatically once CI finishes.

[caveman99]

Both addressed in 21a8f96.

millis() rollover: the expiry now uses Throttle::isWithinTimespanMs(sentAtMs, 300000), which does the rollover-safe (now - sentAtMs) < window unsigned subtraction. The record stores raw millis() rather than millis()/1000 sums, so a request spanning the ~49.7-day wrap no longer reads as permanently live.

Response-type binding: each outstanding record now carries a bitmask of the response variants its requests authorize (adminResponseForRequest maps request tag to response tag, adminResponseBit maps that to a bit). A response is accepted only if its variant's bit is set, so a get_owner request no longer admits a get_module_config response from the same node. I used an accumulating per-remote mask rather than a single expected tag specifically to keep pipelined flows working: a client that fires several getter types at one node within the window still has every matching answer accepted. Added a test asserting the wrong variant is denied while the requested one from the same slot is still accepted.

[coderabbitai]

Caution

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

⚠️ Outside diff range comments (1)

src/modules/AdminModule.cpp (1)

1894-1911: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Fix rollover vulnerability in oldest-slot eviction and prevent resurrection of expired requests.

There are two logic issues in the slot allocation:

1. Rollover hazard: As per coding guidelines, raw timestamp comparisons like o.sentAtMs < slot->sentAtMs are vulnerable to millis() rollover. When millis() wraps, newer timestamps appear smaller than older pre-wrap timestamps, causing the newest pending request to be evicted instead of the oldest. Compare elapsed time instead.
2. Resurrection of expired requests: If an existing slot for the node is found but has already expired, sameNode remains true. This ORs the new request bit with the expired expectedResponses mask, resetting the 5-minute timer and unintentionally resurrecting previously expired request types.

♻️ Proposed fix

-    // Reuse this remote's slot if it has one, else a free slot, else the oldest.
-    OutstandingAdminRequest *slot = nullptr;
-    for (auto &o : outstandingAdminRequests)
-        if (o.to == p.to)
-            slot = &o;
-    const bool sameNode = slot != nullptr;
-    if (!slot)
-        for (auto &o : outstandingAdminRequests)
-            if (o.to == 0) {
-                slot = &o;
-                break;
-            }
-    if (!slot) {
-        slot = &outstandingAdminRequests[0];
-        for (auto &o : outstandingAdminRequests)
-            if (o.sentAtMs < slot->sentAtMs)
-                slot = &o;
-    }
+    // Reuse this remote's slot if it has one, else a free slot, else the oldest.
+    OutstandingAdminRequest *slot = nullptr;
+    for (auto &o : outstandingAdminRequests) {
+        if (o.to == p.to) {
+            slot = &o;
+            break;
+        }
+    }
+
+    bool sameNode = slot != nullptr;
+    if (sameNode && !Throttle::isWithinTimespanMs(slot->sentAtMs, kOutstandingAdminRequestMs)) {
+        sameNode = false; // Expired; do not inherit stale expected responses
+    }
+
+    if (!slot) {
+        for (auto &o : outstandingAdminRequests) {
+            if (o.to == 0) {
+                slot = &o;
+                break;
+            }
+        }
+    }
+    if (!slot) {
+        slot = &outstandingAdminRequests[0];
+        uint32_t now = millis();
+        for (auto &o : outstandingAdminRequests) {
+            if ((uint32_t)(now - o.sentAtMs) > (uint32_t)(now - slot->sentAtMs)) {
+                slot = &o;
+            }
+        }
+    }

🤖 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/modules/AdminModule.cpp` around lines 1894 - 1911, Update the slot
allocation logic around outstandingAdminRequests to select the oldest slot using
rollover-safe elapsed-time comparisons rather than raw sentAtMs ordering. When
reusing a matching node slot, detect whether it has expired and clear/reset that
slot’s prior request state so sameNode is false for the new request, preventing
expired response bits from being resurrected or extending their timer.

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 `@src/modules/AdminModule.cpp`:
- Around line 1894-1911: Update the slot allocation logic around
outstandingAdminRequests to select the oldest slot using rollover-safe
elapsed-time comparisons rather than raw sentAtMs ordering. When reusing a
matching node slot, detect whether it has expired and clear/reset that slot’s
prior request state so sameNode is false for the new request, preventing expired
response bits from being resurrected or extending their timer.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0efc028a-9f2e-4bec-a17e-c0c4be8fb11b

📥 Commits

Reviewing files that changed from the base of the PR and between 35f1cfe and 21a8f96.

📒 Files selected for processing (3)

• src/modules/AdminModule.cpp
• src/modules/AdminModule.h
• test/test_admin_session_repro/test_main.cpp

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

• src/modules/AdminModule.h
• test/test_admin_session_repro/test_main.cpp

[jp-bennett]

Definitely approve of the concept

[thebentern]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Done — merged origin/master (62df860) into the branch in c926cae. All conflicts were in files outside of the AdminModule/MeshService changes; those resolved cleanly by keeping the develop-based state. The AdminModule, MeshService, and test files had no conflicts and are unchanged.

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@bin/config.d/lora-ZebraHatDuo_R1_1W.yaml`:
- Around line 4-6: Correct the Meta.name value in the ZebraHatDuo_R1_1W
configuration from “Radio 0” to “Radio 1,” while leaving the other metadata
unchanged.

In `@variants/esp32s3/ELECROW-ThinkNode-M7/variant.cpp`:
- Around line 12-18: Remove the duplicate initVariant definition in
variants/esp32s3/ELECROW-ThinkNode-M7/variant.cpp, retaining the original
implementation. Also remove the duplicate getCurrentRSSI and enableInterrupt
definitions in src/mesh/SX126xInterface.cpp, preserving each original
implementation so every function has a single definition.

🪄 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: dfb4b76b-a733-4d87-8f26-eeb12c2e84b5

📥 Commits

Reviewing files that changed from the base of the PR and between 21a8f96 and c926cae.

📒 Files selected for processing (14)

• .github/workflows/nightly.yml
• .github/workflows/trunk_annotate_pr.yml
• .github/workflows/trunk_check.yml
• bin/config.d/lora-ZebraHatDuo_R0_1W.yaml
• bin/config.d/lora-ZebraHatDuo_R1_1W.yaml
• bin/org.meshtastic.meshtasticd.metainfo.xml
• debian/changelog
• src/detect/ScanI2CTwoWire.h
• src/graphics/draw/CompassRenderer.h
• src/mesh/SX126xInterface.cpp
• src/platform/nrf52/NRF52Bluetooth.cpp
• src/platform/nrf52/NRF52Bluetooth.h
• variants/esp32s3/ELECROW-ThinkNode-M7/variant.cpp
• variants/esp32s3/m5stack_cardputer_adv/variant.h

💤 Files with no reviewable changes (1)

• src/graphics/draw/CompassRenderer.h

[thebentern]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

The branch is already up to date with origin/master — the merge was completed in c926cae from the previous request. git merge-base HEAD origin/master returns 62df860 (master's current HEAD), so there's nothing left to resolve.