From 73a119cbc7641b5f0005d0ce8bc4c7b0b7ae6a6e Mon Sep 17 00:00:00 2001
From: Christophe Besson <cbesson@gmail.com>
Date: Tue, 1 Sep 2026 18:18:05 +0200
Subject: 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
---
 docs/third-review.md | 635 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 635 insertions(+)
 create mode 100644 docs/third-review.md

(limited to 'docs/third-review.md')

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.
-- 
cgit v1.2.3


From 8685ec09a658157f005925f54832e7a846b5366b Mon Sep 17 00:00:00 2001
From: Christophe Besson <cbesson@gmail.com>
Date: Tue, 1 Sep 2026 18:59:04 +0200
Subject: docs: mark M2 fixed in the third security review

QUIC chat/stream handlers brought to WebRTC parity, and the QUIC
listener gated off by default. Executive summary, findings table and
action plan updated; the original M2 finding text is kept for the
record.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG
---
 docs/third-review.md | 67 +++++++++++++++++++++++++++++++---------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

(limited to 'docs/third-review.md')

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


From 9ee9d7dfca50a64257163236f11b3cd58021963f Mon Sep 17 00:00:00 2001
From: Christophe Besson <cbesson@gmail.com>
Date: Tue, 1 Sep 2026 19:11:20 +0200
Subject: docs: mark M3 fixed in the third security review

Link-preview SSRF surface bounded: per-connection + node-wide rate
limit, port allowlist, connect-address re-check, decompression-bomb
guard. Summary, findings table and action plan updated; original M3
text kept for the record.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG
---
 docs/third-review.md | 53 +++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 15 deletions(-)

(limited to 'docs/third-review.md')

diff --git a/docs/third-review.md b/docs/third-review.md
index b6f8d3c..48e7ccb 100644
--- a/docs/third-review.md
+++ b/docs/third-review.md
@@ -61,8 +61,9 @@ and a few of the second-review fixes did not reach every path.
   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.
+  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** 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.
@@ -71,10 +72,9 @@ and a few of the second-review fixes did not reach every path.
   OpenGraph images and metadata.
 
 None of this breaks the architecture. The cryptographic core and the trust model
-are unchanged and still sound. H1, H2, M1, M2 and M6 were fixed on 2026-09-01.
-The link-preview SSRF (M3) should be bounded before that feature runs on an
-internet-facing node; what remains beyond it (M4 federation, M5 headers, the
-L-list) is hardening, not a hole.
+are unchanged and still sound. H1, H2, M1, M2, M3 and M6 were fixed on
+2026-09-01. What remains — M4 (federation trust), M5 (SPA security headers) and
+the L-list — is hardening, not a hole.
 
 ---
 
@@ -352,6 +352,25 @@ 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`)
 
@@ -581,15 +600,15 @@ Against the v6 §4 claims, updated for this review:
 | 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 | — | — | ⚠️ link previews: unthrottled, ports open, rebinding (M3) |
+| 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/M2/M6 were fixed the day this was written — leaving the
-link-preview SSRF (M3) as the one item to bound before that feature faces the
-internet, and MHP federation / SPA headers as hardening.*
+closed, and H1/H2/M1/M2/M3/M6 were fixed the day this was written — leaving MHP
+federation trust (M4) and the missing SPA security headers (M5) as hardening,
+not holes.*
 
 ---
 
@@ -601,7 +620,7 @@ internet, and MHP federation / SPA headers as hardening.*
 | 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 | Before enabling link previews on an internet-facing node |
+| 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 | 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` |
@@ -623,7 +642,10 @@ Two structural recommendations, both echoing the second review:
    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.
+   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.
 
 ---
 
@@ -643,6 +665,7 @@ required exotic capability, and none of them were architectural — they were th
 cost of adding six subsystems faster than the authorization model grew to cover
 them.
 
-H1, H2, M1, M2 and M6 were fixed the day this review was written. Link previews
-should stay off on an internet-facing node until M3 is bounded. Past that, the
-honest claims are strong ones and are now largely defensible.
+H1, H2, M1, M2, M3 and M6 were fixed the day this review was written. What is
+left — MHP federation trust (M4), the SPA's missing security headers (M5), and
+the L-list — is hardening on a build whose honest claims are now strong and
+largely defensible.
-- 
cgit v1.2.3


From a9eb121476eb8128e0cf1ae280ed0fea84d4b2aa Mon Sep 17 00:00:00 2001
From: Christophe Besson <cbesson@gmail.com>
Date: Tue, 1 Sep 2026 20:05:39 +0200
Subject: docs: mark M4 and M5 fixed in the third security review

