Cog-Creators / Red-DiscordBot

A multi-function Discord bot
https://docs.discord.red
GNU General Public License v3.0
4.69k stars 2.3k forks source link

Greedy can mistake part of reason as a member in `unmute` #6236

Open ghost opened 1 year ago

ghost commented 1 year ago

What Red version are you using?

3.5.4

Cog name

Mutes

Command name

mute, unmute, and other commands using a Greedy Member converter

What did you expect to happen?

Only the intended member will be unmuted

What actually happened?

The intended member and another unrelated member gets unmuted

How can we reproduce this error?

  1. Ensure the mutes cog is loaded
  2. Ensure you have the privileges to mute and unmute members
  3. Mute a member
  4. Unmute the said member with a reason that partially resembles the name of another member who isn't intentionally being unmuted

Anything else?

This problem was discovered by the discord user codzombiestm. The original discussion in #support and extra context can be found here, note that their original usage of the unmute command with slashtags is not the root cause as confirmed by later examples of the command being used normally also manifesting this bug.

discord.ext.commands documentation states that greedy can be a footgun at times. Similar problems may be present elsewhere in the Mutes cog or the Red-Discordbot codebase.

Flame442 commented 11 months ago

It is impossible to both allow multiple members to be muted/unmuted and to allow a reason to be set in the same command, without either requiring explicit quotes around the reason or having the chance for username collisions. I don't believe there is a good solution to this problem, but I will leave the issue open for discussion.

EternalllZM commented 8 months ago

A solution might be to have a setting where you can disable multiple mute/unmutes (only allowing single). Then the reason doesn't require quotes to avoid unintentional matches in large servers.

Flame442 commented 8 months ago

I'm not sure how feasible it is to have a setting like that, it would need to use a custom converter that hotswaps between two converters based on that setting. Not impossible, but definitely strange.

Jackenmen commented 8 months ago

We could follow the existing strategy from the Mod cog - the ban command accepts a single member (that you can pass using anything that the Member converter accepts) and the massban command accepts multiple members that you can only pass using ID or a mention. In a similar fashion, we could split the mass-muting part of mute command to massmute.

EternalllZM commented 8 months ago

We could follow the existing strategy from the Mod cog - the ban command accepts a single member (that you can pass using anything that the Member converter accepts) and the massban command accepts multiple members that you can only pass using ID or a mention. In a similar fashion, we could split the mass-muting part of mute command to massmute.

I agree with this. Correct me if I'm wrong, but I do not see mass unmuting being something used more commonly than a single user. Knowing this, priority should be placed on the unmuting of a single user being as accurate as possible.