#options for radios and checkboxes uses SafeMarkup::checkPlain() to escape - use Html::escape() instead

Looks like for the OptionsWidgetsTests, we now autoescape instead of filter, and for the entity reference ones, the labels we get from getReferenceableEntities() are now not sanitized as mentioned in the interface docs.

Just a quick review of the code.

1. +++ b/core/lib/Drupal/Core/Render/Element/Checkbox.php
   @@ -106,6 +109,12 @@ public static function preRenderCheckbox($element) {
   +    // Ensure the title is safe. Default to escaping to make this the same as
   +    // Twig's auto-escape feature.
   +    if (isset($element['#title']) && !SafeMarkup::isSafe($element['#title'])) {
   +      $element['#title'] = SafeString::create(HtmlUtility::escape($element['#title']));
   +    }
   +
   
   +++ b/core/lib/Drupal/Core/Render/Element/Radio.php
   @@ -66,6 +69,12 @@ public static function preRenderRadio($element) {
   +    // Ensure the title is safe. Default to escaping to make this the same as
   +    // Twig's auto-escape feature.
   +    if (isset($element['#title']) && !SafeMarkup::isSafe($element['#title'])) {
   +      $element['#title'] = SafeString::create(HtmlUtility::escape($element['#title']));
   +    }
   +
   

   Isn't it possible to prevent this code duplication?

2. +++ b/core/modules/options/src/Tests/OptionsWidgetsTest.php
   @@ -112,8 +112,9 @@ function testRadioButtons() {
   +    $this->assertRaw('Some <script>dangerous</script> & unescaped <strong>markup</strong>', 'Option text was properly escaped.');
   ...
   +    $this->assertRaw('Some HTML encoded markup with < & >');
   
   @@ -169,7 +170,7 @@ function testCheckBoxes() {
   +    $this->assertRaw('Some <script>dangerous</script> & unescaped <strong>markup</strong>', 'Option text was properly escaped.');
   
   @@ -264,7 +265,7 @@ function testSelectListSingle() {
   +    $this->assertRaw('Some <script>dangerous</script> & unescaped <strong>markup</strong>', 'Option text was properly escaped.');
   
   @@ -308,8 +309,7 @@ function testSelectListSingle() {
   +    $this->assertRaw('Some <script>dangerous</script> & unescaped <strong>markup</strong>', 'Option text was properly escaped.');
   
   @@ -359,7 +359,7 @@ function testSelectListMultiple() {
   +    $this->assertRaw('Some <script>dangerous</script> & unescaped <strong>markup</strong>', 'Option text was properly escaped.');
   
   @@ -429,8 +429,7 @@ function testSelectListMultiple() {
   +    $this->assertRaw('Some <script>dangerous</script> & unescaped <strong>markup</strong>', 'Option text was properly escaped.');
   

   Shouldn't these be $this->assertEscaped like in ElementTest::testOptions

Just want to make sure that we don't regress #1919338: Select widget (from the options module) prone to double encoding here.

@legolasbo we could make the labels FieldFilteredString again so the test changes are not needed (i.e. revert some of the changes in #3). You can take a stab at this and post a patch if you like?

@stefan.r,

I would love to take a stab at it, but I will not have time to do so until Friday. Just reviewing some patches between work issues to clear my head.

1. +++ b/core/modules/options/src/Plugin/Field/FieldFormatter/OptionsDefaultFormatter.php
   @@ -49,6 +49,7 @@ public function viewElements(FieldItemListInterface $items) {
   +        // @todo was this actaully checkPlain'd already?
   

   ^

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -47,6 +47,14 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // Select form inputs allow unencoded HTML entities, but no HTML tags.
   +    $label = Html::decodeEntities(strip_tags($label));
   

   This seems a bit odd and I wonder if escaping wouldn't make more sense, but I guess we don't want to change existing behavior here.

3. +++ b/core/modules/system/src/Tests/Form/ElementTest.php
   @@ -62,6 +62,12 @@ function testOptions() {
   +    $this->assertEscaped("<em>Special Char</em><scrip>alert('checkboxes');</script>");
   +    $this->assertEscaped("<em>Special Char</em><scrip>alert('radios');</script>");
   
   +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestCheckboxesRadiosForm.php
   @@ -36,8 +37,8 @@ public function buildForm(array $form, FormStateInterface $form_state, $customiz
   +        '>' => "<em>Special Char</em><scrip>alert('checkboxes');</script>",
   
   @@ -60,8 +61,8 @@ public function buildForm(array $form, FormStateInterface $form_state, $customiz
   +        '>' => "<em>Special Char</em><scrip>alert('radios');</script>",
   

   scrip

Patch looks good, this all looks fairly straightforward. For the radios/checkboxes/selects I assume we already have plenty of automated test coverage...

Just posting my progress if someone wants to give a shot on this one. I have created a base for the test but there is still one fail because for some reason the node doesn't still display the entity reference field value. I will work on this tomorrow anyway if nobody has fixed this before it.

reroll

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceXSSTest.php
@@ -0,0 +1,79 @@
+      'field_entity_reference_test' => [

the "field_" bit here was probably breaking things?

The test only fail suggests either something changed in HEAD, the reroll is bad or behavior before and after the patch wasn't the same after all?

1. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceXSSTest.php
   @@ -31,57 +31,37 @@
   +   * Ensures that XSS is not possible through entity reference select.
   

   Also tests the entity view display

2. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceXSSTest.php
   @@ -31,57 +31,37 @@
   -    // Make sure the <em> tags in the title are filtered out.
   -    $this->assertNoRaw($node['title']);
   -    // Make sure the title is not double escaped.
   -    $this->assertNoRaw(Html::escape($node['title']));
   

   We may want to either put these back in or change the title, the title with stripped tags is merely "I am kitten" - which may or may not include the escaped or unescaped <em> tags.

clarifying 2 bits

Thanks for adding the documentation for the few confusing parts. I think I'm allowed to RTBC this because I was only working on a small part of the issue.

Yikes, in Drupal 7 the form API takes care of filtering these automatically, at least I'm pretty sure it does (see theme_form_element_label(), $title = filter_xss_admin($element ['#title'])). Does this issue mean that security feature went missing in Drupal 8? Could indicate some missing test coverage in Drupal 7 if so...

For radio and checkbox labels, Drupal 7 allows HTML by default since filter_xss_admin() is used. I can see changing this in Drupal 8 since the auto-escaping is just a fallback behavior (only used if the string wasn't marked safe already) but does that mean this issue should get a change notice?

Both the points in #40 seem worth addressing here.

+++ b/core/lib/Drupal/Core/Entity/EntityAutocompleteMatcher.php
@@ -72,6 +72,9 @@ public function getMatches($target_type, $selection_handler, $selection_settings
+          // We escape the label as it has not yet been escaped and will be
+          // printed into a JsonResponse.
+          $label = Html::escape($label);

Also, while this is fine, I do worry about authors of JSON consumers being confused about when their JSON is vulnerable to XSS and when it isn't. (Compare with #2503963: XSS in Quick Edit: entity title is not safely encoded. @alexpott and I have discussed this previously, but I'm not sure if there's a reference on d.o.)

Created follow-up for the Json issue: #2565527: Ensure that all different output methods can output safe markup

Re #40 this is about #options so I'm not sure how theme_form_element_label() relates? I must be missing something. Afaics Drupal 7 check_plain()'d everything for select lists in form_select_options() and left checkboxes and radios up to the caller.

That's what I believed for a long time too, but it's not the case. Each element in #options gets converted to an individual checkbox/radio label in form_process_radios() and form_process_checkboxes(), then escaped through filter_xss_admin() via the code I mentioned above. This is actually a documented security feature in Drupal 7 (see https://www.drupal.org/node/28984).

Looking right now, I can't seem to find any tests for this in Drupal 7 (although it's hard to search for the absence of something) so it's possible it did go missing entirely in Drupal 8. If checkboxes and radios aren't being sanitized at all now in Drupal 8 (??) I think this issue should be critical - although it is only a security hardening, a security hardening that was present in Drupal 7 and gone in Drupal 8 seems like a bad idea to release with.

Regarding the test failures, what happens is that you have a render array looking like:

$element = [
  '#title' => [
    '#markup' => [
      '#markup' => 'The title',
    ]
 }
]

so I guess #markup needs render array support ideally? Alternative everytime we do something like

    $variables['legend']['title'] = ['#markup' => $element['#title']];

we would first have to check whether #title is already an array?

1. +++ b/core/lib/Drupal/Core/Entity/EntityAutocompleteMatcher.php
   @@ -72,6 +72,9 @@ public function getMatches($target_type, $selection_handler, $selection_settings
   +          // We escape the label as it has not yet been escaped and will be
   +          // printed into a JsonResponse.
   +          $label = Html::escape($label);
              $key = "$label ($entity_id)";
   

   IMHO its the wrong level to esacpe in HTML and not on the presentation layer, which is javascript, but well, I think its too late to fight that battle ...

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
   @@ -48,8 +48,9 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   +    // We decode HTML entities so as to get characters like "©" instead of
   +    // "©" and we rely on Twig to escape HTML.
   +    $label = Html::decodeEntities($label);
   

   Its not clear for me the label is encoded in the first place ...

3. +++ b/core/modules/entity_reference/src/Tests/EntityReferenceXSSTest.php
   @@ -0,0 +1,71 @@
   +    \Drupal::entityManager()
   +      ->getStorage('entity_form_display')
   +      ->load('node.article.default')
   +      ->setComponent('entity_reference_test', ['type' => 'options_select'])
   +      ->save();
   +    \Drupal::entityManager()
   +      ->getStorage('entity_view_display')
   +      ->load('node.article.default')
   +      ->setComponent('entity_reference_test', ['type' => 'entity_reference_label'])
   +      ->save();
   +
   

   Seriously in tests I kinda think its better to Use EntityFormDisplay::load() | EntityViewDisplay::load() does someone have some opinion about it?

4. +++ b/core/modules/filter/src/FilterFormatFormBase.php
   @@ -79,7 +79,7 @@ public function form(array $form, FormStateInterface $form_state) {
   -      '#options' => array_map('\Drupal\Component\Utility\SafeMarkup::checkPlain', user_role_names()),
   +      '#options' => user_role_names(),
   

   This is soooo much better DX

Thanks for making that change. This looks good for me now!

Does someone mind explaining in the issue summary why auto escaping wasn't applied as in earlier patches?
I think it is totally fine to limit the issue as much as possible, but it would be nice to have some information why this was done.

+++ b/core/modules/entity_reference/src/ConfigurableEntityReferenceItem.php
@@ -80,7 +80,9 @@ public function getSettableOptions(AccountInterface $account = NULL) {
+      // which is only supprted by select elements and auto-escaped.

Nit fixable on commit: supprted.

I think it makes sense to do #2568647: Make #title in form elements autoescape instead of auto XSS admin filter separately.

Yeah it makes sense.

Committed/pushed to 8.0.x, thanks!