From 252f7ffcf2bbbf5bfbd23894a63e44ae9f9d7e54 Mon Sep 17 00:00:00 2001
From: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date: Fri, 22 Jun 2018 13:33:12 +0100
Subject: [PATCH] Issue #2652850 by tstoeckler, mohit_aghera, idebr, hchonov,
mbovan, rodrigoaguilera, jwilson3, pguillard, espurnes, dravenk, ainarend,
handkerchief, alexpott: 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
---
core/includes/form.inc | 5 ++
core/includes/theme.inc | 5 ++
.../src/Functional/NodeTranslationUITest.php | 24 +++++++++
.../modules/form_test/form_test.info.yml | 3 ++
.../form_test/src/Form/FormTestLabelForm.php | 49 +++++++++++++++++++
.../Functional/Form/ElementsLabelsTest.php | 12 +++++
6 files changed, 98 insertions(+)
diff --git a/core/includes/form.inc b/core/includes/form.inc
index d248b7b5a9a9..dacae875bdeb 100644
--- a/core/includes/form.inc
+++ b/core/includes/form.inc
@@ -251,6 +251,11 @@ function template_preprocess_details(&$variables) {
$variables['summary_attributes']['aria-pressed'] = $variables['summary_attributes']['aria-expanded'];
}
$variables['title'] = (!empty($element['#title'])) ? $element['#title'] : '';
+ // If the element title is a string, wrap it a render array so that markup
+ // will not be escaped (but XSS-filtered).
+ if (is_string($variables['title']) && $variables['title'] !== '') {
+ $variables['title'] = ['#markup' => $variables['title']];
+ }
$variables['description'] = (!empty($element['#description'])) ? $element['#description'] : '';
$variables['children'] = (isset($element['#children'])) ? $element['#children'] : '';
$variables['value'] = (isset($element['#value'])) ? $element['#value'] : '';
diff --git a/core/includes/theme.inc b/core/includes/theme.inc
index 30e5aafc6c71..5b9db30831f7 100644
--- a/core/includes/theme.inc
+++ b/core/includes/theme.inc
@@ -570,6 +570,11 @@ function template_preprocess_datetime_wrapper(&$variables) {
if (!empty($element['#title'])) {
$variables['title'] = $element['#title'];
+ // If the element title is a string, wrap it a render array so that markup
+ // will not be escaped (but XSS-filtered).
+ if (is_string($variables['title']) && $variables['title'] !== '') {
+ $variables['title'] = ['#markup' => $variables['title']];
+ }
}
// Suppress error messages.
diff --git a/core/modules/node/tests/src/Functional/NodeTranslationUITest.php b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
index 46516fe74aad..513c074932e3 100644
--- a/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
+++ b/core/modules/node/tests/src/Functional/NodeTranslationUITest.php
@@ -501,4 +501,28 @@ public function testRevisionTranslationRendering() {
$this->assertNoText('First rev en title');
}
+ /**
+ * Test that title is not escaped (but XSS-filtered) for details form element.
+ */
+ public function testDetailsTitleIsNotEscaped() {
+ $this->drupalLogin($this->administrator);
+ // Make the image field a multi-value field in order to display a
+ // details form element.
+ $edit = ['cardinality_number' => 2];
+ $this->drupalPostForm('admin/structure/types/manage/article/fields/node.article.field_image/storage', $edit, t('Save field settings'));
+
+ // Make the image field non-translatable.
+ $edit = ['settings[node][article][fields][field_image]' => FALSE];
+ $this->drupalPostForm('admin/config/regional/content-language', $edit, t('Save configuration'));
+
+ // Create a node.
+ $nid = $this->createEntity(['title' => 'Node with multi-value image field en title'], 'en');
+
+ // Add a French translation and assert the title markup is not escaped.
+ $this->drupalGet("node/$nid/translations/add/en/fr");
+ $markup = 'Image <span class="translation-entity-all-languages">(all languages)</span>';
+ $this->assertSession()->assertNoEscaped($markup);
+ $this->assertSession()->responseContains($markup);
+ }
+
}
diff --git a/core/modules/system/tests/modules/form_test/form_test.info.yml b/core/modules/system/tests/modules/form_test/form_test.info.yml
index 39d45eaaf838..fdf1076ef604 100644
--- a/core/modules/system/tests/modules/form_test/form_test.info.yml
+++ b/core/modules/system/tests/modules/form_test/form_test.info.yml
@@ -4,3 +4,6 @@ description: 'Support module for Form API tests.'
package: Testing
version: VERSION
core: 8.x
+dependencies:
+ - file
+ - filter
diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestLabelForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestLabelForm.php
index 88de2300898a..a22bf7d09210 100644
--- a/core/modules/system/tests/modules/form_test/src/Form/FormTestLabelForm.php
+++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestLabelForm.php
@@ -12,6 +12,40 @@
*/
class FormTestLabelForm extends FormBase {
+ /**
+ * An array of elements that render a title.
+ *
+ * @var array
+ */
+ public static $typesWithTitle = [
+ 'checkbox',
+ 'checkboxes',
+ 'color',
+ 'date',
+ 'datelist',
+ 'datetime',
+ 'details',
+ 'email',
+ 'fieldset',
+ 'file',
+ 'item',
+ 'managed_file',
+ 'number',
+ 'password',
+ 'password_confirm',
+ 'radio',
+ 'radios',
+ 'range',
+ 'search',
+ 'select',
+ 'tel',
+ 'textarea',
+ 'textfield',
+ 'text_format',
+ 'url',
+ 'weight',
+ ];
+
/**
* {@inheritdoc}
*/
@@ -125,6 +159,21 @@ public function buildForm(array $form, FormStateInterface $form_state) {
],
'#required' => TRUE,
];
+
+ foreach (static::$typesWithTitle as $type) {
+ $form['form_' . $type . '_title_no_xss'] = [
+ '#type' => $type,
+ '#title' => "$type <script>alert('XSS')</script> is XSS filtered!",
+ ];
+ // Add keys that are required for some elements to be processed correctly.
+ if (in_array($type, ['checkboxes', 'radios'], TRUE)) {
+ $form['form_' . $type . '_title_no_xss']['#options'] = [];
+ }
+ if ($type === 'datetime') {
+ $form['form_' . $type . '_title_no_xss']['#default_value'] = NULL;
+ }
+ }
+
return $form;
}
diff --git a/core/modules/system/tests/src/Functional/Form/ElementsLabelsTest.php b/core/modules/system/tests/src/Functional/Form/ElementsLabelsTest.php
index 6747ed63fb10..91f3ec4a59d4 100644
--- a/core/modules/system/tests/src/Functional/Form/ElementsLabelsTest.php
+++ b/core/modules/system/tests/src/Functional/Form/ElementsLabelsTest.php
@@ -2,6 +2,7 @@
namespace Drupal\Tests\system\Functional\Form;
+use Drupal\form_test\Form\FormTestLabelForm;
use Drupal\Tests\BrowserTestBase;
/**
@@ -96,6 +97,17 @@ public function testFormLabels() {
$this->assertTrue(!empty($elements), "Title/Label not displayed when 'visually-hidden' attribute is set in radios.");
}
+ /**
+ * Tests XSS-protection of element labels.
+ */
+ public function testTitleEscaping() {
+ $this->drupalGet('form_test/form-labels');
+ foreach (FormTestLabelForm::$typesWithTitle as $type) {
+ $this->assertSession()->responseContains("$type alert('XSS') is XSS filtered!");
+ $this->assertSession()->responseNotContains("$type <script>alert('XSS')</script> is XSS filtered!");
+ }
+ }
+
/**
* Tests different display options for form element descriptions.
*/
--
GitLab