Issue #2567159 by joelpittet, effulgentsia, stefan.r, xjm, dawehner: Fix...

Issue #2567159 by joelpittet, effulgentsia, stefan.r, xjm, dawehner: Fix improper usage of t() in ViewsBlock

core/modules/block/src/Controller/BlockLibraryController.php

@@ -104,9 +104,11 @@ public function listBlocks(Request $request, $theme) {
     foreach ($definitions as $plugin_id => $plugin_definition) {
       $row = [];
       $row['title']['data'] = [
-        '#markup' => $plugin_definition['admin_label'],
-        '#prefix' => '<div class="block-filter-text-source">',
-        '#suffix' => '</div>',
+        '#type' => 'inline_template',
+        '#template' => '<div class="block-filter-text-source">{{ label }}</div>',
+        '#context' => [
+          'label' => $plugin_definition['admin_label'],
+        ],
       ];
       $row['category']['data'] = $plugin_definition['category'];
       $links['add'] = [

core/modules/block/src/Tests/BlockXssTest.php

@@ -26,12 +26,23 @@ class BlockXssTest extends WebTestBase {
    *
    * @var array
    */
-  public static $modules = ['block', 'block_content', 'block_test', 'menu_ui', 'views'];
+  public static $modules = ['block', 'block_content', 'menu_ui', 'views'];
 
   /**
-   * Test XSS in title.
+   * Tests that nothing is escaped other than the blocks explicitly tested.
+   */
+  public function testNoUnexpectedEscaping() {
+    $this->drupalLogin($this->drupalCreateUser(['administer blocks', 'access administration pages']));
+    $this->drupalGet(Url::fromRoute('block.admin_display'));
+    $this->clickLinkPartialName('Place block');
+    $this->assertNoEscaped('<');
+  }
+
+  /**
+   * Tests XSS in title.
    */
   public function testXssInTitle() {
+    $this->container->get('module_installer')->install(['block_test']);
     $this->drupalPlaceBlock('test_xss_title', ['label' => '<script>alert("XSS label");</script>']);
 
     \Drupal::state()->set('block_test.content', $this->randomMachineName());
@@ -48,6 +59,7 @@ public function testXssInTitle() {
    * Tests XSS in category.
    */
   public function testXssInCategory() {
+    $this->container->get('module_installer')->install(['block_test']);
     $this->drupalPlaceBlock('test_xss_title');
     $this->drupalLogin($this->drupalCreateUser(['administer blocks', 'access administration pages']));
     $this->drupalGet(Url::fromRoute('block.admin_display'));
@@ -64,24 +76,60 @@ public function testBlockXss() {
     $this->doViewTest();
     $this->doMenuTest();
     $this->doBlockContentTest();
+
+    $this->drupalGet(Url::fromRoute('block.admin_display'));
+    $this->clickLinkPartialName('Place block');
+    $this->assertNoRaw('&amp;lt;', 'The page does not have double escaped HTML tags.');
   }
 
   /**
    * Tests XSS coming from View block labels.
    */
   protected function doViewTest() {
+    // Create a View without a custom label for its block Display. The
+    // admin_label of the block then becomes just the View's label.
     $view = View::create([
       'id' => $this->randomMachineName(),
-      'label' => '<script>alert("view");</script>',
+      'label' => '<script>alert("view1");</script>',
     ]);
     $view->addDisplay('block');
     $view->save();
 
+    // Create a View with a custom label for its block Display. The
+    // admin_label of the block then becomes the View's label combined with
+    // the Display's label.
+    $view = View::create([
+      'id' => $this->randomMachineName(),
+      'label' => '<script>alert("view2");</script>',
+    ]);
+    $view->addDisplay('block', 'Fish & chips');
+    $view->save();
+
     $this->drupalGet(Url::fromRoute('block.admin_display'));
     $this->clickLinkPartialName('Place block');
-    // The block admin label is automatically XSS admin filtered.
-    $this->assertRaw('alert("view");');
-    $this->assertNoRaw('<script>alert("view");</script>');
+
+    // \Drupal\views\Plugin\Derivative\ViewsBlock::getDerivativeDefinitions()
+    // has a different code path for an admin label based only on the View
+    // label versus one based on both the View label and the Display label.
+    // Ensure that this test is covering both code paths by asserting the
+    // absence of a ":" for the first View and the presence of a ":" for the
+    // second one. Note that the second assertion is redundant with the one
+    // further down which also checks for the Display label, but is included
+    // here for clarity.
+    $this->assertNoEscaped('<script>alert("view1");</script>:');
+    $this->assertEscaped('<script>alert("view2");</script>:');
+
+    // Assert that the blocks have their admin labels escaped and
+    // don't appear anywhere unescaped.
+    $this->assertEscaped('<script>alert("view1");</script>');
+    $this->assertNoRaw('<script>alert("view1");</script>');
+    $this->assertEscaped('<script>alert("view2");</script>: Fish & chips');
+    $this->assertNoRaw('<script>alert("view2");</script>');
+    $this->assertNoRaw('Fish & chips');
+
+    // Assert the Display label doesn't appear anywhere double escaped.
+    $this->assertNoRaw('Fish & chips');
+    $this->assertNoRaw('Fish &amp;amp; chips');
   }
 
   /**
@@ -95,8 +143,8 @@ protected function doMenuTest() {
 
     $this->drupalGet(Url::fromRoute('block.admin_display'));
     $this->clickLinkPartialName('Place block');
-    // The block admin label is automatically XSS admin filtered.
-    $this->assertRaw('alert("menu");');
+
+    $this->assertEscaped('<script>alert("menu");</script>');
     $this->assertNoRaw('<script>alert("menu");</script>');
   }
 
@@ -116,8 +164,8 @@ protected function doBlockContentTest() {
 
     $this->drupalGet(Url::fromRoute('block.admin_display'));
     $this->clickLinkPartialName('Place block');
-    // The block admin label is automatically XSS admin filtered.
-    $this->assertRaw('alert("block_content");');
+
+    $this->assertEscaped('<script>alert("block_content");</script>');
     $this->assertNoRaw('<script>alert("block_content");</script>');
   }
 

core/modules/views/src/Plugin/Derivative/ViewsBlock.php

@@ -9,6 +9,7 @@
 
 use Drupal\Core\Entity\EntityStorageInterface;
 use Drupal\Core\Plugin\Discovery\ContainerDeriverInterface;
+use Drupal\Core\StringTranslation\TranslationWrapper;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
@@ -89,19 +90,24 @@ public function getDerivativeDefinitions($base_plugin_definition) {
         // Add a block plugin definition for each block display.
         if (isset($display) && !empty($display->definition['uses_hook_block'])) {
           $delta = $view->id() . '-' . $display->display['id'];
-          $desc = $display->getOption('block_description');
 
-          if (empty($desc)) {
+          $admin_label = $display->getOption('block_description');
+          if (empty($admin_label)) {
             if ($display->display['display_title'] == $display->definition['title']) {
-              $desc = t('!view', array('!view' => $view->label()));
+              $admin_label = $view->label();
             }
             else {
-              $desc = t('!view: !display', array('!view' => $view->label(), '!display' => $display->display['display_title']));
+              // Allow translators to control the punctuation. Plugin
+              // definitions get cached, so use TranslationWrapper() instead of
+              // t() to avoid double escaping when $admin_label is rendered
+              // during requests that use the cached definition.
+              $admin_label = new TranslationWrapper('@view: @display', ['@view' => $view->label(), '@display' => $display->display['display_title']]);
             }
           }
+
           $this->derivatives[$delta] = array(
             'category' => $display->getOption('block_category'),
-            'admin_label' => $desc,
+            'admin_label' => $admin_label,
             'config_dependencies' => array(
               'config' => array(
                 $view->getConfigDependencyName(),