path: root/docs/third-review.md

# 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. C1 (the unauthenticated node HTTP file API) was deleted outright
rather than patched. C2, C3, C5a, C5b, H1, H4, H5, H6, H7, M7, M8 are all
addressed in the code, and the invite rewrite closed H3/M3. Device linking,
the password split, Argon2id-256 MB on the hub verifier, refresh-token family
rotation, email-at-rest encryption, and session/device teardown on passphrase
change are all present and correct.

**What this review finds is a second generation of the same pattern:** new
surface was added faster than the authorization model was extended to cover it,
and a few of the second-review fixes did not reach every path.

- The **QUIC transport** got the new handshake but not the new *chat* rules:
  `_do_chat_message_sync` took `sender_id` from the wire (NS6), broadcast
  through a connection-global peer registry regardless of group (H1), and ran a
  30-second synchronous `ffmpeg` on the event loop with no concurrency cap (H6).
  **Fixed 2026-09-01** — handlers brought to WebRTC parity, and the QUIC
  listener is now off by default (`[node] quic_enabled`) since nothing ships a
  QUIC client.
- The **hub moderation surface** had a privilege-escalation hole: a *moderator*
  could promote any other account to *admin* (`PATCH /v1/admin/users/{id}` was
  gated by `require_moderator` but wrote `role`). **Fixed 2026-09-01.**
- **`POST /v1/reports`** was unauthenticated, unthrottled, and auto-blocked a
  content hash after **two** reports — a network-wide censorship/DoS primitive
  for anyone who learns a public file's blake3 id. **Fixed 2026-09-01** (auth,
  rate limit, distinct-reporter counting, refused when public groups are off).
- The **registration reCAPTCHA** was inert: the server only checked it when
  `auth_key` was absent, and the real web client always sends `auth_key`, so a
  bot skipped it by including that field. **Fixed 2026-09-01** — gate is now
  unconditional when a captcha is configured; the desktop client renders the
  widget too.
- **Chat link previews** are a real SSRF surface (correctly identified as such in
  the module) but the gate 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, 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.

---

## 2. What is solid (the delta since the second review)

Worth recording, because the remediation was substantial and mostly correct:

1. **`meshbay_common/handshake.py`** — one implementation, called by
   `webrtc_server.py` and `quic_server.py`. `handshake_transcript()` is
   length-prefixed and domain-separated (`meshbay:mnp:handshake:v1`), the role is
   bound so a client proof can never be replayed as a node proof, and
   `make_proof()` **raises** on an empty channel binding instead of degrading to
   nonce-only (L4). `authorize_token()` enforces `scope == "user"` by default
   (M9), requires `group_id` (M1), checks the denylist, the `groups` claim and
   `hosted_groups`.
2. **Mutual authentication (C3).** `_complete_handshake` returns
   `HMAC(GEK, node-transcript)` over the client's nonce **and**
   `Ed25519(sk_node)` over the same transcript; `transport.js` verifies both
   (`verifyNodeSignature`), refuses a bare `handshake_ack`, and TOFU-pins
   `node_pk` in `localStorage` with an explicit change warning
   (`_checkNodePin`).
3. **C1 deleted.** The per-group HTTP file API is gone from `daemon.py`
   (step 9 is now a comment explaining why). Every client path goes through the
   MNP handshake.
4. **C2 closed.** `_authorize_node_ws` resolves the node against the DB, checks
   `scope == "node"`, checks `node.user_id == token.sub`, derives `group_ids`
   from `GroupMember`, and refuses to displace a live registration.
5. **C5a closed.** Uploads: `SAFE_UPLOAD_NAME` allowlist, `_free_name()`
   no-overwrite, `MAX_UPLOAD_BYTES` cap, strict chunk ordering, a quarantine
   subdirectory, and an operator-signed `OP_MEMBER_UPLOAD` kill switch enforced
   by the node (`_do_file_upload`), not by hiding a button.
6. **C5b closed.** `gek_bundle_store` is deleted; `gek_rotate` is an
   operator-signed op where the node generates the key with its own CSPRNG
   (`_admin_exec_gek_rotate` → `ops.set_gek(rotate=True)`).
7. **H1 (WebRTC) closed.** `_do_chat_message` / `_do_chat_history` read
   `self._group_ctx().get("chat_store")`, `_peer_registry()` is per-group, and
   `sender_id` is forced to `self._user_id`.
8. **H4 closed.** `Denylist` persists to `denylist.json`; `on_revocation`
   handles `user`/`group`/`jti`, and `group` also drops live sessions
   (`_drop_group_sessions`); `webrtc_offer` refuses when the shared group is not
   `active`.
