From 3e5372d88e50d76122460abe210726e3b5c3e631 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Fri, 16 Oct 2026 09:11:06 -0500 Subject: [PATCH 1/2] test(contract): pin per-record signature checks at the ban cap; fix debug bans the same way #797 made enforce_user_ban_cap judge each ban record by its own signature, because a BanId hashes the signature alone and records sharing an id were credited with each other's verdicts. Its regression tests were held back until the fixed contract was published (live at 30000404). They cover the owner pool, first-of-pair, and the unknown-target live branch, each in two input orders and comparing whole records; all three fail on the code before the fix. riverctl's debug bans classifier read the same id-keyed verdicts; it now checks each record's own signature first. Display only. --- cli/CHANGELOG.md | 8 +++ cli/src/commands/debug.rs | 55 +++++++++++++++-- common/tests/mutual_rejection_test.rs | 89 +++++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 5 deletions(-) diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index ea7f2dbd6..8417344e0 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -2,6 +2,14 @@ All notable changes to riverctl will be documented in this file. +## [Unreleased] + +### Fixed +- `debug bans` judges each ban record by its own signature. A record that + copies another ban's signature was reported with that ban's status + (freenet/river#797 follow-up). Display only: the contract already drops + such records. + ## [0.2.22] - 2026-10-16 ### Changed diff --git a/cli/src/commands/debug.rs b/cli/src/commands/debug.rs index 9bf03b803..15fe153bd 100644 --- a/cli/src/commands/debug.rs +++ b/cli/src/commands/debug.rs @@ -4,7 +4,7 @@ use crate::output::OutputFormat; use anyhow::{anyhow, Result}; use clap::Subcommand; use ed25519_dalek::VerifyingKey; -use river_core::room_state::ban::AuthorizedUserBan; +use river_core::room_state::ban::{AuthorizedUserBan, BansV1}; use river_core::room_state::member::{AuthorizedMember, BanResolution, MemberId}; use river_core::room_state::{ChatRoomParametersV1, ChatRoomStateV1}; use serde::Serialize; @@ -160,8 +160,10 @@ struct BanInfo { /// /// - A ban naming the owner is `Inert` and permanently dead: the owner can /// never be a target. -/// - A ban whose signature was not verified against its issuer's key is -/// `Inert`. That covers a forgery (the check must come FIRST, since the +/// - A ban whose own signature does not verify against its issuer's key is +/// `Inert`. Each record is checked itself: `resolution` is keyed by `BanId`, +/// a hash of the signature alone, so a record copying another ban's +/// signature would otherwise read that ban's verdict. That covers a forgery (the check must come FIRST, since the /// authority check knows nothing about who signed: pinned by /// `ban_signed_by_someone_other_than_its_attributed_banner_is_inert`), an /// issuer whose record is held nowhere (reversible: if they rejoin with the @@ -176,8 +178,12 @@ fn classify_ban( resolution: &BanResolution, known: &HashMap, owner_id: MemberId, + owner_vk: &VerifyingKey, ) -> BanEnforcement { - if ban.ban.banned_user == owner_id || !resolution.verified.contains(&ban.id()) { + if ban.ban.banned_user == owner_id + || !BansV1::ban_signature_matches_current_key(ban, known, owner_id, owner_vk) + || !resolution.verified.contains(&ban.id()) + { return BanEnforcement::Inert; } if resolution.tier(&ban.id()) > BanResolution::INERT { @@ -220,7 +226,7 @@ fn collect_ban_infos(room_state: &ChatRoomStateV1, owner_vk: &VerifyingKey) -> V banned_user_id: ban.ban.banned_user.to_string(), banned_by_id: ban.banned_by.to_string(), banned_at_secs, - enforcement: classify_ban(ban, &resolution, &known, owner_id), + enforcement: classify_ban(ban, &resolution, &known, owner_id, owner_vk), } }) .collect() @@ -1116,6 +1122,45 @@ mod tests { assert_eq!(verdict(&state, &owner), BanEnforcement::Inert); } + #[test] + fn record_copying_a_genuine_bans_signature_is_inert() { + // A `BanId` hashes the signature alone, so a record that reuses a + // genuine ban's signature with a different target shares its id. The + // classifier must judge each record by its own signature, not read + // the genuine ban's verdict through the shared id. + let owner = key(1); + let alice = key(2); + let bob = key(3); + + let mut state = ChatRoomStateV1::default(); + push_member(&mut state, &owner, &owner, &alice); + push_member(&mut state, &owner, &owner, &bob); + push_ban(&mut state, &owner, &owner, &alice); + let genuine = state.bans.0[0].clone(); + let copy = AuthorizedUserBan::with_signature( + UserBan { + owner_member_id: id(&owner), + banned_at: genuine.ban.banned_at + std::time::Duration::from_secs(1), + banned_user: id(&bob), + }, + id(&owner), + genuine.signature, + ); + assert_eq!(copy.id(), genuine.id(), "precondition: the ids collide"); + state.bans.0.push(copy); + + let infos = collect_ban_infos(&state, &owner.verifying_key()); + let of = |target: &SigningKey| { + infos + .iter() + .find(|i| i.banned_user_id == id(target).to_string()) + .expect("ban listed") + .enforcement + }; + assert_eq!(of(&alice), BanEnforcement::Enforcing); + assert_eq!(of(&bob), BanEnforcement::Inert); + } + #[test] fn ban_naming_the_room_owner_is_inert_not_undetermined() { // `is_ban_authorized` denies `target == owner_id` outright, before any diff --git a/common/tests/mutual_rejection_test.rs b/common/tests/mutual_rejection_test.rs index 25beac922..0f389e960 100644 --- a/common/tests/mutual_rejection_test.rs +++ b/common/tests/mutual_rejection_test.rs @@ -3152,3 +3152,92 @@ fn evidence_member_info_is_kept_up_to_the_first_present_ancestor() { } } } + +// --- freenet/river#797: each ban record is judged by its own signature --- +// +// A `BanId` hashes the signature alone, and records that share an id but +// differ in content are all stored. `enforce_user_ban_cap` must not credit a +// record with the verdict `resolve_bans` reached for another record of the +// same id: a record whose own signature does not verify against its claimed +// issuer is dead. Each case is run in two input orders and compares whole +// records, since the forged record and the genuine one share an id. +type BanRecord = (MemberId, std::time::SystemTime, MemberId); +fn ban_record(b: &AuthorizedUserBan) -> BanRecord { + (b.banned_by, b.ban.banned_at, b.ban.banned_user) +} +fn copy_signature( + sig_of: &AuthorizedUserBan, + issuer: MemberId, + target: MemberId, + at: u64, + owner: MemberId, +) -> AuthorizedUserBan { + AuthorizedUserBan::with_signature( + UserBan { + owner_member_id: owner, + banned_at: std::time::UNIX_EPOCH + std::time::Duration::from_secs(1_800_000_000 + at), + banned_user: target, + }, + issuer, + sig_of.signature, + ) +} +fn cap_both_orders( + room: &Room, + s: &ChatRoomStateV1, + bans: Vec, + cap: usize, +) -> Vec { + let mut out = Vec::new(); + for rev in [false, true] { + let mut b = bans.clone(); + if rev { + b.reverse(); + } + BansV1::enforce_user_ban_cap( + &mut b, + cap, + &s.members, + &s.ban_evidence, + &s.member_info, + &room.params, + ); + out.push(b.iter().map(ban_record).collect::>()); + } + assert_eq!(out[0], out[1], "input order changed the result"); + out.pop().unwrap() +} +#[test] +fn a_record_copying_an_owner_bans_signature_is_dead_at_the_cap() { + let room = capped_room(20, 2); + let s = room.modded_state(&[], &[], vec![]); + let (x, y, z) = (room.person(), room.person(), room.person()); + let b1 = room.owner_ban(&x, 10); + let b2 = room.owner_ban(&y, 11); + let forged = copy_signature(&b1, room.owner_id, z.id, 99, room.owner_id); + let got = cap_both_orders(&room, &s, vec![b1.clone(), b2.clone(), forged], 2); + assert_eq!(got, vec![ban_record(&b1), ban_record(&b2)]); +} +#[test] +fn a_record_copying_a_bans_signature_cannot_take_its_pair() { + let room = capped_room(20, 1); + let a = room.person(); + let s = room.modded_state(&[&a], &[], vec![]); + let t = room.person(); + let g = room.ban(&a, &t, 20); + let forged = copy_signature(&g, a.id, t.id, 10, room.owner_id); + let got = cap_both_orders(&room, &s, vec![g.clone(), forged], 1); + assert_eq!(got, vec![ban_record(&g)]); +} +#[test] +fn a_record_copying_a_bans_signature_is_not_live_on_an_unknown_target() { + let room = capped_room(20, 2); + let a = room.person(); + let s = room.modded_state(&[&a], &[], vec![]); + let (t1, t2, t3) = (room.person(), room.person(), room.person()); + let g1 = room.ban(&a, &t1, 10); + let g2 = room.ban(&a, &t2, 11); + let forged = copy_signature(&g1, a.id, t3.id, 99, room.owner_id); + let got = cap_both_orders(&room, &s, vec![g1.clone(), g2.clone(), forged], 2); + assert_eq!(got, vec![ban_record(&g1), ban_record(&g2)]); +} From 46758ddc7e86868f444d11f76e9304170c9bfd18 Mon Sep 17 00:00:00 2001 From: Ian Clarke Date: Fri, 16 Oct 2026 09:16:30 -0500 Subject: [PATCH 2/2] test: pin each per-record guard at the ban cap on its own (#800 review) - The first-of-pair test gets an older competing live ban, so it fails if only the first-of-pair filter is removed. - New: a copy differing only in owner_member_id matches the genuine ban's first-of-pair rank, so only the per-record check in live() kills it; it fails if only that check is removed. - Record comparison includes owner_member_id. - Rewrap the classify_ban doc. --- cli/src/commands/debug.rs | 7 +++-- common/tests/mutual_rejection_test.rs | 42 ++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/cli/src/commands/debug.rs b/cli/src/commands/debug.rs index 15fe153bd..197b807ac 100644 --- a/cli/src/commands/debug.rs +++ b/cli/src/commands/debug.rs @@ -161,13 +161,14 @@ struct BanInfo { /// - A ban naming the owner is `Inert` and permanently dead: the owner can /// never be a target. /// - A ban whose own signature does not verify against its issuer's key is -/// `Inert`. Each record is checked itself: `resolution` is keyed by `BanId`, -/// a hash of the signature alone, so a record copying another ban's -/// signature would otherwise read that ban's verdict. That covers a forgery (the check must come FIRST, since the +/// `Inert`. That covers a forgery (the check must come FIRST, since the /// authority check knows nothing about who signed: pinned by /// `ban_signed_by_someone_other_than_its_attributed_banner_is_inert`), an /// issuer whose record is held nowhere (reversible: if they rejoin with the /// same key their bans verify again), and an issuer the owner has banned. +/// Each record is checked itself: `resolution` is keyed by `BanId`, a hash +/// of the signature alone, so a record copying another ban's signature +/// would otherwise read that ban's verdict. /// - A ban the resolution ranks owner, effective or mutual is `Enforcing`: /// it removes its target, a mutual-ban cycle's bans included. /// - Any other ban takes no effect today. With its target's record known it diff --git a/common/tests/mutual_rejection_test.rs b/common/tests/mutual_rejection_test.rs index 0f389e960..a52d0f2cc 100644 --- a/common/tests/mutual_rejection_test.rs +++ b/common/tests/mutual_rejection_test.rs @@ -3161,9 +3161,14 @@ fn evidence_member_info_is_kept_up_to_the_first_present_ancestor() { // same id: a record whose own signature does not verify against its claimed // issuer is dead. Each case is run in two input orders and compares whole // records, since the forged record and the genuine one share an id. -type BanRecord = (MemberId, std::time::SystemTime, MemberId); +type BanRecord = (MemberId, std::time::SystemTime, MemberId, MemberId); fn ban_record(b: &AuthorizedUserBan) -> BanRecord { - (b.banned_by, b.ban.banned_at, b.ban.banned_user) + ( + b.banned_by, + b.ban.banned_at, + b.ban.banned_user, + b.ban.owner_member_id, + ) } fn copy_signature( sig_of: &AuthorizedUserBan, @@ -3220,15 +3225,44 @@ fn a_record_copying_an_owner_bans_signature_is_dead_at_the_cap() { } #[test] fn a_record_copying_a_bans_signature_cannot_take_its_pair() { + // An older live ban H competes for the one slot, so the result depends on + // the first-of-pair filter alone: with it, the copy is dead and goes, + // then H (older than G) goes, leaving G. Without it, the copy takes G's + // pair, G and the copy both go as dead, and H is left. let room = capped_room(20, 1); let a = room.person(); let s = room.modded_state(&[&a], &[], vec![]); - let t = room.person(); + let (t, t2) = (room.person(), room.person()); let g = room.ban(&a, &t, 20); + let h = room.ban(&a, &t2, 15); let forged = copy_signature(&g, a.id, t.id, 10, room.owner_id); - let got = cap_both_orders(&room, &s, vec![g.clone(), forged], 1); + let got = cap_both_orders(&room, &s, vec![g.clone(), h, forged], 1); assert_eq!(got, vec![ban_record(&g)]); } + +/// A copy identical to a genuine ban in issuer, target and date, differing +/// only in `owner_member_id`, matches the genuine ban's first-of-pair rank +/// exactly. Only the per-record signature check in `live` makes it dead. +#[test] +fn a_record_differing_only_in_owner_member_id_is_dead_at_the_cap() { + let room = capped_room(20, 2); + let s = room.modded_state(&[], &[], vec![]); + let (x, y) = (room.person(), room.person()); + let b1 = room.owner_ban(&x, 10); + let b2 = room.owner_ban(&y, 11); + let forged = AuthorizedUserBan::with_signature( + UserBan { + owner_member_id: room.person().id, + banned_at: b1.ban.banned_at, + banned_user: x.id, + }, + room.owner_id, + b1.signature, + ); + let got = cap_both_orders(&room, &s, vec![b1.clone(), b2.clone(), forged], 2); + assert_eq!(got, vec![ban_record(&b1), ban_record(&b2)]); +} + #[test] fn a_record_copying_a_bans_signature_is_not_live_on_an_unknown_target() { let room = capped_room(20, 2);