bot
bot copied to clipboard
Unfurl Redirects
Closes #1933.
- [x] Blocked by python-discord/workers#20.
- [x] Blocked by python-discord/site#625.
Description
This PR implements the URL unfurling filter feature described in #1933. It adds a service to do so, the filtering logic, and a command. To prevent the filtering logic from being slow, and to handle the fact that this filter calls another filter, I've had to make modifications to the core filtering logic to allow for delayed processing and modified the alert logic to allow customization of the output.
I've also kaizened in a small change to remove http and https from URLs when being added to the filter lists, after a brief discussion in mod-meta.
See the next comment for the output of the command.
Success

Too many redirects

Long URL

Unhandled Error (on the worker end, logs a warning)

I believe I've addressed all comments on Blue's review, but I can probably clean the commit history before merging.
@onerandomusername Perhaps, though that would require changes to the worker itself. It's also not the goal of this PR at the moment, since the shadysites is only a concern for moderators as far as this command is concerned, but we don't care for moderation purposes.
While testing this I can't get the auto-filtering to work..
- Add YouTube with
b!blacklist add domain_name https://youtube.com/ - Log in with my alt and paste
https://tinyurl.com/materialgworlbustdownwhich redirects to YouTube. - Nothing happens even though the domain is blacklisted?
@Bluenix2 did you add tinyurl to the unfurl filter list?
I'm working on reviewing this. Might take me a couple of review sessions.
This PR is incompatible with #1889, and will likely require conscious review before merging main. Whichever PR is merged second will need to make those changes.
@HassanAbouelela good to know, I'll keep that in mind should mine be merged after.
I've asked Xith to remove me from this issue and put it up for grabs, but an important note for whoever merges this:
The change to the insert/delete command will make any URLs in prod inaccessible if they have https in the name. I've already deleted a majority of the ones that existed at the time, but before you merge, it's a good idea to go through and clean up any that have been added since.
According to a vote we held with the mods, the alert should always ping if the unfurl fails, but otherwise ping only if the destination is blacklisted.
According to a vote we held with the mods, the alert should always ping if the unfurl fails, but otherwise ping only if the destination is blacklisted.
Cool, that means I won't need to do anything since it's current behaviour :D
I've been requested for review, but I do not think all my previous comments have been addressed.
I've been requested for review, but I do not think all my previous comments have been addressed.
You're right, this isn't quite ready for review just yet. It's on my list for after the d.py bump in bot-core and bot.
Given https://github.com/python-discord/bot/pull/2390, implementation of the new filtering list won't be addressed in this PR (relevant code needs to be removed) and will wait until after the mentioned PR is merged. This PR will focus on the unfurling mechanic itself, and is up for grabs.
Bumping this PR, #2390 has been merged.
This PR has been open for around 2 years. Doesn't seem to be needed at the moment to continue, so I'll close this, at least for now.