bot icon indicating copy to clipboard operation
bot copied to clipboard

Unfurl Redirects

Open HassanAbouelela opened this issue 4 years ago • 13 comments
trafficstars

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.

HassanAbouelela avatar Nov 17 '21 11:11 HassanAbouelela

Success

Success

Too many redirects

max depth

Long URL

image

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

error

HassanAbouelela avatar Nov 20 '21 16:11 HassanAbouelela

I believe I've addressed all comments on Blue's review, but I can probably clean the commit history before merging.

HassanAbouelela avatar Nov 28 '21 18:11 HassanAbouelela

@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.

HassanAbouelela avatar Dec 04 '21 01:12 HassanAbouelela

While testing this I can't get the auto-filtering to work..

  1. Add YouTube with b!blacklist add domain_name https://youtube.com/
  2. Log in with my alt and paste https://tinyurl.com/materialgworlbustdown which redirects to YouTube.
  3. Nothing happens even though the domain is blacklisted?

Bluenix2 avatar Dec 16 '21 20:12 Bluenix2

@Bluenix2 did you add tinyurl to the unfurl filter list?

HassanAbouelela avatar Dec 16 '21 20:12 HassanAbouelela

I'm working on reviewing this. Might take me a couple of review sessions.

MarkKoz avatar Dec 23 '21 03:12 MarkKoz

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 avatar Dec 23 '21 15:12 HassanAbouelela

@HassanAbouelela good to know, I'll keep that in mind should mine be merged after.

Kronifer avatar Dec 23 '21 15:12 Kronifer

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.

HassanAbouelela avatar Jan 25 '22 09:01 HassanAbouelela

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.

mbaruh avatar Feb 13 '22 13:02 mbaruh

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

ChrisLovering avatar Feb 21 '22 20:02 ChrisLovering

I've been requested for review, but I do not think all my previous comments have been addressed.

MarkKoz avatar Mar 23 '22 23:03 MarkKoz

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.

ChrisLovering avatar Mar 24 '22 08:03 ChrisLovering

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.

mbaruh avatar Mar 03 '23 13:03 mbaruh

Bumping this PR, #2390 has been merged.

Xithrius avatar May 02 '23 02:05 Xithrius

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.

Xithrius avatar Nov 25 '23 22:11 Xithrius