Skip to content
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

Prevent entity gates from being used on players #2908

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

Aws0mee
Copy link
Contributor

@Aws0mee Aws0mee commented Nov 25, 2023

I noticed for some reason these gates 4 can be used on players to freeze them. Since they don't work on players anyway, I think the best fix would be to just disallow them from working on other players.

@Vurv78
Copy link
Contributor

Vurv78 commented Nov 25, 2023

Think this is an issue with your prop protection. It already checks if the gate owner can physgun the target player

local function isAllowed( gate, ent )
if not IsValid(gate:GetPlayer()) then return false end
return hook.Run( "PhysgunPickup", gate:GetPlayer(), ent ) ~= false
end

@Aws0mee
Copy link
Contributor Author

Aws0mee commented Nov 25, 2023

Think this is an issue with your prop protection. It already checks if the gate owner can physgun the target player

local function isAllowed( gate, ent )
if not IsValid(gate:GetPlayer()) then return false end
return hook.Run( "PhysgunPickup", gate:GetPlayer(), ent ) ~= false
end

Yeah, I did see that when I was looking. However, we had an incident today on my server where a trial mod (lowest rank staff member) had shared prop protection with a user. The user wired his applyforce gate to a target finder which selected every player on the map. This caused every player on the map (around 70) to get completely frozen and unable to move for 10-20 minutes until I could figure out what was going on. One could argue here that it comes down to "train your staff better" or "restrict permissions more", but they do kind of need permission to physgun players.

However, I noticed none of these gates seem to have any effect on players except for freezing them in place, so I think the optimal solution to this problem is to just disallow these gates from working on players. If we really want these gates to work on players it could be possible to fix the gate, and then call a less lenient hook like what I did in #2395.

Overall, in my opinion it doesn't make sense for these gates to not work on players and also allow for abuse.

@Vurv78
Copy link
Contributor

Vurv78 commented Nov 26, 2023

Think this is an issue with your prop protection. It already checks if the gate owner can physgun the target player

local function isAllowed( gate, ent )
if not IsValid(gate:GetPlayer()) then return false end
return hook.Run( "PhysgunPickup", gate:GetPlayer(), ent ) ~= false
end

Yeah, I did see that when I was looking. However, we had an incident today on my server where a trial mod (lowest rank staff member) had shared prop protection with a user. The user wired his applyforce gate to a target finder which selected every player on the map. This caused every player on the map (around 70) to get completely frozen and unable to move for 10-20 minutes until I could figure out what was going on. One could argue here that it comes down to "train your staff better" or "restrict permissions more", but they do kind of need permission to physgun players.

However, I noticed none of these gates seem to have any effect on players except for freezing them in place, so I think the optimal solution to this problem is to just disallow these gates from working on players. If we really want these gates to work on players it could be possible to fix the gate, and then call a less lenient hook like what I did in #2395.

Overall, in my opinion it doesn't make sense for these gates to not work on players and also allow for abuse.

Then just add the player check in here instead of each gate.

Me personally I'd even remove the IsValid check for the ent and move it into isAllowed, and move the isAllowed to the very top. Would deduplicate a lot of the code

@Aws0mee
Copy link
Contributor Author

Aws0mee commented Nov 26, 2023

Think this is an issue with your prop protection. It already checks if the gate owner can physgun the target player

local function isAllowed( gate, ent )
if not IsValid(gate:GetPlayer()) then return false end
return hook.Run( "PhysgunPickup", gate:GetPlayer(), ent ) ~= false
end

Yeah, I did see that when I was looking. However, we had an incident today on my server where a trial mod (lowest rank staff member) had shared prop protection with a user. The user wired his applyforce gate to a target finder which selected every player on the map. This caused every player on the map (around 70) to get completely frozen and unable to move for 10-20 minutes until I could figure out what was going on. One could argue here that it comes down to "train your staff better" or "restrict permissions more", but they do kind of need permission to physgun players.
However, I noticed none of these gates seem to have any effect on players except for freezing them in place, so I think the optimal solution to this problem is to just disallow these gates from working on players. If we really want these gates to work on players it could be possible to fix the gate, and then call a less lenient hook like what I did in #2395.
Overall, in my opinion it doesn't make sense for these gates to not work on players and also allow for abuse.

Then just add the player check in here instead of each gate.

Me personally I'd even remove the IsValid check for the ent and move it into isAllowed, and move the isAllowed to the very top. Would deduplicate a lot of the code

Fair enough, I was thinking the same thing honestly. Should I move all the other duplicated checks into isAllowed too then?

@Aws0mee
Copy link
Contributor Author

Aws0mee commented Nov 26, 2023

Actually, it looks like the only other check repeated in all cases is the check to validate the physics entity. However, it's used later on in most functions, so to avoid calling the function twice or adding another argument to the function I just left it be. Should be good to go now since it's such a basic change, however I didn't test it myself.

Copy link
Contributor

@Vurv78 Vurv78 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny that now I see these gates check if the input vectors are actual proper userdata, so that means E2 Angles and Vectors were completely broken for gates before my change to make them always userdata.

@thegrb93 thegrb93 merged commit d8b8af9 into wiremod:master Nov 27, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants