search

multitheftauto / mtasa-resources

This project maintains a list of up-to-date resources that come with Multi Theft Auto.
https://multitheftauto.com
MIT License
151 stars 151 forks source link

Check the client for permission instead of the event source #300

Closed NanoBob closed 3 years ago

NanoBob commented 3 years ago

This PR changes permission checks to be done on the global client variable instead of on the event source. Since the event source is not guaranteed to be the player that triggered the event. (Think modified clients / injected Lua code)

Some detections are already in place to verify the client and source are equal, but with default configuration these are not enabled.
Furthermore there really is no reason to use source over client in this case since client is always the correct client.

Dutchman101 commented 3 years ago

This causes a change in behavior with some commands (popular across gamemodes, including the default freeroam) that are listed in ACL

Example from chatbox: Access denied for 'repair'

NanoBob commented 3 years ago

There's no reason that should be the case, all this change does is check if the actual client that triggered an event has permission todo so. This would only have an impact in case the event is triggered from a client with a source other than that client's local player.

Furthermore freeroam has its own repair command which is completely unrelated to the admin resource: https://github.com/multitheftauto/mtasa-resources/blob/2c6416a09697b9d8b812c182cff0955b09d753c5/%5Bgameplay%5D/freeroam/fr_client.lua#L1502

So unless any other gamemode were to trigger admin events, supplying a different element than the localplayer (one which does have the required admin access) as source, this change will not impact any such gamemode.

What I do see happening is that the admin panel itself triggers this event through commands (which arguably should not be done in this way, but legacy code..) I'll add an additional update to fall back onto the event source, only if client is nil (or otherwise falsey).

Dezash commented 3 years ago

I think it would be better to separate the functions that are called by these event handlers so that the permission check wouldn't be needed at all if the function is called directly. You could then replace serverside triggerEvent with function calls.

NanoBob commented 3 years ago

I do agree that it would indeed make far more sense for commands and events to be two separate ways to trigger the same function. (Instead of commands using the same events clients use). However I do believe that is out of scope of this PR, since you're talking about a refactor (albeit a good one), while this is PR is about a pretty big security issue.

patrikjuvonen commented 3 years ago

So, did this change break other default resources or no? I expect fixes to them where needed.

xLive commented 3 years ago

So, did this change break other default resources or no? I expect fixes to them where needed.

No, it shouldn't affect other default resources.