Advanced help files are plain HTML files, and the typical pattern used for embedding a centered image is:
<div class="help-imgpos-center" style="max-width:180px">
When I inspect that particular line with the browser's inspector I see:
<div class="help-imgpos-center">
I.e.: The "style" element is gone, breaking rendering of images of some pages.
The root cause of this is described in comment #5: The default text format used to render pages is "Basic HTML", where #markup is passed trough Xss::filter where the 'style' attribute will be stripped from the marked up text..
Enable the submodule Advanced Help Example and visit the page "Image examples" that are part of the example help pages that comes with the submodule.
Use "Full HTML" as the text format.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | advanced_help-prevent-style-being-stripped-3299789-12.patch | 2.05 KB | e.bogatyrev |
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 #2
gisleDrupal 7 regression.
Comment #5
e.bogatyrevHi everyone,
The root cause is #markup will be always passed trough Xss::filter where 'style' attribute will be stripped.
I added a configuration form with ability to choose filter format. To apply this I had to change the output render array type to #processed_text. It allows us (e.g. chosen full_html) to display more attributes including "style".
Please take a look at MR or patch.
Comment #6
gisleThanks for the explanation, and thanks for the MR/patch!
I've tested it, and it works as described, after I have used the configuration form to change the filter format from "Basic HTML" to "Full HTML" and rebuilt caches.
However; I wonder whether it is necessary for the administrator to alter the text filter format via a form? I understand that allowing non-trusted users to insert markup that uses the 'style' attribute is insecure. However, non-trusted users are not to be allowed to add extensions that makes use of the Advanced Help system to a website. This is always done by an administrator that are already trusted to add to the site's code base. If you have access to add PHP code to a website, you can inject all sort of nasty stuff, so by default allowing these users to use the "Full HTML" filter format for the Advanced Help markup does not creates a vulnerability that is already in place when administrators have access to add the PHP code base.
Any thoughts?
Comment #7
e.bogatyrevHi @gisle,
Thank you for your quick response.
For sure, the help pages can be added by developer only and he could add any nasty code to them. May be it could be a lightweight insurance since developer must update the advanced_help.settings config as well to get appropriate display result. In case when MR will have a dozen of help pages administrator (or anyone who has merge permissions) could miss something from HTML attributes but will more accurate and attentive if noticed changes in format settings. Just thoughts, may be it's not a use case.
Anyway, I think we need to leave a chance to change this setting and avoid hardcode.
Comment #8
gisleI still not see the point in forcing the adminstrator to change the filter format in order to get images to display properly on the page, I am pretty sure most administrators using the module will search for this setting as soon as they discover that images don't display properly, so it is not much of an insurance, but a major inconvenience to have to change this setting in order to get the module to work as intended.
I've put a note about this on the project page (see under the heading "Text format"), and I'm going to put similar text in README when this change is deployed as part of the next release.
PS: Take a look at the Advanced Help Example submodule that comes with the latest 8.x-1.x-dev snapshot. The page titled "Images examples" shows why the "Basic HTML" filter breaks image display.
Comment #9
e.bogatyrevHi @gisle,
Yes, I agree with you, any filters except "Full HTML" are not suitable (I've checked "Images examples" you mentioned).
Only one concern here - the "Full HTML" could be disabled by some reason (replaced by more enhanced new filter format).
What filter will we use in this case?
Comment #10
gisleThe "Full HTML" format comes with core, so it is likely that it will be available. However, in principle, as you write, an admin can disable it at any time, as can any other text format that comes with core (except "Plain text").
If it is removed/replaced from core, the module obviously need to be updated to use the new format, but I don't foresee that happening.
Having an admistrator disabling it not something that is very likely to happen, but we want to avoid a WSOD if it does, I suggest to resolve this by just checking if it is enabled, and output a warning: "Text format 'Full HTML' appears to be disabled. You need to have this format enabled to use Advanced Help", instead of rendering the help page if it has been disabled.
Comment #11
e.bogatyrevOk, let's do it. I will remove the configuration form and add this check.
Comment #12
e.bogatyrevMR/patch has been updated, please verify.
Comment #13
gisleThanks for the new patch/MR. I am going to commit this in a slightly modified form soon.
The error message links to the page one would normally expect to find the disabled text format in order to re-enable it. However, currently it is not to be found there. This is a bug in Drupal core that still is unfixed after eight years. Please see: #2502637: Disabled text formats can't be seen in the GUI
I am going to modify the error message slightly to say that until this core issue is fixed, disabled text formats can't be enabled in the GUI.
Comment #14
gisleThis is now pushed and available as the most recent snapshot of 8.x-1.x-dev.
Many thanks to e.bogatyrev for fixing this.