feat(ui): carry private-room secrets in the invitation#301
Conversation
A newly-invited member of a private room currently cannot read the room until the owner's chat-delegate comes online and back-fills an owner-signed `encrypted_secrets` blob into the contract (Bug #6 / #276). This also fails for non-owner member invites — the inviting member holds the secret but cannot mint an owner-signed blob. Embed the room's symmetric secrets directly in the `Invitation` artifact (`Invitation::room_secrets`). The invitee seeds them into `RoomData` on accept and can decrypt the room — and seal their own nickname — at join time, with zero realtime involvement from the owner's delegate. The room contract and chat-delegate are untouched; the `encrypted_secrets` back-fill stays as eventual-consistency durability. Secrets are carried in plaintext, not ECIES-wrapped: the invitation already carries `invitee_signing_key` in the clear, so it is already a bearer credential — wrapping adds no confidentiality. Plaintext also avoids decrypting attacker-influenced ciphertext on the join path (`river_core::ecies::decrypt` panics on a malformed blob and the release build is `panic = "abort"`). Persistence: a new `#[serde(default)]` `RoomData::invitation_secrets` field rides inside the existing `rooms_data` delegate blob so the secret survives a refresh; `repopulate_secrets_from_state` folds it into the in-memory `secrets` map. A contract `encrypted_secrets` blob wins on version collision. UI-only change — no contract or delegate WASM change, no migration entry. Tests: 9 new unit tests covering invitation CBOR round-trip + backward compatibility, secret folding, contract-blob precedence, and the seal-invitee-nickname fallback. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…act Debug
Multi-model review (skeptical + Codex) found two blocking issues:
H1 — `app.rs:197` logs the whole `Invitation` with `{:?}`. With `room_secrets`
added, the derived `Debug` for `[u8; 32]` prints every room-secret byte to the
browser console (`SigningKey`'s own `Debug` is non-exhaustive, so this was a
new leak). Fix: hand-written `Debug` for `Invitation` and `PendingRoomJoin`
that redacts `room_secrets`.
H2 — cross-call ordering: a stale/garbage invitation secret folded before the
owner's `encrypted_secrets` back-fill arrived would permanently shadow the
authentic blob — `repopulate_secrets_from_state`'s `!contains_key` filter
skipped re-decrypting it. Fix: the contract-blob loop now always decrypts and
overwrites (the owner-signed secret is authoritative) and prunes the
superseded entry from `invitation_secrets`. Regression test added.
Also:
- Drop invitation secrets for versions beyond the room's current version, so
a malicious inviter cannot pad the invitation with bogus future versions.
- 5 more tests: minimal-legacy-blob decode (backward compat), refresh
serde round-trip durability, `seal_invitee_nickname` version-mismatch and
contract-precedence, and a source-grep pin for the `seal_invitee_nickname`
call site.
- Doc updates: `repopulate_secrets_from_state` doc-comment; AGENTS.md
(`Invitation` lives in `ui/` not `common/`; Private Room Support and the
#251 note now mention the invitation-secret distribution channel).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sanity
left a comment
There was a problem hiding this comment.
Comprehensive PR Review: #301 — re-review after fixes
Summary
- PR Title: feat(ui): carry private-room secrets in the invitation
- Type: feat
- CI Status:
check-delegate-migration✅,check-room-contract-migration✅,license/cla✅;build+ui-playwright-testspending at review time - Linked Issues: addresses Bug #6 / PR #276 (no
Closes #) - Review tier: Full — touches cryptography / secret handling, wire format (
InvitationCBOR), and persistence (RoomDatadelegate blob) - Reviewers run: Round 1 (HEAD
7a057add) — code-first, testing, skeptical, big-picture + Codex (gpt-5.5). Round 2 re-review (HEAD67d016d8, after blocking fixes) — skeptical + Codex; the code-first / testing / big-picture findings were doc/test items, verified resolved by direct diff inspection.
Code-First Analysis
Alignment: matches. The end-to-end path is complete and verified — inviter (invite_member_modal.rs, invite_via_dm_picker_modal.rs) → Invitation::room_secrets → PendingRoomJoin → get_response.rs pending-invite branch → RoomData::invitation_secrets → repopulate_secrets_from_state fold → in-memory secrets. Non-owner-issued invitations are handled (reads room_data.secrets, which any member holds). No discrepancy between code and the PR description.
Testing Assessment
Coverage: adequate. 235 river-ui unit tests pass (15 new across the two commits).
| Test Type | Status | Notes |
|---|---|---|
| Unit | ✅ | Invitation CBOR round-trip + deterministic encoding + legacy-decode; collect_invitation_secrets; the H2 cross-call regression; RoomData minimal-legacy-blob decode + refresh serde round-trip; seal_invitee_nickname (fallback / version-mismatch / contract-precedence); seal_invitee_nickname call-site source-grep pin |
| Integration | N/A | UI-only change |
| E2E | The owner-offline non-owner-invite scenario is not in cargo test; recommended as a Playwright-MCP manual check before merge |
Skeptical Findings
Round 1 found two blocking issues; both independently corroborated and now fixed:
| Concern | Severity | Status |
|---|---|---|
H1 — app.rs:197 info!("{:?}", invitation) leaked plaintext room_secrets to the browser console (derived Debug for [u8;32] is transparent; SigningKey's own Debug is not, so this was a new leak) |
High | Resolved — hand-written redacting Debug for Invitation (members.rs) and PendingRoomJoin (invites.rs); round-2 skeptical confirmed no remaining transparent {:?} sink |
H2 — cross-call ordering: a stale/garbage invitation secret folded before the owner's encrypted_secrets back-fill permanently shadowed the authentic blob (the !contains_key filter skipped re-decrypting it) |
High | Resolved — the contract-blob loop in repopulate_secrets_from_state now always decrypts and overwrites (owner-signed secret is authoritative) and prunes the superseded invitation_secrets entry; regression test added; round-2 skeptical traced the multi-call sequence and confirmed correctness |
Round-2 re-review found no new blocking findings. Borrow/ownership in the new loop, the version filter (<= current_version, no off-by-one), and seal_invitee_nickname precedence were all verified safe.
Big Picture Assessment
Goal alignment: yes. Diff is tightly scoped (UI-only; every non-core line is a mandatory struct-literal update). No removed tests/code, no anti-patterns. Migration claim verified — no common/, delegates/, contracts/, or Cargo.{toml,lock} changes; check-delegate-migration and check-room-contract-migration both green. AGENTS.md staleness (the Invitation-in-common/ line, the Private Room Support section, the #251 note) was corrected in the fix commit.
Documentation
- Code docs: complete — new public items documented;
repopulate_secrets_from_statedoc-comment updated for the fold. - Architecture docs: AGENTS.md updated (Private Room Support, #251 note,
common/types list).
Recommendations
Must Fix (Blocking)
None outstanding — H1 and H2 resolved and re-verified.
Should Fix (Important)
None outstanding.
Consider (non-blocking)
- CLI invitation mirror (known, out of scope). Codex re-flagged that
cli/src/api.rs's separateInvitationstruct was not givenroom_secrets. This is deliberate and documented in the PR's "Out of scope" section: the CLI cannot decrypt private rooms at all today (it seals nicknames asSealedBytes::public), so a CLI invitee of a private room already gets degraded behavior; the wire format stays compatible via CBOR#[serde(default)]. Full CLI private-room support is a separate feature — recommend a follow-up issue rather than expanding this PR. localStoragecarries the plaintext invitation (skeptical, informational).save_invitation_to_storagepersists the wholeInvitation(now withroom_secrets) tolocalStorage. Not a new exposure class — the invitation already carriesinvitee_signing_key(a private key) in cleartext, solocalStoragealready held a full bearer credential. Consistent with the PR's stated threat model.- E2E verification before merge — run the Playwright-MCP flow: a non-owner member invites someone with the owner's node stopped; the invitee should see decrypted history immediately and survive a refresh.
Verdict
State: Ready to Merge — pending build + ui-playwright-tests going green on the current HEAD.
HEAD SHA reviewed: 67d016d8c75aff7629cffce253e92e32e78d6f01
Re-review after fixes for round-1 findings H1 and H2 — both resolved and confirmed by an independent round-2 adversarial pass (skeptical reviewer + Codex). No new blocking findings. The remaining items are a documented out-of-scope CLI follow-up and a recommended manual E2E check.
[AI-assisted - Claude]
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* feat(cli): carry private-room secrets in riverctl invitations CLI follow-up to PR #301 (UI's `Invitation::room_secrets`), closing #302. Before this change, the riverctl `Invitation` struct had only `room`, `invitee_signing_key`, and `invitee` — no `room_secrets`. CBOR tolerated the missing field (`#[serde(default)]` on the UI side), so old/new invitations still decoded, but: - A UI-created private-room invitation accepted by riverctl silently dropped `room_secrets` and the CLI invitee was stuck on `[Encrypted: N bytes, vN]` until the room owner's chat-delegate back-filled an `encrypted_secrets` blob. - A CLI-created private-room invitation carried no `room_secrets` at all, so the invitee never gained access. This PR adds real private-room support to riverctl: 1. **Wire compatibility.** `Invitation::room_secrets: Vec<(u32, [u8; 32])>` with `#[serde(default)]`, byte-identical to UI's `ui::components::members::Invitation`. Hand-written `Debug` mirrors the UI's PR-#301 redaction so `{:?}`-logging an invitation does not print secret bytes (tests pin both the redaction text AND the absence of the 32-byte array form). 2. **Inviter side (`create_invitation`).** New `cli::private_room` module collects every secret the CLI holds for the room: - Owner: derives via `river_core::key_derivation::derive_room_secret` for every version present in `state.secrets.encrypted_secrets` (plus `current_version`), matching the deterministic convergence the UI's rotation path depends on. - Non-owner: decrypts the member's own `encrypted_secrets` blobs via `decrypt_secret_from_member_blob_raw`; folds in persisted `invitation_secrets` for versions the contract has not yet provided an owner-signed blob for. Owner-signed blob wins on collision — mirrors `RoomData::repopulate_secrets_from_state`. 3. **Invitee side (`accept_invitation`).** Replaces the unconditional `SealedBytes::public` nickname seal with the equivalent of the UI's `seal_invitee_nickname`: public-room → `SealedBytes::Public`; private room → `seal_bytes(nick, secret_for_current_version, version)`. When neither the contract blob nor the invitation carries a secret at `current_version`, member_info is DEFERRED rather than leaking a plaintext nickname into a private room (a `tracing::warn!` surfaces the deferral; full heal parity with UI's `build_member_info_heal` is left as a follow-up). 4. **Storage.** `StoredRoomInfo` grows a persisted `invitation_secrets: HashMap<u32, [u8; 32]>` (`#[serde(default)]` keeps old `rooms.json` files readable). Plaintext-on-disk threat model is identical to the existing `signing_key_bytes` field — protected by filesystem permissions and the user's full-disk encryption. The Invitation-struct duplication between UI and CLI (scope item 4 in #302) is NOT addressed in this PR — the WASM-migration constraint that kept the type out of `common/` for #301 still applies. The CLI helpers include doc-comments cross-referencing the UI counterparts to limit drift until a shared non-WASM crate exists. ## Testing - `invitation_cbor_round_trip_with_secrets` — CBOR encode/decode preserves `room_secrets` byte-for-byte (fingerprint stability for processed-invite dedup). - `invitation_cbor_decodes_legacy_invitation_without_secrets_field` — pre-#302 wire bytes still decode with empty `room_secrets`. - `invitation_with_empty_secrets_round_trips` — public-room invitations carry no secrets, encode/decode unchanged. - `invitation_debug_redacts_room_secrets` — `{:?}`-logging never prints the secret-byte array form. - `private_room::tests::*` — 7 tests covering: empty/public rooms, owner derivation determinism, invitation-secret folding, sorted output, public-room sealing, private-room sealing with the invitation secret, and the "no secret available → defer" branch. `cargo test -p riverctl --lib`: 33 passed (8 new, 25 pre-existing). `cargo test -p river-core --lib`: 173 passed (sanity — no common/ changes; no WASM/migration impact). Closes #302 [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(cli): align private-room helpers with UI naming and simplify - Rename CLI's secrets_to_invitation_vec to collect_invitation_secrets to match its UI counterpart in ui/src/components/members.rs. The doc comment already pointed at that name; the function name now matches. - Inline iterator chain replaces collect/push/sort/dedup in collect_secrets_for_room — the HashMap entry API dedups versions naturally, so the intermediate Vec and sort were dead work. - Import HashMap once in api.rs and drop the std::collections::HashMap qualifier on a local binding. No behavior change. All 33 riverctl --lib tests pass. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): drop derive_room_secret owner-path; add critical regression tests Address multi-model review of PR #303 (Codex P1 + testing/big-picture findings). Net effect: **P1 fix (Codex):** `collect_secrets_for_room` no longer special-cases the owner with `derive_room_secret(seed, owner_vk, v)`. The UI's room-creation path seeds v0 from `generate_room_secret()` — a random value, NOT from derivation. Deriving here would produce the wrong v0 for any room created via the UI and later inherited by a CLI owner; invitees would persist an unusable secret and seal their nickname under a key other members cannot decrypt. The owner addresses an `AuthorizedEncryptedSecretForMember` to themselves at every version, so the contract-blob-decrypt branch covers the owner case uniformly with non-owners. `is_owner` parameter dropped; tests that asserted the (wrong) derive behavior replaced. **Critical regression tests added** (testing-reviewer findings #1–#4): - `owner_recovers_random_v0_from_contract_blob_not_via_derivation` — pins the P1 fix above with a concrete `generate_room_secret()` v0. - `non_owner_decrypts_own_blob_from_state` — exercises the non-owner decrypt branch (previously untested) and asserts the non-owner does NOT recover the owner's secret from a blob addressed to the owner. - `contract_blob_wins_over_stale_invitation_secret_at_same_version` — pins the authority rule documented on `collect_secrets_for_room`. - `seal_invitee_nickname_returns_none_when_invitation_lacks_current_version` — rotation between invitation creation and accept (current_version=1, invitation carries v0 only) must defer rather than seal at the wrong version. - `accept_invitation_calls_seal_invitee_nickname` — source-grep pin so a refactor cannot silently revert to `SealedBytes::public` and leak a plaintext nickname into a private room. **Storage tests added**: - `invitation_secrets_round_trip_via_disk` — pins serde persistence through a fresh `Storage` reload. - `rooms_json_decodes_legacy_blob_without_invitation_secrets_field` — pre-#302 `rooms.json` shape (no `invitation_secrets` key) must load with the new field defaulting to empty. **Doc + comment fixes**: - Self-referential "filed as follow-up #303 for CLI parity" in `accept_invitation` now correctly references #304 (newly-filed CLI heal counterpart). - `StoredRoomInfo::invitation_secrets` doc spells out what the field is currently used for (forwarding only — no CLI message-decrypt yet) and the missing prune path (storage waste — `#304` heal hook). - `AGENTS.md` "Private Room Support" section gains a CLI-parity bullet pointing at `cli::private_room` + `StoredRoomInfo`, the no-derive-for-owners rationale, and the missing heal. - `collect_secrets_for_room` now `warn!`s on decrypt failure instead of silently swallowing the `Err` (big-picture reviewer note). - `create_invitation` gains a comment about using the LOCAL state snapshot (not a fresh GET) — pre-existing behavior, but now more visible since the invitation carries `room_secrets`. **Follow-ups filed**: #304 (CLI build_member_info_heal counterpart) and #305 (cross-side fixed-vector wire format test). Tests: 39 passed (was 33 — six new), 0 failed. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cli): merge invitation_secrets on re-accept instead of overwriting Round-2 skeptical-review finding H1 on PR #303. Previously, `accept_invitation` built `invitation_secrets_map` from ONLY the new invitation's `room_secrets`, then passed it wholesale to `add_room_with_invitation_secrets`, which `insert`s wholesale — silently dropping any pre-existing entries. A re-accept of an older invitation (carrying only v0) when the CLI had already accumulated {v0, v1, v2} from a prior accept would erase v1 and v2. The UI's equivalent path uses `extend()` on the existing map with an explicit "covers a re-accepted invite" comment. Mirror that here via a new `cli::private_room::merge_invitation_secrets` helper that pre- existing-wins for versions the new invitation doesn't carry, new-wins on collision. accept_invitation now passes the merged map through. The room state, signing_key, invite_chain, and self_authorized_member re-accept clobber behavior is pre-existing and out of scope for this PR — `invitation_secrets` is the new field, and the merge-vs-overwrite distinction is what this PR introduces. Tests added: - `merge_invitation_secrets_preserves_existing_entries_new_does_not_carry` - `merge_invitation_secrets_new_wins_on_collision` - `merge_invitation_secrets_empty_cases` Tests: 42 passed (was 39), 0 failed. Filed #306 for the related M2 finding (IdentityExport should carry invitation_secrets for non-owner imports). [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(cli): drop unused HashMap import Round-3 codex P2. The local `HashMap<u32, [u8; 32]>` annotation went away when the merge fix routed through `merge_invitation_secrets` (which returns HashMap, inferred). The two remaining HashMap uses in api.rs are fully-qualified as `std::collections::HashMap`, so the import is dead and `cargo make clippy` (with `-D warnings`) would reject it. Filed #307 (rooms.json file-locking) and #308 (wholesale re-accept clobber) as follow-ups for the two M-level skeptical-review round-3 findings — both pre-existing, neither in #303's scope. [AI-assisted - Claude] Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`common/src/ecies.rs:144` started failing to compile when building
chat-delegate (which enables `river-core/ecies`) on 2026-05-24:
error[E0599]: no method named `as_bytes` found for struct
`GenericArray<u8, UInt<...>>` in the current scope
A transitive dep enabled blake3 1.8.2's `digest` / `traits-preview`
feature, which adds an `impl digest::Digest for blake3::Hasher`.
With `use sha2::Digest` already in scope at the top of this file,
`hasher.finalize()` started resolving to the trait method
(returning `GenericArray<u8, U64>`) instead of the inherent
(returning `blake3::Hash`).
Force the inherent via UFCS — byte-identical compiled output to
all prior builds, no behavior change. Last green main was 1c3643e
on 2026-05-23; this restores buildability on top of the May-24
commits (#297, #301, #303) that caused the feature flip.
Pushed direct-to-main per Ian's explicit per-incident authorization
(main is wedged for everyone; the river-publish workflow is blocked
on this).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Problem
For River private rooms, a newly-invited member can become a member offline (the invitation pre-signs their
AuthorizedMember, the contract is permissionless) but cannot read the room until an owner-signedAuthorizedEncryptedSecretForMemberblob appears in the contract'sencrypted_secrets. Only the room owner's signing key can mint that blob, so the owner's chat-delegate (or owner's open UI) must come online after the join to back-fill it. If it never does, the invitee is stuck on[Encrypted message - secret vN not available](Bug #6 / #276, Ivvor 2026-05-17).This also breaks for non-owner member invites: any member can invite (the invitation chain), and the inviting member holds the room secret — but cannot mint an owner-signed blob.
Approach
Embed the room's symmetric secrets directly in the
Invitationartifact (Invitation::room_secrets). The inviting member — owner or not — already holds the secrets; the invitee seeds them intoRoomDataon accept and can decrypt the room (and seal their own nickname for the join PUT) immediately, with zero realtime involvement from the owner's delegate.The room contract and chat-delegate are untouched —
encrypted_secretsstays owner-only and its owner-delegate back-fill becomes pure eventual-consistency durability. This is a UI-only change: no contract/delegate WASM change, no migration entry.Plaintext, not ECIES-wrapped — a deliberate change from the originally-planned ECIES wrapping. The invitation already carries
invitee_signing_keyin the clear, so the whole artifact is a bearer credential; ECIES-wrapping the secret to the invitee's key adds no confidentiality (the decryption key sits in the same blob). Plaintext also avoids decrypting attacker-influenced ciphertext on the join path:river_core::ecies::decryptpanics on a malformed blob, and the release profile ispanic = "abort", so a malformed invitation would abort the whole UI. Making that path fallible would require changingcommon/(→ delegate WASM change → migration), which this PR specifically avoids.Persistence: a new
#[serde(default)]RoomData::invitation_secretsfield rides inside the existingrooms_datadelegate blob, so the secret survives a page refresh (RoomData::secretsitself is#[serde(skip)]).repopulate_secrets_from_statefolds it into the in-memorysecretsmap on every ingestion. A contractencrypted_secretsblob is authoritative and wins on version collision. Plaintext-on-disk is consistent with the existingself_sk/self_nicknamealready in the persistedRoomData.Prune safety needs no contract change: the invitee's join PUT carries a
join_eventthey authored, sopost_apply_cleanupkeeps them as a message-author regardless of anyencrypted_secretsblob.Security note
Embedding the secret means an interceptor of the invitation link can passively decrypt the room without joining. This is a change in degree only — the invitation already carries a full signing key, so it was always a bearer credential. Mitigation is unchanged: treat invitations as single-use, short-lived, delivered over a secure channel.
Out of scope
riverctlis not modified. CBOR's tolerance of a missing#[serde(default)]field keeps old/new invitations interoperable without touchingcli/. The CLI cannot decrypt private rooms at all today; full CLI private-room support is a separate follow-up.Testing
cargo test -p river-ui --bins— 229 pass, including 9 new tests:InvitationCBOR round-trip preservesroom_secrets; deterministic encoding; legacy (pre-field) invitation decodes to empty (backward compat).collect_invitation_secretssorted / empty.repopulate_secrets_from_statefoldsinvitation_secrets; contract blob wins on collision.seal_invitee_nicknameseals from the invitation secret as fallback; returnsNone(defers member_info) when no secret is available.cargo clippy -p river-ui --target wasm32-unknown-unknown --features no-sync— clean (pre-existing warnings only, none in changed files).cargo fmtapplied.Recommended pre-merge manual check (not yet run): the AGENTS.md Playwright-MCP flow — a non-owner member invites someone while the owner's node is stopped; the invitee should see decrypted history immediately and survive a refresh.
[AI-assisted - Claude]