docs: add third security review (2026-09-01)

Code-level review focused on what changed since second-review.md:
the unified handshake, device linking, account recovery, email
verification, reCAPTCHA, the hub instance-policy store, MHP
federation, the relay registry, chat link previews, and the node's
loopback control API.

The second review's critical/high list is confirmed closed. New
findings H1, H2, M1 and M6 are fixed in the preceding commits and
annotated as such; M2 (QUIC chat handlers regress NS6/H1/H6), M3
(link-preview SSRF), M4 (federation trust), M5 (no SPA CSP) and the
L-list remain.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG

1 files changed, 635 insertions, 0 deletions

diff --git a/docs/third-review.md b/docs/third-review.md
new file mode 100644
index 0000000..6194eff
--- /dev/null
+++ b/docs/third-review.md
@@ -0,0 +1,635 @@
+# MeshBay — Third Architecture & Security Review
+
+> 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 (only a stale docstring in `quic_server.py` still
+says otherwise). 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` 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.
+- 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 has gaps: no per-member rate limit, no port
+  restriction, and DNS rebinding is left as a documented residual.
+- **MHP federation** trusts any registered peer hub to push directory rows and
+  revocations, never checks the token audience, and the revocation-propagation
+  path is a silent no-op because nodes verify only against their own hub's key.
+- There is still **no CSP or security-header policy** on the hub-served SPA
+  (second review L5), which matters more now that the SPA renders third-party
+  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.
+
+---
+
+## 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
+
+**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
+
+**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
+
+**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)
+
+**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
+
+> **Fixed 2026-09-01.** Dependency changed to `require_user_scope`. The
+> regression test `test_node_auth.py::test_node_scope_blocks_add_member` already
+> existed and was red on `main`; it now passes.
+
+**Location:** `api/groups.py:585-619`
+
+Every mutating group endpoint depends on `require_user_scope` — except
+`POST /v1/groups/{group_id}/members/{username}`, which depended on
+`get_current_user`. `deps.py` states plainly: "Node-scoped tokens CANNOT
+create/delete groups or manage membership." This one could: a node-scoped daemon
+token (or a stolen one) whose subject owns the group could add any existing user
+to it. Impact was bounded (own groups, existing accounts only), but it
+contradicted NS7 ("operator manages groups from the browser only") and was the
+same class as the second review's M9. The test asserting the intended behaviour
+was already in the tree — the endpoint had simply drifted off the shared
+dependency.
+
+---
+
+## 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 | ✅ (WebRTC) | ✅ (WebRTC) | ⚠️ **QUIC**: chat `sender_id` spoof + cross-group broadcast (M2) | WebRTC path is sound; QUIC chat path is not |
+| 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) |
+| 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.*
+
+---
+
+## 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 | Before any deployment where native/QUIC peers share a node |
+| 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 |
+| M6 | `add_group_member` accepts node tokens | Medium | S | ✅ **fixed 2026-09-01** — dependency → `require_user_scope` |
+| 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 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.
+
+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.
+
+---
+
+## 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, 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
+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.