fix(ui): refuse private-room edits when secret isn't local yet (#299)

[sanity]

Problem

Three UI edit paths in River fell through to SealedBytes::public (plaintext) when the locally-decrypted room secret was not yet available, leaking plaintext into private rooms:

• NicknameField::save_changes — the Nickname edit in a private room publishes plaintext when the room secret isn't locally available #299 regression
• RoomNameField::update_room_name — same shape, owner-only
• EditRoomModal::RoomDescriptionField::update_description — same shape, owner-only

All three carried the same match current_secret_opt { Some(...) => seal_bytes(...), _ => SealedBytes::public(...) } block. In a private room the _ arm publishes the user's chosen field as plaintext into a nominally encrypted contract delta. The window is the gap between joining a private room and receiving the room secret — narrow in practice but silent, permanent, and propagated to the network.

Approach

Centralised the privacy decision in a single helper seal_for_room(is_private, current_secret_opt, plaintext) in ui/src/util/ecies.rs:

• public room → Some(SealedBytes::public(...)) (correct — no encryption to defer)
• secret available → Some(seal_bytes(...))
• private room + no secret → None (caller must defer)

All three call sites now consult the helper. When it returns None, the caller logs a warning, reverts the input signal to the on-network value (so the UI doesn't lie about what was saved), and skips publishing — no plaintext delta ever leaves the device. The user can retry once the secret has arrived.

The nickname-field save path was also restructured to re-read ROOMS at save time so the is_private, the secret, and the rejoin probe all see the same consistent snapshot — the captured-at-mount current_secret_opt could miss a secret that arrived after the modal opened.

The matching EditRoomModal empty-description case is intentionally exempt: Some(empty) is published as None (clears the field), and there's nothing to leak.

Testing

Four unit tests on seal_for_room pin the decision matrix, including the #299 regression case (private_room_without_secret_defers) which fails if the function ever returns Some(SealedBytes::public(...)) for the private+no-secret path:

• public_room_with_no_secret_returns_public_sealed
• public_room_with_secret_uses_secret_for_consistency
• private_room_with_secret_seals (with round-trip decrypt check)
• private_room_without_secret_defers — pins the regression

Local validation:

• cargo check -p river-ui --target wasm32-unknown-unknown --features no-sync — clean
• cargo test -p river-ui --bins — 4 new tests pass + all 242 pre-existing tests pass
• cd common && cargo test --no-default-features --features migration private_room — clean
• cargo fmt clean

The save-path closures themselves aren't unit-tested (tightly bound to Dioxus signals and global state), but the helper carries the privacy invariant they all depend on — the regression is pinned at the only place it can be expressed in a unit test.

Closes #299

[AI-assisted - Claude]

[sanity]

Comprehensive PR Review: #317

Summary

• PR Title: fix(ui): refuse private-room edits when secret isn't local yet (#299)
• Type: fix (privacy / cryptography surface)
• Linked Issues: Closes #299; spawned follow-up #318
• Review tier: Full (cryptography / privacy)
• Reviewers run: code-first, testing, skeptical, big-picture, plus an external Codex pass — then a re-review (skeptical + Codex) on the updated HEAD

---

Code-First Analysis

Independent Understanding: Introduces seal_for_room(is_private, current_secret_opt, plaintext) -> Option<SealedBytes> at ui/src/util/ecies.rs with three branches keyed on is_private: public → Some(public); private+secret → Some(seal_bytes); private+no-secret → None (caller defers). All three previously-buggy edit sites (nickname_field, room_name_field, edit_room_modal::RoomDescriptionField) route through it and skip publish on None. nickname_field's save closure was restructured to re-read ROOMS at save time so is_private, the secret, and the rejoin probe come from one consistent snapshot.

Alignment with stated intent: Matches. Scope expansion to the two analogous bugs in the room-name and description paths is correct application of the finish-the-fix rule.

---

Testing Assessment

Coverage Level: Adequate.

Test Type	Status	Notes
Unit (helper)	✅	Five tests pin the full (is_private × secret) matrix + a source-grep call-site pin + a substring-absence assertion against to_string_lossy leakage
Closure-level	⚠️	Save closures aren't unit-testable (Dioxus globals); the pure-helper test plus the source-grep pin are the substitute
Integration (common)	N/A	No regression in common/tests/private_room_test.rs

Regression Test: private_room_without_secret_defers pins the exact #299 case (asserts None, not Some(SealedBytes::public)). Defended further by seal_for_room_call_sites_pinned which strips line comments before asserting seal_for_room( is present and SealedBytes::public( is absent in each of the three edit-delta files.

---

Skeptical Findings

Risk Level: Low after fixes.

Concern	Severity	Location	Status
Public→private mid-edit race (Codex)	Medium	nickname_field.rs::save_changes defer	Filed #318 — narrow window; Configuration::apply_delta already backstops room name + description; member_info has no such backstop
Helper match ordered Some(secret) first → doc/code mismatch	Medium	ecies.rs:36-42	Fixed — reordered so is_private is the primary axis
Silent fall-through on ROOMS.try_read() failure / no room	Medium	nickname_field.rs:81-87	Fixed — added warn! logs
Missing source-grep pin for call sites	Important	—	Fixed — seal_for_room_call_sites_pinned added, then hardened per Codex re-review to strip comments first
Substring-absence assertion on private+secret seal	Nice	ecies.rs tests	Fixed — added to private_room_with_secret_seals
temp_nickname not refreshed on prop changes mid-edit	Low	nickname_field.rs:60	Pre-existing, not in scope — worth a separate issue
No UI surface for retry when seal defers	Low	All three call sites	Pre-existing UX gap — worth a separate issue
Empty-description metadata channel	Low	edit_room_modal.rs:407-410	Accepted per River's threat model

All findings either fixed or rationally deferred.

---

Big Picture Assessment

Goal Alignment: Yes — closes the documented #299 residual from PR #297. Scope is appropriate (covers the two analogous adjacent paths with identical bug shape). No dead-code removal or test weakening.

Anti-Patterns Detected: None.

Removed Code Concerns: None — only the inlined buggy match blocks and the captured-at-mount snapshot were removed; both were the bug.

---

Documentation

• Helper doc on seal_for_room: tightened to state the caller contract ("treat None as skip publish") explicitly.
• ecies.rs:20 bullet list rewritten to match the new implementation.
• Optional: a bullet under AGENTS.md "Private Room Support" noting seal_for_room as the canonical helper — not done; the docstring on the helper carries the same information.

---

Recommendations

Must Fix (Blocking)

None.

Should Fix (Important)

All previously-flagged Should Fix items are addressed in commits 428136b5 and 25716d8f.

Consider (Suggestions / Follow-ups)

1. #318 — the public→private mid-edit race for member_info. Filed.
2. temp_nickname prop refresh — pre-existing UX issue; sync round can land a newer nickname under an open modal and a save would publish the older value. Worth filing.
3. Retry UI surface — no user-visible signal when the seal helper defers a private-room edit. Currently logs warn! only.
4. Defense-in-depth in common — extending MemberInfoV1::apply_delta with the same privacy-mode guard that Configuration::apply_delta already has would make the entire class of #299-shaped leaks impossible regardless of UI surface. Contract change → requires delegate WASM migration.

---

Verdict

State: Ready to Merge.
HEAD SHA reviewed: 25716d8f

Two prior review rounds: round-1 surfaced two helper-level items (match ordering + doc/code mismatch) and a P3 pin-robustness concern; both addressed. Round-2 re-review (skeptical + Codex) on 428136b5 returned "Ready to Merge" from skeptical and one residual P3 from Codex (pin could be satisfied by a comment) — fixed in 25716d8f by stripping line comments before the assertion. Test suite passes. CI must be green on the current HEAD before merge.

[AI-assisted - Claude]