fix(mesh): attribute command results, stop broadcasting device commands, bind transfer tokens, bound the registry - #655
Merged
Conversation
β¦tokens, bound the registry Four confirmed mesh/bridge holes, each verified with live probes. 1. completeCommand device-spoofing bypass (core/mesh/service.ts) `if (from && from !== pending.deviceId)` skipped the ownership check entirely when a reply omitted `deviceId` β any connected client could answer another device's pending command and feed the model fabricated exec stdout or a fake install success. A missing deviceId is now dropped like a mismatched one. The alternative (accept an unattributed reply when exactly one command is pending) was considered and rejected: it still cannot attribute the reply, it only narrows the window to whenever the mesh is idle β which is most of the time β while looking like a check. Nothing real is lost: `deviceId` has always been part of the command-result wire contract (protocol/fixtures/mesh_v1.json, asserted by daemon, talon-node and the companion) and both shipped clients send it. An unattributable reply now leaves the command to time out honestly instead of resolving with someone else's data. 2. Device commands no longer broadcast to every SSE client `device_command` params carry one-time transfer tokens, exec command lines, remote paths and β on the chunked fallback β the entire base64 file body. They went to every connected client, with "each client filters by its own device id" as the only protection: courtesy, not enforcement. A client now names itself on connect (`GET /events?deviceId=β¦`), the bridge keeps the claim per connection, and `BridgeServer.sendToDevice` delivers a command only to the client(s) that claimed the target. `locate` still fans out β it carries no secret and pre-command app builds depend on it. Clients that claim nothing remain reachable: they receive a command only when nobody claimed its target, so an updated device's traffic is never visible to them and the fallback shrinks as the fleet updates. The claim is an address, not a credential β the bridge bearer token is still the trust boundary; what changes is that a device stops passively receiving its peers' secrets. talon-node and the companion both claim their id now (apps/node/bridge.go, companion BridgeClient.meshDeviceId β ConnectionConfig.eventsUrl). 3. Transfer tokens bound to the device they were minted for TransferStore.take() validated token/direction/expiry but never who was redeeming. It now refuses a caller that names a different device than the token was minted for, before the single-use latch β so a wrong claim cannot burn the real device's token either. /devices/file passes the caller's `deviceId`; both in-repo clients send it. A caller that claims nothing is still served, deliberately: requiring the claim would break the transfer that ships the client build able to make it (update_device/update_node push the new binary over `download_file`), so a daemon upgraded ahead of its fleet would refuse the very transfer that updates the fleet. The token still only reaches the target device over the now-addressed command channel, is single-use, and expires in 10 minutes. Once the fleet claims, one line makes the binding mandatory. 4. Registry growth bounded (core/mesh/registry.ts) sanitizeDevice bounded only arch/capabilities: id, name and appVersion rode in unbounded up to the 256KB body cap and were persisted forever. Names and versions are now clamped (display-only β a silly name costs the label, not the registration); an over-long id is refused outright, since truncating an identity key would quietly merge two devices onto one entry. Location deviceId and provider get the same treatment. The device map is capped at 128, evicting the least-recently-seen entry (with its location + history) rather than refusing newcomers β a live device re-registers within ~60s and takes its slot back. With the existing per-device history bound this also caps the history sidecar. storeLocation accepted any deviceId, registered or not, each call rewriting all three sidecars. Unregistered ids are still accepted (a fix can arrive just before the first registration lands) but the orphan set is capped at 16, stalest-first, and history keys belonging to neither a device nor a location are swept. Both caps also run on load(), so an oversized sidecar is a one-boot problem rather than permanent state. Tests (all through the real code paths): an unattributed command result is dropped and the command times out; a transfer token is refused under another device's name over real HTTP without burning the token, in both directions; two live SSE clients prove a command reaches only the claiming one while broadcasts still reach everyone, and that unclaimed clients keep working without seeing claimed traffic; registry caps for over-long ids, clamped display fields, device eviction and orphan locations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
1.
completeCommanddevice-spoofing bypassservice.ts:if (from && from !== pending.deviceId) return false;β the empty-string fallback (kept "for older app builds") meant a reply that omitteddeviceIdskipped the ownership check entirely. Combined with #2, any connected device could race and feed the model fabricatedexecstdout or a fake install success.Now rejected outright rather than "accept when exactly one command is pending" β the one-pending variant still can't attribute the reply, it only narrows the window to whenever the mesh is idle, while looking like a check. Back-compat cost is nil:
deviceIdhas always been in the command-result wire contract (protocol/fixtures/mesh_v1.json, asserted by daemon/node/companion conformance tests) and both shipped clients already send it.2. Device commands were broadcast to every SSE client
Comments claimed "each client filters by its own device id" β client-side courtesy, not enforcement. Every connected client received one-time transfer tokens, exec command lines, remote paths, and on the chunked fallback the entire base64 file body. A
device_push_fileto phone A handed the file to phone B.Clients now name themselves with
GET /events?deviceId=β¦;sendToDevice()deliversdevice_commandto the claiming client(s) only.locatestill fans out (no secret in the frame, and pre-command builds depend on it). A client that claims nothing receives a command only when nobody claimed its target, so updated devices' traffic is never visible to legacy clients and the fallback shrinks as the fleet updates.3. Transfer tokens weren't bound to their device
transfers.tstake()validated token/direction/expiry but never that the peer was the device the token was minted for β so a second device could consume another's push token (β arbitrary daemon-host file) or satisfy a pull token (β attacker bytes written to the daemon's destination). Now checked, and checked before the single-use latch so a wrong claim can't burn the real device's token.Deliberate concession: an absent claim is still served. Making it mandatory would break the transfer that ships the client build able to make it β
update_device/update_nodepush the new binary overdownload_file, so a daemon upgraded ahead of its fleet would refuse the very transfer that updates the fleet. Dropping thefromDeviceId &&guard later makes it mandatory in one line.4. Unbounded registry growth
sanitizeDevicebounded onlyarchandcapabilities;id/name/appVersionwere unbounded to the 256 KB body cap, andstoreLocationaccepted any deviceId β registered or not β rewriting all three sidecar files in full each time.Caps: id 128 (refused if longer β truncating an identity key would merge devices), name 128 / appVersion 64 (clamped, display-only), device map capped at 128 with least-recently-seen eviction, orphan locations capped at 16.
Verification
tsc --noEmit,oxlint,prettier --checkclean. Full non-integration suite passes (4067). New tests cover each fix through real code paths, including two live SSE connections proving a command reaches only the claiming client whilebroadcaststill reaches everyone, and an over-HTTP check that a rejected transfer claim doesn't burn the token.Please double-check
n.DeviceIDexists,net/urlalready imported;eventsUrl()output is unchanged when no id is passed, sobridge_models_test.dartshould still pass) β but they want a real build.Most of this work was done by a parallel agent in an isolated worktree; I rebased it, re-verified it in a working environment (its own run had 17 environmental failures from an empty
node_modules), and reviewed the diff.π€ Generated with Claude Code