9. **H5 closed.** `adminop.admin_transcript()` — domain-separated, names the
   operation, subject, node key, group, nonce and timestamp; `ADMIN_CHALLENGE_TTL`
   120 s; verified against `roster.operator_pks()` rebuilt from node state, never
   from the response.
10. **H6 (WebRTC) closed.** 64 KB pre-handshake buffer, a transcode semaphore,
    per-user pending-offer caps and a membership check in `signaling.py`,
    `notify_incoming` requires `peer_ip == caller_ip`.
11. **H7 closed.** The swarm route is mounted correctly, nodes filter by
    visibility, and `GET /v1/swarm/{hash}` requires auth.
12. **M8 closed.** `announce_node` requires a signed proof of possession.
13. **Device linking** (`_do_device_add`, `_verify_device_signer`): a new device
    is admitted only by a signature from a **live pinned device of the same
    account**; the one-time code never reaches the node (it lists candidate
    hashes and the approver recomputes the match); requests are single-use and
    capped by `MAX_DEVICES_PER_USER`. The hub holds no user keys and so cannot
    countersign — this holds against an active hub.
14. **Account lifecycle** (`docs/auth-confirm.md`): passphrase change and reset
    both revoke every refresh token; reset also deletes every `UserDevice` so a
    stored device key cannot sign back in past the reset. The recovery key is a
    pure client-side pass-through — never stored, never logged.

---

## 3. High findings

### H1 — A moderator can promote any account to admin (privilege escalation)

> **Fixed 2026-09-01.** `admin_patch_user` now splits authorization by field:
> `status` between `active`/`suspended` stays at `require_moderator`; setting
> `role`, setting `status = "revoked"`, and touching an admin's account at all
> require `user_is_admin(current_user)` (new helper in `deps.py`). Regression
> test: `test_moderator_cannot_change_roles_or_revoke`.

**Location:** `api/admin.py:185-241` (`admin_patch_user`), `api/deps.py:75-92`

`PATCH /v1/admin/users/{user_id}` depends on `require_moderator`, but its body
accepts `role`, and the handler writes it with no check that the caller is an
admin:

```python
if body.role is not None:
    if body.role not in ("user", "moderator", "admin"):
        raise HTTPException(status_code=422, ...)
    user.role = body.role          # ← moderator can set "admin"
```

