aboutsummaryrefslogtreecommitdiffstats
path: root/docs/third-review.md
diff options
context:
space:
mode:
authorChristophe Besson <cbesson@gmail.com>2026-09-01 18:59:04 +0200
committerChristophe Besson <cbesson@gmail.com>2026-09-01 18:59:04 +0200
commit8685ec09a658157f005925f54832e7a846b5366b (patch)
tree30eae21a294cb98996c8da32fda5dd4b402d7611 /docs/third-review.md
parentf8598f57222028ba5600c9be507394dbcbb6f459 (diff)
downloadmeshbay-8685ec09a658157f005925f54832e7a846b5366b.tar.gz
docs: mark M2 fixed in the third security review
QUIC chat/stream handlers brought to WebRTC parity, and the QUIC listener gated off by default. Executive summary, findings table and action plan updated; the original M2 finding text is kept for the record. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG
Diffstat (limited to 'docs/third-review.md')
-rw-r--r--docs/third-review.md67
1 files changed, 40 insertions, 27 deletions
diff --git a/docs/third-review.md b/docs/third-review.md
index 6194eff..b6f8d3c 100644
--- a/docs/third-review.md
+++ b/docs/third-review.md
@@ -30,8 +30,7 @@ binding; a mutual proof where the node demonstrates GEK possession over the
client's nonce *and* signs the transcript with its long-term key; `scope="user"`
enforced by default; `group_id` mandatory. It is now run by **both** the WebRTC
and the QUIC transports — the C6 divergence that produced most of the second
-review is structurally gone (only a stale docstring in `quic_server.py` still
-says otherwise). C1 (the unauthenticated node HTTP file API) was deleted outright
+review is structurally gone. C1 (the unauthenticated node HTTP file API) was deleted outright
rather than patched. C2, C3, C5a, C5b, H1, H4, H5, H6, H7, M7, M8 are all
addressed in the code, and the invite rewrite closed H3/M3. Device linking,
the password split, Argon2id-256 MB on the hub verifier, refresh-token family
@@ -43,10 +42,12 @@ surface was added faster than the authorization model was extended to cover it,
and a few of the second-review fixes did not reach every path.
- The **QUIC transport** got the new handshake but not the new *chat* rules:
- `_do_chat_message_sync` still takes `sender_id` from the wire (NS6) and
- broadcasts through a connection-global peer registry regardless of group (H1),
- and still runs a 30-second synchronous `ffmpeg` on the event loop with no
- concurrency cap (H6). The WebRTC path fixed all three.
+ `_do_chat_message_sync` took `sender_id` from the wire (NS6), broadcast
+ through a connection-global peer registry regardless of group (H1), and ran a
+ 30-second synchronous `ffmpeg` on the event loop with no concurrency cap (H6).
+ **Fixed 2026-09-01** — handlers brought to WebRTC parity, and the QUIC
+ listener is now off by default (`[node] quic_enabled`) since nothing ships a
+ QUIC client.
- The **hub moderation surface** had a privilege-escalation hole: a *moderator*
could promote any other account to *admin* (`PATCH /v1/admin/users/{id}` was
gated by `require_moderator` but wrote `role`). **Fixed 2026-09-01.**
@@ -70,11 +71,10 @@ and a few of the second-review fixes did not reach every path.
OpenGraph images and metadata.
None of this breaks the architecture. The cryptographic core and the trust model
-are unchanged and still sound. H1, H2, M1 and M6 were fixed on 2026-09-01; the
-QUIC chat path (M2) should be treated as blocking for any deployment where
-native/QUIC peers share a node, and the link-preview SSRF (M3) should be bounded
-before that feature runs on an internet-facing node. What remains on the hub
-(M4 federation, M5 headers, the L-list) is hardening, not a hole.
+are unchanged and still sound. H1, H2, M1, M2 and M6 were fixed on 2026-09-01.
+The link-preview SSRF (M3) should be bounded before that feature runs on an
+internet-facing node; what remains beyond it (M4 federation, M5 headers, the
+L-list) is hardening, not a hole.
---
@@ -288,6 +288,19 @@ the presence of a field any caller can supply.
### M2 — QUIC transport: chat sender spoofing, cross-group broadcast, and a blocking ffmpeg
+> **Fixed 2026-09-01, two ways.**
+> 1. **Exposure removed:** the QUIC listener is now off by default —
+> `[node] quic_enabled = false` (`MESHBAY_QUIC_ENABLED` overrides), gated in
+> `daemon.py`. Nothing ships a QUIC client, so a node started none for no one.
+> 2. **Handlers brought to parity** anyway, for when a client does exist:
+> `_do_chat_message_sync` forces `sender_id` from the authenticated session
+> (M2a), resolves `chat_store` and the peer set per group via `_group_ctx()` /
+> `_peer_registry()` (M2b, kept separate from the WebRTC peer set), and
+> `STREAM_SEGMENT` extraction runs in a thread behind a small semaphore as a
+> tracked task (M2c). Stale C6 docstring corrected.
+>
+> Original finding text kept below for the record.
+
**Location:** `transport/quic_server.py:220-247, 402-463, 509-525`
The QUIC server is started in production (`daemon.py:540`, `host="::"`, default
@@ -562,22 +575,21 @@ Against the v6 §4 claims, updated for this review:
| Data never transits the hub | ✅ | ✅ | — | WebRTC/QUIC P2P; hub relays SDP only |
| Hub stores no content/index/chat | ✅ | ✅ | — | Confirmed in schema; `hub_settings` is instance policy, not group content |
| File content unreadable by the hub | ✅ | ✅ for content | — | GEK never reaches the hub; invite rewrite closed H3 |
-| Node operator is sole content authority | ✅ (WebRTC) | ✅ (WebRTC) | ⚠️ **QUIC**: chat `sender_id` spoof + cross-group broadcast (M2) | WebRTC path is sound; QUIC chat path is not |
+| Node operator is sole content authority | ✅ | ✅ | ✅ since 2026-09-01 — QUIC chat/stream handlers brought to WebRTC parity, and the QUIC listener is off by default (was M2) |
| Mutual node authentication | ✅ | ✅ | ✅ | New handshake + `transport.js` pin — a real improvement |
| Immediate revocation | ✅ | ✅ locally | — | Persisted denylist, group targets handled. **Does not federate** (M4) |
| Suspending/revoking a group blocks connections | ✅ | ✅ | — | `webrtc_offer` checks status; node drops sessions on `revoke` |
| Device linking safe against the hub | ✅ | ✅ | ⚠️ browser link inherits T3 (documented) | Countersignature by a pinned device; hub holds no user keys |
-| Chat authenticated between members | ❌ not yet | ❌ | ❌ | Sender Keys is Phase 15; today chat is node-asserted on WebRTC and **wire-asserted on QUIC** (M2a) |
+| Chat authenticated between members | ❌ not yet | ❌ | ❌ | Sender Keys is Phase 15; today chat is node-asserted on every transport (M2a's wire-asserted QUIC path was closed 2026-09-01) |
| Node does not emit traffic on a member's behalf | — | — | ⚠️ link previews: unthrottled, ports open, rebinding (M3) |
| Hub cannot be used to censor content | — | — | ✅ since 2026-09-01 — `POST /v1/reports` needs auth, distinct reporters, public groups on (was H2) |
| Moderator ≠ administrator | ✅ | — | — | ✅ since 2026-09-01 — `admin_patch_user` split by field (was H1) |
**One-sentence version:** *the E2E story between browser and node is now
-genuinely mutual and covers every WebRTC path — the second review's critical gaps
-are closed, and H1/H2/M1/M6 were fixed the day this was written — leaving the
-QUIC transport's chat handlers (which regress NS6/H1/H6) as the one item that
-should block a shared-node deployment, and the link-preview SSRF as the one to
-bound before that feature faces the internet.*
+genuinely mutual and covers every path — the second review's critical gaps are
+closed, and H1/H2/M1/M2/M6 were fixed the day this was written — leaving the
+link-preview SSRF (M3) as the one item to bound before that feature faces the
+internet, and MHP federation / SPA headers as hardening.*
---
@@ -588,7 +600,7 @@ bound before that feature faces the internet.*
| H1 | Moderator can write `role` → admin | High | S | ✅ **fixed 2026-09-01** — handler split by field |
| H2 | Unauthenticated 2-report global blocklist | High | S | ✅ **fixed 2026-09-01** — auth + distinct-reporter + rate limit + public-groups gate |
| M1 | Registration CAPTCHA inert | Medium | S | ✅ **fixed 2026-09-01** — gate unconditional; desktop renders the widget |
-| M2 | QUIC chat: `sender_id` spoof, cross-group broadcast, sync ffmpeg | Medium | M | Before any deployment where native/QUIC peers share a node |
+| M2 | QUIC chat: `sender_id` spoof, cross-group broadcast, sync ffmpeg | Medium | M | ✅ **fixed 2026-09-01** — handlers at WebRTC parity + `quic_enabled` off by default |
| M3 | Link-preview SSRF: no rate limit, ports open, rebinding | Medium | M | Before enabling link previews on an internet-facing node |
| M4 | Federation: peer over-trust, `aud` unchecked, revoke no-op | Medium | M | Before enabling MHP with any non-self peer |
| M5 | No CSP / security headers on the SPA | Medium | S | Opportunistic — cheap, high value given T3 |
@@ -601,9 +613,11 @@ Two structural recommendations, both echoing the second review:
1. **Make transport parity a test, not a habit — again.** C6 was fixed by moving
the handshake into `meshbay_common`; the *chat* and *stream* handlers were
- not moved, and M2 is the result. Lift `sender_id` enforcement, per-group
- context resolution, and the transcode gate into shared helpers, with a test
- that fails if a transport calls a chat/stream path that bypasses them.
+ not moved, and M2 was the result. The 2026-09-01 fix mirrored the WebRTC
+ logic into `quic_server.py` by hand — the durable version is shared helpers
+ in `meshbay_common` (`sender_id` enforcement, per-group context, the
+ transcode gate) with a test that fails if a transport calls a chat/stream
+ path that bypasses them.
2. **Every new outbound or cross-trust surface needs a rate limit and an
adversary named in the same commit.** Link previews, TMDB/MusicBrainz search,
@@ -625,11 +639,10 @@ The new findings are narrower and more uniform in shape than last time: a role
check that grants too much, an anti-abuse endpoint with no abuse protection, a
CAPTCHA wired to a condition the real client never meets, and a transport that
received the new authentication but not the new authorization. None of them
-require exotic capability, and none of them are architectural — they are the
+required exotic capability, and none of them were architectural — they were the
cost of adding six subsystems faster than the authorization model grew to cover
them.
-H1, H2, M1 and M6 were fixed the day this review was written. The build should
-not host real private data while M2 stands, and link previews should stay off on
-an internet-facing node until M3 is bounded. Past those, the honest claims are
-strong ones and are now largely defensible.
+H1, H2, M1, M2 and M6 were fixed the day this review was written. Link previews
+should stay off on an internet-facing node until M3 is bounded. Past that, the
+honest claims are strong ones and are now largely defensible.