EssentialsX

EssentialsX

2M Downloads

Any Command That Uses EssentialsLoopCommand.java's Method loopOnlinePlayersConsumer Runs The Risk Of Guessing The Wrong Minecraft Player

TomyDoesThings opened this issue ยท 1 comments

commented

Type of bug

Other unexpected behaviour

/ess dump all output

https://essentialsx.net/dump.html?id=bbb3184dada84de4857b02b3b66daf04

Error log (if applicable)

https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/main/java/com/earth2me/essentials/Essentials.java#L995-L1018

What me and GitHub Copilot have to say:
The matchUser method is part of the IEssentials interface and is implemented in the Essentials class. This method is used to find a user based on a search term. It takes five parameters:

  • server: The server instance.
  • sourceUser: The user who is initiating the search.
  • searchTerm: The term to search for.
  • getHidden: A boolean value indicating whether to include hidden users in the search.
  • getOffline: A boolean value indicating whether to include offline users in the search.

The method throws a PlayerNotFoundException if no matching user is found. The method first tries to find an online player with the exact name or UUID matching the search term. If no such player is found, it tries to find an offline player with the exact name matching the search term. If still no player is found, it tries to find an online player whose name contains the search term, not necessarily starting with it. This is due to the use of displayName.contains(matchText).

However, it's important to note that the displayName.contains(matchText) check does not account for situations where more than one player's name contains the matchText. This could potentially lead to unexpected results, as the method does not specify which player it will return in such cases.

If all these attempts fail, it throws a PlayerNotFoundException. The method also checks if the found player is hidden and whether the source user is allowed to interact with the found player. If the found player is hidden and the source user is not allowed to interact with them, the method throws a PlayerNotFoundException.

This method is used in various places in the Essentials plugin to find a user based on a search term, for example when executing commands that require a target user.

Bug description

loopOnlinePlayersConsumer returns the first Username that has String searchTerm on the spot if the player is not online or does not exist, as shown in this code: https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/main/java/com/earth2me/essentials/commands/EssentialsLoopCommand.java#L122 which calls https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/main/java/com/earth2me/essentials/commands/EssentialsCommand.java#L125 where return ess.matchUser(server, sourceUser, searchTerm, getHidden, getOffline); ends up being called on Line 163 which leads to the source of the problem on the method it calls, which leads us to this particular code within that method https://github.com/EssentialsX/Essentials/blob/2.x/Essentials/src/main/java/com/earth2me/essentials/Essentials.java#L995-L1018.

In non-technical terms, whenever a player runs a command that EssentialsX processes which specifically involves inputting a Minecraft username of a player that is online (whether this be natively in the command or done after seeing no offline player exists), when the input is of a Minecraft username that does not exist or is not online, there are (enhanced) for loops done that return the first user that CONTAINS this input, but does not equal this input. Do note this seems to pertain specifically to if any command calls on the code you see in the Github link I put above. Considering multiple users can have a character or string of characters, I find this to be a bug.

If this is not a bug, I would like there to be a toggle where you can have safe online player search and not safe online player search. I can just imagine trying to run a command on somebody, them disconnecting, and then some random other guy receives the effect of the command. Or maybe I do a typo and an unintended online player gets the effect of the command. Please do not ignore this. I even went through the effort to find the code responsible for what is doing this.

Steps to reproduce

  1. Have at least 1 player online. In this example, let's use 4 players whose names are Alpha, Beta, Gamma, and Delta.

  2. Enter an EssentialsX command that involves typing an online Minecraft player's username, but write one of the names incredibly incomplete. The bug worsens and reveals itself more if the string you type is apparent in more than 1 of the online players. In this example we will give the worst case scenario with a character apparent in all 4 of the online player's names: /give a dirt.

  3. You will notice that unpredictably, one of those 4 random online players receives the dirt. This random player that gets the dirt is fully determined by the order of the List returned by the server.

Expected behaviour

Upon entering in an EssentialsX command that involves the argument of an online player's username, if no online player has the EXACT username (case-insensitive) that was entered into the command,

let the player know that there exists no such online player on the server. PlayerNotFoundException could perhaps be thrown.

Actual behaviour

Upon entering in an EssentialsX command that involves the argument of an online player's username, if no online player has the EXACT username (case-insensitive) that was entered into the command,

the first random online player the server finds that has that inputted string anywhere within their username gets the command's effect. Let's bring up the example again. Let's say Alpha, Beta, Gamma, and Delta are online players. Doing /give a dirt is pointing to any one of them randomly. Doing /give m dirt points to Gamma. Doing /give mm dirt points to Gamma. Doing /give ta dirt is pointing randomly to either Beta or Delta.

The easiest fix that would have the least chance of breaking the code would be to make if (displayName.contains(matchText)) be if (displayName.equalsIgnoreCase(matchText)) and if (userMatch.getDisplayName().startsWith(searchTerm) && (getHidden || canInteractWith(sourceUser, userMatch))) be if (userMatch.getDisplayName().equalsIgnoreCase(searchTerm) && (getHidden || canInteractWith(sourceUser, userMatch))).

/give a dirt and /give ta dirt especially are devastating.

commented

This is not a bug, the current behavior is meant to guess the username when you don't specify it exactly. There are times when this is not super ideal which is understandable - in those cases you can easily avoid this by specifying the full username in the command.

There have been discussions around changing this behavior, if you would like to chat with us about your ideas please join the EssentialsX Development Discord server and consider writing a pull request.

Feel free to open a new non-bug "feature request" issue for this as well, but please avoid using AI to automatically generate a summary as this does not provide any useful information for us.