Unable to upload SVG to category custom field (ACF)

[smerriman]

Describe the bug

In 2.3.0 and 2.3.1, uploading an SVG file to an ACF field to a category states "Sorry, you are not allowed to upload this file type". It worked fine in earlier versions.

Steps to Reproduce

1. Install ACF and Safe SVG.
2. Create a new field group with an Image field, with location rule set to Taxonomy = All
3. Edit a category and attempt to upload an SVG to the field.

Screenshots, screen recording, code snippet

No response

Environment information

No response

WordPress information

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[smerriman]

It looks like this is also broken on ACF Options page - both are extremely common uses for SVGs (category icons, logos, etc). In fact, these ACF fields are the only places I ever upload SVGs to and install Safe SVG for.

[dkotter]

This sounds similar to what is described in PR #240. I added a comment there with details but to summarize that, we were alerted to a security issue that we fixed partially in v2.2.6 and more fully in v2.3.0 that ensures we only allow SVGs to be uploaded if they are uploaded in a context where we can be sure our sanitization method runs. Otherwise someone can bypass sanitization and upload an insecure SVG.

I would love to figure out a solution that is less strict as far as the allowed upload contexts but we'll also need to be sure those still pass through sanitization.

For this particular issue, I imagine there's probably an ACF hook we can use to allow SVG uploads while still ensuring those are passed through sanitization, though some investigation will need to be done on that.

[smerriman]

OK - what do you recommend in the meantime? I've had to downgrade to 2.2.6 (again!) in order to keep all of my clients' sites functional. But if this version is insecure, then I'll need to weigh up leaving the insecurity vs shifting to another plugin vs awaiting a quick fix.

(If only trusted admin have access to the backend, I'm guessing sticking with 2.2.6 is the best option for now?)

[dkotter]

OK - what do you recommend in the meantime? I've had to downgrade to 2.2.6 (again!) in order to keep all of my clients' sites functional. But if this version is insecure, then I'll need to weigh up leaving the insecurity vs shifting to another plugin vs awaiting a quick fix.

(If only trusted admin have access to the backend, I'm guessing sticking with 2.2.6 is the best option for now?)

Yes, sticking with 2.2.6 is fine, especially if you trust anyone that has upload access to the site. We're discussing internally the best way to support these situations going forward as it's definitely not an edge case where someone would want/need to upload an SVG outside of the normal WordPress media library flow. Leaning towards either a new setting or at least a new filter that can be used to turn on admin-wide SVG uploads, for those sites that need it. Definitely open to any other ideas though.

[dkotter]

@darylldoyle Curious for your thoughts here as one that worked closely on these security changes. Any thoughts on the best way to allow SVGs to be uploaded in other contexts? For instance, someone that's building a custom settings page and wants to allow SVG uploads? Or someone that wants to allow uploads on a term edit screen? My thought was to add a new setting and/or filter that turns on the old approach, ensuring we have a note in there around the potential security downside to doing that. But wondering if there's a better approach you may have in mind?

[darylldoyle]

@dkotter I've had a quick look into. It looks like we can add the following to the contexts we can load in. This should fix all AFC uploads within the admin area.

add_action( 'acf/input/admin_head', array( $this, 'allow_svg_from_upload' ) );

This would happen here: https://github.com/10up/safe-svg/blob/develop/safe-svg.php#L131-L135.

This would also need checking to ensure we don't open up uploads to more than we want to. The alternative could be that we add a filter that let's people feed in their own actions/filters etc so that they can add to the pre-set contexts.

[stormrockwell]

@darylldoyle this is helpful for ACF powered fields, thank you. I've been using load-site-editor.php and load-edit-tags.php but that doesn't cover all instances. Do you have any insight on how admin_init/admin_head isn't safe/can bypass the safe SVG filtration? I wasn't able to figure it out.

Also, swapping the main class to a singleton pattern would allow us to register our own hooks as well without needing to edit the plugin. This could make it easier for people to make their site insecure as well though

If this is a helpful workaround for anyone, I've instructed users to upload their SVGs directly to the media library and then they can be included in other contexts so we don't need to edit the plugin on all of our sites