This is a really funny one in my opinion :)

If you enter bad HTML in the help text description of field widgets, this can break your page, especially if you are using a wysiwyg editor. But this is not only limited, submission guidelines and even the site slogan is affected, see more screenshots in http://drupal.org/node/1798654#comment-6605172

To reproduce, add following HTML tags to the help text description of the tags field on a article content type on a new installation:

<strong>Do not use tags<strong>

This is what happens on Drupal 8 (haven't been able to test with wysiwyg editors) - All text after the not closed strong tag are in bold.

Screen Shot 2012-09-29 at 10.14.47.png

And this is on Drupal 7 with a wysiwyg editor on the body field. Especially look down at the vertical tabs, it's completely broken.

Screen Shot 2012-09-29 at 10.13.13.png

Proposed solution

Use _filter_htmlcorrector() on field_filter_xss

Files: 
CommentFileSizeAuthor
#22 1798654-22.patch3.98 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 56,263 pass(es).
[ View ]
#22 interdiff.txt635 bytesamateescu
#10 1798654-10.patch3.88 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 55,558 pass(es).
[ View ]
#6 1798654-6.patch5.88 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 42,417 pass(es).
[ View ]
#5 Screen Shot 2012-10-15 at 21.16.00.png28.25 KBswentel
#5 Screen Shot 2012-10-15 at 21.16.13.png29.74 KBswentel
#5 Screen Shot 2012-10-15 at 21.16.23.png45.45 KBswentel
#4 1798654-4-tests-only.patch2.56 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 42,407 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#4 1798654-4.patch3.63 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 42,401 pass(es).
[ View ]
#1 1798654-1.patch455 bytesswentel
PASSED: [[SimpleTest]]: [MySQL] 41,894 pass(es).
[ View ]
Screen Shot 2012-09-29 at 10.13.13.png64.9 KBswentel
Screen Shot 2012-09-29 at 10.14.47.png51.26 KBswentel

Comments

swentel’s picture

Status:Active» Needs review
Issue tags:+needs backport to D7
StatusFileSize
new455 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,894 pass(es).
[ View ]

And the patch.

swentel’s picture

Could probably use a test I guess. there's extensive testing for _filter_htmlcorrector(), so we shouldn't be testing that, but just simply posting a help description to widget and asserting Raw that the HTML is correct.

larowlan’s picture

Issue tags:+Needs tests, +#pnx-sprint
swentel’s picture

StatusFileSize
new3.63 KB
PASSED: [[SimpleTest]]: [MySQL] 42,401 pass(es).
[ View ]
new2.56 KB
FAILED: [[SimpleTest]]: [MySQL] 42,407 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Here's a patch with tests. the '_filter_htmlcorrector' needs to be called in theme_file_upload_help() as well, because otherwise the HTML still can go wrong, that's why there are two tests on two fields, being an image and a 'normal' field. The theme_file_upload_help is also called in locale and image, so that's automatically going to make sure it's being corrected as well.

swentel’s picture

So this also applies to other areas in core, see screenshots:

So, let's rename this issue and fix those all ?

swentel’s picture

Title:Clean faulty HTML in help description of field widgets» Clean faulty HTML in help description of field widgets, content types and site slogan.
StatusFileSize
new5.88 KB
PASSED: [[SimpleTest]]: [MySQL] 42,417 pass(es).
[ View ]

New patch, fixes it also in content type description, submission guidelines and the site slogan. Do we want tests for those as well ?

swentel’s picture

Issue tags:+Usability

Tagging for UX people. Is this something we need to fix or not, or is this by design ?

Bojhan’s picture

Sounds good to me :)

swentel’s picture

Title:Clean faulty HTML in help description of field widgets, content types and site slogan.» Clean faulty HTML in help description of field widgets and content types
Status:Needs review» Needs work
Issue tags:-Usability, -Needs tests, -#pnx-sprint+Field API
swentel’s picture

