[APPROVED] x26-Cogs

Applications
#1

Discord Name:
Twentysix#5252

GitHub Repository (Must be V3):
https://github.com/Twentysix26/x26-Cogs

Description:
Only one cog so far, but it’s pretty rad if I say so myself. It allows you to browse, search and install all repositories and cogs known to mankind, with an ever updating index list sourced by Red-Index, which I also recently made.
I may have another major cog coming out soon but I decided to apply in the meantime, just so we can actually suggest my cog to people :smiley:
Cheers

3 Likes
#2

Added a new cog, Defender :tada:
More info in the readme

2 Likes
#3
#4

Hi Twentysix (Twentysix#5252), thank you for your patience while I review your repo.

Commit hash at time of review - ef43e9eba36c15a1cf8b32c6d154bd4ed313532d

defender

defender.py

  • Lines 148, 216: You have no checks here for embed_links so if the bot is missing the permission it will send the standard error message instead.
  • Line 1508-1516: You might consider utilizing await asyncio.gather(*calls) here to concurrently send the notifications to all guilds at once rather than waiting for the try/except to finish, you can even have a callback function if they fail in some way and then there’s no need to do the await asyncio.sleep(0.5). (Optional)
  • Lines 1545-1557: Again no checks for embed_links permission for the bot before posting.
  • Line 691, 703, 844, 856, 915, 929, 1174, 1349, 1744, 1815, 1884, 1892: You should not use bare excepts, preferably use except Exception or catch known exceptions. (Optional)
  • Line 1180: You might consider using from redbot.core.utils.chat_formatting import humanize_list here simplify the display of errored ID’s. (Optional)
  • Line 1130: You should probably also have a check here that the is_staff author in question actually has ban permissions or await self.bot.is_admin(ctx.author) to be consistent with cores ban command. It’s possible that the mod role setup on the bot is given to mods who only have power to kick and this would grant them limitless access to ban without actually having permission to ban, being also a command meant to hide the actions of the mods this can lead to some unwanted scenarios.
  • Line 1202-1305: Again this should have a check that in the event that a mod does not have ban permission or even kick permission this should not execute the command unless in emergency mode. This one is less strict as kick is generally less of a problem but the members discord permissions should be taken into consideration if it’s not in emergency mode.
  • Line 1546: You might consider using message.jump_url here instead of building the link yourself. (Optional)
  • Line 1704: this is over-indented. (Optional)
  • Lines 590-614: Added after discussion in #testing in Red server. This is good but it occured to me that this only checks role hierarchy and it may be worthwhile checking relevant permissions on the user themselves for all action types. For example you don’t want someone considered a mod with kick but not ban perms to be able to create a warden action that allows them to setup warden to be able to ban a user at their whim.

warden.py

  • Lines 92: Continuation line is over-indented. (Optional)
  • Line 124, 151, 299, and 503: You should not use bare excepts preferably use except Exception or catch known exceptions. (Optional)
  • Lines 444-450: Lines missing indentation. (Optional)
  • Line 457: You could just use message.jump_url instead of formatting the url yourself. (optional)
  • Line 606: You can compare role hierarchy directly with the role object without needing to compare postions. (Optional)

index

index.py

  • Line 19: you import TYPE_CHECKING, List, Literal, Tuple, and Union but don’t use any of them. (Optional)
  • Line 24: You import json but it’s not used anywhere. (Optional)
  • Line 29: you import DMContext, and GuildContext but they’re not used anywhere. (Optional)
  • Line 232: You should not use bare excepts, at the very least use except Exception if you want to capture all exceptions raised from the function. (Optional)
  • Lines 64 and 134: You should be checking if the bot has permission to send embeds for each of these commands and either ignore the command (simple with the @commands.bot_has_permissions(embed_links=True)) decorator) or provide an output to the menu that is just strings.
  • You may also consider adding a check for add_reactions so that the menu is actually usable in the event someone uses the command in a channel the bot doesn’t have add reaction perms in although this only breaks browse it doesn’t affect display of the search command. (Optional)

Some special discussion about defender.py:

  • Lines 670-710: These commands should probably have additional checks that the issuer also has manage messages permission. This one is kindof a grey area but as it is right now the @commands.admin() only checks that the issuer has at least one of the bots assigned adminroles. This one is optional simply because the severity of the actions are minimal. (Optional)
  • Lines 744-803: This should have checks that the issuer of the command has ban permissions to enable the module otherwise a not-so-wise server owner could tell the bot that a role should be considered admin by the bot and grant access to the voteout module. This in conjunction with the other issues with voteout listed below above spell disaster under rare circumstances.

These are under a special discussion because the @commands.admin() decorator is used and generally that’s considered someone who has permission within the server to perform these actions and you can put blame on the server owner for setting up the wrong role as an admin role but it should be stated that that check is strictly for the bots admin role being set. Generally when I set permission decorators on cogs I like to use the @commands.admin_or_permissions(relevant_permission=True) so that if the server owner does not set the admin role the relevant members who can be considered admin under the actions of the command can still run the commands. So I will be marking these both as optional changes to the command permissions but I would highly recommend you consider the possibilities of allowing only bot set admin role from performing the actions.

#5

Thanks for the review Trusty
Regarding index, I have addressed everything you have pointed out
As for Defender:

  • Basic bot permissions checks are now in place. For the embed in send_notifications I have chosen not to do any particular error checking there, I’m doing it where I call the function
  • I have put stricter permissions checks in the silence, voteout and vaporize modules. It now checks for, respectively, manage messages, ban perms (or kick if action is kick / softban) and ban perms
  • I understand your concerns about misconfigured bots, with admins that are not really admins permission wise. Personally, I believe that only so much should be done before a server owner should be held accountable for it.
    But, since this was relatively trivial to adjust, I have now a special check in place for the whole settings subgroup and warden related commands. Now, other than requiring admin role I also check for manage messages + manage roles + ban members.
    In case you’re wondering why I’m not using admin_or_permissions, it’s because I believe that when it comes to this cog a server should have everything properly configured and not rely on simple permissions.
  • As for the rest of the optional points, thanks for your feedback, I’ll likely get to most of them when I merge my dev branch, that will now require a rebase and some good old conflict wrangling when we’re done :joy:

Let me know if I missed anything, thanks!

#6

Thank you for making those changes. I really liked your solution to preventing missing perms as it’s more strict than the basic checks and goes beyond the minimum I was expecting for basic security although the basic checks are still worthwhile in the event that a server owner has not assigned the mod/admin roles to let their mods use the commands even if they have all the required perms (I have a lot of servers that could miss out on using this tool because they’re not aware of the bots mod and admin roles requiring being set).

Regardless this is now in a much better state and I’m happy to mark it as approved!

1 Like
#7
#8