Skip to content

Antimalware cog#528

Merged
GhostofGoes merged 8 commits into
python-discord:masterfrom
bendiller:antimalware-cog
Oct 20, 2019
Merged

Antimalware cog#528
GhostofGoes merged 8 commits into
python-discord:masterfrom
bendiller:antimalware-cog

Conversation

@bendiller
Copy link
Copy Markdown
Contributor

Howdy! This PR is intended to address issue #471 (auto-deleting messages with potentially malicious file attachments). After discussing with @lemonsaurus, it became clear that a new Cog would make for a cleaner implementation, instead of a new rule within the AntiSpam Cog. I'll be happy to elaborate on those reasons if anyone would like further details.

Here's about the simplest implementation I could conceive, using the suggestions made in the issue's notes. Some points for further discussion:

  1. The whitelist (list of allowed extensions) will need vetting - I used a fairly small list of common image and video file formats, and the Cog rejects everything else.
  2. I'm fairly new to this code-base, so there may be some preferred implementation details and such that I neglected, either accidentally or otherwise. For example:
    - AntiSpam.on_message() begins with a handful of early-out conditions. I don't quite understand the purpose of some of those, so I did not include any, pending further review by staff.
    - AntiSpam calls ModLog.ignore() before deleting messages - I wasn't clear on why, so I did not do the same.
  3. I did not implement punishment for attaching files with prohibited extensions, but I can do so if requested. It seems a little overly punitive perhaps, because I think most people attaching files will be doing so with good intent, but that's not my call.

I'll be happy to make any requested changes. Thanks!

Copy link
Copy Markdown
Contributor

@kraktus kraktus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey!

Thank you for your time! I did a few remarks on the form of your code, but I'm a small contributor, so you can wait for a more in-depth review.

Note: I did not see where you delete the message in your code. Is it already handled by another cog?

Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py
@bendiller
Copy link
Copy Markdown
Contributor Author

Good feedback @kraktus, thank you! As for the message deletion, that begins on line 39, directly above the def setup()... function.

@kraktus
Copy link
Copy Markdown
Contributor

kraktus commented Oct 13, 2019

Good feedback @kraktus, thank you! As for the message deletion, that begins on line 39, directly above the def setup()... function.

Yep I did not see it because I reviewed the first commit :)

@MarkKoz MarkKoz added area: cogs t: feature New feature or request labels Oct 14, 2019
Comment thread bot/cogs/antimalware.py Outdated
Comment thread config-default.yml Outdated
Comment thread config-default.yml
Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py Outdated
@bendiller bendiller requested a review from MarkKoz October 16, 2019 17:16
Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py
Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py Outdated
Comment thread bot/cogs/antimalware.py
@bendiller bendiller requested a review from MarkKoz October 17, 2019 23:19
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this locally and it works, even with multiple attachments (sent from phone). One concern is that moderators are not exempt from this functionality but I'll let them have the say on whether they should be.

Copy link
Copy Markdown
Contributor

@GhostofGoes GhostofGoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see a reason mods would need to be exempt from the whitelist. If we decide otherwise in the future, it should be fairly straightforward to add a check for staff and apply a separate or additional whitelist.

@GhostofGoes GhostofGoes merged commit db0c916 into python-discord:master Oct 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants