feat(nodes): add Meshtastic contact QR and URL sharing - #4327
Conversation
Yeraze
left a comment
There was a problem hiding this comment.
Review
Thanks for this — the engineering quality here is high. The byte-exact golden fixture against a real client-generated URL is exactly the right way to validate wire-format work, the route test uses the createRouteTestApp harness with real permission SQL (as CLAUDE.md requires), per-source isolation is covered, and the stale-response cancellation is both implemented and tested. UiIcon used, no raw fetch, no new dependencies, no migrations.
Findings below, verified against the current main.
Should fix
1. The permission gate is not source-scoped — cross-source read leak
nodesRoutes.ts uses bare requirePermission('nodes', 'read') with no sourceIdFrom, then reads sourceId from the query. With sourceIdFrom absent, scopedSourceId stays undefined (authMiddleware.ts:362), and checkPermissionAsync documents that path as "Without sourceId → union across sources (legacy callers that don't scope their lookup)" (database.ts:4358).
So a user holding nodes:read on source B passes the gate when requesting a node from source A.
The second gate, checkNodeChannelAccess(node.nodeId, req.user, sourceId), is correctly source-scoped and narrows this — but it doesn't close it. A user with channel_0:viewOnMap on source A and nodes:read only on source B still gets a contact URL for a source A node.
requirePermission('nodes', 'read', { sourceIdFrom: 'query', requireSourceId: true })This is the same class of bug #3745 fixed ("forward sourceId everywhere"). The adjacent /nodes/:nodeNum/copy-candidates route has the same unscoped shape, so it is a pre-existing pattern in this file — but I'd treat that as a reason to get it right in a new route rather than inherit it.
The tests only grant on sourceA, so this isn't currently pinned. A negative case would catch it: grant nodes:read on B plus channel_0:viewOnMap on A, request A, expect 403.
2. Handler bypasses the shared response envelope
CLAUDE.md: "New or modified handlers must use these" (ok/fail from src/server/utils/apiResponse.ts). This handler hand-rolls res.json({ success: true, data: { url } }) and res.status(400).json({ error, code }).
ok(res, { url }) emits byte-identical output and fail(res, 400, 'INVALID_NODE_NUM', 'Invalid nodeNum') likewise, so this is a pure drop-in with no consumer impact. (nodesRoutes.ts has zero adoption of the helpers today, so this is greenfield in that file.)
3. New component uses a global stylesheet rather than a CSS module
Per the CSS containment rule (#3962 Task 5.6), MeshtasticContactShare.css should be MeshtasticContactShare.module.css scoped to the component. Also, height: auto !important on .node-contact-share-canvas is fighting the inline dimensions QRCode.toCanvas writes onto the element — if it stays, a comment explaining that would help the next reader.
Worth calling out
4. createAddContactMessage silently gains validation it never had
Routing the existing admin path through buildSharedContactPayload adds two throw conditions that did not previously exist: the nodeNum range check, and nodeId must equal !<nodeNum hex>. The validatePublicKeyLength: false option preserves the key leniency but not the identity leniency.
The runtime caller (meshtasticManager.ts:9035) wraps pushContactToRadio in a catch documented as "radio may already have the contact, or the send failed transiently." A nodeId/nodeNum desync now lands in that catch, so the contact is never pushed — while pkiEncrypted was already set true a few lines above, meaning a PKI-encrypted DM goes out to a radio that lacks the contact.
This requires a DB desync so it should be rare, but it is a new failure mode in an existing path, and the catch comment no longer describes everything that can reach it. Either extend the options object to relax identity checking for that path, or log the SharedContactValidationError distinctly so it doesn't masquerade as a transient send failure.
Minor
QrCodeCanvasnever clears a stale render.if (!value || !canvasRef.current) return;leaves the previous QR on the canvas. Not reachable fromMeshtasticContactShare(it unmounts on collapse), andExportConfigModalbehaved the same way before this PR, so no regression — but an explicit clear would be more correct.- Server error text is surfaced verbatim via
requestError.message. Strings likenodeId !x does not match nodeNum Nare developer-facing; consider mapping thecodeto a translated string instead. decodeBase64Bytesonly rejectsnormalized.length % 4 === 1; other malformed input is silently truncated byBuffer.from. Harmless wherever a length is asserted (the 32-byte key), but thevalidatePublicKeyLength: falsepath will accept garbage.- Only
en.jsonis updated; the other nine locales fall back to the inline English defaults. Consistent with how other features have landed, just noting it.
Verdict
No correctness bugs in the encoder itself, and the compatibility fixture gives real confidence in the wire format.
Finding 1 is the one I'd want resolved before merge — it's a per-source permission deviation in a brand-new route, in a codebase where that rule is explicit and has caused a prior incident. 2 and 3 are convention items for a maintainer to call. 4 would benefit from a note on intent from you.
Review by Claude Code at @Yeraze's request.
|
Addressed the review feedback in 5252457:
Validation:
Local caveats: typecheck:tests still reports the existing broad unrelated test-type backlog. The full test run also hit unrelated timeouts in meshcoreVirtualNodeServer.test.ts and remained held open, so it was terminated after the contact-sharing tests had passed independently. |
Review — approve, mergingReviewed the full diff. This conforms to the project's conventions more carefully than most contributions, and the protocol-correctness evidence is unusually strong. Details, since some of it is worth other reviewers knowing about: Protocol correctness is actually demonstrated, not asserted. The encoder test pins byte-for-byte equality against a real known-good Meshtastic URL ( The security-relevant flags are pinned by test. The identity-consistency guard is the right call. Rejecting a node whose Route review:
On exposing the public key and MAC via a new endpoint: considered, and I'm satisfied. Both are broadcast openly in The Frontend conventions: Nothing blocking, and nothing I'd ask you to change. Thanks for the thorough PR description and the deploy-against-live-data validation — it made this much faster to review. |
…4367) * chore(release): 4.13.2 — version bump, changelog backfill, doc gaps Rev 4.13.2-rc4 to 4.13.2 across all five version-tracked files, and bring the documentation current with the 101 commits merged since v4.13.1. CHANGELOG restructure. 4.13.1 shipped on 2026-07-20 without a section of its own, so the nine entries it shipped were still sitting under [Unreleased] and would have been reattributed to 4.13.2. They move into a new [4.13.1] section, which notes that the GitHub release notes carry the parts never written down at all (link-quality badges, Noise Floor, MEDIUM_TURBO, the NeighborInfo-hijack telemetry retry). [4.13.2] now covers all 57 user-facing changes since that tag — 48 entries, up from the 13 that had been recorded. One entry was also stale: ATAK Phase 1 still claimed V2 was "labeled but not yet decoded", which #4321 made untrue. Feature-doc gaps found and filled. The large features were already documented (ATAK, receive-only mode, ok_to_mqtt, 3D map, Link Profile, NodeInfo Enrichment); these four shipped with none: - docs/configurator.md — Portainer Stack export format (#4282) - docs/features/maps.md — share a node as a Meshtastic contact (#4327) - docs/features/automation.md — Auto-Ack resend attempts (#4266) - docs/features/meshcore.md — {ROUTE}/{ROUTE_NAMES}/{HASH_SIZE} tokens (#4276) The contact URL is `https://meshtastic.org/v/#…` (SharedContact), not `/e/#` (channel set) — verified against sharedContactService.ts rather than assumed, and the distinction is called out in the docs since a contact link carries a public key but no PSK. Blog post announcing the release leads with the PostgreSQL/MySQL migration ledger fix (#4233) rather than the features: those backends replayed every migration on each boot, and migration 030 rebuilt route_segments from scratch each time — 865k rows deleted and reinserted per restart on one install. That is the part affected users need in the first sentence. Verified: tsc clean, lint:ci clean, full suite green (11,016 passed, 0 failed). The 12 suites that failed on the first run were the fresh-worktree submodule gap, not this change — all pass after `git submodule update --init`. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01PZtasD4tS76xq2PTDHMA5o * docs(changelog): clarify the ATAK V2 cross-reference Address review: inside the 4.13.2 section, "not decoded in this phase" read oddly when the V2 decoder entry sits a few lines below in the same release. Name the phase and say the decoder ships here too. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01PZtasD4tS76xq2PTDHMA5o --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]>
Summary
Adds canonical Meshtastic
SharedContactURL generation and an expandable Share contact card to Meshtastic Node Details.Any visible node with an internally consistent Meshtastic identity can be shared, including unmessagable nodes and nodes received through MQTT. The generated
https://meshtastic.org/v/#…payload preserves the availableUseridentity, always setsshouldIgnore=false, and never claims manual verification.What changed
SharedContactencoder with strict node identity, MAC, and public-key validationcreateAddContactMessagepathisUnmessagableCompatibility
The encoder reproduces this known-good unmessagable WAM8 contact URL byte-for-byte:
https://meshtastic.org/v/#CPXr_8UEElgKCSE0OGJmZjVmNRIVUi1TRUQtQkzDhUtBTVBFTi1XQU04GgRXQU04IgbB30i_9fUoCTgCQiA1BZ7pj0ZZzX7VjTUKPMB-j6QbrWAoWS6J0ksAArgJQ0gBThe fixture decodes to node
1220539893/!48bff5f5, namesR-SED-BLÅKAMPEN-WAM8/WAM8, its hardware model, role, MAC address and 32-byte public key, withisUnmessagable=true.Protocol/client references:
Validation
npm run typechecknpm run lint:cinpm run buildnpm run build:servernpm run docs:build7b9f716eon Vidda against live Meshtastic node data; the QR and URL sharing flow worked as expectedScope
This intentionally does not add contact import, native sharing, QR download, NFC, public API/OpenAPI changes, dependencies, migrations, version changes, or any MeshCore behavior.