@@ -0,0 +1,172 @@
1+# PROTOCOL.md Review — Holes vs. Implementation
2+
3+**Date:** 2026-06-20
4+**Scope:** Cross-check of gaps found in `PROTOCOL.md` against the actual code
5+(`server.knot`, `client/`). Each item records whether the implementation already
6+handles it, partially handles it, or leaves it open.
7+
8+Legend: ✅ fixed · 🟡 partial / mitigated · ❌ open · ⚪ not-a-bug (spec wording only)
9+
10+---
11+
12+## ✅ 1. Auth challenge is not bound to the server (session relay) — fixed
13+
14+**Severity: high — fixed.**
15+
16+The client signed the bare challenge string and the server verified the bare
17+challenge string. Nothing bound the signed material to a server identity, so a
18+relay could forward another server's challenge and replay the signature.
19+
20+**Attack (historical):** a malicious/relay server the client connects to fetched
21+a challenge for the client's pubkey from a legit server A, handed it to the
22+client, collected the signature, and replayed it to A's `/auth/verify` —
23+obtaining a session **as the client** on A (could `/poll` the client's mailbox
24+and send as them). The challenge's IP-binding (`matchIp`) did not help, because
25+the relay server was the client-of-record to A throughout.
26+
27+**Fix (applied):** the client now signs
28+`"skrepka-auth-v1:" + server_host + ":" + challenge` where `server_host` is the
29+bare lowercased hostname it dialed (`client/ApiClient.fs`, `Crypto.fs`
30+`signChallenge`), and the server verifies against
31+`"skrepka-auth-v1:" ++ beforeColon serverHost ++ ":" ++ challenge` using its own
32+configured `serverHost` (`server.knot` `handleVerifyAuth`). The `skrepka-auth-v1:`
33+prefix is a domain-separation tag so an auth signature can never be confused with
34+a message signature (`recipientPub || compressed`, `Crypto.fs`). Breaking change:
35+client and server must be upgraded together.
36+
37+---
38+
39+## ❌ 2. Gossip presence spoofing redirects a victim's mail to the attacker
40+
41+**Severity: high — open (consequence of open federation).**
42+
43+`/federation/gossip` is unauthenticated. On a newly-`online` event the origin
44+forwards the recipient's queued messages **to the announcing `fromServer`**.
45+
46+- `server.knot:1090` — `fork (forEach newlyOnline (\pk -> forwardQueuedToServer pk fromServer))`.
47+
48+**Attack:** a hostile peer announces `{eventType:"online", pubkey:<victim>,
49+fromServer:<attacker>}`. The origin forwards all of the victim's queued
50+ciphertext to the attacker, who harvests ciphertext copies + recipient/timing/size
51+metadata. The receiver-side "recipient must be locally online" gate is checked by
52+the attacker's own server, so it is not a real gate.
53+
54+**Existing partial defense:** `isBadServerName` SSRF deny-list (`server.knot:505`)
55+blocks internal targets; rate-limited 60/window/IP. Neither authenticates the peer.
56+
57+`PROTOCOL.md` §10 understates this as "a hostile server can observe gossip" — it
58+can actively **redirect delivery**. At minimum the spec should say so; a real fix
59+needs signed/authenticated presence or peer allow-listing.
60+
61+---
62+
63+## 🟡 3. Replay protection is narrower than §10 claims
64+
65+**Severity: medium — partially fixed.**
66+
67+§10 claims "Replay protection"; the only mechanism is the recipient-bound
68+signature, which stops replay to a *different* recipient, not re-delivery of the
69+same blob to the same recipient. Per-type status:
70+
71+- `text` — ✅ protected: deduped by `id` (`client/App.fs:38`).
72+- `delivery.ack` — ✅ harmless: `markDelivered` only flips `Sent → Delivered`,
73+ idempotent (`client/App.fs:43`).
74+- `profile` — ❌ **open**: `withProfile` overwrites name/bio/photo
75+ unconditionally (`client/App.fs:66`). The payload `ts` is parsed
76+ (`client/Protocol.fs:53`) but **not** propagated to `ProfileMessage` or
77+ compared, so a replayed *older* profile blob silently rolls a contact's
78+ profile back.
79+
80+**Fix:** carry `ts` into `Envelope.ProfileMessage` and ignore a profile whose
81+`ts` is older than the last one stored for that contact. Tighten §10 wording to
82+"replay against a *different* recipient."
83+
84+---
85+
86+## 🟡 4. Open `/federation/forward` allows mailbox injection
87+
88+**Severity: medium — mitigated, not closed.**
89+
90+Any host can POST a blob for any locally-online `toKey`; the endpoint is
91+unauthenticated.
92+
93+- Mitigations present: per-IP rate limit 600/window (`server.knot:140/314`),
94+ SSRF filter on origins, `no_presence` gate.
95+- Residual: junk blobs still reach an online recipient's poll stream; the spec
96+ never says what a client does with an undecryptable blob (it is dropped at
97+ `Crypto.decrypt`, `client/Protocol.fs:88`). Worth documenting as an accepted,
98+ rate-limited spam surface.
99+
100+---
101+
102+## ⚪ 5. `receivedAt` "millisecond timestamp" vs "monotonic" — wording only
103+
104+**Not a bug.** `appendMessage` assigns `seqTs = max t (currentSeq seqRows + 1)`
105+(`server.knot:653`), which is **strictly monotonic**, so no two messages share a
106+`receivedAt` and the `cursor >= receivedAt` implicit-ack cannot drop an unseen
107+message. The spec just describes the same value as both a "millisecond timestamp"
108+and "monotonic," which reads as ambiguous. Recommend: describe it as "a
109+monotonic sequence seeded from millisecond wall-clock; treat as an opaque
110+checkpoint, not a clock."
111+
112+---
113+
114+## ✅ 6. Total request-body cap — fixed in deployment, undocumented in spec
115+
116+The per-blob cap is `maxBlobLen = 41943040` (40 MiB hex) enforced by the
117+`BlobHex` type (`server.knot:53/198`), and `maxBatchSize = 100`
118+(`server.knot:56`). The naive reading "100 × 40 MiB = 4 GiB request" is
119+**prevented** by a runtime body cap set in deployment:
120+
121+- `install.sh` — `ExecStart=… --http-max-body-bytes=42M` caps the *entire*
122+ request body (chosen just above `maxBlobLen` + envelope; the runtime default is
123+ 16 MiB).
124+
125+So a batch can carry one max-size blob or many small ones, but total ≤ 42 MiB.
126+`PROTOCOL.md` §5 should state this total-body cap (and that operators must keep
127+`--http-max-body-bytes` > `maxBlobLen`).
128+
129+---
130+
131+## ❌ 7. No crypto / wire-format version negotiation
132+
133+**Severity: low — open.**
134+
135+`info = "skrepka-v1"` is hardcoded (`client/Crypto.fs:178` / `Constants`), and
136+neither the envelope nor the plaintext payload carries a version field. "Ignore
137+unknown `type`" gives payload-type agility but no path to rotate the
138+AEAD/KDF/curve. Recommend a stated version field or an explicit "no crypto
139+agility in v0.1" note.
140+
141+---
142+
143+## ⚪ 8. X-Forwarded-For trust is spoofable without a proxy — by design
144+
145+`trustForwardedFor = True` by default (`server.knot:148`); the session/rate-limit
146+IP is the last XFF hop (`pickClientIp`, `server.knot:302`), and `matchIp` treats
147+an empty bound IP as a wildcard (`server.knot:546`). This is correct **behind a
148+trusted proxy** but spoofable when the server is directly exposed. The flag and
149+its `--trustForwardedFor=False` override exist; the spec presents IP-binding as a
150+protection without the proxy caveat. Recommend documenting it.
151+
152+---
153+
154+## Minor / noted
155+
156+- HKDF salt is `ephemeral_pub || recipient_x25519_pub` (both public), binds no
157+ sender — fine for confidentiality, no KCI/UKS guarantee beyond the signature.
158+- Presence TTL (90 min, `onlineGossipTtl`) outlives the 1 h session, so location
159+ metadata and stale forwards can persist ~90 min after disconnect.
160+- `/auth/challenge` is unauthenticated; fillable by anyone but capped
161+ (`maxChallenges = 10000`, `maxChallengesPerKey = 5`).
162+
163+---
164+
165+## Suggested follow-ups
166+
167+1. **Spec edits (safe, no behavior change):** fold #2, #3, #6, #8 caveats into
168+ `PROTOCOL.md` (§6 auth, §7 federation, §5 size, §10 security).
169+2. **Code fixes worth doing:** #1 (bind challenge to server) is **done**;
170+ #3-profile (`ts` staleness guard) remains a genuine, fixable defect.
171+3. #2 and #4 are inherent to open, unauthenticated federation — decide whether to
172+ document-as-accepted or add peer authentication.