Added a helper function for wp_kses to sanitize a SVG #137
Conversation
There was a problem hiding this comment.
I have a few concerns with this PR.
- I'm not sure what this solves. KSES cannot be used as an SVG sanitisation method, see here. Is it just to help with output? If so, why not use
wp_kses_post()? - There is a list of SVG elements for KSES but no listed attributes which means we'll allow them all. Within the SVG specification, the SVG attributes pose more of a threat to an XSS vulnerability than the elements do.
- To keep this in line with the
svg-sanitiserlibrary, this list will need to be kept up to date with each release of the underlying library. This will likely be labour-intensive. - The Safe SVG plugin includes filters that allow engineers to extend the list of allowed elements 1 and attributes 2. This KSES filter doesn't take that into account and therefore could cause the allowed elements/attributes to be stripped by late escaping.
Whilst I think it's great to push this plugin forward, I think we need to consider how we can ensure we're not overcomplicating things and to always be cognizant of any potential security impacts.
If you're interested in further reading, the below links might be interesting:
| @@ -74,6 +74,14 @@ add_filter( 'svg_allowed_tags', function ( $tags ) { | |||
| } ); | |||
| ``` | |||
|
|
|||
| ### Can `wp_kses` be used with a helper to sanitize an SVG? | |||
There was a problem hiding this comment.
issue: kses will not sanitise SVGs, so we either need to change this title, or re-think the use-case for this filter.
| */ | ||
| public function test_kses_allowed_html() { | ||
| $allowed_html = SafeSvg\SafeSvgTags\safe_svg_tags::kses_allowed_html(); | ||
| $this->assertIsArray( $allowed_html ); |
There was a problem hiding this comment.
thought (non-blocking): This feels like adding a test just to have a test. I'd prefer it if we were either verifying that the correct items were stripped from an SVG, or that the correct params/attributes are included.
|
@darylldoyle Totally get where you're coming from. This was actually in response to this issue: #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? |
|
@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. |
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:
Changelog Entry
Credits
Props @bmarshall511
Checklist: