diff options
| author | Christophe Besson <cbesson@gmail.com> | 2026-09-11 00:19:06 +0200 |
|---|---|---|
| committer | Christophe Besson <cbesson@gmail.com> | 2026-09-11 00:19:06 +0200 |
| commit | f059cb118c556d1f0279350507f74b8a47d5a98a (patch) | |
| tree | 9a97762a844038a06134b4b7dcead1758477dfc1 /docs/third-review.md | |
| parent | b045ba0010d69360b6a0265eb7c73a07900fe328 (diff) | |
| download | meshbay-f059cb118c556d1f0279350507f74b8a47d5a98a.tar.gz | |
docs: remove the documents MESHBAY_DESIGN.md replaces
Twenty-four files, about 17 000 lines: the two architecture drafts, the three
security reviews, eleven design notes, the roadmap, the decisions file, the v1–v4
archive, the deprecated user guide and the stale quickstart. Their content is in
MESHBAY_DESIGN.md, and git history holds the originals.
The reason to delete rather than keep bannered: a document that is superseded but
present still gets read, and a reader cannot always tell which of two accounts of
one mechanism is the live one. That was the argument for retiring the user guide
rather than repairing it, and it applies to the whole set.
What made this safe is the concordance. Roughly 290 comments and docstrings cite
these files by section — `musicbay.md §6`, `mediacenter.md §5.5`, `draft-v6 §2.11`
— and section 16 maps every one onto its replacement, so not a single comment
needs editing to stay followable. It now says plainly that the files are gone and
where to recover them, and it gained rows for the three reviews (their findings
are section 13), and for the two guides.
Four kept documents pointed into the set and were repointed first: `playlists.md`
(nine references — it is a live proposal and must not dangle), `WINDOWS-PORT.md`,
and CLAUDE.md's example. No dangling reference remains outside section 16.
Two files were dropped from the list after checking what they hold. `HTTPS.md` is
an operational runbook — Caddy, certificate renewal, DNS, troubleshooting — and
MESHBAY_DESIGN.md deliberately covers no operations, so nothing would replace it;
the versioned Caddyfile is the config, not the procedure. `cast-smart-tv.md` is
the plan for the unbuilt DLNA phase of a feature whose first two phases ship, and
section 11.4 summarises it in four lines rather than carrying the SSDP/UPnP work.
There is no user guide now, and section 0.1 says so rather than leaving a reader
to discover it.
Suites green: 2258 passed, 4 skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YVoHVCcfBqud6ZjG4db3y7
Diffstat (limited to 'docs/third-review.md')
| -rw-r--r-- | docs/third-review.md | 723 |
1 files changed, 0 insertions, 723 deletions
diff --git a/docs/third-review.md b/docs/third-review.md deleted file mode 100644 index 62ab5f6..0000000 --- a/docs/third-review.md +++ /dev/null @@ -1,723 +0,0 @@ -# MeshBay — Third Architecture & Security Review - -> **Superseded by `MESHBAY_DESIGN.md`.** This was the third security review; its design -> content now lives in §13.4, and the invariants each finding names throughout. -> -> It is kept because code comments, tests and other documents cite its -> sections and its labels, and because it records reasoning a synthesis -> compresses. **Where it disagrees with `MESHBAY_DESIGN.md`, the design -> document is right; where either disagrees with the code, the code is.** -> `MESHBAY_DESIGN.md` §16 maps every section reference here onto its -> replacement, and §13 defines every label. - -> Date: 2026-09-01 -> Scope: the code as it stands on `main` at `8a6294b`, with emphasis on what -> changed since `second-review.md` (2026-08-13): the unified handshake -> (`meshbay_common/handshake.py`), device linking, the invite/pairing rewrite, -> account recovery and passphrase change (`docs/auth-confirm.md`), email -> verification, reCAPTCHA, the hub instance-policy store, MHP federation, -> the community relay registry, chat link previews, the TMDB/MusicBrainz -> enrichment path, and the node's token-gated loopback control API. -> -> Method: code reading of `packages/`. The test suite was not run and no live -> testing was done against meshbay.org. This is a code and design review, not a -> penetration test. Finding numbers are independent of the first two reviews. -> -> The v5/v6 convention is kept: **a claim names the adversary it holds against.** -> The adversaries referenced below are the ones the project already uses — passive -> hub, active hub, malicious node operator, malicious group member, network -> attacker, local attacker — plus two the newer features introduce: **any -> registered hub user with no group membership**, and **a federated peer hub**. - ---- - -## 1. Executive summary - -**The critical and high findings from the second review have genuinely been -closed, and closed well.** The unified handshake is the right shape: one -length-prefixed, domain-separated, role-bound transcript; mandatory channel -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. 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 -rotation, email-at-rest encryption, and session/device teardown on passphrase -change are all present and correct. - -**What this review finds is a second generation of the same pattern:** new -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` 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.** -- **`POST /v1/reports`** was unauthenticated, unthrottled, and auto-blocked a - content hash after **two** reports — a network-wide censorship/DoS primitive - for anyone who learns a public file's blake3 id. **Fixed 2026-09-01** (auth, - rate limit, distinct-reporter counting, refused when public groups are off). -- The **registration reCAPTCHA** was inert: the server only checked it when - `auth_key` was absent, and the real web client always sends `auth_key`, so a - bot skipped it by including that field. **Fixed 2026-09-01** — gate is now - unconditional when a captcha is configured; the desktop client renders the - widget too. -- **Chat link previews** are a real SSRF surface (correctly identified as such in - the module) but the gate had gaps: no per-member rate limit, no port - restriction, and DNS rebinding a documented residual. **Fixed 2026-09-01** - (rate limit, port allowlist, connect-address re-check, bomb guard). -- **MHP federation** trusted any registered peer hub to push directory rows and - revocations, never checked the token audience, and the revocation-propagation - path was a silent no-op. **Fixed 2026-09-01** (source bound to the signer, - push capped, revocation acts on the peer's own directory entries, replay - rejected). -- There was **no CSP or security-header policy** on the hub-served SPA (second - review L5). **Fixed 2026-09-01** — a middleware applies the same policy the - desktop client already enforces on these files. Wants a pass against the - running SPA. - -None of this breaks the architecture. The cryptographic core and the trust model -are unchanged and still sound. H1, H2 and M1–M5 were -fixed on 2026-09-01; what is left is the L-list — opportunistic hardening, not a -hole — plus verifying the SPA CSP (M5) against the live app. - ---- - -## 2. What is solid (the delta since the second review) - -Worth recording, because the remediation was substantial and mostly correct: - -1. **`meshbay_common/handshake.py`** — one implementation, called by - `webrtc_server.py` and `quic_server.py`. `handshake_transcript()` is - length-prefixed and domain-separated (`meshbay:mnp:handshake:v1`), the role is - bound so a client proof can never be replayed as a node proof, and - `make_proof()` **raises** on an empty channel binding instead of degrading to - nonce-only (L4). `authorize_token()` enforces `scope == "user"` by default - (M9), requires `group_id` (M1), checks the denylist, the `groups` claim and - `hosted_groups`. -2. **Mutual authentication (C3).** `_complete_handshake` returns - `HMAC(GEK, node-transcript)` over the client's nonce **and** - `Ed25519(sk_node)` over the same transcript; `transport.js` verifies both - (`verifyNodeSignature`), refuses a bare `handshake_ack`, and TOFU-pins - `node_pk` in `localStorage` with an explicit change warning - (`_checkNodePin`). -3. **C1 deleted.** The per-group HTTP file API is gone from `daemon.py` - (step 9 is now a comment explaining why). Every client path goes through the - MNP handshake. -4. **C2 closed.** `_authorize_node_ws` resolves the node against the DB, checks - `scope == "node"`, checks `node.user_id == token.sub`, derives `group_ids` - from `GroupMember`, and refuses to displace a live registration. -5. **C5a closed.** Uploads: `SAFE_UPLOAD_NAME` allowlist, `_free_name()` - no-overwrite, `MAX_UPLOAD_BYTES` cap, strict chunk ordering, a quarantine - subdirectory, and an operator-signed `OP_MEMBER_UPLOAD` kill switch enforced - by the node (`_do_file_upload`), not by hiding a button. -6. **C5b closed.** `gek_bundle_store` is deleted; `gek_rotate` is an - operator-signed op where the node generates the key with its own CSPRNG - (`_admin_exec_gek_rotate` → `ops.set_gek(rotate=True)`). -7. **H1 (WebRTC) closed.** `_do_chat_message` / `_do_chat_history` read - `self._group_ctx().get("chat_store")`, `_peer_registry()` is per-group, and - `sender_id` is forced to `self._user_id`. -8. **H4 closed.** `Denylist` persists to `denylist.json`; `on_revocation` - handles `user`/`group`/`jti`, and `group` also drops live sessions - (`_drop_group_sessions`); `webrtc_offer` refuses when the shared group is not - `active`. -9. **H5 closed.** `adminop.admin_transcript()` — domain-separated, names the - operation, subject, node key, group, nonce and timestamp; `ADMIN_CHALLENGE_TTL` - 120 s; verified against `roster.operator_pks()` rebuilt from node state, never - from the response. -10. **H6 (WebRTC) closed.** 64 KB pre-handshake buffer, a transcode semaphore, - per-user pending-offer caps and a membership check in `signaling.py`, - `notify_incoming` requires `peer_ip == caller_ip`. -11. **H7 closed.** The swarm route is mounted correctly, nodes filter by - visibility, and `GET /v1/swarm/{hash}` requires auth. -12. **M8 closed.** `announce_node` requires a signed proof of possession. -13. **Device linking** (`_do_device_add`, `_verify_device_signer`): a new device - is admitted only by a signature from a **live pinned device of the same - account**; the one-time code never reaches the node (it lists candidate - hashes and the approver recomputes the match); requests are single-use and - capped by `MAX_DEVICES_PER_USER`. The hub holds no user keys and so cannot - countersign — this holds against an active hub. -14. **Account lifecycle** (`docs/auth-confirm.md`): passphrase change and reset - both revoke every refresh token; reset also deletes every `UserDevice` so a - stored device key cannot sign back in past the reset. The recovery key is a - pure client-side pass-through — never stored, never logged. - ---- - -## 3. High findings - -### H1 — A moderator can promote any account to admin (privilege escalation) - -> **Fixed 2026-09-01.** `admin_patch_user` now splits authorization by field: -> `status` between `active`/`suspended` stays at `require_moderator`; setting -> `role`, setting `status = "revoked"`, and touching an admin's account at all -> require `user_is_admin(current_user)` (new helper in `deps.py`). Regression -> test: `test_moderator_cannot_change_roles_or_revoke`. - -**Location:** `api/admin.py:185-241` (`admin_patch_user`), `api/deps.py:75-92` - -`PATCH /v1/admin/users/{user_id}` depends on `require_moderator`, but its body -accepts `role`, and the handler writes it with no check that the caller is an -admin: - -```python -if body.role is not None: - if body.role not in ("user", "moderator", "admin"): - raise HTTPException(status_code=422, ...) - user.role = body.role # ← moderator can set "admin" -``` - -The only guard is `user.id == current_user.id` ("Cannot modify your own -account"). So a moderator cannot self-promote directly, but can: - -- promote a second account they control, or an accomplice, to `admin`; -- **demote existing admins** to `user`, or set their `status` to `revoked`. - -`admin` is the real instance boundary: `admin_patch_settings` (public-groups -switch), `admin_delete_user` (irreversible erasure), `admin_revoke` -(user/group revocation broadcast to every node), `register_peer`, -`admin_add_blocklist`. A moderator reaching `admin` reaches all of it. - -**Impact.** Full instance takeover from the moderator role. Moderator is meant to -be a content-moderation role (suspend/revoke groups, read logs), not an -administrative one — `admin_delete_user`'s own docstring draws exactly that line -("Admin rather than moderator: suspension is reversible … this is not"). - -**Fix.** Split the handler: `status` changes among `active`/`suspended` stay at -`require_moderator`; `role` changes and `status = "revoked"` require -`require_admin`. Also forbid granting a role higher than the caller's, and forbid -demoting an equal-or-higher role. - ---- - -### H2 — Unauthenticated, unthrottled, permanent global content blocklisting - -> **Fixed 2026-09-01.** `POST /v1/reports` now requires a signed-in account -> (`get_current_user`), is rate-limited (`10/hour`), counts **distinct reporting -> accounts** (one vote per account per hash via `reporter_id`), and is refused -> outright (`403`) when the hub has public groups switched off — a private-only -> hub brokers no public content and nothing syncs the blocklist, so an open write -> endpoint there is pure abuse surface. `AUTO_BLOCK_THRESHOLD` raised 2 → 3. -> Tests rewritten in `test_moderation.py`. -> -> Note also confirmed while fixing: **no node currently consumes -> `ContentBlocklist`** — `GET /v1/blocklist` exists ("nodes sync on startup") but -> nothing fetches it, and `swarm_register` checks the *CSAM* list, not this one. -> So the network-wide censorship effect was latent (it activates when node sync -> ships); the DB-fill / poisoned-moderation-signal / admin-panel-garbage surface -> was live. The auto-block path should stay gated as above when sync lands. - -**Location:** `api/moderation.py:39,58-103` (`report_content`) - -`POST /v1/reports` has **no authentication and no rate limit**. It counts *all* -existing `ContentReport` rows for a hash — regardless of who filed them or from -where — and: - -```python -AUTO_BLOCK_THRESHOLD = 2 -... -if count + 1 >= AUTO_BLOCK_THRESHOLD: - ... db.add(ContentBlocklist(content_hash=..., added_by="auto")) -``` - -So **two unauthenticated HTTP requests** naming the same 64-hex blake3 id add -that id to `ContentBlocklist`. Nodes sync the blocklist -(`GET /v1/blocklist`, unauthenticated) and `swarm_register` refuses a blocked -hash with HTTP 451. Removal is a manual admin action -(`DELETE /v1/admin/blocklist/{hash}`). - -**Impact.** Anyone who learns the blake3 id of a public file — trivially, any -group member sees ids in the index; any registered user can probe -`GET /v1/swarm/{hash}` — can suppress that file across the whole network with two -anonymous requests. It is also a self-inflicted amplifier: one script can block -thousands of hashes. `content_hash` is the only validated field (`group_id`, -`reason`, `detail` are free-form and rendered in the admin UI). - -**Fix.** Require authentication on `POST /v1/reports`; dedupe reports by -`(content_hash, reporter)` so the threshold means *distinct* reporters; add a -rate limit; raise `AUTO_BLOCK_THRESHOLD` and/or make auto-block queue for human -review rather than take effect immediately; authenticate `GET /v1/blocklist` and -`/v1/blocklist/check` (node scope). - ---- - -## 4. Medium findings - -### M1 — The registration CAPTCHA is inert and trivially bypassed - -> **Fixed 2026-09-01 (Option A).** The server gate is now `if -> _cfg.captcha.enabled:` — no `auth_key` carve-out, no client exemption. The web -> client (`registerUser` in `keyderive.js`) forwards `captcha.token`, and the -> desktop client, being Chromium, renders the same widget from the shared UI -> assets. `captcha.reset()` is called on a failed attempt so the single-use -> token is refreshed. Tests: `test_register_captcha.py`. -> -> Consequence to check on the desktop side: the Electron CSP must allow -> `https://www.google.com` and `https://www.gstatic.com` for `script-src` / -> `frame-src`, or the widget will not render and the (already-disabled) submit -> button stays disabled. A headless/CLI `register` has no widget and is the one -> path with no human check — which is the path you would want gated anyway; a CLI -> can open a browser window for it. - -**Location:** `api/users.py:130-191` (`register`), `static/keyderive.js:279-303` -(`registerUser`), `static/auth-page.js:229-256` - -Server side: - -```python -# Captcha gate — web path only (native clients send auth_key) -if _cfg and _cfg.captcha.enabled and not body.auth_key: - await _verify_captcha_or_raise(body.captcha_token, request) -``` - -The CAPTCHA is checked **only when `auth_key` is absent**. But the real web -client's registration path (`window.MeshBayKeys` present, which is always) -calls `registerUser()`, which sends `{ username, email, auth_key }` and **no -`captcha_token`** at all. The branch that sends `captcha_token` -(`auth-page.js:248`) is a dead `else` for a client without `MeshBayKeys`. - -So: a human filling the Register form solves a reCAPTCHA whose token is never -transmitted and never checked, and a bot registers accounts at will by including -any `auth_key`-shaped string. `@limiter.limit("5/minute")` is the only remaining -brake (and see L10 for why that may also be weak). - -Password reset is unaffected — `password_reset_request` checks the CAPTCHA -unconditionally when enabled. - -**Fix.** Gate on `_cfg.captcha.enabled` alone (drop `and not body.auth_key`), and -have `registerUser()` include `captcha_token`. If native clients genuinely cannot -present one, gate on the *client type* explicitly (a header or a scope), not on -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 -port 19000, `groups=groups_ctx`). It got the new unified handshake — and the -GEK proof *is* implemented in `_do_handshake_response_sync`, so the docstring at -`quic_server.py:259-263` ("NOT YET DONE — finding C6 remains open on this -transport") is simply stale. But the chat and streaming handlers were never -brought up to the WebRTC path's rules: - -**M2a — `sender_id` is taken from the wire (NS6 regression).** - -```python -asyncio.ensure_future(chat_store.save_message( - sender_id=msg.get("sender_id", self._user_id), ...)) -... -broadcast = { ... "sender_id": msg.get("sender_id", self._user_id), ... } -``` - -An authenticated QUIC peer can post chat as any `sender_id`. The WebRTC path -forces `sender_id=self._user_id` (`webrtc_server.py:3493,3504`). - -**M2b — the peer registry is connection-global, not per-group (H1 regression).** -`_do_chat_message_sync` broadcasts to `self._ctx.get("_peers", {})`, which is a -single dict on the `QuicChunkServer` instance shared across every group. A member -of group A, connected over QUIC, has their (spoofable) message fanned out to -QUIC peers of every other group on the node. (`chat_store` is never set in the -QUIC ctx, so messages are dropped rather than persisted — but still broadcast.) - -**M2c — synchronous ffmpeg on the event loop, no concurrency cap (H6 -regression).** `_do_stream_segment_sync` → `_extract_segment` runs -`subprocess.run([... "ffmpeg" ...], timeout=30)` directly inside -`quic_event_received`. One request blocks the whole node for up to 30 s; there is -no transcode semaphore. `_do_file_request_sync` likewise does blocking file I/O -in the loop. - -**Impact.** Limited to peers reachable over QUIC/UDP 19000 — LAN, a -port-forwarded node, or hub-less `group://` — and to native clients (browsers use -WebRTC, a separate registry). Still: chat impersonation and cross-group leakage -against exactly the "malicious group member" adversary the WebRTC fixes were -written for, plus a one-request node stall. - -**Fix.** Route the QUIC chat and stream handlers through the same per-group -context and `sender_id`-from-session logic as WebRTC (ideally shared helpers in -`meshbay_common`, the same move that fixed C6); make `_extract_segment` async and -put it behind the transcode semaphore, or disable the QUIC `STREAM_SEGMENT` -handler until it is at parity. Update the stale docstring. - ---- - -### M3 — Chat link previews: SSRF gate has no rate limit, no port restriction, and a known rebinding hole - -> **Fixed 2026-09-01.** -> - **Rate limit:** `_do_link_preview_request` is now bounded per connection (15) -> and node-wide (60) over a 60 s window; a cache hit is free, and over the -> ceiling the reply is a plain `ok: false` (bare link), not cached. -> - **Port allowlist:** `safe_url()` restricts the port to `{80, 443, 8080, -> 8443}` — every real OpenGraph page, none of the SSH/mail/DB/cache/search -> ports. Deliberately not just 80/443, to keep legitimate sites on alt-HTTP -> working. -> - **DNS rebinding:** the connection's actual peer address is re-checked -> against the public-address rule (`_reject_if_rebound`) before the body is -> read. Best-effort — a full literal-pin with cert-for-name is noted as -> remaining hardening. -> - **Decompression bomb:** `_downscale` refuses an image whose header -> dimensions exceed ~40 MP before decode. -> -> Not extended to `MEDIA_META_REQ` / `TMDB_SEARCH_REQ`: those reach a fixed -> host, so they carry a quota concern but not an SSRF one — left for a separate -> pass. - -**Location:** `node/linkpreview.py`, `webrtc_server.py:3588-3636` -(`_do_link_preview_request`) - -The design is right — the *node* fetches, not the browser or the hub — and -`safe_url()` blocks non-http(s) schemes, embedded credentials, and any resolved -address that is not globally routable, re-checking every redirect hop by hand. -But: - -1. **No rate limit / no per-member cap.** `_do_link_preview_request` is reachable - by any group member after the handshake, and the in-memory cache - (`_LINK_PREVIEW_MAX = 256`, TTL 1 h) only dedupes exact repeats. A member - pasting many distinct URLs drives unbounded outbound HTTP from the operator's - machine — an amplification/DoS vector and a way to disclose the operator's IP - to arbitrary hosts on demand. -2. **Port is not restricted.** `safe_url()` validates the scheme and the resolved - IP but passes `parts.port` straight through. A member can point the node at - `http://<globally-routable-ip>:<any-port>` — third-party port scanning from - the operator's address, and reaching services that are internet-routable but - firewalled to the node's network. -3. **DNS rebinding.** `safe_url()` resolves and checks the address, then - `httpx.get()` resolves again at connect time. The module documents this as a - deferred residual ("closed properly by pinning the checked IP"). Until the - pin lands, a name that answers public on check and internal on connect is a - way in. -4. **Image decode.** `fetch_image` → `_downscale` opens attacker-supplied bytes - with Pillow; `Image.open` + `thumbnail` after a full decode. Pillow's default - decompression-bomb guard applies, but a 2 MB input is allowed and the guard is - the only ceiling. - -**Fix.** Add a per-connection and per-node rate limit on `LINK_PREVIEW_REQ` -(and the same for `MEDIA_META_REQ` / `TMDB_SEARCH_REQ`); restrict the port to -80/443; pin the checked IP for the actual connection (resolve once, connect to -the literal, send `Host:`); set `PIL.Image.MAX_IMAGE_PIXELS` low and cap decoded -dimensions before `thumbnail`. - ---- - -### M4 — MHP federation: peer hubs are over-trusted, token audience is unchecked, revocation propagation is a no-op - -> **Fixed 2026-09-01.** -> - `receive_directory` binds `source_hub` to the token's verified `iss`, so a -> peer cannot relay or spoof a third hub's groups; the push is capped -> (500/request, 2000/peer), rows are type/length-checked, and a federated id -> that collides with a local group is refused. -> - `receive_revocation` no longer forwards a foreign-signed token to local nodes -> (the no-op). It verifies the inner token against the sending peer's key and, -> for `target == "group"`, prunes our copy of that peer's directory entry — a -> peer cannot revoke our users or a group it did not advertise. -> - `POST /mhp/directory` and `/mhp/revoke` reject a replayed `jti` within the -> token TTL. Audience binding is still unavailable (the sending side that would -> set `aud` is unbuilt); the replay check covers that concern for now. Tests in -> `test_federation.py`. - -**Location:** `api/federation.py:59-190`, `api/revocation.py:66-80` (node -`verify_and_apply`), `node/daemon.py:575-596` (`on_revocation`) - -1. **Audience never verified.** `_verify_mhp_token` accepts an optional - `expected_aud` but **no caller passes it** — `export_directory`, - `receive_directory` and `receive_revocation` all call - `_verify_mhp_token(token, db)`. `_issue_mhp_token` sets `aud = target_hub_id`, - so the check is available and deliberately unused. A token hub B minted for - hub C (valid 300 s) is replayable at any other hub that has B registered as a - peer. -2. **Any registered peer can inject the directory.** `receive_directory` iterates - an unbounded `body.groups` list with peer-chosen `id` and `name`, upserting - `FederatedGroup` rows. No cap, no validation. `name` is rendered in the SPA - Explore view; `id` is peer-chosen and shares the UUID space with local groups. -3. **Revocation propagation does nothing on nodes.** `receive_revocation` passes - the peer's token straight to `broadcast_revocation`, which forwards it to - local nodes. Nodes verify a revocation token against **their own hub's public - key** (`session.hub_pk_pem`), so a peer-signed token fails - `jwt.decode(...)` and is dropped with a warning. The federated - `/mhp/revoke` path therefore silently accomplishes nothing — false assurance - that "revocations propagate" across a federation. - -**Impact.** A malicious or compromised peer hub can flood/poison the local public -directory and cannot be relied on to actually revoke anything. Cross-hub token -replay within a federation. All of this is bounded by the admin having explicitly -run `POST /mhp/peers` — federation is opt-in and manual — so the adversary is "a -peer the admin chose to trust", which is exactly the adversary MHP's own auth is -supposed to constrain. - -**Fix.** Pass `expected_aud=_hub_id` in every `_verify_mhp_token` call; cap -`body.groups` and validate each row; namespace `FederatedGroup.id` or refuse an -`id` that collides with a local group; for revocation, either re-sign accepted -peer revocations with the local hub key before broadcasting (with a policy on -which peers may revoke which targets) or drop the endpoint and document that -revocation does not federate. - ---- - -### M5 — No CSP or security headers on the hub-served SPA (second review L5, still open) - -> **Fixed 2026-09-01.** A middleware in `create_app` adds `Content-Security-Policy`, -> `X-Content-Type-Options: nosniff`, `Referrer-Policy` and `X-Frame-Options: DENY` -> to every response. `webapp.CSP` is the same policy the desktop client already -> enforces on these exact UI files (`default-src 'none'`, `script-src 'self' -> 'wasm-unsafe-eval' <recaptcha>` — the hub origin is not a script source, -> `frame-ancestors 'none'`, `base-uri 'none'`, `form-action 'none'`), plus the -> reCAPTCHA hosts. The shell's dead `window.__MB_ASSET_V` inline script is -> removed so no inline `'unsafe-inline'`/nonce is needed for scripts. **Wants a -> pass against the running SPA** — a mis-tuned CSP shows as a blank page — but it -> matches a policy already proven with these files under Electron. SRI on the -> `/a/<hash>/` scripts is still not done (same-origin, so lower value than the -> CSP). Tests in `test_security_headers.py`. - -**Location:** `api/webapp.py:80-123`, `app.py:160-214` - -The SPA shell is returned with only `Cache-Control: no-store`. There is no -`Content-Security-Policy`, `X-Content-Type-Options: nosniff`, `Referrer-Policy`, -`X-Frame-Options` / `frame-ancestors`, and no Subresource Integrity on the -scripts loaded from `/a/<hash>/` (including the vendored `argon2.min.js`). The -hub sets no CORS middleware (correct) but also no protective headers at all. - -For an application whose threat model explicitly includes "the hub could inject -JS" (T3) and which now renders third-party OpenGraph images and TMDB/MusicBrainz -metadata inside the group UI, a strict CSP (`default-src 'none'`, an explicit -`connect-src`/`img-src`, `frame-ancestors 'none'`, `base-uri 'none'`) plus SRI is -the cheap mitigation that makes a *silent* injection harder and gives a browser -extension something to pin against. The node's loopback API already sets exactly -this kind of header block (`ui/app.py:100-117`); the hub does not. - -**Fix.** Add a response-header middleware on the hub with a strict CSP for the -SPA routes and `nosniff`/`Referrer-Policy`/`frame-ancestors` globally; add SRI -hashes to the `<script>` tags in `_HTML` (the content hash is already computed). - ---- - -### M6 — `add_group_member` accepts node-scoped tokens — WITHDRAWN - -> **Not a finding. Fixed then reverted 2026-09-01.** -> -> `add_group_member` accepting a node-scoped token is **deliberate** (commit -> `0443cf8`): the node calls `POST /v1/groups/{id}/members/{username}` after a -> CLI `member invite` so the group becomes visible in the invitee's SPA, and it -> authenticates with a node-scoped token. The `group.admin_id == caller` check -> is the real guard — a node can only touch its own operator's groups, adding an -> already-registered account. -> -> This review misread that as drift (a stale test, -> `test_node_scope_blocks_add_member`, asserted the opposite and had been left -> red on `main`). Tightening the dependency to `require_user_scope` **broke the -> CLI invite → hub-membership flow**: `ops.create_invite` swallows the resulting -> 403 with a `log.warning`, so an invited user silently never appears in -> `group_members` and the group is invisible to them. Found in live testing. -> The dependency is back on `get_current_user` and the stale test now asserts -> the intended behaviour (node token may add to its own group, 403 for a group -> it does not own). - -The original M6 concern — a stolen node token adding accounts to the operator's -own groups — is real but low: bounded to the operator's groups, existing -accounts only, and it is now a *required* capability. If it is ever worth -constraining, it needs a dedicated node→membership path, not a scope block on -this shared endpoint. - ---- - -## 5. Low findings / notes - -- **L1 — Community relay registration is unauthenticated and has no proof of - possession.** `api/relay.py:57-78` — `relay_register` only compares the - submitted `pk_relay` against the admin-approved value (both effectively - public); there is no signature over anything. Anyone who knows an approved - `relay_id` + `pk_relay` can repoint that relay's `endpoint`. `GET /v1/relays` - is unauthenticated. `_relays` is in-memory (lost on restart). Relay traffic is - E2E-encrypted, so the impact is redirection / forced-relay / DoS rather than - disclosure — but a registry write should require the relay to sign a fresh - challenge with `sk_relay`, on the `announce_node` pattern. - -- **L2 — Two orphaned modules invite the wrong wiring later.** - `node/replication.py` (`ContentReplicator`) fetches `{endpoint}/index` and - `{endpoint}/file/{id}` — the HTTP API removed by the C1 fix — and joins an - attacker-controlled `file_name` directly onto a local path - (`self._local_dir / file_name`, path traversal) with an unbounded - `read_bytes()`. `node/revocation.py` (`RevocationSubscriber`) is superseded by - `hub_client` + `daemon.on_revocation` and keeps a weaker in-memory-only model - with `verify_exp=False`. Neither is imported by the daemon. Delete both, or - fold `replication.py` into the MNP path with proper name sanitisation before - it is revived. - -- **L3 — Account / email enumeration is wider than before.** `register` and - `PATCH /me` return `409 "Email already in use"`; `register` returns - `409 "Username already taken"`; `GET /v1/users/{username}/pubkeys` returns 404 - vs 200. Together these enumerate which usernames and which email addresses have - accounts. `login`, `device_auth` and `password_reset_request` are correctly - uniform — the leak is on the account-management endpoints. Decide whether this - is acceptable by design (public identities) and document it; if not, make the - responses uniform and move the "email in use" signal into the verification - email instead. - -- **L4 — No aggregate upload quota.** `MAX_UPLOAD_BYTES` caps a single file at - 4 GB (C5a), but there is no per-user or per-group total. A member — or an - active hub minting tokens for many synthetic accounts — can still fill the - operator's disk one 4 GB file at a time. Second review H6 asked for quotas; - only the per-file cap landed. - -- **L5 — Node control-API token accepted in the query string.** - `ui/app.py:92-98` accepts `?t=<token>` as well as the `X-MeshBay-Token` - header. Tokens in URLs end up in access logs and process listings. The - loopback bind + `0600` token file already blunt this, and DNS-rebinding is - covered (a rebound page cannot read the on-disk token) — but header-only would - be cleaner. - -- **L6 — Revocation tokens never expire.** `_sign_revocation` sets no `exp`; - both `on_revocation` and the dead `RevocationSubscriber` decode with - `verify_exp=False`. This is fine for "deny forever" semantics, but a captured - revocation token is replayable indefinitely, there is no positive un-revoke - signal, and clearing is a manual operator action (`denylist clear`). At least - bind a `revoked_at` freshness window on replay, or a monotonic sequence per - hub. - -- **L7 — The CSAM check is exact-hash only.** `csam.py` compares blake3 hex - against a (shipped-empty) newline list; production CSAM databases are - perceptual (PhotoDNA). `check_content_hash` is called from `swarm_register` - only. The control is structural, not yet functional — worth stating in the - docs so it is not relied on operationally. - -- **L8 — Group names are unvalidated.** `create_group` trims the name but - enforces no length or character set (only per-owner uniqueness). The name - flows into emails (`send_invite_notification`), notifications, and the SPA. - Python's `email` package will reject control characters at send time, so header - injection is contained, but a length/charset check at creation is cheap. - -- **L9 — `decode_access_token` / `authorize_token` do not `require=["exp"]` or - check `iss`/`aud`.** Every token the hub issues carries `exp`, so this is - latent — but a future path that mints a token without `exp` would produce a - non-expiring JWT accepted on every transport and every hub endpoint. Pass - `options={"require": ["exp"], "verify_aud": ...}` and bind an audience for the - node vs. user token split. - -- **L10 — Rate-limit keying depends on unpinned proxy-header handling.** - `middleware.py` uses `slowapi`'s `get_remote_address`, which returns - `request.client.host`. Behind the loopback Caddy proxy this is the real client - IP **only** if uvicorn's `ProxyHeadersMiddleware` is active and Caddy sets - `X-Forwarded-For`. `daemon.py` calls `uvicorn.run()` without an explicit - `proxy_headers` / `forwarded_allow_ips`, so this relies entirely on uvicorn's - defaults and the Caddy config, neither pinned in the repo. If it regresses, - every `@limiter.limit` collapses to a single global bucket keyed on - `127.0.0.1` — no per-client throttling, and one abuser starves everyone. Use - the `netutil.client_ip` helper (the M7 fix) as the limiter's `key_func` so the - behaviour is defined in one place. - -- **L11 — `client_diag` and `X-Forwarded-For` log hygiene.** `client_diag` is - correctly truncated/stringified before logging. `netutil.client_ip` correctly - takes the rightmost hop behind a trusted proxy. No action — noting that the - M7 pattern is applied consistently *except* by the rate limiter (L10). - ---- - -## 6. Does the system do what it claims? (2026-09-01) - -Against the v6 §4 claims, updated for this review: - -| Claim | Passive hub | Active hub | Malicious member | Notes | -|---|---|---|---|---| -| 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 | ✅ | ✅ | ✅ 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. Federation prunes the peer's directory entry (was M4); it does not reach nodes, and nothing local hosts a federated group | -| 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 every transport (M2a's wire-asserted QUIC path was closed 2026-09-01) | -| Node does not emit traffic on a member's behalf | — | — | ✅ since 2026-09-01 — link previews rate-limited, ports restricted, connect-address re-checked (was 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 path, the second review's critical gaps are -closed, and H1/H2/M1–M5 were fixed the day this was written (M6 was withdrawn as -a misread of a deliberate design) — leaving the L-list as opportunistic -hardening and one thing to verify: the SPA's new CSP against the live app.* - ---- - -## 7. Prioritised action plan - -| # | Finding | Severity | Effort | When | -|---|---|---|---|---| -| 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 | ✅ **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 | ✅ **fixed 2026-09-01** — rate limit + port allowlist + connect-address re-check + bomb guard | -| M4 | Federation: peer over-trust, `aud` unchecked, revoke no-op | Medium | M | ✅ **fixed 2026-09-01** — source bound to signer, push capped, revoke prunes the peer's own entries, replay rejected | -| M5 | No CSP / security headers on the SPA | Medium | S | ✅ **fixed 2026-09-01** — CSP + `nosniff` + `frame-ancestors` middleware; verify against the live SPA | -| M6 | `add_group_member` accepts node tokens | — | — | ⛔ **withdrawn** — deliberate (CLI invite flow, commit 0443cf8); the "fix" broke it and was reverted | -| L1 | Relay registration no PoP | Low | S | If/when the relay registry is used | -| L2 | Orphaned `replication.py` / `revocation.py` | Low | S | Delete now | -| L3–L11 | See §5 | Low | S | Opportunistic | - -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 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, - `POST /v1/reports`, `receive_directory` — each added a way for a low-privilege - party (a member, an anonymous caller, a peer hub) to make the node or hub do - work or accept state, and each shipped without a bound on how much. The - 2026-09-01 fixes added the bounds to reports and link previews; - `MEDIA_META_REQ` / `TMDB_SEARCH_REQ` still want a quota, and - `receive_directory` a cap. - ---- - -## 8. Conclusion - -The architecture is unchanged and still correct, and the remediation since -2026-08-13 was real: the unified handshake, the deletion of the node HTTP API, -mutual node authentication with client-side pinning, the invite/pairing rewrite, -device linking, and the account-recovery design are all solid security -engineering, and most of the second review's C- and H-list is genuinely closed. - -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, a transport that -received the new authentication but not the new authorization, an over-trusted -federation peer, and a missing header policy. None of them 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 and M1–M5 were fixed the day this was written; M6 was withdrawn — it -misread the node registering a hub membership during the CLI invite flow -(deliberate, commit `0443cf8`) as authorization drift, and the "fix" broke that -flow in live testing. What is left is the L-list — opportunistic hardening — and -one verification: the SPA's new CSP (M5) against the running app, since a -mis-tuned CSP shows as a blank page. On a build whose honest claims are now -strong and largely -defensible. |