Problem/Motivation

If a "details" renderable object type has html markup in title, this markup is sanitized and displayed instead of rendering the markup.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...
Ex:

$form['author'] = array(
  '#type' => 'details',
  '#title' => '<span>' . $this->t('Author') . '</span>',
);

$form['author']['name'] = array(
  '#type' => 'textfield',
  '#title' => $this->t('Name'),
);

will display '<span>Author</span>' because '<' and '>' are encoded.

Proposed resolution

Look at how fieldset display and filter label to make same thing with details title.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

GoZ created an issue. See original summary.

GoZ’s picture

Title: Title of details element is sanitized to much » Title of details element is sanitized too much
GoZ’s picture

Issue summary: View changes
GoZ’s picture

Issue summary: View changes
cilefen’s picture

bogdan.racz’s picture

Assigned: Unassigned » bogdan.racz
Issue tags: +DevDaysSeville
bogdan.racz’s picture

Please review attached patch.
We should use markup as in the template_preprocess_fieldset function.

bogdan.racz’s picture

Assigned: bogdan.racz » Unassigned
Status: Active » Needs review
GoZ’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Perfect, looks like what we expect.
thanks @rbmboogie

I think we should add tests (one patch for current failing test only, one other with fix and success test)

bogdan.racz’s picture

Assigned: Unassigned » bogdan.racz
Status: Needs work » Active
bogdan.racz’s picture

@GoZ, I'm not exactly sure where this test should live.
Would ThemeRenderAndAutoescapeTest under "Drupal\KernelTests\Core\Theme" be a good place?

bogdan.racz’s picture

Please review.
I created a new test method ThemeRenderAndAutoescapeTest::testDisplayRenderElementSanitization I'm not sure if this is the right way to go.
Should this test be instead in the ThemeRenderAndAutoescapeTest::providerTestThemeRenderAndAutoescape, or it is ok as it is in the attached patch?
The issue is that if I put it in the above method, it will be quite a long string, something like:

'Details title is not sanitized' => [
        ['#type' => 'details', '#title' => '<span>title</span>'],
        "<details class=\"js-form-wrapper form-wrapper\">\n<summary role=\"button\" aria-expanded=\"false\"\naria-pressed=\"false\"><span>title</span></summary>\n\n\n\n</details>"
      ]

Another option would be to tackle the tests in the TwigExtensionTest class.
What do you think?

bogdan.racz’s picture

Assigned: bogdan.racz » Unassigned
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 12: title_of_details-2862452-12-test-only.patch, failed testing.

GoZ’s picture

thanks @rbmboogie,

Discussing with @lendude, tests should be put in \Drupal\KernelTests\Core\Render\Element\RenderElementTypesTest. Please, add a testDetails() test and use assertElements() which render element instead of using RenderContext.

bogdan.racz’s picture

Assigned: Unassigned » bogdan.racz
Status: Needs work » Active
bogdan.racz’s picture

bogdan.racz’s picture

Assigned: bogdan.racz » Unassigned
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 17: title_of_details-2862452-17-test-only.patch, failed testing.

bogdan.racz’s picture

Status: Needs work » Needs review
GoZ’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks @rbmboogie.

May be someone else will ask for other tests. Let's see

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

I'm not sure that this is the correct change. When something should be XSS filter or escaped is often up to the usecase. And changing this midway through a release cycle is disruptive.

A developer can just as well do:

$form['author'] = array(
  '#type' => 'details',
  '#title' => ['#markup' => '<span>' . $this->t('Author') . '</span>'],
);

Because they know what they are putting in #title - template_preprocess_details() has no clue about how the #title is being set.

GoZ’s picture

Solution has been inspired by preprocess_fieldset(), unless fieldset make it in a wrong way too:

https://api.drupal.org/api/drupal/core%21includes%21form.inc/function/te...

function template_preprocess_fieldset(&$variables) {
  (...)
  if (isset($element['#title']) && $element['#title'] !== '') {
    $variables['legend']['title'] = ['#markup' => $element['#title']];
  }
  (...)
 }

