TASVideos / tasvideos

The code for the live TASVideos website
https://tasvideos.org/
GNU General Public License v3.0
62 stars 29 forks source link

Automatic downscaling of profile pictures #1711

Closed moozooh closed 8 months ago

moozooh commented 10 months ago

It's a persistent issue that some users fail to downscale their profile pictures by themselves, and the site itself does nothing to enforce maximum dimensions of profile pictures.

From what I've seen on other sites and social networks, the technology to downscale images automatically indeed exists by this point. Ideally, it should be used. Failing that, the site should simply not accept images bigger than the allowed limits (currently 125×125) at all instead of trusting the user to do their due diligence.

Some message to inform the user should be shown in either case.

Masterjun3 commented 10 months ago

The problem here is that we currently simply allow a string to be the image link. We don't save it ourselves #227 . The server does not visit this link, or even validate that it's an image at all. Giving users the ability to make our server visit an arbitrary page is dangerous and probably unwanted. So I think this issue ticket is impossible without hosting images ourselves.

YoshiRulz commented 10 months ago

Giving users the ability to make our server visit an arbitrary page is dangerous and probably unwanted.

I agree, but consider that passing along an arbitrary user-provided URI to other users means any attack would just hit them instead...

[...] the technology to downscale images automatically indeed exists [...]

The go-to would be ImageMagick, which there is a .NET library for, or you could use its CLI: magick mogrify -resize '125x125>' file.png

moozooh commented 10 months ago

The problem here is that we currently simply allow a string to be the image link. We don't save it ourselves #227 . The server does not visit this link, or even validate that it's an image at all. Giving users the ability to make our server visit an arbitrary page is dangerous and probably unwanted. So I think this issue ticket is impossible without hosting images ourselves.

In that case, shouldn't it be possible to limit it in the markup? I.e. giving the space in which the picture would be displayed a fixed maximum width?

At least that should be a quick enough workaround. Hosting the images by ourselves is the ideal solution; I think it shouldn't be too big of a burden considering how many images we're already hosting for games and whatnot.

Spikestuff commented 10 months ago

Wouldn't it be easier to just switch it around with?: bigavatar

CasualPokePlayer commented 10 months ago

The fundemental issue is the site does not visit the image link (and very much should not visit the image link, as you're opening up a potential vector for the entire server to be compromised), so it cannot do any kind of size detection here. In any case, you have to be uploading the image to the site for any kind of automatic action to be taken.

YoshiRulz commented 10 months ago

I think @moozooh was suggesting that the user('s browser) includes a fixed size when submitting the URI. That would fix the problem of oversized images, but wouldn't change the situation w.r.t. third-party hosting edit: nor, as Masterjun notes below, page load times.

Masterjun3 commented 10 months ago

In that case, shouldn't it be possible to limit it in the markup? I.e. giving the space in which the picture would be displayed a fixed maximum width?

If you mean just changing our html to have a maximum width, yes that would work and is simple. But we intentionally do not want that. We want to notice if an image is too large. The problem is that the filesize for large images is usually too big as well. If we hide the problem, our pages would load slower as more large images go unnoticed. The good thing about our ~125 pixel limit is that the filesize automatically goes down to reasonable levels.

vadosnaprimer commented 8 months ago

So this is all "by design" then?

Masterjun3 commented 8 months ago

Yes. I will close this, since we can't currently implement it. Instead, I made a new issue with something we can implement here #1732 .