# Consolidated Codebase Review — quicproquo **Date:** 2026-03-04 **Reviewers:** 4 independent agents (security, architecture, code quality, correctness) **Scope:** Full codebase — all workspace crates, schemas, Cargo.toml --- ## CRITICAL (7 findings) ### C1. Federation service has NO authentication on inbound requests **Source:** Security | **File:** `crates/quicproquo-server/src/federation/service.rs:22-201` `FederationServiceImpl` handles inbound federation requests (`relay_enqueue`, `relay_batch_enqueue`, `proxy_fetch_key_package`, `proxy_resolve_user`) but performs **zero authentication** on the caller. The `auth` field in the request is only logged (`origin` string), never validated. While mTLS protects the transport, any server with a valid federation certificate can inject arbitrary messages, enumerate users, and fetch KeyPackages. The `FederationAuth.origin` field is a self-declared string, not verified against the mTLS certificate's subject. **Fix:** Validate `origin` against the mTLS client certificate's CN/SAN. Enforce per-peer rate limits. Consider signing federation messages at the application layer. ### C2. WebSocket bridge bypasses DM channel authorization **Source:** Security | **File:** `crates/quicproquo-server/src/ws_bridge.rs:230-305` The `handle_send` function lets any authenticated user enqueue a message to any recipient, bypassing the DM channel membership verification that the Cap'n Proto `enqueue` path enforces in `delivery.rs:93-135`. The WS bridge calls `store.enqueue()` directly, skipping channel membership auth, payload size limits (5 MB), rate limiting, hook invocations, delivery proof generation, and audit logging. **Fix:** Apply the same authorization, size limits, rate limiting, hooks, and audit logging as the Cap'n Proto delivery path. ### C3. `hpke_seal` silently returns empty ciphertext on error **Source:** Quality | **File:** `crates/quicproquo-core/src/hybrid_crypto.rs:198-201,216-219` `HybridCrypto::hpke_seal()` catches errors and returns `Ok(vec![])` instead of propagating. Empty ciphertexts are sent as if valid — data loss and security issue. **Fix:** Propagate errors via `Result`. ### C4. NodeServiceImpl god object (15 fields, 27 RPC methods, no capability segmentation) **Source:** Architecture | **File:** `crates/quicproquo-server/src/node_service/mod.rs:253-336` Single struct implements 27 methods spanning auth, delivery, key management, channels, blobs, devices, federation, and account lifecycle. `handle_node_connection` takes 15 parameters. Unauthenticated clients get capability to invoke all 27 methods. **Fix:** Extract `ServerContext` struct. Split Cap'n Proto schema into capability interfaces (AuthService, DeliveryService, etc.) vended after auth. ### C5. FileBackedStore O(n) full-map serialization on every mutation **Source:** Architecture | **File:** `crates/quicproquo-server/src/storage.rs:327-442` Every write locks Mutex, mutates HashMap, serializes **entire** map to disk via `fs::write`. No fsync, no atomic rename. Performance cliff and data-loss vector. **Fix:** Make SqlStore the default. If FileBackedStore remains for dev, use write-to-temp-then-rename. ### C6. `std::sync::Mutex` in async context (server and P2P) **Source:** Architecture | **Files:** `storage.rs:1-7,25-28`, `sql_store.rs:4,50`, `node_service/mod.rs:272` Holding `std::sync::Mutex` across disk I/O blocks Tokio worker threads, causing head-of-line blocking. **Fix:** Replace with `tokio::sync::Mutex` or use `spawn_blocking` for disk I/O. ### C7. `hpke_setup_sender_and_export` silently downgrades to classical crypto on parse error **Source:** Security | **File:** `crates/quicproquo-core/src/hybrid_crypto.rs:263` On invalid hybrid public key, silently falls back to classical RustCrypto provider. Malicious server can force PQ downgrade. **Fix:** Return error on hybrid key parse failure, consistent with `hpke_seal`. --- ## HIGH (14 findings) ### H1. Global `AUTH_CONTEXT: RwLock>` **Source:** Architecture + Quality | **File:** `crates/quicproquo-client/src/lib.rs:36,40,82` Blocks multi-account, creates hidden coupling, root cause of `AUTH_LOCK` test serialization hack. **Fix:** Replace with `ClientContext` struct passed to all functions. ### H2. Store trait is 30+ method monolith **Source:** Architecture | **File:** `crates/quicproquo-server/src/storage.rs:33-180` Any new storage backend must implement every method. Cannot be composed or partially implemented. **Fix:** Split into sub-traits: `KeyPackageStore`, `DeliveryStore`, `UserStore`, `ChannelStore`, etc. ### H3. CoreError::Mls wraps errors as String, losing type info **Source:** Architecture | **File:** `crates/quicproquo-core/src/error.rs:16-17` Impossible to match on specific MLS error conditions. **Fix:** Create MLS sub-error variants or wrap boxed error. ### H4. Proto `from_bytes` uses default 64 MiB traversal limit **Source:** Architecture | **File:** `crates/quicproquo-proto/src/lib.rs:67-72` DoS amplification vector for direct callers (client, bot, FFI). **Fix:** Accept `ReaderOptions` as parameter, make default stricter. ### H5. Mobile crate hardcodes SkipServerVerification **Source:** Architecture + Security | **File:** `crates/quicproquo-mobile/src/lib.rs:93-100,165-172` Unconditionally skips TLS verification. Inherently MITM-vulnerable. **Fix:** Add certificate_verifier parameter or feature flag. ### H6. Duplicate InsecureServerCertVerifier implementations **Source:** Architecture | **Files:** `client/rpc.rs:27-29`, `mobile/lib.rs:165-167` **Fix:** Consolidate into shared crate behind `cfg(feature = "insecure")`. ### H7. DiskKeyStore writes HPKE private keys to disk unencrypted **Source:** Security | **File:** `crates/quicproquo-core/src/keystore.rs` No encryption, no file permissions. HPKE private keys are MLS epoch secrets. **Fix:** Encrypt with Argon2id + ChaCha20-Poly1305. Set 0o600 permissions. ### H8. `identity.rs:seed_bytes()` returns unzeroized copy of secret seed **Source:** Security | **File:** `crates/quicproquo-core/src/identity.rs:52` Copies 32-byte Ed25519 seed out of `Zeroizing` wrapper. Returned value not zeroized. **Fix:** Return `&[u8]` reference or `Zeroizing<[u8; 32]>`. ### H9. `hybrid_kem.rs:private_to_bytes()` returns unzeroized `Vec` **Source:** Security | **File:** `crates/quicproquo-core/src/hybrid_kem.rs:162` Hybrid private key material lingers in memory. **Fix:** Return `Zeroizing>`. ### H10. MeshIdentity stores Ed25519 seed as plaintext JSON **Source:** Security | **File:** `crates/quicproquo-p2p/src/identity.rs:72-79` No encryption, no file permissions. **Fix:** Encrypt identity file, set 0o600 permissions. ### H11. WebSocket bridge has rate_limits field but never checks it **Source:** Security | **File:** `crates/quicproquo-server/src/ws_bridge.rs:28-36` **Fix:** Apply `check_rate_limit()` in all WS bridge handlers. ### H12. ~100 lines duplicated between `receive_message` and `receive_message_with_sender` **Source:** Quality | **File:** `crates/quicproquo-core/src/group.rs:471-583` Bug fix in one must be manually replicated to other. Security-critical MLS code. **Fix:** Extract shared MLS message processing helper. ### H13. Synchronous file I/O in async blob handler **Source:** Quality | **File:** `crates/quicproquo-server/src/node_service/blob_ops.rs:124-137` Blocking `std::fs` calls in async handler stall event loop. **Fix:** Use `tokio::fs` or `spawn_blocking`. ### H14. `MeshEnvelope::forwarded()` invalidates signature without re-signing **Source:** Quality + Correctness + Security | **File:** `crates/quicproquo-p2p/src/envelope.rs:172-176` Increments `hop_count` included in signed bytes. All forwarded envelopes fail `verify()`. **Fix:** Exclude `hop_count` from signature, or add separate forwarding signature. --- ## MEDIUM (19 findings) ### M1. fetch_wait TOCTOU: missed notification window **Source:** Correctness | **File:** `crates/quicproquo-server/src/node_service/delivery.rs:496-522` TOCTOU between initial fetch (empty) and waiter registration. Enqueue between these points fires notify before waiter exists. **Fix:** Register waiter before initial fetch. ### M2. `verify_transcript_chain` never checks hashes — misleading name **Source:** Correctness | **File:** `crates/quicproquo-core/src/transcript.rs:215-251` Only validates structural integrity, not hash chain. Name implies verification. **Fix:** Rename to `validate_transcript_structure` or implement actual chain verification. ### M3. Non-atomic file writes in FileBackedStore **Source:** Correctness | **File:** `crates/quicproquo-server/src/storage.rs:332-337` (all flush_* methods) `fs::write` directly — crash mid-write corrupts file, loses all data. **Fix:** Use `tempfile::NamedTempFile` + `persist()`. ### M4. `delete_account` non-atomic multi-lock **Source:** Correctness | **File:** `crates/quicproquo-server/src/storage.rs:800-864` 6 sequential Mutex locks. Concurrent fetch could see partially deleted account. **Fix:** Use single transaction or hold all locks simultaneously. ### M5. Timing side channel in `resolveIdentity` — no timing floor **Source:** Security | **File:** `crates/quicproquo-server/src/node_service/user_ops.rs:142-178` Unlike `resolveUser` which has 5ms floor. **Fix:** Apply same `RESOLVE_TIMING_FLOOR`. ### M6. WS bridge `resolve_user` has no timing floor **Source:** Security | **File:** `crates/quicproquo-server/src/ws_bridge.rs:158-181` **Fix:** Add same timing floor as Cap'n Proto handler. ### M7. `AuthContext.token` not zeroized **Source:** Security | **File:** `crates/quicproquo-server/src/auth.rs:68-72` **Fix:** Wrap in `Zeroizing>`. ### M8. Client `ClientAuth.access_token` not zeroized **Source:** Security | **File:** `crates/quicproquo-client/src/lib.rs:50-55` **Fix:** Use `Zeroizing>`. ### M9. `SessionState.password` stores plaintext password in memory **Source:** Security | **File:** `crates/quicproquo-client/src/client/session.rs:29` **Fix:** Use `Zeroizing`, derive key at startup and zeroize password. ### M10. `conversation.rs:172` hex-encodes derived key without zeroization **Source:** Security | **File:** `crates/quicproquo-client/src/client/conversation.rs:172` **Fix:** Use `Zeroizing` for `hex_key`. ### M11. `device_ops.rs:49` uses `.unwrap_or("")` on untrusted input **Source:** Security | **File:** `crates/quicproquo-server/src/node_service/device_ops.rs:49` **Fix:** Return error for invalid UTF-8. ### M12. `MeshEnvelope::forwarded()` invalidates signature (duplicate of H14) **Source:** Security | **File:** `crates/quicproquo-p2p/src/envelope.rs:172-176` ### M13. FileBackedStore `create_channel` O(n) linear scan **Source:** Architecture + Quality + Correctness | **File:** `crates/quicproquo-server/src/storage.rs:749-765` **Fix:** Secondary index or deterministic channel ID from member pair hash. ### M14. `resolve_identity_key` O(n) linear scan **Source:** Architecture + Quality | **File:** `crates/quicproquo-server/src/storage.rs:676-684` **Fix:** Maintain reverse map. ### M15. FFI error classification by string matching **Source:** Architecture | **File:** `crates/quicproquo-ffi/src/lib.rs:183` **Fix:** Match on typed error variants. ### M16. Documentation drift: master-prompt.md says Noise/TCP, code uses QUIC/TLS **Source:** Architecture | **Files:** `master-prompt.md`, server/client `Cargo.toml` **Fix:** Update master-prompt.md to reflect actual transport. ### M17. Plugin `HookVTable` unsafe Send+Sync without safety docs **Source:** Architecture | **File:** `crates/quicproquo-plugin-api/src/lib.rs:190-192` **Fix:** Add `// SAFETY:` documentation blocks. ### M18. OPAQUE register_finish: spurious RegistrationRequest deserialization **Source:** Quality + Correctness | **File:** `crates/quicproquo-server/src/node_service/auth_ops.rs:335-343` Dead code — deserializes upload_bytes as wrong type first. **Fix:** Remove lines 335-343. ### M19. Mixed serialization formats in DiskKeyStore (bincode + serde_json) **Source:** Quality | **File:** `crates/quicproquo-core/src/keystore.rs` **Fix:** Standardize on one format. --- ## LOW (23 findings) ### L1. `BroadcastChannel.key` not zeroized on drop **File:** `crates/quicproquo-p2p/src/broadcast.rs:18-19` ### L2. Plugin loader `CStr::from_ptr` on plugin-returned string (UB risk) **File:** `crates/quicproquo-server/src/plugin_loader.rs:102` ### L3. Token cache stores session token as plaintext hex when no password set **File:** `crates/quicproquo-client/src/client/token_cache.rs:63-68` ### L4. `--password` CLI flag visible in process list **File:** `crates/quicproquo-client/src/main.rs:104` ### L5. `clippy::unwrap_used` is warn not deny **File:** `Cargo.toml:88` ### L6. `strip = "symbols"` hinders post-mortem debugging **File:** `Cargo.toml:94` ### L7. `thread_rng()` for channel ID generation instead of `OsRng` **Files:** `storage.rs:760`, `sql_store.rs:545` ### L8. `conversation.rs:548` — `limit` cast from `usize` to `u32` without saturation **File:** `crates/quicproquo-client/src/client/conversation.rs:548` ### L9. `conversation.rs:363-365,420-422` — bincode deserialize errors silently dropped **File:** `crates/quicproquo-client/src/client/conversation.rs` ### L10. `repl.rs:610` — static AtomicU64 for padding timer instead of session state **File:** `crates/quicproquo-client/src/client/repl.rs:610` ### L11. `command_engine.rs:148` — `to_slash()` clones entire Command enum unnecessarily **File:** `crates/quicproquo-client/src/client/command_engine.rs:148` ### L12. `conversation.rs:201-203` — SQL ATTACH with format string **File:** `crates/quicproquo-client/src/client/conversation.rs:201-203` ### L13. Client `hex.rs` trivial wrapper with zero value-add **File:** `crates/quicproquo-client/src/client/hex.rs` ### L14. `config.rs` `#[allow(dead_code)]` on EffectiveFederationConfig **File:** `crates/quicproquo-server/src/config.rs` ### L15. `federation/address.rs` `#[allow(dead_code)]` on entire module **File:** `crates/quicproquo-server/src/federation/address.rs` ### L16. ANSI escape codes hardcoded without terminal capability detection **File:** `crates/quicproquo-client/src/client/display.rs` ### L17. GUI `lib.rs:75` `.expect()` on Tauri run **File:** `crates/quicproquo-gui/src/lib.rs:75` ### L18. `session.rs:186` DiskKeyStore failure silently falls back to ephemeral **File:** `crates/quicproquo-client/src/client/session.rs:186` ### L19. `retry.rs:37` `thread_rng()` for jitter instead of `OsRng` **File:** `crates/quicproquo-client/src/client/retry.rs:37` ### L20. `MeshStore.seen` set grows unboundedly **File:** `crates/quicproquo-p2p/src/` (MeshStore) ### L21. `envelope.rs:58` `.expect()` on system clock in non-test code **File:** `crates/quicproquo-p2p/src/envelope.rs:58` ### L22. `broadcast.rs:47` `.expect()` on encryption in non-test code **File:** `crates/quicproquo-p2p/src/broadcast.rs:47` ### L23. Bot crate hardcodes sender as "peer" (TODO) **File:** `crates/quicproquo-bot/src/lib.rs` --- ## TESTING GAPS (6 findings) ### T1. No unit tests for `plugin_loader.rs` ### T2. No unit tests for `federation/tls.rs` ### T3. No tests for `blob_ops.rs` ### T4. No tests for `delivery.rs` ### T5. `conversation.rs` migration code untested ### T6. No negative test for `MeshEnvelope::verify()` with wrong key --- ## Strengths (positive findings from security audit) - Constant-time token comparison via `subtle::ConstantTimeEq` - Parameterized SQL everywhere (no injection) - Proper Argon2id (19 MiB, t=2, p=1) + ChaCha20-Poly1305 for client state - Zero `.unwrap()` in non-test server code (grep-verified) - TLS 1.3 only, mTLS for federation - Rate limiting: 100 enqueues/60s, 50 connections/IP/60s - Delivery proof signing: SHA-256(seq||recipient||timestamp) + Ed25519 - KeyPackage ciphersuite validation (only 0x0001 accepted) - Payload size limits: 5 MB message, 50 MB blob, 1 MB KeyPackage - Queue depth limits: 1000/inbox, 100K sessions, 100K waiters - Blob path traversal protection via hex-encoded hash - Audit logging with secret redaction (identity keys prefix-only) - Production config validation (rejects devtoken, empty auth, missing TLS) --- ## Recommended fix priority 1. **Federation auth** (C1) — auth-gate inbound requests, validate origin against mTLS cert 2. **WS bridge authz** (C2) + rate limits (H11) + timing floors (M6) — parity with Cap'n Proto path 3. **Crypto error propagation** (C3, C7) — hpke_seal and hpke_setup_sender_and_export 4. **Zeroization sweep** (H8, H9, H10, M7-M10, L1) — all leaked secret material 5. **ServerContext extraction** (C4) — foundation for capability-based security 6. **FileBackedStore atomic writes** (C5, M3) — prevent data loss on crash 7. **std::sync::Mutex → tokio::sync::Mutex** (C6) — unblock Tokio workers 8. **Mobile TLS verification** (H5) — remove hardcoded skip 9. **fetch_wait TOCTOU** (M1) — register waiter before fetch 10. **Testing gaps** (T1-T6) — critical untested paths