Title:Clean faulty HTML in help description of field widgets and content types» Clean faulty HTML in help description of field widgets
StatusFileSize
new3.88 KB
PASSED: [[SimpleTest]]: [MySQL] 55,558 pass(es).
[ View ]

Narrowing even more, only field widgets.

swentel’s picture

Status:Needs work» Needs review
jmarkel’s picture

Status:Needs review» Reviewed & tested by the community

Seems fine, tests pass, works as specified.

alexpott’s picture

Status:Reviewed & tested by the community» Needs review

I'm not sure about this change... might it break this logic from Drupal\number\Plugin\field\widget\NumberWidget::formElement

    // Add prefix and suffix.
    if (!empty($instance['settings']['prefix'])) {
      $prefixes = explode('|', $instance['settings']['prefix']);
      $element['#field_prefix'] = field_filter_xss(array_pop($prefixes));
    }
    if (!empty($instance['settings']['suffix'])) {
      $suffixes = explode('|', $instance['settings']['suffix']);
      $element['#field_suffix'] = field_filter_xss(array_pop($suffixes));
    }

I'm kind of with #7.... someone's entered broken html... auto-fixing it in this instance is a bit meh...

alexpott’s picture

Assigned:Unassigned» yched

@swentel thinks the prefix / suffix thing won't be a issue... assigning to yched for a final call on this...

swentel’s picture

Note that I can live with a won't fix as well, it feels a little like babysitting ..

yched’s picture

Status:Needs review» Reviewed & tested by the community

Actually I can see where this could mess with the "prefix" and "suffix" settings on number fields.

Probably rare, but strictly speaking, you can currently use them to open a tag in "prefix" and close it in "suffix", thus wrapping the field value.
The way the current patch changes the behavior of field_filter_xss(), the opening and closing tags in prefix and suffix would end up being both removed separately.

I would tend to consider this "feature" as something we don't officially want to support. Adding wrapping html should be done in the theme layer.

Thus, as far as I'm concerned, this is RTBC.

alexpott’s picture

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Committed 0e59a8d and pushed to 8.x. Thanks!

Moving to D7 queue for backport discussion...

swentel’s picture

Title:Clean faulty HTML in help description of field widgets» [HEAD BROKEN] Clean faulty HTML in help description of field widgets
Version:7.x-dev» 8.x-dev
Assigned:yched» Unassigned
Priority:Normal» Critical
Status:Patch (to be ported)» Needs work

This seems to have broken head, looking into it, but I'm on a plane right now, so might not be fixing it in the end. Anyone can look into it of course.

swentel’s picture

Status:Needs work» Needs review

Breakage is due to #1875992: Add EntityFormDisplay objects for entity forms which got in first.

alexpott’s picture

Priority:Critical» Normal
Status:Needs review» Needs work

Arg... I knew I didn't like this fix...

Reverted this with commit 8e77891 and pushed to 8.x.

alexpott’s picture

Title:[HEAD BROKEN] Clean faulty HTML in help description of field widgets» Clean faulty HTML in help description of field widgets

Fixing title

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new635 bytes
new3.98 KB
PASSED: [[SimpleTest]]: [MySQL] 56,263 pass(es).
[ View ]

This is all that's needed to fix the test..

swentel’s picture

Status:Needs review» Reviewed & tested by the community
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed bda4742 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.

sun’s picture

Issue summary:View changes

Hm. I just noticed this gem in #2195745: Replace _filter_htmlcorrector() with a utility class in core

It is weird that we're making an exception for the settings of configurable fields. The entire rest of Drupal's configuration and administrative interface (1) actively assumes that the user knows HTML and (2) administrative input is solely protected by Xss::filterAdmin().

Created #2195891: Either clean faulty HTML in all administrative strings or revert #1798654

yched’s picture

Feedback welcome in #2195745: Replace _filter_htmlcorrector() with a utility class in core - see #32 / #34 over there.

In short : after #1825952: Turn on twig autoescape by default, field_filter_xss() now dow two calls to SafeMarkup::set() : one in Xss::filter(), the other one after Html::normalize(), to mark that new string as safe too.