chat: attachment cache, download, and transfer hardening (Phase 3)
CI / check (push) Successful in 2m33s
CI / check (push) Successful in 2m33s
Phase 3 of docs/chat-hardening-plan.md — attachments can no longer turn into unbounded memory, bandwidth, decoder, or task pressure (S15 closed; S14's filename half closed). Cache and image cost (3A): AttachmentCache now carries encoded- and decoded-byte budgets (96 MiB / 64 MiB) on top of the count cap, with per-entry weights, replacement accounting, and oldest-first eviction; an individually over-budget fetch services any pending Save/Play from the bytes in hand and is exposed as Evicted instead of retained. validate_image_bytes prechecks header dimensions (per-side AND a new 14 MP total-pixel limit) before any decode; the renderer only ever receives a ≤1600 px downscaled RGBA preview whose w*h*4 cost counts against the decoded budget — originals stay encoded-only for Save. sanitize_filename strips the bidi/zero-width spoofing set (RTL-override extension spoof). Download policy and state (3B): images auto-fetch only when roster- authored AND declared ≤4 MiB, gated by a new deterministic AutoFetchBudget (per-author and session request+byte token buckets, check-then-take, bounded author map) alongside the existing dedup and four-permit bound. Attachment state is now explicit — absence/Loading/ Ready/Failed/Evicted — driven by a new AttachmentFetchStarted event, so skipped or evicted images render a "Load image" button instead of an indefinite "loading…", and repeated clicks can never spawn duplicate fetch tasks. Exact transfers and serve store (3C): fetch_blob requires the received length to equal the declared size (short = local error, overlong = bounded-read reject, empty keeps meaning "sender no longer has it"); the file picker's unbounded read is replaced by a metadata-prechecked cap+1 bounded reader; one Arc<Vec<u8>> now backs the UI cache, command queue, and serve store; served_files is a count- and byte-budgeted FIFO ServeStore (16 entries / 128 MiB). 37 new tests (568 lib total) including a real two-endpoint loopback exercising exact/short/overlong/unknown-id transfers. Plan checkboxes ticked and constant deviations decision-logged. Tests-green-only: the plan's two-machine field-test section remains open. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
+86
-33
@@ -1,11 +1,15 @@
|
||||
# Chat hardening — ephemeral implementation plan
|
||||
|
||||
**Status (2026-07-17):** Phase 1 COMPLETE (shared text policy in `src/sanitize.rs`,
|
||||
ceilings enforced at UI input, sign point, and gossip ingress; all gates green).
|
||||
Phases 2–5 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.
|
||||
**Status (2026-07-18):** Phases 1–3 COMPLETE. Phase 1 = shared text policy in
|
||||
`src/sanitize.rs`, ceilings enforced at UI input, sign point, and gossip ingress.
|
||||
Phase 2 = roster-bound authorship (`src/core/chatroster.rs`), replay dedup + rate
|
||||
limits (`ChatIngressGate` in `src/network/gossip.rs`). Phase 3 = attachment
|
||||
cache/serve-store budgets, downscaled previews, auto-fetch byte/request budgets
|
||||
(`src/core/fetchbudget.rs`), exact transfers, bounded local reads. All gates
|
||||
green each phase. Phases 4–5 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
|
||||
|
||||
@@ -182,70 +186,70 @@ unbounded memory, bandwidth, decoder, or task pressure.
|
||||
|
||||
### 3A. Cache and image cost
|
||||
|
||||
- [ ] Extend `AttachmentCache` with encoded-byte and decoded-preview-byte counters.
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] 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.
|
||||
- [x] Ensure a repeated click cannot create duplicate unguarded fetch tasks.
|
||||
- [x] 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`.
|
||||
- [x] 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
|
||||
- [x] 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.
|
||||
- [x] Avoid duplicating a full attachment across UI, command queue, and serve store.
|
||||
Prefer `Arc<Vec<u8>>`/`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-
|
||||
- [x] 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
|
||||
- [x] 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,
|
||||
- [x] 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
|
||||
- [x] 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
|
||||
- [x] Image with valid per-side dimensions but excessive total pixels is rejected.
|
||||
- [x] 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
|
||||
- [x] 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,
|
||||
- [x] Short, exact, and overlong file responses.
|
||||
- [x] Local file reader stops at cap + 1 instead of allocating the full source.
|
||||
- [x] Served-file FIFO/byte eviction and replacement accounting.
|
||||
- [x] Same attachment id from two authors remains isolated throughout fetch, cache,
|
||||
save, and display.
|
||||
|
||||
## Phase 4 — URL and rendering resilience
|
||||
@@ -431,3 +435,52 @@ The plan is complete when:
|
||||
security boundary (every peer independently enforces the buckets), and a
|
||||
local silent drop would be a UX regression better designed alongside Phase
|
||||
5's honest send status.
|
||||
- **2026-07-18 (Phase 3):** Constants that deviate from the proposed table, all
|
||||
bound-tested: total decoded pixels **14 MP** (not 16 MP) so the bound clears
|
||||
12 MP phone photos (4032×3024) yet actually binds inside the 4096²≈16.8 MP
|
||||
per-side envelope; cache encoded budget **96 MiB** (not 128) — still several
|
||||
full-size files, tighter worst case; serve store **128 MiB + 16 entries**
|
||||
(not 256 MiB) — a sender's own session should not pin a quarter GiB.
|
||||
- **2026-07-18 (Phase 3):** `validate_image_bytes`/`decode_preview` precheck
|
||||
dimensions from the container HEADER (`into_dimensions`) before any pixel
|
||||
decode, so an over-limit decode bomb is rejected without paying its decode
|
||||
cost; the decode-time `image::Limits` remain as defense in depth, and the
|
||||
decoded dimensions must equal the prechecked header dimensions.
|
||||
- **2026-07-18 (Phase 3):** Budget-pressure evictions leave NO cache entry
|
||||
(absence = NotFetched → the same Load/Download affordance), while the
|
||||
explicit `Evicted` state marks only an *individually over-budget* fetch whose
|
||||
bytes were used once (pending Save/Play serviced from hand) and dropped. Both
|
||||
render load-on-demand; only the bookkeeping differs.
|
||||
- **2026-07-18 (Phase 3):** The core still runs `validate_image_bytes` before
|
||||
emitting `AttachmentReady`, and the UI decodes once more to build the ≤1600px
|
||||
preview. Two bounded decodes per image were accepted over shipping decoded
|
||||
RGBA across the channel (which would defeat the encoded-only Arc sharing).
|
||||
- **2026-07-18 (Phase 3):** The image lightbox now enlarges the ≤1600px preview
|
||||
handle, not the original bitmap — originals are retained encoded-only for
|
||||
Save. At the lightbox's window-sized draw area the visual difference is nil
|
||||
for the chat use case; full fidelity remains one Save away.
|
||||
- **2026-07-18 (Phase 3):** `AutoFetchBudget` checks all four buckets
|
||||
(author/session × requests/bytes) and only then consumes atomically, so a
|
||||
rejection burns nothing (no refund path like Phase 2's room bucket needed).
|
||||
Tokens ARE consumed if the four-permit semaphore then rejects the spawn —
|
||||
that only happens mid-flood, when charging the author is the intent.
|
||||
- **2026-07-18 (Phase 3):** The auto-fetch budget's author map prunes
|
||||
least-recently-active past 64 entries instead of wiring roster eviction into
|
||||
the event task: authors are roster-gated upstream (≤32 live members), so
|
||||
strangers cannot churn the map, and a pruned author returning with full
|
||||
buckets is within policy.
|
||||
- **2026-07-18 (Phase 3):** Music-track serving shares the bounded serve store
|
||||
with chat attachments. A user who sends enough large attachments during a
|
||||
broadcast can evict their own current track; listeners then get the standard
|
||||
"sender no longer has the file" failure. Accepted: budget honesty over a
|
||||
second store, and the store comfortably fits current+next track plus a
|
||||
normal chat working set.
|
||||
- **2026-07-18 (Phase 3):** The clip player's command channel still takes one
|
||||
owned byte copy at the moment of a Play click (small, human-initiated). The
|
||||
Arc de-duplication targeted the send path (UI cache / command queue / serve
|
||||
store), which now shares a single allocation.
|
||||
- **2026-07-18 (Phase 3):** Overlong transfers are rejected by the transport
|
||||
read itself (`read_to_end(size)` errors past the bound) rather than an
|
||||
explicit length compare; short transfers get the explicit
|
||||
`len == declared_size` check. Music fetches ride `fetch_blob`, so they
|
||||
inherit exactness for free.
|
||||
|
||||
Reference in New Issue
Block a user