From cc574ff23c712f8aea113fc9384f7627b752bf05 Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Tue, 15 Sep 2015 13:42:04 +0100 Subject: [PATCH] Issue #2560863 by alexpott, stefan.r, lauriii: #options for radios and checkboxes uses SafeMarkup::checkPlain() to escape - use Html::escape() instead --- .../SelectionInterface.php | 2 +- .../SelectionBase.php | 4 +- .../src/ConfigurableEntityReferenceItem.php | 10 ++- .../src/Tests/EntityReferenceXSSTest.php | 82 +++++++++++++++++++ .../filter/src/FilterFormatFormBase.php | 2 +- .../node/src/Plugin/Search/NodeSearch.php | 2 +- .../Tests/Entity/EntityAutocompleteTest.php | 8 +- .../system/src/Tests/Form/ElementTest.php | 6 ++ .../system/src/Tests/Form/FormTest.php | 4 + .../src/Form/FormTestCheckboxesRadiosForm.php | 8 +- .../form_test/src/Form/FormTestSelectForm.php | 2 +- .../TermSelection.php | 4 +- .../Plugin/views/filter/TaxonomyIndexTid.php | 4 +- core/modules/user/src/AccountForm.php | 2 +- .../user/src/Plugin/views/access/Role.php | 2 +- .../Plugin/views/argument_validator/User.php | 2 +- .../src/Plugin/views/filter/Permissions.php | 4 +- .../exposed_form/ExposedFormPluginBase.php | 4 +- .../Plugin/views/filter/FilterPluginBase.php | 2 +- 19 files changed, 124 insertions(+), 30 deletions(-) create mode 100644 core/modules/entity_reference/src/Tests/EntityReferenceXSSTest.php diff --git a/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionInterface.php b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionInterface.php index 65d86c9aaf1..864e717d857 100644 --- a/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionInterface.php +++ b/core/lib/Drupal/Core/Entity/EntityReferenceSelection/SelectionInterface.php @@ -27,7 +27,7 @@ interface SelectionInterface extends PluginFormInterface { * * @return array * A nested array of entities, the first level is keyed by the - * entity bundle, which contains an array of entity labels (safe HTML), + * entity bundle, which contains an array of entity labels (escaped), * keyed by the entity ID. */ public function getReferenceableEntities($match = NULL, $match_operator = 'CONTAINS', $limit = 0); diff --git a/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php index cb8c7d668af..c67dc682341 100644 --- a/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php +++ b/core/lib/Drupal/Core/Entity/Plugin/EntityReferenceSelection/SelectionBase.php @@ -7,7 +7,7 @@ namespace Drupal\Core\Entity\Plugin\EntityReferenceSelection; -use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\Html; use Drupal\Core\Database\Query\AlterableInterface; use Drupal\Core\Database\Query\SelectInterface; use Drupal\Core\Entity\EntityManagerInterface; @@ -235,7 +235,7 @@ class SelectionBase extends PluginBase implements SelectionInterface, ContainerF $entities = entity_load_multiple($target_type, $result); foreach ($entities as $entity_id => $entity) { $bundle = $entity->bundle(); - $options[$bundle][$entity_id] = SafeMarkup::checkPlain($entity->label()); + $options[$bundle][$entity_id] = Html::escape($entity->label()); } return $options; diff --git a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php index be11617e3ac..3ce16ab5678 100644 --- a/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php +++ b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php @@ -7,7 +7,7 @@ namespace Drupal\entity_reference; -use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\Html; use Drupal\Core\Entity\EntityTypeInterface; use Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem; use Drupal\Core\Field\PreconfiguredFieldUiOptionsInterface; @@ -80,7 +80,9 @@ class ConfigurableEntityReferenceItem extends EntityReferenceItem implements Opt $return = array(); foreach ($options as $bundle => $entity_ids) { - $bundle_label = SafeMarkup::checkPlain($bundles[$bundle]['label']); + // The label does not need sanitizing since it is used as an optgroup + // which is only supported by select elements and auto-escaped. + $bundle_label = $bundles[$bundle]['label']; $return[$bundle_label] = $entity_ids; } @@ -135,11 +137,11 @@ class ConfigurableEntityReferenceItem extends EntityReferenceItem implements Opt // entity type specific plugins (e.g., 'default:node', 'default:user', // etc.). if (array_key_exists($selection_group_id, $selection_plugins[$selection_group_id])) { - $handlers_options[$selection_group_id] = SafeMarkup::checkPlain($selection_plugins[$selection_group_id][$selection_group_id]['label']); + $handlers_options[$selection_group_id] = Html::escape($selection_plugins[$selection_group_id][$selection_group_id]['label']); } elseif (array_key_exists($selection_group_id . ':' . $this->getSetting('target_type'), $selection_plugins[$selection_group_id])) { $selection_group_plugin = $selection_group_id . ':' . $this->getSetting('target_type'); - $handlers_options[$selection_group_plugin] = SafeMarkup::checkPlain($selection_plugins[$selection_group_id][$selection_group_plugin]['base_plugin_label']); + $handlers_options[$selection_group_plugin] = Html::escape($selection_plugins[$selection_group_id][$selection_group_plugin]['base_plugin_label']); } } diff --git a/core/modules/entity_reference/src/Tests/EntityReferenceXSSTest.php b/core/modules/entity_reference/src/Tests/EntityReferenceXSSTest.php new file mode 100644 index 00000000000..cea10f20ad6 --- /dev/null +++ b/core/modules/entity_reference/src/Tests/EntityReferenceXSSTest.php @@ -0,0 +1,82 @@ +drupalCreateContentType(['type' => 'article']); + + // Create a node with markup in the title. + $node_type_one = $this->drupalCreateContentType(); + $node = [ + 'type' => $node_type_one->id(), + 'title' => 'I am kitten', + ]; + $referenced_node = $this->drupalCreateNode($node); + + $node_type_two = $this->drupalCreateContentType(['name' => 'bundle with markup']); + $this->drupalCreateNode([ + 'type' => $node_type_two->id(), + 'title' => 'My bundle has markup', + ]); + + $this->createEntityReferenceField('node', 'article', 'entity_reference_test', 'Entity Reference test', 'node', 'default', ['target_bundles' => [$node_type_one->id(), $node_type_two->id()]]); + + EntityFormDisplay::load('node.article.default') + ->setComponent('entity_reference_test', ['type' => 'options_select']) + ->save(); + EntityViewDisplay::load('node.article.default') + ->setComponent('entity_reference_test', ['type' => 'entity_reference_label']) + ->save(); + + // Create a node and reference the node with markup in the title. + $this->drupalLogin($this->rootUser); + $this->drupalGet('node/add/article'); + $this->assertEscaped($referenced_node->getTitle()); + $this->assertEscaped($node_type_two->label()); + + $edit = [ + 'title[0][value]' => $this->randomString(), + 'entity_reference_test' => $referenced_node->id() + ]; + $this->drupalPostForm(NULL, $edit, 'Save and publish'); + $this->assertEscaped($referenced_node->getTitle()); + + // Test the options_buttons type. + EntityFormDisplay::load('node.article.default') + ->setComponent('entity_reference_test', ['type' => 'options_buttons']) + ->save(); + $this->drupalGet('node/add/article'); + $this->assertEscaped($referenced_node->getTitle()); + // options_buttons does not support optgroups. + $this->assertNoText('bundle with markup'); + } + +} diff --git a/core/modules/filter/src/FilterFormatFormBase.php b/core/modules/filter/src/FilterFormatFormBase.php index 77b54335984..e1222b45d19 100644 --- a/core/modules/filter/src/FilterFormatFormBase.php +++ b/core/modules/filter/src/FilterFormatFormBase.php @@ -79,7 +79,7 @@ abstract class FilterFormatFormBase extends EntityForm { $form['roles'] = array( '#type' => 'checkboxes', '#title' => $this->t('Roles'), - '#options' => array_map('\Drupal\Component\Utility\SafeMarkup::checkPlain', user_role_names()), + '#options' => array_map('\Drupal\Component\Utility\Html::escape', user_role_names()), '#disabled' => $is_fallback, '#weight' => -10, ); diff --git a/core/modules/node/src/Plugin/Search/NodeSearch.php b/core/modules/node/src/Plugin/Search/NodeSearch.php index ce5ddb66aef..ecd6e869f85 100644 --- a/core/modules/node/src/Plugin/Search/NodeSearch.php +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php @@ -554,7 +554,7 @@ class NodeSearch extends ConfigurableSearchPluginBase implements AccessibleInter ); // Add node types. - $types = array_map(array('\Drupal\Component\Utility\SafeMarkup', 'checkPlain'), node_type_get_names()); + $types = array_map(array('\Drupal\Component\Utility\Html', 'escape'), node_type_get_names()); $form['advanced']['types-fieldset'] = array( '#type' => 'fieldset', '#title' => t('Types'), diff --git a/core/modules/system/src/Tests/Entity/EntityAutocompleteTest.php b/core/modules/system/src/Tests/Entity/EntityAutocompleteTest.php index 4db0e209bce..d42a07a1de4 100644 --- a/core/modules/system/src/Tests/Entity/EntityAutocompleteTest.php +++ b/core/modules/system/src/Tests/Entity/EntityAutocompleteTest.php @@ -57,8 +57,8 @@ class EntityAutocompleteTest extends EntityUnitTestBase { $entity_2 = entity_create($this->entityType, array('name' => '10/17/2011')); $entity_2->save(); - // Add another entity that has both a comma and a slash character. - $entity_3 = entity_create($this->entityType, array('name' => 'label with, and / test')); + // Add another entity that has both a comma, a slash and markup. + $entity_3 = entity_create($this->entityType, array('name' => 'label with, and / test')); $entity_3->save(); // Try to autocomplete a entity label that matches both entities. @@ -84,8 +84,8 @@ class EntityAutocompleteTest extends EntityUnitTestBase { $data = $this->getAutocompleteResult($input); $this->assertIdentical($data[0]['label'], Html::escape($entity_2->name->value), 'Autocomplete returned the second matching entity'); - // Try to autocomplete a entity label with both a comma and a slash. - $input = '"label with, and / t'; + // Try to autocomplete a entity label with both a comma, a slash and markup. + $input = '"label with, and / '; $data = $this->getAutocompleteResult($input); $n = $entity_3->name->value . ' (3)'; // Entity labels containing commas or quotes must be wrapped in quotes. diff --git a/core/modules/system/src/Tests/Form/ElementTest.php b/core/modules/system/src/Tests/Form/ElementTest.php index b7295ff49e1..1cc6cd8d814 100644 --- a/core/modules/system/src/Tests/Form/ElementTest.php +++ b/core/modules/system/src/Tests/Form/ElementTest.php @@ -62,6 +62,12 @@ class ElementTest extends WebTestBase { } } + // Verify that the choices are admin filtered as expected. + $this->assertRaw("Special Charalert('checkboxes');"); + $this->assertRaw("Special Charalert('radios');"); + $this->assertRaw('Bar - checkboxes'); + $this->assertRaw('Bar - radios'); + // Enable customized option sub-elements. $this->drupalGet('form-test/checkboxes-radios/customize'); diff --git a/core/modules/system/src/Tests/Form/FormTest.php b/core/modules/system/src/Tests/Form/FormTest.php index d3d182645d6..83b9129014a 100644 --- a/core/modules/system/src/Tests/Form/FormTest.php +++ b/core/modules/system/src/Tests/Form/FormTest.php @@ -363,6 +363,10 @@ class FormTest extends WebTestBase { $form = \Drupal::formBuilder()->getForm('Drupal\form_test\Form\FormTestSelectForm'); $this->drupalGet('form-test/select'); + // Verify that the options are escaped as expected. + $this->assertEscaped('four'); + $this->assertNoRaw('four'); + // Posting without any values should throw validation errors. $this->drupalPostForm(NULL, array(), 'Submit'); $no_errors = array( diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestCheckboxesRadiosForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestCheckboxesRadiosForm.php index 7bd43747a21..318f618c3d0 100644 --- a/core/modules/system/tests/modules/form_test/src/Form/FormTestCheckboxesRadiosForm.php +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestCheckboxesRadiosForm.php @@ -36,8 +36,8 @@ class FormTestCheckboxesRadiosForm extends FormBase { 0 => 'Zero', 'foo' => 'Foo', 1 => 'One', - 'bar' => 'Bar', - '>' => 'Special Char', + 'bar' => $this->t('Bar - checkboxes'), + '>' => "Special Char", ), ); if ($customize) { @@ -60,8 +60,8 @@ class FormTestCheckboxesRadiosForm extends FormBase { 0 => 'Zero', 'foo' => 'Foo', 1 => 'One', - 'bar' => 'Bar', - '>' => 'Special Char', + 'bar' => 'Bar - radios', + '>' => "Special Char", ), ); if ($customize) { diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestSelectForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestSelectForm.php index dab04346f39..9c2953a1067 100644 --- a/core/modules/system/tests/modules/form_test/src/Form/FormTestSelectForm.php +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestSelectForm.php @@ -29,7 +29,7 @@ class FormTestSelectForm extends FormBase { public function buildForm(array $form, FormStateInterface $form_state) { $base = array( '#type' => 'select', - '#options' => array('one' => 'one', 'two' => 'two', 'three' => 'three'), + '#options' => array('one' => 'one', 'two' => 'two', 'three' => 'three', 'four' => 'four'), ); $form['select'] = $base + array( diff --git a/core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php b/core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php index ddf99cabc93..1ddde502bbe 100644 --- a/core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php +++ b/core/modules/taxonomy/src/Plugin/EntityReferenceSelection/TermSelection.php @@ -7,7 +7,7 @@ namespace Drupal\taxonomy\Plugin\EntityReferenceSelection; -use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\Html; use Drupal\Core\Database\Query\SelectInterface; use Drupal\Core\Entity\Plugin\EntityReferenceSelection\SelectionBase; use Drupal\Core\Form\FormStateInterface; @@ -73,7 +73,7 @@ class TermSelection extends SelectionBase { if ($vocabulary = Vocabulary::load($bundle)) { if ($terms = $this->entityManager->getStorage('taxonomy_term')->loadTree($vocabulary->id(), 0, NULL, TRUE)) { foreach ($terms as $term) { - $options[$vocabulary->id()][$term->id()] = str_repeat('-', $term->depth) . SafeMarkup::checkPlain($term->getName()); + $options[$vocabulary->id()][$term->id()] = str_repeat('-', $term->depth) . Html::escape($term->getName()); } } } diff --git a/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php index 38c0f2db3a5..5c46a85129a 100644 --- a/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php @@ -183,7 +183,7 @@ class TaxonomyIndexTid extends ManyToOne { if ($tree) { foreach ($tree as $term) { $choice = new \stdClass(); - $choice->option = array($term->id() => str_repeat('-', $term->depth) . SafeMarkup::checkPlain(\Drupal::entityManager()->getTranslationFromContext($term)->label())); + $choice->option = array($term->id() => str_repeat('-', $term->depth) . \Drupal::entityManager()->getTranslationFromContext($term)->label()); $options[] = $choice; } } @@ -201,7 +201,7 @@ class TaxonomyIndexTid extends ManyToOne { } $terms = Term::loadMultiple($query->execute()); foreach ($terms as $term) { - $options[$term->id()] = SafeMarkup::checkPlain(\Drupal::entityManager()->getTranslationFromContext($term)->label()); + $options[$term->id()] = \Drupal::entityManager()->getTranslationFromContext($term)->label(); } } diff --git a/core/modules/user/src/AccountForm.php b/core/modules/user/src/AccountForm.php index 63c6739679d..45423c56258 100644 --- a/core/modules/user/src/AccountForm.php +++ b/core/modules/user/src/AccountForm.php @@ -205,7 +205,7 @@ abstract class AccountForm extends ContentEntityForm { '#access' => $admin, ); - $roles = array_map(array('\Drupal\Component\Utility\SafeMarkup', 'checkPlain'), user_role_names(TRUE)); + $roles = array_map(array('\Drupal\Component\Utility\Html', 'escape'), user_role_names(TRUE)); $form['account']['roles'] = array( '#type' => 'checkboxes', diff --git a/core/modules/user/src/Plugin/views/access/Role.php b/core/modules/user/src/Plugin/views/access/Role.php index 4c2b732560e..cd3a32233b2 100644 --- a/core/modules/user/src/Plugin/views/access/Role.php +++ b/core/modules/user/src/Plugin/views/access/Role.php @@ -115,7 +115,7 @@ class Role extends AccessPluginBase implements CacheablePluginInterface { '#type' => 'checkboxes', '#title' => $this->t('Role'), '#default_value' => $this->options['role'], - '#options' => array_map('\Drupal\Component\Utility\SafeMarkup::checkPlain', user_role_names()), + '#options' => array_map('\Drupal\Component\Utility\Html::escape', user_role_names()), '#description' => $this->t('Only the checked roles will be able to access this display.'), ); } diff --git a/core/modules/user/src/Plugin/views/argument_validator/User.php b/core/modules/user/src/Plugin/views/argument_validator/User.php index ede8dd51ac2..416008db9e7 100644 --- a/core/modules/user/src/Plugin/views/argument_validator/User.php +++ b/core/modules/user/src/Plugin/views/argument_validator/User.php @@ -65,7 +65,7 @@ class User extends Entity { $form['roles'] = array( '#type' => 'checkboxes', '#title' => $this->t('Restrict to the selected roles'), - '#options' => array_map(array('\Drupal\Component\Utility\SafeMarkup', 'checkPlain'), user_role_names(TRUE)), + '#options' => array_map(array('\Drupal\Component\Utility\Html', 'escape'), user_role_names(TRUE)), '#default_value' => $this->options['roles'], '#description' => $this->t('If no roles are selected, users from any role will be allowed.'), '#states' => array( diff --git a/core/modules/user/src/Plugin/views/filter/Permissions.php b/core/modules/user/src/Plugin/views/filter/Permissions.php index a637705271b..c113b2eee39 100644 --- a/core/modules/user/src/Plugin/views/filter/Permissions.php +++ b/core/modules/user/src/Plugin/views/filter/Permissions.php @@ -7,7 +7,7 @@ namespace Drupal\user\Plugin\views\filter; -use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\Html; use Drupal\Core\Extension\ModuleHandlerInterface; use Drupal\user\PermissionHandlerInterface; use Drupal\views\Plugin\views\filter\ManyToOne; @@ -76,7 +76,7 @@ class Permissions extends ManyToOne { foreach ($permissions as $perm => $perm_item) { $provider = $perm_item['provider']; $display_name = $this->moduleHandler->getName($provider); - $this->valueOptions[$display_name][$perm] = SafeMarkup::checkPlain(strip_tags($perm_item['title'])); + $this->valueOptions[$display_name][$perm] = Html::escape(strip_tags($perm_item['title'])); } } else { diff --git a/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php b/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php index d17f70c7828..60d0b66ba87 100644 --- a/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php +++ b/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php @@ -7,7 +7,7 @@ namespace Drupal\views\Plugin\views\exposed_form; -use Drupal\Component\Utility\SafeMarkup; +use Drupal\Component\Utility\Html; use Drupal\Core\Form\FormState; use Drupal\Core\Form\FormStateInterface; use Drupal\views\Form\ViewsExposedForm; @@ -211,7 +211,7 @@ abstract class ExposedFormPluginBase extends PluginBase implements CacheablePlug $exposed_sorts = array(); foreach ($this->view->sort as $id => $handler) { if ($handler->canExpose() && $handler->isExposed()) { - $exposed_sorts[$id] = SafeMarkup::checkPlain($handler->options['expose']['label']); + $exposed_sorts[$id] = Html::escape($handler->options['expose']['label']); } } diff --git a/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php index 6df8b447219..e3988239589 100644 --- a/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php @@ -594,7 +594,7 @@ abstract class FilterPluginBase extends HandlerBase implements CacheablePluginIn '#default_value' => $this->options['expose']['remember'], ); - $role_options = array_map('\Drupal\Component\Utility\SafeMarkup::checkPlain', user_role_names()); + $role_options = array_map('\Drupal\Component\Utility\Html::escape', user_role_names()); $form['expose']['remember_roles'] = array( '#type' => 'checkboxes', '#title' => $this->t('User roles'),