Skip to content

mesh,web: add frozen Hello discovery layer for cross-firmware onboarding - #452

Merged
Variour merged 8 commits into
mainfrom
claude/mesh-devices-not-discovering-3tc5al
Jul 24, 2026
Merged

mesh,web: add frozen Hello discovery layer for cross-firmware onboarding#452
Variour merged 8 commits into
mainfrom
claude/mesh-devices-not-discovering-3tc5al

Conversation

@Variour

@Variour Variour commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Summary

  • Two devices on the same ESP-NOW channel could fail to find each other in the mesh whenever their PresenceMsg schema/version differed — MeshManager::_onRecv silently drops any mismatched frame, by design (same-firmware-only policy, see docs/mesh-compatibility.md). That blocked the whole initial-setup path for a new device: it must be discoverable, receive a WiFi-config push, and get a firmware-update nudge before it's guaranteed to run compatible firmware.
  • Adds MsgType::Hello / HelloMsg — a minimal, permanently frozen broadcast (name + fwVersion only, no version field, never to gain fields) carved out as the one deliberate exception to the same-firmware policy. Every device sends it every heartbeat regardless of onboarding state.
  • PeerRegistry tracks hello-only peers via a new helloOnly flag (reusing the existing registry rather than adding a second one); receiving a Hello also feeds ChannelManager's peer-heard signal, so channel-lock convergence no longer silently depends on PresenceMsg compatibility either.
  • /api/peers (REST + WS peers push) now exposes a discoveredPeers array for hello-only devices. Dashboard gets a new "Devices found (setup required)" card that reuses the existing config-push modal and check-update flow unchanged, since both already key off a MAC address rather than PeerRegistry contents.
  • Mock server (server/index.js) updated for parity: MOCK_DISCOVERED_PEERS seeded on both the REST route and the WS pushes.
  • docs/mesh-compatibility.md documents the Hello exception and why it doesn't reopen the "no compatibility code" rule for the rest of the protocol.

Explicitly out of scope (separate, independent issue): ChannelManager's search sequence only tries channels {1, 6, 11} on a fresh device and never reconverges onto a router's channel outside that set.

Test plan

  • npm run lint — clean
  • npm test — 36/36 passing
  • Live Playwright check against the mock server: "Devices found" card renders the seeded hello-only device's name/version/MAC; "Push config" opens the existing modal correctly targeted at that MAC; "Check for update" POSTs the correct MAC to /api/peers/checkupdate
  • Manual hardware check (not run in this session): flash two devices with deliberately different PRESENCE_MSG_VERSION, confirm mutual discovery via the new card, confirm WiFi-config push and update-check both succeed against a hello-only peer, confirm the device migrates into the normal peers list once it reboots onto matching firmware

Generated by Claude Code

@Variour Variour added the enhancement New feature or request label Jul 24, 2026 — with Claude
@github-actions

Copy link
Copy Markdown

Preview

URL: https://batterylight-pr-452.blackpond-ef672c92.germanywestcentral.azurecontainerapps.io

Hosted on Azure Container Apps — accessible from the internet.

claude added 3 commits July 24, 2026 10:35
Two devices on the same channel could fail to find each other whenever
their PresenceMsg schema/version differed, since MeshManager silently
drops any mismatched frame (by design — see mesh-compatibility.md).
That blocked the entire onboarding path for a new device: it must be
discoverable, receive a WiFi-config push, and get an update nudge
before it's guaranteed to run compatible firmware.

Add MsgType::Hello, a minimal, permanently-frozen broadcast (name +
fwVersion only) carved out as the one deliberate exception to the
same-firmware policy. Every device sends it every heartbeat regardless
of onboarding state; PeerRegistry tracks hello-only peers separately
from full peers via a new helloOnly flag. Receiving a Hello also feeds
ChannelManager's peer-heard signal, so channel-lock convergence no
longer depends on PresenceMsg compatibility either.

