Improve client events

[rj00a]

Is your feature request related to a problem? Please describe.

Currently, most client events are a thin layer above the serverbound packets without any kind of filtering or validation applied. This is bad because:

• Too many low-level details are expected to be dealt with, leading to boilerplate code.
• Leaves users vulnerable to gameplay exploits if events are not properly sanitized.

Describe the solution you'd like

• Sanitize events before emitting them. Don't emit an event if they fail the check. Examples:

  • Entity interactions must be a certain distance from the player. Interactions with nonexistent entities are ignored.
  • Block breaking time in survival mode is measured.
  • Players should not be able to fly or move too quickly.
  • Players should not be able to clip into or though blocks.
  • Inventory interactions cannot create/destroy items.
  • Slot indices of inventory interactions are within bounds.

  The vanilla server should be used as a reference (See ServerPlayNetworkHandler.java). If desired, the checks can be disabled with a config option.

• Remove low-level details from events. Examples:

  • Entity interactions include the Entity of the McEntity that was interacted with.
  • Movement packets are combined into a single event.
  • Inventory events include the inventory that was interacted with.
    • The inventory module may need to be redesigned to handle inventory packets before the user stage.
  • Combine packets together where it makes sense. Separate them where it doesn't.

In cases where direct access to packets is needed, we can accept a callback taking a &C2sPlayPacket as an argument.

Blocked on:

• Refactor valence_protocol #253

Additional context

Minestom's events

[JadedBlueEyes]

I think that generally this is a really good idea, and it's even slowly happening a bit already - #252 merged the drop events. 😍

I'm a bit wary about the performance implications of automatically pulling in the actual entities with the events though, especially when they might not even get used. Perhaps adding convenience methods might be better? 🙂

Regarding event sanitisation, I think that's an awesome idea. However
a) I think that it must be designed as extensibility as a first priority, as minigame servers often don't comply with Vanilla mechanics and/or want to implement their own customised anticheat.
b) I think the Vanilla code should be ignored - it's pretty much useless. It fails to do a thing about cheats in common clients, like Wurst.
Instead it should probably be modeled more on the carefully designed and modular NCP - https://github.com/Updated-NoCheatPlus/NoCheatPlus

In any case, I really like this! Great to see! 👍

[dyc3]

@JadedBlueEyes Pulling in Entity will be essentially free. In the ECS architecture, Entity is just an identifier. If you don't need the McEntity, you don't query for it.

[JadedBlueEyes]

Thank you! Excuse my ignorance - this project is the first time I've used Bevy!

Continuing on from my thoughts on anticheat, I think that any kind of anticheat, especially one that is effective, is likely to be a significant endeavor in itself no matter how we go about doing it. It’s probably best to work in stages - start with simple checks (is this person self-hitting, has this person been breaking this block for roughly the right amount of time) to more complex ones that require guessing the client's state (is this person actually looking at this entity).

(Sort for the brain vomit - stream-of-thought queen here!)

[rj00a]

Continuing on from my thoughts on anticheat, I think that any kind of anticheat, especially one that is effective, is likely to be a significant endeavor in itself no matter how we go about doing it.

I agree. The checks here should be enough to prevent some of the most egregious exploits like teleporting around arbitrarily, attacking every entity in the entire world, etc. I expect advanced users will need to disable these checks, but this should provide a reasonable default for regular users. Unfortunately there are no perfect solutions and anticheat is very "context sensitive".

Pulling in Entity will be essentially free. In the ECS architecture, Entity is just an identifier. If you don't need the McEntity, you don't query for it.

A hashmap lookup is needed to map the entity ID to an Entity, but I don't expect this will be a problem. This is also necessary to check that the entity actually exists.