M4: federation `source_hub` bound to the token signer, push capped,
revocation prunes the peer's own directory entries, state-changing
MHP tokens are single-use. M5: a middleware adds a CSP and the other
protective headers to every response, matching the desktop client's
policy for these files.

Every finding in the review (H1, H2, M1-M6) is now fixed; the summary,
findings table and action plan reflect that. Original finding texts
kept for the record.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG
---
 docs/third-review.md | 83 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 57 insertions(+), 26 deletions(-)

(limited to 'docs/third-review.md')

diff --git a/docs/third-review.md b/docs/third-review.md
index 48e7ccb..6839ef7 100644
--- a/docs/third-review.md
+++ b/docs/third-review.md
@@ -64,17 +64,20 @@ and a few of the second-review fixes did not reach every path.
   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** 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.
+- **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, M1, M2, M3 and M6 were fixed on
-2026-09-01. What remains — M4 (federation trust), M5 (SPA security headers) and
-the L-list — is hardening, not a hole.
+are unchanged and still sound. Every finding in this review (H1, H2, M1–M6) was
+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.
 
 ---
 
@@ -410,6 +413,20 @@ 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`)
 
@@ -450,6 +467,19 @@ 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
@@ -596,7 +626,7 @@ Against the v6 §4 claims, updated for this review:
 | 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. **Does not federate** (M4) |
+| 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) |
@@ -605,10 +635,10 @@ Against the v6 §4 claims, updated for this review:
 | 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/M2/M3/M6 were fixed the day this was written — leaving MHP
-federation trust (M4) and the missing SPA security headers (M5) as hardening,
-not holes.*
+genuinely mutual and covers every path, the second review's critical gaps are
+closed, and every finding in this review (H1, H2, M1–M6) was fixed the day it was
+written — leaving the L-list as opportunistic hardening and one thing to verify:
+the SPA's new CSP against the live app.*
 
 ---
 
@@ -621,8 +651,8 @@ not holes.*
 | 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 | Before enabling MHP with any non-self peer |
-| M5 | No CSP / security headers on the SPA | Medium | S | Opportunistic — cheap, high value given T3 |
+| 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 | 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 |
@@ -659,13 +689,14 @@ 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
-required exotic capability, and none of them were architectural — they were the
-cost of adding six subsystems faster than the authorization model grew to cover
-them.
-
-H1, H2, M1, M2, M3 and M6 were fixed the day this review was written. What is
-left — MHP federation trust (M4), the SPA's missing security headers (M5), and
-the L-list — is hardening on a build whose honest claims are now strong and
-largely defensible.
+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.
+
+Every finding in this review (H1, H2, M1–M6) was fixed the day it was written.
+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.
-- 
cgit v1.2.3


From 1e6db7d23c70b7bd7e1422f09911b3645f0fb2e2 Mon Sep 17 00:00:00 2001
From: Christophe Besson <cbesson@gmail.com>
Date: Tue, 1 Sep 2026 20:50:45 +0200
Subject: revert(hub): M6 — add_group_member must keep accepting node tokens
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

M6 in the third review was a misread. `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 shows up in the invitee's SPA, authenticating with a
node-scoped token. `group.admin_id == caller` is the real guard.

An older test (`test_node_scope_blocks_add_member`) asserted the
opposite and had been left red on main; the M6 "fix" (commit 6b38704)
satisfied that test by switching the dependency to `require_user_scope`
— which made `ops.create_invite`'s hub-membership call 403. That
exception is swallowed with a log.warning, so an invited user silently
never lands in group_members and the group is invisible to them.
Reported from live testing (CLI `member invite grenet`, grenet saw
nothing).

Dependency back to `get_current_user`. The stale test now asserts the
intended behaviour: a node token may add a member to its own
operator's group (201) but not to a group it does not own (403).
Third-review M6 marked WITHDRAWN.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG
---
 docs/third-review.md                               | 63 +++++++++++++---------
 packages/meshbay-hub/src/meshbay_hub/api/groups.py | 14 ++---
 packages/meshbay-hub/tests/test_node_auth.py       | 23 +++++---
 3 files changed, 62 insertions(+), 38 deletions(-)

(limited to 'docs/third-review.md')

diff --git a/docs/third-review.md b/docs/third-review.md
index 6839ef7..531f73e 100644
--- a/docs/third-review.md
+++ b/docs/third-review.md
@@ -75,7 +75,7 @@ and a few of the second-review fixes did not reach every path.
   running SPA.
 
 None of this breaks the architecture. The cryptographic core and the trust model
-are unchanged and still sound. Every finding in this review (H1, H2, M1–M6) was
+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.
 
@@ -502,24 +502,32 @@ hashes to the `<script>` tags in `_HTML` (the content hash is already computed).
 
 ---
 
