search

10up / safe-svg

Enable SVG uploads and sanitize them to stop XML/SVG vulnerabilities in your WordPress website.
https://wordpress.org/plugins/safe-svg/
GNU General Public License v2.0
272 stars 31 forks source link

Added a helper function for wp_kses to sanitize a SVG #137

Closed bmarshall511 closed 1 year ago

bmarshall511 commented 1 year ago

Description of the Change

Added a helper function used for wp_kses() to return an array of allowed HTML tags & attributes.

Closes #115

How to test the Change

Example usage:

echo wp_kses('<svg xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 25"><path fill="currentcolor" d="M17.525 9.302H14v-2c0-1.032.084-1.682 1.563-1.682h1.868V2.44a26.065 26.065 0 0 0-2.738-.138C11.98 2.302 10 3.959 10 7v2.3H7v4h3v9h4V13.3l3.066-.001.459-3.996Z"/></svg>', \SafeSvg\SafeSvgTags\safe_svg_tags::kses_allowed_html())

Changelog Entry

Added - Added the kses_allowed_html helper function for wp_kses to sanitize SVG images

Credits

Props @bmarshall511

Checklist:

bmarshall511 commented 1 year ago

@darylldoyle Totally get where you're coming from. This was actually in response to this issue: https://github.com/10up/safe-svg/issues/115. In a few projects that I've been dabbling in recently, we've been using something similar within the theme to sanitize SVG output, so thought it might be a pretty cool solution for that request.

You've made some solid points, especially about KSES not being suitable for SVG sanitization. And I agree, maintaining the SVG elements list in line with the svg-sanitiser library might be quite a handful.

I hear ya about the fact that the KSES filter doesn't consider the extendibility of the list of allowed elements/attributes. This could indeed lead to some elements/attributes being stripped off due to late escaping. And for sure, the last thing we want is to overcomplicate things and neglect potential security implications.

Gonna check out those resources you've shared - seems like some really useful stuff there. I'm all about that SVG security knowledge, so much appreciated for that! How about for now I close this & can be used as reference for the #115 issue?

darylldoyle commented 1 year ago

@bmarshall511 I appreciate your willingness to understand this! I hope the shared resources are helpful 🙂

In regards to #115, I think it'd be useful for Safe SVG to provide a helper function which wraps these lines:

https://github.com/10up/safe-svg/blob/develop/safe-svg.php#L277-L283

That would give people a way to output SVGs whilst ensuring they're sanitised. There's something to say for sanitising it to appease a linter. If the SVG is bundled with a theme, do we need to sanitise it on output? We should know what's in that file. We mainly need to sanitise files coming from third parties.

It would make sense if we were outputting SVGs from an API or other third-party integration.

I'm happy for you to close this issue and move the discussion to #115 if you like.