diff options
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. |