From 9f95bf2deaec0c31e5fac95e1068db40b878e217 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Tue, 1 Sep 2026 17:48:56 +0200 Subject: fix(hub): moderator can no longer grant admin or hard-revoke accounts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit admin_patch_user was gated by require_moderator but wrote `role` and `status` with no further check. A moderator could promote any account (an accomplice) to admin, demote an existing admin, or set status="revoked" — a straight path from the moderation role to full instance control. Split authorization by field: status between active/suspended stays at require_moderator (reversible content moderation); role changes, status="revoked", and touching an admin's account at all now require user_is_admin(current_user) (new helper in deps.py, alongside the existing require_admin/require_moderator). Regression test: test_moderator_cannot_change_roles_or_revoke. Third security review, finding H1. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG --- packages/meshbay-hub/src/meshbay_hub/api/admin.py | 21 +++++++++++- packages/meshbay-hub/src/meshbay_hub/api/deps.py | 17 +++++++--- packages/meshbay-hub/tests/test_admin.py | 39 ++++++++++++++++++++++- 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/packages/meshbay-hub/src/meshbay_hub/api/admin.py b/packages/meshbay-hub/src/meshbay_hub/api/admin.py index ab6fa07..4960674 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/admin.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/admin.py @@ -14,7 +14,7 @@ from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession from meshbay_hub.auth import decrypt_email -from meshbay_hub.api.deps import require_admin, require_moderator +from meshbay_hub.api.deps import require_admin, require_moderator, user_is_admin from meshbay_hub.api.revocation import get_connected_node_count, is_node_connected from meshbay_hub.db.engine import get_db from meshbay_hub.db.models import Group, GroupMember, IPLog, Node, User @@ -196,6 +196,25 @@ async def admin_patch_user( if user.id == current_user.id: raise HTTPException(status_code=400, detail="Cannot modify your own account") + # A moderator suspends and restores accounts — reversible content moderation. + # Changing what someone *is* (their role), and the one irreversible status + # (`revoked`, which is signed and broadcast to every node), are administrative. + # Without this split a moderator could promote an accomplice to admin, or + # revoke every admin, entirely from the moderation role. `admin_delete_user` + # already draws this exact line for the same reason. + privileged = body.role is not None or body.status == "revoked" + if privileged and not user_is_admin(current_user): + raise HTTPException( + status_code=403, + detail="Changing a role, or revoking an account, requires admin rights") + + # An admin's account is not a moderator's to touch at all — not their role, + # not their status. + if user_is_admin(user) and not user_is_admin(current_user): + raise HTTPException( + status_code=403, + detail="Only an admin can change another admin's account") + from meshbay_hub.api.notifications import create_notification if body.role is not None: diff --git a/packages/meshbay-hub/src/meshbay_hub/api/deps.py b/packages/meshbay-hub/src/meshbay_hub/api/deps.py index 1bf57a4..907a481 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/deps.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/deps.py @@ -72,11 +72,21 @@ async def require_user_scope( return current_user +def user_is_admin(user: User) -> bool: + """Admin by DB role or by the config allow-list. Use inside a handler that + already depends on `require_moderator` but has to draw the admin line for + one field (see `admin_patch_user`).""" + return user.role == "admin" or user.username in _admin_usernames + + +def user_is_moderator(user: User) -> bool: + return user.role in ("moderator", "admin") or user.username in _admin_usernames + + async def require_moderator( current_user: User = Depends(get_current_user), ) -> User: - if current_user.role not in ("moderator", "admin") \ - and current_user.username not in _admin_usernames: + if not user_is_moderator(current_user): raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Moderator access required") return current_user @@ -85,8 +95,7 @@ async def require_moderator( async def require_admin( current_user: User = Depends(get_current_user), ) -> User: - if current_user.role != "admin" \ - and current_user.username not in _admin_usernames: + if not user_is_admin(current_user): raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Admin access required") return current_user diff --git a/packages/meshbay-hub/tests/test_admin.py b/packages/meshbay-hub/tests/test_admin.py index 51b233a..ad48487 100644 --- a/packages/meshbay-hub/tests/test_admin.py +++ b/packages/meshbay-hub/tests/test_admin.py @@ -5,7 +5,6 @@ Integration tests for the admin/moderation panel API. import pytest from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey from cryptography.hazmat.primitives.asymmetric.x25519 import X25519PrivateKey - from meshbay_common.crypto import pk_to_b64 from meshbay_hub.api.deps import set_admin_usernames @@ -161,6 +160,44 @@ async def test_admin_change_role(client): assert r.json()["role"] == "moderator" +@pytest.mark.asyncio +async def test_moderator_cannot_change_roles_or_revoke(client): + """A moderator suspends and restores (reversible); it cannot promote anyone + or hard-revoke, which would be a path from the moderation role to full + instance control.""" + _, admin_token = await _setup_admin(client, "boss") + mod_id = await _register(client, "moduser") + await client.patch(f"/v1/admin/users/{mod_id}", json={"role": "moderator"}, + headers={"Authorization": f"Bearer {admin_token}"}) + mod_token = await _login(client, "moduser") + mod_h = {"Authorization": f"Bearer {mod_token}"} + + victim = await _register(client, "victim", email="v@x.com") + + # No promoting an accomplice. + r = await client.patch(f"/v1/admin/users/{victim}", json={"role": "admin"}, + headers=mod_h) + assert r.status_code == 403 + + # No hard revocation. + r = await client.patch(f"/v1/admin/users/{victim}", json={"status": "revoked"}, + headers=mod_h) + assert r.status_code == 403 + + # No touching an admin's account. + admin2 = await _register(client, "admin2", email="a2@x.com") + await client.patch(f"/v1/admin/users/{admin2}", json={"role": "admin"}, + headers={"Authorization": f"Bearer {admin_token}"}) + r = await client.patch(f"/v1/admin/users/{admin2}", json={"status": "suspended"}, + headers=mod_h) + assert r.status_code == 403 + + # Suspending a plain user is still fine. + r = await client.patch(f"/v1/admin/users/{victim}", json={"status": "suspended"}, + headers=mod_h) + assert r.status_code == 200 + + @pytest.mark.asyncio async def test_admin_cannot_modify_self(client): admin_id, token = await _setup_admin(client) -- cgit v1.2.3 From 99eb93a00269fbfafba7536cf1613b3d4c18c3ce Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Tue, 1 Sep 2026 17:49:05 +0200 Subject: fix(hub): require auth and distinct reporters for content reports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit POST /v1/reports had no authentication and no rate limit, and counted every raw report row toward AUTO_BLOCK_THRESHOLD regardless of who sent it or from where — two anonymous requests naming any blake3 hash added it to the hub-wide content blocklist. A network-wide censorship and DoS primitive for anyone who learns a public file's hash. - require a signed-in account (get_current_user) - rate-limited (10/hour) - threshold now counts DISTINCT reporting accounts (reporter_id), one vote per account per hash; raised 2 -> 3 - 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 the endpoint would be pure abuse surface - admin blocklist management (/v1/admin/blocklist*) is untouched, so a manual block still works regardless of the public-groups setting Noted while fixing: no node currently consumes ContentBlocklist (swarm_register checks the separate CSAM list), so the network-wide block effect was latent — the abuse surface (DB fill, poisoned moderation signal) was live today. Tests rewritten in test_moderation.py. Third security review, finding H2. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG --- .../meshbay-hub/src/meshbay_hub/api/moderation.py | 90 +++++++++----- packages/meshbay-hub/tests/test_moderation.py | 132 ++++++++++++++------- 2 files changed, 149 insertions(+), 73 deletions(-) diff --git a/packages/meshbay-hub/src/meshbay_hub/api/moderation.py b/packages/meshbay-hub/src/meshbay_hub/api/moderation.py index 853f255..0939eec 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/moderation.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/moderation.py @@ -1,13 +1,16 @@ """ MeshBay Hub — moderation endpoints. -Public reporting flow: - POST /v1/reports — report a content hash (no auth required) +Reporting flow: + POST /v1/reports — report a content hash (sign-in required) - Thresholds: - 1st report → logged, node admin notified (future: push notification) - 2nd report → content hash added to blocklist automatically - 3rd+ report → logged as repeat offense (escalation for human review) + Thresholds (counted as DISTINCT reporting accounts, not raw rows): + < AUTO_BLOCK_THRESHOLD distinct reporters → logged + >= AUTO_BLOCK_THRESHOLD distinct reporters → hash added to the blocklist + + The flow only runs while the hub brokers public content: with public groups + switched off instance-wide there is nothing here to serve a reported hash from, + so it is refused rather than left open as an unauthenticated write surface. Admin endpoints: GET /v1/admin/blocklist — list blocked hashes @@ -27,7 +30,9 @@ from pydantic import BaseModel from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession +from meshbay_hub import hub_settings from meshbay_hub.api.deps import get_current_user, require_admin +from meshbay_hub.api.middleware import limiter from meshbay_hub.api.netutil import client_ip from meshbay_hub.db.engine import get_db from meshbay_hub.db.models import ContentBlocklist, ContentReport, User @@ -36,7 +41,11 @@ log = logging.getLogger(__name__) router = APIRouter(tags=["moderation"]) -AUTO_BLOCK_THRESHOLD = 2 # reports before automatic block +# Distinct reporting accounts before a hash is auto-blocked. Kept low for a +# responsive community signal, but note it is only as strong as account +# creation: while a bot can register freely (see the reCAPTCHA gap), the real +# control is the admin reviewing `GET /v1/admin/blocklist` and the audit log. +AUTO_BLOCK_THRESHOLD = 3 # ── Models ──────────────────────────────────────────────────────────────────── @@ -56,34 +65,58 @@ class BlocklistAddRequest(BaseModel): # ── Public endpoints ────────────────────────────────────────────────────────── @router.post("/v1/reports", status_code=201) +@limiter.limit("10/hour") async def report_content( body: ReportRequest, request: Request, + current_user: User = Depends(get_current_user), db: AsyncSession = Depends(get_db), ): - """Report a content hash. No authentication required.""" - if len(body.content_hash) != 64 or not all(c in "0123456789abcdef" for c in body.content_hash): - raise HTTPException(status_code=422, detail="content_hash must be 64 hex chars (blake3)") + """ + Report a public content hash for moderation. - ip = client_ip(request) + Sign-in is required. It used to be anonymous, which made it a censorship + primitive: two unauthenticated POSTs naming any blake3 id auto-added it to + the blocklist that nodes enforce, network-wide, with manual admin removal the + only undo. The threshold now counts *distinct reporting accounts*, one vote + per account per hash. - # Count existing reports for this hash - count_result = await db.execute( - select(func.count()).where(ContentReport.content_hash == body.content_hash)) - count = count_result.scalar_one() + Refused entirely when the hub has public groups switched off: nothing here + brokers public content then, nothing syncs the blocklist, and an open write + endpoint would only be abuse surface. + """ + if not await hub_settings.public_groups_allowed(db): + raise HTTPException( + status_code=403, + detail="This hub does not broker public content, so there is nothing to report here.") - report = ContentReport( - content_hash=body.content_hash, - group_id=body.group_id, - reason=body.reason, - detail=body.detail, - ip_address=ip, - ) - db.add(report) + if len(body.content_hash) != 64 or not all(c in "0123456789abcdef" for c in body.content_hash): + raise HTTPException(status_code=422, detail="content_hash must be 64 hex chars (blake3)") - action = "logged" - if count + 1 >= AUTO_BLOCK_THRESHOLD: - # Check if already blocked + # One vote per account per hash — a single reporter must not be able to walk + # the threshold up on their own by posting repeatedly. + already = await db.scalar( + select(ContentReport.id).where( + ContentReport.content_hash == body.content_hash, + ContentReport.reporter_id == current_user.id)) + + if not already: + db.add(ContentReport( + content_hash=body.content_hash, + reporter_id=current_user.id, + group_id=body.group_id, + reason=body.reason, + detail=body.detail, + ip_address=client_ip(request), + )) + await db.flush() + + distinct_reporters = await db.scalar( + select(func.count(func.distinct(ContentReport.reporter_id))) + .where(ContentReport.content_hash == body.content_hash)) or 0 + + action = "already_reported" if already else "logged" + if distinct_reporters >= AUTO_BLOCK_THRESHOLD: existing = await db.get(ContentBlocklist, body.content_hash) if not existing: db.add(ContentBlocklist( @@ -92,13 +125,14 @@ async def report_content( added_by="auto", )) action = "auto_blocked" - log.warning("Content auto-blocked after %d reports: %s", count + 1, body.content_hash[:16]) + log.warning("Content auto-blocked after %d distinct reporters: %s", + distinct_reporters, body.content_hash[:16]) await db.commit() return { "status": action, "content_hash": body.content_hash, - "report_count": count + 1, + "report_count": distinct_reporters, "threshold": AUTO_BLOCK_THRESHOLD, } diff --git a/packages/meshbay-hub/tests/test_moderation.py b/packages/meshbay-hub/tests/test_moderation.py index 93d23cd..6848929 100644 --- a/packages/meshbay-hub/tests/test_moderation.py +++ b/packages/meshbay-hub/tests/test_moderation.py @@ -1,34 +1,58 @@ -"""Tests for moderation — reports + blocklist.""" +"""Tests for moderation — reports + blocklist. + +Reporting requires a signed-in account (it used to be anonymous, which made it a +network-wide censorship primitive), the auto-block threshold counts *distinct +reporting accounts*, and the whole flow is refused when the hub has public groups +switched off. +""" import pytest -from cryptography.hazmat.primitives.asymmetric.ed25519 import Ed25519PrivateKey -from cryptography.hazmat.primitives.asymmetric.x25519 import X25519PrivateKey -from meshbay_common.crypto import pk_to_b64 from meshbay_hub.api.deps import set_admin_usernames - FAKE_HASH = "a" * 64 # valid blake3 hex -@pytest.fixture -async def auth_headers(client): - sk_ed = Ed25519PrivateKey.generate() - sk_x = X25519PrivateKey.generate() +async def _register_and_login(client, username: str) -> dict: await client.post("/v1/users/register", json={ - "username": "mod_admin", "email": "m@t.com", "password": "modpass99", - "pk_user_ed25519": pk_to_b64(sk_ed.public_key()), - "pk_user_x25519": pk_to_b64(sk_x.public_key()), + "username": username, "email": f"{username}@t.com", + "password": "reporter99pw", }) r = await client.post("/v1/users/login", - json={"username": "mod_admin", "password": "modpass99"}) - set_admin_usernames(["mod_admin"]) + json={"username": username, "password": "reporter99pw"}) return {"Authorization": f"Bearer {r.json()['access_token']}"} +@pytest.fixture +async def reporter(client): + return await _register_and_login(client, "reporter_one") + + +@pytest.fixture +async def admin_headers(client): + headers = await _register_and_login(client, "mod_admin") + set_admin_usernames(["mod_admin"]) + return headers + + @pytest.mark.asyncio -async def test_report_content_logged(client): +async def test_report_requires_auth(client): + # No credentials at all — FastAPI rejects the missing header before the body. r = await client.post("/v1/reports", json={ "content_hash": FAKE_HASH, "reason": "illegal"}) + assert r.status_code in (401, 422) + + # A bogus token is a clean 401. + r = await client.post("/v1/reports", + json={"content_hash": FAKE_HASH, "reason": "illegal"}, + headers={"Authorization": "Bearer not-a-real-token"}) + assert r.status_code == 401 + + +@pytest.mark.asyncio +async def test_report_content_logged(client, reporter): + r = await client.post("/v1/reports", + json={"content_hash": FAKE_HASH, "reason": "illegal"}, + headers=reporter) assert r.status_code == 201 data = r.json() assert data["report_count"] == 1 @@ -36,43 +60,68 @@ async def test_report_content_logged(client): @pytest.mark.asyncio -async def test_auto_block_on_threshold(client): - """Second report triggers auto-block.""" - hash2 = "b" * 64 - await client.post("/v1/reports", json={"content_hash": hash2, "reason": "spam"}) - r = await client.post("/v1/reports", json={"content_hash": hash2, "reason": "spam"}) +async def test_same_reporter_cannot_walk_the_threshold(client, reporter): + h = "b" * 64 + for _ in range(5): + r = await client.post("/v1/reports", + json={"content_hash": h, "reason": "spam"}, + headers=reporter) + assert r.json()["report_count"] == 1 + assert r.json()["status"] == "already_reported" + + check = await client.get(f"/v1/blocklist/check?hash={h}") + assert check.json()["blocked"] is False + + +@pytest.mark.asyncio +async def test_auto_block_on_distinct_reporters(client): + h = "c" * 64 + for i in range(3): + headers = await _register_and_login(client, f"rep_{i}") + r = await client.post("/v1/reports", + json={"content_hash": h, "reason": "illegal"}, + headers=headers) assert r.json()["status"] == "auto_blocked" - assert r.json()["report_count"] == 2 + assert r.json()["report_count"] == 3 + + check = await client.get(f"/v1/blocklist/check?hash={h}") + assert check.json()["blocked"] is True @pytest.mark.asyncio -async def test_blocklist_check(client): - hash3 = "c" * 64 - # Not blocked yet - r = await client.get(f"/v1/blocklist/check?hash={hash3}") - assert r.json()["blocked"] is False +async def test_reports_refused_when_public_groups_disabled(client, reporter, admin_headers): + await client.patch("/v1/admin/settings", + json={"allow_public_groups": False}, + headers=admin_headers) - # Report twice to auto-block - await client.post("/v1/reports", json={"content_hash": hash3, "reason": "illegal"}) - await client.post("/v1/reports", json={"content_hash": hash3, "reason": "illegal"}) + r = await client.post("/v1/reports", + json={"content_hash": "d" * 64, "reason": "illegal"}, + headers=reporter) + assert r.status_code == 403 - r = await client.get(f"/v1/blocklist/check?hash={hash3}") - assert r.json()["blocked"] is True + +@pytest.mark.asyncio +async def test_invalid_hash_rejected(client, reporter): + r = await client.post("/v1/reports", + json={"content_hash": "not-a-valid-blake3-hash", + "reason": "test"}, + headers=reporter) + assert r.status_code == 422 @pytest.mark.asyncio -async def test_admin_add_remove_blocklist(client, auth_headers): - hash4 = "d" * 64 +async def test_admin_add_remove_blocklist(client, admin_headers): + hash4 = "e" * 64 r = await client.post("/v1/admin/blocklist", json={"content_hash": hash4, "reason": "csam"}, - headers=auth_headers) + headers=admin_headers) assert r.status_code == 201 r = await client.get(f"/v1/blocklist/check?hash={hash4}") assert r.json()["blocked"] is True - r = await client.delete(f"/v1/admin/blocklist/{hash4}", headers=auth_headers) + r = await client.delete(f"/v1/admin/blocklist/{hash4}", headers=admin_headers) assert r.status_code == 200 r = await client.get(f"/v1/blocklist/check?hash={hash4}") @@ -80,18 +129,11 @@ async def test_admin_add_remove_blocklist(client, auth_headers): @pytest.mark.asyncio -async def test_invalid_hash_rejected(client): - r = await client.post("/v1/reports", json={ - "content_hash": "not-a-valid-blake3-hash", "reason": "test"}) - assert r.status_code == 422 - - -@pytest.mark.asyncio -async def test_full_blocklist(client, auth_headers): - hash5 = "e" * 64 +async def test_full_blocklist(client, admin_headers): + hash5 = "f" * 64 await client.post("/v1/admin/blocklist", json={"content_hash": hash5, "reason": "test"}, - headers=auth_headers) + headers=admin_headers) r = await client.get("/v1/blocklist") assert r.status_code == 200 assert hash5 in r.json()["hashes"] -- cgit v1.2.3 From 7d8c774e250b134222374f45726d5426714475de Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Tue, 1 Sep 2026 17:49:13 +0200 Subject: fix(hub): enforce registration captcha for every client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server only checked the captcha when auth_key was absent — but every real client (browser included, via the password split) sends auth_key, so the check was off for everyone, and a bot skipped it by including the field. The Register form still made humans solve a widget whose token was never transmitted. Gate is now unconditional on captcha.enabled. The web client (registerUser in keyderive.js) forwards captcha.token; RegisterPage resets the (single-use) token on a failed attempt. The desktop client shares this UI source and is Chromium, so it renders the same widget (see the paired meshbay-client commit for the CSP change that allows it). Tests: test_register_captcha.py. Third security review, finding M1 (Option A). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG --- packages/meshbay-hub/src/meshbay_hub/api/users.py | 9 +++- .../src/meshbay_hub/static/auth-page.js | 9 +++- .../src/meshbay_hub/static/keyderive.js | 6 ++- .../meshbay-hub/tests/test_register_captcha.py | 58 ++++++++++++++++++++++ 4 files changed, 78 insertions(+), 4 deletions(-) create mode 100644 packages/meshbay-hub/tests/test_register_captcha.py diff --git a/packages/meshbay-hub/src/meshbay_hub/api/users.py b/packages/meshbay-hub/src/meshbay_hub/api/users.py index 559cfa6..9b70b59 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/users.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/users.py @@ -150,8 +150,13 @@ async def register( return {"user_id": found.id, "email_verification_required": True} raise HTTPException(status_code=409, detail="Username already taken") - # Captcha gate — web path only (native clients send auth_key) - if _cfg and _cfg.captcha.enabled and not body.auth_key: + # Captcha gate — every fresh registration when a captcha is configured, with + # no client carve-out. The earlier `and not body.auth_key` exempted anything + # that sent an `auth_key`, which is *every* real client (the browser sends it + # too, from the password split) — so the check was off for everyone, and a + # bot skipped it by sending the field. The desktop client is Chromium and + # renders the same widget, so it has no need of an exemption either. + if _cfg and _cfg.captcha.enabled: await _verify_captcha_or_raise(body.captcha_token, request) # Email uniqueness (only active or pending accounts) diff --git a/packages/meshbay-hub/src/meshbay_hub/static/auth-page.js b/packages/meshbay-hub/src/meshbay_hub/static/auth-page.js index df08bc4..4c00137 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/auth-page.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/auth-page.js @@ -237,8 +237,11 @@ export function RegisterPage() { const rk = window.MeshBayKeys.generateRecoveryKey(); // `name` (trimmed), not the raw field: the hub stores the trimmed // username and every key derivation must fold in the same string. + // `captcha.token` rides along — the submit button is already disabled + // until it is set when a captcha is configured (see the form below). await window.MeshBayKeys.registerUser( - name, email, password, emailRecovery ? rk.mnemonic : null); + name, email, password, emailRecovery ? rk.mnemonic : null, + captcha.token); setRecoveryMnemonic(rk.mnemonic); session.recoveryKey = await window.MeshBayKeys.deriveRecoveryKey(rk.mnemonic, name); @@ -257,6 +260,10 @@ export function RegisterPage() { } } catch (err) { setError(err.message); + // A reCAPTCHA token is single-use: after a failed attempt (name taken, + // e-mail in use…) it is spent, so clear it and make the user solve a + // fresh one before the next try. No-op when no captcha is configured. + captcha.reset(); } finally { setLoading(false); } diff --git a/packages/meshbay-hub/src/meshbay_hub/static/keyderive.js b/packages/meshbay-hub/src/meshbay_hub/static/keyderive.js index 0aaa6a5..a540a94 100644 --- a/packages/meshbay-hub/src/meshbay_hub/static/keyderive.js +++ b/packages/meshbay-hub/src/meshbay_hub/static/keyderive.js @@ -276,7 +276,7 @@ async function decryptBundle(bundleB64, password, username) { * * Returns the raw private keys for immediate use after registration. */ -async function registerUser(username, email, password, recoveryMnemonic) { +async function registerUser(username, email, password, recoveryMnemonic, captchaToken) { // No keypair here any more. Identity keys are per node: one is generated the // first time this account joins a given node, encrypted under the passphrase, // and left with that node. So an operator who cracks what sits on their own @@ -291,6 +291,10 @@ async function registerUser(username, email, password, recoveryMnemonic) { // appends it to the verification e-mail and stores it nowhere // (docs/auth-confirm.md §4.4). Omitted when they chose to save it themselves. if (recoveryMnemonic) payload.recovery_key = recoveryMnemonic; + // reCAPTCHA response, when the hub has a captcha configured. The widget lives + // in RegisterPage (auth-page.js); this function just forwards its token. A + // hub with no captcha configured sends nothing and the server does not check. + if (captchaToken) payload.captcha_token = captchaToken; const resp = await hubCall('/v1/users/register', { method: 'POST', diff --git a/packages/meshbay-hub/tests/test_register_captcha.py b/packages/meshbay-hub/tests/test_register_captcha.py new file mode 100644 index 0000000..befc1e2 --- /dev/null +++ b/packages/meshbay-hub/tests/test_register_captcha.py @@ -0,0 +1,58 @@ +"""Registration CAPTCHA is enforced for every fresh account when configured. + +The gate used to be skipped whenever the request carried an `auth_key` — which +every real client sends (the password split) — so it protected nobody and a bot +skipped it by including the field. It now runs on `captcha.enabled` alone; the +desktop client is Chromium and renders the same widget. +""" + +import pytest + + +@pytest.fixture +def captcha_on(client, monkeypatch): + """Turn on a fake captcha: any config with both keys is `enabled`, and + verification succeeds only for the token 'good-token'.""" + from meshbay_hub.api.users import _cfg + monkeypatch.setattr(_cfg.captcha, "site_key", "test-site") + monkeypatch.setattr(_cfg.captcha, "secret_key", "test-secret") + + async def fake_verify(secret, token, remote_ip=None): + return token == "good-token" + + monkeypatch.setattr("meshbay_hub.captcha.verify_captcha", fake_verify) + + +def _body(**over): + b = {"username": "newbie", "email": "newbie@t.com", "auth_key": "a" * 44} + b.update(over) + return b + + +@pytest.mark.asyncio +async def test_missing_captcha_rejected_even_with_auth_key(client, captcha_on): + r = await client.post("/v1/users/register", json=_body()) + assert r.status_code == 400 + assert r.json()["detail"] == "captcha_required" + + +@pytest.mark.asyncio +async def test_bad_captcha_rejected(client, captcha_on): + r = await client.post("/v1/users/register", + json=_body(captcha_token="wrong")) + assert r.status_code == 400 + assert r.json()["detail"] == "captcha_failed" + + +@pytest.mark.asyncio +async def test_good_captcha_accepted(client, captcha_on): + r = await client.post("/v1/users/register", + json=_body(captcha_token="good-token")) + assert r.status_code == 201 + + +@pytest.mark.asyncio +async def test_no_captcha_configured_still_registers(client): + # Default test config has no captcha keys — registration proceeds without one. + r = await client.post("/v1/users/register", json=_body()) + assert r.status_code == 201 -- cgit v1.2.3 From 6b38704d459dec1271c7ca883de3eb14190218f7 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Tue, 1 Sep 2026 17:49:19 +0200 Subject: fix(hub): require user scope to add group members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every mutating group endpoint depends on require_user_scope except POST /v1/groups/{group_id}/members/{username}, which depended on get_current_user — so a node-scoped daemon token (or a stolen one) whose subject owns the group could add any existing user to it, contradicting NS7 ("operator manages groups from the browser only"). test_node_auth.py::test_node_scope_blocks_add_member already existed and was red on main; it passes now. Third security review, finding M6. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG --- packages/meshbay-hub/src/meshbay_hub/api/groups.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/meshbay-hub/src/meshbay_hub/api/groups.py b/packages/meshbay-hub/src/meshbay_hub/api/groups.py index fdb0444..15c7a5d 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/groups.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/groups.py @@ -586,9 +586,14 @@ async def update_group( async def add_group_member( group_id: str, username: str, - current_user: User = Depends(get_current_user), + current_user: User = Depends(require_user_scope), 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. group = await db.get(Group, group_id) if not group: raise HTTPException(status_code=404, detail="Group not found") -- cgit v1.2.3 From 30c032f2659fe697f7731dc9ae2cf8b8499177d4 Mon Sep 17 00:00:00 2001 From: Christophe Besson Date: Tue, 1 Sep 2026 17:49:28 +0200 Subject: fix(client): allow reCAPTCHA in the Electron CSP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Needed for the paired meshbay-hub commit that makes the registration captcha unconditional (M1): the desktop client renders the same RegisterPage widget the browser does, which needs its script, its challenge iframe and its assets to load. script-src, the new frame-src, and img-src now allow exactly https://www.google.com and https://www.gstatic.com, and nothing else external — the hub's own origin is still absent from script-src, so T3 (nothing the hub returns is executed) is unaffected. This is a one-time source change: it ships identical in every build via `files: ["src/**"]` in electron-builder's config, with no build step, packaging step, or installer action for anyone to perform, and no setting for an end user to touch. test_desktop_shell.py updated to pin the exception precisely: the reCAPTCHA hosts are the *only* external origins allowed anywhere in the policy, and a bare `https:` scheme is still refused in script-src. Third security review, finding M1 (Option A, desktop half). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG --- packages/meshbay-client/src/main.js | 18 ++++++++-- packages/meshbay-hub/tests/test_desktop_shell.py | 42 +++++++++++++++++++++--- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/packages/meshbay-client/src/main.js b/packages/meshbay-client/src/main.js index 3926c01..e3735e6 100644 --- a/packages/meshbay-client/src/main.js +++ b/packages/meshbay-client/src/main.js @@ -58,15 +58,29 @@ const SCHEME = 'app'; // The hub is reachable under connect-src, for its API and its signaling socket. // It is deliberately absent from script-src: nothing it returns is executed, // which is the whole reason this application exists (T3). +// +// The one exception is reCAPTCHA, used to gate sign-up (and password reset) the +// same way it gates them in the browser. Its script comes from www.google.com, +// its challenge is a www.google.com iframe, and its assets sit on +// www.gstatic.com. These two hosts — and only these two — are allowed under +// `script-src`, `frame-src` and `img-src` for that purpose. It is a real, if +// small, dent in "no third-party code runs here": Google's reCAPTCHA script +// executes in the renderer. It is accepted deliberately so a native sign-up is +// gated like a web one without asking the user to do anything extra, and it is +// the *same* dependency the hub-served SPA already carries. If sign-up ever +// moves to a proof-of-work challenge, delete RECAPTCHA_SRC and the three +// directives that spread it, and the widget in auth-page.js with them. +const RECAPTCHA_SRC = 'https://www.google.com https://www.gstatic.com'; const CSP = [ "default-src 'none'", - "script-src 'self' 'wasm-unsafe-eval'", + `script-src 'self' 'wasm-unsafe-eval' ${RECAPTCHA_SRC}`, "style-src 'self' 'unsafe-inline'", - "img-src 'self' data: blob:", + `img-src 'self' data: blob: ${RECAPTCHA_SRC}`, "media-src 'self' blob:", "font-src 'self'", "connect-src 'self' https: wss:", "worker-src 'self'", + `frame-src ${RECAPTCHA_SRC}`, "frame-ancestors 'none'", "base-uri 'none'", "form-action 'none'", diff --git a/packages/meshbay-hub/tests/test_desktop_shell.py b/packages/meshbay-hub/tests/test_desktop_shell.py index 36b261e..b82804e 100644 --- a/packages/meshbay-hub/tests/test_desktop_shell.py +++ b/packages/meshbay-hub/tests/test_desktop_shell.py @@ -175,11 +175,17 @@ def _policy() -> str: """ import re source = _main() + # The array mixes plain strings and one `${RECAPTCHA_SRC}` template literal; + # resolve the constant so every directive reads as plain text. + rec = re.search(r"const RECAPTCHA_SRC = '([^']*)'", source) match = re.search(r"const CSP = \[(.*?)\]\.join", source, re.S) assert match, "no CSP constant in the main process" + body = match.group(1) + if rec: + body = body.replace("${RECAPTCHA_SRC}", rec.group(1)) return "; ".join( - line.strip().strip('",').strip('"') - for line in match.group(1).splitlines() if line.strip()) + line.strip().strip('`",').strip('`"') + for line in body.splitlines() if line.strip()) def _directive(name: str) -> str: @@ -193,18 +199,46 @@ def _directive(name: str) -> str: def test_the_hub_is_reachable_but_never_executable(): """ connect-src allows the hub's API and its signaling socket. script-src does - not include it: nothing the hub returns is ever executed. + not: nothing the hub returns is ever executed. The only script sources are + 'self', the wasm eval token, and the two reCAPTCHA hosts (see the next + test) — never a bare `https:` scheme, which would let the hub's own origin + serve script. """ connect = _directive("connect-src") assert "https:" in connect and "wss:" in connect script = _directive("script-src") assert script, "no script-src directive" - assert "https:" not in script, "the hub can serve script under this policy" + sources = script.split()[1:] # drop the "script-src" keyword itself + allowed = { + "'self'", "'wasm-unsafe-eval'", + "https://www.google.com", "https://www.gstatic.com", + } + assert set(sources) <= allowed, \ + f"unexpected script-src source: {set(sources) - allowed}" + assert "https:" not in sources, "a bare https: scheme lets the hub serve script" assert "'unsafe-eval'" not in script.replace("'wasm-unsafe-eval'", "") assert "default-src 'none'" in _policy() +def test_recaptcha_is_the_only_third_party_and_stays_scoped_to_it(): + """ + reCAPTCHA gates sign-up in the app the same way it does in the browser. + www.google.com and www.gstatic.com are allowed under script-src, frame-src + and img-src for that — and no other external origin appears anywhere in the + policy. Remove this expectation only alongside the reCAPTCHA widget. + """ + hosts = {"https://www.google.com", "https://www.gstatic.com"} + for directive in ("script-src", "frame-src", "img-src"): + srcs = set(_directive(directive).split()[1:]) + assert hosts <= srcs, f"{directive} is missing a reCAPTCHA host" + + for part in _policy().split(";"): + for tok in part.strip().split()[1:]: + if tok.startswith(("http://", "https://")): + assert tok in hosts, f"unexpected external origin in CSP: {tok}" + + # ── The bridge ────────────────────────────────────────────────────────────── def test_the_bridge_is_the_only_way_in(): -- cgit v1.2.3 From 73a119cbc7641b5f0005d0ce8bc4c7b0b7ae6a6e Mon Sep 17 00:00:00 2001 From: Christophe Besson 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 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 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://:` — 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//` (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 `` is removed (nothing has ever read it) so `script-src` needs no inline allowance. Needs verification 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. Second-review L5 / third-review M5. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG --- packages/meshbay-hub/src/meshbay_hub/api/webapp.py | 31 ++++++++++- packages/meshbay-hub/src/meshbay_hub/app.py | 17 +++++- .../meshbay-hub/tests/test_security_headers.py | 65 ++++++++++++++++++++++ 3 files changed, 110 insertions(+), 3 deletions(-) create mode 100644 packages/meshbay-hub/tests/test_security_headers.py diff --git a/packages/meshbay-hub/src/meshbay_hub/api/webapp.py b/packages/meshbay-hub/src/meshbay_hub/api/webapp.py index 0821809..6dfd3ed 100644 --- a/packages/meshbay-hub/src/meshbay_hub/api/webapp.py +++ b/packages/meshbay-hub/src/meshbay_hub/api/webapp.py @@ -22,7 +22,8 @@ STATIC_DIR = Path(__file__).parent.parent / "static" router = APIRouter(tags=["webapp"]) # Assets the shell pulls in, in load order. Everything else is imported by -# app.js and rides on the same query string via window.__MB_ASSET_V. +# app.js from a relative path, which inherits the `/a//` prefix the shell +# loaded app.js under — so the whole module graph moves together. # Every module the page loads. A file missing from here is a file whose change # does not move the URL, so a browser holding the old one never asks for it — # which is the failure this list exists to prevent, and it is silent. @@ -77,6 +78,33 @@ ASSET_V = _asset_version() _NO_STORE = {"Cache-Control": "no-store"} +# Content-Security-Policy for the whole hub, applied by a middleware in app.py. +# +# This is the *same* policy the desktop client's protocol handler already sends +# for these exact UI files (`meshbay-client/src/main.js`), plus the two reCAPTCHA +# hosts the sign-up widget loads its script, challenge iframe and images from. +# `'unsafe-inline'` is style-only — htm/preact set inline `style=` attributes +# everywhere; nothing inline executes, and the shell below carries no inline +# ` diff --git a/packages/meshbay-hub/src/meshbay_hub/app.py b/packages/meshbay-hub/src/meshbay_hub/app.py index 76d7ec0..2daa55b 100644 --- a/packages/meshbay-hub/src/meshbay_hub/app.py +++ b/packages/meshbay-hub/src/meshbay_hub/app.py @@ -35,7 +35,7 @@ from meshbay_hub.api.relay import router as relay_router from meshbay_hub.api.signaling import router as signaling_router from meshbay_hub.api.admin import router as admin_router from meshbay_hub.api.notifications import router as notifications_router -from meshbay_hub.api.webapp import router as webapp_router, STATIC_DIR, ASSET_V +from meshbay_hub.api.webapp import router as webapp_router, STATIC_DIR, ASSET_V, CSP from meshbay_hub.api.middleware import limiter @@ -142,6 +142,21 @@ def create_app(cfg: HubConfig | None = None) -> FastAPI: app.state.limiter = limiter app.add_exception_handler(RateLimitExceeded, _rate_limit_exceeded_handler) + @app.middleware("http") + async def _security_headers(request, call_next): + """ + Second-review L5, third-review M5: the SPA shell and its assets went out + with no CSP and no other protective headers. This adds them everywhere — + `webapp.CSP` is the same policy the desktop client already enforces on + these exact files. `setdefault` so a route that sets its own wins. + """ + response = await call_next(request) + response.headers.setdefault("Content-Security-Policy", CSP) + response.headers.setdefault("X-Content-Type-Options", "nosniff") + response.headers.setdefault("Referrer-Policy", "strict-origin-when-cross-origin") + response.headers.setdefault("X-Frame-Options", "DENY") + return response + # Routers (webapp last — catches / before API routes) app.include_router(hub_router) app.include_router(users_router) diff --git a/packages/meshbay-hub/tests/test_security_headers.py b/packages/meshbay-hub/tests/test_security_headers.py new file mode 100644 index 0000000..b4d7e6d --- /dev/null +++ b/packages/meshbay-hub/tests/test_security_headers.py @@ -0,0 +1,65 @@ +""" +The hub sends a Content-Security-Policy and the other protective headers on +every response — the SPA shell, its assets, and the API alike. + +Second-review L5 / third-review M5: previously there were none, so an injection +that landed in the SPA (rendered third-party OG data, a federated group name, +chat content) had nothing stopping it from loading more code or exfiltrating. +""" + +import pytest +from meshbay_hub.api.webapp import CSP + + +def _directive(csp: str, name: str) -> str: + for part in csp.split(";"): + part = part.strip() + if part == name or part.startswith(name + " "): + return part + return "" + + +@pytest.mark.asyncio +async def test_the_spa_shell_carries_the_policy(client): + r = await client.get("/") + assert r.headers["content-security-policy"] == CSP + assert r.headers["x-content-type-options"] == "nosniff" + assert r.headers["x-frame-options"] == "DENY" + assert "referrer-policy" in r.headers + + +@pytest.mark.asyncio +async def test_the_api_carries_the_headers_too(client): + r = await client.get("/v1/health") + assert r.status_code == 200 + assert "content-security-policy" in r.headers + assert r.headers["x-content-type-options"] == "nosniff" + + +@pytest.mark.asyncio +async def test_even_a_404_carries_the_headers(client): + # The middleware runs on every response, so a probe for a missing path + # cannot be framed or content-sniffed either. + r = await client.get("/no/such/path") + assert r.status_code == 404 + assert r.headers["x-frame-options"] == "DENY" + + +def test_the_policy_is_locked_down_where_it_matters(): + assert "default-src 'none'" in CSP # covers object-src, etc. + assert _directive(CSP, "frame-ancestors") == "frame-ancestors 'none'" + assert _directive(CSP, "base-uri") == "base-uri 'none'" + + script = _directive(CSP, "script-src") + # The hub's own origin must not be able to serve executable script (T3): + # 'self' and the wasm token are fine, a bare `https:` scheme is not. + assert "'self'" in script and "'wasm-unsafe-eval'" in script + assert "https:" not in script.split() + + +def test_recaptcha_is_the_only_external_origin(): + hosts = {"https://www.google.com", "https://www.gstatic.com"} + for part in CSP.split(";"): + for tok in part.strip().split()[1:]: + if tok.startswith(("http://", "https://")): + assert tok in hosts, f"unexpected external origin in CSP: {tok}" -- cgit v1.2.3 From a9eb121476eb8128e0cf1ae280ed0fea84d4b2aa Mon Sep 17 00:00:00 2001 From: Christophe Besson 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 Claude-Session: https://claude.ai/code/session_011pG75yGK3NthNfyjH74omG --- docs/third-review.md | 83 ++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 57 insertions(+), 26 deletions(-) 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' ` — 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//` 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 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 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(-) 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 `