backdrop
Follow-up: evaluate the use of try/catch in image_add_svg_attributes()
Description of the bug
This is a follow-up to #6577, where it has been discovered, that the indent of this code block isn't correct, but also a question/concern has been raised, whether this try/catch actually works as intended there. Or maybe it's unnecessary, or should get wrapped around a bigger part of the function code.
To not block the important bug fix there, this discussion has been postponed, so let's start it here.
Steps To Reproduce
Hard to tell, as it's rather a code logic issue, nothing user-facing. Possibly it's really only the indent (coding standards). Most likely it'll be easier to discuss, when trying out actually broken icon contents (e.g. invalid svg).
Eventually functional tests will need some extension, too.
Additional information
Add any other information that could help, such as:
- Backdrop CMS version: 1.28.x (since icon library addition)
- PHP version: any supported
Some testing with a broken svg (invalid markup in content):
If the svg is broken, and it's attempted to either add "alt" (hence a title tag) or attributes, the first problem is already loading:
Warning: DOMDocument::loadXML(): Specification mandates value for attribute svg in Entity... (line 586).
But at that point it's "only" a warning, not fatal. Any attempt to act on the (not) loaded svg ends up fatal, though, with:
Error: Call to a member function THE_FUNC on null...
So the try/catch seems indeed mis-placed there. As soon as we know, the svg's xml structure can't get loaded, we should skip attempts to do anything with it.
But what is the desired outcome then? An empty string? Some sort of informational comment like "unparseable content"?
I would throw a warning along the lines of "Provided SVG file is malformed" or "Provided SVG file does not follow mandated specification values" (or something like that anyway). As a bonus, we could add the DOMDocument::loadXML() warning in the form of "details" in the warning we throw.
But what is the desired outcome then? An empty string?
I would leave the provided .svg file untouched in that case.
...perhaps we should consider to change image_add_svg_attributes() to return FALSE when there's any sort of failure?
I would leave the provided .svg file untouched in that case.
Let's keep in mind that image_add_svg_attributes() is a helper function. Currently only theme_icon() uses it, but it's available for wider usage. If we know, we're dealing with invalid contents... should we still output it? Worst case, it might also break page markup.
...perhaps we should consider to change image_add_svg_attributes() to return FALSE when there's any sort of failure?
I'd find it important to stick with one and only one return type in this function: string.
It can be an empty string, it can be the unaltered string, it can be some HTML comment for easier debugging (by site admins, similar to what icon() does for "not found"). Additionally we might write something to dblog or show a message. Not sure.
In this case, I think an empty string is sufficient to make the caller understand something wrong happened (which could also mean the caller is trying to add attributes to an empty SVG.) I would also log an error, instead of showing an error, considering the function is called by a theme function. (IMO, it should be a task for the caller, to show an error message; there are surely cases where that is not possible nor preferable.)
In this case, I think an empty string is sufficient to make the caller understand...
So let's start with that. :wink: An initial PR is available for further discussion.
Testing can only happen locally, as core doesn't provide broken SVG files.
I also discovered, that the theme_icon() doc block's currently lies about the return type. :stuck_out_tongue_winking_eye:
With broken SVG, the messages in dblog will contain something like:
"Failed to parse SVG content. Premature end of data in tag svg line 1" or "Failed to parse SVG content. Opening and ending tag mismatch: svg line 1 and xml"
I think the PR looks good, @indigoxela.
Here's a question that will most likely prompt a separate issue, as it might require some changes to theme_icon(). I noticed that image_add_svg_attributes() is called in theme_icon() after the svg contents have been sanitized with filter_xss(). I haven't traced the code for $variables['attributes'], but is it technically possible that a malicious agent would send something like an onclick of onmouseover attribute that executes custom js? Would it be a good idea to add the attributes before running the filter_xss()?
I haven't traced the code for $variables['attributes'], but is it technically possible that a malicious agent would send something like an onclick attribute that executes custom js?
Interesting question, not sure if I get, what you're after. Currently it's all code and API, no UI at all. So malicious agents need to add their code to the site (module, theme...) to inject code. :shrug: There's currently no UI, that takes user input and turns it into attributes, so there's no attack vector.
... Would it be a good idea to add the attributes before running the
filter_xss()?
That sounds reasonable to me @argiepiano 👍🏼 ...separate issue though, as you said.
Merge conflict resolved and PR rebased. Ready for another round of testing / discussion...
PR rebased. Ready for another round of testing / discussion / review... :grinning:
Another rebase ... just in case.
@klonos any chance, you'll find the time to take a look at all my responses to your suggestions?
OK, I tested by removing the closing </svg> tag for an existing icon and running
dpm(icon('badyarn', array('attributes' => array('alt' => 'A bad yarn!'))));
in Devel. The code works as expected - an error is created in the log, the alt attribute is not processed at all (making the try/catch unnecessary) and an empty string is returned.
Thus, this works for me.
The comment about pre-existing title tags within the SVG markup is a valid one. I agree that if a title tag is provided already, then it should be respected and not replaced by the code in image_add_svg_attributes(). Perhaps a new issue for this?
The comment about pre-existing title tags within the SVG markup is a valid one. I agree that if a title tag is provided already, then it should be respected and not replaced by the code in image_add_svg_attributes(). Perhaps a new issue for this?
A follow-up might make sense, yes. IMO there's still at least one open questions re behavior. If a title element exists, but the alt param is set to an empty string (for a decorative element). Should we still remove the title? What "wins" then?
And that special handling's actually out of scope here. Purpose of this issue's to fix the broken try/catch.
Thank you @indigoxela, @klonos, @avpaderno, and @argiepiano! Merged https://github.com/backdrop/backdrop/pull/4797 into 1.x and 1.29.x.
