fix(NodeDB): stop addFromContact() erasing a stored public key - #11432
Conversation
Clients send `add_contact` before every text-message DM, because a phone often holds a larger contact database (with public keys) than the radio can keep. That makes `addFromContact()` the highest-volume key-write path on the device - and it had no protection against key erasure. Its only key guard covered the manually-verified case: if the local entry was marked manually verified and the incoming contact was not, a key mismatch aborted the update. Every ordinary entry fell straight through to `CopyUserToNodeInfoLite()`, which assigns `public_key` unconditionally. So a SharedContact with `has_user` set and an empty `public_key` overwrote a peer's stored, XEdDSA-proven key with zeros - and `addFromContact()` calls `saveNodeDatabaseToDisk()`, so the erasure survived a reboot. Subsequent DMs to that peer then failed with PKI_SEND_FAIL_PUBLIC_KEY, with no way to recover until the peer's NodeInfo was re-exchanged. `public_key` is a singular (non-optional) bytes field, so "absent" and "empty" both decode to size 0; a client that simply has no key for a contact is indistinguishable on the wire from one asking to clear it. The fix is deliberately narrow: keep the stored key when the entry already holds a full 32-byte key and the incoming contact does not. A well-formed 32-byte contact key still updates the entry exactly as before. Deliberately NOT changed here: - `updateUser()`'s first-key-wins pin is not applied to this path. Clients legitimately use add_contact to supply keys the radio never had and to update them (QR-code contact sharing); a blanket pin would break that documented flow. Only erasure is refused. - `CopyUserToNodeInfoLite()` itself is untouched - it has many other callers (self-record refresh, updateUser, warm-tier rehydration), so the guard lives at this call site. - The manually-verified branch is unchanged. - Node-number validation (reserved/broadcast/self) on this path remains open and is tracked separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesContact key preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/mesh/NodeDB.cpp (3)
3491-3493: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a lower log level for the expected keyless-contact path.
Clients send
add_contactbefore text-message DMs. This branch can therefore emit oneLOG_INFOrecord per DM. UseLOG_DEBUGso expected traffic does not flood INFO logs or add unnecessary serial I/O.Proposed fix
- LOG_INFO("Contact 0x%08x has no key, keep the stored one", contact.node_num); + LOG_DEBUG("Contact 0x%08x has no key, keep the stored one", contact.node_num);🤖 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/NodeDB.cpp` around lines 3491 - 3493, Change the logging call in the stored-key assignment branch to LOG_DEBUG instead of LOG_INFO, while preserving the existing message and key update behavior.
3489-3494: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd regression coverage for both key-preservation branches.
Test that a stored 32-byte key survives incoming key sizes
0and a short value. Also test that an incoming 32-byte key replaces the stored key, including when no key was stored previously. Assert bothbytesandsize.The PR objectives require preserving incomplete keys while retaining legitimate key insertion and replacement. Native tests were not performed locally, so verify these cases before merge.
🤖 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/NodeDB.cpp` around lines 3489 - 3494, 1. Add regression tests covering the key-preservation logic around CopyUserToNodeInfoLite: verify a stored 32-byte key remains unchanged for incoming key sizes 0 and short values, asserting both bytes and size. 2. Verify an incoming 32-byte key replaces the stored key and is inserted when no key was previously stored, also asserting bytes and size. 3. Ensure the tests exercise both branches of the NodeInfoLite public-key handling.
3485-3488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the added comment to two lines.
This four-line block exceeds the repository limit for code comments. Keep only the key preservation rationale.
Proposed comment
- // CopyUserToNodeInfoLite assigns public_key unconditionally, and clients send add_contact before every - // DM - often from an entry that carries no key at all. A contact may still supply or update a full - // 32-byte key (that's what add_contact is for), but it must never *erase* a key we already hold, which - // would be persisted below and break subsequent DMs with PKI_SEND_FAIL_PUBLIC_KEY. + // CopyUserToNodeInfoLite overwrites public_key unconditionally. + // Preserve a stored 32-byte key when the contact has no complete key.As per coding guidelines, keep code comments minimal—normally one or two lines—and comment only when the rationale is not obvious.
🤖 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/NodeDB.cpp` around lines 3485 - 3488, Shorten the comment above the relevant key-handling logic to two lines, retaining only the rationale that add_contact entries may omit a key and must not erase an existing public key. Remove the client-flow and failure-code details while preserving the implementation unchanged.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.
Nitpick comments:
In `@src/mesh/NodeDB.cpp`:
- Around line 3491-3493: Change the logging call in the stored-key assignment
branch to LOG_DEBUG instead of LOG_INFO, while preserving the existing message
and key update behavior.
- Around line 3489-3494: 1. Add regression tests covering the key-preservation
logic around CopyUserToNodeInfoLite: verify a stored 32-byte key remains
unchanged for incoming key sizes 0 and short values, asserting both bytes and
size. 2. Verify an incoming 32-byte key replaces the stored key and is inserted
when no key was previously stored, also asserting bytes and size. 3. Ensure the
tests exercise both branches of the NodeInfoLite public-key handling.
- Around line 3485-3488: Shorten the comment above the relevant key-handling
logic to two lines, retaining only the rationale that add_contact entries may
omit a key and must not erase an existing public key. Remove the client-flow and
failure-code details while preserving the implementation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a09e8d66-5075-47a7-a396-d7607d9ac04a
📒 Files selected for processing (1)
src/mesh/NodeDB.cpp
579d26e
|
Thanks for merging — flagging that both review items landed on the branch after the merge, so they are not in
Both are now in #11443, cherry-picked onto current |
2 participants
addFromContact()reachesTypeConversions::CopyUserToNodeInfoLite(), which assignspublic_keyunconditionally. So aSharedContactwithhas_userset and an emptypublic_keyerases a peer's stored, XEdDSA-proven key — and the erasure is persisted. Subsequent DMs to that peer then fail withPKI_SEND_FAIL_PUBLIC_KEY.Per the comment in the same function, clients send
add_contactbefore every text-message DM, making this the highest-volume key-write path on the device and the only one with no protection against erasure.Fix: refuse only the erasure. If the entry already holds a 32-byte key and the incoming contact carries an empty/short one, keep the stored key.
Deliberately NOT changed
updateUser()'s refuse-on-mismatch is not replicated here.add_contactexists to supply keys the radio never had and to update them (QR contact sharing); a blanket pin would break that documented flow. A valid 32-byte contact key still replaces a stored one exactly as today.CopyUserToNodeInfoLiteuntouched. It has several other callers; the guard is at theaddFromContactcall site only.Is an empty key ever an intentional "clear"?
No evidence for it, and the field shape argues against it:
public_keyis a singular, non-optional bytes field, so "absent" and "explicitly empty" are indistinguishable on the wire — a client that merely lacks a key looks identical to one requesting a clear. The one intentional key-clear that did exist on this path (theshould_ignorebranch) was removed in #10705 and replaced with a comment stating an ignored peer keeps a usable identity.test_traffic_managementasserts the same merge rule elsewhere: a keyless identity must not erase a key already learned.Validation
heltec-v4(ESP32-S3) against a clean baseline build of the same environment.heltec-v4andseeed-xiao-s3.bin/run-tests.shrefuses off-Linux) andbin/test-native-docker.shneeds Docker, which was unavailable on the dev host. Relying on CI for the native suite.Found during an adversarial review of deriving
NodeNumfrom the node public key. Filed as a draft for maintainer judgement.🤖 Generated with Claude Code
Summary by CodeRabbit
Integration test on hardware
This fix was included in an integration branch of all 11 review fixes (merged with no conflicts) and flashed to two boards from erased flash:
2.8.0.684f6b1Result: both booted, LoRa init OK, region set applied, config persisted across power-cycle, and the two nodes discovered and verified each other over the air.
region UNSET, MAC-derived node num0x1dd29d300xb29fb324my_node_num == crc32(public_key)0x4dc9fb0f✔0xd71bb46a✔Every config write in that sequence goes through
SafeFile→saveProto(), and all of them succeeded, persisted across reboot, and triggered no spuriousfsFormat().Note
The integration run above is a regression smoke test — it proves this change does not break normal operation on real hardware. It does not exercise the specific defect fixed here, which needs a condition that cannot be induced with two bench nodes. That part remains verified by code inspection and, once CI runs, by the native suite.