diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/third-review.md | 81 |
1 files changed, 56 insertions, 25 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' <recaptcha>` — the hub origin is not a script source, +> `frame-ancestors 'none'`, `base-uri 'none'`, `form-action 'none'`), plus the +> reCAPTCHA hosts. The shell's dead `window.__MB_ASSET_V` inline script is +> removed so no inline `'unsafe-inline'`/nonce is needed for scripts. **Wants a +> pass against the running SPA** — a mis-tuned CSP shows as a blank page — but it +> matches a policy already proven with these files under Electron. SRI on the +> `/a/<hash>/` scripts is still not done (same-origin, so lower value than the +> CSP). Tests in `test_security_headers.py`. + **Location:** `api/webapp.py:80-123`, `app.py:160-214` The SPA shell is returned with only `Cache-Control: no-store`. There is no @@ -596,7 +626,7 @@ Against the v6 §4 claims, updated for this review: | File content unreadable by the hub | ✅ | ✅ for content | — | GEK never reaches the hub; invite rewrite closed H3 | | Node operator is sole content authority | ✅ | ✅ | ✅ since 2026-09-01 — QUIC chat/stream handlers brought to WebRTC parity, and the QUIC listener is off by default (was M2) | | Mutual node authentication | ✅ | ✅ | ✅ | New handshake + `transport.js` pin — a real improvement | -| Immediate revocation | ✅ | ✅ locally | — | Persisted denylist, group targets handled. **Does not federate** (M4) | +| Immediate revocation | ✅ | ✅ locally | — | Persisted denylist, group targets handled. Federation prunes the peer's directory entry (was M4); it does not reach nodes, and nothing local hosts a federated group | | Suspending/revoking a group blocks connections | ✅ | ✅ | — | `webrtc_offer` checks status; node drops sessions on `revoke` | | Device linking safe against the hub | ✅ | ✅ | ⚠️ browser link inherits T3 (documented) | Countersignature by a pinned device; hub holds no user keys | | Chat authenticated between members | ❌ not yet | ❌ | ❌ | Sender Keys is Phase 15; today chat is node-asserted on every transport (M2a's wire-asserted QUIC path was closed 2026-09-01) | @@ -605,10 +635,10 @@ Against the v6 §4 claims, updated for this review: | Moderator ≠ administrator | ✅ | — | — | ✅ since 2026-09-01 — `admin_patch_user` split by field (was H1) | **One-sentence version:** *the E2E story between browser and node is now -genuinely mutual and covers every path — the second review's critical gaps are -closed, and H1/H2/M1/M2/M3/M6 were fixed the day this was written — leaving MHP -federation trust (M4) and the missing SPA security headers (M5) as hardening, -not holes.* +genuinely mutual and covers every path, the second review's critical gaps are +closed, and every finding in this review (H1, H2, M1–M6) was fixed the day it was +written — leaving the L-list as opportunistic hardening and one thing to verify: +the SPA's new CSP against the live app.* --- @@ -621,8 +651,8 @@ not holes.* | M1 | Registration CAPTCHA inert | Medium | S | ✅ **fixed 2026-09-01** — gate unconditional; desktop renders the widget | | M2 | QUIC chat: `sender_id` spoof, cross-group broadcast, sync ffmpeg | Medium | M | ✅ **fixed 2026-09-01** — handlers at WebRTC parity + `quic_enabled` off by default | | M3 | Link-preview SSRF: no rate limit, ports open, rebinding | Medium | M | ✅ **fixed 2026-09-01** — rate limit + port allowlist + connect-address re-check + bomb guard | -| M4 | Federation: peer over-trust, `aud` unchecked, revoke no-op | Medium | M | Before enabling MHP with any non-self peer | -| M5 | No CSP / security headers on the SPA | Medium | S | Opportunistic — cheap, high value given T3 | +| M4 | Federation: peer over-trust, `aud` unchecked, revoke no-op | Medium | M | ✅ **fixed 2026-09-01** — source bound to signer, push capped, revoke prunes the peer's own entries, replay rejected | +| M5 | No CSP / security headers on the SPA | Medium | S | ✅ **fixed 2026-09-01** — CSP + `nosniff` + `frame-ancestors` middleware; verify against the live SPA | | M6 | `add_group_member` accepts node tokens | Medium | S | ✅ **fixed 2026-09-01** — dependency → `require_user_scope` | | L1 | Relay registration no PoP | Low | S | If/when the relay registry is used | | L2 | Orphaned `replication.py` / `revocation.py` | Low | S | Delete now | @@ -659,13 +689,14 @@ engineering, and most of the second review's C- and H-list is genuinely closed. The new findings are narrower and more uniform in shape than last time: a role check that grants too much, an anti-abuse endpoint with no abuse protection, a -CAPTCHA wired to a condition the real client never meets, and a transport that -received the new authentication but not the new authorization. None of them -required exotic capability, and none of them were architectural — they were the -cost of adding six subsystems faster than the authorization model grew to cover -them. +CAPTCHA wired to a condition the real client never meets, a transport that +received the new authentication but not the new authorization, an over-trusted +federation peer, and a missing header policy. None of them required exotic +capability, and none of them were architectural — they were the cost of adding +six subsystems faster than the authorization model grew to cover them. -H1, H2, 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. +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. |