Issue #2571935 by izus, dawehner, stefan.r, alexpott, Wim Leers, joelpittet,...

Issue #2571935 by izus, dawehner, stefan.r, alexpott, Wim Leers, joelpittet, lauriii, xjm, joelpittet: Fix use of !placeholder for imploding in views.views.inc

core/modules/views/src/Tests/FieldApiDataTest.php

@@ -7,6 +7,8 @@
 
 namespace Drupal\views\Tests;
 
+use Drupal\Component\Utility\SafeStringInterface;
+use Drupal\field\Entity\FieldConfig;
 use Drupal\field\Tests\Views\FieldTestBase;
 
 /**
@@ -26,9 +28,19 @@ protected function setUp() {
       'field_name' => $field_names[0],
       'entity_type' => 'node',
       'bundle' => 'page',
+      'label' => 'The giraffe" label'
     );
     entity_create('field_config', $field)->save();
 
+    // Attach the same field to a different bundle with a different label.
+    $this->drupalCreateContentType(['type' => 'article']);
+    FieldConfig::create([
+      'field_name' => $field_names[0],
+      'entity_type' => 'node',
+      'bundle' => 'article',
+      'label' => 'The giraffe2" label'
+    ])->save();
+
     // Now create some example nodes/users for the view result.
     for ($i = 0; $i < 5; $i++) {
       $edit = array(
@@ -86,6 +98,12 @@ function testViewsData() {
     $this->assertTrue($data[$current_table][$field_storage->getName()]['field']['click sortable'], 'String field is click sortable.');
     // Click sort should only be on the primary field.
     $this->assertTrue(empty($data[$revision_table][$field_storage->getName()]['field']['click sortable']), 'Non-primary fields are not click sortable');
+
+    $this->assertTrue($data[$current_table][$field_storage->getName()]['help'] instanceof SafeStringInterface);
+    $this->assertEqual($data[$current_table][$field_storage->getName()]['help'], 'Appears in: page, article. Also known as: Content: The giraffe2&quot; label');
+
+    $this->assertTrue($data[$current_table][$field_storage->getName() . '_value']['help'] instanceof SafeStringInterface);
+    $this->assertEqual($data[$current_table][$field_storage->getName() . '_value']['help'], 'Appears in: page, article. Also known as: Content: The giraffe&quot; label (field_name_0)');
   }
 
 }

core/modules/views/views.views.inc

@@ -8,6 +8,7 @@
 use Drupal\Component\Utility\NestedArray;
 use Drupal\Core\Entity\EntityStorageInterface;
 use Drupal\Core\Entity\Sql\SqlContentEntityStorage;
+use Drupal\Core\Render\SafeString;
 use Drupal\field\FieldConfigInterface;
 use Drupal\field\FieldStorageConfigInterface;
 use Drupal\system\ActionConfigEntityInterface;
@@ -449,7 +450,16 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
     }
     if ($aliases) {
       $data[$table_alias][$field_alias]['aliases'] = $aliases;
-      $data[$table_alias][$field_alias]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));
+      // The $also_known variable contains markup that is HTML escaped and that
+      // loses safeness when imploded. The help text is used in #description
+      // and therefore XSS admin filtered by default. Escaped HTML is not
+      // altered by XSS filtering, therefore it is safe to just concatenate the
+      // strings. Afterwards we mark the entire string as safe, so it won't be
+      // escaped, no matter where it is used.
+      // Considering the dual use of this help data (both as metadata and as
+      // help text), other patterns such as use of #markup would not be correct
+      // here.
+      $data[$table_alias][$field_alias]['help'] = SafeString::create($data[$table_alias][$field_alias]['help'] . ' ' . t('Also known as:') . ' ' . implode(', ', $also_known));
     }
 
     $keys = array_keys($field_columns);
@@ -550,7 +560,16 @@ function views_field_default_views_data(FieldStorageConfigInterface $field_stora
       }
       if ($aliases) {
         $data[$table_alias][$column_real_name]['aliases'] = $aliases;
-        $data[$table_alias][$column_real_name]['help'] .= ' ' . t('Also known as: !also.', array('!also' => implode(', ', $also_known)));
+        // The $also_known variable contains markup that is HTML escaped and
+        // that loses safeness when imploded. The help text is used in
+        // #description and therefore XSS admin filtered by default. Escaped
+        // HTML is not altered by XSS filtering, therefore it is safe to just
+        // concatenate the strings. Afterwards we mark the entire string as
+        // safe, so it won't be escaped, no matter where it is used.
+        // Considering the dual use of this help data (both as metadata and as
+        // help text), other patterns such as use of #markup would not be
+        // correct here.
+        $data[$table_alias][$column_real_name]['help'] = SafeString::create($data[$table_alias][$column_real_name]['help'] . ' ' . t('Also known as:') . ' ' . implode(', ', $also_known));
       }
 
       $data[$table_alias][$column_real_name]['argument'] = array(

core/modules/views_ui/src/Tests/HandlerTest.php

@@ -8,6 +8,8 @@
 namespace Drupal\views_ui\Tests;
 
 use Drupal\Component\Utility\SafeMarkup;
+use Drupal\field\Entity\FieldConfig;
+use Drupal\field\Entity\FieldStorageConfig;
 use Drupal\views\ViewExecutable;
 
 /**
@@ -23,7 +25,7 @@ class HandlerTest extends UITestBase {
    *
    * @var array
    */
-  public static $testViews = array('test_view_empty', 'test_view_broken');
+  public static $testViews = array('test_view_empty', 'test_view_broken', 'node');
 
   /**
    * Overrides \Drupal\views\Tests\ViewTestBase::schemaDefinition().
@@ -137,6 +139,40 @@ public function testUICRUD() {
     $this->assertTrue(isset($display['display_options'][$type_info['plural']][$id]), 'Ensure the field was added to the view itself.');
   }
 
+  /**
+   * Tests escaping of field labels in help text.
+   */
+  public function testHandlerHelpEscaping() {
+    // Setup a field with two instances using a different label.
+    // Ensure that the label is escaped properly.
+
+    $this->drupalCreateContentType(['type' => 'article']);
+    $this->drupalCreateContentType(['type' => 'page']);
+
+    FieldStorageConfig::create([
+      'field_name' => 'field_test',
+      'entity_type' => 'node',
+      'type' => 'string',
+    ])->save();
+
+    FieldConfig::create([
+      'field_name' => 'field_test',
+      'entity_type' => 'node',
+      'bundle' => 'page',
+      'label' => 'The giraffe" label'
+    ])->save();
+
+    FieldConfig::create([
+      'field_name' => 'field_test',
+      'entity_type' => 'node',
+      'bundle' => 'article',
+      'label' => 'The <em>giraffe"</em> label <script>alert("the return of the xss")</script>'
+    ])->save();
+
+    $this->drupalGet('admin/structure/views/nojs/add-handler/content/default/field');
+    $this->assertEscaped('Appears in: page, article. Also known as: Content: The <em>giraffe"</em> label <script>alert("the return of the xss")</script>');
+  }
+
   /**
    * Tests broken handlers.
    */