diff --git a/core/lib/Drupal/Core/Menu/MenuLinkDefault.php b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php index c2241428ce5d5693dd1954e87ecd13be4cf053b5..c17fe79c1237d2fd7556889096ac2a62766a7448 100644 --- a/core/lib/Drupal/Core/Menu/MenuLinkDefault.php +++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php @@ -67,26 +67,14 @@ public static function create(ContainerInterface $container, array $configuratio * {@inheritdoc} */ public function getTitle() { - // Subclasses may pull in the request or specific attributes as parameters. - $options = array(); - if (!empty($this->pluginDefinition['title_context'])) { - $options['context'] = $this->pluginDefinition['title_context']; - } - $args = array(); - if (isset($this->pluginDefinition['title_arguments']) && $title_arguments = $this->pluginDefinition['title_arguments']) { - $args = (array) $title_arguments; - } - return $this->t($this->pluginDefinition['title'], $args, $options); + return (string) $this->pluginDefinition['title']; } /** * {@inheritdoc} */ public function getDescription() { - if ($this->pluginDefinition['description']) { - return $this->t($this->pluginDefinition['description']); - } - return ''; + return (string) $this->pluginDefinition['description']; } /** diff --git a/core/lib/Drupal/Core/Menu/MenuLinkManager.php b/core/lib/Drupal/Core/Menu/MenuLinkManager.php index 5a18a31bf99df0407e158cf390ab8528885ede55..6a4a5108a6d15b86d26108a89154163f87f30892 100644 --- a/core/lib/Drupal/Core/Menu/MenuLinkManager.php +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php @@ -40,15 +40,11 @@ class MenuLinkManager implements MenuLinkManagerInterface { 'route_parameters' => array(), // The external URL if this link has one (required if route_name is empty). 'url' => '', - // The static title for the menu link. You can specify placeholders like on - // any translatable string and the values in title_arguments. + // The static title for the menu link. If this came from a YAML definition + // or other safe source this may be a TranslationWrapper object. 'title' => '', - // The values for the menu link placeholders. - 'title_arguments' => array(), - // A context for the title string. - // @see \Drupal\Core\StringTranslation\TranslationInterface::translate() - 'title_context' => '', - // The description. + // The description. If this came from a YAML definition or other safe source + // this may be be a TranslationWrapper object. 'description' => '', // The plugin ID of the parent link (or NULL for a top-level link). 'parent' => '', @@ -147,8 +143,10 @@ protected function processDefinition(array &$definition, $plugin_id) { */ protected function getDiscovery() { if (!isset($this->discovery)) { - $this->discovery = new YamlDiscovery('links.menu', $this->moduleHandler->getModuleDirectories()); - $this->discovery = new ContainerDerivativeDiscoveryDecorator($this->discovery); + $yaml_discovery = new YamlDiscovery('links.menu', $this->moduleHandler->getModuleDirectories()); + $yaml_discovery->addTranslatableProperty('title', 'title_context'); + $yaml_discovery->addTranslatableProperty('description', 'description_context'); + $this->discovery = new ContainerDerivativeDiscoveryDecorator($yaml_discovery); } return $this->discovery; } diff --git a/core/lib/Drupal/Core/Menu/MenuTreeStorage.php b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php index 3f2d403d6cda855d57da01fc61fbac48d742809d..7a79fc1aa758257442e28d2036f28843a979cb16 100644 --- a/core/lib/Drupal/Core/Menu/MenuTreeStorage.php +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php @@ -94,8 +94,6 @@ class MenuTreeStorage implements MenuTreeStorageInterface { 'route_parameters', 'url', 'title', - 'title_arguments', - 'title_context', 'description', 'parent', 'weight', @@ -366,7 +364,9 @@ protected function preSave(array &$link, array $original) { $fields['route_param_key'] = $fields['route_parameters'] ? UrlHelper::buildQuery($fields['route_parameters']) : ''; foreach ($this->serializedFields() as $name) { - $fields[$name] = serialize($fields[$name]); + if (isset($fields[$name])) { + $fields[$name] = serialize($fields[$name]); + } } $this->setParents($fields, $parent, $original); @@ -618,7 +618,9 @@ protected function updateParentalStatus(array $link) { */ protected function prepareLink(array $link, $intersect = FALSE) { foreach ($this->serializedFields() as $name) { - $link[$name] = unserialize($link[$name]); + if (isset($link[$name])) { + $link[$name] = unserialize($link[$name]); + } } if ($intersect) { $link = array_intersect_key($link, array_flip($this->definitionFields())); @@ -735,7 +737,9 @@ protected function loadFullMultiple(array $ids) { $loaded = $this->safeExecuteSelect($query)->fetchAllAssoc('id', \PDO::FETCH_ASSOC); foreach ($loaded as &$link) { foreach ($this->serializedFields() as $name) { - $link[$name] = unserialize($link[$name]); + if (isset($link[$name])) { + $link[$name] = unserialize($link[$name]); + } } } return $loaded; @@ -926,15 +930,19 @@ protected function loadLinks($menu_name, MenuTreeParameters $parameters) { if (!empty($parameters->conditions)) { // Only allow conditions that are testing definition fields. $parameters->conditions = array_intersect_key($parameters->conditions, array_flip($this->definitionFields())); + $serialized_fields = $this->serializedFields(); foreach ($parameters->conditions as $column => $value) { - if (!is_array($value)) { - $query->condition($column, $value); - } - else { + if (is_array($value)) { $operator = $value[1]; $value = $value[0]; - $query->condition($column, $value, $operator); } + else { + $operator = '='; + } + if (in_array($column, $serialized_fields)) { + $value = serialize($value); + } + $query->condition($column, $value, $operator); } } @@ -1241,30 +1249,18 @@ protected static function schemaDefinition() { 'default' => '', ), 'title' => array( - 'description' => 'The text displayed for the link.', - 'type' => 'varchar', - 'length' => 255, - 'not null' => TRUE, - 'default' => '', - ), - 'title_arguments' => array( - 'description' => 'A serialized array of arguments to be passed to t() (if this plugin uses it).', + 'description' => 'The serialized title for the link. May be a TranslationWrapper.', 'type' => 'blob', 'size' => 'big', 'not null' => FALSE, 'serialize' => TRUE, ), - 'title_context' => array( - 'description' => 'The translation context for the link title.', - 'type' => 'varchar', - 'length' => 255, - 'not null' => TRUE, - 'default' => '', - ), 'description' => array( - 'description' => 'The description of this link - used for admin pages and title attribute.', - 'type' => 'text', + 'description' => 'The serialized description of this link - used for admin pages and title attribute. May be a TranslationWrapper.', + 'type' => 'blob', + 'size' => 'big', 'not null' => FALSE, + 'serialize' => TRUE, ), 'class' => array( 'description' => 'The class for this link plugin.', diff --git a/core/lib/Drupal/Core/Menu/menu.api.php b/core/lib/Drupal/Core/Menu/menu.api.php index 1d8298feb3fe7948e8ab6373cca643f81bf587bf..8550fc515082c1437d3d1c9785082bfe6992cd0f 100644 --- a/core/lib/Drupal/Core/Menu/menu.api.php +++ b/core/lib/Drupal/Core/Menu/menu.api.php @@ -334,8 +334,12 @@ * * The value corresponding to each machine name key is an associative array * that may contain the following key-value pairs: - * - title: (required) The untranslated title of the menu link. - * - description: The untranslated description of the link. + * - title: (required) The title of the menu link. If this should be + * translated, create a \Drupal\Core\StringTranslation\TranslationWrapper + * object. + * - description: The description of the link. If this should be + * translated, create a \Drupal\Core\StringTranslation\TranslationWrapper + * object. * - route_name: (optional) The route name to be used to build the path. * Either the route_name or url element must be provided. * - route_parameters: (optional) The route parameters to build the path. @@ -360,7 +364,16 @@ function hook_menu_links_discovered_alter(&$links) { // Change the weight and title of the user.logout link. $links['user.logout']['weight'] = -10; - $links['user.logout']['title'] = 'Logout'; + $links['user.logout']['title'] = new \Drupal\Core\StringTranslation\TranslationWrapper('Logout'); + // Conditionally add an additional link with a title that's not translated. + if (\Drupal::moduleHandler()->moduleExists('search')) { + $links['menu.api.search'] = array( + 'title' => \Drupal::config('system.site')->get('name'), + 'route_name' => 'menu.api.search', + 'description' => new \Drupal\Core\StringTranslation\TranslationWrapper('View popular search phrases for this site.'), + 'parent' => 'system.admin_reports', + ); + } } /** diff --git a/core/modules/content_translation/content_translation.module b/core/modules/content_translation/content_translation.module index 3f9ace427ec3f63dde1798cb48a1a6807fbbf0c2..da027710f09aeff27e184b3494946c968be6dfa9 100644 --- a/core/modules/content_translation/content_translation.module +++ b/core/modules/content_translation/content_translation.module @@ -13,6 +13,7 @@ use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Language\LanguageInterface; use Drupal\Core\Routing\RouteMatchInterface; +use Drupal\Core\StringTranslation\TranslationWrapper; /** * Implements hook_help(). @@ -251,8 +252,8 @@ function content_translation_views_data_alter(array &$data) { */ function content_translation_menu_links_discovered_alter(array &$links) { // Clarify where translation settings are located. - $links['language.content_settings_page']['title'] = 'Content language and translation'; - $links['language.content_settings_page']['description'] = 'Configure language and translation support for content.'; + $links['language.content_settings_page']['title'] = new TranslationWrapper('Content language and translation'); + $links['language.content_settings_page']['description'] = new TranslationWrapper('Configure language and translation support for content.'); } /** diff --git a/core/modules/dblog/dblog.module b/core/modules/dblog/dblog.module index 01c4d23950b07c8081f742427c7bf50867e1495e..91c6038e6ee839bab8194153be36db9e1127e318 100644 --- a/core/modules/dblog/dblog.module +++ b/core/modules/dblog/dblog.module @@ -11,6 +11,7 @@ use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Routing\RouteMatchInterface; +use Drupal\Core\StringTranslation\TranslationWrapper; /** * Implements hook_help(). @@ -41,9 +42,9 @@ function dblog_help($route_name, RouteMatchInterface $route_match) { function dblog_menu_links_discovered_alter(&$links) { if (\Drupal::moduleHandler()->moduleExists('search')) { $links['dblog.search'] = array( - 'title' => 'Top search phrases', + 'title' => new TranslationWrapper('Top search phrases'), 'route_name' => 'dblog.search', - 'description' => 'View most popular search phrases.', + 'description' => new TranslationWrapper('View most popular search phrases.'), 'parent' => 'system.admin_reports', ); } diff --git a/core/modules/editor/editor.module b/core/modules/editor/editor.module index 7fa3c05e842bff31a018650809fb7b6873950835..470f26938633276fdb8114af2293273ed6c1cc14 100644 --- a/core/modules/editor/editor.module +++ b/core/modules/editor/editor.module @@ -11,7 +11,7 @@ use Drupal\Core\Form\FormStateInterface; use Drupal\Core\Render\Element; use Drupal\Core\Routing\RouteMatchInterface; -use Drupal\Component\Utility\NestedArray; +use Drupal\Core\StringTranslation\TranslationWrapper; use Drupal\Core\Entity\EntityInterface; use Drupal\filter\FilterFormatInterface; use Drupal\filter\Plugin\FilterInterface; @@ -47,8 +47,8 @@ function editor_help($route_name, RouteMatchInterface $route_match) { * of text editors. */ function editor_menu_links_discovered_alter(array &$links) { - $links['filter.admin_overview']['title'] = 'Text formats and editors'; - $links['filter.admin_overview']['description'] = 'Configure how user-contributed content is filtered and formatted, as well as the text editor user interface (WYSIWYGs or toolbars).'; + $links['filter.admin_overview']['title'] = new TranslationWrapper('Text formats and editors'); + $links['filter.admin_overview']['description'] = new TranslationWrapper('Configure how user-contributed content is filtered and formatted, as well as the text editor user interface (WYSIWYGs or toolbars).'); } /** diff --git a/core/modules/menu_link_content/src/Tests/MenuLinkContentDeriverTest.php b/core/modules/menu_link_content/src/Tests/MenuLinkContentDeriverTest.php index c7a1ae1fc2b81ac9e439c00ff7c1bf7576a5a61f..b7238f8cbb5b33ccb035f49e9a94750c1c07b533 100644 --- a/core/modules/menu_link_content/src/Tests/MenuLinkContentDeriverTest.php +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentDeriverTest.php @@ -7,7 +7,9 @@ namespace Drupal\menu_link_content\Tests; +use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Menu\MenuTreeParameters; +use Drupal\Core\StringTranslation\TranslationWrapper; use Drupal\menu_link_content\Entity\MenuLinkContent; use Drupal\simpletest\KernelTestBase; use Symfony\Component\Routing\Route; @@ -45,7 +47,7 @@ public function testRediscover() { // Set up a custom menu link pointing to a specific path. MenuLinkContent::create([ - 'title' => 'Example', + 'title' => '<script>alert("Welcome to the discovered jungle!")</script>', 'link' => [['uri' => 'internal:/example-path']], 'menu_name' => 'tools', ])->save(); @@ -67,6 +69,10 @@ public function testRediscover() { /** @var \Drupal\Core\Menu\MenuLinkTreeElement $tree_element */ $tree_element = reset($menu_tree); $this->assertEqual('route_name_2', $tree_element->link->getRouteName()); + $title = $tree_element->link->getTitle(); + $this->assertFalse($title instanceof TranslationWrapper); + $this->assertIdentical('<script>alert("Welcome to the discovered jungle!")</script>', $title); + $this->assertFalse(SafeMarkup::isSafe($title)); } } diff --git a/core/modules/system/src/Tests/Menu/MenuLinkSecurityTest.php b/core/modules/system/src/Tests/Menu/MenuLinkSecurityTest.php new file mode 100644 index 0000000000000000000000000000000000000000..245b0c81934651288f7efa542f4a1a464cb9ee13 --- /dev/null +++ b/core/modules/system/src/Tests/Menu/MenuLinkSecurityTest.php @@ -0,0 +1,45 @@ +<?php + +/** + * @file + * Contains \Drupal\system\Tests\Menu\MenuLinkSecurityTest. + */ + +namespace Drupal\system\Tests\Menu; + +use Drupal\menu_link_content\Entity\MenuLinkContent; +use Drupal\simpletest\WebTestBase; + +/** + * Ensures that menu links don't cause XSS issues. + * + * @group Menu + */ +class MenuLinkSecurityTest extends WebTestBase { + + /** + * {@inheritdoc} + */ + public static $modules = ['menu_link_content', 'block', 'menu_test']; + + /** + * Ensures that a menu link does not cause an XSS issue. + */ + public function testMenuLink() { + $menu_link_content = MenuLinkContent::create([ + 'title' => '<script>alert("Wild animals")</script>', + 'menu_name' => 'tools', + 'link' => ['uri' => 'route:<front>'], + ]); + $menu_link_content->save(); + + $this->drupalPlaceBlock('system_menu_block:tools'); + + $this->drupalGet('<front>'); + $this->assertNoRaw('<script>alert("Wild animals")</script>'); + $this->assertNoRaw('<script>alert("Even more wild animals")</script>'); + $this->assertEscaped('<script>alert("Wild animals")</script>'); + $this->assertEscaped('<script>alert("Even more wild animals")</script>'); + } + +} diff --git a/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php index 1df40504929aa8461db65c3bf14078984b6b6e69..57f1f2a0ff5f899d96438512a7420cdd07582256 100644 --- a/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php +++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php @@ -354,7 +354,6 @@ protected function addMenuLink($id, $parent = '', $route_name = 'test', $route_p 'menu_name' => $menu_name, 'route_name' => $route_name, 'route_parameters' => $route_parameters, - 'title_arguments' => array(), 'title' => 'test', 'parent' => $parent, 'options' => array(), diff --git a/core/modules/system/src/Tests/Update/MenuTreeSerializationTitleTest.php b/core/modules/system/src/Tests/Update/MenuTreeSerializationTitleTest.php new file mode 100644 index 0000000000000000000000000000000000000000..2439b1c39255692e3f59c871f9e84c613d65dfdc --- /dev/null +++ b/core/modules/system/src/Tests/Update/MenuTreeSerializationTitleTest.php @@ -0,0 +1,68 @@ +<?php + +/** + * @file + * Contains \Drupal\system\Tests\Update\MenuTreeSerializationTitleTest. + */ + +namespace Drupal\system\Tests\Update; + +use Drupal\Core\StringTranslation\TranslationWrapper; + +/** + * Tests system_update_8001(). + * + * @group Update + */ +class MenuTreeSerializationTitleTest extends UpdatePathTestBase { + + /** + * {@inheritdoc} + */ + public function setUp() { + $this->databaseDumpFiles = [ + __DIR__ . '/../../../tests/fixtures/update/drupal-8.bare.standard.php.gz', + ]; + parent::setUp(); + } + + /** + * Ensures that the system_update_8001() runs as expected. + */ + public function testUpdate() { + $this->runUpdates(); + + // Ensure that some fields got dropped. + $database = \Drupal::database(); + $schema = $database->schema(); + + if (!$schema->tableExists('menu_tree')) { + return; + } + + $this->assertFalse($schema->fieldExists('menu_tree', 'title_arguments')); + $this->assertFalse($schema->fieldExists('menu_tree', 'title_contexts')); + + // Ensure that all titles and description values can be unserialized. + $select = $database->select('menu_tree'); + $result = $select->fields('menu_tree', ['id', 'title', 'description']) + ->execute() + ->fetchAllAssoc('id'); + + // The test coverage relies upon the fact that unserialize() would emit a + // warning if the value is not a valid serialized value. + foreach ($result as $link) { + $title = unserialize($link->title); + $description = unserialize($link->description); + // Verify that all the links from system module have a been updated with + // a TranslationWrapper as title and description due to the rebuild. + if (strpos($link->id, 'system.') === 0) { + $this->assertTrue($title instanceof TranslationWrapper, get_class($title)); + if ($description) { + $this->assertTrue($description instanceof TranslationWrapper, get_class($description)); + } + } + } + } + +} diff --git a/core/modules/system/src/Tests/Update/UpdatePathTestBase.php b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php index da0b3be42703d394778a7893ce826a6e41b5084d..a9e0ad2d245acc6aa6cf2dedb477c398e215b3ae 100644 --- a/core/modules/system/src/Tests/Update/UpdatePathTestBase.php +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php @@ -199,4 +199,21 @@ protected function runUpdates() { $this->clickLink(t('Apply pending updates')); } + /** + * {@inheritdoc} + */ + protected function rebuildAll() { + parent::rebuildAll(); + + // Remove the notices we get due to the menu link rebuild prior to running + // the system updates for the schema change. + foreach ($this->assertions as $key => $assertion) { + if ($assertion['message_group'] == 'Notice' && basename($assertion['file']) == 'MenuTreeStorage.php' && strpos($assertion['message'], 'unserialize(): Error at offset 0') !== FALSE) { + unset($this->assertions[$key]); + $this->deleteAssert($assertion['message_id']); + $this->results['#exception']--; + } + } + } + } diff --git a/core/modules/system/system.install b/core/modules/system/system.install index c1b6fe5967c2174353e27284692f5a71e1c79238..042c8f29c96f3e70ffe2ceaca7239edcb510db47 100644 --- a/core/modules/system/system.install +++ b/core/modules/system/system.install @@ -1140,3 +1140,66 @@ function system_schema() { return $schema; } + +/** + * Change two fields on the default menu link storage to be serialized data. + */ +function system_update_8001(&$sandbox = NULL) { + $database = \Drupal::database(); + $schema = $database->schema(); + if ($schema->tableExists('menu_tree')) { + + if (!isset($sandbox['current'])) { + $spec = array( + 'description' => 'The title for the link. May be a serialized TranslationWrapper.', + 'type' => 'blob', + 'size' => 'big', + 'not null' => FALSE, + 'serialize' => TRUE, + ); + $schema->changeField('menu_tree', 'title', 'title', $spec); + $spec = array( + 'description' => 'The description of this link - used for admin pages and title attribute.', + 'type' => 'blob', + 'size' => 'big', + 'not null' => FALSE, + 'serialize' => TRUE, + ); + $schema->changeField('menu_tree', 'description', 'description', $spec); + + $sandbox['current'] = 0; + $sandbox['max'] = $database->query('SELECT COUNT(mlid) FROM {menu_tree}') + ->fetchField(); + } + + $menu_links = $database->queryRange('SELECT mlid, title, description FROM {menu_tree} ORDER BY mlid ASC', $sandbox['current'], $sandbox['current'] + 50) + ->fetchAllAssoc('mlid'); + + foreach ($menu_links as $menu_link) { + $menu_link = (array) $menu_link; + // Convert title and description to serialized strings. + $menu_link['title'] = serialize($menu_link['title']); + $menu_link['description'] = serialize($menu_link['description']); + + $database->update('menu_tree') + ->fields($menu_link) + ->condition('mlid', $menu_link['mlid']) + ->execute(); + + $sandbox['current']++; + } + + $sandbox['#finished'] = empty($sandbox['max']) ? 1 : ($sandbox['current'] / $sandbox['max']); + + if ($sandbox['#finished'] >= 1) { + // Drop unnecessary fields from {menu_tree}. + $schema->dropField('menu_tree', 'title_arguments'); + $schema->dropField('menu_tree', 'title_context'); + } + return t('Menu links converted'); + } + else { + return t('Menu link conversion skipped, because the {menu_tree} table did not exist yet.'); + } +} + diff --git a/core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml b/core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml index 914fbd5e1fd315db11c91d6035e42654fa41c259..613bc9211776ca4956be4c00eb4446cb0aa7ebec 100644 --- a/core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml +++ b/core/modules/system/tests/modules/menu_test/menu_test.links.menu.yml @@ -79,3 +79,7 @@ menu_test.child: title: 'Test menu_name child' route_name: menu_test.menu_name_test parent: menu_test.parent + +menu_test.unsafe: + route_name: menu_test.menu_name_test + deriver: '\Drupal\menu_test\Plugin\Derivative\MenuLinkTestWithUnsafeTitle' diff --git a/core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/MenuLinkTestWithUnsafeTitle.php b/core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/MenuLinkTestWithUnsafeTitle.php new file mode 100644 index 0000000000000000000000000000000000000000..8ddd86ac5dcef4305f4810ee3d04857d11f3df17 --- /dev/null +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/MenuLinkTestWithUnsafeTitle.php @@ -0,0 +1,29 @@ +<?php + +/** + * @file + * Contains \Drupal\menu_test\Plugin\Derivative\MenuLinkTestWithUnsafeTitle. + */ + +namespace Drupal\menu_test\Plugin\Derivative; + +use Drupal\Component\Plugin\Derivative\DeriverBase; + +/** + * Test derivative with an unsafe string. + */ +class MenuLinkTestWithUnsafeTitle extends DeriverBase { + + /** + * {@inheritdoc} + */ + public function getDerivativeDefinitions($base_plugin_definition) { + $this->derivatives['unsafe'] = [ + 'title' => '<script>alert("Even more wild animals")</script>', + 'menu_name' => 'tools', + ] + $base_plugin_definition; + + return $this->derivatives; + } + +}