add plan
This commit is contained in:
@@ -0,0 +1,825 @@
|
||||
# Coop Stability Audit — Pre-Release
|
||||
|
||||
Date: 2026-11-12 · Revision audited: `8fa79aef` (master) · Scope: entire workspace (~35k lines, 18 crates + `desktop` + `web`)
|
||||
|
||||
## 1. Scope and Method
|
||||
|
||||
Every crate was audited file-by-file against the project conventions (`AGENTS.md`), Rust best
|
||||
practices (ownership, error handling, async, API design, anti-patterns), and GPUI lifecycle rules
|
||||
(entity updates, task/subscription ownership, `ElementId` stability, render purity). Severe claims
|
||||
were verified against the vendored dependency sources (`gpui-pre 0.3.12`, `gpui-base 0.9.1`,
|
||||
`gpui-component 0.9.1`, rust-nostr 0.45.6, `gpui-updater-pre 0.2.0`), and the highest-severity
|
||||
findings were independently re-verified by hand.
|
||||
|
||||
Tooling results: `cargo clippy --workspace --all-targets` — **clean, zero warnings**.
|
||||
`cargo fmt --all --check` — **clean**. `cargo check -p workspace` and `-p person` — pass.
|
||||
|
||||
## 2. Executive Summary
|
||||
|
||||
**Verdict: not ready for stable release yet.** The codebase is well-structured — clean clippy,
|
||||
disciplined GPUI entity/subscription hygiene in most crates, good hostile-input handling in the
|
||||
crypto-adjacent code, and no lock-across-await anywhere. But there are **3 Critical** and
|
||||
**~27 High** findings that must be addressed first, concentrated in three areas:
|
||||
|
||||
1. **Money and key safety** (`nip60`, `state`, `browser-signer-proxy`): a one-event wallet-wipe
|
||||
vector, a startup crash loop, a plaintext client key in web `localStorage`, and an
|
||||
unauthenticated localhost signing API.
|
||||
2. **Silent failure paths**: errors from wallet ops, message loads, relay connects, settings
|
||||
persistence, and backup/keyring reads are swallowed; several failure states strand data or
|
||||
wedge the UI (stuck `loading_more`, permanently-blacklisted profile fetches, dictation dying
|
||||
after 30 s).
|
||||
3. **Wrong results visible to users**: reaction double-counting, multi-line messages flattened to
|
||||
one line, contact-list updates that erase relay hints and petnames, stale group state from
|
||||
out-of-order relay delivery, duplicate tabs.
|
||||
|
||||
### Must fix before stable (P0)
|
||||
|
||||
| # | Severity | Location | Issue |
|
||||
|---|----------|----------|-------|
|
||||
| 1 | Critical | `nip60/src/lib.rs:420,552` | Spoofed kind:5 deletion from **any** author wipes wallet proofs (operator precedence bug + missing author check in `absorb_deletion`) |
|
||||
| 2 | Critical | `state/src/lib.rs:117-122` | LMDB open `block_on` + `.expect` → unrecoverable startup crash loop on any DB error |
|
||||
| 3 | Critical | `state/src/lib.rs:447-449` | Plaintext NIP-46 client secret key persisted to `localStorage` on web |
|
||||
| 4 | High | `browser-signer-proxy/src/lib.rs:443-571` | Unauthenticated localhost signing API with `Access-Control-Allow-Origin: *`; plaintext theft, request suppression, response forgery |
|
||||
| 5 | High | `nip60/src/ops.rs:455-488` | Reconcile marks relay proofs `Unspent` unconditionally → resurrects locally-spent proofs (double-spend risk) |
|
||||
| 6 | High | `nip60/src/ops.rs:381-417` | Publish failure after store commit strands sent/received proofs; no retry path |
|
||||
| 7 | High | `state/src/file.rs:418-443` | Decrypted-file cache keyed by filename only → user can be served another attachment's contents |
|
||||
| 8 | High | `chat_ui/src/lib.rs:605-611` | Own reactions counted twice (backup gift-wrap re-delivery, no dedup) |
|
||||
| 9 | High | `common/src/media_extractor.rs:52-58` | Whitespace collapse destroys line breaks in every message |
|
||||
| 10 | High | `workspace/src/panels/contact_list.rs:187-191` | Contact-list Update silently strips NIP-02 relay hints and petnames from the published event |
|
||||
| 11 | High | `nip29/src/lib.rs:541-555` + `group.rs:696-728` | No `created_at` guards: stale relay data resurrects left groups; group state applied last-write-wins |
|
||||
| 12 | High | `settings/src/lib.rs:403-404,423-438` | Failed settings load silently resets and overwrites the user's file; saves discard errors and race |
|
||||
| 13 | High | `speech/src/parakeet.rs:134` | Dictation worker exits after 30 s idle → longer dictations silently lost |
|
||||
| 14 | High | `browser-signer-proxy/src/lib.rs:339-366` | Timed-out signing requests leak plaintext in memory forever |
|
||||
| 15 | High | `dock/mod.rs:50-73` + `sidebar/rows.rs:54-57` | Tab dedup is dead code (duplicate tabs); "Ignore" screening action dispatches to a handler that can never receive it |
|
||||
| 16 | High | `nip60_ui/src/wallet.rs:615-726` + `nip60/src/lib.rs:147-185` | Lightning invoice flow broken end-to-end (quotes never rendered or claimable) |
|
||||
| 17 | High | `state/src/lib.rs:553-554` | Proxy credential write silently discarded (`save.await.ok()`) |
|
||||
| 18 | High | `person/src/lib.rs:310-333` | One 2 s metadata timeout permanently blacklists those profiles until restart |
|
||||
| 19 | High | `chat/src/lib.rs:398-403` | Failed-to-decrypt messages are trashed with no UI surface anywhere |
|
||||
| 20 | High | `chat_ui/src/lib.rs:325-334`, `workspace/src/dialogs/profile.rs:227-235`, `workspace/src/panels/backup.rs:59-76` | Silent failure cluster: room loads, profile URL saves, keyring reads |
|
||||
|
||||
The full finding list follows, crate by crate.
|
||||
|
||||
## 3. Systemic Patterns (cross-cutting)
|
||||
|
||||
**S1 — Task handles accumulate unboundedly (~20 containers, ~48 declaration sites).**
|
||||
`tasks: Vec<Task<_>>` / `SmallVec<[Task<_>; N]>` fields are pushed on every operation and never
|
||||
drained; finished `Task` handles held in a container are not reaped by GPUI. Worst offenders:
|
||||
`nip60/src/lib.rs:79` (a handle per relay event, whole session), `nip4e/src/lib.rs:90` (runs on
|
||||
every settings change), `chat_ui/src/lib.rs:110`, `nip29/src/lib.rs:90`, `nip29_ui/src/lib.rs:337`,
|
||||
`person/src/directory.rs:110`, `state/src/lib.rs:95`, plus every workspace dialog. The codebase
|
||||
already contains the correct patterns: single `Option<Task<_>>` replacement
|
||||
(`screening.rs`, `greeter.rs`, `debounced_delay.rs`) and reset-on-teardown (`nip29::reset`).
|
||||
Fix convention-wide: replace-per-spawn for one-shots, `detach_and_log_err` for true fire-and-forget.
|
||||
|
||||
**S2 — Silent error discards (~156 `let _ =` / `.ok()` sites outside tests).**
|
||||
Project rule forbids `let _ =` on fallible ops; `AGENTS.md` requires async errors to reach the UI.
|
||||
Beyond the P0s above, the recurring case is `_ = this.update(cx, …)` — mostly benign
|
||||
(entity dropped at shutdown) but should be `.log_err()` for visibility. Files with the most sites:
|
||||
`dock/tab_panel.rs` (6), `nip60/src/lib.rs` (10), `nip4e/src/lib.rs` (8).
|
||||
|
||||
**S3 — `chat_ui` ↔ `nip29_ui` message-panel duplication.**
|
||||
`ListState` sync + splice bookkeeping (including divergent `ix + 1` vs `ix + 2` offsets),
|
||||
`rendered_texts_by_id` cache (both share the stale-mention bug), speech reveal + `RevealText`
|
||||
fallback timer + `end_events` loop, `Speaker` wiring, reaction grouping, `MessageRow` assembly are
|
||||
near-verbatim copies — and they have already diverged: `nip29_ui` splices at the prepend position
|
||||
while `chat_ui` always appends (bug 5.7.4). Extract a shared message-panel controller in `ui`.
|
||||
|
||||
**S4 — Dialog error-banner + auto-clear timer duplicated four times** with drifted timings
|
||||
(`restore.rs` 3 s, `import.rs` 3 s, `relays.rs` 2 s, `contact_list.rs` 2 s). One shared
|
||||
`error: Entity<Option<SharedString>>` helper removes the drift.
|
||||
|
||||
**S5 — `DefaultHasher` used for network-persisted IDs.**
|
||||
`common::EventExt::uniq_id` (`event.rs:12-18`) is hashed into published `r` tags by
|
||||
`chat/src/lib.rs:1031-1041`; `DefaultHasher`'s algorithm is not stable across Rust versions, so a
|
||||
toolchain bump silently changes every published room id. Also hand-rolled in three places
|
||||
(`nip29/protocol.rs:282-286`, `nip29/utils.rs:27-32`). Replace with a stable hash (SHA-256 of
|
||||
sorted hex pubkeys) in one shared helper.
|
||||
|
||||
**S6 — Index-based element ids.** Rows keyed by list index (`sidebar/rows.rs:174+`,
|
||||
`contact_list.rs:238`, `greeter.rs:226+`, `nip29_ui` reply previews `message.rs:359`) reassign to
|
||||
different data after mutations; benign today because handlers capture data, but any future
|
||||
stateful element (hover/focus) in a row will mis-attach. Prefer domain ids (pubkey hex, event id,
|
||||
room id) as `group_view.rs` already does.
|
||||
|
||||
## 4. Findings by Crate
|
||||
|
||||
Severity: **[C]** Critical · **[H]** High · **[M]** Medium · **[L]** Low
|
||||
|
||||
### 4.1 `state` — central Nostr registry, signers, persistence, files
|
||||
|
||||
- **[C] `lib.rs:117-122` — Startup LMDB open: `block_on` + `.expect` = crash loop.** The
|
||||
`block_on` itself is acceptable (runs once before the window opens; `NostrLmdb::open` schedules
|
||||
nothing back onto the GPUI executor — add a comment saying so). The real bug: a corrupted or
|
||||
unwritable data dir makes **every** launch panic with no dialog and no recovery except manually
|
||||
deleting data. The web path already does it right (`web_database.rs:30-33` falls back to memory
|
||||
with a logged error). Fix: mirror that on desktop — memory DB fallback + `StateEvent::Error` so
|
||||
the UI can tell the user local history is unavailable.
|
||||
- **[C] `lib.rs:447-449` (+ `credentials.rs:29-38`, `common/web_store.rs:21-25`) — Plaintext
|
||||
bunker client key in web `localStorage`.** `import_bunker` embeds a fresh `Keys::generate()` into
|
||||
the bunker URI and stores the credential unencrypted on wasm. XSS/malicious extension/shared
|
||||
machine = signature capability through the bunker (bunkers commonly auto-approve registered
|
||||
clients). Desktop stores it in the OS keyring, which is fine. Fix: on wasm, passphrase-encrypt
|
||||
(NIP-49) like `import_nsec` does, or keep the session memory-only.
|
||||
- **[H] `lib.rs:553-554` — Proxy credential write silently discarded.**
|
||||
`cx.background_spawn(async move { save.await.ok() }).detach();` — keyring failure = user silently
|
||||
dropped to login next launch. Log + emit `StateEvent::error`.
|
||||
- **[H] `file.rs:418-443` — Decrypted-file cache collides by filename.** Cache key is
|
||||
`sha256(name)` and the `metadata().is_err()` guard skips the write, so two attachments named
|
||||
`invoice.pdf` from different chats resolve to the **first** file's path. Fix: include the
|
||||
attachment sha256/URL in the key, or always overwrite via the atomic-rename path.
|
||||
- **[M] `lib.rs:373-386` — Web `ncryptsec` stored with `KeySecurity::Medium` (2¹⁶ PBKDF2).**
|
||||
Encrypted, but offline guessing at 2¹⁶ defeats human passphrases. Raise to `Strong` and/or move
|
||||
to IndexedDB + session-only keys. Acceptable if documented as a tradeoff.
|
||||
- **[M] `lib.rs:564-589` — Stale proxy monitor can log out a newer session.** The monitor's guard
|
||||
is only `current_user.is_some()`; if the user disconnects the proxy and signs in with nsec/bunker
|
||||
before the next 5 s tick, the monitor swaps the **active** signer for `Keys::generate()`. Fix:
|
||||
capture the proxy pubkey at spawn and only act if it's still the active signer.
|
||||
- **[M] `lib.rs:95` — `tasks` vec grows forever** (see S1; `connect_proxy` alone pushes four).
|
||||
- **[L] `lib.rs:714` `to_bech32().expect("infallible")`; `signer.rs` ×7
|
||||
`.expect("RwLock poisoned")`.** Practically unreachable, but a poisoned signer lock is the worst
|
||||
failure mode for a signer — prefer `unwrap_or_else(|e| e.into_inner())`.
|
||||
- **[L] `lib.rs:93` — `passphrase_required` written, never read** (UI learns via
|
||||
`StateEvent::PassphraseRequired`). Delete or expose.
|
||||
- **[L] `file.rs:381-384` — `DownloadedFile::save` (wasm) has no callers.** Remove or wire up.
|
||||
- **[L] `lib.rs:647-656` — Dead search-relay guard.** Vendored nostr-sdk: `Client::relay()` returns
|
||||
`Result<Option<_>>`, so `is_ok()` is always true and the "Failed to add search relay" branch is
|
||||
unreachable. Test `is_ok_and(|r| r.is_some())` or drop the check.
|
||||
- **[L] `file.rs:221-258` — No size limit on pick/upload** (`MAX_FILE_SIZE` enforced only on
|
||||
download); a multi-GB pick is read fully into memory. Check size before reading.
|
||||
- **[L] `file.rs:427-441` — Plaintext decrypted files in `temp_dir()/coop-files`, default
|
||||
permissions, never purged.** Consider 0600 + best-effort purge on exit. Also `file.rs:150`
|
||||
builds `file://{path}` by string concat (breaks on `#`/`?`/`%`).
|
||||
- **[L] `file.rs:291` — Blossom rejection classified by `message.contains("415")`** — match the
|
||||
HTTP status instead. `file.rs:172` — 16-byte GCM nonce; 12 bytes is the norm.
|
||||
- **[L] `lib.rs:280-289` — `restore_session` cfg branches compile to empty bodies on wrong
|
||||
target**, silently emitting no event. `debug_assert!` or fall through to `NoSigner`.
|
||||
- **[L] `file.rs:446-451` + `web_file.rs:164-174` — Web blob URLs never revoked** when replaced;
|
||||
every decrypted file pins its bytes for the page lifetime.
|
||||
- **[L] `lib.rs:683-687` — Search results can contain duplicate pubkeys.** Dedupe.
|
||||
|
||||
Positives: device key stored only NIP-44-encrypted to self in the local DB; bunker credentials
|
||||
verified before persisting; `UniversalSigner` clones the inner `Arc` out of the lock before
|
||||
awaiting (no locks across `.await`); downloads size-capped and hash-verified; `Keys`' vendored
|
||||
`Debug` hides secrets.
|
||||
|
||||
### 4.2 `nip60` — NIP-60 Cashu wallet engine
|
||||
|
||||
- **[C] `lib.rs:420` + `lib.rs:552-572` — Spoofed kind:5 deletion can wipe the wallet.**
|
||||
`let wallet_event = ours && is_cashu || is_token_deletion(&event);` parses as
|
||||
`(ours && is_cashu) || is_token_deletion(&event)` — deletion events from **any** author are
|
||||
forwarded, and `absorb_deletion` never checks `event.pubkey == me`. Kind 7375 event ids are
|
||||
public on relays; one attacker event removes our tokens, marks them stale, and the Reconcile
|
||||
deletes the real proofs from the store (restore-only recovery). Fix:
|
||||
`wallet_event = ours && (is_cashu || is_token_deletion(&event))` + author check inside
|
||||
`absorb_deletion` for defense in depth.
|
||||
- **[H] `ops.rs:455-488` (with `proof_info` `ops.rs:601-617`) — Reconcile resurrects spent
|
||||
proofs.** Every relay-token proof is added to the store hardcoded `State::Unspent`, ignoring the
|
||||
proof's existing store state. After any failed-publish op, reconcile re-adds proofs this device
|
||||
itself marked spent/reserved (incl. offline sends the recipient also holds) → bogus balance,
|
||||
real double-spend race. Fix: skip `y`s already in the store with non-Unspent state; verify
|
||||
unknown proofs against the mint (NUT-08) before adding.
|
||||
- **[H] `ops.rs:381-417` + `lib.rs:836-843` — Publish failure after store commit strands funds.**
|
||||
If `send_event` fails, the confirmed `TokenUpdate` is dropped: for `SendToken` the token string
|
||||
is lost (proofs spent locally, recipient never gets it); for `Receive` proofs sit unpublished and
|
||||
a retry errors as duplicate. Reconcile only republishes when `mint_unissued_quotes() != 0`
|
||||
(`ops.rs:429-432`), so it doesn't heal this. Fix: retain the confirmed update on error; force
|
||||
republish whenever the store's unspent set differs from relay state.
|
||||
- **[H] `ops.rs:420-453` — Quote-minting publish can `del` events whose proofs aren't converged
|
||||
yet.** `previous_ids` is the schedule-time relay snapshot; if another device's token was absorbed
|
||||
but not yet converged, the published event `del`s the id while omitting its proofs — those proofs
|
||||
get pruned everywhere while remaining spendable at the mint. Fix: converge before publish, or
|
||||
only `del` when the published proof set fully covers the id.
|
||||
- **[H] `lib.rs:616,651` + `lib.rs:902-912` — Failed nutzap redemption stuck until restart.**
|
||||
`redeeming.insert` happens before scheduling; the `Err` branch never removes the id and
|
||||
`absorb_nutzap` skips anything in `redeeming`, and nothing requeues to `pending_nutzaps`. Fix:
|
||||
remove + requeue with bounded retries on error.
|
||||
- **[H] `lib.rs:79` — `tasks` grows by a handle per relay event** (see S1).
|
||||
- **[H] `lib.rs:147-149,172-185` — `mint_quotes()`/`claim_mint_quote()` dead → created invoices
|
||||
are never displayable or claimable.** `Outcome::QuoteCreated` pushes into `self.quotes` and
|
||||
nothing ever reads it (workspace-wide grep). Quotes also accumulate forever. Fix with the invoice finding in 4.3 below.
|
||||
- **[M] `lib.rs:188-215` — `melt()` and `send_token()` unreachable; `WalletEvent::TokenCreated`
|
||||
emitted only from the dead path** while `workspace/src/lib.rs:226-236` promises "copy it from
|
||||
the wallet panel". Wire into UI or remove; fix the notification text regardless.
|
||||
- **[M] `lib.rs:285-370` — Concurrent `setup()` can fork the wallet key** (only guarded by
|
||||
`loading`); absorption tie-breaks with `created_at >=` on second-granularity timestamps, and
|
||||
stores are keyed by wallet pubkey — the losing key orphans its funds. Fix: `setup_in_progress`
|
||||
guard; deterministic tie-break (event id).
|
||||
- **[M] `lib.rs:430-432` — `processed_events.clear()` at 1000 entries** re-delivers a burst;
|
||||
evict oldest instead.
|
||||
- **[M] `lib.rs:615-661` — Nutzap `unit` tag never validated**; a `usd` nutzap passes local
|
||||
validation. Require `unit == "sat"` in `Nutzap::from_event`.
|
||||
- **[L] `lib.rs:301-303,699-701` — Silent no-ops**: `setup()` when mint already listed;
|
||||
`ensure_wallets` on unparseable mint URLs forever. Surface feedback.
|
||||
- **[L] `ops.rs:107` — ClaimQuote history records `quote.amount.unwrap_or_default()`** (0-sat
|
||||
inflow when mint omits amount); use the summed proof amount.
|
||||
- **[L] `Cargo.toml:23` — `tokio` dep unused** (only `gpui_tokio::Tokio` is referenced). Remove.
|
||||
|
||||
Positives: store-before-publish enforced via cdk prepare/confirm sagas; all relay-derived content
|
||||
author-checked before decryption; `keys.rs` secret handling clean (no Debug on `WalletData`, no
|
||||
secrets in logs, HKDF salt pinned by test); single-flight job queue; `WalletEvent::Error` reaches
|
||||
notifications even with the panel closed.
|
||||
|
||||
### 4.3 `nip60_ui` — wallet panel + dialogs
|
||||
|
||||
- **[H] `wallet.rs:615-726` — Invoice button creates an invoice the user can never see or pay.**
|
||||
After `create_mint_quote` the bolt11 string lands in `WalletRegistry::quotes`, which nothing
|
||||
renders. Receive-via-lightning is broken end-to-end, silently. Fix: render pending quotes
|
||||
(request + copy + check/claim) in the dialog/panel, refreshed on `WalletEvent::Updated`.
|
||||
- **[M] `wallet.rs:765` — `render()` clones the whole history `Vec` every frame** (history is
|
||||
unbounded). Snapshot into `Rc` only on `WalletEvent::Updated`. Same for `filtered_contacts`
|
||||
(`wallet.rs:542`) — cache on input change.
|
||||
- **[L] `wallet.rs:206,284,356` — `SendDialog::tasks` accumulates** (see S1).
|
||||
- **[L] `wallet.rs:262-264` — `load_contacts` gives up silently pre-login**; retry on
|
||||
`StateEvent::SignerChanged` (sidebar already does this).
|
||||
- **[L] `wallet.rs:728-733` vs `lib.rs:28-34` — amount parsing duplicated.** Share `parse_amount`.
|
||||
|
||||
Positives: `cx.notify()` on all mutating paths; per-key hex ElementIds; stored subscriptions;
|
||||
history list windowed via `uniform_list`.
|
||||
|
||||
### 4.4 `browser-signer-proxy` — NIP-46 localhost bridge
|
||||
|
||||
- **[H] `lib.rs:443-571` — Unauthenticated API + `ACAO: *`.** Every response sends
|
||||
`Access-Control-Allow-Origin: *` (verified at :556, :588, :607, :631); no `Host`/`Origin` checks.
|
||||
Consequences: (1) `GET /api/pending` returns full `sign_event`/`nip44_encrypt` params (message
|
||||
plaintext) readable by any web page in Firefox-family browsers and by DNS rebinding anywhere;
|
||||
(2) `:516-517` clears the queue on read → third party can steal requests so the real extension
|
||||
page never sees them; (3) `POST /api/response` accepts any `{id, result}` → forged
|
||||
`get_public_key` substitutes an attacker pubkey as "the user". Fix: drop CORS entirely (the
|
||||
served page is same-origin), reject non-`127.0.0.1:7400` Host, gate `/api/*` on a per-session
|
||||
random token embedded in the served page.
|
||||
- **[H] `lib.rs:339-366` — Timed-out requests leak forever.** The `RequestData` and oneshot sender
|
||||
stay in the maps after the 30 s timeout — plaintext retained for the session, re-served to
|
||||
whoever polls next, map grows unboundedly. Remove entries on timeout (self-removing guard or
|
||||
id-returning `request`).
|
||||
- **[M] `lib.rs:374-380` — `_sign_event` verifies the signature but not the event.** Nothing checks
|
||||
the returned event matches the requested `UnsignedEvent` (pubkey/content/tags/kind/created_at);
|
||||
a compromised signer page can substitute content. Compare before accepting.
|
||||
- **[M] `lib.rs:538` — Decrypted plaintext debug-logged.** `tracing::debug!("Received response …
|
||||
{message:?}")` leaks `nip44_decrypt` output and event content the moment anyone runs with
|
||||
`RUST_LOG=debug`. Log id + method only.
|
||||
- **[M] `lib.rs:447,465-480,523` — Unbounded reads/allocations**: header line grows unbounded;
|
||||
`vec![0u8; content_length]` trusts `Content-Length` (4 GiB reservation from one POST); no
|
||||
connection timeout. Cap header block (16 KiB), cap body (1 MiB), add idle timeout.
|
||||
- **[L] `lib.rs:650-678` — `race_timeout` reports channel-closed as timeout.** Distinguish.
|
||||
- **[L] `lib.rs:574-638` — Three near-identical response writers**; one function with a header
|
||||
options struct (also makes the CORS removal enforceable in one place).
|
||||
- **[L] `lib.rs:546,563,593,612,636` — bare `let _ =` on writes**; centralize the
|
||||
best-effort-write rationale in the helper.
|
||||
|
||||
Positives: loopback-only bind, graceful shutdown tied to handle lifetime, per-request timeouts,
|
||||
UUID ids, bounded channels.
|
||||
|
||||
### 4.5 `nip4e` — per-device keys
|
||||
|
||||
- **[M] `lib.rs:1036,774-860` — Approval button wedged + double-clickable.** `loading.set(true)`
|
||||
without `cx.notify()` keeps the old button enabled (double-click sends the secret in two 4455
|
||||
events); on failure `loading` is never reset and the notification isn't removed. Notify after
|
||||
setting, reset on error, rebuild the notification on both outcomes.
|
||||
- **[M] `lib.rs:403-405` — Rotation skips archiving silently, then overwrites the key.** If the DB
|
||||
read/decrypt fails transiently, `archive_key` is skipped but `set_keys` still overwrites → old
|
||||
chats permanently undecryptable. Make archive failure abort rotation (or log loudly).
|
||||
- **[M] `lib.rs:175-206` — Relay listener can die silently.** `tx.send_async(...).await?` aborts on
|
||||
full bounded(100) channel or dead consumer; the task's `Err` is never inspected. All 4e event
|
||||
processing stops until the next signer change. Log and handle backpressure.
|
||||
- **[M] `lib.rs:90` — `tasks` grows on every settings change** (`get_announcement` runs per
|
||||
settings mutation) — see S1.
|
||||
- **[M] `lib.rs:177,184` — `processed_events` inserts **every** relay event** (insert precedes the
|
||||
kind filter) and grows unbounded. Filter first; bound the set.
|
||||
- **[L] `lib.rs:303-309` — Desktop key backup file written 0644** (subject to umask); use 0600.
|
||||
- **[L] `lib.rs:178` — `current_user` snapshotted per listener**; events between signer change and
|
||||
`get_public_key_async()` resolving are dropped, and a stale snapshot can briefly forward one
|
||||
account's events to the next handler. Re-check per event.
|
||||
- **[L] `storage.rs:29-32` — Malformed stored `created_at` silently resets the rotation
|
||||
interval.** Log the fallback.
|
||||
- **[L] Zeroization gaps** (`app_keys`, decrypted secrets in `decrypt_approval`/`approve`): Low —
|
||||
the nostr stack itself doesn't zeroize; note as defense-in-depth.
|
||||
- **[L] `lib.rs:141,246,426,458,845,854,894,1041` — `.ok()` discards** (see S2); `lib.rs:479`
|
||||
single-letter `n`.
|
||||
- **[L] `lib.rs:136,241,888,914` — `cx.windows().first()` targets an arbitrary window** (matters
|
||||
only if multi-window ever lands).
|
||||
|
||||
Positives: secret never published/logged; transfer encrypted to a fresh one-time keypair;
|
||||
approvals re-validated against the live announcement; stale requests dropped by age; bounded
|
||||
archive; account switch wipes pending state.
|
||||
|
||||
### 4.6 `chat` — chat registry + rooms
|
||||
|
||||
- **[H] `lib.rs:331-338` + `message.rs:203-220` — Failed-to-decrypt messages silently trashed;
|
||||
nothing renders `trash`.** The `trash: Entity<BTreeSet<FailedMessage>>` is written, cleared, and
|
||||
reprocessed, but has **no reader or renderer workspace-wide**. A user whose incoming message
|
||||
can't be decrypted gets zero feedback — core E2EE UX gap. Emit a `ChatEvent` or render an
|
||||
indicator.
|
||||
- **[M] `lib.rs:284,327,620-635,726,733,854,892` — `RwLock` `.read().unwrap()`/`.write().unwrap()`
|
||||
poisoning crash loop.** `search.write()` runs on a background thread; one panic while holding it
|
||||
turns every later foreground read into a panic. These maps are reconstructible heuristics — use
|
||||
the tolerant `poisoned.into_inner()` pattern the codebase already has
|
||||
(`person/src/directory.rs:226-233`).
|
||||
- **[M] `lib.rs:779-780` — Off-by-one index stored in `room_map`** (`rooms.len()` after push).
|
||||
Currently unreachable (batch rooms come from an id-keyed set) but any future reader panics at
|
||||
`self.rooms[index]`. Store `len() - 1`, or drop the never-usefully-read index bookkeeping.
|
||||
- **[M] `lib.rs:440-443,454-461,825-833` — Discarded subscribe result + `unwrap_or_default()`.**
|
||||
`_ = client.subscribe(...)` (rule violation); a DB error becomes "not found" → false
|
||||
"Messaging Relays not found" retry prompt, and a contacts DB error classifies peer rooms as
|
||||
`Request` instead of `Ongoing` (wrong trust classification). Distinguish errors from empty.
|
||||
- **[M] `lib.rs:110,199,232-358,362-414` — Stored task results never inspected** (see S1/S2); a
|
||||
dead notification listener silently stops message delivery — at least `detach_and_log_err`.
|
||||
- **[L] `room.rs:637-646` — `deliver<S>` generic has one implementation**; inline it.
|
||||
- **[L] `lib.rs:867` — `max_by_key().unwrap()`** relies on an invariant three lines up;
|
||||
`let Some(...) else { continue }` documents it.
|
||||
- **[L] `lib.rs:332-338` — Empty-tags rumor is both trashed and delivered** (appears as failure
|
||||
entry *and* rendered message until reprocess). Pick one path.
|
||||
- **[L] `room.rs:350` — `self.members[0]` safe only via a cross-crate invariant**
|
||||
(`extract_public_keys` always appends the author). Add `debug_assert!` or a comment.
|
||||
- **[L] `Cargo.toml:28-29` — `smol` unused.** Remove.
|
||||
- **[L] `lib.rs:182-186` — `let _ = entity.update(...)`** in deferred init; `.log_err()`.
|
||||
|
||||
Positives: signal batching bounds per-update work; wasm yield loop; gift-wrap dedup; seal
|
||||
verification; `WeakEntity` room handles; `ChatEvent::Error`/`Loaded`/`Ping` genuinely surfaced.
|
||||
|
||||
### 4.7 `chat_ui` — chat panel
|
||||
|
||||
- **[H] `lib.rs:605-611` — Your own reactions count twice.** `insert_reaction` pushes
|
||||
unconditionally (keyed only by target id); with `backup: true` (default) the reaction comes back
|
||||
as your own backup gift wrap → inserted again → "👍 2". Text messages are dedup'd by
|
||||
`binary_search`; reactions aren't. The dormant `RoomEvent::Reload` path double-counts too.
|
||||
Track seen reaction event ids.
|
||||
- **[H] `lib.rs:325-334` (+ `272-277`) — Room message-load and connect failures swallowed.** The
|
||||
stored task's `Result` is never inspected: user sees an empty room with no error; relay
|
||||
subscription failures invisible. Push a notification on error.
|
||||
- **[M] `lib.rs:557-581` — Bulk load is O(n²) on the foreground thread.** Per-event
|
||||
binary-search + index rebuild + splice inside one `update`; multi-second stall on big rooms.
|
||||
Add a bulk variant (sort once, rebuild index once, splice once).
|
||||
- **[M] `lib.rs:569-570` — Insert position and `ListState::splice` disagree.** Messages are
|
||||
binary-inserted by `created_at` but the list always appends at `len..len`; mid-`Vec` arrivals
|
||||
shift rows while the list state learns "appended one" → stale height caches, scroll jumps.
|
||||
`nip29_ui/src/lib.rs:516` does it right: `splice(pos + 1..pos + 1, 1)`.
|
||||
- **[M] `lib.rs:1183-1198` — `rendered_texts_by_id` caches mention names forever** (same defect as
|
||||
`nip29_ui` 4.8). A message rendered before the profile loaded shows `@npub1…` all session.
|
||||
Key the cache on directory generation or drop the resolved-text cache.
|
||||
- **[M] `lib.rs:50-51,395-398` — `\p{Emoji}` matches plain digits: typing "2" while replying to
|
||||
exactly one message sends a "2" reaction instead of a message.** Require
|
||||
`[\p{Emoji_Presentation}\p{Extended_Pictographic}\u{200D}\u{FE0F}\u{20E3}]`.
|
||||
- **[M] `lib.rs:110` — `tasks: Vec<Task>` grows without bound** (see S1).
|
||||
- **[M] `lib.rs:547-549,618,623` — `reports_by_id: Arc<RwLock<...>>` nothing else shares.** Plain
|
||||
`BTreeMap` field; removes the poisoning risk for zero benefit.
|
||||
- **[L] `lib.rs:499` — `rumor.id.expect("rumor must have an id")`** — invariant holds today, but
|
||||
degrade + notify instead of panic per house style (also `to_bech32().unwrap()` at 652, 1072).
|
||||
- **[L] `lib.rs:362-370,797-802` — `.ok()` discards `room.update`** (subject change, retry action).
|
||||
- **[L] `lib.rs:722-727` — `encrypted` snapshotted before the file-picker await**; typing while the
|
||||
picker is open classifies the file by stale input state. Re-read after the await.
|
||||
- **[L] `lib.rs:1170-1178` — `load_file` invoked from the render path** (guarded, but render should
|
||||
be pure — move to a subscription).
|
||||
- **[L] `lib.rs:147-221` — Summary comments** ("Define input state", …) violate the comment rule.
|
||||
|
||||
### 4.8 `nip29` — NIP-29 registry + protocol
|
||||
|
||||
- **[H] `lib.rs:541-555` (merge), `582-607` (absorb_list) — Stale group-list events resurrect left
|
||||
groups and get republished.** The kind 10004 list is absorbed without comparing `created_at` and
|
||||
`merge` never removes entries absent from older snapshots; a relay still holding the pre-leave
|
||||
version re-adds the left group, `ensure()` re-creates subscription, and the next
|
||||
`refresh_entry`/`publish_list` republishes the left group to all write relays (privacy angle).
|
||||
Track the last accepted list event's `created_at`; ignore older.
|
||||
- **[H] `group.rs:696-728` (absorb_state) — Group state applied last-write-wins, no ordering
|
||||
guard.** Out-of-order relay delivery (newest-first is common) leaves the *oldest*
|
||||
metadata/members/admins/roles/pins displayed indefinitely. Cache hydration is safe (sorted
|
||||
ascending at `group.rs:507`); the live path is not. Keep `created_at` per state kind (the
|
||||
`forks` map at `lib.rs:669-675` shows the pattern).
|
||||
- **[M] `lib.rs:173-174,189` — Join published optimistically; refusal classified by relay error
|
||||
prose** (`reason.starts_with("duplicate")`); a refused join stays in the public list. Drop the
|
||||
entry on refusal; detect duplicates via a stable marker.
|
||||
- **[M] `lib.rs:173,825-834` — Rejoin flips a private list entry to public** (`join_key` hardcodes
|
||||
`private: false`, `upsert` replaces the whole entry). Preserve the existing flag.
|
||||
- **[M] `group.rs:418-448` + `lib.rs:383-397,1146-1159` — Failed page load leaves `loading_more`
|
||||
stuck true forever** (subscribe error path emits no signal; guard then permanently blocks
|
||||
pagination for the session). Route a synthetic settle on error.
|
||||
- **[M] `lib.rs:308-330` + `discovery.rs:253,267,274,281` — Network failures silently produce
|
||||
"empty" results** (`and_connect().await.ok()`, discarded sync errors): browse shows zero
|
||||
candidates, discovery silently stale. Surface `GroupsEvent::Error`.
|
||||
- **[M] `discovery.rs:240-252,264-267` — Empty contact list → `Filter::authors(∅)` serializes as
|
||||
absent → the sync reconciles **every user's** list, and the pool-wide sweep (kinds
|
||||
`[GroupMetadata, GroupMembers]`, no ids/limit) pulls all NIP-29 state from all relays.**
|
||||
Early-return when `authors.is_empty()`; constrain the sweep.
|
||||
- **[M] `group.rs:457-511` + `lib.rs:906-941,1042-1071` — Every live event triggers a full
|
||||
unlimited `load_envelopes` reload** (re-query + JSON-decode + sort of entire group history per
|
||||
message) and every event is cached forever. Use the single-event absorb path live; cap/prune the
|
||||
cache.
|
||||
- **[M] `lib.rs:90` — `tasks: SmallVec<[Task; 2]>` accumulates per incoming event** (see S1).
|
||||
- **[L] `protocol.rs:389-399,534,650,964` — One malformed optional tag fails the whole parse**
|
||||
(one bad `supported_kinds` value blanks a group's metadata display). Skip + log instead.
|
||||
- **[L] `protocol.rs:24` — Custom kind 9010 not in rust-nostr 0.45.6's NIP-29 range**; confirm
|
||||
against target relay implementations before stable.
|
||||
- **[L] `group.rs:602-611` — `previous` mismatch only logs a warning**; an empty `previous` tag
|
||||
counts as fully verified (`protocol.rs:1046-1056`). Align comment with behavior or escalate.
|
||||
- **[L] `protocol.rs:995-1016` — `TimelineWindow.seen` grows unbounded** (u32/event, never pruned).
|
||||
- **[L] `lib.rs:582-607,661-677` — No client-side author re-check on list/fork subscriptions.**
|
||||
Mitigated by `verify_subscriptions(true)` + `ban_relay_on_mismatch(true)`; note the residual
|
||||
NIP-29 trust model rests on relay admission policy.
|
||||
- **[L] `lib.rs:860-904` — Concurrent `publish_list` can complete out of order** (each snapshots at
|
||||
call time; older snapshot can win). Serialize publishes.
|
||||
- **[L] `lib.rs:1225-1230` — Negentropy/foreign-EOSE cross-talk** fires spurious `Synced` signals
|
||||
from unrelated subscriptions.
|
||||
- **[L] `utils.rs:24,31,35` — Subscription ids truncate the digest to 32 bits**; collision would
|
||||
cross-wire routes. Negligible at this cardinality; use the full digest anyway.
|
||||
- **[L] `lib.rs:1198-1200` — `processed` dedup wholesale-cleared at 10k**; evict oldest instead.
|
||||
- **[L] `lib.rs:723-736` — `previews` grow unbounded per session** (entity + subscription each).
|
||||
- **[L] `group.rs:197-202` — `Group::message(id)` O(n) scan** called per reply preview in render
|
||||
paths → O(n²) timelines. Build an id→row map.
|
||||
- **[L] `Cargo.toml:14` — `person` dep unused.** Remove.
|
||||
- **[L] `lib.rs:1208-1229` — `.ok()` on signal sends** (benign; log or comment per rule).
|
||||
- **[L] `lib.rs:358,949,975` — Inline global-handle chains** (rule: bind to a local).
|
||||
|
||||
Positives: zero reachable panics; tag parsing consistently `get`/`skip`/`first`;
|
||||
`saturating_sub` timestamp math; tasks/subscriptions stored; `WeakEntity` spawns; membership
|
||||
derivation order-independent; strong publish-error propagation.
|
||||
|
||||
### 4.9 `nip29_ui` — group panel + dialogs
|
||||
|
||||
- **[H] `lib.rs:1053-1067` — `rendered_texts_by_id` bakes mention names in permanently.**
|
||||
Computed once per `EventId`; when the profile arrives later the body keeps `@npub1ab…` (the
|
||||
header recovers via re-render, the body doesn't). Key on directory generation or don't cache the
|
||||
resolved text (only the markdown source needs caching).
|
||||
- **[M] `lib.rs:337` — `tasks` accumulates** from every send/upload/react/delete/pin (see S1).
|
||||
- **[M] `lib.rs:1068` — Content `TextView` keyed by row index.** Prepending shifts every index →
|
||||
every visible `TextViewState` re-parses markdown in one frame ("Load earlier messages" jank).
|
||||
Use the message id as element id.
|
||||
- **[M] `message.rs:359` + `lib.rs:1068` — Sibling ElementId collision: reply previews use bare
|
||||
reply index as sibling of the row-index TextView**; when `replies.count > row_index` both sides
|
||||
share `…/[N]/[N]`. Latent today, breaks silently when element types coincide. Namespaced ids.
|
||||
- **[L] `message.rs:192,227,238` — `let _ = panel.update_in(...)`** where sibling call sites log
|
||||
(`log::warn!` at 110-160). Mirror the logged pattern.
|
||||
- **[L] `lib.rs:630` — File-picker failure silently swallowed** (task result never inspected).
|
||||
- **[L] `lib.rs:436-445` — Utterance-end loop clears the reveal unconditionally**; interrupted
|
||||
utterances still fire `on_utterance_end` on many backends, cancelling B's reveal while B's audio
|
||||
plays. Guard with the session id like `message.rs:144` does.
|
||||
- **[L] `message.rs:97,130,168-180` — Per-frame per-row allocations** (`preview()` computed twice
|
||||
per row per frame, custom-emoji `Vec` rebuilt + cloned into the submenu closure every render —
|
||||
and gpui-pre redraws the window on every keystroke). Compute once per render.
|
||||
- **[L] `lib.rs:677,956` — Inline global chains** (rule).
|
||||
- **[L] `lib.rs:407-419` — Subscribe + observe both run `sync` + `track_participants`** (events
|
||||
and notifies fire together) → double O(rows) scan per change.
|
||||
- **[L] `message.rs:322` — Empty role list renders "is now "** (trailing space, no roles). Emit
|
||||
"now a regular member" for the empty case.
|
||||
|
||||
### 4.10 `common` — shared utilities
|
||||
|
||||
- **[H] `media_extractor.rs:52-58` — Every message body has all whitespace collapsed, destroying
|
||||
line breaks.** `cleanup_text`'s `\s+ → " "` runs on the entire remaining text unconditionally,
|
||||
so multi-line messages render as one line in both chat and groups (via `chat/message.rs:99-103`).
|
||||
Normalize only around removed URLs; never touch `\n` in untouched regions.
|
||||
- **[M] `media_extractor.rs:15-18,56,62-65` — Regexes compiled per message** (the cache struct's
|
||||
own entry point defeats it). `static LazyLock<Regex>`.
|
||||
- **[M] `parser.rs:240,254,260` — `NostrParserIter::next` recurses per token; depth is
|
||||
attacker-controlled** (~9 bytes/token of crafted `nostr:` URIs). Desktop's 8 MB stack survives;
|
||||
the wasm32 web build can crash the tab. Rewrite as a loop.
|
||||
- **[M] `event.rs:12-18` — `uniq_id` uses `DefaultHasher` and is persisted to published events**
|
||||
(see S5).
|
||||
- **[M] `paths.rs:7,14,23,31` — `expect()` on `dirs::download_dir()` etc.** — legitimately `None`
|
||||
on Linux without XDG user-dirs → panic on export (`workspace/src/lib.rs:405`). Return `Option`
|
||||
and handle.
|
||||
- **[L] `display.rs:18,20,39` — `as i64` on relay-controlled timestamps** (contained by
|
||||
`LocalResult`); fallback magic strings `"9999"`/`"1m"`.
|
||||
- **[L] `event.rs:12-53` — `Event`/`UnsignedEvent` impls duplicate the same logic with drift**
|
||||
(one sorts, one doesn't). Factor into one helper — same helper fixes S5.
|
||||
- **[L] `debounced_delay.rs:33` — `_ = channel.send(())`** explicit silent discard (harmless;
|
||||
satisfy the rule with a comment + `if let Err`).
|
||||
|
||||
Positives: `web_store.rs` error handling exemplary; parser char-boundary-safe throughout;
|
||||
`DebouncedDelay` cancellation correct; time formatting degrades gracefully.
|
||||
|
||||
### 4.11 `person` — profiles + web of trust
|
||||
|
||||
- **[H] `lib.rs:310-333` + `204-245` — One failed metadata batch permanently blacklists those
|
||||
pubkeys.** `in_flight` removal happens only on metadata arrival; `get_metadata` uses
|
||||
`TIMEOUT = 2 s` (tight for bootstrap relays + EOSE) and on failure keys stay in `in_flight`
|
||||
forever → those people show truncated-pubkey placeholders until restart. Return failed keys on
|
||||
error and release them; raise the request timeout independently of the batching window.
|
||||
- **[M] `lib.rs:175` — Malformed metadata wipes a good profile.**
|
||||
`Metadata::from_json(...).unwrap_or_default()` replaces previously-loaded data with a default
|
||||
`Person`; content is attacker-influenceable (anyone can publish metadata for any key). Skip on
|
||||
parse failure.
|
||||
- **[M] `directory.rs:110` (+ `lib.rs:255-262`) — Task handles accumulate per `track`** (see S1).
|
||||
- **[M] `directory.rs:161-172` — `person()` performs side effects (lock write + network fetch) from
|
||||
a render-time getter.** Deduped via `pending`, but order-dependent render behavior; consider an
|
||||
explicit `ensure_tracked` from update paths.
|
||||
- **[L] `wot.rs:100,343` — DB errors silently become empty results**; add `log::warn`.
|
||||
- **[L] `wot.rs:150-153` — `let _ = this.update(...)`** (commented, periodic; noted for the rule).
|
||||
- **[L] `person.rs:23-47` — `PartialEq/Eq/PartialOrd/Ord/Hash` for `Person` unused
|
||||
workspace-wide**; the `Ord` impl clones `Metadata` twice per comparison. Delete.
|
||||
- **[L] `person.rs:66-68,124-138` — `metadata()` clones per call; `name()` calls it twice** per
|
||||
invocation → 2+ clones per avatar/name row per frame. Borrow or cache.
|
||||
- **[L] `person.rs:166-177` — `shorten_pubkey` doc example doesn't match output; panics if
|
||||
`len ≥ 63`** (callers use 4/8 today). Note: the `let Ok(...) =` pattern compiles only because
|
||||
`ToBech32::Err = Infallible` — worth a comment since it looks like a compile error.
|
||||
|
||||
Positives: WoT builder caps hostile publishers; poisoning-tolerant lock helpers; bounded dispatch
|
||||
channel; `limit(1)` newest-event lookups verified correct against nostr-lmdb ordering.
|
||||
|
||||
### 4.12 `speech` — dictation + TTS
|
||||
|
||||
- **[H] `parakeet.rs:134` — Inference worker exits after 30 s idle, permanently disabling
|
||||
dictation.** `recv_timeout(30s)` kills the thread; the `OnceLock` sender survives, so after
|
||||
that every `send` fails — and `send_or_log` (`:200-204`) logs at **debug** and drops. A
|
||||
`MAX_SECONDS = 120` dictation records fine, then `finish()`'s `Transcribe` goes into the void;
|
||||
gpui-kit's 180 s `stop_timeout` then force-ends with an **empty** transcript. Net: dictations
|
||||
longer than 30 s silently lost after up to a 3-minute hang. Fix: block on `recv()` (an `Unload`
|
||||
command already exists for explicit retirement), and make `finish()` surface send failure via
|
||||
`sink.error`.
|
||||
- **[M] `parakeet.rs:63-68` — Audio past 120 s silently discarded** mid-sentence with no
|
||||
indication. Surface once or stop capture at the cap.
|
||||
- **[M] `parakeet.rs:119-128` — Worker-spawn failure leaves a permanently dead channel** (receiver
|
||||
dropped, sender cached in the `OnceLock` forever). Don't cache on failure.
|
||||
- **[M] `speaker.rs:36-53` — `Tts::default()` constructed lazily inside `speak()` invoked from
|
||||
render paths**; first-use engine init can take tens-to-hundreds of ms on the UI thread
|
||||
mid-render. Initialize eagerly at panel creation or on the background executor.
|
||||
- **[L] `model.rs:82-97` — `status()` does filesystem I/O per call** while the settings dialog
|
||||
polls every 500 ms on the foreground. Cache status in the state mutex.
|
||||
- **[L] `model.rs:183` — `let _unused = fs::remove_file(...)`**; log per rule.
|
||||
- **[L] `web.rs:23-25` — `is_available` hardcodes `true`**; mic button renders enabled in browsers
|
||||
without `SpeechRecognition`. Reflect actual support.
|
||||
- **[L] `web.rs:200,218` — `let _ignored` abort/stop** — commented, best-effort; fine.
|
||||
|
||||
Positives: `catch_unwind` isolation around model load/transcribe; staged download with SHA-256 +
|
||||
size cap + atomic rename; dedicated threads for inference/download; all other fallible paths
|
||||
reach `sink.error`.
|
||||
|
||||
### 4.13 `settings` — persistence + themes
|
||||
|
||||
- **[H] `lib.rs:403-404` + `311-314` — Failed load silently resets settings and destroys the saved
|
||||
file.** `unwrap_or(Settings::default())` swallows I/O and JSON errors; the observer then
|
||||
immediately writes defaults over the user's file (losing `trusted_relays`, pins, wallet prefs)
|
||||
with no log. Log, keep defaults in memory, but don't persist until a real user change.
|
||||
- **[H] `lib.rs:423-438` — Saves discard errors and race each other.** `.await.ok()` hides
|
||||
disk-full/permission failures; every change detaches a new concurrent write to the same path —
|
||||
last-completion-wins can overwrite newer snapshots and interleaved truncate/write can corrupt
|
||||
the file (font-size input ticks each trigger a save). Single writer slot + temp-file + rename +
|
||||
`log_err`.
|
||||
- **[L] `lib.rs:425` — Serialization failure silently skips saving**; log.
|
||||
- **[L] `lib.rs:596-639,671-683` — `apply_persisted_theme` re-reads and re-parses the settings
|
||||
file four times.** Parse once.
|
||||
- **[L] `lib.rs:211,408` — `Url::parse(DEFAULT_FILE_SERVER).unwrap()`**; `OnceLock` or a reasoned
|
||||
`expect`.
|
||||
|
||||
Positives: accessor macros keep 20 settings consistent; serde default/alias migration fields with
|
||||
tests covering old formats; wasm path handled.
|
||||
|
||||
### 4.14 `workspace` — core + dock + sidebar
|
||||
|
||||
- **[H] `dock/mod.rs:50-73` + `lib.rs:309-320` — Tab dedup is unreachable; repeated opens stack
|
||||
duplicate tabs.** `find_panel` matches on `PanelId`, which is always `PanelId::from(entity_id())`
|
||||
of a panel created in the same statement — so it can never match (verified against gpui-base
|
||||
0.9.1 `add_panel_inner`, which inserts without dedup). Clicking the same chat row twice opens
|
||||
two live panels for one room; ShowInbox/Browse/Contacts/Backup multiply tabs. Key panels by
|
||||
domain identity (singleton entities per panel name, or room/group key). The ~55 lines of
|
||||
`find_panel`/`move_panel` are dead until then.
|
||||
- **[H] `sidebar/rows.rs:54-57` — "Ignore" dispatches `ClosePanel` where it can never be handled.**
|
||||
The only handler is on the tab-group frame; the alert dialog holds focus at that moment, and
|
||||
dispatch resolves along the focus node's ancestor path — the dock tree isn't on it. Clicking
|
||||
"Ignore" is a silent no-op for a destructive-intent action. Close the room via the dock handle
|
||||
already in scope, or restore focus first.
|
||||
- **[H] `sidebar/header.rs:38,236-268` — Updater buttons render stale status.** Nothing observes
|
||||
the `AutoUpdater` entity (gpui-pre has no read-tracking); a staged update can sit invisible on
|
||||
an idle window indefinitely. Add `cx.observe(&updater, …)`.
|
||||
- **[M] `sidebar/mod.rs:192-206` + `search.rs:70-117` — Full message-index scan on the foreground
|
||||
thread for every keystroke.** Only the *global* search is debounced; local search synchronously
|
||||
scans every indexed message, sorts the entire match set, then truncates. Debounce + background
|
||||
spawn + partial top-`limit` select.
|
||||
- **[M] `sidebar/list.rs:41,178-248` + `group_view.rs:88-136` — Row model rebuilt on every sidebar
|
||||
render** (clone + partition + pin-tag scans ×visible rows; sidebar re-renders on 500 ms ticks,
|
||||
keystrokes, group updates). Cache the row model in state, refresh on registry events.
|
||||
- **[M] `sidebar/search.rs:152-155` — Global search failure renders as "No profiles found".**
|
||||
Surface the error state.
|
||||
- **[M] `lib.rs:410-421` — Save-dialog failure silently cancels the backup** (`Err(_) => return`):
|
||||
indistinguishable from user cancel; user believes the key was backed up. Split arms + notify.
|
||||
- **[M] `sidebar/mod.rs:139-142` — Whole-sidebar re-render on every `GroupsEvent::Updated`**
|
||||
(acknowledged TODO; compounds the row-rebuild cost). Gate on visibility.
|
||||
- **[L] `dock/skin.rs:25,35-37,55` — `toggle_button_visible: Cell<bool>` is write-once** (set true
|
||||
in ctor, never elsewhere) → dead state + dead branch at `tab_panel.rs:212`. Delete.
|
||||
- **[L] `sidebar/rows.rs:174-374` — Index-based row ids** (see S6).
|
||||
- **[L] `dock/tab_panel.rs` ×6, `skin.rs:262`, `sidebar/search.rs:164,178`, `lib.rs:641` — `let _ =
|
||||
`/`.ok()` on fallible updates** (see S2); `lib.rs:641` is pre-destructive-reset — make it visible.
|
||||
- **[L] `rows.rs:220-222,346-351`, `list.rs:53-55` — Inline global chains** (rule).
|
||||
- **[L] `lib.rs:84`, `sidebar/mod.rs:99`, `sidebar/inbox.rs:68,118` — `tasks` accumulate** (see S1).
|
||||
- **[L] `lib.rs:54` — Typo: `DeviceNotifcation`.**
|
||||
|
||||
Positives: careful `WeakEntity` hygiene; virtualized lists via `uniform_list` + `Rc` snapshots;
|
||||
`InvalidPanel` preserves unknown panels across load/save; `theme_name_at` maps the palette's
|
||||
original `IndexPath` correctly; `cargo check -p workspace` passes.
|
||||
|
||||
### 4.15 `workspace` — dialogs + panels
|
||||
|
||||
- **[H] `panels/contact_list.rs:187-191` — "Update" republishes the contact list with relay hints
|
||||
and petnames stripped.** `load()` keeps only `tags.public_keys()`; `update()` rebuilds every
|
||||
entry as `Contact::new(*public_key)` (`relay_url: None, alias: None`) while the UI reports
|
||||
success. Silent NIP-02 data loss on relays. Carry the original `Contact`s through.
|
||||
- **[H] `dialogs/new_chat.rs:60-64` — Contact list doesn't re-filter while typing.** No
|
||||
`InputEvent::Change` handling; the input renders its own state entity so typing doesn't notify
|
||||
`NewChat`; results stay stale until an unrelated click/Enter. `cx.notify()` on Change. Same
|
||||
pattern, cosmetic: `profile.rs:320-326`.
|
||||
- **[H] `panels/backup.rs:59-76` — Keyring read failures silently swallowed**; backup panel just
|
||||
shows empty inputs. Log + surface.
|
||||
- **[H] `dialogs/profile.rs:227-235` — Invalid avatar/website URLs silently dropped on save** while
|
||||
"Profile updated successfully" shows. Validate both up front; inline error.
|
||||
- **[M] `dialogs/screening.rs:52-55` — `on_release_in` calls `window.close_all_dialogs(cx)`**,
|
||||
nuking unrelated stacked dialogs (including the mutual-contacts dialog this view opens). Close
|
||||
just the hosting dialog / cancel tasks only.
|
||||
- **[M] `dialogs/screening.rs:126-168` — DB-fallback "mutual contacts" has different semantics than
|
||||
the WoT list** (followers vs. your-contacts-who-follow). Intersect or relabel.
|
||||
- **[M] `dialogs/screening.rs:211-232` — NIP-05 network errors render as "address does not
|
||||
match"** (`unwrap_or(false)` asserts infrastructure failure as fact about the person). Tri-state.
|
||||
- **[M] `dialogs/screening.rs:250-282` — Report submit gives no failure feedback.**
|
||||
- **[M] `dialogs/import.rs:129-185` — Enter bypasses the loading guard** (no in-method guard; all
|
||||
three `PressEnter` subscriptions call `login()` directly) → concurrent double import. Same class:
|
||||
`contact_list.rs:176-228`, `relays.rs:377,521`.
|
||||
- **[M] `dialogs/import.rs:31-40` — Unlock dialog is inescapable** (no cancel/logout path for a
|
||||
forgotten passphrase). Add a "start over" escape hatch for stable.
|
||||
- **[M] `panels/browse.rs:484-493,316-348` — Card list recomputed per render *and* per visible
|
||||
row** (O(n) clone + registry reads ×(rows+1) per frame during scroll). Compute once per render
|
||||
(`new_chat.rs`'s `Rc<Vec>` pattern).
|
||||
- **[M] `panels/greeter.rs:127-137` — Every refresh fetches the full message history of every room
|
||||
to take `.last()`.** Query latest-per-room instead.
|
||||
- **[M] `dialogs/new_chat.rs:138-160,388` — Filter allocates per contact per keystroke-scale
|
||||
render** (`to_hex().to_lowercase()` per contact). Cache lowercase names / filter once per change.
|
||||
- **[M] `dialogs/settings.rs:124-148` — Speech-model errors can be consumed before the status
|
||||
change check** and dropped. Check `error` before the equality early-return.
|
||||
- **[M] `new_chat.rs:282-315` vs `profile.rs:274-313` — Duplicated room-open logic with divergent
|
||||
dedup.** Unify on find-or-create.
|
||||
- **[L] `panels/contact_list.rs:238,259` + `greeter.rs:226-402` — HashSet/index-based element ids**
|
||||
(see S6).
|
||||
- **[L] `panels/contact_list.rs:288` — Copy bug: contacts panel says "Please add some relays."**
|
||||
- **[L] `dialogs/new_group.rs:392` — `let _ = this.update(...)`** (see S2; `relays.rs:623-629` is
|
||||
the correct logged pattern).
|
||||
- **[L] `panels/backup.rs:66,70` — `.unwrap()` on bech32** (infallible today; still banned pattern).
|
||||
- **[L] `dialogs/restore.rs:52-62`, `contact_list.rs:130`, `relays.rs:352-355,497-500` — Untrimmed
|
||||
paste input fails parse** ("trailing newline = invalid key"); `relays` has a weak
|
||||
`starts_with("ws")` check.
|
||||
- **[L] `dialogs/relays.rs:124-140,360-366` — Relay metadata can never be reset; duplicate add is a
|
||||
silent no-op** that can't update metadata.
|
||||
- **[L] `dialogs/settings.rs:517-543` — Toggles compute `!rendered_value` instead of the switch's
|
||||
`checked`**; two fast clicks collapse into one. Match `onboarding.rs:398-399`.
|
||||
- **[L] Dialog `tasks` fields never pruned** (`import.rs:63`, `restore.rs:24`, `relays.rs:45,57`,
|
||||
`contact_list.rs:52`, `profile.rs:55`, `new_chat.rs:50`, `new_group.rs:94`) — see S1;
|
||||
`screening.rs`/`greeter.rs` show the right patterns.
|
||||
- **[L] `new_chat.rs:93-95`, `profile.rs:278-280`, `screening.rs:90-92` — Signed-out paths return
|
||||
silently**; no retry when login completes later.
|
||||
- **[L] Dialogs can be stacked repeatedly** — no `has_active_dialog` checks in any `open()`.
|
||||
- **[L] `new_group.rs:137-140` — `GroupsEvent::Created { warning, .. }` discards the non-fatal
|
||||
warning.**
|
||||
|
||||
Positives: `limit(1)` newest-event pattern verified correct on both backends; entities created
|
||||
before `open_dialog` per gpui-kit contract; `new_group.rs` has a solid error lifecycle; heavy work
|
||||
consistently on background; no unused deps in the crate.
|
||||
|
||||
### 4.16 `ui` — shared components
|
||||
|
||||
- **[M] `avatar.rs:128-186` — `PixelAvatar` regenerates the full pattern every frame** (FNV-1a +
|
||||
xorshift 64-cell grid + ~50 child divs per avatar; ~30 visible avatars, redrawn on every
|
||||
keystroke) — the biggest hot-path cost in the main view. Memoize the deterministic pattern
|
||||
(seed → `[u8; 64]` cache); hoist `FALLBACK_SEED` to a `&'static str` path.
|
||||
- **[L] `text.rs:11-38` — `InlineReplacement` is `pub` but unreachable externally**; same for
|
||||
`RUN_WINDOW_SECS` (`message.rs:28`). `pub(crate)` or document as extension points.
|
||||
- **[L] `message.rs:766` — `RevealText::new` is the only public constructor without a doc line.**
|
||||
- **[L] `message.rs:775-796` — `RevealText` re-splits text into a `Vec<SharedString>` every
|
||||
animation frame**; split once in `new`.
|
||||
- **[L] `text.rs:89-95` — Non-web, non-nevent Nostr links are silently dead** (`npub`/`nprofile`
|
||||
clicks do nothing). Route to a profile view or drop the link markup.
|
||||
- **[L] `Cargo.toml:15,19` — `web-time` and `uuid` unused.** Remove.
|
||||
|
||||
Positives: consistent `RenderOnce` + `Styled` builder pattern; `#[must_use]` builders; documented
|
||||
public items; char-boundary-safe slicing; truncating casts provably bounded.
|
||||
|
||||
### 4.17 `auto_update`
|
||||
|
||||
- **[M] `lib.rs:92` — Checksum-only verification; no signing key configured.** SHA256SUMS comes
|
||||
from the same host as the artifact — protects against corruption, not a compromised release
|
||||
host/account. The engine already supports minisign/Ed25519; publish a sidecar signature and pin
|
||||
the public key before stable.
|
||||
- **Verified non-issue:** the `futures::executor::block_on` in `source.rs:152,170` runs on the GPUI
|
||||
**background** executor (verified through `gpui-updater-pre 0.2.0`'s `Updater::check` →
|
||||
`engine.check()` → `fetch_latest` call chain) — no UI freeze. Severity Low: it parks one
|
||||
background thread up to ~60 s during a check. Optional tidy-up only.
|
||||
- **[L] `source.rs:156-163,180-183` — Asset URLs not scheme-checked**; assert `https://`.
|
||||
- **[L] `lib.rs:119-131,145-147` — A failed check shows for 5 s then reports "Up to date"**
|
||||
(`error_revert` → `Idle`, and `Idle` counts as up-to-date). Distinct "check failed" state.
|
||||
- **[L] `lib.rs:103,128` — `.ok()` on entity updates** (see S2).
|
||||
- **[L] `lib.rs:134-139` — Auto-download/stage ~2 min after startup without a prompt**; fine on
|
||||
unmetered, worth a setting.
|
||||
|
||||
Positives: strict `latest > current` gate (no downgrades); drafts/prereleases skipped; 30 s
|
||||
timeouts; defensive checksum parsing; good tests.
|
||||
|
||||
### 4.18 `gpui_tokio`
|
||||
|
||||
- **[L] `lib.rs:14` — `.expect("Failed to initialize Tokio")`**; per the no-panic rule, `init`
|
||||
returning `Result` would be more in keeping (realistically unreachable).
|
||||
- **[L] `lib.rs:38` — `pub struct Tokio {}`** namespace struct; `pub struct Tokio;` or free
|
||||
functions.
|
||||
|
||||
Positives: the `defer`-guard cancellation pattern is correct in both completion and drop cases;
|
||||
`shutdown_background()` avoids blocking exit; `spawn_result` maps `JoinError` into `anyhow`.
|
||||
No deadlock risk across the two runtimes (verified: nothing waits on GPUI from tokio or holds a
|
||||
lock across the boundary).
|
||||
|
||||
### 4.19 `desktop` / `web` / `assets` entries
|
||||
|
||||
- **[M] `desktop/src/main.rs:20-26,153-170` — Secret key accepted as a CLI argument.** `--sec
|
||||
nsec1…` puts the master key in the process list (`ps`, `/proc`, shell history); the parse-error
|
||||
path echoes the raw (near-)key to stderr. Read from stdin/env; print error kind only, never the
|
||||
input.
|
||||
- **[L] `desktop/src/main.rs:131-150` — Seven startup unwraps on embedded assets.** Deterministic,
|
||||
but one corrupt font aborts launch; skip bad fonts with a warning and fail only if zero loaded.
|
||||
`open_window().expect` is fair (genuinely unrecoverable). The startup `block_on` for fonts
|
||||
(main.rs:135) is intentional and acceptable — noted so it isn't confused with the anti-pattern.
|
||||
- **[L] `web/src/lib.rs:122` — `expect("Failed to open window…")` aborts the wasm module** (blank
|
||||
page + console error; `panic = "abort"`). A DOM-level error would be kinder.
|
||||
- **[L] `web/src/lib.rs:43` — `.ok()` on console-log init** (justified; noted for completeness).
|
||||
- **[L] `desktop/Cargo.toml:37` + `web/Cargo.toml:13` — `ui` dep unused in both entries.** Remove
|
||||
unless it exists to force feature unification (then comment why).
|
||||
- **[L] `assets/Cargo.toml:9` — `anyhow` unused** (code uses `gpui::Result`). Remove.
|
||||
|
||||
Positives: `web` opens IndexedDB before `run()`, panic hook + both logging stacks via `OnceLock`;
|
||||
`assets` ships the theme-schema test.
|
||||
|
||||
## 5. Tooling Validation
|
||||
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| `cargo clippy --workspace --all-targets` | clean — zero warnings |
|
||||
| `cargo fmt --all --check` | clean |
|
||||
| `cargo check -p workspace`, `-p person` | pass |
|
||||
| `let _ =` / `.ok()` sites (non-test) | ~156 — mostly entity-update-expected-error cases; see S2 |
|
||||
| `tasks:` container declarations | ~48 sites across 20 owners — see S1 |
|
||||
| Startup `block_on` sites | 3 (`state` DB open, `desktop` fonts — both acceptable-at-startup; `auto_update` — background thread, non-issue) |
|
||||
|
||||
## 6. Verified Non-Issues (checked, do not re-flag)
|
||||
|
||||
- `auto_update/source.rs` `block_on`: background executor only (verified through the
|
||||
`gpui-updater-pre 0.2.0` call chain) — no UI freeze.
|
||||
- `nip29/src/protocol.rs:1072` `block_on`: test-only.
|
||||
- No locks held across `.await` anywhere in the workspace (all audited crates).
|
||||
- `person.rs:170` `let Ok(pubkey) = to_bech32()` — irrefutable (`Err = Infallible`).
|
||||
- `stored_contact_list`'s `limit(1)` + `.next()` — correct: nostr-lmdb orders by `created_at`
|
||||
descending.
|
||||
- `nip60`/`nip60_ui` have zero non-test `unwrap`/`expect`/`let _ =`; indexing is guarded; no
|
||||
truncating casts.
|
||||
- `nip29_ui`'s `sync()` splice math traced against `ListState` semantics — correct (it's `chat_ui`
|
||||
' that has the mismatch).
|
||||
- gpui-pre's entity read-tracking re-renders panels on `Directory`/`SpeechState` notifications —
|
||||
the missing-`cx.notify()` suspicion in speech handlers is fine; the stale-cache bugs above are
|
||||
real, the observation isn't.
|
||||
- `nip60::init` startup ordering is sound for normal flows; the CLI-key path fires `SignerChanged`
|
||||
before wallet subscription exists (app-wide init-order caveat, affects all registries equally).
|
||||
- `nip60` melt double-spend paths are latent (dead API), not live.
|
||||
|
||||
## 7. What's Done Well
|
||||
|
||||
- **Clippy-clean, fmt-clean workspace** with no reachable panics in `nip29`, `nip60`, `ui`,
|
||||
`nip29_ui`, `person` production paths; hostile-input discipline in the WoT builder, speech
|
||||
downloads (SHA-256 + size cap + atomic rename), and auto-update (fail-closed checksums, no
|
||||
downgrades).
|
||||
- **`verify_subscriptions(true)` + `ban_relay_on_mismatch(true)`** in `state` is the single most
|
||||
valuable client-side trust boundary, and it's on.
|
||||
- **Entity/subscription hygiene is strong overall**: subscriptions stored in `_subscriptions`,
|
||||
`WeakEntity` used for room/group/dock back-references, no strong entity cycles found anywhere,
|
||||
no dropped-subscription bugs.
|
||||
- **Secrets discipline**: no secret is ever logged or published in `nip4e`/`nip60`/`state` desktop
|
||||
paths (verified against every log call); `Keys`' vendored `Debug` hides secrets.
|
||||
- **Errors generally do reach the UI** where it matters most (group writes via `util::report`,
|
||||
wallet events, speech errors, import/restore flows) — the gaps are enumerated above.
|
||||
|
||||
## 8. Recommended Remediation Order
|
||||
|
||||
**P0 — before any release candidate (blocks release):**
|
||||
1. nip60 deletion spoofing (4.2 #1) — one-line precedence fix + author check.
|
||||
2. browser-signer-proxy auth/CORS/token + timeout cleanup (4.4 #1-2).
|
||||
3. state startup DB fallback (4.1 #1) and web localStorage encryption (4.1 #2).
|
||||
4. nip60 reconcile/republish money-safety trio (4.2 #2-4) + stuck `redeeming` (4.2 #5).
|
||||
5. state file-cache collision (4.1 #4) — wrong-file disclosure.
|
||||
6. chat_ui reaction double-count (4.7 #1) and common whitespace collapse (4.10 #1).
|
||||
7. contact-list Update data loss (4.15 #1).
|
||||
8. speech parakeet worker lifetime (4.12 #1).
|
||||
9. settings load/save durability (4.13 #1-2).
|
||||
10. nip29 `created_at` guards (4.8 #1-2).
|
||||
|
||||
**P1 — fix during the release cycle:**
|
||||
Dock panel identity + "Ignore" dispatch (4.14 #1-2); updater observation (4.14 #3); invoice flow
|
||||
or removal (4.3 #1); person `in_flight` release (4.11 #1); chat trash UI (4.6 #1); chat_ui load
|
||||
errors + splice + quadratic insert (4.7 #2-4); profile URL validation, backup keyring surfacing,
|
||||
new_chat re-filter, screening dialog cluster (4.15); parser recursion for wasm (4.10 #3);
|
||||
`DefaultHasher` → stable hash before more room ids ship (S5); nip4e approval button + rotation
|
||||
archive + listener death (4.5 #1-3); proxy plaintext leak + event-substitution check (4.4 #3-4).
|
||||
|
||||
**P2 — hygiene, in one mechanical sweep:**
|
||||
S1 (`tasks` containers → replace-per-spawn / `detach_and_log_err`), S2 (`let _ =`/`.ok()` →
|
||||
`.log_err()`), S6 (domain-keyed element ids), unused deps (`chat`: smol; `nip29`: person;
|
||||
`nip60`: tokio; `ui`: web-time, uuid; `assets`: anyhow; entries: ui), dead code
|
||||
(`nip60` melt/send/quotes decision, `state::passphrase_required`, `DownloadedFile::save`,
|
||||
`dock::find_panel` fix-or-delete, `skin.rs` write-once flag, `Person` trait impls), avatar
|
||||
memoization, sidebar row-model caching, rename `DeviceNotifcation`, summary-comment cleanup.
|
||||
|
||||
---
|
||||
|
||||
*Method note: every crate's sources were read in full by a dedicated audit pass; GPUI/fork API
|
||||
claims were verified against the vendored `gpui-pre 0.3.12`, `gpui-base 0.9.1`,
|
||||
`gpui-component 0.9.1`, and rust-nostr sources; the Critical and top-High findings were
|
||||
independently re-verified by hand against the working tree at `8fa79aef`. No files were modified
|
||||
during the audit.*
|
||||
Reference in New Issue
Block a user