From e524c0619dee8b83cc9dc2bff9f36e3047ce4ee5 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 11:47:09 -0400 Subject: [PATCH 01/49] somewhat minimal validation --- crates/valence/src/client/event.rs | 17 +- crates/valence/src/inventory.rs | 4 + crates/valence/src/inventory/validate.rs | 253 ++++++++++++++++++ .../src/packet/c2s/play/click_slot.rs | 2 + 4 files changed, 275 insertions(+), 1 deletion(-) create mode 100644 crates/valence/src/inventory/validate.rs diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index f9a4e167e..05569ead7 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -8,7 +8,7 @@ use bevy_ecs::schedule::ScheduleLabel; use bevy_ecs::system::{SystemParam, SystemState}; use glam::{DVec3, Vec3}; use paste::paste; -use tracing::warn; +use tracing::{debug, warn}; use uuid::Uuid; use valence_protocol::block_pos::BlockPos; use valence_protocol::ident::Ident; @@ -852,6 +852,21 @@ fn handle_one_packet( }); } C2sPlayPacket::ClickSlotC2s(p) => { + if !crate::inventory::validate_click_slot_impossible(&p, &q.inventory) { + debug!("client {:#?} invalid click slot packet: {:#?}", q.entity, p); + return Ok(true); + } + if !crate::inventory::validate_click_slot_item_duplication( + &p, + &q.inventory, + &q.cursor_item, + ) { + debug!( + "client {:#?} click slot packet tried to incorrectly modify items: {:#?}", + q.entity, p + ); + return Ok(true); + } if p.slot_idx < 0 { if let Some(stack) = q.cursor_item.0.take() { events.2.drop_item_stack.send(DropItemStack { diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index d1c0b6caa..471c2109d 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -49,6 +49,10 @@ use crate::component::GameMode; use crate::packet::WritePacket; use crate::prelude::FlushPacketsSet; +mod validate; + +pub(crate) use validate::*; + #[derive(Debug, Clone, Component)] pub struct Inventory { title: Text, diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs new file mode 100644 index 000000000..3f756a010 --- /dev/null +++ b/crates/valence/src/inventory/validate.rs @@ -0,0 +1,253 @@ +use valence_protocol::packet::c2s::play::click_slot::ClickMode; +use valence_protocol::packet::c2s::play::ClickSlotC2s; + +use super::Inventory; +use crate::prelude::CursorItem; + +/// Validates a click slot packet enforcing that all fields are valid. +pub(crate) fn validate_click_slot_impossible(packet: &ClickSlotC2s, inventory: &Inventory) -> bool { + match packet.mode { + ClickMode::Click => { + if !(0..=1).contains(&packet.button) { + return false; + } + + if !(0..=inventory.slot_count()).contains(&(packet.slot_idx as u16)) + && packet.slot_idx != -999 + { + return false; + } + } + ClickMode::ShiftClick => todo!(), + ClickMode::Hotbar => todo!(), + ClickMode::CreativeMiddleClick => todo!(), + ClickMode::DropKey => { + if !(0..=1).contains(&packet.button) { + return false; + } + + if packet.carried_item.is_some() { + return false; + } + } + ClickMode::Drag => todo!(), + ClickMode::DoubleClick => todo!(), + } + + true +} + +/// Validates a click slot packet, enforcing that items can't be duplicated, eg. +/// conservation of mass. +pub(crate) fn validate_click_slot_item_duplication( + packet: &ClickSlotC2s, + inventory: &Inventory, + cursor_item: &CursorItem, +) -> bool { + match packet.mode { + ClickMode::Click => { + if packet.slot_idx == -999 { + // Clicked outside the window, so the client is dropping an item + if cursor_item.0.is_none() { + // Nothing to drop + return false; + } + + if !packet.slots.is_empty() { + return false; + } + + // Clicked outside the window + match packet.button { + 0 => { + // drop entire stack + if packet.carried_item.is_none() { + // Dropping an item + return true; + } + } + 1 => { + // drop single item from stack + return match (&cursor_item.0, &packet.carried_item) { + (Some(server_item), Some(client_item)) => { + server_item.count() - 1 == client_item.count() + } + (Some(server_item), None) => server_item.count() == 1, + (None, _) => { + // can't possibly drop an item + false + } + }; + } + _ => { + // Invalid button + return false; + } + } + true + } else { + if packet.slots.len() != 1 { + return false; + } + + true + } + } + ClickMode::ShiftClick => todo!(), + ClickMode::Hotbar => matches!(packet.button, 0..=8 | 40), + ClickMode::CreativeMiddleClick => todo!(), + ClickMode::DropKey => matches!(packet.button, 0..=1), + ClickMode::Drag => todo!(), + ClickMode::DoubleClick => todo!(), + } +} + +#[cfg(test)] +mod test { + use valence_protocol::item::{ItemKind, ItemStack}; + use valence_protocol::packet::c2s::play::click_slot::Slot; + use valence_protocol::var_int::VarInt; + + use super::*; + use crate::prelude::InventoryKind; + + #[test] + fn click_filled_slot_with_empty_cursor_success() { + let mut inventory = Inventory::new(InventoryKind::Generic9x1); + inventory.set_slot(0, ItemStack::new(ItemKind::Diamond, 20, None)); + let cursor_item = CursorItem::default(); + let packet = ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 0, + slots: vec![Slot { idx: 0, item: None }], + carried_item: inventory.slot(0).cloned(), + }; + + assert!(validate_click_slot_impossible(&packet, &inventory)); + assert!(validate_click_slot_item_duplication( + &packet, + &inventory, + &cursor_item + )); + } + + #[test] + fn click_slot_with_filled_cursor_success() { + let inventory1 = Inventory::new(InventoryKind::Generic9x1); + let mut inventory2 = Inventory::new(InventoryKind::Generic9x1); + inventory2.set_slot(0, ItemStack::new(ItemKind::Diamond, 10, None)); + let cursor_item = CursorItem(Some(ItemStack::new(ItemKind::Diamond, 20, None))); + let packet1 = ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 0, + slots: vec![Slot { + idx: 0, + item: Some(ItemStack::new(ItemKind::Diamond, 20, None)), + }], + carried_item: None, + }; + let packet2 = ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 0, + slots: vec![Slot { + idx: 0, + item: Some(ItemStack::new(ItemKind::Diamond, 30, None)), + }], + carried_item: None, + }; + + assert!(validate_click_slot_impossible(&packet1, &inventory1)); + assert!(validate_click_slot_item_duplication( + &packet1, + &inventory1, + &cursor_item + )); + + assert!(validate_click_slot_impossible(&packet2, &inventory2)); + assert!(validate_click_slot_item_duplication( + &packet2, + &inventory2, + &cursor_item + )); + } + + #[test] + fn click_slot_with_filled_cursor_failure() { + let inventory1 = Inventory::new(InventoryKind::Generic9x1); + let mut inventory2 = Inventory::new(InventoryKind::Generic9x1); + inventory2.set_slot(0, ItemStack::new(ItemKind::Diamond, 10, None)); + let cursor_item = CursorItem(Some(ItemStack::new(ItemKind::Diamond, 20, None))); + let packet1 = ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 0, + slots: vec![Slot { + idx: 0, + item: Some(ItemStack::new(ItemKind::Diamond, 22, None)), + }], + carried_item: None, + }; + let packet2 = ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 0, + slots: vec![Slot { + idx: 0, + item: Some(ItemStack::new(ItemKind::Diamond, 32, None)), + }], + carried_item: None, + }; + let packet3 = ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 0, + slots: vec![ + Slot { + idx: 0, + item: Some(ItemStack::new(ItemKind::Diamond, 22, None)), + }, + Slot { + idx: 1, + item: Some(ItemStack::new(ItemKind::Diamond, 22, None)), + }, + ], + carried_item: None, + }; + + assert!(validate_click_slot_impossible(&packet1, &inventory1)); + assert!(!validate_click_slot_item_duplication( + &packet1, + &inventory1, + &cursor_item + )); + + assert!(validate_click_slot_impossible(&packet2, &inventory2)); + assert!(!validate_click_slot_item_duplication( + &packet2, + &inventory2, + &cursor_item + )); + + assert!(validate_click_slot_impossible(&packet3, &inventory1)); + assert!(!validate_click_slot_item_duplication( + &packet3, + &inventory1, + &cursor_item + )); + } +} diff --git a/crates/valence_protocol/src/packet/c2s/play/click_slot.rs b/crates/valence_protocol/src/packet/c2s/play/click_slot.rs index b69b09160..fe5de83fd 100644 --- a/crates/valence_protocol/src/packet/c2s/play/click_slot.rs +++ b/crates/valence_protocol/src/packet/c2s/play/click_slot.rs @@ -7,6 +7,8 @@ pub struct ClickSlotC2s { pub window_id: u8, pub state_id: VarInt, pub slot_idx: i16, + /// The button used to click the slot. An enum can't easily be used for this + /// because the meaning of this value depends on the mode. pub button: i8, pub mode: ClickMode, pub slots: Vec, From 11fbb4f438f7b4e506fdb903b6c561442f9d76b6 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 12:38:17 -0400 Subject: [PATCH 02/49] pass in open inventory to use to validate add some more checks --- crates/valence/src/client/event.rs | 26 ++++-- crates/valence/src/inventory/validate.rs | 106 ++++++++++++++++++----- 2 files changed, 103 insertions(+), 29 deletions(-) diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index 05569ead7..7e21cfe89 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -41,6 +41,7 @@ use crate::client::Client; use crate::component::{Look, OnGround, Ping, Position}; use crate::entity::{EntityAnimation, EntityKind, McEntity, TrackedData}; use crate::inventory::Inventory; +use crate::prelude::OpenInventory; #[derive(Clone, Debug)] pub struct QueryBlockNbt { @@ -676,6 +677,7 @@ pub(crate) struct EventLoopQuery { keepalive_state: &'static mut KeepaliveState, cursor_item: &'static mut CursorItem, inventory: &'static mut Inventory, + open_inventory: Option<&'static mut OpenInventory>, position: &'static mut Position, look: &'static mut Look, on_ground: &'static mut OnGround, @@ -687,10 +689,15 @@ pub(crate) struct EventLoopQuery { /// An exclusive system for running the event loop schedule. fn run_event_loop( world: &mut World, - state: &mut SystemState<(Query, ClientEvents, Commands)>, + state: &mut SystemState<( + Query, + ClientEvents, + Commands, + Query<&Inventory, Without>, + )>, mut clients_to_check: Local>, ) { - let (mut clients, mut events, mut commands) = state.get_mut(world); + let (mut clients, mut events, mut commands, mut inventories) = state.get_mut(world); update_all_event_buffers(&mut events); @@ -708,7 +715,7 @@ fn run_event_loop( q.client.dec.queue_bytes(bytes); - match handle_one_packet(&mut q, &mut events) { + match handle_one_packet(&mut q, &mut events, &mut inventories) { Ok(had_packet) => { if had_packet { // We decoded one packet, but there might be more. @@ -728,7 +735,7 @@ fn run_event_loop( while !clients_to_check.is_empty() { world.run_schedule(EventLoopSchedule); - let (mut clients, mut events, mut commands) = state.get_mut(world); + let (mut clients, mut events, mut commands, mut inventories) = state.get_mut(world); clients_to_check.retain(|&entity| { let Ok(mut q) = clients.get_mut(entity) else { @@ -736,7 +743,7 @@ fn run_event_loop( return false; }; - match handle_one_packet(&mut q, &mut events) { + match handle_one_packet(&mut q, &mut events, &mut inventories) { Ok(had_packet) => had_packet, Err(e) => { warn!("failed to dispatch events for client {:?}: {e:?}", q.entity); @@ -753,6 +760,7 @@ fn run_event_loop( fn handle_one_packet( q: &mut EventLoopQueryItem, events: &mut ClientEvents, + inventories: &mut Query<&Inventory, Without>, ) -> anyhow::Result { let Some(pkt) = q.client.dec.try_next_packet::()? else { // No packets to decode. @@ -852,13 +860,19 @@ fn handle_one_packet( }); } C2sPlayPacket::ClickSlotC2s(p) => { - if !crate::inventory::validate_click_slot_impossible(&p, &q.inventory) { + let open_inv = q + .open_inventory + .as_ref() + .map(|open| inventories.get(open.entity).ok()) + .flatten(); + if !crate::inventory::validate_click_slot_impossible(&p, &q.inventory, open_inv) { debug!("client {:#?} invalid click slot packet: {:#?}", q.entity, p); return Ok(true); } if !crate::inventory::validate_click_slot_item_duplication( &p, &q.inventory, + open_inv, &q.cursor_item, ) { debug!( diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 3f756a010..9d393a475 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -5,23 +5,40 @@ use super::Inventory; use crate::prelude::CursorItem; /// Validates a click slot packet enforcing that all fields are valid. -pub(crate) fn validate_click_slot_impossible(packet: &ClickSlotC2s, inventory: &Inventory) -> bool { + +pub(crate) fn validate_click_slot_impossible( + packet: &ClickSlotC2s, + player_inventory: &Inventory, + open_inventory: Option<&Inventory>, +) -> bool { + if (packet.window_id == 0) != open_inventory.is_none() { + return false; + } + + let max_slot = match open_inventory { + Some(inv) => inv.slot_count() + 36, + None => player_inventory.slot_count(), + }; + + if !packet + .slots + .iter() + .all(|s| (0..=max_slot).contains(&(s.idx as u16))) + { + return false; + } + match packet.mode { ClickMode::Click => { if !(0..=1).contains(&packet.button) { return false; } - if !(0..=inventory.slot_count()).contains(&(packet.slot_idx as u16)) - && packet.slot_idx != -999 - { + if !(0..=max_slot).contains(&(packet.slot_idx as u16)) && packet.slot_idx != -999 { return false; } } - ClickMode::ShiftClick => todo!(), - ClickMode::Hotbar => todo!(), - ClickMode::CreativeMiddleClick => todo!(), - ClickMode::DropKey => { + ClickMode::ShiftClick => { if !(0..=1).contains(&packet.button) { return false; } @@ -29,9 +46,18 @@ pub(crate) fn validate_click_slot_impossible(packet: &ClickSlotC2s, inventory: & if packet.carried_item.is_some() { return false; } + + if !(0..=max_slot).contains(&(packet.slot_idx as u16)) { + return false; + } + } + ClickMode::Hotbar => return matches!(packet.button, 0..=8 | 40), + ClickMode::CreativeMiddleClick => todo!(), + ClickMode::DropKey => { + return (0..=1).contains(&packet.button) && packet.carried_item.is_none() } ClickMode::Drag => todo!(), - ClickMode::DoubleClick => todo!(), + ClickMode::DoubleClick => return packet.button == 0, } true @@ -41,7 +67,8 @@ pub(crate) fn validate_click_slot_impossible(packet: &ClickSlotC2s, inventory: & /// conservation of mass. pub(crate) fn validate_click_slot_item_duplication( packet: &ClickSlotC2s, - inventory: &Inventory, + player_inventory: &Inventory, + open_inventory: Option<&Inventory>, cursor_item: &CursorItem, ) -> bool { match packet.mode { @@ -94,7 +121,7 @@ pub(crate) fn validate_click_slot_item_duplication( } } ClickMode::ShiftClick => todo!(), - ClickMode::Hotbar => matches!(packet.button, 0..=8 | 40), + ClickMode::Hotbar => todo!(), ClickMode::CreativeMiddleClick => todo!(), ClickMode::DropKey => matches!(packet.button, 0..=1), ClickMode::Drag => todo!(), @@ -113,6 +140,7 @@ mod test { #[test] fn click_filled_slot_with_empty_cursor_success() { + let player_inventory = Inventory::new(InventoryKind::Player); let mut inventory = Inventory::new(InventoryKind::Generic9x1); inventory.set_slot(0, ItemStack::new(ItemKind::Diamond, 20, None)); let cursor_item = CursorItem::default(); @@ -126,16 +154,22 @@ mod test { carried_item: inventory.slot(0).cloned(), }; - assert!(validate_click_slot_impossible(&packet, &inventory)); + assert!(validate_click_slot_impossible( + &packet, + &player_inventory, + Some(&inventory) + )); assert!(validate_click_slot_item_duplication( &packet, - &inventory, + &player_inventory, + Some(&inventory), &cursor_item )); } #[test] fn click_slot_with_filled_cursor_success() { + let player_inventory = Inventory::new(InventoryKind::Player); let inventory1 = Inventory::new(InventoryKind::Generic9x1); let mut inventory2 = Inventory::new(InventoryKind::Generic9x1); inventory2.set_slot(0, ItemStack::new(ItemKind::Diamond, 10, None)); @@ -165,23 +199,34 @@ mod test { carried_item: None, }; - assert!(validate_click_slot_impossible(&packet1, &inventory1)); + assert!(validate_click_slot_impossible( + &packet1, + &player_inventory, + Some(&inventory1), + )); assert!(validate_click_slot_item_duplication( &packet1, - &inventory1, + &player_inventory, + Some(&inventory1), &cursor_item )); - assert!(validate_click_slot_impossible(&packet2, &inventory2)); + assert!(validate_click_slot_impossible( + &packet2, + &player_inventory, + Some(&inventory2) + )); assert!(validate_click_slot_item_duplication( &packet2, - &inventory2, + &player_inventory, + Some(&inventory2), &cursor_item )); } #[test] fn click_slot_with_filled_cursor_failure() { + let player_inventory = Inventory::new(InventoryKind::Player); let inventory1 = Inventory::new(InventoryKind::Generic9x1); let mut inventory2 = Inventory::new(InventoryKind::Generic9x1); inventory2.set_slot(0, ItemStack::new(ItemKind::Diamond, 10, None)); @@ -229,24 +274,39 @@ mod test { carried_item: None, }; - assert!(validate_click_slot_impossible(&packet1, &inventory1)); + assert!(validate_click_slot_impossible( + &packet1, + &player_inventory, + Some(&inventory1), + )); assert!(!validate_click_slot_item_duplication( &packet1, - &inventory1, + &player_inventory, + Some(&inventory1), &cursor_item )); - assert!(validate_click_slot_impossible(&packet2, &inventory2)); + assert!(validate_click_slot_impossible( + &packet2, + &player_inventory, + Some(&inventory2) + )); assert!(!validate_click_slot_item_duplication( &packet2, - &inventory2, + &player_inventory, + Some(&inventory2), &cursor_item )); - assert!(validate_click_slot_impossible(&packet3, &inventory1)); + assert!(validate_click_slot_impossible( + &packet3, + &player_inventory, + Some(&inventory1) + )); assert!(!validate_click_slot_item_duplication( &packet3, - &inventory1, + &player_inventory, + Some(&inventory1), &cursor_item )); } From d91bc9f13ea570d9c39b17d25a83f283744babef Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 12:41:30 -0400 Subject: [PATCH 03/49] add more validations --- crates/valence/src/inventory/validate.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 9d393a475..f5bf25f11 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -5,7 +5,6 @@ use super::Inventory; use crate::prelude::CursorItem; /// Validates a click slot packet enforcing that all fields are valid. - pub(crate) fn validate_click_slot_impossible( packet: &ClickSlotC2s, player_inventory: &Inventory, @@ -52,11 +51,16 @@ pub(crate) fn validate_click_slot_impossible( } } ClickMode::Hotbar => return matches!(packet.button, 0..=8 | 40), - ClickMode::CreativeMiddleClick => todo!(), + ClickMode::CreativeMiddleClick => { + return packet.button == 2 && (0..=max_slot).contains(&(packet.slot_idx as u16)) + } ClickMode::DropKey => { return (0..=1).contains(&packet.button) && packet.carried_item.is_none() } - ClickMode::Drag => todo!(), + ClickMode::Drag => { + return matches!(packet.button, 0..=2 | 4..=6 | 8..=10) + && ((0..=max_slot).contains(&(packet.slot_idx as u16)) || packet.slot_idx == -999) + } ClickMode::DoubleClick => return packet.button == 0, } From 682e8df516f66c8307d762f0e9773aa4a0b5f6e2 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 13:04:16 -0400 Subject: [PATCH 04/49] more validations --- crates/valence/src/inventory/validate.rs | 55 ++++++++++++++++++++++-- 1 file changed, 52 insertions(+), 3 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index f5bf25f11..bbbfd6978 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -55,7 +55,9 @@ pub(crate) fn validate_click_slot_impossible( return packet.button == 2 && (0..=max_slot).contains(&(packet.slot_idx as u16)) } ClickMode::DropKey => { - return (0..=1).contains(&packet.button) && packet.carried_item.is_none() + return (0..=1).contains(&packet.button) + && packet.carried_item.is_none() + && (0..=max_slot).contains(&(packet.slot_idx as u16)) } ClickMode::Drag => { return matches!(packet.button, 0..=2 | 4..=6 | 8..=10) @@ -121,18 +123,65 @@ pub(crate) fn validate_click_slot_item_duplication( return false; } - true + let count_deltas = + calculate_net_item_delta(packet, player_inventory, open_inventory, cursor_item); + + return count_deltas == 0; } } ClickMode::ShiftClick => todo!(), ClickMode::Hotbar => todo!(), ClickMode::CreativeMiddleClick => todo!(), - ClickMode::DropKey => matches!(packet.button, 0..=1), + ClickMode::DropKey => { + let count_deltas = + calculate_net_item_delta(packet, player_inventory, open_inventory, cursor_item); + + return match packet.button { + 0 => count_deltas == -1, + 1 => count_deltas < 0, // TODO: grab stack amount from target slot + _ => unreachable!(), + }; + } ClickMode::Drag => todo!(), ClickMode::DoubleClick => todo!(), } } +fn calculate_net_item_delta( + packet: &ClickSlotC2s, + player_inventory: &Inventory, + open_inventory: Option<&Inventory>, + cursor_item: &CursorItem, +) -> i32 { + // impled by copilot, might be wrong + let mut net_item_delta: i32 = 0; + + for slot in &packet.slots { + let item = match open_inventory { + Some(inv) => inv.slot(slot.idx as u16), + None => player_inventory.slot(slot.idx as u16), + }; + + if let Some(item) = item { + net_item_delta -= item.count() as i32; + } + + if let Some(item) = &slot.item { + net_item_delta += item.count() as i32; + } + } + + if let Some(item) = &packet.carried_item { + net_item_delta -= item.count() as i32; + } + + if let Some(item) = &cursor_item.0 { + net_item_delta += item.count() as i32; + } + + net_item_delta +} + #[cfg(test)] mod test { use valence_protocol::item::{ItemKind, ItemStack}; From 7dde0124ace41cf007d863cc6b151359a995ab30 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 13:04:27 -0400 Subject: [PATCH 05/49] add InventoryWindow and InventoryWindowMut type --- crates/valence/src/inventory/validate.rs | 101 +++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index bbbfd6978..349558992 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -1,9 +1,105 @@ +use valence_protocol::item::ItemStack; use valence_protocol::packet::c2s::play::click_slot::ClickMode; use valence_protocol::packet::c2s::play::ClickSlotC2s; use super::Inventory; use crate::prelude::CursorItem; +/// Represents the inventory window that the player is currently viewing. +/// Handles dispatching reads to the correct inventory. +/// +/// This is a read-only version of [`InventoryWindowMut`]. +struct InventoryWindow<'a> { + player_inventory: &'a Inventory, + open_inventory: Option<&'a Inventory>, +} + +impl InventoryWindow<'_> { + #[track_caller] + pub fn slot(&self, idx: u16) -> Option<&ItemStack> { + if let Some(open_inv) = self.open_inventory.as_ref() { + if idx < open_inv.slot_count() { + return open_inv.slot(idx); + } else { + return self + .player_inventory + .slot(super::convert_to_player_slot_id(open_inv.kind(), idx)); + } + } else { + return self.player_inventory.slot(idx); + } + } + + #[track_caller] + pub fn slot_count(&self) -> u16 { + match self.open_inventory.as_ref() { + Some(inv) => inv.slot_count() + 36, + None => self.player_inventory.slot_count(), + } + } +} + +/// Represents the inventory window that the player is currently viewing. +/// Handles dispatching reads/writes to the correct inventory. +/// +/// This is a writable version of [`InventoryWindow`]. +struct InventoryWindowMut<'a> { + player_inventory: &'a mut Inventory, + open_inventory: Option<&'a mut Inventory>, +} + +impl InventoryWindowMut<'_> { + #[track_caller] + pub fn slot(&self, idx: u16) -> Option<&ItemStack> { + if let Some(open_inv) = self.open_inventory.as_ref() { + if idx < open_inv.slot_count() { + return open_inv.slot(idx); + } else { + return self + .player_inventory + .slot(super::convert_to_player_slot_id(open_inv.kind(), idx)); + } + } else { + return self.player_inventory.slot(idx); + } + } + + #[track_caller] + #[must_use] + pub fn replace_slot( + &mut self, + idx: u16, + item: impl Into>, + ) -> Option { + assert!(idx < self.slot_count(), "slot index of {idx} out of bounds"); + + if let Some(open_inv) = self.open_inventory.as_mut() { + if idx < open_inv.slot_count() { + return open_inv.replace_slot(idx, item); + } else { + return self + .player_inventory + .replace_slot(super::convert_to_player_slot_id(open_inv.kind(), idx), item); + } + } else { + return self.player_inventory.replace_slot(idx, item); + } + } + + #[track_caller] + #[inline] + pub fn set_slot(&mut self, idx: u16, item: impl Into>) { + let _ = self.replace_slot(idx, item); + } + + pub fn slot_count(&self) -> u16 { + match self.open_inventory.as_ref() { + Some(inv) => inv.slot_count() + 36, + None => self.player_inventory.slot_count(), + } + } +} + /// Validates a click slot packet enforcing that all fields are valid. pub(crate) fn validate_click_slot_impossible( packet: &ClickSlotC2s, @@ -77,6 +173,11 @@ pub(crate) fn validate_click_slot_item_duplication( open_inventory: Option<&Inventory>, cursor_item: &CursorItem, ) -> bool { + let mut window = InventoryWindowMut { + player_inventory, + open_inventory, + }; + match packet.mode { ClickMode::Click => { if packet.slot_idx == -999 { From f092beaf934dba729beab3ca74ad2110b84b76f2 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 13:27:33 -0400 Subject: [PATCH 06/49] fix calculate_net_item_delta (i think?) --- crates/valence/src/inventory/validate.rs | 64 ++++++++++++------------ 1 file changed, 31 insertions(+), 33 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 349558992..79bfb977e 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -173,7 +173,7 @@ pub(crate) fn validate_click_slot_item_duplication( open_inventory: Option<&Inventory>, cursor_item: &CursorItem, ) -> bool { - let mut window = InventoryWindowMut { + let window = InventoryWindow { player_inventory, open_inventory, }; @@ -224,8 +224,7 @@ pub(crate) fn validate_click_slot_item_duplication( return false; } - let count_deltas = - calculate_net_item_delta(packet, player_inventory, open_inventory, cursor_item); + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); return count_deltas == 0; } @@ -234,12 +233,17 @@ pub(crate) fn validate_click_slot_item_duplication( ClickMode::Hotbar => todo!(), ClickMode::CreativeMiddleClick => todo!(), ClickMode::DropKey => { - let count_deltas = - calculate_net_item_delta(packet, player_inventory, open_inventory, cursor_item); + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); return match packet.button { 0 => count_deltas == -1, - 1 => count_deltas < 0, // TODO: grab stack amount from target slot + 1 => { + count_deltas + == window + .slot(packet.slot_idx as u16) + .map(|s| s.count() as i32) + .unwrap_or(0) + } _ => unreachable!(), }; } @@ -250,35 +254,29 @@ pub(crate) fn validate_click_slot_item_duplication( fn calculate_net_item_delta( packet: &ClickSlotC2s, - player_inventory: &Inventory, - open_inventory: Option<&Inventory>, + window: &InventoryWindow, cursor_item: &CursorItem, ) -> i32 { - // impled by copilot, might be wrong let mut net_item_delta: i32 = 0; for slot in &packet.slots { - let item = match open_inventory { - Some(inv) => inv.slot(slot.idx as u16), - None => player_inventory.slot(slot.idx as u16), + let old_slot = window.slot(slot.idx as u16); + let new_slot = slot.item.as_ref(); + + net_item_delta += match (old_slot, new_slot) { + (Some(old), Some(new)) => new.count() as i32 - old.count() as i32, + (Some(old), None) => -(old.count() as i32), + (None, Some(new)) => new.count() as i32, + (None, None) => 0, }; - - if let Some(item) = item { - net_item_delta -= item.count() as i32; - } - - if let Some(item) = &slot.item { - net_item_delta += item.count() as i32; - } - } - - if let Some(item) = &packet.carried_item { - net_item_delta -= item.count() as i32; } - if let Some(item) = &cursor_item.0 { - net_item_delta += item.count() as i32; - } + net_item_delta += match (cursor_item.0.as_ref(), packet.carried_item.as_ref()) { + (Some(old), Some(new)) => new.count() as i32 - old.count() as i32, + (Some(old), None) => -(old.count() as i32), + (None, Some(new)) => new.count() as i32, + (None, None) => 0, + }; net_item_delta } @@ -299,7 +297,7 @@ mod test { inventory.set_slot(0, ItemStack::new(ItemKind::Diamond, 20, None)); let cursor_item = CursorItem::default(); let packet = ClickSlotC2s { - window_id: 0, + window_id: 1, button: 0, mode: ClickMode::Click, state_id: VarInt(0), @@ -329,7 +327,7 @@ mod test { inventory2.set_slot(0, ItemStack::new(ItemKind::Diamond, 10, None)); let cursor_item = CursorItem(Some(ItemStack::new(ItemKind::Diamond, 20, None))); let packet1 = ClickSlotC2s { - window_id: 0, + window_id: 1, button: 0, mode: ClickMode::Click, state_id: VarInt(0), @@ -341,7 +339,7 @@ mod test { carried_item: None, }; let packet2 = ClickSlotC2s { - window_id: 0, + window_id: 1, button: 0, mode: ClickMode::Click, state_id: VarInt(0), @@ -386,7 +384,7 @@ mod test { inventory2.set_slot(0, ItemStack::new(ItemKind::Diamond, 10, None)); let cursor_item = CursorItem(Some(ItemStack::new(ItemKind::Diamond, 20, None))); let packet1 = ClickSlotC2s { - window_id: 0, + window_id: 1, button: 0, mode: ClickMode::Click, state_id: VarInt(0), @@ -398,7 +396,7 @@ mod test { carried_item: None, }; let packet2 = ClickSlotC2s { - window_id: 0, + window_id: 1, button: 0, mode: ClickMode::Click, state_id: VarInt(0), @@ -410,7 +408,7 @@ mod test { carried_item: None, }; let packet3 = ClickSlotC2s { - window_id: 0, + window_id: 1, button: 0, mode: ClickMode::Click, state_id: VarInt(0), From 470476b9946e3c250762becd89388c85411b845c Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 13:32:06 -0400 Subject: [PATCH 07/49] fix some unit tests --- crates/valence/src/inventory.rs | 12 +++++++++--- crates/valence/src/inventory/validate.rs | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index 471c2109d..cc65b4ce7 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -1346,7 +1346,7 @@ mod test { mod dropping_items { use valence_protocol::block_pos::BlockPos; - use valence_protocol::packet::c2s::play::click_slot::ClickMode; + use valence_protocol::packet::c2s::play::click_slot::{ClickMode, Slot}; use valence_protocol::packet::c2s::play::player_action::Action; use valence_protocol::types::Direction; @@ -1571,7 +1571,10 @@ mod test { button: 0, mode: ClickMode::DropKey, state_id: VarInt(state_id), - slots: vec![], + slots: vec![Slot { + idx: 40, + item: Some(ItemStack::new(ItemKind::IronIngot, 31, None)), + }], carried_item: None, }); @@ -1619,7 +1622,10 @@ mod test { button: 1, // pressing control mode: ClickMode::DropKey, state_id: VarInt(state_id), - slots: vec![], + slots: vec![Slot { + idx: 40, + item: None, + }], carried_item: None, }); diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 79bfb977e..6cb6129e1 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -239,7 +239,7 @@ pub(crate) fn validate_click_slot_item_duplication( 0 => count_deltas == -1, 1 => { count_deltas - == window + == -window .slot(packet.slot_idx as u16) .map(|s| s.count() as i32) .unwrap_or(0) From dabbe4093412834a9babf4d9b9a8e4d3c001f50a Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 13:33:53 -0400 Subject: [PATCH 08/49] fix more unit tests --- crates/valence/src/inventory.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index cc65b4ce7..d8211f53f 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -1102,6 +1102,11 @@ mod test { let mut app = App::new(); let (client_ent, mut client_helper) = scenario_single_client(&mut app); let inventory_ent = set_up_open_inventory(&mut app, client_ent); + let mut inventory = app + .world + .get_mut::(inventory_ent) + .expect("could not find inventory for client"); + inventory.set_slot(20, ItemStack::new(ItemKind::Diamond, 2, None)); // Process a tick to get past the "on join" logic. app.update(); From 5b7ab980571f1789fdb0d6e1723612ff87c22ec9 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Sun, 19 Mar 2023 13:48:28 -0400 Subject: [PATCH 09/49] more validations --- crates/valence/src/inventory/validate.rs | 83 ++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 6 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 6cb6129e1..f60ba60d8 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -225,13 +225,18 @@ pub(crate) fn validate_click_slot_item_duplication( } let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - return count_deltas == 0; } } - ClickMode::ShiftClick => todo!(), - ClickMode::Hotbar => todo!(), - ClickMode::CreativeMiddleClick => todo!(), + ClickMode::ShiftClick | ClickMode::Hotbar => { + if packet.slots.len() != 2 { + return false; + } + + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); + return count_deltas == 0; + } + ClickMode::CreativeMiddleClick => true, ClickMode::DropKey => { let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); @@ -247,8 +252,10 @@ pub(crate) fn validate_click_slot_item_duplication( _ => unreachable!(), }; } - ClickMode::Drag => todo!(), - ClickMode::DoubleClick => todo!(), + ClickMode::Drag | ClickMode::DoubleClick => { + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); + return count_deltas == 0; + } } } @@ -376,6 +383,70 @@ mod test { )); } + #[test] + fn click_filled_slot_with_filled_cursor_stack_overflow_success() { + let player_inventory = Inventory::new(InventoryKind::Player); + let mut inventory = Inventory::new(InventoryKind::Generic9x1); + inventory.set_slot(0, ItemStack::new(ItemKind::Diamond, 20, None)); + let cursor_item = CursorItem(Some(ItemStack::new(ItemKind::Diamond, 64, None))); + let packet = ClickSlotC2s { + window_id: 1, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 0, + slots: vec![Slot { + idx: 0, + item: Some(ItemStack::new(ItemKind::Diamond, 64, None)), + }], + carried_item: Some(ItemStack::new(ItemKind::Diamond, 20, None)), + }; + + assert!(validate_click_slot_impossible( + &packet, + &player_inventory, + Some(&inventory), + )); + assert!(validate_click_slot_item_duplication( + &packet, + &player_inventory, + Some(&inventory), + &cursor_item + )); + } + + #[test] + fn click_filled_slot_with_filled_cursor_different_item_success() { + let player_inventory = Inventory::new(InventoryKind::Player); + let mut inventory = Inventory::new(InventoryKind::Generic9x1); + inventory.set_slot(0, ItemStack::new(ItemKind::IronIngot, 2, None)); + let cursor_item = CursorItem(Some(ItemStack::new(ItemKind::Diamond, 2, None))); + let packet = ClickSlotC2s { + window_id: 1, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 0, + slots: vec![Slot { + idx: 0, + item: Some(ItemStack::new(ItemKind::Diamond, 2, None)), + }], + carried_item: Some(ItemStack::new(ItemKind::IronIngot, 2, None)), + }; + + assert!(validate_click_slot_impossible( + &packet, + &player_inventory, + Some(&inventory), + )); + assert!(validate_click_slot_item_duplication( + &packet, + &player_inventory, + Some(&inventory), + &cursor_item + )); + } + #[test] fn click_slot_with_filled_cursor_failure() { let player_inventory = Inventory::new(InventoryKind::Player); From 2a865570d0ad781b8f7869fde5c72bff5d820be9 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 07:57:00 -0400 Subject: [PATCH 10/49] add unit test for alchemy denial --- crates/valence/src/inventory/validate.rs | 79 ++++++++++++++++++++++++ 1 file changed, 79 insertions(+) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index f60ba60d8..495df7440 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -533,4 +533,83 @@ mod test { &cursor_item )); } + + #[test] + fn disallow_item_transmutation() { + // no alchemy allowed - make sure that lead can't be turned into gold + + let mut player_inventory = Inventory::new(InventoryKind::Player); + player_inventory.set_slot(9, ItemStack::new(ItemKind::Lead, 2, None)); + let cursor_item = CursorItem::default(); + + let packets = vec![ + ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::ShiftClick, + state_id: VarInt(0), + slot_idx: 9, + slots: vec![ + Slot { idx: 9, item: None }, + Slot { + idx: 36, + item: Some(ItemStack::new(ItemKind::GoldIngot, 2, None)), + }, + ], + carried_item: None, + }, + ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::Hotbar, + state_id: VarInt(0), + slot_idx: 9, + slots: vec![ + Slot { idx: 9, item: None }, + Slot { + idx: 36, + item: Some(ItemStack::new(ItemKind::GoldIngot, 2, None)), + }, + ], + carried_item: None, + }, + ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::Click, + state_id: VarInt(0), + slot_idx: 9, + slots: vec![Slot { idx: 9, item: None }], + carried_item: Some(ItemStack::new(ItemKind::GoldIngot, 2, None)), + }, + ClickSlotC2s { + window_id: 0, + button: 0, + mode: ClickMode::DropKey, + state_id: VarInt(0), + slot_idx: 9, + slots: vec![Slot { + idx: 9, + item: Some(ItemStack::new(ItemKind::GoldIngot, 1, None)), + }], + carried_item: None, + }, + ]; + + for (i, packet) in packets.iter().enumerate() { + assert!( + validate_click_slot_impossible(&packet, &player_inventory, None,), + "packet {i} failed validation" + ); + assert!( + !validate_click_slot_item_duplication( + &packet, + &player_inventory, + None, + &cursor_item + ), + "packet {i} passed item duplication check when it should have failed" + ); + } + } } From fa978d7357398e17fcb98740dfa3b6e919634fa8 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 08:44:32 -0400 Subject: [PATCH 11/49] add some sanity tests and a doc comment --- crates/valence/src/inventory/validate.rs | 81 ++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 495df7440..eee8e8123 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -259,6 +259,11 @@ pub(crate) fn validate_click_slot_item_duplication( } } +/// Calculate the total difference in item counts if the changes in this packet +/// were to be applied. +/// +/// Returns a positive number if items were added to the window, and a negative +/// number if items were removed from the window. fn calculate_net_item_delta( packet: &ClickSlotC2s, window: &InventoryWindow, @@ -297,6 +302,82 @@ mod test { use super::*; use crate::prelude::InventoryKind; + #[test] + fn net_item_delta_1() { + let drag_packet = ClickSlotC2s { + window_id: 2, + state_id: VarInt(14), + slot_idx: -999, + button: 2, + mode: ClickMode::Drag, + slots: vec![ + Slot { + idx: 4, + item: Some(ItemStack::new(ItemKind::Diamond, 21, None)), + }, + Slot { + idx: 3, + item: Some(ItemStack::new(ItemKind::Diamond, 21, None)), + }, + Slot { + idx: 5, + item: Some(ItemStack::new(ItemKind::Diamond, 21, None)), + }, + ], + carried_item: Some(ItemStack::new(ItemKind::Diamond, 1, None)), + }; + + let player_inventory = Inventory::new(InventoryKind::Player); + let inventory = Inventory::new(InventoryKind::Generic9x1); + let window = InventoryWindow::new(&player_inventory, Some(&inventory)); + let cursor_item = CursorItem(Some(ItemStack::new(ItemKind::Diamond, 64, None))); + + assert_eq!( + calculate_net_item_delta(&drag_packet, &window, &cursor_item), + 0 + ); + } + + #[test] + fn net_item_delta_2() { + let drag_packet = ClickSlotC2s { + window_id: 2, + state_id: VarInt(14), + slot_idx: -999, + button: 2, + mode: ClickMode::Click, + slots: vec![ + Slot { + idx: 2, + item: Some(ItemStack::new(ItemKind::Diamond, 2, None)), + }, + Slot { + idx: 3, + item: Some(ItemStack::new(ItemKind::IronIngot, 2, None)), + }, + Slot { + idx: 4, + item: Some(ItemStack::new(ItemKind::GoldIngot, 2, None)), + }, + Slot { + idx: 5, + item: Some(ItemStack::new(ItemKind::Emerald, 2, None)), + }, + ], + carried_item: Some(ItemStack::new(ItemKind::OakWood, 2, None)), + }; + + let player_inventory = Inventory::new(InventoryKind::Player); + let inventory = Inventory::new(InventoryKind::Generic9x1); + let window = InventoryWindow::new(&player_inventory, Some(&inventory)); + let cursor_item = CursorItem::default(); + + assert_eq!( + calculate_net_item_delta(&drag_packet, &window, &cursor_item), + 10 + ); + } + #[test] fn click_filled_slot_with_empty_cursor_success() { let player_inventory = Inventory::new(InventoryKind::Player); From 715d0067d5a0e35e279316754a65ff5b2d5cf096 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 09:40:53 -0400 Subject: [PATCH 12/49] fix validation for drag operations --- crates/valence/src/inventory/validate.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index eee8e8123..0fc9978da 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -14,7 +14,14 @@ struct InventoryWindow<'a> { open_inventory: Option<&'a Inventory>, } -impl InventoryWindow<'_> { +impl<'a> InventoryWindow<'a> { + pub fn new(player_inventory: &'a Inventory, open_inventory: Option<&'a Inventory>) -> Self { + Self { + player_inventory, + open_inventory, + } + } + #[track_caller] pub fn slot(&self, idx: u16) -> Option<&ItemStack> { if let Some(open_inv) = self.open_inventory.as_ref() { @@ -252,7 +259,14 @@ pub(crate) fn validate_click_slot_item_duplication( _ => unreachable!(), }; } - ClickMode::Drag | ClickMode::DoubleClick => { + ClickMode::Drag => { + if matches!(packet.button, 2 | 6 | 10) { + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); + return count_deltas == 0; + } + return packet.slots.is_empty() && packet.carried_item == cursor_item.0; + } + ClickMode::DoubleClick => { let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); return count_deltas == 0; } From feb01e7f0f60bcfe714fae40c7f20c15963de1f5 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 09:41:11 -0400 Subject: [PATCH 13/49] add unit test for handling dragging operations --- crates/valence/src/inventory.rs | 59 +++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index d8211f53f..beb172da5 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -831,6 +831,8 @@ impl From for InventoryKind { mod test { use bevy_app::App; use valence_protocol::item::ItemKind; + use valence_protocol::packet::c2s::play::click_slot::{ClickMode, Slot}; + use valence_protocol::packet::c2s::play::ClickSlotC2s; use valence_protocol::packet::S2cPlayPacket; use super::*; @@ -1653,4 +1655,61 @@ mod test { Ok(()) } } + + #[test] + fn dragging_items() { + let mut app = App::new(); + let (client_ent, mut client_helper) = scenario_single_client(&mut app); + app.world.get_mut::(client_ent).unwrap().0 = + Some(ItemStack::new(ItemKind::Diamond, 64, None)); + + // Process a tick to get past the "on join" logic. + app.update(); + client_helper.clear_sent(); + + let drag_packet = ClickSlotC2s { + window_id: 0, + state_id: VarInt(0), + slot_idx: -999, + button: 2, + mode: ClickMode::Drag, + slots: vec![ + Slot { + idx: 9, + item: Some(ItemStack::new(ItemKind::Diamond, 21, None)), + }, + Slot { + idx: 10, + item: Some(ItemStack::new(ItemKind::Diamond, 21, None)), + }, + Slot { + idx: 11, + item: Some(ItemStack::new(ItemKind::Diamond, 21, None)), + }, + ], + carried_item: Some(ItemStack::new(ItemKind::Diamond, 1, None)), + }; + client_helper.send(&drag_packet); + + app.update(); + + let cursor_item = app + .world + .get::(client_ent) + .expect("could not find client"); + assert_eq!( + cursor_item.0, + Some(ItemStack::new(ItemKind::Diamond, 1, None)) + ); + let inventory = app + .world + .get::(client_ent) + .expect("could not find inventory"); + for i in 9..12 { + assert_eq!( + inventory.slot(i), + Some(&ItemStack::new(ItemKind::Diamond, 21, None)) + ); + } + } } From 6f7e2ea5b37e7b13f7e5e35b5da856ebd71874cb Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 09:54:36 -0400 Subject: [PATCH 14/49] fix inventory desync on click and drag packets, fixes #270 --- crates/valence/src/client/event.rs | 2 +- crates/valence/src/inventory.rs | 16 +++++++++++----- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index 7e21cfe89..cfa6e84f7 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -881,7 +881,7 @@ fn handle_one_packet( ); return Ok(true); } - if p.slot_idx < 0 { + if p.slot_idx < 0 && p.mode == ClickMode::Click { if let Some(stack) = q.cursor_item.0.take() { events.2.drop_item_stack.send(DropItemStack { client: entity, diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index beb172da5..9fd413254 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -612,8 +612,6 @@ fn handle_click_container( continue; } - // TODO: do more validation on the click - cursor_item.set_if_neq(CursorItem(event.carried_item.clone())); inv_state.client_updated_cursor_item = true; @@ -1657,7 +1655,7 @@ mod test { } #[test] - fn dragging_items() { + fn dragging_items() -> anyhow::Result<()> { let mut app = App::new(); let (client_ent, mut client_helper) = scenario_single_client(&mut app); app.world.get_mut::(client_ent).unwrap().0 = @@ -1667,9 +1665,13 @@ mod test { app.update(); client_helper.clear_sent(); + let inv_state = app.world.get::(client_ent).unwrap(); + let window_id = inv_state.window_id; + let state_id = inv_state.state_id.0; + let drag_packet = ClickSlotC2s { - window_id: 0, - state_id: VarInt(0), + window_id, + state_id: VarInt(state_id), slot_idx: -999, button: 2, mode: ClickMode::Drag, @@ -1692,6 +1694,8 @@ mod test { client_helper.send(&drag_packet); app.update(); + let sent_packets = client_helper.collect_sent()?; + assert_eq!(sent_packets.len(), 0); let cursor_item = app .world @@ -1711,5 +1715,7 @@ mod test { Some(&ItemStack::new(ItemKind::Diamond, 21, None)) ); } + + Ok(()) } } From 806eb9f3809bd8460327785865c4cd6c62aeccc8 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 10:00:17 -0400 Subject: [PATCH 15/49] validate item stack amounts --- crates/valence/src/inventory/validate.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 0fc9978da..6861fad5a 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -122,10 +122,20 @@ pub(crate) fn validate_click_slot_impossible( None => player_inventory.slot_count(), }; + // check all slot ids and item counts are valid + if !packet.slots.iter().all(|s| { + (0..=max_slot).contains(&(s.idx as u16)) + && (0..=64).contains(&(s.item.as_ref().map(|i| i.count()).unwrap_or(0))) + }) { + return false; + } + + // check carried item count is valid if !packet - .slots - .iter() - .all(|s| (0..=max_slot).contains(&(s.idx as u16))) + .carried_item + .as_ref() + .map(|i| (0..=64).contains(&i.count())) + .unwrap_or(true) { return false; } From e1ceddb2e7d4da95970ad5a193c28cbf0c8ae032 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 10:08:45 -0400 Subject: [PATCH 16/49] move InventoryWindow helpers to inventory, make pub --- crates/valence/src/inventory.rs | 114 ++++++++++++++++++++++- crates/valence/src/inventory/validate.rs | 105 +-------------------- 2 files changed, 114 insertions(+), 105 deletions(-) diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index 9fd413254..72dcd10bb 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -307,6 +307,118 @@ impl OpenInventory { } } +/// A helper to represent the inventory window that the player is currently +/// viewing. Handles dispatching reads to the correct inventory. +/// +/// This is a read-only version of [`InventoryWindowMut`]. +pub struct InventoryWindow<'a> { + player_inventory: &'a Inventory, + open_inventory: Option<&'a Inventory>, +} + +impl<'a> InventoryWindow<'a> { + pub fn new(player_inventory: &'a Inventory, open_inventory: Option<&'a Inventory>) -> Self { + Self { + player_inventory, + open_inventory, + } + } + + #[track_caller] + pub fn slot(&self, idx: u16) -> Option<&ItemStack> { + if let Some(open_inv) = self.open_inventory.as_ref() { + if idx < open_inv.slot_count() { + return open_inv.slot(idx); + } else { + return self + .player_inventory + .slot(convert_to_player_slot_id(open_inv.kind(), idx)); + } + } else { + return self.player_inventory.slot(idx); + } + } + + #[track_caller] + pub fn slot_count(&self) -> u16 { + match self.open_inventory.as_ref() { + Some(inv) => inv.slot_count() + 36, + None => self.player_inventory.slot_count(), + } + } +} + +/// A helper to represent the inventory window that the player is currently +/// viewing. Handles dispatching reads/writes to the correct inventory. +/// +/// This is a writable version of [`InventoryWindow`]. +pub struct InventoryWindowMut<'a> { + player_inventory: &'a mut Inventory, + open_inventory: Option<&'a mut Inventory>, +} + +impl<'a> InventoryWindowMut<'a> { + pub fn new( + player_inventory: &'a mut Inventory, + open_inventory: Option<&'a mut Inventory>, + ) -> Self { + Self { + player_inventory, + open_inventory, + } + } + + #[track_caller] + pub fn slot(&self, idx: u16) -> Option<&ItemStack> { + if let Some(open_inv) = self.open_inventory.as_ref() { + if idx < open_inv.slot_count() { + return open_inv.slot(idx); + } else { + return self + .player_inventory + .slot(convert_to_player_slot_id(open_inv.kind(), idx)); + } + } else { + return self.player_inventory.slot(idx); + } + } + + #[track_caller] + #[must_use] + pub fn replace_slot( + &mut self, + idx: u16, + item: impl Into>, + ) -> Option { + assert!(idx < self.slot_count(), "slot index of {idx} out of bounds"); + + if let Some(open_inv) = self.open_inventory.as_mut() { + if idx < open_inv.slot_count() { + return open_inv.replace_slot(idx, item); + } else { + return self + .player_inventory + .replace_slot(convert_to_player_slot_id(open_inv.kind(), idx), item); + } + } else { + return self.player_inventory.replace_slot(idx, item); + } + } + + #[track_caller] + #[inline] + pub fn set_slot(&mut self, idx: u16, item: impl Into>) { + let _ = self.replace_slot(idx, item); + } + + pub fn slot_count(&self) -> u16 { + match self.open_inventory.as_ref() { + Some(inv) => inv.slot_count() + 36, + None => self.player_inventory.slot_count(), + } + } +} + pub(crate) struct InventoryPlugin; impl Plugin for InventoryPlugin { @@ -578,7 +690,7 @@ fn handle_click_container( continue; } - cursor_item.0 = event.carried_item.clone(); + cursor_item.set_if_neq(CursorItem(event.carried_item.clone())); for slot in event.slot_changes.clone() { if (0i16..target_inventory.slot_count() as i16).contains(&slot.idx) { diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 6861fad5a..d62056215 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -1,112 +1,9 @@ -use valence_protocol::item::ItemStack; use valence_protocol::packet::c2s::play::click_slot::ClickMode; use valence_protocol::packet::c2s::play::ClickSlotC2s; -use super::Inventory; +use super::{Inventory, InventoryWindow}; use crate::prelude::CursorItem; -/// Represents the inventory window that the player is currently viewing. -/// Handles dispatching reads to the correct inventory. -/// -/// This is a read-only version of [`InventoryWindowMut`]. -struct InventoryWindow<'a> { - player_inventory: &'a Inventory, - open_inventory: Option<&'a Inventory>, -} - -impl<'a> InventoryWindow<'a> { - pub fn new(player_inventory: &'a Inventory, open_inventory: Option<&'a Inventory>) -> Self { - Self { - player_inventory, - open_inventory, - } - } - - #[track_caller] - pub fn slot(&self, idx: u16) -> Option<&ItemStack> { - if let Some(open_inv) = self.open_inventory.as_ref() { - if idx < open_inv.slot_count() { - return open_inv.slot(idx); - } else { - return self - .player_inventory - .slot(super::convert_to_player_slot_id(open_inv.kind(), idx)); - } - } else { - return self.player_inventory.slot(idx); - } - } - - #[track_caller] - pub fn slot_count(&self) -> u16 { - match self.open_inventory.as_ref() { - Some(inv) => inv.slot_count() + 36, - None => self.player_inventory.slot_count(), - } - } -} - -/// Represents the inventory window that the player is currently viewing. -/// Handles dispatching reads/writes to the correct inventory. -/// -/// This is a writable version of [`InventoryWindow`]. -struct InventoryWindowMut<'a> { - player_inventory: &'a mut Inventory, - open_inventory: Option<&'a mut Inventory>, -} - -impl InventoryWindowMut<'_> { - #[track_caller] - pub fn slot(&self, idx: u16) -> Option<&ItemStack> { - if let Some(open_inv) = self.open_inventory.as_ref() { - if idx < open_inv.slot_count() { - return open_inv.slot(idx); - } else { - return self - .player_inventory - .slot(super::convert_to_player_slot_id(open_inv.kind(), idx)); - } - } else { - return self.player_inventory.slot(idx); - } - } - - #[track_caller] - #[must_use] - pub fn replace_slot( - &mut self, - idx: u16, - item: impl Into>, - ) -> Option { - assert!(idx < self.slot_count(), "slot index of {idx} out of bounds"); - - if let Some(open_inv) = self.open_inventory.as_mut() { - if idx < open_inv.slot_count() { - return open_inv.replace_slot(idx, item); - } else { - return self - .player_inventory - .replace_slot(super::convert_to_player_slot_id(open_inv.kind(), idx), item); - } - } else { - return self.player_inventory.replace_slot(idx, item); - } - } - - #[track_caller] - #[inline] - pub fn set_slot(&mut self, idx: u16, item: impl Into>) { - let _ = self.replace_slot(idx, item); - } - - pub fn slot_count(&self) -> u16 { - match self.open_inventory.as_ref() { - Some(inv) => inv.slot_count() + 36, - None => self.player_inventory.slot_count(), - } - } -} - /// Validates a click slot packet enforcing that all fields are valid. pub(crate) fn validate_click_slot_impossible( packet: &ClickSlotC2s, From bf6deee39dd3c5d6b22eb54972a56e8a135bf31b Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 10:17:06 -0400 Subject: [PATCH 17/49] add doc comments for inventory window helpers and add to prelude --- crates/valence/src/inventory.rs | 24 ++++++++++++++++++++++++ crates/valence/src/lib.rs | 4 +++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index 72dcd10bb..d3d717c7b 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -311,6 +311,18 @@ impl OpenInventory { /// viewing. Handles dispatching reads to the correct inventory. /// /// This is a read-only version of [`InventoryWindowMut`]. +/// +/// ``` +/// # use valence::prelude::*; +/// let mut player_inventory = Inventory::new(InventoryKind::Player); +/// player_inventory.set_slot(36, ItemStack::new(ItemKind::Diamond, 1, None)); +/// let target_inventory = Inventory::new(InventoryKind::Generic9x3); +/// let window = InventoryWindow::new(&player_inventory, Some(&target_inventory)); +/// assert_eq!( +/// window.slot(54), +/// Some(&ItemStack::new(ItemKind::Diamond, 1, None)) +/// ); +/// ``` pub struct InventoryWindow<'a> { player_inventory: &'a Inventory, open_inventory: Option<&'a Inventory>, @@ -352,6 +364,18 @@ impl<'a> InventoryWindow<'a> { /// viewing. Handles dispatching reads/writes to the correct inventory. /// /// This is a writable version of [`InventoryWindow`]. +/// +/// ``` +/// # use valence::prelude::*; +/// let mut player_inventory = Inventory::new(InventoryKind::Player); +/// let mut target_inventory = Inventory::new(InventoryKind::Generic9x3); +/// let mut window = InventoryWindowMut::new(&mut player_inventory, Some(&mut target_inventory)); +/// window.set_slot(54, ItemStack::new(ItemKind::Diamond, 1, None)); +/// assert_eq!( +/// player_inventory.slot(36), +/// Some(&ItemStack::new(ItemKind::Diamond, 1, None)) +/// ); +/// ``` pub struct InventoryWindowMut<'a> { player_inventory: &'a mut Inventory, open_inventory: Option<&'a mut Inventory>, diff --git a/crates/valence/src/lib.rs b/crates/valence/src/lib.rs index 8d56e0a7c..b1887224f 100644 --- a/crates/valence/src/lib.rs +++ b/crates/valence/src/lib.rs @@ -62,7 +62,9 @@ pub mod prelude { }; pub use glam::DVec3; pub use instance::{Block, BlockMut, BlockRef, Chunk, Instance}; - pub use inventory::{Inventory, InventoryKind, OpenInventory}; + pub use inventory::{ + Inventory, InventoryKind, InventoryWindow, InventoryWindowMut, OpenInventory, + }; pub use player_list::{PlayerList, PlayerListEntry}; pub use protocol::block::{BlockState, PropName, PropValue}; pub use protocol::ident::Ident; From 716ecff50c42f90dbe41ff93486d69c9b9e2f82f Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 10:45:35 -0400 Subject: [PATCH 18/49] validate transmutation for shift click and hotbar clicks --- crates/valence/src/inventory/validate.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index d62056215..d09b0ca6d 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -148,7 +148,21 @@ pub(crate) fn validate_click_slot_item_duplication( } let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - return count_deltas == 0; + if count_deltas != 0 { + return false; + } + + // assert that a swap occurs + let old_slots = [ + window.slot(packet.slots[0].idx as u16), + window.slot(packet.slots[1].idx as u16), + ]; + return old_slots + .iter() + .any(|s| s == &packet.slots[0].item.as_ref()) + && old_slots + .iter() + .any(|s| s == &packet.slots[1].item.as_ref()); } ClickMode::CreativeMiddleClick => true, ClickMode::DropKey => { From edc89b3e4c564bce7e873f5baac3d45b5eafe710 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 10:58:47 -0400 Subject: [PATCH 19/49] validate transmutations for regular clicks --- crates/valence/src/inventory/validate.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index d09b0ca6d..7e70ccaf8 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -138,8 +138,20 @@ pub(crate) fn validate_click_slot_item_duplication( return false; } - let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - return count_deltas == 0; + let old_slot = window.slot(packet.slots[0].idx as u16); + if old_slot.is_none() + || cursor_item.0.is_none() + || (old_slot.unwrap().item != cursor_item.0.as_ref().unwrap().item + && old_slot.unwrap().nbt != cursor_item.0.as_ref().unwrap().nbt) + { + // assert that a swap occurs + return old_slot == packet.carried_item.as_ref() + && cursor_item.0 == packet.slots[0].item; + } else { + // assert that a merge occurs + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); + return count_deltas == 0; + } } } ClickMode::ShiftClick | ClickMode::Hotbar => { From 64aa265497ad1c9e85cea0aa68ec92201a944af1 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 11:02:04 -0400 Subject: [PATCH 20/49] refactor drop via click validation --- crates/valence/src/inventory/validate.rs | 36 ++++++++---------------- 1 file changed, 11 insertions(+), 25 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 7e70ccaf8..6169de622 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -106,33 +106,19 @@ pub(crate) fn validate_click_slot_item_duplication( } // Clicked outside the window - match packet.button { + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); + return match packet.button { + 1 => count_deltas == -1, 0 => { - // drop entire stack - if packet.carried_item.is_none() { - // Dropping an item - return true; - } - } - 1 => { - // drop single item from stack - return match (&cursor_item.0, &packet.carried_item) { - (Some(server_item), Some(client_item)) => { - server_item.count() - 1 == client_item.count() - } - (Some(server_item), None) => server_item.count() == 1, - (None, _) => { - // can't possibly drop an item - false - } - }; + count_deltas + == -cursor_item + .0 + .as_ref() + .map(|s| s.count() as i32) + .unwrap_or(0) } - _ => { - // Invalid button - return false; - } - } - true + _ => unreachable!(), + }; } else { if packet.slots.len() != 1 { return false; From 6ef19ea6f9bc81dc7bfa5c99945c388c338354ae Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 11:13:57 -0400 Subject: [PATCH 21/49] validate transmuting for dropkey clicks --- crates/valence/src/inventory/validate.rs | 27 ++++++++++++++++++------ 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 6169de622..67db1fdb0 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -164,17 +164,30 @@ pub(crate) fn validate_click_slot_item_duplication( } ClickMode::CreativeMiddleClick => true, ClickMode::DropKey => { + if packet.slots.len() == 0 + || packet.slot_idx != packet.slots.first().map(|s| s.idx).unwrap_or(-2) + { + return false; + } + + let old_slot = window.slot(packet.slot_idx as u16); + let new_slot = packet.slots[0].item.as_ref(); + let is_transmuting = match (old_slot, new_slot) { + (Some(old_slot), Some(new_slot)) => { + old_slot.item != new_slot.item || old_slot.nbt != new_slot.nbt + } + (_, None) => false, + (None, Some(_)) => true, + }; + if is_transmuting { + return false; + } + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); return match packet.button { 0 => count_deltas == -1, - 1 => { - count_deltas - == -window - .slot(packet.slot_idx as u16) - .map(|s| s.count() as i32) - .unwrap_or(0) - } + 1 => count_deltas == -old_slot.map(|s| s.count() as i32).unwrap_or(0), _ => unreachable!(), }; } From f21b1d38b09757e3571aad29574b5eb00b811104 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 11:18:27 -0400 Subject: [PATCH 22/49] fix clippy lints --- crates/valence/src/client/event.rs | 3 +-- crates/valence/src/inventory.rs | 9 ++++----- crates/valence/src/inventory/validate.rs | 15 ++++++++------- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index cfa6e84f7..740f1c6bf 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -863,8 +863,7 @@ fn handle_one_packet( let open_inv = q .open_inventory .as_ref() - .map(|open| inventories.get(open.entity).ok()) - .flatten(); + .and_then(|open| inventories.get(open.entity).ok()); if !crate::inventory::validate_click_slot_impossible(&p, &q.inventory, open_inv) { debug!("client {:#?} invalid click slot packet: {:#?}", q.entity, p); return Ok(true); diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index d3d717c7b..a80140d7b 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -418,14 +418,13 @@ impl<'a> InventoryWindowMut<'a> { if let Some(open_inv) = self.open_inventory.as_mut() { if idx < open_inv.slot_count() { - return open_inv.replace_slot(idx, item); + open_inv.replace_slot(idx, item) } else { - return self - .player_inventory - .replace_slot(convert_to_player_slot_id(open_inv.kind(), idx), item); + self.player_inventory + .replace_slot(convert_to_player_slot_id(open_inv.kind(), idx), item) } } else { - return self.player_inventory.replace_slot(idx, item); + self.player_inventory.replace_slot(idx, item) } } diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 67db1fdb0..a3f336f14 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -136,7 +136,7 @@ pub(crate) fn validate_click_slot_item_duplication( } else { // assert that a merge occurs let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - return count_deltas == 0; + count_deltas == 0 } } } @@ -164,7 +164,7 @@ pub(crate) fn validate_click_slot_item_duplication( } ClickMode::CreativeMiddleClick => true, ClickMode::DropKey => { - if packet.slots.len() == 0 + if packet.slots.is_empty() || packet.slot_idx != packet.slots.first().map(|s| s.idx).unwrap_or(-2) { return false; @@ -185,22 +185,23 @@ pub(crate) fn validate_click_slot_item_duplication( let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - return match packet.button { + match packet.button { 0 => count_deltas == -1, 1 => count_deltas == -old_slot.map(|s| s.count() as i32).unwrap_or(0), _ => unreachable!(), - }; + } } ClickMode::Drag => { if matches!(packet.button, 2 | 6 | 10) { let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - return count_deltas == 0; + count_deltas == 0 + } else { + packet.slots.is_empty() && packet.carried_item == cursor_item.0 } - return packet.slots.is_empty() && packet.carried_item == cursor_item.0; } ClickMode::DoubleClick => { let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - return count_deltas == 0; + count_deltas == 0 } } } From 5df4eaf19facb2b5315f8ce2e6cd82f6dbf80160 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Mon, 20 Mar 2023 11:38:09 -0400 Subject: [PATCH 23/49] fix more lints --- crates/valence/src/inventory/validate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index a3f336f14..2bbef1757 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -626,12 +626,12 @@ mod test { for (i, packet) in packets.iter().enumerate() { assert!( - validate_click_slot_impossible(&packet, &player_inventory, None,), + validate_click_slot_impossible(packet, &player_inventory, None,), "packet {i} failed validation" ); assert!( !validate_click_slot_item_duplication( - &packet, + packet, &player_inventory, None, &cursor_item From a19280ac5fd533bc619514e19a84cbad1ae0812f Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 09:05:13 -0400 Subject: [PATCH 24/49] Update crates/valence/src/inventory/validate.rs Co-authored-by: Ryan Johnson --- crates/valence/src/inventory/validate.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 2bbef1757..4303d98b9 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -22,7 +22,7 @@ pub(crate) fn validate_click_slot_impossible( // check all slot ids and item counts are valid if !packet.slots.iter().all(|s| { (0..=max_slot).contains(&(s.idx as u16)) - && (0..=64).contains(&(s.item.as_ref().map(|i| i.count()).unwrap_or(0))) + && s.item.as_ref().map_or(true, |i| (1..=64).contains(&i.count())) }) { return false; } From 7d77e0436c1e280f7ef406f09cf9f96f9c860816 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 09:05:22 -0400 Subject: [PATCH 25/49] Update crates/valence/src/inventory/validate.rs Co-authored-by: Ryan Johnson --- crates/valence/src/inventory/validate.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 4303d98b9..bb39ac39e 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -31,8 +31,7 @@ pub(crate) fn validate_click_slot_impossible( if !packet .carried_item .as_ref() - .map(|i| (0..=64).contains(&i.count())) - .unwrap_or(true) + .map_or(true, |i| (1..=64).contains(&i.count())) { return false; } From 279e5f2462fef94e734abd79c449abe9cea49a0e Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 09:09:47 -0400 Subject: [PATCH 26/49] replace magic number with constant --- crates/valence/src/inventory.rs | 10 +++++++--- crates/valence/src/inventory/validate.rs | 8 +++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index a80140d7b..3cb4722d8 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -53,6 +53,10 @@ mod validate; pub(crate) use validate::*; +/// The number of slots in the "main" part of the player inventory. 3 rows of 9, +/// plus the hotbar. +pub const PLAYER_INVENTORY_MAIN_SLOTS_COUNT: u16 = 36; + #[derive(Debug, Clone, Component)] pub struct Inventory { title: Text, @@ -354,7 +358,7 @@ impl<'a> InventoryWindow<'a> { #[track_caller] pub fn slot_count(&self) -> u16 { match self.open_inventory.as_ref() { - Some(inv) => inv.slot_count() + 36, + Some(inv) => inv.slot_count() + PLAYER_INVENTORY_MAIN_SLOTS_COUNT, None => self.player_inventory.slot_count(), } } @@ -436,7 +440,7 @@ impl<'a> InventoryWindowMut<'a> { pub fn slot_count(&self) -> u16 { match self.open_inventory.as_ref() { - Some(inv) => inv.slot_count() + 36, + Some(inv) => inv.slot_count() + PLAYER_INVENTORY_MAIN_SLOTS_COUNT, None => self.player_inventory.slot_count(), } } @@ -829,7 +833,7 @@ fn convert_to_player_slot_id(target_kind: InventoryKind, slot_id: u16) -> u16 { } fn convert_hotbar_slot_id(slot_id: u16) -> u16 { - slot_id + 36 + slot_id + PLAYER_INVENTORY_MAIN_SLOTS_COUNT } #[derive(Copy, Clone, PartialEq, Eq, Debug)] diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index bb39ac39e..88affbcba 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -1,7 +1,7 @@ use valence_protocol::packet::c2s::play::click_slot::ClickMode; use valence_protocol::packet::c2s::play::ClickSlotC2s; -use super::{Inventory, InventoryWindow}; +use super::{Inventory, InventoryWindow, PLAYER_INVENTORY_MAIN_SLOTS_COUNT}; use crate::prelude::CursorItem; /// Validates a click slot packet enforcing that all fields are valid. @@ -15,14 +15,16 @@ pub(crate) fn validate_click_slot_impossible( } let max_slot = match open_inventory { - Some(inv) => inv.slot_count() + 36, + Some(inv) => inv.slot_count() + PLAYER_INVENTORY_MAIN_SLOTS_COUNT, None => player_inventory.slot_count(), }; // check all slot ids and item counts are valid if !packet.slots.iter().all(|s| { (0..=max_slot).contains(&(s.idx as u16)) - && s.item.as_ref().map_or(true, |i| (1..=64).contains(&i.count())) + && s.item + .as_ref() + .map_or(true, |i| (1..=64).contains(&i.count())) }) { return false; } From 5c633e05fad8d7a56c4dbcb5d33df7d27fc940c4 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 09:19:30 -0400 Subject: [PATCH 27/49] add comment --- crates/valence/src/inventory/validate.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 88affbcba..950c36bdc 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -82,6 +82,8 @@ pub(crate) fn validate_click_slot_impossible( /// Validates a click slot packet, enforcing that items can't be duplicated, eg. /// conservation of mass. +/// +/// Relies on assertions made by [`validate_click_slot_impossible`]. pub(crate) fn validate_click_slot_item_duplication( packet: &ClickSlotC2s, player_inventory: &Inventory, From 956eacb4079a7103c9a1fccb463e1126e81d3cc9 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 09:36:52 -0400 Subject: [PATCH 28/49] convert validate_click_slot_impossible to return Result --- crates/valence/src/inventory/validate.rs | 115 ++++++++++++++--------- 1 file changed, 69 insertions(+), 46 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 950c36bdc..c5e6bf915 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -1,3 +1,4 @@ +use anyhow::ensure; use valence_protocol::packet::c2s::play::click_slot::ClickMode; use valence_protocol::packet::c2s::play::ClickSlotC2s; @@ -9,10 +10,13 @@ pub(crate) fn validate_click_slot_impossible( packet: &ClickSlotC2s, player_inventory: &Inventory, open_inventory: Option<&Inventory>, -) -> bool { - if (packet.window_id == 0) != open_inventory.is_none() { - return false; - } +) -> anyhow::Result<()> { + ensure!( + (packet.window_id == 0) == open_inventory.is_none(), + "window id and open inventory mismatch: window_id: {} open_inventory: {}", + packet.window_id, + open_inventory.is_some() + ); let max_slot = match open_inventory { Some(inv) => inv.slot_count() + PLAYER_INVENTORY_MAIN_SLOTS_COUNT, @@ -20,64 +24,83 @@ pub(crate) fn validate_click_slot_impossible( }; // check all slot ids and item counts are valid - if !packet.slots.iter().all(|s| { - (0..=max_slot).contains(&(s.idx as u16)) - && s.item - .as_ref() - .map_or(true, |i| (1..=64).contains(&i.count())) - }) { - return false; - } + ensure!( + packet.slots.iter().all(|s| { + (0..=max_slot).contains(&(s.idx as u16)) + && s.item + .as_ref() + .map_or(true, |i| (1..=127).contains(&i.count())) + }), + "invalid slot ids or item counts" + ); // check carried item count is valid - if !packet - .carried_item - .as_ref() - .map_or(true, |i| (1..=64).contains(&i.count())) - { - return false; - } + ensure!( + packet + .carried_item + .as_ref() + .map_or(true, |i| (1..=127).contains(&i.count())), + "invalid carried item count" + ); match packet.mode { ClickMode::Click => { - if !(0..=1).contains(&packet.button) { - return false; - } - - if !(0..=max_slot).contains(&(packet.slot_idx as u16)) && packet.slot_idx != -999 { - return false; - } + ensure!((0..=1).contains(&packet.button), "invalid button"); + ensure!( + (0..=max_slot).contains(&(packet.slot_idx as u16)) || packet.slot_idx == -999, + "invalid slot index" + ) } ClickMode::ShiftClick => { - if !(0..=1).contains(&packet.button) { - return false; - } - - if packet.carried_item.is_some() { - return false; - } - - if !(0..=max_slot).contains(&(packet.slot_idx as u16)) { - return false; - } + ensure!((0..=1).contains(&packet.button), "invalid button"); + ensure!( + packet.carried_item.is_none(), + "carried item must be empty for a hotbar swap" + ); + ensure!( + (0..=max_slot).contains(&(packet.slot_idx as u16)), + "invalid slot index" + ) + } + ClickMode::Hotbar => { + ensure!(matches!(packet.button, 0..=8 | 40), "invalid button"); + ensure!( + packet.carried_item.is_none(), + "carried item must be empty for a hotbar swap" + ); } - ClickMode::Hotbar => return matches!(packet.button, 0..=8 | 40), ClickMode::CreativeMiddleClick => { - return packet.button == 2 && (0..=max_slot).contains(&(packet.slot_idx as u16)) + ensure!(packet.button == 2, "invalid button"); + ensure!( + (0..=max_slot).contains(&(packet.slot_idx as u16)), + "invalid slot index" + ) } ClickMode::DropKey => { - return (0..=1).contains(&packet.button) - && packet.carried_item.is_none() - && (0..=max_slot).contains(&(packet.slot_idx as u16)) + ensure!((0..=1).contains(&packet.button), "invalid button"); + ensure!( + packet.carried_item.is_none(), + "carried item must be empty for an item drop" + ); + ensure!( + (0..=max_slot).contains(&(packet.slot_idx as u16)), + "invalid slot index" + ) } ClickMode::Drag => { - return matches!(packet.button, 0..=2 | 4..=6 | 8..=10) - && ((0..=max_slot).contains(&(packet.slot_idx as u16)) || packet.slot_idx == -999) + ensure!( + matches!(packet.button, 0..=2 | 4..=6 | 8..=10), + "invalid button" + ); + ensure!( + (0..=max_slot).contains(&(packet.slot_idx as u16)) || packet.slot_idx == -999, + "invalid slot index" + ) } - ClickMode::DoubleClick => return packet.button == 0, + ClickMode::DoubleClick => ensure!(packet.button == 0, "invalid button"), } - true + Ok(()) } /// Validates a click slot packet, enforcing that items can't be duplicated, eg. From 85519c3fc637be04daa7915bc17ae9c568d1a75e Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 09:43:20 -0400 Subject: [PATCH 29/49] update unit tests --- crates/valence/src/inventory/validate.rs | 62 +++++++----------------- 1 file changed, 18 insertions(+), 44 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index c5e6bf915..3ba6e0436 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -367,11 +367,8 @@ mod test { carried_item: inventory.slot(0).cloned(), }; - assert!(validate_click_slot_impossible( - &packet, - &player_inventory, - Some(&inventory) - )); + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) + .expect("packet should be valid"); assert!(validate_click_slot_item_duplication( &packet, &player_inventory, @@ -412,11 +409,8 @@ mod test { carried_item: None, }; - assert!(validate_click_slot_impossible( - &packet1, - &player_inventory, - Some(&inventory1), - )); + validate_click_slot_impossible(&packet1, &player_inventory, Some(&inventory1)) + .expect("packet should be valid"); assert!(validate_click_slot_item_duplication( &packet1, &player_inventory, @@ -424,11 +418,8 @@ mod test { &cursor_item )); - assert!(validate_click_slot_impossible( - &packet2, - &player_inventory, - Some(&inventory2) - )); + validate_click_slot_impossible(&packet2, &player_inventory, Some(&inventory2)) + .expect("packet should be valid"); assert!(validate_click_slot_item_duplication( &packet2, &player_inventory, @@ -456,11 +447,8 @@ mod test { carried_item: Some(ItemStack::new(ItemKind::Diamond, 20, None)), }; - assert!(validate_click_slot_impossible( - &packet, - &player_inventory, - Some(&inventory), - )); + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) + .expect("packet should be valid"); assert!(validate_click_slot_item_duplication( &packet, &player_inventory, @@ -488,11 +476,8 @@ mod test { carried_item: Some(ItemStack::new(ItemKind::IronIngot, 2, None)), }; - assert!(validate_click_slot_impossible( - &packet, - &player_inventory, - Some(&inventory), - )); + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) + .expect("packet should be valid"); assert!(validate_click_slot_item_duplication( &packet, &player_inventory, @@ -551,11 +536,8 @@ mod test { carried_item: None, }; - assert!(validate_click_slot_impossible( - &packet1, - &player_inventory, - Some(&inventory1), - )); + validate_click_slot_impossible(&packet1, &player_inventory, Some(&inventory1)) + .expect("packet should be valid"); assert!(!validate_click_slot_item_duplication( &packet1, &player_inventory, @@ -563,11 +545,8 @@ mod test { &cursor_item )); - assert!(validate_click_slot_impossible( - &packet2, - &player_inventory, - Some(&inventory2) - )); + validate_click_slot_impossible(&packet2, &player_inventory, Some(&inventory2)) + .expect("packet should be valid"); assert!(!validate_click_slot_item_duplication( &packet2, &player_inventory, @@ -575,11 +554,8 @@ mod test { &cursor_item )); - assert!(validate_click_slot_impossible( - &packet3, - &player_inventory, - Some(&inventory1) - )); + validate_click_slot_impossible(&packet3, &player_inventory, Some(&inventory1)) + .expect("packet should be valid"); assert!(!validate_click_slot_item_duplication( &packet3, &player_inventory, @@ -651,10 +627,8 @@ mod test { ]; for (i, packet) in packets.iter().enumerate() { - assert!( - validate_click_slot_impossible(packet, &player_inventory, None,), - "packet {i} failed validation" - ); + validate_click_slot_impossible(packet, &player_inventory, None) + .expect(format!("packet {i} should be valid").as_str()); assert!( !validate_click_slot_item_duplication( packet, From 8d0963bbcb078333705034156876e58dd5b855cb Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 09:45:51 -0400 Subject: [PATCH 30/49] update call site --- crates/valence/src/client/event.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index 740f1c6bf..036fd19be 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -864,8 +864,13 @@ fn handle_one_packet( .open_inventory .as_ref() .and_then(|open| inventories.get(open.entity).ok()); - if !crate::inventory::validate_click_slot_impossible(&p, &q.inventory, open_inv) { - debug!("client {:#?} invalid click slot packet: {:#?}", q.entity, p); + if let Err(msg) = + crate::inventory::validate_click_slot_impossible(&p, &q.inventory, open_inv) + { + debug!( + "client {:#?} invalid click slot packet: \"{}\" {:#?}", + q.entity, msg, p + ); return Ok(true); } if !crate::inventory::validate_click_slot_item_duplication( From c1f37b7a0748695aa25114d139e40d0410990cbe Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 10:31:23 -0400 Subject: [PATCH 31/49] refactor validate_click_slot_item_duplication to return Result --- crates/valence/src/client/event.rs | 7 +- crates/valence/src/inventory/validate.rs | 211 +++++++++++++---------- 2 files changed, 128 insertions(+), 90 deletions(-) diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index 036fd19be..a9a4dbde7 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -873,15 +873,16 @@ fn handle_one_packet( ); return Ok(true); } - if !crate::inventory::validate_click_slot_item_duplication( + if let Err(msg) = crate::inventory::validate_click_slot_item_duplication( &p, &q.inventory, open_inv, &q.cursor_item, ) { debug!( - "client {:#?} click slot packet tried to incorrectly modify items: {:#?}", - q.entity, p + "client {:#?} click slot packet tried to incorrectly modify items: \"{}\" \ + {:#?}", + q.entity, msg, p ); return Ok(true); } diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 3ba6e0436..16cd4e37d 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -112,7 +112,7 @@ pub(crate) fn validate_click_slot_item_duplication( player_inventory: &Inventory, open_inventory: Option<&Inventory>, cursor_item: &CursorItem, -) -> bool { +) -> anyhow::Result<()> { let window = InventoryWindow { player_inventory, open_inventory, @@ -122,33 +122,36 @@ pub(crate) fn validate_click_slot_item_duplication( ClickMode::Click => { if packet.slot_idx == -999 { // Clicked outside the window, so the client is dropping an item - if cursor_item.0.is_none() { - // Nothing to drop - return false; - } - - if !packet.slots.is_empty() { - return false; - } + ensure!(packet.slots.is_empty(), "slot modifications must be empty"); // Clicked outside the window let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - return match packet.button { - 1 => count_deltas == -1, - 0 => { + match packet.button { + 1 => ensure!( + count_deltas == -1, + "invalid item delta: expected -1, got {}", count_deltas - == -cursor_item - .0 - .as_ref() - .map(|s| s.count() as i32) - .unwrap_or(0) + ), + 0 => { + ensure!( + count_deltas + == -cursor_item + .0 + .as_ref() + .map(|s| s.count() as i32) + .unwrap_or(0), + "invalid item delta: {}", + count_deltas + ) } _ => unreachable!(), - }; - } else { - if packet.slots.len() != 1 { - return false; } + } else { + ensure!( + packet.slots.len() == 1, + "click must modify one slot, got {}", + packet.slots.len() + ); let old_slot = window.slot(packet.slots[0].idx as u16); if old_slot.is_none() @@ -157,44 +160,60 @@ pub(crate) fn validate_click_slot_item_duplication( && old_slot.unwrap().nbt != cursor_item.0.as_ref().unwrap().nbt) { // assert that a swap occurs - return old_slot == packet.carried_item.as_ref() - && cursor_item.0 == packet.slots[0].item; + ensure!( + old_slot == packet.carried_item.as_ref() + && cursor_item.0 == packet.slots[0].item, + "swapped items must match" + ); } else { // assert that a merge occurs let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - count_deltas == 0 + ensure!( + count_deltas == 0, + "invalid item delta for stack merge: {}", + count_deltas + ); } } } ClickMode::ShiftClick | ClickMode::Hotbar => { - if packet.slots.len() != 2 { - return false; - } + ensure!( + packet.slots.len() == 2, + "shift click and hotbar swaps must modify exactly two slots" + ); let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - if count_deltas != 0 { - return false; - } + ensure!( + count_deltas == 0, + "invalid item delta: expected 0, got {}", + count_deltas + ); // assert that a swap occurs let old_slots = [ window.slot(packet.slots[0].idx as u16), window.slot(packet.slots[1].idx as u16), ]; - return old_slots - .iter() - .any(|s| s == &packet.slots[0].item.as_ref()) - && old_slots + ensure!( + old_slots .iter() - .any(|s| s == &packet.slots[1].item.as_ref()); + .any(|s| s == &packet.slots[0].item.as_ref()) + && old_slots + .iter() + .any(|s| s == &packet.slots[1].item.as_ref()), + "swapped items must match" + ); } - ClickMode::CreativeMiddleClick => true, + ClickMode::CreativeMiddleClick => {} ClickMode::DropKey => { - if packet.slots.is_empty() - || packet.slot_idx != packet.slots.first().map(|s| s.idx).unwrap_or(-2) - { - return false; - } + ensure!( + packet.slots.len() == 1, + "drop key must modify exactly one slot" + ); + ensure!( + packet.slot_idx == packet.slots.first().map(|s| s.idx).unwrap_or(-2), + "slot index does not match modified slot" + ); let old_slot = window.slot(packet.slot_idx as u16); let new_slot = packet.slots[0].item.as_ref(); @@ -205,31 +224,45 @@ pub(crate) fn validate_click_slot_item_duplication( (_, None) => false, (None, Some(_)) => true, }; - if is_transmuting { - return false; - } + ensure!(!is_transmuting, "transmuting items is not allowed"); let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - match packet.button { - 0 => count_deltas == -1, - 1 => count_deltas == -old_slot.map(|s| s.count() as i32).unwrap_or(0), + let expected_delta = match packet.button { + 0 => -1, + 1 => -old_slot.map(|s| s.count() as i32).unwrap_or(0), _ => unreachable!(), - } + }; + ensure!( + count_deltas == expected_delta, + "invalid item delta: expected {}, got {}", + expected_delta, + count_deltas + ); } ClickMode::Drag => { if matches!(packet.button, 2 | 6 | 10) { let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - count_deltas == 0 + ensure!( + count_deltas == 0, + "invalid item delta: expected 0, got {}", + count_deltas + ); } else { - packet.slots.is_empty() && packet.carried_item == cursor_item.0 + ensure!(packet.slots.is_empty() && packet.carried_item == cursor_item.0); } } ClickMode::DoubleClick => { let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - count_deltas == 0 + ensure!( + count_deltas == 0, + "invalid item delta: expected 0, got {}", + count_deltas + ); } } + + Ok(()) } /// Calculate the total difference in item counts if the changes in this packet @@ -369,12 +402,13 @@ mod test { validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) .expect("packet should be valid"); - assert!(validate_click_slot_item_duplication( + validate_click_slot_item_duplication( &packet, &player_inventory, Some(&inventory), - &cursor_item - )); + &cursor_item, + ) + .expect("packet should not fail item duplication check"); } #[test] @@ -411,21 +445,23 @@ mod test { validate_click_slot_impossible(&packet1, &player_inventory, Some(&inventory1)) .expect("packet should be valid"); - assert!(validate_click_slot_item_duplication( + validate_click_slot_item_duplication( &packet1, &player_inventory, Some(&inventory1), - &cursor_item - )); + &cursor_item, + ) + .expect("packet should not fail item duplication check"); validate_click_slot_impossible(&packet2, &player_inventory, Some(&inventory2)) .expect("packet should be valid"); - assert!(validate_click_slot_item_duplication( + validate_click_slot_item_duplication( &packet2, &player_inventory, Some(&inventory2), - &cursor_item - )); + &cursor_item, + ) + .expect("packet should not fail item duplication check"); } #[test] @@ -449,12 +485,13 @@ mod test { validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) .expect("packet should be valid"); - assert!(validate_click_slot_item_duplication( + validate_click_slot_item_duplication( &packet, &player_inventory, Some(&inventory), - &cursor_item - )); + &cursor_item, + ) + .expect("packet should not fail item duplication check"); } #[test] @@ -478,12 +515,13 @@ mod test { validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) .expect("packet should be valid"); - assert!(validate_click_slot_item_duplication( + validate_click_slot_item_duplication( &packet, &player_inventory, Some(&inventory), - &cursor_item - )); + &cursor_item, + ) + .expect("packet should not fail item duplication check"); } #[test] @@ -537,31 +575,34 @@ mod test { }; validate_click_slot_impossible(&packet1, &player_inventory, Some(&inventory1)) - .expect("packet should be valid"); - assert!(!validate_click_slot_item_duplication( + .expect("packet 1 should be valid"); + validate_click_slot_item_duplication( &packet1, &player_inventory, Some(&inventory1), - &cursor_item - )); + &cursor_item, + ) + .expect_err("packet 1 should fail item duplication check"); validate_click_slot_impossible(&packet2, &player_inventory, Some(&inventory2)) - .expect("packet should be valid"); - assert!(!validate_click_slot_item_duplication( + .expect("packet 2 should be valid"); + validate_click_slot_item_duplication( &packet2, &player_inventory, Some(&inventory2), - &cursor_item - )); + &cursor_item, + ) + .expect_err("packet 2 should fail item duplication check"); validate_click_slot_impossible(&packet3, &player_inventory, Some(&inventory1)) - .expect("packet should be valid"); - assert!(!validate_click_slot_item_duplication( + .expect("packet 3 should be valid"); + validate_click_slot_item_duplication( &packet3, &player_inventory, Some(&inventory1), - &cursor_item - )); + &cursor_item, + ) + .expect_err("packet 3 should fail item duplication check"); } #[test] @@ -629,15 +670,11 @@ mod test { for (i, packet) in packets.iter().enumerate() { validate_click_slot_impossible(packet, &player_inventory, None) .expect(format!("packet {i} should be valid").as_str()); - assert!( - !validate_click_slot_item_duplication( - packet, - &player_inventory, - None, - &cursor_item - ), - "packet {i} passed item duplication check when it should have failed" - ); + validate_click_slot_item_duplication(packet, &player_inventory, None, &cursor_item) + .expect_err( + format!("packet {i} passed item duplication check when it should have failed") + .as_str(), + ); } } } From b7898121fa508dd6fac3807eabc5c6c2869fb691 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 10:33:20 -0400 Subject: [PATCH 32/49] small refactor --- crates/valence/src/inventory/validate.rs | 33 ++++++++++-------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 16cd4e37d..a22639e90 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -126,26 +126,21 @@ pub(crate) fn validate_click_slot_item_duplication( // Clicked outside the window let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); - match packet.button { - 1 => ensure!( - count_deltas == -1, - "invalid item delta: expected -1, got {}", - count_deltas - ), - 0 => { - ensure!( - count_deltas - == -cursor_item - .0 - .as_ref() - .map(|s| s.count() as i32) - .unwrap_or(0), - "invalid item delta: {}", - count_deltas - ) - } + let expected_delta = match packet.button { + 1 => -1, + 0 => -cursor_item + .0 + .as_ref() + .map(|s| s.count() as i32) + .unwrap_or(0), _ => unreachable!(), - } + }; + ensure!( + count_deltas == expected_delta, + "invalid item delta: expected {}, got {}", + expected_delta, + count_deltas + ); } else { ensure!( packet.slots.len() == 1, From f5a1e56f6c02d4604a2e75a1fb68b7168341ac00 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 10:39:46 -0400 Subject: [PATCH 33/49] fix splitting stacks failing validation --- crates/valence/examples/chest.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/valence/examples/chest.rs b/crates/valence/examples/chest.rs index d8ddf1d9f..364934a96 100644 --- a/crates/valence/examples/chest.rs +++ b/crates/valence/examples/chest.rs @@ -1,6 +1,6 @@ #![allow(clippy::type_complexity)] -use tracing::warn; +use tracing::{warn, Level}; use valence::client::despawn_disconnected_clients; use valence::client::event::{default_event_handler, PlayerInteractBlock, StartSneaking}; use valence::prelude::*; @@ -9,10 +9,12 @@ const SPAWN_Y: i32 = 64; const CHEST_POS: [i32; 3] = [0, SPAWN_Y + 1, 3]; pub fn main() { - tracing_subscriber::fmt().init(); + tracing_subscriber::fmt() + .with_max_level(Level::DEBUG) + .init(); App::new() - .add_plugin(ServerPlugin::new(())) + .add_plugin(ServerPlugin::new(()).with_connection_mode(ConnectionMode::Offline)) .add_startup_system(setup) .add_system(init_clients) .add_systems( From 1d8045e9024dd96472c4b36a4080efd7778efff1 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 11:18:38 -0400 Subject: [PATCH 34/49] send inventory packet upon failing validation --- crates/valence/src/client/event.rs | 28 +++++++++++++++++++++++- crates/valence/src/inventory.rs | 2 +- crates/valence/src/inventory/validate.rs | 9 ++++---- 3 files changed, 33 insertions(+), 6 deletions(-) diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index a9a4dbde7..2d17e616b 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::cmp; use anyhow::bail; @@ -29,9 +30,11 @@ use valence_protocol::packet::c2s::play::update_structure_block::{ use valence_protocol::packet::c2s::play::{ AdvancementTabC2s, ClientStatusC2s, ResourcePackStatusC2s, UpdatePlayerAbilitiesC2s, }; +use valence_protocol::packet::s2c::play::InventoryS2c; use valence_protocol::packet::C2sPlayPacket; use valence_protocol::tracked_data::Pose; use valence_protocol::types::{Difficulty, Direction, Hand}; +use valence_protocol::var_int::VarInt; use super::{ CursorItem, KeepaliveState, PlayerActionSequence, PlayerInventoryState, TeleportState, @@ -41,6 +44,7 @@ use crate::client::Client; use crate::component::{Look, OnGround, Ping, Position}; use crate::entity::{EntityAnimation, EntityKind, McEntity, TrackedData}; use crate::inventory::Inventory; +use crate::packet::WritePacket; use crate::prelude::OpenInventory; #[derive(Clone, Debug)] @@ -863,7 +867,7 @@ fn handle_one_packet( let open_inv = q .open_inventory .as_ref() - .and_then(|open| inventories.get(open.entity).ok()); + .and_then(|open| inventories.get_mut(open.entity).ok()); if let Err(msg) = crate::inventory::validate_click_slot_impossible(&p, &q.inventory, open_inv) { @@ -871,6 +875,17 @@ fn handle_one_packet( "client {:#?} invalid click slot packet: \"{}\" {:#?}", q.entity, msg, p ); + let inventory = open_inv.unwrap_or(&q.inventory); + q.client.write_packet(&InventoryS2c { + window_id: if open_inv.is_some() { + q.player_inventory_state.window_id + } else { + 0 + }, + state_id: VarInt(q.player_inventory_state.state_id.0), + slots: Cow::Borrowed(inventory.slot_slice()), + carried_item: Cow::Borrowed(&q.cursor_item.0), + }); return Ok(true); } if let Err(msg) = crate::inventory::validate_click_slot_item_duplication( @@ -884,6 +899,17 @@ fn handle_one_packet( {:#?}", q.entity, msg, p ); + let inventory = open_inv.unwrap_or(&q.inventory); + q.client.write_packet(&InventoryS2c { + window_id: if open_inv.is_some() { + q.player_inventory_state.window_id + } else { + 0 + }, + state_id: VarInt(q.player_inventory_state.state_id.0), + slots: Cow::Borrowed(inventory.slot_slice()), + carried_item: Cow::Borrowed(&q.cursor_item.0), + }); return Ok(true); } if p.slot_idx < 0 && p.mode == ClickMode::Click { diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index 3cb4722d8..7bea77b15 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -245,7 +245,7 @@ impl Inventory { std::mem::replace(&mut self.title, title.into()) } - fn slot_slice(&self) -> &[Option] { + pub(crate) fn slot_slice(&self) -> &[Option] { self.slots.as_ref() } diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index a22639e90..4e140c917 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -149,10 +149,11 @@ pub(crate) fn validate_click_slot_item_duplication( ); let old_slot = window.slot(packet.slots[0].idx as u16); - if old_slot.is_none() - || cursor_item.0.is_none() - || (old_slot.unwrap().item != cursor_item.0.as_ref().unwrap().item - && old_slot.unwrap().nbt != cursor_item.0.as_ref().unwrap().nbt) + if packet.button == 0 + && (old_slot.is_none() + || cursor_item.0.is_none() + || (old_slot.unwrap().item != cursor_item.0.as_ref().unwrap().item + && old_slot.unwrap().nbt != cursor_item.0.as_ref().unwrap().nbt)) { // assert that a swap occurs ensure!( From f3d521d56666d55fe96890aec9a74a4d2d0a5d18 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 11:21:55 -0400 Subject: [PATCH 35/49] fix clippy lints --- crates/valence/src/inventory/validate.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 4e140c917..8d39399c8 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -665,12 +665,13 @@ mod test { for (i, packet) in packets.iter().enumerate() { validate_click_slot_impossible(packet, &player_inventory, None) - .expect(format!("packet {i} should be valid").as_str()); + .unwrap_or_else(|e| panic!("packet {i} should be valid: {e}")); validate_click_slot_item_duplication(packet, &player_inventory, None, &cursor_item) - .expect_err( - format!("packet {i} passed item duplication check when it should have failed") - .as_str(), - ); + .unwrap_or_else(|e| { + panic!( + "packet {i} passed item duplication check when it should have failed: {e}" + ) + }); } } } From 057ba5a4fadf6f0d25276248ec3e400d16d3e7fd Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 16:24:38 -0400 Subject: [PATCH 36/49] fix typo --- crates/valence/src/inventory/validate.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 8d39399c8..96602c02a 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -667,11 +667,9 @@ mod test { validate_click_slot_impossible(packet, &player_inventory, None) .unwrap_or_else(|e| panic!("packet {i} should be valid: {e}")); validate_click_slot_item_duplication(packet, &player_inventory, None, &cursor_item) - .unwrap_or_else(|e| { - panic!( - "packet {i} passed item duplication check when it should have failed: {e}" - ) - }); + .expect_err(&format!( + "packet {i} passed item duplication check when it should have failed" + )); } } } From 182a24fcc9024374458a1ed4123929bb2aa61a8b Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Tue, 21 Mar 2023 16:32:57 -0400 Subject: [PATCH 37/49] add feature flag to let users opt out of item duplication checks --- crates/valence/Cargo.toml | 4 ++++ crates/valence/src/client/event.rs | 1 + 2 files changed, 5 insertions(+) diff --git a/crates/valence/Cargo.toml b/crates/valence/Cargo.toml index 459f08e63..549b74840 100644 --- a/crates/valence/Cargo.toml +++ b/crates/valence/Cargo.toml @@ -11,6 +11,10 @@ categories = ["game-engines"] build = "build/main.rs" authors = ["Ryan Johnson "] +[features] +default = ["anti_cheat_item_duplication"] +anti_cheat_item_duplication = [] + [dependencies] anyhow = "1.0.65" arrayvec = "0.7.2" diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index 2d17e616b..192c2d5ae 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -888,6 +888,7 @@ fn handle_one_packet( }); return Ok(true); } + #[cfg(feature = "anti_cheat_item_duplication")] if let Err(msg) = crate::inventory::validate_click_slot_item_duplication( &p, &q.inventory, From f809ba62444f61b09f22965a511afbe719a9958b Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Wed, 22 Mar 2023 09:02:31 -0400 Subject: [PATCH 38/49] revert changes to chest example --- crates/valence/examples/chest.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/valence/examples/chest.rs b/crates/valence/examples/chest.rs index 364934a96..d8ddf1d9f 100644 --- a/crates/valence/examples/chest.rs +++ b/crates/valence/examples/chest.rs @@ -1,6 +1,6 @@ #![allow(clippy::type_complexity)] -use tracing::{warn, Level}; +use tracing::warn; use valence::client::despawn_disconnected_clients; use valence::client::event::{default_event_handler, PlayerInteractBlock, StartSneaking}; use valence::prelude::*; @@ -9,12 +9,10 @@ const SPAWN_Y: i32 = 64; const CHEST_POS: [i32; 3] = [0, SPAWN_Y + 1, 3]; pub fn main() { - tracing_subscriber::fmt() - .with_max_level(Level::DEBUG) - .init(); + tracing_subscriber::fmt().init(); App::new() - .add_plugin(ServerPlugin::new(()).with_connection_mode(ConnectionMode::Offline)) + .add_plugin(ServerPlugin::new(())) .add_startup_system(setup) .add_system(init_clients) .add_systems( From ebc0c30460e1a8c4d1e6d670fc7f9514faaed4fb Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Wed, 22 Mar 2023 09:13:59 -0400 Subject: [PATCH 39/49] refine how the maximum stack size is determined --- crates/valence/src/client/event.rs | 9 ++++++--- crates/valence/src/inventory/validate.rs | 25 +++++++++++++++++------- 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index 192c2d5ae..1825050d7 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -868,9 +868,12 @@ fn handle_one_packet( .open_inventory .as_ref() .and_then(|open| inventories.get_mut(open.entity).ok()); - if let Err(msg) = - crate::inventory::validate_click_slot_impossible(&p, &q.inventory, open_inv) - { + if let Err(msg) = crate::inventory::validate_click_slot_impossible( + &p, + &q.inventory, + open_inv, + &q.cursor_item, + ) { debug!( "client {:#?} invalid click slot packet: \"{}\" {:#?}", q.entity, msg, p diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 96602c02a..89dfe0c5a 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -10,6 +10,7 @@ pub(crate) fn validate_click_slot_impossible( packet: &ClickSlotC2s, player_inventory: &Inventory, open_inventory: Option<&Inventory>, + cursor_item: &CursorItem, ) -> anyhow::Result<()> { ensure!( (packet.window_id == 0) == open_inventory.is_none(), @@ -23,23 +24,33 @@ pub(crate) fn validate_click_slot_impossible( None => player_inventory.slot_count(), }; + let window = InventoryWindow::new(player_inventory, open_inventory); + // check all slot ids and item counts are valid ensure!( packet.slots.iter().all(|s| { (0..=max_slot).contains(&(s.idx as u16)) - && s.item - .as_ref() - .map_or(true, |i| (1..=127).contains(&i.count())) + && s.item.as_ref().map_or(true, |i| { + (1..=i.item.max_stack().max( + window + .slot(s.idx as u16) + .as_ref() + .map(|c| c.count()) + .unwrap_or(0), + )) + .contains(&i.count()) + }) }), "invalid slot ids or item counts" ); // check carried item count is valid ensure!( - packet - .carried_item - .as_ref() - .map_or(true, |i| (1..=127).contains(&i.count())), + packet.carried_item.as_ref().map_or(true, |i| (1..=i + .item + .max_stack() + .max(cursor_item.0.as_ref().map(|c| c.count()).unwrap_or(0))) + .contains(&i.count())), "invalid carried item count" ); From d417dad3b8119b8c6502b1d4be416b4033aa19b7 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Wed, 22 Mar 2023 09:29:34 -0400 Subject: [PATCH 40/49] add resource for inventory settings --- crates/valence/Cargo.toml | 4 -- crates/valence/src/client/event.rs | 63 ++++++++++++++++-------------- crates/valence/src/inventory.rs | 13 ++++++ crates/valence/src/server.rs | 3 +- 4 files changed, 49 insertions(+), 34 deletions(-) diff --git a/crates/valence/Cargo.toml b/crates/valence/Cargo.toml index 549b74840..459f08e63 100644 --- a/crates/valence/Cargo.toml +++ b/crates/valence/Cargo.toml @@ -11,10 +11,6 @@ categories = ["game-engines"] build = "build/main.rs" authors = ["Ryan Johnson "] -[features] -default = ["anti_cheat_item_duplication"] -anti_cheat_item_duplication = [] - [dependencies] anyhow = "1.0.65" arrayvec = "0.7.2" diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index 1825050d7..b60a0cfcc 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -43,7 +43,7 @@ use super::{ use crate::client::Client; use crate::component::{Look, OnGround, Ping, Position}; use crate::entity::{EntityAnimation, EntityKind, McEntity, TrackedData}; -use crate::inventory::Inventory; +use crate::inventory::{Inventory, InventorySettings}; use crate::packet::WritePacket; use crate::prelude::OpenInventory; @@ -698,10 +698,12 @@ fn run_event_loop( ClientEvents, Commands, Query<&Inventory, Without>, + Res, )>, mut clients_to_check: Local>, ) { - let (mut clients, mut events, mut commands, mut inventories) = state.get_mut(world); + let (mut clients, mut events, mut commands, mut inventories, inventory_settings) = + state.get_mut(world); update_all_event_buffers(&mut events); @@ -719,7 +721,7 @@ fn run_event_loop( q.client.dec.queue_bytes(bytes); - match handle_one_packet(&mut q, &mut events, &mut inventories) { + match handle_one_packet(&mut q, &mut events, &mut inventories, &inventory_settings) { Ok(had_packet) => { if had_packet { // We decoded one packet, but there might be more. @@ -739,7 +741,8 @@ fn run_event_loop( while !clients_to_check.is_empty() { world.run_schedule(EventLoopSchedule); - let (mut clients, mut events, mut commands, mut inventories) = state.get_mut(world); + let (mut clients, mut events, mut commands, mut inventories, inventory_settings) = + state.get_mut(world); clients_to_check.retain(|&entity| { let Ok(mut q) = clients.get_mut(entity) else { @@ -747,7 +750,7 @@ fn run_event_loop( return false; }; - match handle_one_packet(&mut q, &mut events, &mut inventories) { + match handle_one_packet(&mut q, &mut events, &mut inventories, &inventory_settings) { Ok(had_packet) => had_packet, Err(e) => { warn!("failed to dispatch events for client {:?}: {e:?}", q.entity); @@ -765,6 +768,7 @@ fn handle_one_packet( q: &mut EventLoopQueryItem, events: &mut ClientEvents, inventories: &mut Query<&Inventory, Without>, + inventory_settings: &Res, ) -> anyhow::Result { let Some(pkt) = q.client.dec.try_next_packet::()? else { // No packets to decode. @@ -891,30 +895,31 @@ fn handle_one_packet( }); return Ok(true); } - #[cfg(feature = "anti_cheat_item_duplication")] - if let Err(msg) = crate::inventory::validate_click_slot_item_duplication( - &p, - &q.inventory, - open_inv, - &q.cursor_item, - ) { - debug!( - "client {:#?} click slot packet tried to incorrectly modify items: \"{}\" \ - {:#?}", - q.entity, msg, p - ); - let inventory = open_inv.unwrap_or(&q.inventory); - q.client.write_packet(&InventoryS2c { - window_id: if open_inv.is_some() { - q.player_inventory_state.window_id - } else { - 0 - }, - state_id: VarInt(q.player_inventory_state.state_id.0), - slots: Cow::Borrowed(inventory.slot_slice()), - carried_item: Cow::Borrowed(&q.cursor_item.0), - }); - return Ok(true); + if inventory_settings.enable_item_dupe_check { + if let Err(msg) = crate::inventory::validate_click_slot_item_duplication( + &p, + &q.inventory, + open_inv, + &q.cursor_item, + ) { + debug!( + "client {:#?} click slot packet tried to incorrectly modify items: \"{}\" \ + {:#?}", + q.entity, msg, p + ); + let inventory = open_inv.unwrap_or(&q.inventory); + q.client.write_packet(&InventoryS2c { + window_id: if open_inv.is_some() { + q.player_inventory_state.window_id + } else { + 0 + }, + state_id: VarInt(q.player_inventory_state.state_id.0), + slots: Cow::Borrowed(inventory.slot_slice()), + carried_item: Cow::Borrowed(&q.cursor_item.0), + }); + return Ok(true); + } } if p.slot_idx < 0 && p.mode == ClickMode::Click { if let Some(stack) = q.cursor_item.0.take() { diff --git a/crates/valence/src/inventory.rs b/crates/valence/src/inventory.rs index 7bea77b15..8fc7ee07c 100644 --- a/crates/valence/src/inventory.rs +++ b/crates/valence/src/inventory.rs @@ -964,6 +964,19 @@ impl From for InventoryKind { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Resource)] +pub struct InventorySettings { + pub enable_item_dupe_check: bool, +} + +impl Default for InventorySettings { + fn default() -> Self { + Self { + enable_item_dupe_check: true, + } + } +} + #[cfg(test)] mod test { use bevy_app::App; diff --git a/crates/valence/src/server.rs b/crates/valence/src/server.rs index 079ebf816..ae65fe4f9 100644 --- a/crates/valence/src/server.rs +++ b/crates/valence/src/server.rs @@ -25,7 +25,7 @@ use crate::config::{AsyncCallbacks, ConnectionMode, ServerPlugin}; use crate::dimension::{validate_dimensions, Dimension, DimensionId}; use crate::entity::EntityPlugin; use crate::instance::{Instance, InstancePlugin}; -use crate::inventory::InventoryPlugin; +use crate::inventory::{InventoryPlugin, InventorySettings}; use crate::player_list::PlayerListPlugin; use crate::prelude::event::ClientEventPlugin; use crate::prelude::ComponentPlugin; @@ -303,6 +303,7 @@ pub fn build_plugin( // Insert resources. app.insert_resource(server); + app.insert_resource(InventorySettings::default()); // Make the app loop forever at the configured TPS. { From f49e094b13622e821a7af6f8077ca62bd830982a Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Wed, 22 Mar 2023 09:31:19 -0400 Subject: [PATCH 41/49] fix unit tests --- crates/valence/src/inventory/validate.rs | 53 +++++++++++++++++------- 1 file changed, 39 insertions(+), 14 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 89dfe0c5a..9c7e012cd 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -407,7 +407,7 @@ mod test { carried_item: inventory.slot(0).cloned(), }; - validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory), &cursor_item) .expect("packet should be valid"); validate_click_slot_item_duplication( &packet, @@ -450,8 +450,13 @@ mod test { carried_item: None, }; - validate_click_slot_impossible(&packet1, &player_inventory, Some(&inventory1)) - .expect("packet should be valid"); + validate_click_slot_impossible( + &packet1, + &player_inventory, + Some(&inventory1), + &cursor_item, + ) + .expect("packet should be valid"); validate_click_slot_item_duplication( &packet1, &player_inventory, @@ -460,8 +465,13 @@ mod test { ) .expect("packet should not fail item duplication check"); - validate_click_slot_impossible(&packet2, &player_inventory, Some(&inventory2)) - .expect("packet should be valid"); + validate_click_slot_impossible( + &packet2, + &player_inventory, + Some(&inventory2), + &cursor_item, + ) + .expect("packet should be valid"); validate_click_slot_item_duplication( &packet2, &player_inventory, @@ -490,7 +500,7 @@ mod test { carried_item: Some(ItemStack::new(ItemKind::Diamond, 20, None)), }; - validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory), &cursor_item) .expect("packet should be valid"); validate_click_slot_item_duplication( &packet, @@ -520,7 +530,7 @@ mod test { carried_item: Some(ItemStack::new(ItemKind::IronIngot, 2, None)), }; - validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory), &cursor_item) .expect("packet should be valid"); validate_click_slot_item_duplication( &packet, @@ -581,8 +591,13 @@ mod test { carried_item: None, }; - validate_click_slot_impossible(&packet1, &player_inventory, Some(&inventory1)) - .expect("packet 1 should be valid"); + validate_click_slot_impossible( + &packet1, + &player_inventory, + Some(&inventory1), + &cursor_item, + ) + .expect("packet 1 should be valid"); validate_click_slot_item_duplication( &packet1, &player_inventory, @@ -591,8 +606,13 @@ mod test { ) .expect_err("packet 1 should fail item duplication check"); - validate_click_slot_impossible(&packet2, &player_inventory, Some(&inventory2)) - .expect("packet 2 should be valid"); + validate_click_slot_impossible( + &packet2, + &player_inventory, + Some(&inventory2), + &cursor_item, + ) + .expect("packet 2 should be valid"); validate_click_slot_item_duplication( &packet2, &player_inventory, @@ -601,8 +621,13 @@ mod test { ) .expect_err("packet 2 should fail item duplication check"); - validate_click_slot_impossible(&packet3, &player_inventory, Some(&inventory1)) - .expect("packet 3 should be valid"); + validate_click_slot_impossible( + &packet3, + &player_inventory, + Some(&inventory1), + &cursor_item, + ) + .expect("packet 3 should be valid"); validate_click_slot_item_duplication( &packet3, &player_inventory, @@ -675,7 +700,7 @@ mod test { ]; for (i, packet) in packets.iter().enumerate() { - validate_click_slot_impossible(packet, &player_inventory, None) + validate_click_slot_impossible(packet, &player_inventory, None, &cursor_item) .unwrap_or_else(|e| panic!("packet {i} should be valid: {e}")); validate_click_slot_item_duplication(packet, &player_inventory, None, &cursor_item) .expect_err(&format!( From 34287f664a7e59210d8c94d83e8985ebc79f9c57 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Wed, 22 Mar 2023 09:58:15 -0400 Subject: [PATCH 42/49] remove nbt checks because they were causing problems, add todo --- crates/valence/src/inventory/validate.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 9c7e012cd..7ca1ed493 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -160,11 +160,13 @@ pub(crate) fn validate_click_slot_item_duplication( ); let old_slot = window.slot(packet.slots[0].idx as u16); + // TODO: make sure NBT is the same + // Sometimes, the client will add nbt data to an item if it's missing, like + // "Damage" to a sword if packet.button == 0 && (old_slot.is_none() || cursor_item.0.is_none() - || (old_slot.unwrap().item != cursor_item.0.as_ref().unwrap().item - && old_slot.unwrap().nbt != cursor_item.0.as_ref().unwrap().nbt)) + || old_slot.unwrap().item != cursor_item.0.as_ref().unwrap().item) { // assert that a swap occurs ensure!( @@ -225,9 +227,10 @@ pub(crate) fn validate_click_slot_item_duplication( let old_slot = window.slot(packet.slot_idx as u16); let new_slot = packet.slots[0].item.as_ref(); let is_transmuting = match (old_slot, new_slot) { - (Some(old_slot), Some(new_slot)) => { - old_slot.item != new_slot.item || old_slot.nbt != new_slot.nbt - } + // TODO: make sure NBT is the same + // Sometimes, the client will add nbt data to an item if it's missing, like "Damage" + // to a sword + (Some(old_slot), Some(new_slot)) => old_slot.item != new_slot.item, (_, None) => false, (None, Some(_)) => true, }; From 5de9190f10941230595854cc84fc1717199f5c3d Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Thu, 23 Mar 2023 13:19:12 -0400 Subject: [PATCH 43/49] fix some assertions --- crates/valence/src/inventory/validate.rs | 37 ++++++++++++------------ 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 7ca1ed493..d7976c722 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -29,30 +29,29 @@ pub(crate) fn validate_click_slot_impossible( // check all slot ids and item counts are valid ensure!( packet.slots.iter().all(|s| { - (0..=max_slot).contains(&(s.idx as u16)) - && s.item.as_ref().map_or(true, |i| { - (1..=i.item.max_stack().max( - window - .slot(s.idx as u16) - .as_ref() - .map(|c| c.count()) - .unwrap_or(0), - )) - .contains(&i.count()) - }) + if !(0..=max_slot).contains(&(s.idx as u16)) { + return false; + } + if let Some(slot) = s.item.as_ref() { + let max_stack_size = slot.item.max_stack().max(slot.count()); + if !(1..=max_stack_size).contains(&slot.count()) { + return false; + } + } + + true }), "invalid slot ids or item counts" ); // check carried item count is valid - ensure!( - packet.carried_item.as_ref().map_or(true, |i| (1..=i - .item - .max_stack() - .max(cursor_item.0.as_ref().map(|c| c.count()).unwrap_or(0))) - .contains(&i.count())), - "invalid carried item count" - ); + if let Some(carried_item) = &packet.carried_item { + let max_stack_size = carried_item.item.max_stack().max(carried_item.count()); + ensure!( + (1..=max_stack_size).contains(&carried_item.count()), + "invalid carried item count" + ); + } match packet.mode { ClickMode::Click => { From 5a2b2a1107d3e0b21f856698bb94c36a0615188d Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Thu, 23 Mar 2023 13:20:25 -0400 Subject: [PATCH 44/49] add a couple more new unit tests --- crates/valence/src/inventory/validate.rs | 55 ++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index d7976c722..b5d82a623 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -710,4 +710,59 @@ mod test { )); } } + + #[test] + fn allow_shift_click_overflow_to_new_stack() { + let mut player_inventory = Inventory::new(InventoryKind::Player); + player_inventory.set_slot(9, ItemStack::new(ItemKind::Diamond, 64, None)); + player_inventory.set_slot(36, ItemStack::new(ItemKind::Diamond, 32, None)); + let cursor_item = CursorItem::default(); + + let packet = ClickSlotC2s { + window_id: 0, + state_id: VarInt(2), + slot_idx: 9, + button: 0, + mode: ClickMode::ShiftClick, + slots: vec![ + Slot { + idx: 37, + item: Some(ItemStack::new(ItemKind::Diamond, 32, None)), + }, + Slot { + idx: 36, + item: Some(ItemStack::new(ItemKind::Diamond, 64, None)), + }, + Slot { idx: 9, item: None }, + ], + carried_item: None, + }; + + validate_click_slot_impossible(&packet, &player_inventory, None, &cursor_item) + .expect("packet should be valid"); + validate_click_slot_item_duplication(&packet, &player_inventory, None, &cursor_item) + .expect("packet should pass item duplication check"); + } + + #[test] + fn allow_pickup_overfull_stack_click() { + let mut player_inventory = Inventory::new(InventoryKind::Player); + player_inventory.set_slot(9, ItemStack::new(ItemKind::Apple, 100, None)); + let cursor_item = CursorItem::default(); + + let packet = ClickSlotC2s { + window_id: 0, + state_id: VarInt(2), + slot_idx: 9, + button: 0, + mode: ClickMode::Click, + slots: vec![Slot { idx: 9, item: None }], + carried_item: Some(ItemStack::new(ItemKind::Apple, 100, None)), + }; + + validate_click_slot_impossible(&packet, &player_inventory, None, &cursor_item) + .expect("packet should be valid"); + validate_click_slot_item_duplication(&packet, &player_inventory, None, &cursor_item) + .expect("packet should pass item duplication check"); + } } From e7c43b3b44cf380a3fdf4bfc4fc388769b122efc Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Thu, 23 Mar 2023 13:23:24 -0400 Subject: [PATCH 45/49] clamp max stack size to actual max stack size --- crates/valence/src/inventory/validate.rs | 12 ++++++++++-- crates/valence_protocol/src/item.rs | 4 ++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index b5d82a623..badcb1f4d 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -33,7 +33,11 @@ pub(crate) fn validate_click_slot_impossible( return false; } if let Some(slot) = s.item.as_ref() { - let max_stack_size = slot.item.max_stack().max(slot.count()); + let max_stack_size = slot + .item + .max_stack() + .max(slot.count()) + .min(valence_protocol::item::STACK_MAX); if !(1..=max_stack_size).contains(&slot.count()) { return false; } @@ -46,7 +50,11 @@ pub(crate) fn validate_click_slot_impossible( // check carried item count is valid if let Some(carried_item) = &packet.carried_item { - let max_stack_size = carried_item.item.max_stack().max(carried_item.count()); + let max_stack_size = carried_item + .item + .max_stack() + .max(carried_item.count()) + .min(valence_protocol::item::STACK_MAX); ensure!( (1..=max_stack_size).contains(&carried_item.count()), "invalid carried item count" diff --git a/crates/valence_protocol/src/item.rs b/crates/valence_protocol/src/item.rs index 002d1797c..2963b08cb 100644 --- a/crates/valence_protocol/src/item.rs +++ b/crates/valence_protocol/src/item.rs @@ -16,8 +16,8 @@ pub struct ItemStack { pub nbt: Option, } -const STACK_MIN: u8 = 1; -const STACK_MAX: u8 = 127; +pub const STACK_MIN: u8 = 1; +pub const STACK_MAX: u8 = 127; impl ItemStack { pub fn new(item: ItemKind, count: u8, nbt: Option) -> Self { From 5406f8a26e856a1f70225b9ba5a05e16f7fd662a Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Thu, 23 Mar 2023 13:35:31 -0400 Subject: [PATCH 46/49] fix assertion so `allow_place_overfull_stack_click` passes --- crates/valence/src/inventory/validate.rs | 40 +++++++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index badcb1f4d..7e0fe2bbe 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -170,11 +170,17 @@ pub(crate) fn validate_click_slot_item_duplication( // TODO: make sure NBT is the same // Sometimes, the client will add nbt data to an item if it's missing, like // "Damage" to a sword - if packet.button == 0 - && (old_slot.is_none() - || cursor_item.0.is_none() - || old_slot.unwrap().item != cursor_item.0.as_ref().unwrap().item) - { + let should_swap = packet.button == 0 + && match (old_slot, cursor_item.0.as_ref()) { + (Some(old_slot), Some(cursor_item)) => old_slot.item != cursor_item.item, + (Some(_), None) => true, + (None, Some(cursor_item)) => { + cursor_item.count() <= cursor_item.item.max_stack() + } + (None, None) => false, + }; + + if should_swap { // assert that a swap occurs ensure!( old_slot == packet.carried_item.as_ref() @@ -773,4 +779,28 @@ mod test { validate_click_slot_item_duplication(&packet, &player_inventory, None, &cursor_item) .expect("packet should pass item duplication check"); } + + #[test] + fn allow_place_overfull_stack_click() { + let player_inventory = Inventory::new(InventoryKind::Player); + let cursor_item = CursorItem(Some(ItemStack::new(ItemKind::Apple, 100, None))); + + let packet = ClickSlotC2s { + window_id: 0, + state_id: VarInt(2), + slot_idx: 9, + button: 0, + mode: ClickMode::Click, + slots: vec![Slot { + idx: 9, + item: Some(ItemStack::new(ItemKind::Apple, 64, None)), + }], + carried_item: Some(ItemStack::new(ItemKind::Apple, 36, None)), + }; + + validate_click_slot_impossible(&packet, &player_inventory, None, &cursor_item) + .expect("packet should be valid"); + validate_click_slot_item_duplication(&packet, &player_inventory, None, &cursor_item) + .expect("packet should pass item duplication check"); + } } From 31a17a656f8b8bbb68d133d444986104cc615ef9 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Thu, 23 Mar 2023 13:47:24 -0400 Subject: [PATCH 47/49] fix assertions for shift clicking --- crates/valence/src/inventory/validate.rs | 49 ++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 7e0fe2bbe..055d99042 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -1,4 +1,4 @@ -use anyhow::ensure; +use anyhow::{bail, ensure}; use valence_protocol::packet::c2s::play::click_slot::ClickMode; use valence_protocol::packet::c2s::play::ClickSlotC2s; @@ -198,10 +198,53 @@ pub(crate) fn validate_click_slot_item_duplication( } } } - ClickMode::ShiftClick | ClickMode::Hotbar => { + ClickMode::ShiftClick => { + ensure!( + (2..=3).contains(&packet.slots.len()), + "shift click must modify 2 or 3 slots, got {}", + packet.slots.len() + ); + + let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); + ensure!( + count_deltas == 0, + "invalid item delta: expected 0, got {}", + count_deltas + ); + + let Some(item_kind) = packet + .slots + .iter() + .filter_map(|s| s.item.as_ref()) + .next() + .map(|s| s.item) else { + bail!("shift click must move an item"); + }; + + let Some(old_slot_kind) = window.slot(packet.slot_idx as u16).map(|s| s.item) else { + bail!("shift click must move an item"); + }; + ensure!( + old_slot_kind == item_kind, + "shift click must move the same item kind as modified slots" + ); + + // assert all moved items are the same kind + ensure!( + packet + .slots + .iter() + .filter_map(|s| s.item.as_ref()) + .all(|s| s.item == item_kind), + "shift click must move the same item kind" + ); + } + + ClickMode::Hotbar => { ensure!( packet.slots.len() == 2, - "shift click and hotbar swaps must modify exactly two slots" + "hotbar swap must modify two slots, got {}", + packet.slots.len() ); let count_deltas = calculate_net_item_delta(packet, &window, cursor_item); From 2759d3d1191b1d3a3db90656e103eba275908aa7 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Thu, 23 Mar 2023 13:50:33 -0400 Subject: [PATCH 48/49] update call site --- crates/valence/src/client/event.rs | 9 +++------ crates/valence/src/inventory/validate.rs | 3 --- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/crates/valence/src/client/event.rs b/crates/valence/src/client/event.rs index f03cde0e1..9540cb7c5 100644 --- a/crates/valence/src/client/event.rs +++ b/crates/valence/src/client/event.rs @@ -867,12 +867,9 @@ fn handle_one_packet( .open_inventory .as_ref() .and_then(|open| inventories.get_mut(open.entity).ok()); - if let Err(msg) = crate::inventory::validate_click_slot_impossible( - &p, - &q.inventory, - open_inv, - &q.cursor_item, - ) { + if let Err(msg) = + crate::inventory::validate_click_slot_impossible(&p, &q.inventory, open_inv) + { debug!( "client {:#?} invalid click slot packet: \"{}\" {:#?}", q.entity, msg, p diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index 055d99042..f4b3e63a9 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -10,7 +10,6 @@ pub(crate) fn validate_click_slot_impossible( packet: &ClickSlotC2s, player_inventory: &Inventory, open_inventory: Option<&Inventory>, - cursor_item: &CursorItem, ) -> anyhow::Result<()> { ensure!( (packet.window_id == 0) == open_inventory.is_none(), @@ -24,8 +23,6 @@ pub(crate) fn validate_click_slot_impossible( None => player_inventory.slot_count(), }; - let window = InventoryWindow::new(player_inventory, open_inventory); - // check all slot ids and item counts are valid ensure!( packet.slots.iter().all(|s| { From 3d03b41ce2d6e995a8bdcd7507bc6f06db006175 Mon Sep 17 00:00:00 2001 From: Carson McManus Date: Thu, 23 Mar 2023 13:52:54 -0400 Subject: [PATCH 49/49] fix call sites in unit tests --- crates/valence/src/inventory/validate.rs | 59 +++++++----------------- 1 file changed, 17 insertions(+), 42 deletions(-) diff --git a/crates/valence/src/inventory/validate.rs b/crates/valence/src/inventory/validate.rs index f4b3e63a9..a1ff1902d 100644 --- a/crates/valence/src/inventory/validate.rs +++ b/crates/valence/src/inventory/validate.rs @@ -463,7 +463,7 @@ mod test { carried_item: inventory.slot(0).cloned(), }; - validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory), &cursor_item) + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) .expect("packet should be valid"); validate_click_slot_item_duplication( &packet, @@ -506,13 +506,8 @@ mod test { carried_item: None, }; - validate_click_slot_impossible( - &packet1, - &player_inventory, - Some(&inventory1), - &cursor_item, - ) - .expect("packet should be valid"); + validate_click_slot_impossible(&packet1, &player_inventory, Some(&inventory1)) + .expect("packet should be valid"); validate_click_slot_item_duplication( &packet1, &player_inventory, @@ -521,13 +516,8 @@ mod test { ) .expect("packet should not fail item duplication check"); - validate_click_slot_impossible( - &packet2, - &player_inventory, - Some(&inventory2), - &cursor_item, - ) - .expect("packet should be valid"); + validate_click_slot_impossible(&packet2, &player_inventory, Some(&inventory2)) + .expect("packet should be valid"); validate_click_slot_item_duplication( &packet2, &player_inventory, @@ -556,7 +546,7 @@ mod test { carried_item: Some(ItemStack::new(ItemKind::Diamond, 20, None)), }; - validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory), &cursor_item) + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) .expect("packet should be valid"); validate_click_slot_item_duplication( &packet, @@ -586,7 +576,7 @@ mod test { carried_item: Some(ItemStack::new(ItemKind::IronIngot, 2, None)), }; - validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory), &cursor_item) + validate_click_slot_impossible(&packet, &player_inventory, Some(&inventory)) .expect("packet should be valid"); validate_click_slot_item_duplication( &packet, @@ -647,13 +637,8 @@ mod test { carried_item: None, }; - validate_click_slot_impossible( - &packet1, - &player_inventory, - Some(&inventory1), - &cursor_item, - ) - .expect("packet 1 should be valid"); + validate_click_slot_impossible(&packet1, &player_inventory, Some(&inventory1)) + .expect("packet 1 should be valid"); validate_click_slot_item_duplication( &packet1, &player_inventory, @@ -662,13 +647,8 @@ mod test { ) .expect_err("packet 1 should fail item duplication check"); - validate_click_slot_impossible( - &packet2, - &player_inventory, - Some(&inventory2), - &cursor_item, - ) - .expect("packet 2 should be valid"); + validate_click_slot_impossible(&packet2, &player_inventory, Some(&inventory2)) + .expect("packet 2 should be valid"); validate_click_slot_item_duplication( &packet2, &player_inventory, @@ -677,13 +657,8 @@ mod test { ) .expect_err("packet 2 should fail item duplication check"); - validate_click_slot_impossible( - &packet3, - &player_inventory, - Some(&inventory1), - &cursor_item, - ) - .expect("packet 3 should be valid"); + validate_click_slot_impossible(&packet3, &player_inventory, Some(&inventory1)) + .expect("packet 3 should be valid"); validate_click_slot_item_duplication( &packet3, &player_inventory, @@ -756,7 +731,7 @@ mod test { ]; for (i, packet) in packets.iter().enumerate() { - validate_click_slot_impossible(packet, &player_inventory, None, &cursor_item) + validate_click_slot_impossible(packet, &player_inventory, None) .unwrap_or_else(|e| panic!("packet {i} should be valid: {e}")); validate_click_slot_item_duplication(packet, &player_inventory, None, &cursor_item) .expect_err(&format!( @@ -792,7 +767,7 @@ mod test { carried_item: None, }; - validate_click_slot_impossible(&packet, &player_inventory, None, &cursor_item) + validate_click_slot_impossible(&packet, &player_inventory, None) .expect("packet should be valid"); validate_click_slot_item_duplication(&packet, &player_inventory, None, &cursor_item) .expect("packet should pass item duplication check"); @@ -814,7 +789,7 @@ mod test { carried_item: Some(ItemStack::new(ItemKind::Apple, 100, None)), }; - validate_click_slot_impossible(&packet, &player_inventory, None, &cursor_item) + validate_click_slot_impossible(&packet, &player_inventory, None) .expect("packet should be valid"); validate_click_slot_item_duplication(&packet, &player_inventory, None, &cursor_item) .expect("packet should pass item duplication check"); @@ -838,7 +813,7 @@ mod test { carried_item: Some(ItemStack::new(ItemKind::Apple, 36, None)), }; - validate_click_slot_impossible(&packet, &player_inventory, None, &cursor_item) + validate_click_slot_impossible(&packet, &player_inventory, None) .expect("packet should be valid"); validate_click_slot_item_duplication(&packet, &player_inventory, None, &cursor_item) .expect("packet should pass item duplication check");