-
Notifications
You must be signed in to change notification settings - Fork 175
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
Fix negative values being accepted in ban commands (solve issue #949) #953
Conversation
You can keep it DRY with the existing code. if (!StringToIntEx(buffer, time) || time < 0)
{
ReplyToCommand(client, "%sUsage: sm_ban <#userid|name> <time|0> [reason]", Prefix);
return Plugin_Handled;
} Edit: Also please keep using tranlations, sourcebanspp is used by tons of users. |
Implemented this way for the CommandBan function, will add more new commits soon |
It should also apply to every changes you made. |
I don't see how it is possible in the other parts. The other parts don't print the right message after getting the int value of time. And since the minutes variable is used multiple times, wouldn't it be better there to just leave a separate check for if the time is negative or not? Next to that, I can add translations for this phrase. But only when all other issues with the code I provided are resolved. |
To avoid create new translations, we can just stick to the current command usage phrase. if (!StringToIntEx(time, minutes) || minutes < 0)
{
ReplyToCommand(client, "%sUsage: sm_banip <#userid|name> <time> [reason]", Prefix);
return Plugin_Handled;
} |
The ban commands now do not accept negative ban values anymore while keeping the code dry as possible.
Done, the approach is now used for all commands. |
By the way, should the sourcebans version be bumped? I decided not to do it, just in case. I can always add a commit if needed. |
LGTM. |
Description
The changes add a check on all ban commands to see whether the given duration is 0 or more. If the duration given is smaller than zero, it will tell the player and the ban will not proceed.
Motivation and Context
The motivation behind this pull request is that this fixes an open issue. Issue #949. According to the issue, banning with a negative time value is useless and only bans the player for the session. With this pull request banning with a negative value should not be possible anymore
How Has This Been Tested?
The changes have been tested on my own community server on a real player. When entering a negative value, we want the entire command to stop after giving the command issuer a warning. This approach should be valid and should not affect any other areas in the code. Values of 0 or higher have also been tested and these do get accepted by the command.
Screenshots (if appropriate):
Types of changes
Checklist: