Problem/Motivation

SimpletestResultsForm.php 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
#2 remove_safemarkup_set-2501749-2.patch832 bytesjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,487 pass(es). View

Comments

Cottser’s picture

joelpittet’s picture

Assigned: lokapujya » Unassigned
Issue summary: View changes
Status: Active » Needs review
FileSize
832 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,487 pass(es). View

Using admin xss because it's in admin.

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

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

Status: Needs work » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Seems to be a good solution for me

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I discussed this with @xjm and we agreed that this solution was a good one. Committed 3dd3837 and pushed to 8.0.x. Thanks!

  • alexpott committed 3dd3837 on 8.0.x
    Issue #2501749 by joelpittet: Remove SafeMarkup::set in...

Status: Fixed » Closed (fixed)

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