Skip to content

D
drupal
Commit 0cd35ba0 authored by catch's avatar catch
Browse files
Options

Issue #2571673 by plach, dawehner: Convert Views t() usage where it is used as an attribute value

parent 7d37585e
Hide whitespace changes
Inline Side-by-side
Showing with 140 additions and 20 deletions
core/lib/Drupal/Core/Form/FormBuilder.php
View file @ 0cd35ba0
......@@ -1316,7 +1316,11 @@ protected function buttonWasClicked($element, FormStateInterface &$form_state) {
// long as $form['#name'] puts the value at the top level of the tree of
// \Drupal::request()->request data.
$input = $form_state->getUserInput();
if (isset($input[$element['#name']]) && $input[$element['#name']] == $element['#value']) {
// The input value attribute is treated as CDATA by browsers. This means
// that they replace character entities with characters. Therefore, we need
// to decode the value in $element['#value']. For more details see
// http://www.w3.org/TR/html401/types.html#type-cdata.
if (isset($input[$element['#name']]) && $input[$element['#name']] == Html::decodeEntities($element['#value'])) {
return TRUE;
}
// When image buttons are clicked, browsers do NOT pass the form element
......
core/lib/Drupal/Core/Template/Attribute.php
View file @ 0cd35ba0
......@@ -7,6 +7,8 @@
namespace Drupal\Core\Template;
use Drupal\Component\Utility\PlainTextOutput;
use Drupal\Component\Utility\SafeMarkup;
use Drupal\Component\Utility\SafeStringInterface;
/**
......@@ -52,7 +54,18 @@
* // Produces <a href="alert(&quot;xss&quot;);">
* @endcode
*
* The attribute values are considered plain text and are treated as such. If a
* safe HTML string is detected, it is converted to plain text with
* PlainTextOutput::renderFromHtml() before being escaped. For example:
* @code
* $value = t('Highlight the @tag tag', ['@tag' => '<em>']);
* $attributes = new Attribute(['value' => $value]);
* echo '<input' . $attributes . '>';
* // Produces <input value="Highlight the &lt;em&gt; tag">
* @endcode
*
* @see \Drupal\Component\Utility\Html::escape()
* @see \Drupal\Component\Utility\PlainTextOutput::renderFromHtml()
* @see \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols()
*/
class Attribute implements \ArrayAccess, \IteratorAggregate, SafeStringInterface {
......@@ -125,7 +138,13 @@ protected function createAttributeValue($name, $value) {
$value = new AttributeBoolean($name, $value);
}
// As a development aid, we allow the value to be a safe string object.
elseif (!is_object($value) || $value instanceof SafeStringInterface) {
elseif (SafeMarkup::isSafe($value)) {
// Attributes are not supposed to display HTML markup, so we just convert
// the value to plain text.
$value = PlainTextOutput::renderFromHtml($value);
$value = new AttributeString($name, $value);
}
elseif (!is_object($value)) {
$value = new AttributeString($name, $value);
}
return $value;
......
core/modules/views/src/Views.php
View file @ 0cd35ba0
......@@ -155,7 +155,7 @@ public static function fetchPluginNames($type, $key = NULL, array $base = array(
}
if (empty($plugin['no_ui']) && (empty($base) || empty($plugin['base']) || array_intersect($base, $plugin['base']))) {
$plugins[$id] = (string) $plugin['title'];
$plugins[$id] = $plugin['title'];
}
}
......
core/modules/views_ui/src/Tests/DisplayTest.php
View file @ 0cd35ba0
......@@ -9,9 +9,9 @@
use Drupal\Component\Serialization\Json;
use Drupal\Component\Utility\SafeMarkup;
use Drupal\views\Views;
use Drupal\Core\Template\Attribute;
use Drupal\views\Entity\View;
use Drupal\views\Views;
/**
* Tests the display UI.
......@@ -214,6 +214,35 @@ public function testViewStatus() {
$this->assertTrue($elements, 'The disabled class was found on the form wrapper.');
}
/**
* Ensures that no XSS is possible for buttons.
*/
public function testDisplayTitleInButtonsXss() {
$xss_markup = '"><script>alert(123)</script>';
$view = $this->randomView();
$view = View::load($view['id']);
\Drupal::configFactory()->getEditable('views.settings')->set('ui.show.master_display', TRUE)->save();
foreach ([$xss_markup, '&quot;><script>alert(123)</script>'] as $input) {
$display =& $view->getDisplay('page_1');
$display['display_title'] = $input;
$view->save();
$this->drupalGet("admin/structure/views/view/{$view->id()}");
$escaped = views_ui_truncate($input, 25);
$this->assertEscaped($escaped);
$this->assertNoRaw($xss_markup);
$this->drupalGet("admin/structure/views/view/{$view->id()}/edit/page_1");
$this->assertEscaped("View $escaped");
$this->assertNoRaw("View $xss_markup");
$this->assertEscaped("Duplicate $escaped");
$this->assertNoRaw("Duplicate $xss_markup");
$this->assertEscaped("Delete $escaped");
$this->assertNoRaw("Delete $xss_markup");
}
}
/**
* Tests the action links on the edit display UI.
*/
......@@ -244,4 +273,5 @@ public function testActionLinks() {
$this->assertEscaped($display_title);
$this->assertNoRaw($display_title);
}
}
core/modules/views_ui/src/ViewEditForm.php
View file @ 0cd35ba0
......@@ -8,23 +8,20 @@
namespace Drupal\views_ui;
use Drupal\Component\Utility\Html;
use Drupal\Component\Utility\UrlHelper;
use Drupal\Component\Utility\SafeMarkup;
use Drupal\Component\Utility\Xss;
use Drupal\Core\Ajax\AjaxResponse;
use Drupal\Core\Ajax\HtmlCommand;
use Drupal\Core\Ajax\ReplaceCommand;
use Drupal\Core\Datetime\DateFormatter;
use Drupal\Component\Utility\NestedArray;
use Drupal\Component\Utility\SafeMarkup;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Render\Element;
use Drupal\Core\Render\ElementInfoManagerInterface;
use Drupal\Core\Url;
use Drupal\user\SharedTempStoreFactory;
use Drupal\views\Views;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\RequestStack;
/**
* Form controller for the Views edit form.
......@@ -402,7 +399,7 @@ public function getDisplayDetails($view, $display) {
if (!$is_enabled) {
$build['top']['actions']['enable'] = array(
'#type' => 'submit',
'#value' => $this->t('Enable !display_title', array('!display_title' => $display_title)),
'#value' => $this->t('Enable @display_title', ['@display_title' => $display_title]),
'#limit_validation_errors' => array(),
'#submit' => array('::submitDisplayEnable', '::submitDelayDestination'),
'#prefix' => '<li class="enable">',
......@@ -424,7 +421,7 @@ public function getDisplayDetails($view, $display) {
}
$build['top']['actions']['path'] = array(
'#type' => 'link',
'#title' => $this->t('View !display_title', array('!display_title' => $display_title)),
'#title' => $this->t('View @display_title', ['@display_title' => $display_title]),
'#options' => array('alt' => array($this->t("Go to the real page for this display"))),
'#url' => $url,
'#prefix' => '<li class="view">',
......@@ -435,7 +432,7 @@ public function getDisplayDetails($view, $display) {
if (!$is_default) {
$build['top']['actions']['duplicate'] = array(
'#type' => 'submit',
'#value' => $this->t('Duplicate !display_title', array('!display_title' => $display_title)),
'#value' => $this->t('Duplicate @display_title', ['@display_title' => $display_title]),
'#limit_validation_errors' => array(),
'#submit' => array('::submitDisplayDuplicate', '::submitDelayDestination'),
'#prefix' => '<li class="duplicate">',
......@@ -445,7 +442,7 @@ public function getDisplayDetails($view, $display) {
// Always allow a display to be deleted.
$build['top']['actions']['delete'] = array(
'#type' => 'submit',
'#value' => $this->t('Delete !display_title', array('!display_title' => $display_title)),
'#value' => $this->t('Delete @display_title', ['@display_title' => $display_title]),
'#limit_validation_errors' => array(),
'#submit' => array('::submitDisplayDelete', '::submitDelayDestination'),
'#prefix' => '<li class="delete">',
......@@ -459,7 +456,7 @@ public function getDisplayDetails($view, $display) {
$build['top']['actions']['duplicate_as'][$type] = array(
'#type' => 'submit',
'#value' => $this->t('Duplicate as !type', array('!type' => $label)),
'#value' => $this->t('Duplicate as @type', ['@type' => $label]),
'#limit_validation_errors' => array(),
'#submit' => array('::submitDuplicateDisplayAsType', '::submitDelayDestination'),
'#prefix' => '<li class="duplicate">',
......@@ -470,7 +467,7 @@ public function getDisplayDetails($view, $display) {
else {
$build['top']['actions']['undo_delete'] = array(
'#type' => 'submit',
'#value' => $this->t('Undo delete of !display_title', array('!display_title' => $display_title)),
'#value' => $this->t('Undo delete of @display_title', ['@display_title' => $display_title]),
'#limit_validation_errors' => array(),
'#submit' => array('::submitDisplayUndoDelete', '::submitDelayDestination'),
'#prefix' => '<li class="undo-delete">',
......@@ -480,7 +477,7 @@ public function getDisplayDetails($view, $display) {
if ($is_enabled) {
$build['top']['actions']['disable'] = array(
'#type' => 'submit',
'#value' => $this->t('Disable !display_title', array('!display_title' => $display_title)),
'#value' => $this->t('Disable @display_title', ['@display_title' => $display_title]),
'#limit_validation_errors' => array(),
'#submit' => array('::submitDisplayDisable', '::submitDelayDestination'),
'#prefix' => '<li class="disable">',
......
core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
View file @ 0cd35ba0
......@@ -7,9 +7,10 @@
namespace Drupal\Tests\Core\Form;
use Drupal\Component\Utility\Html;
use Drupal\Component\Utility\NestedArray;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Access\AccessResultForbidden;
use Drupal\Core\Cache\Context\CacheContextsManager;
use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Form\EnforcedResponseException;
use Drupal\Core\Form\FormBuilder;
......@@ -19,11 +20,10 @@
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\Core\Session\AccountProxyInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\RequestStack;
use Drupal\Core\Cache\Context\CacheContextsManager;
use Symfony\Component\DependencyInjection\ContainerBuilder;
/**
* @coversDefaultClass \Drupal\Core\Form\FormBuilder
......@@ -306,6 +306,53 @@ public function testBuildFormWithObject() {
$this->assertArrayHasKey('#id', $form);
}
/**
* Tests whether the triggering element is properly identified.
*
* @param string $element_value
* The input element "#value" value.
* @param string $input_value
* The corresponding submitted input value.
*
* @covers ::buildForm
*
* @dataProvider providerTestBuildFormWithTriggeringElement
*/
public function testBuildFormWithTriggeringElement($element_value, $input_value) {
$form_id = 'test_form_id';
$expected_form = $form_id();
$expected_form['actions']['other_submit'] = [
'#type' => 'submit',
'#value' => $element_value,
];
$form_arg = $this->getMockForm($form_id, $expected_form, 2);
$form_state = new FormState();
$form_state->setProcessInput();
$form_state->setUserInput(['form_id' => $form_id, 'op' => $input_value]);
$this->request->setMethod('POST');
$this->formBuilder->buildForm($form_arg, $form_state);
$this->assertEquals($expected_form['actions']['other_submit']['#value'], $form_state->getTriggeringElement()['#value']);
}
/**
* Data provider for ::testBuildFormWithTriggeringElement().
*/
public function providerTestBuildFormWithTriggeringElement() {
$plain_text = 'Other submit value';
$markup = 'Other submit <input> value';
return [
'plain-text' => [$plain_text, $plain_text],
'markup' => [$markup, $markup],
// Note: The input is always decoded, see
// \Drupal\Core\Form\FormBuilder::buttonWasClicked, so we do not need to
// escape the input.
'escaped-markup' => [Html::escape($markup), $markup],
];
}
/**
* Tests the rebuildForm() method for a POST submission.
*/
......
core/tests/Drupal/Tests/Core/Template/AttributeTest.php
View file @ 0cd35ba0
......@@ -7,6 +7,8 @@
namespace Drupal\Tests\Core\Template;
use Drupal\Component\Utility\Html;
use Drupal\Core\Render\SafeString;
use Drupal\Core\Template\Attribute;
use Drupal\Core\Template\AttributeArray;
use Drupal\Core\Template\AttributeString;
......@@ -353,6 +355,27 @@ public function testPrint() {
$this->assertTrue(strpos($html, 'enabled') !== FALSE);
}
/**
* @covers ::createAttributeValue
* @dataProvider providerTestAttributeValues
*/
public function testAttributeValues(array $attributes, $expected) {
$this->assertEquals($expected, (new Attribute($attributes))->__toString());
}
public function providerTestAttributeValues() {
$data = [];
$string = '"> <script>alert(123)</script>"';
$data['safe-object-xss1'] = [['title' => SafeString::create($string)], ' title="&quot;&gt; alert(123)&quot;"'];
$data['non-safe-object-xss1'] = [['title' => $string], ' title="' . Html::escape($string) . '"'];
$string = '&quot;><script>alert(123)</script>';
$data['safe-object-xss2'] = [['title' => SafeString::create($string)], ' title="&quot;&gt;alert(123)"'];
$data['non-safe-object-xss2'] = [['title' => $string], ' title="' . Html::escape($string) . '"'];
return $data;
}
/**
* Checks that the given CSS class is present in the given HTML snippet.
*
......
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