TrafficManagement: gate role/NodeInfo cache writes on signer authenticity#11048
Conversation
…city The tier-3 role cache and the PSRAM NodeInfo response cache were updated from any received NodeInfo with no authenticity check, so a spoofed NodeInfo could set a node's cached role (granting dedup exceptions) or poison the cached user served in direct responses. Skip both cache writes when a known signer's NodeInfo arrives unsigned, matching the identity-update gate on the direct-response path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesNodeInfo authentication handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/modules/TrafficManagementModule.cpp (1)
735-736: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider deduplicating the
getMeshNodelookup.This exact
senderNodelookup andunauthenticatedSignercalculation are duplicated below at lines 756-757. SincegetMeshNodeinvolves an O(N) array traversal, you could optionally hoist this calculation to a higher scope to avoid performing the lookup twice forNODEINFO_APPpackets.🤖 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/TrafficManagementModule.cpp` around lines 735 - 736, Deduplicate the sender lookup by hoisting the getMeshNode call and unauthenticatedSigner calculation into the shared scope used by both paths in the relevant packet-handling function. Remove the repeated declarations near the NODEINFO_APP handling while preserving the existing senderNode and signature checks.
🤖 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.
Nitpick comments:
In `@src/modules/TrafficManagementModule.cpp`:
- Around line 735-736: Deduplicate the sender lookup by hoisting the getMeshNode
call and unauthenticatedSigner calculation into the shared scope used by both
paths in the relevant packet-handling function. Remove the repeated declarations
near the NODEINFO_APP handling while preserving the existing senderNode and
signature checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74301f7b-12cd-43ba-b6ed-26b2c508b426
📒 Files selected for processing (1)
src/modules/TrafficManagementModule.cpp
Compute the sender node lookup and unauthenticated-signer check once per NodeInfo packet and reuse it for both the cache-refresh gate and the direct-response identity gate, avoiding a second O(N) getMeshNode scan.
⚡ Try this PR in the Web Flasher
Warning This is an automated, unreviewed CI test build. Back up your device configuration Supported boards built by this PR (29)
Build artifacts expire on 2026-08-16. Updated for |
NomDeTom
left a comment
NomDeTom
left a comment
There was a problem hiding this comment.
It will do the job. There's a couple of wider testing and overall performance points which I'll pick up in a separate PR that builds on this work.
3 participants
The tier-3 role cache (
updateCachedRoleFromNodeInfo) and the PSRAM NodeInfo response cache (cacheNodeInfoPacket) were refreshed from any received NodeInfo without an authenticity check. A spoofed NodeInfo (the sender field is forgeable) could therefore:Both cache writes now use the same gate the identity-update path already applies: when a node known to sign its NodeInfo (
nodeInfoLiteHasXeddsaSigned) sends an unsigned NodeInfo, the update is skipped. Nodes that have never signed are unaffected. No overlap with the existing direct-responseupdateUsergate; that guards a separate sink.Summary by CodeRabbit