The only guard is `user.id == current_user.id` ("Cannot modify your own
account"). So a moderator cannot self-promote directly, but can:

- promote a second account they control, or an accomplice, to `admin`;
- **demote existing admins** to `user`, or set their `status` to `revoked`.

`admin` is the real instance boundary: `admin_patch_settings` (public-groups
switch), `admin_delete_user` (irreversible erasure), `admin_revoke`
(user/group revocation broadcast to every node), `register_peer`,
`admin_add_blocklist`. A moderator reaching `admin` reaches all of it.

**Impact.** Full instance takeover from the moderator role. Moderator is meant to
be a content-moderation role (suspend/revoke groups, read logs), not an
administrative one — `admin_delete_user`'s own docstring draws exactly that line
("Admin rather than moderator: suspension is reversible … this is not").

**Fix.** Split the handler: `status` changes among `active`/`suspended` stay at
`require_moderator`; `role` changes and `status = "revoked"` require
`require_admin`. Also forbid granting a role higher than the caller's, and forbid
demoting an equal-or-higher role.

---

### H2 — Unauthenticated, unthrottled, permanent global content blocklisting

> **Fixed 2026-09-01.** `POST /v1/reports` now requires a signed-in account
> (`get_current_user`), is rate-limited (`10/hour`), counts **distinct reporting
> accounts** (one vote per account per hash via `reporter_id`), and is refused
> outright (`403`) when the hub has public groups switched off — a private-only
> hub brokers no public content and nothing syncs the blocklist, so an open write
> endpoint there is pure abuse surface. `AUTO_BLOCK_THRESHOLD` raised 2 → 3.
> Tests rewritten in `test_moderation.py`.
>
> Note also confirmed while fixing: **no node currently consumes
> `ContentBlocklist`** — `GET /v1/blocklist` exists ("nodes sync on startup") but
> nothing fetches it, and `swarm_register` checks the *CSAM* list, not this one.
> So the network-wide censorship effect was latent (it activates when node sync
> ships); the DB-fill / poisoned-moderation-signal / admin-panel-garbage surface
> was live. The auto-block path should stay gated as above when sync lands.

**Location:** `api/moderation.py:39,58-103` (`report_content`)

`POST /v1/reports` has **no authentication and no rate limit**. It counts *all*
existing `ContentReport` rows for a hash — regardless of who filed them or from
where — and:

```python
AUTO_BLOCK_THRESHOLD = 2
...
if count + 1 >= AUTO_BLOCK_THRESHOLD:
    ... db.add(ContentBlocklist(content_hash=..., added_by="auto"))
```

So **two unauthenticated HTTP requests** naming the same 64-hex blake3 id add
that id to `ContentBlocklist`. Nodes sync the blocklist
(`GET /v1/blocklist`, unauthenticated) and `swarm_register` refuses a blocked
hash with HTTP 451. Removal is a manual admin action
(`DELETE /v1/admin/blocklist/{hash}`).

**Impact.** Anyone who learns the blake3 id of a public file — trivially, any
group member sees ids in the index; any registered user can probe
`GET /v1/swarm/{hash}` — can suppress that file across the whole network with two
anonymous requests. It is also a self-inflicted amplifier: one script can block
thousands of hashes. `content_hash` is the only validated field (`group_id`,
`reason`, `detail` are free-form and rendered in the admin UI).

**Fix.** Require authentication on `POST /v1/reports`; dedupe reports by
`(content_hash, reporter)` so the threshold means *distinct* reporters; add a
rate limit; raise `AUTO_BLOCK_THRESHOLD` and/or make auto-block queue for human
review rather than take effect immediately; authenticate `GET /v1/blocklist` and
`/v1/blocklist/check` (node scope).

---

## 4. Medium findings

### M1 — The registration CAPTCHA is inert and trivially bypassed

> **Fixed 2026-09-01 (Option A).** The server gate is now `if
> _cfg.captcha.enabled:` — no `auth_key` carve-out, no client exemption. The web
> client (`registerUser` in `keyderive.js`) forwards `captcha.token`, and the
> desktop client, being Chromium, renders the same widget from the shared UI
> assets. `captcha.reset()` is called on a failed attempt so the single-use
> token is refreshed. Tests: `test_register_captcha.py`.
>
> Consequence to check on the desktop side: the Electron CSP must allow
> `https://www.google.com` and `https://www.gstatic.com` for `script-src` /
> `frame-src`, or the widget will not render and the (already-disabled) submit
> button stays disabled. A headless/CLI `register` has no widget and is the one
> path with no human check — which is the path you would want gated anyway; a CLI
> can open a browser window for it.

**Location:** `api/users.py:130-191` (`register`), `static/keyderive.js:279-303`
(`registerUser`), `static/auth-page.js:229-256`

Server side:

```python
# Captcha gate — web path only (native clients send auth_key)
if _cfg and _cfg.captcha.enabled and not body.auth_key:
    await _verify_captcha_or_raise(body.captcha_token, request)
```

The CAPTCHA is checked **only when `auth_key` is absent**. But the real web
client's registration path (`window.MeshBayKeys` present, which is always)
calls `registerUser()`, which sends `{ username, email, auth_key }` and **no
`captcha_token`** at all. The branch that sends `captcha_token`
(`auth-page.js:248`) is a dead `else` for a client without `MeshBayKeys`.

So: a human filling the Register form solves a reCAPTCHA whose token is never
transmitted and never checked, and a bot registers accounts at will by including
any `auth_key`-shaped string. `@limiter.limit("5/minute")` is the only remaining
brake (and see L10 for why that may also be weak).

Password reset is unaffected — `password_reset_request` checks the CAPTCHA
unconditionally when enabled.

**Fix.** Gate on `_cfg.captcha.enabled` alone (drop `and not body.auth_key`), and
have `registerUser()` include `captcha_token`. If native clients genuinely cannot
present one, gate on the *client type* explicitly (a header or a scope), not on
the presence of a field any caller can supply.

---

### M2 — QUIC transport: chat sender spoofing, cross-group broadcast, and a blocking ffmpeg

> **Fixed 2026-09-01, two ways.**
> 1. **Exposure removed:** the QUIC listener is now off by default —
>    `[node] quic_enabled = false` (`MESHBAY_QUIC_ENABLED` overrides), gated in
>    `daemon.py`. Nothing ships a QUIC client, so a node started none for no one.
> 2. **Handlers brought to parity** anyway, for when a client does exist:
>    `_do_chat_message_sync` forces `sender_id` from the authenticated session
>    (M2a), resolves `chat_store` and the peer set per group via `_group_ctx()` /
>    `_peer_registry()` (M2b, kept separate from the WebRTC peer set), and
>    `STREAM_SEGMENT` extraction runs in a thread behind a small semaphore as a
>    tracked task (M2c). Stale C6 docstring corrected.
>
> Original finding text kept below for the record.

**Location:** `transport/quic_server.py:220-247, 402-463, 509-525`

The QUIC server is started in production (`daemon.py:540`, `host="::"`, default
port 19000, `groups=groups_ctx`). It got the new unified handshake — and the
GEK proof *is* implemented in `_do_handshake_response_sync`, so the docstring at
`quic_server.py:259-263` ("NOT YET DONE — finding C6 remains open on this
transport") is simply stale. But the chat and streaming handlers were never
brought up to the WebRTC path's rules:

**M2a — `sender_id` is taken from the wire (NS6 regression).**

```python
asyncio.ensure_future(chat_store.save_message(
    sender_id=msg.get("sender_id", self._user_id), ...))
...
broadcast = { ... "sender_id": msg.get("sender_id", self._user_id), ... }
```

An authenticated QUIC peer can post chat as any `sender_id`. The WebRTC path
forces `sender_id=self._user_id` (`webrtc_server.py:3493,3504`).

**M2b — the peer registry is connection-global, not per-group (H1 regression).**
`_do_chat_message_sync` broadcasts to `self._ctx.get("_peers", {})`, which is a
single dict on the `QuicChunkServer` instance shared across every group. A member
of group A, connected over QUIC, has their (spoofable) message fanned out to
QUIC peers of every other group on the node. (`chat_store` is never set in the
QUIC ctx, so messages are dropped rather than persisted — but still broadcast.)

**M2c — synchronous ffmpeg on the event loop, no concurrency cap (H6
regression).** `_do_stream_segment_sync` → `_extract_segment` runs
`subprocess.run([... "ffmpeg" ...], timeout=30)` directly inside
`quic_event_received`. One request blocks the whole node for up to 30 s; there is
no transcode semaphore. `_do_file_request_sync` likewise does blocking file I/O
in the loop.

**Impact.** Limited to peers reachable over QUIC/UDP 19000 — LAN, a
port-forwarded node, or hub-less `group://` — and to native clients (browsers use
WebRTC, a separate registry). Still: chat impersonation and cross-group leakage
against exactly the "malicious group member" adversary the WebRTC fixes were
written for, plus a one-request node stall.

**Fix.** Route the QUIC chat and stream handlers through the same per-group
context and `sender_id`-from-session logic as WebRTC (ideally shared helpers in
`meshbay_common`, the same move that fixed C6); make `_extract_segment` async and
put it behind the transcode semaphore, or disable the QUIC `STREAM_SEGMENT`
handler until it is at parity. Update the stale docstring.

---

### M3 — Chat link previews: SSRF gate has no rate limit, no port restriction, and a known rebinding hole

**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 | ✅ | ✅ | ✅ 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 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 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.*

---

## 7. Prioritised action plan

| # | Finding | Severity | Effort | When |
|---|---|---|---|---|
| H1 | Moderator can write `role` → admin | High | S | ✅ **fixed 2026-09-01** — handler split by field |
| H2 | Unauthenticated 2-report global blocklist | High | S | ✅ **fixed 2026-09-01** — auth + distinct-reporter + rate limit + public-groups gate |
| M1 | Registration CAPTCHA inert | Medium | S | ✅ **fixed 2026-09-01** — gate unconditional; desktop renders the widget |
| M2 | QUIC chat: `sender_id` spoof, cross-group broadcast, sync ffmpeg | Medium | M | ✅ **fixed 2026-09-01** — handlers at WebRTC parity + `quic_enabled` off by default |
| M3 | Link-preview SSRF: no rate limit, ports open, rebinding | Medium | M | 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 was the result. The 2026-09-01 fix mirrored the WebRTC
   logic into `quic_server.py` by hand — the durable version is shared helpers
   in `meshbay_common` (`sender_id` enforcement, per-group context, the
   transcode gate) with a test that fails if a transport calls a chat/stream
   path that bypasses them.

2. **Every new outbound or cross-trust surface needs a rate limit and an
   adversary named in the same commit.** Link previews, TMDB/MusicBrainz search,
   `POST /v1/reports`, `receive_directory` — each added a way for a low-privilege
   party (a member, an anonymous caller, a peer hub) to make the node or hub do
   work or accept state, and each shipped without a bound on how much.

---

## 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
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 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.