Problem/Motivation

template_preprocess_views_ui_view_info() 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.
  4. Manual testing/ markup before & after screenshots.

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
#6 remove_safemarkup_set-2502095-6.patch2.47 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,483 pass(es). View
#4 remove_safemarkup_set-2502089-3.patch2.51 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,490 pass(es), 273 fail(s), and 2 exception(s). View
#2 remove_safemarkup_set-2502095-2.patch5.78 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,412 pass(es), 273 fail(s), and 2 exception(s). View

Comments

joelpittet’s picture

joelpittet’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs manual testing
FileSize
5.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,412 pass(es), 273 fail(s), and 2 exception(s). View

I've tried this out manually and it worked. Let's see what the testbot says (may be whitespace fails in the tests I'd expect).

Could use another round of manual testing/screenshots.

Status: Needs review » Needs work

The last submitted patch, 2: remove_safemarkup_set-2502095-2.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,490 pass(es), 273 fail(s), and 2 exception(s). View

Whoops left some stuff from another patch in there.

Status: Needs review » Needs work

The last submitted patch, 4: remove_safemarkup_set-2502089-3.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,483 pass(es). View

LOL, wow I must be tired, I only put the other issue stuff...

joelpittet’s picture

That's the ticket!

cwells’s picture

Status: Needs review » Reviewed & tested by the community

This is all display logic, so it can all be in the template. We're getting a much better experience for the themer here, and removing an entire preprocess function. Not to mention getting rid of a SafeMarkup::set() and capitalizing on the existing safe_join filter.

Win all around. "I love this plan... I'm excited to be a part of it! Let's to do it!"

dawehner’s picture

The more templates the better, IMHO +1

Cottser’s picture

Title: Remove SafeMarkup::set in core/modules/views_ui/views_ui.theme.inc » Remove SafeMarkup::set in template_preprocess_views_ui_view_info()
dawehner’s picture

Issue tags: -Needs manual testing

Manually tested it. Looks perfect

  • xjm committed 691697f on 8.0.x
    Issue #2502095 by joelpittet, cwells: Remove SafeMarkup::set in...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Awesome! This is now my favorite issue from the New Hampshire sprint (aside from the epic #2273925: Ensure #markup is XSS escaped in Renderer::doRender() of course). It's exactly the best-case scenario.

I don't know all of the best practices for Twig, but everything in the patch makes sense to me reading it and all the previous logic seems to be there. Given that this was authored by one of the theme system maintainers as well as reviewed and manually tested by a Views maintainer, I'm comfortable committing this.

This issue only changes the themeable output and is a required part of a critical, 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. Yay!

Status: Fixed » Closed (fixed)

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