From 6243267522c7de0db01091d5e026ac2083bd153b Mon Sep 17 00:00:00 2001 From: Joseph Mearman Date: Fri, 18 Sep 2026 09:53:11 +0100 Subject: [PATCH 1/3] fix(core): stop gating hub-relayed room-domain verbs behind bare-device trust Extract HubSession's per-request dispatch decision into an exported dispatchHubRequest function and remove the coarse isTrusted allowlist check for a real room-domain verb (room.send, room.join, room.notify, ...): that traffic is already independently gated by its own room:member capability token, verified regardless of transport path and principal-aware since the #187 delegation work, so stacking the bare-device gate in front of it was redundant and, for room.join specifically, prevented an untrusted device from ever reaching the deliberate human-approval step. The legacy FRAME_VERB path keeps its isTrusted gate, since it carries no independent per-message security of its own. A request with no fromDevice at all is now refused before either path: dispatching it to a room verb handler would otherwise call deviceIdFromHex on a non-hex placeholder and throw, killing the whole session's drain loop instead of answering unauthorized. Widen the hub gossip directory-merge filter to accept a device that is itself a trusted user principal, not only one on the bare-device allowlist, so a principal already trusted via addPrincipal surfaces in gossip without a separate per-device bootstrap. --- src/core/hub-session.ts | 145 ++++++++----- src/core/wire-mesh-transport.ts | 5 +- src/test/hub-session-dispatch.test.ts | 286 ++++++++++++++++++++++++++ 3 files changed, 380 insertions(+), 56 deletions(-) create mode 100644 src/test/hub-session-dispatch.test.ts diff --git a/src/core/hub-session.ts b/src/core/hub-session.ts index 6ea29a53..d5884ab3 100644 --- a/src/core/hub-session.ts +++ b/src/core/hub-session.ts @@ -54,8 +54,10 @@ export interface HubSessionDeps { request: IncomingManageRequest, handle: Readonly, ) => Promise; - /** The gateway trust boundary (agent-comms#156): whether the given device-id (hex) is currently trusted. Checked against every gossiped directory entry's own device and every relayed request's own fromDevice before this side merges or dispatches it -- see consume()/connect()'s own doc comments for exactly where and why. */ + /** The gateway trust boundary (agent-comms#156): whether the given device-id (hex) is currently trusted as a bare device. Since agent-comms#192, this gates only the traffic that carries no independent per-message security of its own: the legacy FRAME_VERB path (consume()'s own doc explains why) and dispatchHubRequest's own hubPeersKnown bookkeeping for a room-domain sender. A real room-domain verb's own dispatch is never gated on this at all; see dispatchHubRequest's own doc for why that is sound. connect()'s own directory-merge filter uses isTrustedForDirectory below instead, not this. */ isTrusted: (deviceHex: string) => boolean; + /** Whether the given device-id (hex) is trusted for gossip directory-merge purposes (agent-comms#192): true when it is on the same bare-device allowlist isTrusted above checks, or when it is itself a trusted user-principal's own device-id (agent-comms#187's GatewayTrust.isTrustedPrincipal). A gossiped directory entry carries no capability token to chain-verify, only a bare device-id, so this can't reuse isTrustedFor's own bearer/rootIssuer chain check the way a real capability-token verification does; checking the gossiped device-id directly against the principal set is the sound degenerate case of that check, since a principal's own device presenting itself is, trivially, its own chain root. Scoped to connect()'s own directory-merge filter only: every other isTrusted call site keeps its original bare-device-only meaning, since agent-comms#192 only asked for the principal extension here. */ + isTrustedForDirectory: (deviceHex: string) => boolean; /** Forwards a room-domain manage-request on to a specific LOCAL peer session (one this gateway is directly connected to over the ordinary local mesh, keyed by device-id hex) rather than dispatching it against this gateway's own local state -- consume()'s own toDevice disambiguation (agent-comms#184, wire-mesh-core 1.48.1's IncomingManageRequest.toDevice). Returns undefined when no local session exists for that device-id, in which case consume() falls back to handleRoomRequest exactly as it always has. */ forwardToLocalPeer: ( deviceHex: string, @@ -122,7 +124,7 @@ export class HubSession { this.session = session; this.connection = connection; this.deps.trackForShutdown(session); - // Merge the hub's directory (its catch-up arrives as the first session events) and keep refreshing it on every subsequent one. Every trusted remote entry is also surfaced via onDirectory, so the transport can merge it into its own mesh-wide knownDevices (agent-comms#155's remote-directory-merge leg). Filtered to isTrusted (agent-comms#156) before either hubPeersKnown tracking or onDirectory sees it: an untrusted device's gossiped presence is not merely withheld from listAgents, it is never even recorded as "known" here, so nothing downstream can act on it via any path this class exposes. + // Merge the hub's directory (its catch-up arrives as the first session events) and keep refreshing it on every subsequent one. Every trusted remote entry is also surfaced via onDirectory, so the transport can merge it into its own mesh-wide knownDevices (agent-comms#155's remote-directory-merge leg). Filtered to isTrustedForDirectory (agent-comms#156, widened by agent-comms#187's principal allowlist per agent-comms#192) before either hubPeersKnown tracking or onDirectory sees it: an untrusted device's gossiped presence is not merely withheld from listAgents, it is never even recorded as "known" here, so nothing downstream can act on it via any path this class exposes. void (async () => { const ownHex = deviceIdToHex(identity.deviceId); for await (const event of session.events) { @@ -130,7 +132,7 @@ export class HubSession { const remoteEntries = event.directory.filter( (entry) => deviceIdToHex(entry.device) !== ownHex && - this.deps.isTrusted(deviceIdToHex(entry.device)), + this.deps.isTrustedForDirectory(deviceIdToHex(entry.device)), ); for (const entry of remoteEntries) { this.hubPeersKnown.add(deviceIdToHex(entry.device)); @@ -145,61 +147,14 @@ export class HubSession { })(); } - /** Dispatches inbound relayed manage-requests: each is handled with a handle keyed by the SENDING device (request.fromDevice names it on relay-routed requests), so every downstream consumer sees the true origin, never the hub -- the same discipline extends to a real room-domain verb (agent-comms#155's "remote to local" leg) as it already applied to the legacy opaque-frame path. Every request is first checked against the gateway trust boundary (agent-comms#156, deps.isTrusted): a request with no fromDevice at all (senderHex falls back to the literal string "hub-peer", never a real trusted device-id) or an unrecognised fromDevice is never dispatched to either path below -- a legacy FRAME_VERB message is silently dropped (matching isStateMutatingMessage's own swallow-and-ack style, so an untrusted sender learns nothing about why), and a room-domain request gets an explicit `unauthorized` error rather than being dispatched, so its caller fails fast instead of waiting out HUB_ROOM_REQUEST_TIMEOUT_MS's local-session-side counterpart. A legacy FRAME_VERB carrying state_sync/state_update is dropped before ever reaching onMessage/applyPatch even from an otherwise-trusted sender -- see isStateMutatingMessage's own doc for why: gateway trust says "this device's traffic is worth acting on," not "this device may directly overwrite this side's mesh state," which is a strictly stronger claim the trust boundary here was never meant to grant (a security review finding on agent-comms#169). A real room-domain verb (room.send, room.join, room.notify, ...) carries no equivalent risk -- it is independently gated by its own room:member capability token, verified regardless of which transport path it arrived over. Multi-device gateway routing (agent-comms#184, wire-mesh-core 1.48.1's own IncomingManageRequest.toDevice, read directly from each relay-data frame's own to-device field rather than guessed from pairing state): when the request carries a toDevice that names a different device than ownDeviceHex, the command is first re-stamped with an "on-behalf-of" params field naming senderHex (already verified trusted above) before deps.forwardToLocalPeer forwards it on to that device's own local mesh session (one this gateway is directly connected to, never merely gossiped-about) -- room-router.ts's own resolveHandle reads that field back out on the receiving end, so the forwarded request is attributed to the true remote sender there, not to this gateway, matching this same method's own "every downstream consumer sees the true origin, never the hub" discipline for the local hop too. Its outcome is relayed straight back. Only when forwardToLocalPeer finds no such local session (toDevice is absent, matches ownDeviceHex, or names a device this gateway doesn't actually front) does the request fall through to handleRoomRequest, dispatched to the same roomVerbHandlers a local peer session's own drainSession uses, against THIS side's own local mesh state -- correct for traffic genuinely addressed to this gateway's own agent, and the same fallback a sender still on a pre-#184 wire-mesh-core (never stamping toDevice at all) already relied on. */ + /** Consumes one hub session's inbound relayed manage-requests until it ends, dispatching each in arrival order to dispatchHubRequest -- see that function's own doc for the actual per-request trust decision. Split out purely so the decision itself is directly unit-testable against fake requests/deps (hub-session-dispatch.test.ts), the same reason hub-forwarding.ts's own forwardAdvertsToHub/pushHubCatchUp are standalone functions rather than private methods. */ private consume(session: AcceptedMeshSession, ownDeviceHex: string): void { void (async () => { for await (const request of session.incomingManageRequests) { if (this.deps.isShuttingDown()) break; - const senderHex = - request.fromDevice !== undefined - ? deviceIdToHex(request.fromDevice) - : "hub-peer"; - const handle: Readonly = { id: senderHex }; - if (!this.deps.isTrusted(senderHex)) { - if (request.command.verb === FRAME_VERB) { - await request.respond({ result: "ok" }).catch(() => undefined); - continue; - } - await request - .respond({ result: "error", code: "unauthorized" }) - .catch(() => undefined); - continue; - } - this.hubPeersKnown.add(senderHex); - if (request.command.verb === FRAME_VERB) { - const message = extractMessage(request.command); - if (message !== undefined && !isStateMutatingMessage(message)) { - this.deps.events.onMessage(handle, message); - } - await request.respond({ result: "ok" }).catch(() => undefined); - continue; - } - const toDeviceHex = - request.toDevice !== undefined - ? deviceIdToHex(request.toDevice) - : undefined; - if (toDeviceHex !== undefined && toDeviceHex !== ownDeviceHex) { - // Stamps the already-verified true sender (senderHex -- trusted above, never the "hub-peer" fallback, since an untrusted or fromDevice-less request already continued away) onto the forwarded command's own params, so the local peer's own resolveHandle (room-router.ts) can attribute the request to senderHex instead of this side's own device once it arrives over that peer's ordinary local-mesh session -- otherwise every downstream consumer at the local peer would see this gateway as the requester, never the real remote origin, defeating consume()'s own "true origin, never the hub" discipline for this forwarded leg specifically. - const forwardedCommand = { - ...request.command, - params: { - ...request.command.params, - "on-behalf-of": senderHex, - }, - }; - const forwarded = this.deps.forwardToLocalPeer( - toDeviceHex, - forwardedCommand, - request.scope, - request.token, - ); - if (forwarded !== undefined) { - const outcome = await forwarded; - await request.respond(outcome).catch(() => undefined); - continue; - } - } - await this.deps.handleRoomRequest(request, handle); + await dispatchHubRequest(request, ownDeviceHex, this.deps, (hex) => { + this.hubPeersKnown.add(hex); + }); } })(); } @@ -268,7 +223,87 @@ function hexToBytes(hex: string): Uint8Array { return bytes; } -/** A state_sync or state_update carries authority to directly overwrite or patch this side's own mesh state (agents, rooms, messages, deliveries) -- on an ordinary peer session that authority is meaningful because the peer already passed connect_request/introduce approval or the coordinator's own trusted mesh membership. A hub-relayed sender has passed neither, and gateway trust (agent-comms#156, consume()'s own isTrusted gate) doesn't grant it either: being on the allowlist means "this device's traffic is worth acting on," not "this device may directly overwrite this side's mesh state" -- a strictly stronger claim no hub-relayed sender has ever been asked to prove, since the hub itself still accepts any self-generated identity with no admission control of its own (gating the hub is deliberately out of scope for agent-comms#156). Treating a trusted sender's state_sync/state_update as equally authoritative would let it inject an outcome indistinguishable from a genuine mesh event, e.g. a spoofed inbound delivery -- so this filter stays unconditional, applied even to a sender consume() has already let past the trust gate. Every other legacy message method this session might relay is already inert on receipt (PeerLifecycle's own handleDataMessage only reacts to these two), so filtering exactly these two is a complete fix for this specific path, not a partial one. */ +/** The slice of HubSessionDeps dispatchHubRequest actually needs. Named so its own tests, and consume()'s call site, don't repeat the same Pick inline. */ +export type HubRequestDispatchDeps = Pick< + HubSessionDeps, + "isTrusted" | "events" | "handleRoomRequest" | "forwardToLocalPeer" +>; + +/** + * Decides what to do with one inbound relayed manage-request, keeping every downstream consumer's handle keyed by the SENDING device (request.fromDevice names it on relay-routed requests) rather than by this gateway, exactly as it already did for the legacy opaque-frame path and now also for a real room-domain verb (agent-comms#155's "remote to local" leg). + * + * A request with no fromDevice at all is refused before either path below ever sees it. Neither has anything sound to check against a value that isn't a real device-id: a legacy FRAME_VERB request has no security beyond the sender's own identity, and a room-domain verb's own downstream handler (room-router.ts's resolveHandle, then a capability-token check such as verifyRoomToken) calls deviceIdFromHex on whatever handle.id it's given, which throws on anything that isn't valid hex rather than answering with an ordinary unauthorized outcome. Answering unauthorized here, before that, is what keeps this a clean, fast failure for the request's own caller instead of an unhandled rejection that would kill this session's whole drain loop. + * + * The legacy FRAME_VERB path (P2's opaque-payload carriage, still the only path for anything core/room doesn't yet have real semantics for) carries no security of its own beyond the identity of its sender, so it stays gated by the coarse per-device gateway allowlist (isTrusted, agent-comms#156): an untrusted sender's frame is silently dropped, matching isStateMutatingMessage's own swallow-and-ack style so it learns nothing about why. Even a trusted sender's state_sync/state_update is still filtered out before ever reaching onMessage; see isStateMutatingMessage's own doc for why (agent-comms#169's security finding). + * + * A real room-domain verb (room.send, room.join, room.notify, ...) is never gated on isTrusted at all (agent-comms#192): it is independently gated by its own room:member capability token, verified regardless of which transport path it arrived over, and already principal-aware since agent-comms#187 (device-membership-verification.ts's own chain-walk). Stacking the coarse bare-device allowlist in front of that check would be redundant, not protective, and for room.join specifically (the one deliberately ungated verb, whose own security model is a human decision) it would actively defeat the protocol's own intended design by never letting an untrusted device reach that approval step at all. onKnownPeer is still only called here for a sender isTrusted already recognises: reaching this branch has not yet had its own token verified (that happens inside handleRoomRequest/forwardToLocalPeer), so hubPeersKnown stays "gateway-trusted hub peers", never "every device that has sent a syntactically valid room-domain request". + * + * Multi-device gateway routing (agent-comms#184, wire-mesh-core 1.48.1's own IncomingManageRequest.toDevice, read directly from each relay-data frame's own to-device field rather than guessed from pairing state): when the request carries a toDevice naming a different device than ownDeviceHex, the command is first re-stamped with an on-behalf-of params field naming the true sender before forwardToLocalPeer forwards it on to that device's own local mesh session (one this gateway is directly connected to, never merely gossiped-about); room-router.ts's own resolveHandle reads that field back out on the receiving end, so the forwarded request is attributed to the true remote sender there, never to this gateway. Its outcome is relayed straight back. Only when forwardToLocalPeer finds no such local session (toDevice is absent, matches ownDeviceHex, or names a device this gateway doesn't actually front) does the request fall through to handleRoomRequest, dispatched against this side's own local mesh state, the same fallback a sender still on a pre-#184 wire-mesh-core (never stamping toDevice at all) already relied on. + */ +export async function dispatchHubRequest( + request: IncomingManageRequest, + ownDeviceHex: string, + deps: Readonly, + onKnownPeer: (deviceHex: string) => void, +): Promise { + if (request.fromDevice === undefined) { + if (request.command.verb === FRAME_VERB) { + await request.respond({ result: "ok" }).catch(() => undefined); + } else { + await request + .respond({ result: "error", code: "unauthorized" }) + .catch(() => undefined); + } + return; + } + + const senderHex = deviceIdToHex(request.fromDevice); + const handle: Readonly = { id: senderHex }; + + if (request.command.verb === FRAME_VERB) { + if (!deps.isTrusted(senderHex)) { + await request.respond({ result: "ok" }).catch(() => undefined); + return; + } + onKnownPeer(senderHex); + const message = extractMessage(request.command); + if (message !== undefined && !isStateMutatingMessage(message)) { + deps.events.onMessage(handle, message); + } + await request.respond({ result: "ok" }).catch(() => undefined); + return; + } + + if (deps.isTrusted(senderHex)) onKnownPeer(senderHex); + + const toDeviceHex = + request.toDevice !== undefined + ? deviceIdToHex(request.toDevice) + : undefined; + if (toDeviceHex !== undefined && toDeviceHex !== ownDeviceHex) { + const forwardedCommand = { + ...request.command, + params: { + ...request.command.params, + "on-behalf-of": senderHex, + }, + }; + const forwarded = deps.forwardToLocalPeer( + toDeviceHex, + forwardedCommand, + request.scope, + request.token, + ); + if (forwarded !== undefined) { + const outcome = await forwarded; + await request.respond(outcome).catch(() => undefined); + return; + } + } + await deps.handleRoomRequest(request, handle); +} + +/** A state_sync or state_update carries authority to directly overwrite or patch this side's own mesh state (agents, rooms, messages, deliveries) -- on an ordinary peer session that authority is meaningful because the peer already passed connect_request/introduce approval or the coordinator's own trusted mesh membership. A hub-relayed sender has passed neither, and gateway trust (agent-comms#156, dispatchHubRequest's own isTrusted gate for the legacy frame path) doesn't grant it either: being on the allowlist means "this device's traffic is worth acting on," not "this device may directly overwrite this side's mesh state" -- a strictly stronger claim no hub-relayed sender has ever been asked to prove, since the hub itself still accepts any self-generated identity with no admission control of its own (gating the hub is deliberately out of scope for agent-comms#156). Treating a trusted sender's state_sync/state_update as equally authoritative would let it inject an outcome indistinguishable from a genuine mesh event, e.g. a spoofed inbound delivery -- so this filter stays unconditional, applied even to a sender dispatchHubRequest has already let past the trust gate. Every other legacy message method this session might relay is already inert on receipt (PeerLifecycle's own handleDataMessage only reacts to these two), so filtering exactly these two is a complete fix for this specific path, not a partial one. */ function isStateMutatingMessage(message: MeshMessage): boolean { return message.method === "state_sync" || message.method === "state_update"; } diff --git a/src/core/wire-mesh-transport.ts b/src/core/wire-mesh-transport.ts index 21675768..95a6c893 100644 --- a/src/core/wire-mesh-transport.ts +++ b/src/core/wire-mesh-transport.ts @@ -258,7 +258,7 @@ export class WireMeshTransport implements MeshTransport { /** Every peer this side has ever received a frame from, keyed by device-id hex, tracking the raw wire-mesh-core Connection each frame arrived on -- what sendDataFrame needs, since neither AcceptedMeshSession nor MeshSession exposes a generic "send an arbitrary frame" method the way the raw Connection itself does. Registered eagerly on the very first frame from a connection (including one still in quarantine, e.g. before connect_request approval) so a later sendDataFrame call can reach it -- handleDataFrame's own trust gate (peerSessions.has) is what actually decides whether to act on anything received this way, not this map. */ private readonly connectionsByPeer = new Map(); - /** The cross-machine trust boundary (agent-comms#156): gates outbound gossip advertisement (hasAny), inbound directory merge/request dispatch, and outbound targeted hub requests (both isTrusted) -- see GatewayTrust's own class doc. Defaults to a fresh, empty (deny-all) instance when no caller wires one in, matching every existing construction site that predates this feature. */ + /** The cross-machine trust boundary (agent-comms#156): gates outbound gossip advertisement (hasAny), inbound directory merge (isTrusted or isTrustedPrincipal, wired into HubSession as isTrustedForDirectory since agent-comms#192), the legacy per-device frame path (isTrusted), and outbound targeted hub requests (isTrusted); see GatewayTrust's own class doc. A real room-domain manage-request relayed through the hub is never gated on this at all since agent-comms#192: hub-session.ts's own dispatchHubRequest relies purely on that verb's own capability-token verification instead. Defaults to a fresh, empty (deny-all) instance when no caller wires one in, matching every existing construction site that predates this feature. */ private readonly gatewayTrust: GatewayTrustReader; constructor( @@ -299,6 +299,9 @@ export class WireMeshTransport implements MeshTransport { }, handleRoomRequest: this.roomRouter.handleRequest, isTrusted: (deviceHex) => this.gatewayTrust.isTrusted(deviceHex), + isTrustedForDirectory: (deviceHex) => + this.gatewayTrust.isTrusted(deviceHex) || + this.gatewayTrust.isTrustedPrincipal(deviceHex), forwardToLocalPeer: sendToLocalPeer.bind(null, this.peerSessions), }); this.pendingConnectionTimeoutMs = pendingConnectionTimeoutMs; diff --git a/src/test/hub-session-dispatch.test.ts b/src/test/hub-session-dispatch.test.ts new file mode 100644 index 00000000..e400045a --- /dev/null +++ b/src/test/hub-session-dispatch.test.ts @@ -0,0 +1,286 @@ +/** + * Unit tests for dispatchHubRequest (agent-comms#192): the per-request trust decision hub-session.ts's own consume() loop delegates to, exercised directly against fake requests and fake deps rather than a real hub connection, mirroring hub-forwarding.test.ts's own fake-object approach for the sibling gateway-trust gates. hub-mode-session.integration.test.ts already proves the legacy FRAME_VERB gate end to end over a real hub; this file is what actually exercises the fix itself, that a real room-domain verb (room.send, room.join, room.notify, ...) is no longer rejected by the coarse per-device isTrusted allowlist at all, and the edge case that gate's removal would otherwise expose: a request with no fromDevice at all still has to be refused before ever reaching a room-verb handler, since resolveHandle/verifyRoomToken would call deviceIdFromHex on a non-hex placeholder and throw synchronously rather than answering with an ordinary unauthorized outcome. + */ + +import { describe, expect, it, vi } from "vitest"; +import type { + IncomingManageRequest, + ManageOutcome, +} from "wire-mesh-core/domain/mesh-session"; +import type { + CapabilityScope, + DeviceId, +} from "wire-mesh-core/generated/protocol"; +import { deviceIdToHex } from "wire-mesh-core/domain/device-id"; +import { dispatchHubRequest } from "../core/hub-session.js"; +import type { HubRequestDispatchDeps } from "../core/hub-session.js"; +import { FRAME_VERB } from "../core/wire-mesh-transport.js"; +import type { ConnectionHandle, TransportEvents } from "../core/transport.js"; +import type { MeshMessage } from "../core/wire-protocol.js"; + +const DEVICE_ID_HEX_LENGTH = 64; +const SENDER_HEX = "a".repeat(DEVICE_ID_HEX_LENGTH); +const OWN_HEX = "b".repeat(DEVICE_ID_HEX_LENGTH); +const OTHER_LOCAL_PEER_HEX = "c".repeat(DEVICE_ID_HEX_LENGTH); + +function deviceIdBytes(hex: string): DeviceId { + const bytes = new Uint8Array(new ArrayBuffer(hex.length / 2)); + for (let i = 0; i < bytes.length; i += 1) { + bytes[i] = Number.parseInt(hex.slice(i * 2, i * 2 + 2), 16); + } + // A fixture device-id: the real DeviceId type wire-mesh-core mints only through its own deviceIdFromHex/generateIdentity is structurally just a Uint8Array of the right length, and every dispatchHubRequest call site only ever round-trips this value back through deviceIdToHex, so a plain byte array is a faithful stand-in for a test fixture with no cast needed. + return bytes; +} + +function fakeOnKnownPeer(): ReturnType< + typeof vi.fn<(deviceHex: string) => void> +> { + return vi.fn<(deviceHex: string) => void>(); +} + +function noopEvents(): TransportEvents { + return { + onMessage: () => undefined, + onPeerConnected: () => undefined, + onPeerDisconnected: () => undefined, + onIntroduction: () => undefined, + onConnectionRequest: () => undefined, + onPeerList: () => undefined, + onPeerJoined: () => undefined, + onBecomeCoordinator: () => undefined, + onRevocationAnnounce: () => undefined, + onPresenceAdvert: () => undefined, + }; +} + +function fakeRequest( + options: Readonly<{ + verb?: string; + fromDevice?: string; + toDevice?: string; + }>, +): { + request: IncomingManageRequest; + responses: ManageOutcome[]; +} { + const responses: ManageOutcome[] = []; + const scope: Readonly = { kind: "room", path: "owner/room" }; + const request: IncomingManageRequest = { + requestId: 1, + command: { verb: options.verb ?? "room:member", params: {} }, + scope, + ...(options.fromDevice !== undefined + ? { fromDevice: deviceIdBytes(options.fromDevice) } + : {}), + ...(options.toDevice !== undefined + ? { toDevice: deviceIdBytes(options.toDevice) } + : {}), + respond: async (outcome: ManageOutcome): Promise => { + responses.push(outcome); + }, + }; + return { request, responses }; +} + +function fakeDeps( + overrides: Readonly, "events">> = {}, +): HubRequestDispatchDeps & { + events: TransportEvents & { messages: MeshMessage[] }; +} { + const messages: MeshMessage[] = []; + const events = { + ...noopEvents(), + messages, + onMessage: (_handle: Readonly, message: MeshMessage) => { + messages.push(message); + }, + }; + return { + isTrusted: () => false, + events, + handleRoomRequest: vi.fn().mockResolvedValue(undefined), + forwardToLocalPeer: () => undefined, + ...overrides, + }; +} + +describe("dispatchHubRequest", () => { + it("drops a FRAME_VERB request with no fromDevice, responding ok, without ever calling onMessage", async () => { + const { request, responses } = fakeRequest({ verb: FRAME_VERB }); + const deps = fakeDeps(); + const onKnownPeer = fakeOnKnownPeer(); + + await dispatchHubRequest(request, OWN_HEX, deps, onKnownPeer); + + expect(responses).toEqual([{ result: "ok" }]); + expect(deps.events.messages).toEqual([]); + expect(onKnownPeer).not.toHaveBeenCalled(); + }); + + it("refuses a room-domain verb with no fromDevice with an ordinary unauthorized outcome, never dispatching to handleRoomRequest", async () => { + const { request, responses } = fakeRequest({ verb: "room:member" }); + const deps = fakeDeps(); + const onKnownPeer = fakeOnKnownPeer(); + + await dispatchHubRequest(request, OWN_HEX, deps, onKnownPeer); + + expect(responses).toEqual([{ result: "error", code: "unauthorized" }]); + expect(deps.handleRoomRequest).not.toHaveBeenCalled(); + expect(onKnownPeer).not.toHaveBeenCalled(); + }); + + it("drops a FRAME_VERB request from an untrusted device, responding ok, without calling onMessage", async () => { + const { request, responses } = fakeRequest({ + verb: FRAME_VERB, + fromDevice: SENDER_HEX, + }); + const deps = fakeDeps({ isTrusted: () => false }); + const onKnownPeer = fakeOnKnownPeer(); + + await dispatchHubRequest(request, OWN_HEX, deps, onKnownPeer); + + expect(responses).toEqual([{ result: "ok" }]); + expect(deps.events.messages).toEqual([]); + expect(onKnownPeer).not.toHaveBeenCalled(); + }); + + it("delivers a FRAME_VERB request from a trusted device to onMessage and records it as a known hub peer", async () => { + const message: MeshMessage = { + method: "peer_joined", + peer: { id: "hi", port: 0, startedAt: "2026-01-01T00:00:00.000Z" }, + }; + const { request, responses } = fakeRequest({ + verb: FRAME_VERB, + fromDevice: SENDER_HEX, + }); + request.command.params = { message }; + const deps = fakeDeps({ isTrusted: (hex) => hex === SENDER_HEX }); + const onKnownPeer = fakeOnKnownPeer(); + + await dispatchHubRequest(request, OWN_HEX, deps, onKnownPeer); + + expect(responses).toEqual([{ result: "ok" }]); + expect(deps.events.messages).toEqual([message]); + expect(onKnownPeer).toHaveBeenCalledWith(SENDER_HEX); + }); + + it("never delivers a state_sync or state_update from a trusted device to onMessage (agent-comms#169)", async () => { + const { request, responses } = fakeRequest({ + verb: FRAME_VERB, + fromDevice: SENDER_HEX, + }); + request.command.params = { + message: { + method: "state_update", + patch: { type: "agent_offline", agentId: "spoofed" }, + }, + }; + const deps = fakeDeps({ isTrusted: () => true }); + + await dispatchHubRequest(request, OWN_HEX, deps, fakeOnKnownPeer()); + + expect(responses).toEqual([{ result: "ok" }]); + expect(deps.events.messages).toEqual([]); + }); + + it("dispatches a room-domain verb from an UNTRUSTED but identified device straight to handleRoomRequest, never rejecting it as unauthorized (the actual agent-comms#192 fix)", async () => { + const { request, responses } = fakeRequest({ + verb: "room:member", + fromDevice: SENDER_HEX, + }); + const handleRoomRequest = vi + .fn() + .mockImplementation(async (req) => { + await req.respond({ result: "ok" }); + }); + const deps = fakeDeps({ isTrusted: () => false, handleRoomRequest }); + const onKnownPeer = fakeOnKnownPeer(); + + await dispatchHubRequest(request, OWN_HEX, deps, onKnownPeer); + + expect(handleRoomRequest).toHaveBeenCalledTimes(1); + const [dispatchedRequest, dispatchedHandle] = + handleRoomRequest.mock.calls[0] ?? []; + expect(dispatchedRequest).toBe(request); + expect(dispatchedHandle).toEqual({ id: SENDER_HEX }); + expect(responses).toEqual([{ result: "ok" }]); + // hubPeersKnown stays "gateway-trusted hub peers": a room-domain sender's own capability token hasn't been verified yet at this point (that happens inside handleRoomRequest), so an untrusted sender reaching this branch must not be recorded as known just because it presented a syntactically valid device-id. + expect(onKnownPeer).not.toHaveBeenCalled(); + }); + + it("still records a TRUSTED room-domain sender as a known hub peer", async () => { + const { request } = fakeRequest({ + verb: "room:member", + fromDevice: SENDER_HEX, + }); + const deps = fakeDeps({ isTrusted: () => true }); + const onKnownPeer = fakeOnKnownPeer(); + + await dispatchHubRequest(request, OWN_HEX, deps, onKnownPeer); + + expect(onKnownPeer).toHaveBeenCalledWith(SENDER_HEX); + }); + + it("forwards a room-domain verb addressed to a different local device via forwardToLocalPeer, stamping on-behalf-of with the true sender", async () => { + const { request } = fakeRequest({ + verb: "room:member", + fromDevice: SENDER_HEX, + toDevice: OTHER_LOCAL_PEER_HEX, + }); + request.command.params = { verb: "room.send", text: "hi" }; + const outcome: ManageOutcome = { result: "ok" }; + const forwardToLocalPeer = vi + .fn() + .mockReturnValue(Promise.resolve(outcome)); + const handleRoomRequest = vi.fn().mockResolvedValue(undefined); + const deps = fakeDeps({ + isTrusted: () => false, + forwardToLocalPeer, + handleRoomRequest, + }); + + await dispatchHubRequest(request, OWN_HEX, deps, fakeOnKnownPeer()); + + expect(forwardToLocalPeer).toHaveBeenCalledTimes(1); + const [toDeviceArg, commandArg] = forwardToLocalPeer.mock.calls[0] ?? []; + expect(toDeviceArg).toBe(OTHER_LOCAL_PEER_HEX); + expect(commandArg).toMatchObject({ + params: { verb: "room.send", text: "hi", "on-behalf-of": SENDER_HEX }, + }); + expect(handleRoomRequest).not.toHaveBeenCalled(); + }); + + it("falls back to handleRoomRequest when forwardToLocalPeer finds no local session for the addressed device", async () => { + const { request } = fakeRequest({ + verb: "room:member", + fromDevice: SENDER_HEX, + toDevice: OTHER_LOCAL_PEER_HEX, + }); + const handleRoomRequest = vi.fn().mockResolvedValue(undefined); + const deps = fakeDeps({ + isTrusted: () => false, + forwardToLocalPeer: () => undefined, + handleRoomRequest, + }); + + await dispatchHubRequest(request, OWN_HEX, deps, fakeOnKnownPeer()); + + expect(handleRoomRequest).toHaveBeenCalledTimes(1); + }); + + it("uses deviceIdToHex(request.fromDevice) as the resolved handle id for a room-domain verb addressed to this side's own device", async () => { + const { request } = fakeRequest({ + verb: "room:member", + fromDevice: SENDER_HEX, + toDevice: OWN_HEX, + }); + const handleRoomRequest = vi.fn().mockResolvedValue(undefined); + const deps = fakeDeps({ isTrusted: () => true, handleRoomRequest }); + + await dispatchHubRequest(request, OWN_HEX, deps, fakeOnKnownPeer()); + + expect(handleRoomRequest).toHaveBeenCalledTimes(1); + const [, handleArg] = handleRoomRequest.mock.calls[0] ?? []; + expect(handleArg).toEqual({ id: deviceIdToHex(deviceIdBytes(SENDER_HEX)) }); + }); +}); From 9621cc8cd640196cfbc8b4e623ab46d2d2ebeaa6 Mon Sep 17 00:00:00 2001 From: Joseph Mearman Date: Fri, 18 Sep 2026 10:03:07 +0100 Subject: [PATCH 2/3] test(core): prove a hub-relayed room.join reaches human approval untrusted Add wireTestTransportWithHub alongside the existing wireTestTransport, returning the constructed WireMeshTransport instance itself so a test can reach transport.hub directly (connectHub, hub.peers(), isConnected) on top of a real MeshStore's own room verb handlers, since MeshStore exposes no public hub-connect wrapper outside CoordinatorGateway's own becomeCoordinator flow. Add an end-to-end test over a real relay hub proving a room.join from a device the owner's own GatewayTrust never trusts still reaches the deliberate human-approval flow (listPendingRoomJoins, then accept or reject) instead of being rejected outright by the coarse gateway allowlist, and that hubPeersKnown stays scoped to gateway-trusted devices even after a successful admission. Confirmed this fails against the pre-fix dispatch logic before restoring it. --- ...main-verb-trust-bypass.integration.test.ts | 130 ++++++++++++++++++ src/test/test-transport.ts | 64 +++++++-- 2 files changed, 179 insertions(+), 15 deletions(-) create mode 100644 src/test/hub-room-domain-verb-trust-bypass.integration.test.ts diff --git a/src/test/hub-room-domain-verb-trust-bypass.integration.test.ts b/src/test/hub-room-domain-verb-trust-bypass.integration.test.ts new file mode 100644 index 00000000..5f4e51e2 --- /dev/null +++ b/src/test/hub-room-domain-verb-trust-bypass.integration.test.ts @@ -0,0 +1,130 @@ +/** + * End-to-end proof of agent-comms#192 over a real wire-mesh relay hub (the same domain logic the production mesh.exadev.io Durable Object runs, served over local WebSockets via hub-helpers.ts): a real room.join from a device the receiving gateway's own GatewayTrust does NOT trust reaches the deliberate human-approval flow instead of being rejected outright by the coarse bare-device gateway allowlist. Under the pre-#192 code, the owner's own consume() would answer this request with an immediate unauthorized outcome, and owner.listPendingRoomJoins() would never show it at all. + * + * Only a hub connection between owner and requester is ever established, deliberately never a direct local peer connection: hub mode carries no legacy full-state-sync (room-join-admission.test.ts's own header comment explains why that path makes "a room I've never heard of" untestable over an ordinary two-peer local mesh), so this is the one topology where the requester genuinely has never heard of the owner's room before sending a real wire-level room.join for it. + */ + +import { afterEach, describe, expect, it } from "vitest"; +import { MeshStore } from "../core/mesh-store.js"; +import { wireTestTransportWithHub } from "./test-transport.js"; +import { realHubOverWs, waitForCondition } from "./hub-helpers.js"; + +const cleanups: (() => Promise)[] = []; + +afterEach(async () => { + for (const close of cleanups.splice(0)) { + await close(); + } +}); + +/** Wires up an owner and a requester MeshStore, each connected to the same real hub, with the requester trusting the owner's device (the outbound leg WireMeshTransport.sendRoomRequest needs before it will even attempt routing via the hub) but the owner trusting nothing at all -- the absence that is the whole point of every test in this file. Deliberately never calls MeshStore.init(): that method's own local-mesh coordinator election (connectToCoordinator/becomeCoordinator against the real, well-known port 19876) has nothing to do with hub mode and risks colliding with an unrelated coordinator already running on the machine -- hub-mode-session.integration.test.ts's own raw-WireMeshTransport tests never call it either, for the same reason. */ +async function connectedOwnerAndRequester(hubUrl: string): Promise<{ + owner: MeshStore; + requester: MeshStore; +}> { + const owner = new MeshStore(); + const { transport: ownerTransport } = await wireTestTransportWithHub(owner); + await owner.registerAgent({ + name: "owner", + harness: "test", + cwd: "/test/owner", + pid: process.pid, + visibility: "visible", + tags: [], + }); + + const requester = new MeshStore(); + const { transport: requesterTransport } = + await wireTestTransportWithHub(requester); + await requester.registerAgent({ + name: "requester", + harness: "test", + cwd: "/test/requester", + pid: process.pid, + visibility: "visible", + tags: [], + }); + + requester.gatewayTrust.add(owner.peerId); + expect(owner.gatewayTrust.hasAny()).toBe(false); + + await ownerTransport.connectHub?.(hubUrl); + await requesterTransport.connectHub?.(hubUrl); + await waitForCondition( + () => ownerTransport.hub.isConnected && requesterTransport.hub.isConnected, + ); + + return { owner, requester }; +} + +describe("hub-relayed room.join versus the gateway trust boundary", () => { + it("reaches the owner's human-approval flow, and grants membership on acceptance, even though the owner's gateway does not trust the requester's device at all", async () => { + const hub = await realHubOverWs(); + cleanups.push(hub.close); + const { owner, requester } = await connectedOwnerAndRequester(hub.url); + + const room = await owner.createRoom({ + name: "general", + type: "public", + owner: owner.peerId, + description: "", + }); + expect(await requester.getRoom(room.id)).toBeUndefined(); + + const joinPromise = requester.joinRoom(room.id, requester.peerId); + + await waitForCondition(() => + owner + .listPendingRoomJoins() + .some( + (pending) => + pending.roomPath === room.id && + pending.requesterId === requester.peerId, + ), + ); + + owner.acceptRoomJoin(room.id, requester.peerId); + const joined = await joinPromise; + + expect(joined.id).toBe(room.id); + expect(joined.members.includes(requester.peerId)).toBe(true); + // hubPeersKnown (peers()) stays "gateway-trusted hub peers" even after a real, successful room-domain admission: the requester's own device was never on the owner's bare-device allowlist, only its capability token was ever verified, so it must never surface as a "known" hub peer just because a room-domain request from it happened to succeed. + expect(owner.gatewayTrust.hasAny()).toBe(false); + + await requester.shutdown(); + await owner.shutdown(); + }); + + it("still lets the owner reject the same untrusted-gateway request through the ordinary human-decision outcome, not a gateway-level error", async () => { + const hub = await realHubOverWs(); + cleanups.push(hub.close); + const { owner, requester } = await connectedOwnerAndRequester(hub.url); + + const room = await owner.createRoom({ + name: "private-room", + type: "public", + owner: owner.peerId, + description: "", + }); + + const joinPromise = requester.joinRoom(room.id, requester.peerId); + + await waitForCondition(() => + owner + .listPendingRoomJoins() + .some( + (pending) => + pending.roomPath === room.id && + pending.requesterId === requester.peerId, + ), + ); + + owner.rejectRoomJoin(room.id, requester.peerId, "not today"); + + await expect(joinPromise).rejects.toThrow(); + expect(await requester.getRoom(room.id)).toBeUndefined(); + + await requester.shutdown(); + await owner.shutdown(); + }); +}); diff --git a/src/test/test-transport.ts b/src/test/test-transport.ts index 00342380..acc2dd03 100644 --- a/src/test/test-transport.ts +++ b/src/test/test-transport.ts @@ -20,13 +20,13 @@ import type { MeshStore } from "../core/mesh-store.js"; const TEST_IDENTITY_CWD_ID_LENGTH = 8; /** Wires store onto a fresh WireMeshTransport and a persisted identity slot, returning the slot so a test can inspect (or reuse) the persisted room tokens directly via loadRoomTokens(). Defaults to a throwaway temp-dir slot per call -- pass an explicit slot when a test needs the same identity to survive across more than one wireTestTransport call (e.g. simulating a restart). pendingConnectionTimeoutMs overrides WireMeshTransport's own default 5-minute connect_request expiry -- a test proving that expiry behaviour needs it far shorter than any real approval window. presenceReadvertiseIntervalMs likewise overrides the default 20s presence re-advertisement cadence -- a test proving that behaviour needs it far shorter too, or a test that doesn't care about presence at all wants it long enough to never fire spuriously mid-test. Wires getSelfAgentAdvert (`() => store.selfAgentAdvert`) the same way createBridgeMesh does -- a test relying on gossip carrying the registered agent/self extension (e.g. remote-directory-merge coverage, agent-comms#155) needs this wired exactly like production does. */ -export async function wireTestTransport( +async function wireTransportInternal( store: MeshStore, slot?: Readonly, pendingConnectionTimeoutMs?: number, presenceReadvertiseIntervalMs?: number, userIdentityOptions?: Readonly, -): Promise { +): Promise<{ slot: IdentitySlot; transport: WireMeshTransport }> { const resolvedSlot: IdentitySlot = slot ?? { harness: "test", cwd: nanoid(TEST_IDENTITY_CWD_ID_LENGTH), @@ -45,20 +45,19 @@ export async function wireTestTransport( store.peerId = deviceIdToHex(Uint8Array.from(identity.deviceId)); // One shared dataStorage instance for both the transport's own data-domain frame responder and the store's own durable-send mint path (P5, agent-comms#50) -- memory-backed, matching every other throwaway test identity here, rather than a real createNodeFsStorage a test would need to clean up afterwards. const dataStorage = createMemoryStorage(); - store.setTransport( - new WireMeshTransport( - store.events, - identity, - store.roomVerbHandlers, - pendingConnectionTimeoutMs, - () => store.selfStatus, - presenceReadvertiseIntervalMs, - undefined, - dataStorage, - () => store.selfAgentAdvert, - store.gatewayTrust, - ), + const transport = new WireMeshTransport( + store.events, + identity, + store.roomVerbHandlers, + pendingConnectionTimeoutMs, + () => store.selfStatus, + presenceReadvertiseIntervalMs, + undefined, + dataStorage, + () => store.selfAgentAdvert, + store.gatewayTrust, ); + store.setTransport(transport); store.setIdentity({ identity: await toIdentityPort(identity), clock: createSystemClock(), @@ -72,9 +71,44 @@ export async function wireTestTransport( store.onError = (e) => { console.error(`[transport error, peerId=${store.peerId}]`, e.message); }; + return { slot: resolvedSlot, transport }; +} + +/** Wires store onto a fresh WireMeshTransport and a persisted identity slot, returning the slot so a test can inspect (or reuse) the persisted room tokens directly via loadRoomTokens(). Defaults to a throwaway temp-dir slot per call -- pass an explicit slot when a test needs the same identity to survive across more than one wireTestTransport call (e.g. simulating a restart). pendingConnectionTimeoutMs overrides WireMeshTransport's own default 5-minute connect_request expiry -- a test proving that expiry behaviour needs it far shorter than any real approval window. presenceReadvertiseIntervalMs likewise overrides the default 20s presence re-advertisement cadence -- a test proving that behaviour needs it far shorter too, or a test that doesn't care about presence at all wants it long enough to never fire spuriously mid-test. Wires getSelfAgentAdvert (`() => store.selfAgentAdvert`) the same way createBridgeMesh does -- a test relying on gossip carrying the registered agent/self extension (e.g. remote-directory-merge coverage, agent-comms#155) needs this wired exactly like production does. */ +export async function wireTestTransport( + store: MeshStore, + slot?: Readonly, + pendingConnectionTimeoutMs?: number, + presenceReadvertiseIntervalMs?: number, + userIdentityOptions?: Readonly, +): Promise { + const { slot: resolvedSlot } = await wireTransportInternal( + store, + slot, + pendingConnectionTimeoutMs, + presenceReadvertiseIntervalMs, + userIdentityOptions, + ); return resolvedSlot; } +/** Same wiring as wireTestTransport, but also returns the constructed WireMeshTransport instance itself -- needed by any test that has to reach transport.hub directly (connectHub, hub.peers(), hub.isConnected), since MeshStore exposes no public hub-connect wrapper outside CoordinatorGateway's own becomeCoordinator flow (agent-comms#192's own hub-mode room-verb-gating tests are the first to need this, alongside the real room verb handlers store.roomVerbHandlers already wires in). */ +export async function wireTestTransportWithHub( + store: MeshStore, + slot?: Readonly, + pendingConnectionTimeoutMs?: number, + presenceReadvertiseIntervalMs?: number, + userIdentityOptions?: Readonly, +): Promise<{ slot: IdentitySlot; transport: WireMeshTransport }> { + return wireTransportInternal( + store, + slot, + pendingConnectionTimeoutMs, + presenceReadvertiseIntervalMs, + userIdentityOptions, + ); +} + // Generous on purpose: waitFor returns the instant its condition holds, so a long ceiling costs nothing on the happy path (a local run settles in well under a second) and only matters for the worst case -- a loaded CI runner working through a real, sequential chain of TLS handshakes (each one genuine X.509 certificate work, not instant) for the accept-flow's second connection direction, confirmed to need meaningfully more than 5s on at least one real CI run. const DEFAULT_WAIT_FOR_TIMEOUT_MS = 20_000; const WAIT_FOR_POLL_INTERVAL_MS = 20; From 995e1c3e2f9754745601b7526445fbd3b228d418 Mon Sep 17 00:00:00 2001 From: Joseph Mearman Date: Fri, 18 Sep 2026 10:05:07 +0100 Subject: [PATCH 3/3] test(core): prove gossip directory-merge accepts a principal-only trust Add an end-to-end hub test where the discovering side trusts the remote device only as a user principal (addPrincipal), never on the bare-device allowlist, and confirms connect()'s own directory-merge filter still surfaces it in hub.peers() while isTrusted itself stays false for that device -- the two allowlists remain genuinely separate, only the directory-merge filter now checks both. --- src/test/hub-mode-session.integration.test.ts | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/src/test/hub-mode-session.integration.test.ts b/src/test/hub-mode-session.integration.test.ts index 894a5e3e..1dcfc8ca 100644 --- a/src/test/hub-mode-session.integration.test.ts +++ b/src/test/hub-mode-session.integration.test.ts @@ -113,6 +113,55 @@ describe("connectToHub", () => { await transportB.shutdown(); }); + it("discovers a peer via gossip when that peer is trusted only as a principal, not as a bare device (agent-comms#192's own directory-merge widening)", async () => { + const hub = await realHubOverWs(); + cleanups.push(hub.close); + + const eventsA = recordingEvents(); + const eventsB = recordingEvents(); + const identityA = generateIdentity(); + const identityB = generateIdentity(); + const deviceA = deviceIdToHex(Uint8Array.from(identityA.deviceId)); + const deviceB = deviceIdToHex(Uint8Array.from(identityB.deviceId)); + // B trusts A's device as a PRINCIPAL (addPrincipal), never on the bare-device allowlist (add) -- proving connect()'s own directory-merge filter now accepts isTrustedForDirectory (bare-device OR principal), not only the original bare-device isTrusted. A trusts nothing at all: whether B's own gossip surfaces here has nothing to do with what A trusts, only with what B's own incoming filter accepts. + const gatewayTrustA = new GatewayTrust(); + const gatewayTrustB = new GatewayTrust(); + gatewayTrustB.addPrincipal(deviceA); + const transportA = new WireMeshTransport( + eventsA, + identityA, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + gatewayTrustA, + ); + const transportB = new WireMeshTransport( + eventsB, + identityB, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + undefined, + gatewayTrustB, + ); + await transportA.hub.connect(hub.url); + await transportB.hub.connect(hub.url); + + await waitForCondition(() => transportB.hub.peers().includes(deviceA)); + // The bare-device allowlist is untouched by this widening: A was never added() to gatewayTrustB, only addPrincipal()'d, so isTrusted(deviceA) itself must still read false even though isTrustedForDirectory let the gossip through. + expect(gatewayTrustB.isTrusted(deviceA)).toBe(false); + + await transportA.shutdown(); + await transportB.shutdown(); + }); + it('never applies a state_sync or state_update relayed by a hub peer, even one this side now explicitly trusts (agent-comms#169 security finding: real per-peer admission landed in #156, but gateway trust means "this device\'s traffic is worth acting on", not "this device may directly overwrite this side\'s mesh state")', async () => { const hub = await realHubOverWs(); cleanups.push(hub.close);