Conversation
…from
sendRoutingErrorResponse() passes mp->from straight through as the NAK's
destination. Every other ack/nak call site in the tree uses getFrom(), which
resolves the "unset" sentinel 0 to our own node number.
The single caller is PhoneAPI's 2-second TEXT_MESSAGE_APP rate limit
(PhoneAPI.cpp:1865). That path returns false before MeshService::handleToRadio()
reaches its `p.from = 0` normalisation, so `from` is whatever the client put
there. A client that leaves it unset gets:
- to == 0 on the NAK, so isToUs() is false and sendLocal() does not deliver
it to the phone - the client receives no ROUTING_APP response at all for
the message it just had rejected, only a QueueStatus;
- the NAK encrypted and transmitted over LoRa addressed to node 0.
Both current first-party clients do set `from` (Android uses myNodeNum with a
0 fallback, Apple sets deviceNum outside its Lockdown path), so this is
reachable via an early-connection race before myNodeNum is known, Apple's
Lockdown path, and third-party clients.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesRouting error response handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The routing-error destination change is covered by native tests for unset, explicit, and null packet cases, with no unresolved merge risk identified. 🚥 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 |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
6f965b6 to
d5272e9
Compare
RCGV1
left a comment
RCGV1
left a comment
There was a problem hiding this comment.
I independently traced the unset-from rate-limit path and agree with this fix. sendRoutingErrorResponse() is reached before handleToRadio() normalizes phone-originated packets, so using mp->from can address the NAK to node 0. getFrom(mp) is the correct normalization and matches the other ACK/NAK call sites.
I also exercised a focused native regression that constructs a phone request with from = 0, sends RATE_LIMIT_EXCEEDED, and asserts the generated ACK/NAK targets the local node while preserving the request ID and channel. It passes on this PR head; the complete test_mesh_module suite passed 25/25.
I recommend adding that regression to this PR so the pre-normalization behavior stays covered. The essential assertion is that ackNaks[0].to == LOCAL_NODE after calling sendRoutingErrorResponse(..., &request) with request.from = 0.
sendRoutingErrorResponse() had no native coverage before or after the fix, so nothing held the getFrom() call in place. test_routingErrorResponse_unsetFromAddressesUs builds the phone request the 2 second TEXT_MESSAGE_APP rate limit rejects, with from left unset as a client may send it and as handleToRadio() has not yet normalised it, and asserts the NAK is addressed to us rather than to node 0. It also pins the request id and channel, which are what let the client match the verdict to the message it just sent. Two more hold the surrounding contract: a request that already carries a sender is passed through untouched, since getFrom() only rewrites 0, and a null packet generates no NAK at all. Reverting the fix to mp->from fails the first with "Expected 286331153 Was 0", 0x11111111 being the harness's local node. The other two pass either way, which is the intent: they describe behaviour the fix does not change. Regression suggested by RCGV1, who traced the same path independently.
|
Thanks, that is a useful review, and the regression is in as Your reading of the ordering is right, and the assertion you named is the one that discriminates. Reverting the fix to I added the id and channel assertions you mentioned for the same reason: those are what let the client match the verdict to the message it just sent, so a NAK that arrives correctly addressed but unmatchable would still leave the client without an answer. Two more went in alongside it, to state the parts the fix does not change:
Both pass with or without the fix, which is deliberate: they describe the surrounding contract, and only the unset-
One correction to the PR description, which I will update: it says there is no native coverage for this path either before or after, which was true when I wrote it and is now only true of "before". |
2 participants
MeshService::sendRoutingErrorResponse()passesmp->fromstraight through as the NAK destination.Every other ack/nak call site uses
getFrom(), which turns the unset sentinel 0 into our own nodenumber.
The only caller is the 2 second
TEXT_MESSAGE_APPrate limit atPhoneAPI.cpp:1865. That pathreturns false before
MeshService::handleToRadio()reaches itsp.from = 0normalisation, sofromis whatever the client put in the ToRadio packet.If the client left it unset,
toon the NAK is 0.isToUs(0)is false soRouter::sendLocal()does not deliver it to the phone, and the client gets no
ROUTING_APPresponse at all for themessage that was just rejected, only a
QueueStatus. The NAK is instead encrypted and transmittedover LoRa addressed to node 0. It goes out with the default
hopLimitof 0 so nothing rebroadcastsit, which keeps the cost to one wasted transmission plus a client left without a verdict.
Since
getFrom()only rewrites 0, none of this shows up when the client setsfromto its own nodenumber, which both first party clients normally do. On Android
CommandSenderImpl.ktusesfrom = nodeManager.myNodeNum.value ?: 0, so it degrades to 0 during the window beforemyNodeNumis populated. I have not reproduced that: it needs two text sends inside the 2 second window during
startup. The convention of leaving
fromunset is real and used by first party code, for instanceAccessoryManager+Lockdown.swiftcarries the comment "meshPacket.from intentionally NOT set. Protodefault 0 means firmware...". That path does not send text so it cannot trigger this rate limit, but
it does show unset
fromis expected rather than malformed.Fix is
getFrom(mp), matching the other call sites. No behaviour change whenfromis already set.sendRoutingErrorResponse()had no native coverage before this change;test_mesh_modulenowcarries three cases for it.
test_routingErrorResponse_unsetFromAddressesUsis the one that holdsthe fix in place: it builds the request the rate limit rejects with
fromunset and asserts the NAKis addressed to us rather than node 0, plus the request id and channel the client needs to match the
verdict to its message. Reverting to
mp->fromfails it withExpected 286331153 Was 0.The other two state what the fix does not change, and pass either way: a sender that is already set
is passed through untouched, and a null packet generates no NAK. Suite is 27/27.
Testing: builds clean on
heltec-v4andseeed-xiao-s3, targeted native suites unchanged. Notexercised on hardware, since the CLI sets
fromand I could not construct the failing case with it.🤖 Generated with Claude Code
🤝 Attestations
Summary by CodeRabbit