Use strict number format for the /pay command #5507
Conversation
Essentials/src/main/java/com/earth2me/essentials/commands/Commandpay.java
Show resolved
Hide resolved
|
|
||
| import static com.earth2me.essentials.I18n.tl; | ||
|
|
||
| public class Commandpay extends EssentialsLoopCommand { | ||
| private static final Pattern NUMBER_FORMAT = Pattern.compile("([0-9][0-9_'`,]*(?:\\.[0-9]+)?|\\.[0-9]+)([kmbt])?"); | ||
| private static final Pattern SANITIZE = Pattern.compile("[^0-9.]"); |
There was a problem hiding this comment.
Need to confirm whether we ever supported commas as a decimal separator via the config options in the past. I don't think we did, but would like to be sure first
There was a problem hiding this comment.
Well, the current behavior is that 1,000.23 is currently being interpreted as 1000.23, so all I did was try to keep it this way.
There was a problem hiding this comment.
Yeah, that's going to be since anything aside from numbers and decimal places are ignored by the SANITIZE regex (which was just unnamed before). Comma was not specifically designated as a separator however (and there is another open issue for that, as based on server locale, technically 1,000.23 should be as equally valid as 1.000,23 for example). Doesn't necessarily need to be a concern for this PR though as long as the existing behavior is maintained.
|
Changed the Pattern to avoid nonsense like |
|
|
||
| import static com.earth2me.essentials.I18n.tl; | ||
|
|
||
| public class Commandpay extends EssentialsLoopCommand { | ||
| private static final Pattern NUMBER_FORMAT = Pattern.compile("([0-9][0-9_'`,]*(?:\\.[0-9]+)?|\\.[0-9]+)([kmbt])?"); | ||
| private static final Pattern SANITIZE = Pattern.compile("[^0-9.]"); |
There was a problem hiding this comment.
Yeah, that's going to be since anything aside from numbers and decimal places are ignored by the SANITIZE regex (which was just unnamed before). Comma was not specifically designated as a separator however (and there is another open issue for that, as based on server locale, technically 1,000.23 should be as equally valid as 1.000,23 for example). Doesn't necessarily need to be a concern for this PR though as long as the existing behavior is maintained.
|
|
||
| import static com.earth2me.essentials.I18n.tl; | ||
|
|
||
| public class Commandpay extends EssentialsLoopCommand { | ||
| private static final Pattern AMOUNT_FORMAT = Pattern.compile("((?:[0-9][_'`,]?)+(?:\\.[0-9]+)?|\\.[0-9]+)([kmbt])?"); |
There was a problem hiding this comment.
I have concerns that this regex isn't doing anything particularly useful here. It isn't strict enough to prevent non-numbers from being entered and introduces some assumptions as md mentioned about what a valid number separator is. Perhaps it would be "good enough" to say that there can't be any letters as part of the number except for the suffix? Then you remove the suffix part and sanitize the rest for the number part.
I'm also pretty sure the \\. here is probably intended to be \. since otherwise you're matching backslashes and not periods.
There was a problem hiding this comment.
Not sure what do you mean by "isn't strict enough to prevent non-numbers" tbh. The only concern I see here there is this one, but I don't think it's a major one.
As for allowing everything non-letter, I'm very 50/50 about it. E.g. if a server has a forum/discord/etc and somebody asks there to copy and use the command /pay 1ɢᴇm, it will be interpreted as 1m/1000000, since ɢᴇ are simply unicode characters.
I'm also pretty sure the
\\.here is probably intended to be\.since otherwise you're matching backslashes and not periods.
The reason why there's two backslashes is that Java requires backslash to be escaped. To match backslash you'll actually need to type four of those - \\\\.
There was a problem hiding this comment.
I'm referencing mostly what you mentioned above (e.g. 1,2_3`4'.5 being valid, but also stuff like 1,0,0,0.23). It feels like you would want to be stricter here if the goal is to block anything potentially ambiguous, but maybe I'm just nit picking too much. Probably unlikely to become an issue for anyone.
Information
This PR is related to #5495. The issue actually contains two issues, so can't say PR fixes it.
Details
Proposed fix:
Enforces more strict number format for the pay command. The difference with the #5496 approach is that this one also fixes cases like
12m34.56, which I don't think should be allowed. This also fixes the issue with inputs like123.456.789- previously Ess was just swallowing theNumberFormatExceptionand was showing untranslated exception message to the player.Environments tested:
OS: Linux Mint 21.2
Java version: openjdk 17.0.8 2023-07-18 LTS
Demonstration: