Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#35 | title_of_details-8.6.x-2862452-35.patch | 1.9 KB | esod |
#17 | title_of_details-2862452-17-test-only.patch | 1.08 KB | bogdan.racz |
#17 | title_of_details-2862452-17.patch | 1.96 KB | bogdan.racz |
Comments
Comment #2
GoZ CreditAttribution: GoZ at Centarro commentedComment #3
GoZ CreditAttribution: GoZ at Centarro commentedComment #4
GoZ CreditAttribution: GoZ at Centarro commentedComment #5
cilefen CreditAttribution: cilefen commentedComment #6
bogdan.racz CreditAttribution: bogdan.racz commentedComment #7
bogdan.racz CreditAttribution: bogdan.racz commentedPlease review attached patch.
We should use markup as in the template_preprocess_fieldset function.
Comment #8
bogdan.racz CreditAttribution: bogdan.racz commentedComment #9
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedPerfect, 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)
Comment #10
bogdan.racz CreditAttribution: bogdan.racz commentedComment #11
bogdan.racz CreditAttribution: bogdan.racz commented@GoZ, I'm not exactly sure where this test should live.
Would ThemeRenderAndAutoescapeTest under "Drupal\KernelTests\Core\Theme" be a good place?
Comment #12
bogdan.racz CreditAttribution: bogdan.racz commentedPlease 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:
Another option would be to tackle the tests in the TwigExtensionTest class.
What do you think?
Comment #13
bogdan.racz CreditAttribution: bogdan.racz commentedComment #15
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedthanks @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.
Comment #16
bogdan.racz CreditAttribution: bogdan.racz commentedComment #17
bogdan.racz CreditAttribution: bogdan.racz commentedI have moved the test to RenderElementTypesTest.
Please review.
Comment #18
bogdan.racz CreditAttribution: bogdan.racz commentedComment #20
bogdan.racz CreditAttribution: bogdan.racz commentedComment #21
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedLooks good, thanks @rbmboogie.
May be someone else will ask for other tests. Let's see
Comment #22
alexpottI'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:
Because they know what they are putting in #title - template_preprocess_details() has no clue about how the #title is being set.
Comment #23
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedSolution 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...
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...
Comment #24
alexpott@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?
Thing break horribly.
Comment #25
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedI 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.
Comment #26
alexpott@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.
Comment #27
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commented@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.
Comment #28
alexpottBC 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.
Comment #29
GoZ CreditAttribution: GoZ at Barbe-Rousse, Centarro commentedNot 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 :
['#title' => ['#markup' => 'something']]
in a not translated details element (but in a translatable entity form), content_translation will break due to string concatenation.Comment #30
alexpott@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.
Comment #32
tstoecklerAs far as I can tell this is a duplicate 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.
Comment #35
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedRerolling the patch for Drupal 8.6.0-rc1. Can't create an interdiff because
title_of_details-2862452-17.patch
is unable to patch.Comment #36
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedRerolling 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
Comment #37
tstoecklerPretty sure this was fixed for Drupal 8.6 with #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. Are you sure this is still an issue in 8.6.x-rc1?
Comment #38
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedI'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.
Comment #39
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedComment #40
esod CreditAttribution: esod at Memorial Sloan Kettering Cancer Center commentedRemoving the 8.7.x patch is not happening, and whoops from me for rerolling these patches in the first place.
Comment #41
idebr CreditAttribution: idebr at ezCompany commented#38
Closing this issue as a duplicate 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