Allow SVGs to be uploaded on all admin pages

[stormrockwell]

Description of the Change

#228
In 2.3.0, there was a breaking change for us, which prevented the addition of SVGs outside of posts or media library. I need the ability to add SVGs on admin pages/tools/terms/etc.

I've made it a lot broader by instead hooking into admin_init, and all of the tests seem to pass except one that doesn't run on develop either for me, "Admin can add SVG block to a post."

I initially went with adding hooks for load-admin.php/load-edit-tags.php/etc, but I didn't see an issue broadening it to all of the admin interfaces, which might be my ignorance of this project. If there are better hooks we can use to cover every case, let me know, and I will make the changes!

How to test the Change

Try adding SVGs to option pages, often added by plugins/themes.

Changelog Entry

Fixed - Bug that prevented admins from uploading SVGs on admin pages outside of the post and media library pages.

Added - New feature
Changed - Existing functionality
Deprecated - Soon-to-be removed feature
Removed - Feature
Fixed - Bug fix
Security - Vulnerability
Developer - Non-functional update

Credits

Just me and whomever helps if we change the hooks!

Checklist:

• I agree to follow this project's Code of Conduct.
• I have updated the documentation accordingly.
• I have added Critical Flows, Test Cases, and/or End-to-End Tests to cover my change.
• All new and existing tests pass.

[dkotter]

Thanks for the PR @stormrockwell. While I understand the desire to upload SVGs in more contexts, the changes made in #228 were done to fix a security vulnerability and as such, we're unlikely to change the behavior introduced there.

The short version here is that we only want to allow SVGs to be uploaded in contexts where we can be sure the SVG is passing through our sanitization methods. Otherwise someone could upload an insecure SVG and that could then possibly execute unwanted code. So allowing SVGs to be uploaded in any admin context could allow SVGs to bypass sanitization.

[stormrockwell]

@dkotter I understand the reasoning, but it's a bit of a bummer. Has anyone looked into if there's a way to ensure SVGs are sanitized in additional contexts? If not, I can try to look into it at a later date

[dkotter]

Has anyone looked into if there's a way to ensure SVGs are sanitized in additional contexts? If not, I can try to look into it at a later date

Our main goal was to ensure SVGs uploaded in traditional contexts pass through sanitization so not sure much time was spent looking at other contexts. Definitely interested though in expanding this so if you have time/interest to dive further into that, would be greatly appreciated

[stormrockwell]

@dkotter Wanted to post an update. I wasn't able to figure out how to bypass the sanitization after the previous security patch adding the wp_handle_sideload_prefilter hook. If someone could provide additional context on an additional hole with hooking in at the admin_init level I can explore more

[wpexplorer]

@dkotter
@darylldoyle (sorry for the mention, but since you submitted the original pull request I thought you may be interested)

Setting the upload_mimes in the various context (load-post.php, load-post-new.php...etc) can still have security concerns as 3rd party scripts can create their own upload functions within this context (in a metabox for example).

I think it would be better to hook into "plupload_default_settings" which is used to localize the settings passed to the WP uploader.

This would be done by adding svg to the list of allowed extensions:

add_filter( 'plupload_default_settings', function( $settings ) {
	if ( ! empty( $settings['filters']['mime_types'][0]['extensions'] ) ) {
		$settings['filters']['mime_types'][0]['extensions'] .= ',svg';
	} else {
		$settings['filters']['mime_types'][0]['extensions'] = 'svg';
	}
	return $settings;
} );

This way upload_mimes would remain unchanged and only the WP uploader script would be allowed to upload svg files. The the plugin hooks into wp_handle_upload_prefilter which should always be running when using the native WP uploader.

Besides hardening security, SVG's would be allowed in any context that uses the native WP uploader, regardless of where it's being used ;)

• AJ

[jeffpaul]

@darylldoyle @dkotter seems like this should be closed as unlikely to be actioned upon for the plugin, correct?

[jeffpaul]

@darylldoyle @dkotter bumping this back onto your radar to review whether there's an approach here that's viable or if we should close this out?

[wpexplorer]

hopefully you guys had a look at my comment above, I would be happy to provide a pull request if you wanted me to.

[darylldoyle]

@jeffpaul I think it could be beneficial to look into the plupload_default_settings as @wpexplorer mentioned. My hunch is that it won't entirely solve the issue, though. I think the problem will then be that we're not hooked in for the REST API handler for uploads, but it's worth a look.

I'm pretty tied up for the next few weeks, but can likely take a look sometime in June if you don't have anyone to look before then.

[wpexplorer]

plupload_default_settings doesn't seem to work anymore (not sure when it stopped working) but it seems like WordPress may have added an extra check somewhere. So you can close this as it's no longer possible to just use this filter. At least it's no longer working for me for woff/woff2 files - I would suspect it's the same for any extension.

[dkotter]

Closing this in favor of #279