The dashboard gets a "Devices found (setup required)" card for
hello-only peers, reusing the existing config-push modal and
check-update flow unchanged, since both already operate purely on a
target MAC address. Mock server updated for parity.
@Variour
Variour force-pushed the claude/mesh-devices-not-discovering-3tc5al branch from 0ac9076 to bc01aae Compare July 24, 2026 10:36
claude added 3 commits July 24, 2026 10:48
A hello-only device (incompatible firmware, see mesh-compatibility.md)
can't report wifiConnected/hasWifiNetworks via PresenceMsg, so there
was no way to tell whether "Check for update" on it could succeed
right now versus needing a WiFi-config push first. Extend HelloMsg
with those two fields while it's still pre-release (mirrors
PresenceMsg's own "reset before first real deployment" convention),
and surface the status as a badge on the discovered-devices card.
The old two-button/badge UI (Push config, Check for update, plus a
WiFi-status badge) left the sequencing up to the user, never actually
installed anything (check-only), and gave no feedback beyond a native
alert() on failure. Replace it with a single "Set up device" button
that pushes this device's WiFi config only if the target isn't
already connected, waits for it to reconnect, then triggers a
check-and-install update (TriggerUpdateMsg already does both) —
narrating each phase honestly from what HelloMsg actually carries,
with bounded stall timeouts instead of a fake progress bar.

Two PeerRegistry fixes were required for the dashboard to notice
these transitions promptly: updateHello() only fired onChange() for
brand-new entries, missing a later wifiConnected flip; update() only
fired onChange() based on name/light diffs, missing a device
graduating from hello-only to a full peer with no other change.

Mock server simulates both transitions (WiFi reconnect after a config
push, graduating to a full peer after a triggered update) so the flow
is testable without hardware.
…utton flow

Leftover text still described the old separate push-config/check-update
buttons.

Variour commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Follow-up on the adversarial review findings:

1. Missing compile-time freeze guard — agreed, we should do that.
Add static_assert(sizeof(HelloMsg) == 51, "HelloMsg is frozen forever — see comment above; add a new MsgType instead") in MeshTypes.h right after the struct. Cheap, and it turns "please don't" into "won't compile."

