Exploitable Server RPCs

[leetNightshade]

If you want your plugin to potentially be more widely used and taken seriously, I would evaluate what I'm calling out here.

This project has bad security implications out of the box if used as-is, in regards to hacked clients and cheaters.

If someone were to use this plugin in an MMO, your server RPC's are user accessible and player's can freely add items they want to their inventory, and can also increase the quantity of items they want willy nilly.

Your interfaces such as AddItem_Server and ChangeItemQuantity_Server should not be server RPCs (that can be called by the client), they are exploitable by a hacked client and cheaters. Considering I spotted that, if I actually wanted to use any of your plugins, I'd have to thoroughly audit the code and modify interfaces like that.

Any interface a client can call cannot be trusted. If the server wants to modify the inventory by adding items and modifying quantity, it should be called by another server function dealing with gathering items, item transfers, or what have you.

I call these out because we had some exploitable server RPCs when developing Palia, but thankfully they weren't too bad and we were able to fix them quickly early on. So far it's an impressive looking plugin though, I hope you don't mind me calling this out.

[pavlicekdominik]

Hello, and thank you for this! The Inventory & Equipment is currently in development, so all code that you see is very much experimental.

I do not have any major experience with property replication security. I would be interested in any suggestions and ideas on how to make the system more reliable and less prone to fail under cheat/attacks.

If you have any ideas in mind, feel free to share (merging fixes in a branch or join Discord and discuss there). I will be happy for any help!

[leetNightshade]

I would just caution for any Server RPC's, UFunction(Server), to be careful what you put that on where operations are done that users can abuse without server validation. Since this is a generic plugin, you can't validate whether or not a user should be able to rpc add/remove/change quantity; that's really up to server side logic to determine.

Unless you're 100% sure a server function needs to be an RPC , it's safer to not add it. Leave it up to users of your plugin to call it server side. And if they're really sure they want such an RPC, they can add one.

If I go further into the rabbit hole using your plugin, I may pop a PR your way.

Happy developing!

[leetNightshade]

From what I noted, making such changes might not be easy.

Looking at one example, let's say AddItem_Server(...) and AddItem_Implementation(...). Okay clearly the latter is intended on being a server function. So removing AddItem_Server(...) entirely might suffice in that case. User would have to add item via another mechanism, they can't add items directly themselves.

[pavlicekdominik]

Sorry for so late reply, I have been on business trip.

I have this simple idea:

1. Keep the responsibility on the developer for release of first version with the CanAddItem method
2. Once system is established and stable, implement advanced queue for requests to be evaluated on server side

Would that be aligned with your idea?