Issue has been found implementing details + content_translation, with translatable field. \Drupal\content_translation\ContentTranslationHandler::addTranslatabilityClue() then concatenate suffix to #title attribute of element.
https://api.drupal.org/api/drupal/core%21modules%21content_translation%2...

alexpott’s picture

@GoZ the problem is Drupal is horribly inconsistent as to whether different things are filtered or escaped. Changing this potentially breaks things. Also what happens if someone has already done code like in #22?

>>> $a = ['#markup' => ['#markup' => 'Test']];
=> [
     "#markup" => [
       "#markup" => "Test",
     ],
   ]
>>> (string) \Drupal::service('renderer')->renderPlain($a);
PHP warning:  strlen() expects parameter 1 to be string, array given in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Component/Utility/Unicode.php on line 686

Thing break horribly.

GoZ’s picture

I see 2 solutions:
1/ We check if #title is array or string before adding ’#markup'.
2/ Fix the way content_translation (and others if there are) suffix the title.

May be there is an another solution too.

Note this code also actually break for fieldset title.

alexpott’s picture

@GoZ but the fieldset title behaviour is not changing. Yes I agree that the differences in behaviour are super-awkward and would love to see an initiative to make the approach to santisation / filtering consistent at the API level. But changing it in a minor or patch release for one render element property is not going to achieve that. I would love to see an issue about #title that has a proposition where we can make them all behave the same way. Note there are plenty of existing issues about this.

GoZ’s picture

@alexpott i don't see any implementation of ['#title' => ['#markup' => 'something']] in core modules.
Can we at least fix this issue before moving to an issue for every #title sanitisation ?

Details element is supposed to replace a lot of previous fieldset uses. If fields using details element are translated, the will appear. We can't wait after a long issue about #title to fix that.

I'm ok it's not perfect (all comments prove it), but we should make something clean for the front first, and then move on another issue to propose a better implementation of sanitisation for #title attributes.

alexpott’s picture

i don't see any implementation of ['#title' => ['#markup' => 'something']] in core modules.

BC doesn't just core - it covers core, contrib and custom. There's no way we can know what is being done in custom. And this change has a security implication - it is making HTML that is escaped not escaped.

GoZ’s picture

And this change has a security implication - it is making HTML that is escaped not escaped.

Not really, there still is a XSS filter on this field. We can add a span, but no scripts per example

So we are in a dead end :

  • Actually if someone use ['#title' => ['#markup' => 'something']] in a not translated details element (but in a translatable entity form), content_translation will break due to string concatenation.
  • If someone use #title as a string in a not translated details element (but in a translatable entity form), content_translation will concatenate span suffix string but this span is encoded.
alexpott’s picture

@GoZ the translatable entity form can be fixed because making that support render arrays is an API addition as such - a behaviour widening. Whereas changing the behaviour of the details #title is an API change that result in semantic difference for the same original value.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tstoeckler’s picture

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

esod’s picture

Rerolling the patch for Drupal 8.6.0-rc1. Can't create an interdiff because title_of_details-2862452-17.patch is unable to patch.

esod’s picture

Rerolling the patch for Drupal 8.7.x. Can't create an interdiff because title_of_details-2862452-17.patch is unable to patch.

This patch is probably unnecessary because of #2652850: Title for details form elements is not set as '#markup' and it will be escaped, but all other form elements use '#markup' and are not escaped

tstoeckler’s picture

esod’s picture

I'm planning on checking it and I'll report back. If https://www.drupal.org/project/drupal/issues/2652850 has fixed the issue, I'll be able to tell for sure, at least on our system.

I needed those patches rerolled quickly for a development build. Hopefully I won't need to use them and will be able to remove them from this issue.

Let's assume this is fixed in the other issue and remove the 8.7.x patch right now.

esod’s picture

esod’s picture

Removing the 8.7.x patch is not happening, and whoops from me for rerolling these patches in the first place.

idebr’s picture