fix: stop MemberInfo/message verify from permanently forking rooms

[sanity]

Problem

Production forensics (freenet-core#4861) found room contract instances stuck in a permanent two-fork oscillation: divergent states that reject each other's deltas/full-states forever. The confirmed error is MemberInfo exists for non-existent member: MemberId(...).

MemberInfoV1::verify (and, same bug class, MessagesV1::verify) hard-rejects the entire state whenever it carries an entry (a MemberInfo record, or a message) whose author/member is absent from the local members set. But apply_delta's unconditional retain-sweep and post_apply_cleanup step 4/4b already tolerate and prune exactly this case on every normal delta application — so the verify() path was strictly more conservative than the rest of the contract's own convergence logic, and that asymmetry is what makes a fork permanent: once fork A prunes a member that fork B still carries (with a MemberInfo record or message), every full-state PUT/resync between A and B is rejected outright by verify(), and the two forks can never reconcile.

Approach

Mirror the existing precedent for the identical situation in BansV1::verify_excluding_cap ("banning member not in current members list"): skip re-verifying the signature for an entry whose member isn't currently known (we can't recover their key to check it anyway, and the signature was already checked when the entry was created), instead of rejecting the whole state. The entry carries no authority for a non-current member (deputies_of, ban enforcement, and nickname rendering are only ever consulted for ids present in parent_state.members), and it self-heals: apply_delta + post_apply_cleanup prune it on the very next applied delta.

Applied the identical fix to MessagesV1::verify ("Message author not found"), since it's the same asymmetry against the same post_apply_cleanup step 4b sweep, and is equally low-risk and well-precedented.

Left DirectMessagesV1::verify's analogous "sender/recipient is not a current member" checks alone — DM tombstone/purge semantics make that a more delicate change, and it isn't what's implicated by the reported forensics; flagging it as a plausible follow-up rather than bundling it here.

This is a common/ change compiled into both the room-contract and chat-delegate WASM, so it re-keys both:

• legacy_delegates.toml / common/legacy_room_contracts.toml: migration entries added (old hashes recorded) so existing rooms/secrets survive the re-key.
• pointer-records.toml: re-signed against the new WASM (third-party integrators resolve this instead of pinning a key).
• ui/public/contracts/, cli/contracts/: WASMs resynced via cargo make sync-wasm.

Testing

• Updated the existing unit tests in member_info.rs / message.rs that previously asserted verify()rejects an orphaned entry — they now assert it's tolerated.
• Added orphaned_member_info_and_message_do_not_reject_full_state in common/src/room_state.rs: builds a full ChatRoomStateV1 carrying a MemberInfo record and a message for a member absent from members, asserts the top-level composedverify() (what validate_state actually calls) still succeeds, then applies a no-op delta and asserts post_apply_cleanup prunes both orphans and the healed state still verifies — demonstrating the self-healing convergence path this fix unblocks.
• cargo test -p river-core: all 211+ tests pass (incl. convergence_tests.rs, retention_proptest.rs, summary_determinism_test.rs).
• cargo make sync-wasm, check-migration, check-room-contract-migration, check-pointer-freshness, cargo test -p river-core --test migration_test --test room_contract_migration_test: all pass.
• cargo clippy -p river-core --all-targets: no new warnings introduced by this change.
• Not verified in this PR: a full local UI build (cargo check -p river-ui) — this environment's ui/assets/styles.css (gitignored, tailwind-generated) isn't built and tailwindcss/npm deps aren't installed, so that check fails on an unrelated missing asset (asset!("/assets/styles.css")) rather than anything in this diff. chat-delegate and room-contract, which depend on the same common crate, both build and their WASMs were regenerated successfully.

What I'd want a reviewer to check first

1. Whether skipping signature re-verification for an orphaned MemberInfo/message entry (rather than, say, still validating the signature shape/size while skipping only the "member exists" gate) is the right level of tolerance — I followed the BansV1 precedent exactly since it's the same "can't recover the key" situation.
2. Whether DirectMessagesV1::verify's analogous checks should be fixed in a follow-up now that this asymmetry is named, or left as-is given the tombstone/purge complexity.
3. The migration/pointer-signing bookkeeping (legacy_delegates.toml, common/legacy_room_contracts.toml, pointer-records.toml) — this re-keys both WASMs, so please double check the added registry entries look right before merge.

