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..197b807ac 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,12 +160,15 @@ 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 +/// - A ban whose own signature does not verify against its issuer's key is /// `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 @@ -176,8 +179,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 +227,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 +1123,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..a52d0f2cc 100644 --- a/common/tests/mutual_rejection_test.rs +++ b/common/tests/mutual_rejection_test.rs @@ -3152,3 +3152,126 @@ 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, MemberId); +fn ban_record(b: &AuthorizedUserBan) -> BanRecord { + ( + b.banned_by, + b.ban.banned_at, + b.ban.banned_user, + b.ban.owner_member_id, + ) +} +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() { + // 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, 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(), 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); + 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)]); +}