Rough click slot packet validation

[dyc3]

Description

This adds some validation for incoming inventory packets that makes it so that you can't just spawn items by sending malicious packets. It adds type 1 and type 2 validations as outlined in #292.

This also adds some new helpers, InventoryWindow and InventoryWindowMut.

fixes #292

Playground

use valence::client::{default_event_handler, despawn_disconnected_clients};
use valence::prelude::event::PlayerInteractBlock;
use valence::prelude::*;

#[allow(unused_imports)]
use crate::extras::*;

const SPAWN_Y: i32 = 64;
const CHEST_POS: [i32; 3] = [0, SPAWN_Y + 1, 3];

pub fn build_app(app: &mut App) {
    app.add_plugin(ServerPlugin::new(()).with_connection_mode(ConnectionMode::Offline))
        .add_startup_system(setup)
        .add_system(default_event_handler.in_schedule(EventLoopSchedule))
        .add_system(init_clients)
        .add_system(despawn_disconnected_clients)
        .add_systems((toggle_gamemode_on_sneak, open_chest).in_schedule(EventLoopSchedule));
}

fn setup(mut commands: Commands, server: Res<Server>) {
    let mut instance = server.new_instance(DimensionId::default());

    for z in -5..5 {
        for x in -5..5 {
            instance.insert_chunk([x, z], Chunk::default());
        }
    }

    for z in -25..25 {
        for x in -25..25 {
            instance.set_block([x, SPAWN_Y, z], BlockState::GRASS_BLOCK);
        }
    }
    instance.set_block(CHEST_POS, BlockState::CHEST);

    commands.spawn(instance);

    let mut inventory = Inventory::new(InventoryKind::Generic9x3);
    inventory.set_slot(0, ItemStack::new(ItemKind::Apple, 100, None));
    inventory.set_slot(1, ItemStack::new(ItemKind::Diamond, 40, None));
    inventory.set_slot(2, ItemStack::new(ItemKind::Diamond, 30, None));
    commands.spawn(inventory);
}

fn init_clients(
    mut clients: Query<(&mut Position, &mut Location, &mut Inventory), Added<Client>>,
    instances: Query<Entity, With<Instance>>,
) {
    for (mut pos, mut loc, mut inv) in &mut clients {
        pos.0 = [0.5, SPAWN_Y as f64 + 1.0, 0.5].into();
        loc.0 = instances.single();

        inv.set_slot(24, ItemStack::new(ItemKind::Apple, 100, None));
        inv.set_slot(25, ItemStack::new(ItemKind::Apple, 10, None));
    }
}

// Add new systems here!

fn open_chest(
    mut commands: Commands,
    inventories: Query<Entity, (With<Inventory>, Without<Client>)>,
    mut events: EventReader<PlayerInteractBlock>,
) {
    let Ok(inventory) = inventories.get_single() else {
        return;
    };

    for event in events.iter() {
        if event.position != CHEST_POS.into() {
            continue;
        }
        let open_inventory = OpenInventory::new(inventory);
        commands.entity(event.client).insert(open_inventory);
    }
}

Test Plan

Steps:

1. cargo test

[rj00a]

Getting some validation failures when splitting stacks with right clicks and shift-clicking stacks to/from the chest in the chest example.

[rj00a]

I'm getting Client state id mismatch, resyncing when adding items to my inventory with code, switching to survival, and clicking on it. (But no validation errors when doing that with pickaxes? Not sure what's going on here).

[dyc3]

It turns out the client will actually add NBT data to clicked items in some circumstances.

Ugh, that's really annoying. Do we extract data for what nbt data an item is supposed to have?

[rj00a]

Do we extract data for what nbt data an item is supposed to have?

Not yet.

[dyc3]

Would you be opposed to loosening the assertions on nbt data so we can defer this problem for later?

[rj00a]

How about we leave it as a TODO for now. Maybe make note of it in the issue.

[rj00a]

Still getting the Client state id mismatch, resyncing as mentioned above. (Not sure if you're finished with this)

[rj00a]

Also doesn't look like the item count check is working properly.

client 2v1 invalid click slot packet: "invalid carried item count" ClickSlotC2s {
    window_id: 0,
    state_id: VarInt(
        2,
    ),
    slot_idx: 24,
    button: 0,
    mode: Click,
    slots: [
        Slot {
            idx: 24,
            item: None,
        },
    ],
    carried_item: Some(
        ItemStack {
            item: Apple,
            count: 77,
            nbt: None,
        },
    ),
}

(I added a stack of 77 apples to the inventory)

[dyc3]

I'm not able to reproduce that. I am getting different issues though, like I can't manually set a stack of 100 apples, it just shows up as 64 in the client. If i apply it to the cursor item instead, it works? Not sure what's up with that.

Can you list out all the steps to repro using the playground I added to the PR?

[rj00a]

I made an edit to the playground. When you click on the apple stacks in the player's inventory you'll see the issues manifest.

[dyc3]

Ok, I fixed the issue you were talking about, but from now on I think we should consider behavior related to overfull stacks (eg stacks that have amount > max stack) to be undefined given that the notchian client is inconsistent in those scenarios.

[rj00a]

but from now on I think we should consider behavior related to overfull stacks (eg stacks that have amount > max stack) to be undefined given that the notchian client is inconsistent in those scenarios.

Yeah agree. We could clamp the stack size in ItemStack::new but I don't think that will work if we move to using dynamic registries.

[rj00a]

• Still seeing the refresh when clicking on one of the apple stacks in the player inventory for the fist time. It only happens when the chest is not open. Guessing a variable wasn't initialized properly somewhere.

[dyc3]

I can repro this bug with the item dupe check turned off, so it's unrelated to this PR. I'll open another issue for it. See #304