Closes#423

[AI-assisted - Claude]

[sanity]

Drive-by from the merge-law conformance work (#671) — one factual correction plus an argument I think strengthens this PR. Not a review, and I have no objection to the change.

Correction: the reasoning behind this PR included that apply_delta + post_apply_cleanup already prune orphans consistently within one pass. post_apply_cleanup is measurably not idempotent. Execution trace from live Official-room state: merge(state_07, state_01) returns 123 members, and running post_apply_cleanup on that result again drops it to 122 — the leftover member satisfies none of the five retention rules in the returned state. Its own doc comment (common/src/room_state.rs:100) declares idempotence a MUST, for exactly the reason that peers run it a variable number of times. Details and the trace are on #671.

Where that conclusion still holds: steps 3 and 4 prune members and member_info against the samerequired_ids (room_state.rs:326-334), so the two stay mutually consistent even on the non-idempotent pass. The residue is a member that should not be there together with its member_info — not an orphaned member_info. So I do not think #671's defect is the source of #423's orphans, and I would not claim it is.

The argument I would put in the PR description instead, because it does not depend on identifying the orphan's origin at all:

A full-state PUT bypasses post_apply_cleanup entirely — it reaches state through validate_state → verify only. So peers legitimately hold states that cleanup would never have produced. A verify strict enough to reject them turns "this peer holds slightly unusual state" into "this state is permanently rejected", which is the fork.

Framed that way this is not tolerating a bug, it is verify accepting what the PUT path can legitimately deliver — and it stays correct after #671 is fixed. The non-idempotence above is useful here only as proof that at least one such path demonstrably exists.

Mirroring BansV1::verify_excluding_cap looks like the right precedent to me.

[AI-assisted - Claude]

[sanity]

Converted to draft — the tolerant branch admits unsigned MemberInfo, and that is reachable

CI is green and the convergence fix works, but an adversarial review found a privilege-escalation path introduced by the tolerance itself. Flagging before merge rather than after. Nothing here is exploitable today: this is unmerged, and main still has the old hard-reject.

The change

Where a member is absent from parent_state.members, the old code errored; the new code accepts the record with no signature check at all (common/src/room_state/member_info.rs:359-378, and the same shape in message.rs:139-154). The stated justification is that such a record "carries no authority for a non-current member".

Why that holds only while the id stays absent

Absence is not an invariant. An AuthorizedMember is public and replayable, so a pruned member can be re-added by anyone — and once re-added, the record is no longer an orphan.

deputies_of is what confers ban authority, and it is read in three places (common/src/room_state/member.rs:378, :384, :393 — the last for every strict ancestor of the target). So the sequence is:

1. PUT a state where ancestor A is absent from members, carrying a forged MemberInfo { member_id: A, version: u32::MAX, deputies: [attacker] } with a garbage signature — now accepted.
2. A is later re-added, or the attacker replays A's public member record.
3. Field order applies members before member_info, so the retain sweep (member_info.rs:527) now keeps the record rather than pruning it, and canonical selects it on rank u32::MAX.
4. deputies_of(A) returns [attacker], granting ban authority over A's entire subtree.

Pre-PR this was unreachable, because verify rejected the state outright.

The cited precedent is real but the comment omits its second half

The code comment points at BansV1::verify_excluding_cap as doing "the identical" thing. It does skip the same signature check — but it compensates at enforcement: ban_signature_matches_current_key (ban.rs:216, enforced at ban.rs:268), whose own doc comment describes this exact re-add attack. MemberInfoV1 and MessagesV1 have no equivalent re-check, so the analogy imports the risk without the mitigation.

Also worth fixing while here

MemberInfoV1::verify caps deputies per record (member_info.rs:342) but never the record count — previously bounded implicitly by members, now unbounded. MessagesV1::verify has no count cap at all (max_recent_messages is enforced only in apply_delta, message.rs:346), so unsigned orphan messages arrive unbounded on a single full-state PUT.

Suggested direction

Keep the tolerance — the convergence fix is right and #423 is a real bug — but add the half the bans path has: an enforcement-time signature re-check before an orphaned record is allowed to confer anything, plus a record/message count cap on the verify path.

Unrelated, and already fixed on this branch

The four red checks were migration bookkeeping, now green: the room-contract registry pin (31 → 32 + new blake3), the riverctl version bump for check-wasm-sync, and the two delegate pins. The migration substance was correct throughout — legacy_delegates.toml records the old delegate code_hash, so existing room keys stay findable, and migration_test and check-migration both pass. See sanity/torvan#39 for the pattern.

Draft rather than closed: the work is nearly there, and a dispatcher with landing authority would otherwise merge it on green.

[AI-assisted - Claude]

[sanity]

One more instance of the same class, not covered here: DirectMessagesV1::verify hard-rejects on a non-member DM endpoint.

common/src/room_state/direct_messages.rs:881-893:

let sender_vk = resolve_member_vk(msg.message.sender, owner_id, parameters,&members_by_id).ok_or_else(|| format!("DM sender {:?} is not a current member", msg.message.sender))?;ifresolve_member_vk(msg.message.recipient, owner_id, parameters,&members_by_id).is_none(){returnErr(format!("DM recipient {:?} is not a current member", msg.message.recipient));}

One unresolvable endpoint rejects the entire state, exactly as MemberInfoV1::verify and MessagesV1::verify did before this PR. This diff touches member_info.rs and message.rs but not direct_messages.rs.

Note bans are correctly not enforced here — the module comment says so deliberately, so verify stays stable across ban churn. It is the membership check that has the hard-reject shape.

Why it is reachable rather than theoretical: a full-state PUT bypasses post_apply_cleanup entirely, which is the same gap that motivates this PR. Step 6's own comment says as much — "Without this, a fresh ban (or member-prune) would leave the DMs in state but break verify because the sender/recipient can no longer be resolved." That sentence is describing this hazard from the other side.

It is also directly on the path of the #671 fix, which will re-key the contract and therefore migrate every room's state through a PUT.

Your call whether to fold it into this PR (it is the same three-line shape and would share the re-key you are already paying for) or take it separately. Flagging rather than assuming — I have not touched your branch.

[AI-assisted - Claude]

[sanity]

🔴 Merge-blocking: this PR would publish riverctl 0.2.15 against a stale, immutable river-core

Found while reviewing #673, which has the identical defect. Flagging here because whichever merges first ships it.

cli/Cargo.toml:53 on d8ca9182:

river-core = { version = "=0.1.20", path = "../common", features = [… "migration"…] }

and workspace Cargo.toml:67 is still 0.1.20, while cli bumps 0.2.14 → 0.2.15 and the PR adds a V32 entry to common/legacy_room_contracts.toml.

river-core 0.1.20 is already on crates.io (2026-08-23) and immutable. A reviewer downloaded and inspected the published crate:

river-core-0.1.20/legacy_room_contracts.toml → 31 entries, last = V31 dd63bcc9…
grep -c e765339b… → 0

cargo publish rewrites the path dependency to the registry dependency. So riverctl 0.2.15 ships bundling the new room contract against a registry that omits the generation live on the network right now. A cargo install riverctl build derives the new key, probes back to V31 — dead since 2026-07-30 — and skips e765339b entirely. It could not find any existing room, the Official room included, until a UI client migrated that room forward.

It is silent by construction. .github/workflows/release-riverctl.yml:130 is "Publish river-core then riverctl (skip if already on crates.io)" — publish_if_needed river-core sees 0.1.20 present, prints "already on crates.io — skipping", exits 0, and publishes riverctl anyway. In-workspace builds resolve via path, so nothing in CI ever sees it, and check-cli-wasm only gates the cli version.

The fix

Bump all three together, as the V31 precedent commit 394c27dd (#572) did — workspace 0.1.18→0.1.19, cli 0.2.9→0.2.10, and the = pin. Here that is workspace 0.1.20 → 0.1.21 and the pin =0.1.20 → =0.1.21.

0.1.21 rather than 0.2.0: crates.io reverse-dependencies for river-core is total: 1 (riverctl, in-workspace), and 394c27dd already shipped a genuinely breaking public type change as a patch bump.

It has to be in the same commit as the artifacts. A river-core version bump re-keys the delegate on its own, so doing it afterwards invalidates pointer-records.toml, both legacy registries' recorded hashes, and the two WASM pins — the whole migration payload.

Why merge-blocking rather than publish-blocking

crates.io is the one irreversible step here. A wrong riverctl 0.2.15 can be yanked but not replaced, and yanking does nothing for anyone who already installed. Everything else in this change is recoverable via the append-only registry.

Also relevant to both PRs

• You and fix(contract): stop doomed DMs winning global-cap slots and breaking cleanup idempotence #673 both bump riverctl to 0.2.15. Whichever merges second needs 0.2.16, and a fresh river-core bump with it.
• Second to merge must also: keep the first's V32 entry and append a new one for the first PR's published hash (append-only — both generations may hold rooms), regenerate both WASMs via the co-build (-p room-contract alone yields a different key, because chat-delegate unifies river-core's ecies feature), re-sign both pointer records at version 3, and re-run review and CI on the rebased head.
• Corroboration in your favour: your rebuilt delegate hash is byte-identical to fix(contract): stop doomed DMs winning global-cap slots and breaking cleanup idempotence #673's, and your delegate pointer record is byte-identical too. Two unrelated branches independently signing the same delegate generation is good evidence the delegate move is toolchain drift rather than either fix.
• Heads-up unrelated to this PR: the delegate pointer record 6qF2H5JR… resolves to "Contract not found" on the live network — its committed v1 apparently never landed, so integrators fall back to a baked-in key. publish-pointer-records.sh reads back after writing; do not skip that step this time.

Nothing here is a criticism of the change itself — the migration payload is otherwise correct and was verified against the live network's signed pointer record.

[AI-assisted - Claude]

[sanity]

🔴 New blocker: riverctl 0.2.15 is already published — origin/main moved

origin/main advanced from 40ca5b05 to 73a6ef3f (#680, fix(cli): emit author_verifying_key on every message stream JSON event). That PR bumped riverctl to 0.2.15, and it is now on crates.io and immutable:

riverctl published: 0.2.11, 0.2.12, 0.2.13, 0.2.14, 0.2.15
origin/main cli/Cargo.toml: version = "0.2.15"

This PR also sets riverctl 0.2.15, so the merge result carries a version that is already taken.

Consequence — the same defect as the river-core one, from a different direction.publish_if_needed riverctl queries the index, sees 0.2.15, prints "already on crates.io — skipping", exits 0. riverctl is never republished. The CLI on crates.io stays the artifact #680 built, carrying the pre-fix room contract and a registry without the current generation. A cargo install riverctl build then cannot find the Official room. Green pipeline, wrong artifact.

Fix: bump riverctl to 0.2.16. river-core 0.1.21 is still unpublished and remains correct.

Then rebase onto 73a6ef3f and rebuild-and-compare rather than assuming the bump is hash-neutral. It should not reach the contract build — the WASM comes from room-contract + chat-delegate via river-core, not riverctl — but Cargo.lock changes, and this review has already established that a single comment line re-keys the room contract. If either hash moves, re-sign the pointer records.

Both #672 and #673 are affected, and both currently claim 0.2.15. Whichever merges second needs a further bump again, plus the registry append (not replace), regenerated artifacts via the co-build, and pointer records re-signed at the next version.

How this was found, since it is the interesting part

Not by checking this branch. A reviewer noticed origin/main had moved, then built the actual auto-merge result rather than the branch tip — parents 73a6ef3f + 8c833270, merge 008d3021. Git auto-merged Cargo.lock and cli/Cargo.toml, both of which carry release payload state. The merge itself was clean and reproduced both artifacts byte-identically with the =0.1.21 pin intact — but the version collision is only visible once you look at the merge, not the branch.

Verifying a branch tip does not verify what lands on main. Worth adding to the release checklist, alongside the fact that nothing in CI compares a committed artifact to a build of its source (#678) and that two check-pointer-freshness clauses never execute (#677).

[AI-assisted - Claude]