Commit b80a4b62 authored by alexpott's avatar alexpott

Issue #2532476 by dawehner, pwolanin, pfrenssen, Gábor Hojtsy, effulgentsia:...

Issue #2532476 by dawehner, pwolanin, pfrenssen, Gábor Hojtsy, effulgentsia: Menu links should use a TranslationWrapper to encapsulate safe translatable strings from YAML files
parent 3c19df59
......@@ -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'];
}
/**
......
......@@ -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;
}
......
......@@ -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.',
......
......@@ -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',
);
}
}
/**
......
......@@ -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.');
}
/**
......
......@@ -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',
);
}
......
......@@ -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).');
}
/**
......
......@@ -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));
}
}
<?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>');
}
}
......@@ -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(),
......
<?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));
}
}
}
}
}
......@@ -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']--;
}
}
}
}
......@@ -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.');
}
}
......@@ -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'
<?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;
}
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment