From e724167b03c74a5e935a9ad5661050d8dafdba92 Mon Sep 17 00:00:00 2001 From: Mollusk Date: Fri, 17 Jul 2026 00:27:47 -0400 Subject: [PATCH] docs: add chat-hardening plan as scope contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GPT-5.6's 5-phase plan for the chat identity/replay/rate-limit cluster (2026-07-16 review findings 5-8): roster-bound display names, replay dedup, quiet rate limiting, bidi-aware sanitization, attachment size checks. Self-describes as temporary — delete when the work completes. Co-Authored-By: Claude Fable 5 --- docs/chat-hardening-plan.md | 385 ++++++++++++++++++++++++++++++++++++ 1 file changed, 385 insertions(+) create mode 100644 docs/chat-hardening-plan.md diff --git a/docs/chat-hardening-plan.md b/docs/chat-hardening-plan.md new file mode 100644 index 0000000..d37bcee --- /dev/null +++ b/docs/chat-hardening-plan.md @@ -0,0 +1,385 @@ +# Chat hardening — ephemeral implementation plan + +**Status (2026-07-15):** PLANNED, not started. This is a temporary scope +contract for hardening the existing room chat. Update the checkboxes and decision +log as work lands, then delete this file when the work is complete. Do not add +link previews as part of this effort. + +## Goal + +Strengthen the current encrypted, signed, session-only room chat without changing +its product model: plain selectable text, clickable web links, and peer-to-peer +attachments over the existing gossip and files planes. The work should make chat +resistant to identity spoofing, replay, spam, oversized input, expensive rendering, +and attachment-driven memory/bandwidth pressure while preserving normal Unicode +conversation and the existing full-mesh architecture. + +## Existing foundation to preserve + +- Gossip payloads are signed by the claimed `EndpointId`, bound to the raw room + topic and protocol domain, and checked before dispatch. +- The signed envelope timestamp is admitted only within the two-minute gossip + freshness window. +- Inbound gossip frames are capped at 128 KiB before JSON deserialization. This + larger plane-wide cap must remain because `Announce` may contain a custom avatar. +- Chat history is session-only and capped at 300 entries. +- Only `http://` and `https://` links are opened, as a single process argument + without a shell. +- Attachment descriptors are signed with the chat payload; attachment bytes use + the encrypted files plane, have a 25 MiB per-file cap, and are keyed by both + author and attachment id. +- Image bytes are decoded defensively and automatic image fetches already have a + four-task concurrency limit. + +## Working design decisions + +These are the implementation defaults unless code inspection or tests reveal a +concrete reason to adjust them. Record any adjustment in the decision log. + +1. **No wire change.** Keep `GossipMessage::Chat` unchanged and do not bump + `GOSSIP_PROTO`. The redundant wire `name` and inner `Chat.ts` remain serialized + for compatibility but are not trusted. Remove them only during a future planned + gossip-version bump. +2. **Roster identity is authoritative.** A chat line is admitted only for an + authenticated identity already known to the current room (including the + reconnect grace state). Its displayed name comes from the sanitized roster + state, never from `GossipMessage::Chat.name`. +3. **Body Unicode remains expressive.** Do not apply the short-label sanitizer to + the message body; it strips format characters used by some languages and emoji. + Continue neutralizing controls and whitespace, while treating author labels, + filenames, and URLs more strictly because those are spoof-sensitive surfaces. +4. **Bounds apply at every trust boundary.** UI input is bounded while editing, + outgoing text is normalized before signing, and incoming text is byte-checked + and normalized before it leaves the gossip layer. UI-only truncation is not an + adequate ingress defense. +5. **Automatic network work is stricter than manual work.** Keep the 25 MiB manual + attachment ceiling, but auto-fetch only small images. Larger images remain + available behind an explicit Load/Download action. +6. **Caches are bounded by cost, not only entry count.** Count encoded bytes and + estimated decoded image bytes. A count cap remains as a secondary bound. +7. **Rate limiting degrades quietly.** Drop excess/replayed peer messages with a + rate-limited log entry. Do not let a spammer produce a second UI-notification + flood. + +## Proposed policy constants + +Keep these together near the code that enforces them and cover them with boundary +tests. Values are starting points, not a compatibility contract. + +| Policy | Initial value | Reason | +| --- | ---: | --- | +| Chat body characters | 2,000 | Preserves current UI behavior | +| Chat body UTF-8 bytes | 8 KiB | Covers 2,000 four-byte scalars with small headroom | +| Live input characters/bytes | Same as body | Prevent oversized paste/edit state | +| Clickable links per message | 8 | Bounds spans and opener targets | +| Retained chat text | 512 KiB plus 300 entries | Bounds redraw and selection work | +| Per-author chat limiter | Burst 8, refill 1/second | Allows normal bursts, stops sustained spam | +| Room-wide chat limiter | Burst 32, refill 8/second | Protects shared event/UI queues | +| Exact-chat replay cache | 1,024 digests, 2-minute TTL | Covers freshness window with a hard bound | +| Auto-fetch image encoded size | 4 MiB | Limits unsolicited bandwidth and allocations | +| Attachment cache encoded budget | 128 MiB | Allows several ordinary files without GiB growth | +| Attachment cache decoded-preview budget | 64 MiB | Bounds renderer-side image pressure | +| Served attachment budget | 256 MiB plus a count cap | Bounds sender memory for a long session | +| Inline preview longest side | 1,600 px | Chat renders near 260 px; full 4K decode is wasteful | +| Decoded source image pixels | 16 megapixels maximum | Adds a total-pixel bound to per-side bounds | + +## Phase 1 — Shared text policy and live-input bounds + +**Target:** downstream layers never receive or retain an unexpectedly large or +unsafe chat string. + +- [ ] Move chat constants and `sanitize_chat` from `src/app/mod.rs` into + `src/sanitize.rs` (or a narrowly scoped shared chat-policy module if that keeps + the API clearer). +- [ ] Implement a single-pass sanitizer that: + - maps control characters to spaces; + - collapses whitespace and trims ends; + - enforces both the character and UTF-8 byte ceilings without splitting a scalar; + - returns empty for content with no visible text. +- [ ] Add `cap_chat_input` for live editing. It must preserve the user's current + whitespace while enforcing character and byte ceilings; normalization remains a + submit/ingress operation so typing does not visibly jump. +- [ ] Apply `cap_chat_input` in `AppMessage::ChatInputChanged`, covering keyboard, + clipboard, primary-selection, and context-menu paste paths through the controlled + input widget. +- [ ] Sanitize outgoing text immediately before local echo and `CoreCommand` send. +- [ ] Sanitize again before `GossipMessage::Chat` is signed, so a future non-UI + caller cannot bypass policy. +- [ ] At gossip ingress, reject raw chat text over the byte ceiling before doing + downstream sanitization; sanitize accepted text before creating `RoomEvent`. +- [ ] Keep attachment-only messages when the sanitized caption is empty; drop a + chat with neither visible text nor a valid attachment. +- [ ] Stop sanitizing an incoming chat `name` with the body sanitizer. Phase 2 + replaces it with the roster-bound name. + +### Phase 1 tests + +- [ ] ASCII, multibyte Unicode, emoji, whitespace, NUL/CR/LF/TAB/ESC, empty input. +- [ ] Exact character and byte boundaries, including a four-byte scalar at the + cutoff. +- [ ] Oversized paste never makes `state.chat_input` exceed either ceiling. +- [ ] Outgoing, incoming, and direct core/network paths converge on the same + normalized result. +- [ ] Empty captions are retained only when a valid attachment remains. + +## Phase 2 — Admission, identity binding, replay, and spam control + +**Target:** only current authenticated room members can create chat UI work, and a +member cannot impersonate another participant or monopolize the control/UI queues. + +- [ ] Change the core event task's chat roster from a bare `HashSet` to + a bounded map containing each member's latest sanitized display name (or retain a + parallel name map if less invasive). +- [ ] Insert/update the map on `PeerJoined`/`PeerUpdated`, retain it during transient + reconnect grace, and remove it on graceful or terminal eviction. +- [ ] Before attachment handling or UI forwarding, reject `RoomEvent::ChatMessage` + whose author is not present in that authoritative roster. +- [ ] Replace the embedded wire name with the roster map's name before constructing + `UiEvent::ChatMessage`. The UI may keep storing a name snapshot so old chat lines + remain labeled after a peer leaves. +- [ ] Add a lightweight early known-author gate in the gossip loop using its live + and disconnected-peer sets. Keep the core roster gate as defense in depth and as + the final authority. +- [ ] Validate that the inner `Chat.ts` equals the signed envelope timestamp, or + ignore it entirely. Do not use the inner timestamp for replay or ordering. +- [ ] Add exact-chat replay suppression after signature verification and before + event-channel send: + - hash the canonical signed bytes, not raw JSON formatting; + - use BLAKE3 (make it a direct dependency if needed; it is already in the iroh + dependency graph) or an equally collision-resistant existing primitive; + - store a `HashSet` plus FIFO/TTL order for bounded lookup and eviction; + - prune by both the gossip freshness window and the hard entry cap. +- [ ] Add a bounded token bucket per admitted author and a room-wide bucket before + awaiting `event_tx.send`. Limiter state must be removed with roster eviction and + remain bounded by the roster cap. +- [ ] Ensure duplicate messages are dropped before consuming rate-limit tokens, so + a replay cannot starve a legitimate new message from that author. +- [ ] Rate-limit rejection logging per author/reason. +- [ ] Consider applying the same local submit policy to accidental rapid Enter or + button activation, without routing chat through the coalescing command path. + +### Phase 2 tests + +- [ ] Valid roster author is admitted; never-announced, post-leave, forged, and + stale authors are rejected. +- [ ] A peer sending `name = "Victim"` renders under its own roster name. +- [ ] A name update affects future messages without rewriting history. +- [ ] Reconnect grace continues accepting the known author; terminal eviction does + not. +- [ ] The same signed chat is displayed once; distinct chats created in the same + millisecond are both admitted. +- [ ] Replay-cache TTL/cap pruning cannot grow without bound. +- [ ] Per-author burst/refill and room-wide burst/refill boundaries. +- [ ] Excess chat cannot prevent a subsequent `Leave` or `Announce` from reaching + the event loop in a deterministic channel-pressure test. + +## Phase 3 — Attachment transfer and memory hardening + +**Target:** neither peers nor long local sessions can turn chat attachments into +unbounded memory, bandwidth, decoder, or task pressure. + +### 3A. Cache and image cost + +- [ ] Extend `AttachmentCache` with encoded-byte and decoded-preview-byte counters. + Preserve the count cap, but evict oldest entries until all three budgets fit. +- [ ] Give every entry an explicit weight. Replacement must subtract the old + weight before checking/inserting the new one. +- [ ] Decide behavior for a single entry larger than the cache budget: service an + immediate pending Save/Play request without retaining it, then expose it as + evicted/unavailable rather than exceeding the budget. +- [ ] Add a total-pixel limit to `validate_image_bytes` in addition to the existing + width/height limit. +- [ ] Build a downscaled inline preview handle with a maximum 1,600 px side. Keep + original bytes only for Save; do not hand a full-resolution 4K image to the + renderer merely to display it at chat width. +- [ ] Count estimated RGBA preview cost (`width * height * 4`) against the decoded + budget even if iced internally copies or uploads it. +- [ ] Strip the same bidi/zero-width spoofing characters used for display labels + from attachment filenames, while preserving ordinary Unicode filenames. + +### 3B. Automatic download policy and state + +- [ ] Auto-fetch only roster-authored images whose declared size is at or below + `MAX_AUTO_IMAGE_BYTES`; keep the existing `(author,id)` dedup and four-permit + concurrency bound. +- [ ] Add per-author and session byte/request budgets for automatic fetches so a + peer cannot drain bandwidth sequentially after each permit is released. +- [ ] Represent `NotFetched`, `Loading`, `Ready`, `Failed`, and `Evicted` distinctly + enough for the UI to avoid an indefinite “loading…” label when auto-fetch was + skipped or the cache evicted an item. +- [ ] Render a Load image button for large/skipped images. A manual click may use + the 25 MiB file cap but still observes cache/decoder budgets. +- [ ] Ensure a repeated click cannot create duplicate unguarded fetch tasks. +- [ ] Keep non-image attachments manual-only. + +### 3C. Exact transfers, local reads, and served files + +- [ ] In `IrohTransport::fetch_blob`, require `bytes.len() as u64 == declared_size`. + Reject empty, short, and overlong transfers with a concise local error. +- [ ] Replace the file picker's unbounded `FileHandle::read()` with a helper that + reads at most `MAX_ATTACHMENT_BYTES + 1`. Check metadata first where available, + but retain the bounded read because metadata can race or be unavailable through + a portal. +- [ ] Avoid duplicating a full attachment across UI, command queue, and serve store. + Prefer `Arc>`/`Arc<[u8]>` through `AttachmentState`, `CoreCommand`, and + `serve_attachment`, subject to iced handle API constraints. +- [ ] Replace the unbounded session `served_files` map with a count- and byte- + budgeted FIFO store. Evicted ids should produce the existing “sender no longer + has the file” response rather than stale or aliased data. +- [ ] Keep attachment ids keyed by author on receipt and preserve all existing + request-length, timeout, filename, and decoder checks. + +### Phase 3 tests + +- [ ] Byte-budget eviction, count eviction, replacement accounting, clear/reset, + and an individually overweight entry. +- [ ] Decoded-preview budget and downscale dimensions for wide, tall, square, and + boundary images. +- [ ] Image with valid per-side dimensions but excessive total pixels is rejected. +- [ ] A declared 4 MiB image auto-fetches; the first byte over the limit requires a + click. +- [ ] Per-author/session auto-fetch budgets recover according to their policy and + never exceed task concurrency. +- [ ] Short, exact, and overlong file responses. +- [ ] Local file reader stops at cap + 1 instead of allocating the full source. +- [ ] Served-file FIFO/byte eviction and replacement accounting. +- [ ] Same attachment id from two authors remains isolated throughout fetch, cache, + save, and display. + +## Phase 4 — URL and rendering resilience + +**Target:** keep clickable links without making malformed/deceptive input or many +small spans an unnecessary UI/launcher surface. + +- [ ] Make `url` a direct dependency (already present transitively) and validate + link candidates with `url::Url`. +- [ ] A clickable URL must have an `http` or `https` scheme and a valid host. +- [ ] Treat URLs containing username/password syntax as plain text, or require an + explicit confirmation that shows the parsed destination host. Prefer plain text + for the first implementation. +- [ ] Preserve the existing defense-in-depth validation in `AppMessage::OpenUrl`; + replace prefix checks with the shared parsed-URL policy. +- [ ] Cap clickable candidates at eight per message. Remaining content stays + selectable plain text and must still round-trip exactly. +- [ ] Refactor linkification to return borrowed ranges/offsets or cache link ranges + in `ChatEntry`, avoiding allocation and rescanning on every redraw. +- [ ] Bound retained history by total sanitized text bytes as well as 300 entries. + Eviction must keep attachment bookkeeping coherent and should not invalidate an + open Save/Play operation. +- [ ] Do not add metadata fetching, remote images, Markdown, or link previews. + +### Phase 4 tests + +- [ ] Valid HTTP/HTTPS, malformed host, empty host, mixed case, Unicode path/query, + punctuation, credentials/userinfo, and non-web schemes. +- [ ] Eight-link boundary and many-link adversarial input. +- [ ] Segment/range reconstruction exactly reproduces the sanitized message. +- [ ] Entry-count and total-text-budget history eviction. +- [ ] Opener policy cannot launch a non-web scheme even if called directly. + +## Phase 5 — Honest local send status + +**Target:** never present a locally echoed message as successfully broadcast when +the core rejected it or gossip broadcast failed. + +- [ ] Add a local-only message id and `Pending`/`Broadcast`/`Failed` state to local + chat entries. Do not put this id or state on the wire. +- [ ] Carry the local id through `CoreCommand::SendChat`/`SendChatFile` and return a + `UiEvent` result after the local gossip broadcast call succeeds or fails. +- [ ] If the core is not in an active session, return failure instead of silently + doing nothing. +- [ ] Show failure compactly with a retry action. A successful local broadcast must + not be labeled “delivered” or “read”; PeerSpeak has no peer acknowledgements. +- [ ] Retry creates one new signed broadcast while retaining replay correctness and + attachment serving state. + +### Phase 5 tests + +- [ ] Local echo starts pending, becomes broadcast on success, and becomes failed + on no-session/channel/gossip error. +- [ ] Results update only the matching local entry, including after history + eviction or room reset. +- [ ] Retry does not duplicate served bytes or mutate an unrelated entry. + +## Compatibility and versioning + +- The planned implementation changes validation, local data structures, and + internal `CoreCommand`/`UiEvent` shapes only. Keep the serialized + `GossipMessage::Chat` and file request/response formats unchanged. +- Therefore do **not** bump `GOSSIP_PROTO`, `FILES_PROTO`, or the pre-1.0 MINOR + solely for this plan. The eventual release is a compatible PATCH unless scope + expands into a wire change. +- If implementation requires removing/adding serialized fields, changing + attachment request framing, or introducing acknowledgements on the wire, stop + and revise this section before coding that part. Follow `VERSIONING.md` and use + the appropriate protocol plus release MINOR bump. + +## Verification gates + +Run after each phase, with focused tests first and the full gates before handoff: + +```text +cargo fmt --check +cargo test --lib +cargo test --all-targets +cargo clippy --all-targets -- -D warnings +``` + +Also retain the existing ignored/loopback coverage where the environment supports +it; do not make ordinary unit tests depend on external network access. + +### Two-machine field test + +- [ ] Ordinary ASCII/Unicode conversation, rapid short burst, long boundary text, + and oversized paste. +- [ ] Rename during a room: new lines use the new roster name; old lines retain + their snapshot. +- [ ] Disconnect/reconnect grace and post-leave chat admission behavior. +- [ ] Multiple normal images, one image above the auto threshold, a malformed + “image”, and a maximum-size manual file. +- [ ] Download/save after cache eviction; clear failure state and no runaway + memory across repeated attachments. +- [ ] Observe process RSS and UI responsiveness during a bounded spam/attachment + stress run; verify leave/reconnect controls remain responsive. +- [ ] Linux and Windows URL opening for valid links; malformed/userinfo links remain + selectable but do not launch. + +## Completion criteria + +The plan is complete when: + +1. Only active/grace-rostered authenticated authors reach chat UI state. +2. Chat identity is roster-bound and cannot be overridden by the embedded wire + name. +3. Exact replay and sustained spam are bounded before shared event queues. +4. Live input, inbound/outbound body size, history text, attachment caches, + automatic transfers, served files, and decoded previews all have tested hard + bounds. +5. File transfer length and image decoding/display costs are validated. +6. Clickable links pass a shared parsed-URL policy and rendering work is bounded. +7. Local broadcast failure is visible without claiming peer delivery. +8. Unit/all-target/clippy gates and the two-machine field test pass. +9. Relevant durable docs (`README.md`, `docs/FEATURES.md`, `CHANGELOG.md`, security + notes, and comments) describe the final behavior. +10. This ephemeral plan is deleted after its useful status/history is transferred + to durable documentation. + +## Out of scope + +- Link previews, metadata fetches, or remote thumbnail requests. +- Persistent/offline chat history or server-side message storage. +- Markdown, rich embeds, reactions, editing, deletion, threads, or search. +- Read receipts or peer delivery acknowledgements. +- Moderation UI, kicking, blocking, or trust-list redesign. +- Antivirus/malware scanning of user-requested downloaded files. +- A new application-layer group-encryption protocol or a broader cryptographic + redesign. If PeerSpeak makes a formal end-to-end-encryption product claim, audit + and document the exact iroh/gossip/relay threat model as a separate project. + +## Decision log + +- **2026-07-15:** Chose hardening over automatic link previews because receiving a + message should not trigger third-party web requests or weaken PeerSpeak's + privacy-oriented design. +- **2026-07-15:** Initial scope keeps all wire formats stable; hardening is local + admission, validation, resource accounting, and honest UI state.