WordPress.org

Making WordPress.org

Opened 8 weeks ago

Last modified 6 weeks ago

#5464 new defect

Plugin Directory: Prevent SVN uploads of animated banners and icons

Reported by: Ipstenu Owned by:
Milestone: Priority: normal
Component: Plugin Directory Keywords:
Cc:

Description

Related to #5171, animation in icons and banners is getting bonkers. It's not a11y friendly at all, and it's annoying to everyone else.

IMO the BEST solution would be to have SVN block all animated images named icon* or banner* from being uploaded, though that would require checking GIF, PNG, an SVG.

There are some other options. For example, we could actually allow gifs but set them to not animate.

Change History (10)

#2 @eatingrules
8 weeks ago

In Slack we discussed the ability to play/pause the loops, but the more I think about that, the more I think that's pretty silly in this context -- and it would also get in the way of being able to click on the thumbnail (in the repo search results, for example) to go to a plugin's full page.

I think it would be good if the system would allow uploads of animated images, but then only display the first frame. But if the system just flat-out rejects any animated image files, I think that's fine too. Plugin authors will learn quickly not to submit animated files either way! ;)

Thank you!

#3 follow-up: @dd32
8 weeks ago

There are four things that need to be accounted for here:

  1. Multi frame Gifs
  2. Animated PNGs
  3. The above uploaded as a .jpg
  4. SVGs with CSS transitions or <animate> tags

There's several options that I can think of..

  • For 1-3 is to proxy the image via something that strips the animation
  • For 1-3 upon plugin parse, detect multi-frame images and ignore them
  • For 4 (SVGs) SVGRoot.pauseAnimations() should work for the animation tags, but I don't think that'll affect any JS/CSS transitions..

One of the complexities here is that the ps.w.org cdn where these images are loaded from is just a caching code-less cdn based on plugins.svn.wordpress.org, so inserting something else in there between SVN and clients is not super ideal.

I guess that means that the ideal place to handle this is at plugin import time, but the limitations there is that it means you can't strip it back to being a single frame when it's detected to be animated without also making a commit to svn.

Detecting animated images and looking for animate within SVGs and ignoring them if present is probably the simplest option here.

#4 in reply to: ↑ 3 @dd32
8 weeks ago

Replying to dd32:

One of the complexities here is that the ps.w.org cdn where these images are loaded from is just a caching code-less cdn based on plugins.svn.wordpress.org, so inserting something else in there between SVN and clients is not super ideal.

If wanting to take that route, we could use the s.w.org cdn which is very similar, perhaps even just for icons that are detected as being animated or svg? eg how the https://s.w.org/plugins/geopattern-icon/hello-dolly.svg icons work.

#5 follow-up: @Ipstenu
8 weeks ago

I guess that means that the ideal place to handle this is at plugin import time, but the limitations there is that it means you can't strip it back to being a single frame when it's detected to be animated without also making a commit to svn.

Would that be, we can't tell without a commit of the image to SVN? Or would it check on any import of anything to SVN?

#6 in reply to: ↑ 5 @dd32
7 weeks ago

Replying to Ipstenu:

Would that be, we can't tell without a commit of the image to SVN? Or would it check on any import of anything to SVN?

The intention of it was, We'd check on any import from SVN (Manual or triggered by a commit), but with the limitation, that stripping it back from animated to non-animated would require an asset commit. Comment #4 however points out that that limitation only applies when using ps.w.org if it was switched to using s.w.org for those affected images it'd be fine.

This ticket was mentioned in Slack in #meta by tellyworth. View the logs.


6 weeks ago

#8 @dd32
6 weeks ago

So, I went on a bit of an implementation attempt today here..

I went with attempting to return animated assets like so:

https://s.w.org/plugins/remove-animation/$plugin/$animated_asset

That works great for animated Gifs & PNGs (even when saved as .jpg), but SVGs are quite a bit harder. For standard images, I'm just running them through imagecreatefromstring() and imagepng() to output the first frame.

I've found that appending the following to SVGs tends to work well as well:

<script> SVGRoot.pauseAnimations(); </script>
<style>* { animation: none !important; }</style>

The problem I ran into was that it increases the potential for Javascript execution on a WordPress.org domain, something that plugins.svn.wordpress.org & ps.w.org currently protects against, and so serving those SVGs via s.w.org is a bit too risky for me.

So.. I think the correct way to handle this is just to rip the bandaid off and block them.

I don't think blocking the SVN commit itself is worth it, just block it at the plugin directory import step.

Blocking images based on multi-frame is easy enough it seems. SVGs are more difficult, but we can probably just block on the file containing <script, animate, or animation. I'm sure there's a few ways around that though, but if someone can work around that, they deserve a ban hammer :)

This ticket was mentioned in PR #22 on WordPress/wordpress.org by dd32.


6 weeks ago

  • Keywords has-patch added

Initial attempt at serving non-animated icons/banners.
No animation detection included yet.

Work in progress.

Trac: https://meta.trac.wordpress.org/ticket/5464

#10 @dd32
6 weeks ago

  • Keywords has-patch removed

Of course, as soon as I posted that, I realised that simply serving the SVG as Content-Disposition: attachment would probably work around the XSS issues, but I'm still not convinced that it's the right move.

I've uploaded a PR of that for later reference if that's the route that gets taken.

I still think that blocking the import of assets we don't want around is probably the better move.

Note: See TracTickets for help on using tickets.