Problem/Motivation

search_excerpt 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. create a node, enable search module, run cron, and search for a word in your article
  3. Compare the output of the snippet above in HEAD and with the patch applied. (You should see strongs wrapped around the keywords you searched for). Confirm that there is no double-escaping.
  4. 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
#5 after-search.png121.24 KBjoelpittet
#5 before-search.png120.9 KBjoelpittet
#2 remove_or_document-2501747-2.patch890 bytescwells
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,717 pass(es). View

Comments

cwells’s picture

Issue summary: View changes
cwells’s picture

Status: Active » Needs review
FileSize
890 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,717 pass(es). View

The only tag that is expected to be in there is a strong, and it's put there via preg_replace. So we can run it through Xss:filter() instead rather than just assuming it safe.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Perfect! Because the only tag that should be there is strong this is great. And it's checkplain'd right before.

xjm’s picture

Title: Remove or document SafeMarkup::set in search_excerpt » Remove SafeMarkup::set in search_excerpt
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Nice -- this looks like an excellent solution.

Has this been tested manually? If so can we document it on the issue?

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
120.9 KB
121.24 KB

Added a couple pages with various HTML into the body of Article in full HTML filter.

Before:

After:

  • xjm committed faec539 on 8.0.x
    Issue #2501747 by cwells, joelpittet, peezy: Remove SafeMarkup::set in...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Excellent, thanks for the manual testing with different markup in the content.

This issue is part of a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Thanks @peezy, @joelpittet, and @cwells!

Status: Fixed » Closed (fixed)

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