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

I agree, but maybe the formatting should be similar to the two other preproccess functions:

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

function template_preprocess_form_element_label(&$variables) {
  ...
  // If title and required marker are both empty, output no label.
  if (isset($element['#title']) && $element['#title'] !== '') {
    $variables['title'] = ['#markup' => $element['#title']];
  }
  ...
}

I think to do this in 8.0.x we may need to add some BC code to Stable to maintain the previous behaviour. Doing this in 8.1.x makes it easier to justify this type of change. Consistency is good.

Works for me.

Merging with work from #2819069: HTML is escaped in "all languages" in Datetime Field UI after SafeMarkup change; didn't address anything else yet. Please make sure @sylus gets credit on this issue.

+++ b/core/includes/form.inc
@@ -250,7 +250,9 @@ function template_preprocess_details(&$variables) {
+  if (isset($element['#title']) && $element['#title'] !== '') {

+++ b/core/includes/theme.inc
@@ -569,8 +569,8 @@ function template_preprocess_datetime_form(&$variables) {
+  if (isset($element['#title']) && $element['#title'] !== '') {

Lets keep the if statement unchanged and use the if (!empty()). This is unnecessary change.

@lauriii with that we can't use '0' in a title, which is rare but we shouldn't be excluding that.

Does need tests though.

@joelpittet TBH that doesn't seem to be in scope of this issue :)

Ah yeah, your probably right.

Also what's wrong with the details title being escaped? This issue needs a use-case in the issue summary.

Updated IS w/ example from #2819069: HTML is escaped in "all languages" in Datetime Field UI after SafeMarkup change.

Updating patch. I guess still needs some tests.

@tstoeckler, this patch is fine.

I came across the problem with the date field of the date range module only, and reproduced that on head of 8.2.x,

BEFORE :

AFTER:

I do not pass this one to RTBC because I didn't reproduced that on other fields yet.

I can also confirm that this patch works both with the $node->created field in the node edit screen sidebar, as well as a multiple cardinality file field on a node.

Node Created Date

Before

After

File Field w/ Multiple Cardinality

Not updating status to RTBC because apparently still needs tests.

I just came across this, and the patch works fine. Should this issue not be moved to 8.4 dev?

This will need test coverage with explicit XSS tests.

#17 works for me

@adelossantos it seems like your patch doesn't contain any new changes. In case you haven't made any changes for the latest patch, there is no need to re-upload the patch. If you tested the latest patch manually, you can write your review in the comment and if you have to make any remarks, you can use screenshots to help the communication.

Leaving this to "Needs Work" because we still need automated test coverage as pointed on #24.

@lauriii noted sorry about that

Stumbled upon this problem and created #2901250: HTML of the (All languages) suffix is not escaped in summary. Closed it in favour of this issue.

Here is the test coverage for the use-case I faced based on the patch #17. The tests can probably be improved now as I wasn't aware of all the places where this problem can appear when I was writing tests.

Came across #2833031: Field label shows HTML if field is marked as not translatable which I closed as duplicated.

The test case looks good for me. Never seen this issue in other cases.
Patch works.

This patch works great for me, thanks @mbovan. My use case is a custom entity with an embedded entity browser (which appears in a details element).

Which patch to use for 8.3.x ?

Hi sgurit,
The patches in #17 (which was at 8.3.x) and #26 (when moved to 8.4.x) are identical, and the actual code change is still the same in #30 (after moving to 8.5.x) so I would guess that #17 or #26 should still apply OK at 8.3.x

+++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
@@ -499,4 +499,27 @@ public function testRevisionTranslationRendering() {
+    $this->assertNoRaw('Image <span class="translation-entity-all-languages">(all languages)</span>');
+    $this->assertRaw('Image <span class="translation-entity-all-languages">(all languages)</span>');
+  }

These are deprecated. Should probably just do one assertion with $this->assertSession()->assertNoEscaped() instead, or at least $this->assertSession()->responseNotContains() and $this->assertSession()->responseContains() instead.

I did some manual testing w/ some hacks, and think this is XSS safe, but we really need an explicit test here to catch any regressions here. Using the string <script>alert(String.fromCharCode(88,83,83))</script> is handy since you don't have to worry about the quotation marks.

NW for the XSS assertion.

To build further on the issue, not escaped title is also used in form validation. Using the patch from #30 and Drupal 8.3.6.

I created a custom node type and added two fields. One which is a regular text field and other that is a datetime field. After that i made the node type translatable, but kept the fields as not translatable. The patch made the title escape the all languages span, but when validating the form it still is not escaped.

Added a screenshot in the attachments. I tried to make the element title a formattable markup in core module content_translaton class ContentTranslationHandler in the method entityFormSharedElements after the translatability clue is added, but to no help.

With drupal 8.4.0

the effect also occurs with some other labels.

The problem persist in 8.4.2

Start date <span class="translation-entity-all-languages">(all languages)</span>
End date <span class="translation-entity-all-languages">(all languages)</span>

But it is solved after applying the patch #30.

BEFORE

AFTER

This fixes the things mentioned in #36. I added a generic test that outputs a hacky XSS title for all elements in core where I thought it made sense.

So this breaks the Migrate Drupal UI form that was updated in #2905227: Migrate UI: Improve 'Review Upgrade' page UX. That now contains this:

    // Missing migrations.
    $missing_module_list = [
      '#type' => 'details',
      '#open' => TRUE,
      '#title' => [
        '#type' => 'html_tag',
        '#tag' => 'span',
        '#value' => $this->t('Missing upgrade paths'),
        '#attributes' => ['id' => ['warning']],
      ],
      '#description' => $this->t('The following items will not be upgraded. For more information see <a href=":migrate">Upgrading from Drupal 6 or 7 to Drupal 8</a>.', [':migrate' => 'https://www.drupal.org/upgrade/migrate']),
      '#weight' => 2,
    ];

(and a similar thing for available migrations, as well)

This is broken by this patch. Not sure what to do about that.

Here's a patch that uses a workaround and adds a @todo for #2930407: Allow specifying summary attributes for details elements. That would be a sensible way forward, in my opinion, but I'm neither a markup nor a migrate expert, so feel free to disagree.

There are few more elements like title which are causing similar issues.
I found one similar in class that provides link element render implementation.
Attached patch for it.

Fixing issue for use cases where $element['#title'] is string
For some cases like mentioned below, when we render this inside form, title is coming as string, instead of markup array or translatable markup object.
So creating array when there is direct string as $element['#title']

$form['test'] = [
      '#type' => 'link',
      '#title' => t('Copy component'),
      '#url' => $copy_url,
      '#weight' => -100,
];

@mohit_aghera: I'm not exactly sure what use-case you are trying to fix with your patch in #50 / #52. But more importantly, are you sure this needs to be part of this issue, or could we split that into a separate issue instead?

Agreed, that seems like a separate issue to me :)

+++ b/core/includes/form.inc
@@ -250,7 +250,9 @@ function template_preprocess_details(&$variables) {
   }
-  $variables['title'] = (!empty($element['#title'])) ? $element['#title'] : '';
+  if (!empty($element['#title'])) {
+    $variables['title'] = ['#markup' => $element['#title']];
+  }

What if only wrap into a render array if it is a string, like the last patch did with the link? Then the existing use case should continue to work for the migrate form?

Hmm... interesting proposal in #55. That would certainly be preferable in terms of backwards-compatibility.

What I think speaks against that is the fact that we then have an inconsistent behavior between details elements and all other form elements, where the former supports a render array as title and the latter does not. So I could also see an argument for keeping it consistent with template_preprocess_form_element_label().

(Although strictly speaking we should change it to if (isset($element['#title']) && $element['#title'] !== '') { to be consistent with that, then.)

Not sure.

@tstoeckler
I was trying to add a link inside form. I used following snippet and added via form alter.

$form['test'] = [
'#type' => 'link',
'#title' => t('Copy component'),
'#url' => $copy_url,
'#weight' => -100,
];

Now when i enable translations, and i see the link in the form it is displayed like this (i.e. button title)
"Copy component <span class="translation-entity-all-languages">(' . t('all languages') . ')</span>"

Above patch in #52 fixes that issue.

However as you and @Berdir mentioned, it will be better to split it up and create different issue for this.

#52 also fixed the issue for me, but has some failing tests. Are these related or unrelated? Or do the tests have to be changed accordingly?

Attached is a reroll of #43 against 8.6.x

forgot the patch...

I tested path #60. It's fixed bug.It also fixes problems with the third-party module address.Thanks.

Confirming #61. Looking forward to have this in 8.6! :)

1. +++ b/core/includes/form.inc
   @@ -250,7 +250,9 @@ function template_preprocess_details(&$variables) {
   +  if (!empty($element['#title'])) {
   +    $variables['title'] = ['#markup' => $element['#title']];
   +  }
   
   +++ b/core/includes/theme.inc
   @@ -569,7 +569,7 @@ function template_preprocess_datetime_wrapper(&$variables) {
   -    $variables['title'] = $element['#title'];
   +    $variables['title'] = ['#markup' => $element['#title']];
   

   This we should still set the #title to an empty string.

   I think we should only convert to an array if it is not already an array.

2. +++ b/core/modules/migrate_drupal_ui/src/Form/ReviewForm.php
   @@ -257,15 +257,20 @@ public function buildForm(array $form, FormStateInterface $form_state) {
        // Missing migrations.
   +    // @todo Use #summary_attributes after
   +    //   https://www.drupal.org/project/drupal/issues/2930407
   +    /* @var \Drupal\Core\Render\RendererInterface $renderer */
   +    $renderer = \Drupal::service('renderer');
   +    $missing_module_list_title = [
   +      '#type' => 'html_tag',
   +      '#tag' => 'span',
   +      '#value' => $this->t('Modules that will not be upgraded'),
   +      '#attributes' => ['id' => ['error']],
   +    ];
        $missing_module_list = [
          '#type' => 'details',
          '#open' => TRUE,
   -      '#title' => [
   -        '#type' => 'html_tag',
   -        '#tag' => 'span',
   -        '#value' => $this->t('Modules that will not be upgraded'),
   -        '#attributes' => ['id' => ['error']],
   -      ],
   +      '#title' => $renderer->render($missing_module_list_title),
   
   @@ -299,14 +304,17 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // @todo Use #summary_attributes after
   +    //   https://www.drupal.org/project/drupal/issues/2930407
   +    $available_module_list_title = [
   +      '#type' => 'html_tag',
   +      '#tag' => 'span',
   +      '#value' => $this->t('Modules that will be upgraded'),
   +      '#attributes' => ['id' => ['checked']],
   +    ];
        $available_module_list = [
          '#type' => 'details',
   -      '#title' => [
   -        '#type' => 'html_tag',
   -        '#tag' => 'span',
   -        '#value' => $this->t('Modules that will be upgraded'),
   -        '#attributes' => ['id' => ['checked']],
   -      ],
   +      '#title' => $renderer->render($available_module_list_title),
          '#weight' => 3,
        ];
   

   This change should be reverted and is a sign the approach is not quite right.

3. +++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
   @@ -501,4 +501,28 @@ public function testRevisionTranslationRendering() {
   +  /**
   +   * Test that title is not escaped for details form element.
   +   */
   

   I think we should add a note that it is XSS filtered here. Because otherwise this sounds scary.

4. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestLabelForm.php
   @@ -125,6 +159,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        '#title' => "$type: <script>alert('XSS')</script>",
   
   +++ b/core/modules/system/tests/src/Functional/Form/ElementsLabelsTest.php
   @@ -96,6 +98,18 @@ public function testFormLabels() {
   +      $script = "$type: <script>alert('XSS')</script>";
   

   Let's make the string
   $type <script>alert('XSS')</script>is XSS filtered!
   because that makes the responseContains assertion better.

5. +++ b/core/modules/system/tests/src/Functional/Form/ElementsLabelsTest.php
   @@ -96,6 +98,18 @@ public function testFormLabels() {
   +      $this->assertSession()->responseContains(Xss::filterAdmin($script));
   

   Let's hard code the result of the filter rather than relying on the same code in both the system-under-test and assertion.

Patch no longer applied because #2930407: Allow specifying summary attributes for details elements landed in the meantime. Luckily this allows us to remove the hunk from Migrate Drupal UI. I "rerolled" the patch simply by removing that from the patch file.

Also removing the "Quick fix" tag... ;-)

Also fixed #63:

1. Fixed this. Re-reading #55, #56 and #63 I changed the condition so that it
   • is fully backwards-compatible. I.e. wouldn't require any changes in Migrate Drupal UI even if #2930407: Allow specifying summary attributes for details elements wasn't in yet and it also always sets $variables['title'] even if $element['#title'] is not set so that other preprocess functions can rely on it.
   • avoids needlessly creating a render array for an empty string
   • allows '0' as a title

   so I think should cover all bases. Added the same condition to template_preprocess_datetime_wrapper(), as well.

   This makes the code a bit more verbose than previously but I couldn't come up with a nicer solution that satisfies all the above conditions. Feel free to suggest improvements!

2. Removed this per the above(s).
3. ✔
4. ✔
5. ✔

OK, that wasn't so smart, as I wasn't setting the title variable at all for e.g. markup objects.

This one then tries to go for the minimally invasive fix. It reverts the fix to allow '0' titles as that's a separate issue as I now realize that was discussed above already. Let's see...

Did a manual test with a element to confirm the patch works as expected.

Before:

After:

Also added a 'tests-only' patch to confirm the state in HEAD is incorrect. Actual patch content is unchanged.

Committed 252f7ff and pushed to 8.6.x. Thanks!

Yay, thanks!

Since this is a bugfix, could we get this into 8.5, as well? Not a big deal if not, but wanted to ask...

Thanks for fixing this!

I'll second the request for this to be backported to 8.5.x, it has been causing problems for sites using Metatag.

OK, let's do this, then.

Works for me on 8.5.5

Same as above - works on 8.5.5

8.5.x is now in critical bugfix only mode ¯\_(ツ)_/¯

OK, fair enough. Fixing version, though.