-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(onesync/server): basic state bag write policies #2257
feat(onesync/server): basic state bag write policies #2257
Conversation
* Basic rejection logic for write-protected keys in player/entity state bags. * Resync replicated state bags with the source client (on rejection). * Native handler 'SET_STATE_BAG_KEY_POLICY' to allow setting write policies per key.
4a7d7e5
to
dbdc89c
Compare
I'm changing this into a draft, as this feature requires more design work and consider more use-cases before it's a feature we can keep supporting in the future. Some internal points I have on this, to consider and to give context:
|
Hey @thorium-cfx, thank you for noting the internal points you have on this here. While trying to come up with an implementation for most of your proposed features/policies, I've come across one major concern that's hard to solve:
Also, I do have to disagree with some of the proposed policy scopes: IMO, the disagreement about policy scopes comes from the fact that this PR only exists to solve the "main" reason this feature is so anticipated: Having server-authoritative player/entity state bags. Reading comments about this feature on the forums, the Discord guild, or here shows that the "main" reason users want this feature is to implement use cases like: I do agree that broader policy scopes like per state bag category make sense, but other than that I don't see the need for the other proposed policy options. |
I think there should be a way to go back to the default way how state bags worked before, where only the server could create state bags, and also have a way to default everything to be server write, client read unless set otherwise for the state bag. This would be the most sane security default but I don't think it can be swapped easily without breaking compatibility so it would be good to just have a "sane default" ConVar.
I think @tens0rfl0w hit this on the head, clients should never be allowed to write to global state. As for the disallowing clients to read player/entity/global state the simplest solution would to probably be to have an enum. enum eStateSyncFlags {
NoFlags = 0, // default flag
OwnerOnly = 1, // only sync the state to the owning client
NoSync = 2, // only the server should ever know this value, and it shouldn't ever be sync'd to clients
ReadOnly = 4, // this would reject any change made by the client before ever being sent to the server
// more if needed later
}
I don't think this should be under consideration, the server is meant to be authoritative, nothing should be able to tell it its not able to touch anything.
I think more context is needed here @thorium-cfx (I'm assuming this might be related to other stuff i posted here already)
On this regard it might be worth swapping away from std::map as iirc @gottfriedleibniz mentioned this was causing some bad access times (though I can't find the comment right now). It might be work making our own way of handling data to maintain consistency, like maintaining ordering that state bags are set in or allow state bags to be assigned priority to be able to be used consistently to avoid some confusion
I think this is somewhat explained how I would think to do it elsewhere Another other issue that should probably be addressed #1854 |
So long as policies can be implemented in way future updates don't break backwards compatibility, I believe it's worth implementing what can easily be done right now. Right now there is nothing in place to prevent bad actors from creating a limitless number of statebag keys with enormous amounts of data.
I ran a simple function to spam statebags and managed to increase fxserver's memory use by 1gb, which cleared the moment I dropped and statebags were wiped. Considering the 100,000 states I added would then get synced to any player who entered my scope.. bad time for all. |
In this situation I'm not sure why the default was changed away from a server has to initialize the state bag to client can initialize any state bag it wants (which is the main reason this becomes an issue). |
Superseded by an internal proposal for a new state bag API. |
This introduces a basic rejection logic for replicated player/entity state bags.
"Use-case" should be any type of state bag usage where "truthful" data matters, for example syncing economy-related data.
Implementation:
SET_STATE_BAG_KEY_POLICY
to set a write policy for a certain state bag key.I originally implemented this on a per state-bag/entity/player -> key logic, but reconsidering the actual use cases for this type of feature this didn't make much sense.