Problem/Motivation
"Missing help topic." text and warning is shown if Full HTML text format is disabled/missed in the installation. The topic contents isn't shown, event if it is plain text, that completely break help pages. It expected that page working previously continue be displayed, even with warning, breaking changes were made in the parent issue.
IMHO, it really bad practice to hardcode non-callback text format and force a site owner to enable it. The text format is used it should be configurable, in the next reasons:
- Not all sites would like to use Full HTML format
-- The site installed with "minimal" profile and don't have Full HTML format at all.
-- Basic HTML is enough for the site needs. Enabling Full HTML switcher with formats appearing under the text fields, when they don't need it.
-- Site is using custom WYSIWYG and custom format for it. Site builders wouldn't like to let Full HTML usage with CKeditor.
-- Site is using non-HTML markup, and enabling Full HTML format could broke API contract if chosen in some field.
-- The site uses twig templates for fields and uses plain text for input only. HTML formats could break the page structure.
- The Full HTML format could be changed along to the site needs, and e.g. filter DIV elements, that leads to wrong help pages display.
The format could be made configurable, but does the module needs text post-processing with format at all? I see match post processing staff is done inside of "::viewTopic" method, e.g. line breaks converting to tags. What is needed from prepossessing in Full HTML? Track images uploaded via a Text Editor or Restrict images to this site? I assume no.
The parent issue says about "style" attribute support in the Advanced Help sources. I think Text Format isn't needed for it and only does damage.
Proposed resolution
1. Do not check sources for XSS at all, as it is internal sources and keep XSS protection on the responsibility of a developer. It could work if external sources (e.g. by url or user input) are prohibited.
2. Create own implementation like XSS::filterAdmin(), that allows style attribute, but in this way clickjacking could be possible. See https://www.bioinformatics.org/phplabware/forum/viewtopic.php?id=164.
3. Agree that "style" attribute usage is unsafe, replace it with classes in examples, use XSS::filterAdmin().
Remaining tasks
1. In a new implementation $build is replaces by $output, that reads "#makup" key only and lose "#attached" property. But CSS are still included for the pages. Is it some code duplication?:
if (!empty($info['css'])) {
$build['#attached']['library'][] = $info['module'] . '/' . $info['css'];
}
2. MD sources are already filtered and wrapped, it shouldn't be filtered and wrapped one more time.
if ('md' == $ext) {
$build['#markup'] = '<div class="advanced-help-topic">' . Xss::filterAdmin(MarkdownExtra::defaultTransform($build['#markup'])) . '</div>';
}