fix(core): stop gating hub-relayed room-domain verbs behind bare-device trust - #195
Conversation
…ce 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.
…usted 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.
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.
9924c25 to
995e1c3
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🎉 This PR is included in version 3.17.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: 995e1c3e2f
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
| return; | ||
| } | ||
| } | ||
| await deps.handleRoomRequest(request, handle); |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
When A previously initiated a DM with B, any untrusted hub client C can now send room.join for A+B: this dispatch reaches handleRoomJoin without isTrusted, which checks only that A is a participant and auto-approves solely because the path is in dmRequestsInitiatedByMe. admitRoomJoin then signs a room:member token for C. C can use it for room.notify; a dm event has no room field, so the scope check accepts an event claiming from: B and delivers a spoofed DM to A. Require handle.id to equal the other DM participant before auto-approval or grant minting.
Useful? React with 👍 / 👎.
1 participant
Closes #192
Extracts HubSession's per-request dispatch decision into an exported
dispatchHubRequestfunction and removes the coarseisTrustedallowlist check for a real room-domain verb (room.send, room.join, room.notify, ...). That traffic already carries its own independent room:member capability token, verified regardless of transport path and principal-aware since #187, so stacking the bare-device gateway allowlist in front of it was redundant, and for room.join specifically it prevented an untrusted device from ever reaching the deliberate human-approval step.The legacy FRAME_VERB path keeps its
isTrustedgate, since it carries no independent per-message security of its own (that's the actual spoofed-delivery risk #169 caught). A request with nofromDeviceat all is now refused before either path, rather than falling through to a room-verb handler that would calldeviceIdFromHexon a non-hex placeholder and throw.Also widens the hub gossip directory-merge filter to accept a device that is itself a trusted user principal (via
GatewayTrust.isTrustedPrincipal), not only one on the bare-device allowlist.Verification against the issue's own reasoning
The issue's decided fix assumed room-domain verb dispatch is safe to leave ungated at the gateway layer because it has its own independent capability-token verification. Reading the actual downstream code (
room-protocol.ts) confirmed this is true for an identified sender, but also surfaced a real gap the issue's own text didn't call out: every room-verb handler callsdeviceIdFromHex(handle.id)unconditionally to build the token's expected bearer (or, for room.join, the minted grant's own bearer). Under the old code, an unidentified sender (nofromDeviceon the relayed request) always fell into the coarseisTrustedrejection first, so this was never reachable. Removing that gate for room-domain verbs would have let an unidentified sender's request reach a handler that throws ondeviceIdFromHex("hub-peer"), an unhandled exception that kills the whole session's drain loop rather than answering with an ordinary error. Fixed by refusing any request with nofromDeviceat all before either dispatch path, restoring the intended fail-fast behaviour.Test plan
dispatchHubRequestcovering every branch (FRAME_VERB trusted/untrusted, room-domain trusted/untrusted, missingfromDevice, on-behalf-of forwarding, state_sync/state_update filtering)