2. MAX_PEERS = 16 shared pool — limit is fine, but is it documented?
No. Checked README.md, all of docs/*.md (including architecture.md and mesh-compatibility.md) — grepped for MAX_PEERS, "16 devices", "16 peers", nothing. It only exists as PeerRegistry::MAX_PEERS in code (src/mesh/PeerRegistry.h:47). If you want it user-facing, the natural spot is README's "Adding more devices to the mesh" section (line ~103) — happy to add a one-line note there if that's wanted, otherwise leaving as-is since you said the limit itself is fine.

3. Dead _onChange wiring — elaborated + suggested fix.
PeerRegistry::setOnChange is never called anywhere in the codebase — grepped the full src/ tree, only the setter definition exists, no call site in main.cpp or elsewhere. So the graduated/wifiChanged change-detection this PR adds to update()/updateHello() never actually fires anything; its own justifying comments ("without this, the dashboard wouldn't notice") don't hold in practice. What actually notifies the dashboard: setOnPresence calls publishTelemetry() unconditionally every heartbeat, but setOnHelloPeer only fires when isNew is true — so a wifiConnected flip on an existing hello-only peer (the exact moment that should advance waiting-wifi → triggering-update) produces no WS push on real hardware at all. It still works today only because of the frontend's 1.5s REST poll fallback.

Two ways to fix, pick one:

  • Wire mesh.peers.setOnChange([]{ publishTelemetry(); }); once in main.cpp::setup(), matching the style of the other callbacks. Makes the PR's own change-detection logic meaningful and gives an immediate push on wifi-flip/graduation instead of relying solely on polling. Slight redundancy: some other paths (e.g. updateLightGroup) already call publishTelemetry() explicitly, so this would double-push in a few cases — harmless, just extra WS traffic.
  • Narrower: make the Hello case in MeshManager::_onRecv call _onHelloPeer unconditionally instead of only if (isNew), mirroring how Presence already pushes every heartbeat regardless of newness.

Either way, the 1.5s poll should stay as a backstop, not the primary mechanism it currently is by accident.

4. Open-redirect via name — mitigation.
esc() only escapes & < > ", not / or @. href="http://${esc(p.name)}.local" lets a spoofed device (unauthenticated broadcast, attacker fully controls name) name itself e.g. evil.example.com/x, producing a link whose actual host is evil.example.com, not .local. Fix: swap esc(p.name) for encodeURIComponent(p.name) in that one href — percent-encodes //@/: so the result can't escape the .local suffix, while still resolving to nothing for legitimate device names (which don't contain those characters). Belt-and-suspenders option: also reject/ignore the mDNS link (fall back to no link) if p.name doesn't match a plain-hostname charset.

5. Possible OOB read via non-null-terminated attacker payload — mitigation.
HelloMsg.name/fwVersion have no guaranteed wire-level null terminator; a crafted 51-byte frame with no zero byte anywhere makes strlcpy(p->name, name, sizeof(p->name)) in updateHello() scan past the field into adjacent buffer memory looking for one. Fix: before treating m->name/m->fwVersion as C-strings in the MsgType::Hello case of _onRecv, verify each is null-terminated within its declared size (e.g. memchr(m->name, 0, sizeof(m->name)) — reject the frame if not found) rather than trusting the sender. Worth doing once here since HelloMsg's wire format can never change again — this is the one place we can't "harden it in the next version."

6. Test coverage — filed #454 to track a general review of test coverage across the project (this PR's ~250 new lines of frontend state-machine logic and new mock-server endpoints with no test additions being the trigger example).


Generated by Claude Code

- Add a static_assert freeze guard on HelloMsg's size, so a future
  edit to that struct fails to compile instead of silently reopening
  the same-firmware compatibility gate it's meant to be exempt from.
- Wire PeerRegistry::setOnChange in main.cpp — it was defined but
  never called, so the change-detection added earlier (wifiConnected
  flip, hello-only -> full-peer graduation) never actually pushed a
  WS update on real hardware; only the frontend's REST poll fallback
  papered over it.
- Fix an open-redirect in the discovered-device card's mDNS link:
  device name is attacker-controlled (unauthenticated broadcast), so
  only build the link for names that look like a plain hostname, and
  percent-encode as defense in depth.
- Reject a Hello frame whose name/fwVersion aren't null-terminated
  within their declared size, instead of letting updateHello()'s
  strlcpy() scan past the buffer looking for one.

Variour commented Jul 24, 2026

Copy link
Copy Markdown
Owner Author

Pushed fixes for the review findings (cdd4803):

  1. Freeze guard — added static_assert(sizeof(HelloMsg) == 51, ...) right after the struct in MeshTypes.h.
  2. Dead _onChange wiring — wired mesh.peers.setOnChange([]() { publishTelemetry(); }); in main.cpp::setup(), alongside the other mesh callbacks. The 1.5s frontend poll stays as a backstop, but the dashboard now gets a prompt WS push on a hello-only device's wifiConnected flip or graduation, not just on the next poll tick.
  3. Open-redirect via name — the "Open device" link now only renders when p.name matches ^[A-Za-z0-9-]+$, and uses encodeURIComponent(p.name) on top of that as defense in depth.
  4. OOB read on non-null-terminated payload — the Hello case in _onRecv now rejects the frame if memchr doesn't find a null terminator within name's or fwVersion's declared size, before either is ever treated as a C-string.

Left as-is per your note in the follow-up:
2. MAX_PEERS documentation — no change, since you said the limit itself is fine; happy to add the README note if you'd still like it.
6. Test coverage — tracked separately in #454, out of scope for this PR.

clang-format, eslint, and the test suite are all clean; re-ran the mock-server setup flow via Playwright to confirm nothing regressed.


Generated by Claude Code

Follow-up to the PR #452 review discussion — the limit itself is fine,
just wasn't documented anywhere user-facing.
@Variour
Variour merged commit 85e1075 into main Jul 24, 2026
11 checks passed
@Variour
Variour deleted the claude/mesh-devices-not-discovering-3tc5al branch July 24, 2026 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants