Problem/Motivation

StylePluginBase::renderFields 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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, change-set-to-safemarkup-replace.patch, failed testing.

lokapujya’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
840 bytes

Status: Needs review » Needs work

The last submitted patch, 2: 2502561-2.patch, failed testing.

lokapujya’s picture

Assigned: lokapujya » Unassigned

Won't have time for a couple days.

The last submitted patch, change-set-to-safemarkup-replace.patch, failed testing.

joelpittet’s picture

No worries, these failures seem to be prove that this SafeMarkup::set() isn't really safe.
Either: $values and/or $rendered_field are not safe going in. May need to escape them before.

xjm’s picture

YesCT’s picture

Looked up replace to better understand what @joelpittet said in #7

   * Replaces all occurrences of the search string with the replacement string.
   *
   * Functions identically to str_replace(), but marks the returned output as
   * safe if all the inputs and the subject have also been marked as safe.
   *

So, I'm going to just try some things. :)

The last submitted patch, 9: 2502561.9-values.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2502561.9-both.patch, failed testing.

The last submitted patch, 9: 2502561.9-placeholders.patch, failed testing.

YesCT’s picture

So.

some fails were:

Drupal\action\Tests\ActionUninstallTest 13 0 12
Message Group Filename Line Function Status
Array to string conversion Notice SafeMarkup.php 94 Drupal\Component\Utility\SafeMarkup::isSafe()

so I guess we need to escape each string that could be in the arrays.

... but why are the placeholders and the values the same?

YesCT’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
1.27 KB

oh, they are not, one is the array values and the other is the array keys. :)

here is a try at escaping all of both.

--
xjm also suggested

this might be a ok to use SafeMarkup::set(), because in theory it's behind a Views admin permission
stepping through it in a debugger could be helpful though time-consuming

[edit: oops. that interdiff should have ended in .txt so it would not have been sent to the testbot. oops.]

The last submitted patch, 14: interdiff.2502561.9-both.14.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2502561.14.patch, failed testing.

YesCT’s picture

Issue tags: +VDC

as this is views specific, adding a tag for that.

alexpott’s picture

This SafeMarkup::set() is removed as part of #2506581: Remove SafeMarkup::set() from Renderer::doRender - it is still mark the content here as safe but in a different way. I think everything here should be already safe because it has come from the render system already.

Wim Leers’s picture

#18: so should we close this as a duplicate then? Or postpone it until the other lands and then re-evaluate?

joelpittet’s picture

Status: Needs work » Postponed

Postpone for now, and close as duplicate if and when that lands. If not, re-open:)

Before closing, make sure this has been resolved please.

#2506581: Remove SafeMarkup::set() from Renderer::doRender

lauriii’s picture

Status: Postponed » Needs work

Unpostponing

cilefen’s picture

Status: Needs work » Closed (duplicate)

SafeMarkup::set was removed from this class in #2506581: Remove SafeMarkup::set() from Renderer::doRender.