In #2874641: Select Text Format in Views Field for textual content with values from search index, options were added to allow displaying indexed data containing some HTML, i.e. only some HTML is filtered out of the indexed data.
But one use-case is to store the rendered result, e.g. an entire rendered view mode, in the index so that it can be shown when rendering in hopes of improving the performance of the search page. (The page no longer needs to build each displayed entity's display, it can just pull the already prepared HTML from the index.) In fact, this is the motivation from the Issue Summary of #2874641: Select Text Format in Views Field for textual content with values from search index.
With this use in mind, the rendered data in the search index has already been rendered. It's probably not desirable to additionally filter out some HTML from previously rendered and prepared HTML. One example of a problem is that if the rendered HTML contains an SVG, the SVG is stripped by the additional filtering/sanitization.
Add an additional option to "Allow all HTML" so that site builders can display their previously prepared and processed HTML without some tags being stripped.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
sonfdComment #5
drunken monkeyThanks a lot for proposing this, it does seem to make sense. I just amended the title and description of the option in the UI slightly to read:
However, I’m still not sure this is something we want to do. Reaching out to the Security Team to maybe give this a quick glance, just to make sure we won’t have a security issue filed in a few weeks to again revert this change.
Comment #6
drunken monkeyComment #7
gregglesI can see how:
What permission is allowed to set the "Allow any HTML" checkbox?
I agree it should have a warning on it as you point out in #5.
Comment #10
drunken monkey@greggles: Thanks a lot for your input!
The permission would just be
administer views, I guess. Which I think should be sufficient, someone with this permission could wreak all kinds of havoc. Putting an additional restriction on this specific option would also be possible, though, of course.Further suggestions from Slack:
1. does seem like a good option, to either only have the “Allow any HTML” option available for those fields marked as safe, or to even hide the setting for those altogether. It just would be a bit more work and might even require some architectural changes if we don’t just want to hard-code this specific field as “safe”.
2. would be a much quicker option to make sure people who use this know what they’re doing – do not add this option to the UI (or only if it is already active, to prevent users from accidentally reverting it) but document it in
README.md. Then only advanced users will even be able to set it, and they should be very much aware that they are doing something potentially risky.The problem then, of course, is that even advanced users will have a hard time finding this option (see #3220868: Create an “Advanced options” sub-module for hidden/advanced config for this whole set of problems). Though I guess we could add a mention of this to the description for the setting and just refer users to the
README.mdfor details?@sonfd: What’s your opinion on these suggestions?
Other input of course also welcome!
Comment #11
gregglesThe administer views permission definition includes
restrict access: true. Our documented expectation is that people who have that are trusted with making potentially risky choices.Comment #12
drunken monkey@greggles: Thanks, it’s good to know that this at least won’t cause a security advisory.
Currently, I’m still leaning towards option 2, however, of removing the setting from the UI and just mentioning it in
README.md(or maybe, maybe, in the form element description). But would be great to still hear from @sonfd what he thinks of that.Comment #13
sonfdI like option 1, but I don't understand the challenges required to implement. I don't think I like it if it adds a lot of complexity. And I worry about removing the option and defaulting to no sanitization even for fields we think are safe if it's already been out there sanitizing these fields.
RE: Option 2, hide the option from the UI - my personal feeling is that this just makes it more difficult and confusing to use this option, but doesn't really prevent it from being misused. I think folks will search for an answer and then just make the config edit because they saw it in some stack overflow answer without understanding it anyway. And maybe that is even worse with AI suggestions? I would personally feel better about showing the option with a warning, a situation where I can more easily control the information the person gets before making the change. 🤷
But I think it's a close call and I'll support whatever path you choose.
Comment #14
drunken monkeyThanks for your feedback!
Hm, maybe as a first step we can only show the option under certain circumstances – for our own “Excerpt” field and for processor-added fields containing HTML, like the “Rendered HTML output” field.
Adapted the MR accordingly, please test/review and tell me if this still looks good to you.
Especially, does this alleviate your concern regarding hiding the option, if it is available for the main use cases, or would you say your criticism still applies? In the latter case, it’s also easy to adapt this so that the option will really only work for these fields.
In any case, we can later still go back to make this option more widely available in case there is support/demand for it.
(Also, advanced users can still enable it for all fields that use the
SearchApiTextplugin via the config – I specifically included code to always show the option if it is already selected to avoid accidentally resetting it in this case.)Comment #15
drunken monkeyAny more feedback on this?
Comment #16
drunken monkeyI decided to just go ahead with the current solution. As stated in #14, we can always go back and extend the functionality later.
So, merged. Thanks again, everyone!