-### M6 — `add_group_member` accepts node-scoped tokens
+### M6 — `add_group_member` accepts node-scoped tokens — WITHDRAWN
 
-> **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.
+> **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.
 
 ---
 
@@ -636,9 +644,9 @@ Against the v6 §4 claims, updated for this review:
 
 **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 every finding in this review (H1, H2, M1–M6) was fixed the day it was
-written — leaving the L-list as opportunistic hardening and one thing to verify:
-the SPA's new CSP against the live app.*
+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.*
 
 ---
 
@@ -653,7 +661,7 @@ the SPA's new CSP against the live app.*
 | 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 | Medium | S | ✅ **fixed 2026-09-01** — dependency → `require_user_scope` |
+| 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 |
@@ -695,8 +703,11 @@ 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.
 
-Every finding in this review (H1, H2, M1–M6) was fixed the day it was written.
-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
+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.
diff --git a/packages/meshbay-hub/src/meshbay_hub/api/groups.py b/packages/meshbay-hub/src/meshbay_hub/api/groups.py
index 15c7a5d..07d3ec0 100644
--- a/packages/meshbay-hub/src/meshbay_hub/api/groups.py
+++ b/packages/meshbay-hub/src/meshbay_hub/api/groups.py
@@ -586,14 +586,16 @@ async def update_group(
 async def add_group_member(
     group_id: str,
     username: str,
-    current_user: User = Depends(require_user_scope),
+    current_user: User = Depends(get_current_user),
     db: AsyncSession = Depends(get_db),
 ):
-    # `require_user_scope`, like every other mutating group endpoint: a
-    # node-scoped daemon token must not manage membership (NS7 — the operator
-    # manages groups from the browser). This was the one membership endpoint
-    # still on `get_current_user`, so a node token could add members to its
-    # operator's own groups.
+    # `get_current_user`, not `require_user_scope`: the node calls this after a
+    # CLI `member invite` so the group becomes visible in the invitee's SPA
+    # (commit 0443cf8). The node authenticates with a node-scoped token, and the
+    # `group.admin_id == current_user.id` check below is the real guard — a node
+    # can only touch its own operator's groups, adding an already-registered
+    # account. (Third-review M6 proposed tightening this to `require_user_scope`;
+    # that broke the CLI invite flow and was reverted — see the review doc.)
     group = await db.get(Group, group_id)
     if not group:
         raise HTTPException(status_code=404, detail="Group not found")
diff --git a/packages/meshbay-hub/tests/test_node_auth.py b/packages/meshbay-hub/tests/test_node_auth.py
index 72ce412..a104a72 100644
--- a/packages/meshbay-hub/tests/test_node_auth.py
+++ b/packages/meshbay-hub/tests/test_node_auth.py
@@ -148,24 +148,35 @@ async def test_node_scope_blocks_group_create(client):
 
 
 @pytest.mark.asyncio
-async def test_node_scope_blocks_add_member(client):
-    sk_node, user_token = await _setup_node_user(client, "op1")
+async def test_node_token_may_add_a_member_to_its_own_operators_group(client):
+    """The node calls this after a CLI `member invite` so the group shows up in
+    the invitee's SPA (commit 0443cf8). A node-scoped token is accepted here —
+    the `group.admin_id == caller` check is the guard — but only for a group the
+    node's operator owns."""
+    sk_op, op_token = await _setup_node_user(client, "op1")
 
     r = await client.post("/v1/groups", json={
         "name": "mygroup", "visibility": "private", "join_policy": "invite",
-    }, headers={"Authorization": f"Bearer {user_token}"})
-    assert r.status_code == 201
+    }, headers={"Authorization": f"Bearer {op_token}"})
     gid = r.json()["group_id"]
 
     _, pk2 = _gen_ed25519()
     _, px2 = _gen_x25519()
     await _register(client, "member1", pk2, px2)
 
-    r = await _node_auth(client, "op1", sk_node)
-    node_token = r.json()["access_token"]
+    node_token = (await _node_auth(client, "op1", sk_op)).json()["access_token"]
 
     r = await client.post(f"/v1/groups/{gid}/members/member1",
                           headers={"Authorization": f"Bearer {node_token}"})
+    assert r.status_code == 201
+
+    # …but not to a group it does not own.
+    sk_other, other_token = await _setup_node_user(client, "op2")
+    r = await client.post("/v1/groups", json={"name": "theirs", "visibility": "private"},
+                          headers={"Authorization": f"Bearer {other_token}"})
+    other_gid = r.json()["group_id"]
+    r = await client.post(f"/v1/groups/{other_gid}/members/member1",
+                          headers={"Authorization": f"Bearer {node_token}"})
     assert r.status_code == 403
 
 
-- 
cgit v1.2.3