Problem/Motivation

\Drupal\views_ui\ViewUI::renderPreview() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code.
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented.

Manual testing steps (for XSS and double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  3. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#13 PATCH11-admin-structure-views-view-taxonomy_term-1434224006829.png62.79 KBcrowdcg
#13 PATCH11-admin-structure-views-view-content-1434223986754.png1.39 MBcrowdcg
#13 HEAD-admin-structure-views-view-taxonomy_term-1434223922281.png62.72 KBcrowdcg
#13 HEAD-admin-structure-views-view-content-1434223876907.png1007.58 KBcrowdcg
#11 interdiff.txt4.11 KBkgoel
#11 2501639-11.patch4.11 KBkgoel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,787 pass(es). View
#10 interdiff.txt4.11 KBkgoel
#10 2501639-10.patch5.35 KBkgoel
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#7 interdiff.txt889 bytesdawehner
#6 remove_safemarkup_set-2501947-6.patch1.89 KBleslieg
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,448 pass(es). View
#4 Screen Shot 2015-06-08 at 16.42.04.png425.03 KBdawehner
#4 Screen Shot 2015-06-08 at 16.41.53.png220.09 KBdawehner
#3 remove_safemarkup_set-2501947-3.patch1.9 KBleslieg
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,718 pass(es). View

Comments

Cottser’s picture

This one is up for grabs.

leslieg’s picture

working on this as part of NHDevDays2

leslieg’s picture

Status: Active » Needs review
FileSize
1.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,718 pass(es). View

changed SafeMarkup::set to SafeMarkup::format in 4 places in renderpreview()

dawehner’s picture

I'm sorry but this is kinda broken. Feel free to compare the two screenshots

dawehner’s picture

In order to reproduce use drush vd in your drupal root and then preview a view.

leslieg’s picture

FileSize
1.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,448 pass(es). View

fixed formatting issue identified in testing

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
889 bytes

Thank you!

For easier additional reviews it is nice to provide an interdiff, see https://www.drupal.org/documentation/git/interdiff

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks @leslieg and thanks @dawehner for the manual testing and screenshots. (Adding dawehner to the proposed issue credit.)

Looking at the code here, I think this markup should be added to render arrays instead of using SafeMarkup::format() to get it in there. There's already so much render array goodness being built in ViewUI::renderPreview(). Let's build it once and avoid early and nested sanitization, duplicated SafeMarkup entries, and unalterable markup.

Edit: I mean if you don't use the render API in a method called renderPreview(), where can you? :)

kgoel’s picture

working on it

kgoel’s picture

Status: Needs work » Needs review
FileSize
5.35 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
4.11 KB
kgoel’s picture

FileSize
4.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,787 pass(es). View
4.11 KB

Ooops...patch had wrong file, fixed it

I have added markup to render arrays as suggested in #8 and removed SafeMarkup::format from the patch

The last submitted patch, 10: 2501639-10.patch, failed testing.

crowdcg’s picture

Reviewed the patch and tested on a clean branch of HEAD and then with the patch that refactored the code per #8, screenshots below. All the concerns in that comment have been addressed and now use render arrays instead of SafeMarkup::format().

In order to test it I first enabled the following Views Live Preview Settings (Show the SQL query; Show performance statistics; Show other queries run during render during live preview). Then I previewed several views, including Content and Taxonomy as depicted in the screenshots. In each case the results visually and in the markup were identical.

HEAD:

PATCH 11:

xjm’s picture

Status: Reviewed & tested by the community » Fixed

Nice work @kgoel! This is much cleaner now. Thanks also @crowdcg for the manual testing.

This issue is a required part of resolving a critical issue, so per https://www.drupal.org/core/beta-changes, this can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x.

Removed the now unused use statement on commit:

diff --git a/core/modules/views_ui/src/ViewUI.php b/core/modules/views_ui/src/ViewUI.php
index 6abfecb..92b004d 100644
--- a/core/modules/views_ui/src/ViewUI.php
+++ b/core/modules/views_ui/src/ViewUI.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\views_ui;
 
-use Drupal\Component\Utility\SafeMarkup;
 use Drupal\Component\Utility\Html;
 use Drupal\Component\Utility\Timer;
 use Drupal\Component\Utility\Xss;

  • xjm committed 7c62cee on 8.0.x
    Issue #2501947 by kgoel, leslieg, crowdcg, dawehner: Remove SafeMarkup::...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.