Problem/Motivation

SearchExtraTypeSearch::execute() 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. Install the search_extra_type module
  3. Go to /search, click the "Dummy search type" tab, and enter <test>
  4. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  5. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#7 2502009-after.png230.37 KBwim leers
#7 2502009-before.png212.74 KBwim leers
#4 interdiff.txt1.11 KBjoelpittet
#4 remove_safemarkup_set-2502009-4.patch1.99 KBjoelpittet
#2 remove_safemarkup_set-2502009-2.patch1.03 KBstar-szr

Comments

star-szr’s picture

star-szr
he/him
Primary language English
commented
Parent issue: #2501819: Remove SafeMarkup::set in search_embedded_form_preprocess_search_result() » #2280965: [meta] Remove every SafeMarkup::set() call
star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new remove_safemarkup_set-2502009-2.patch1.03 KB

Completely untested, letting testbot do the work for now.

Status: Needs review » Needs work

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

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new remove_safemarkup_set-2502009-4.patch1.99 KB
new interdiff.txt1.11 KB

So close, here's the other side that needs escaping because of the => returned from checkPlain'd print_r(array(), TRUE)

izus queued 4: remove_safemarkup_set-2502009-4.patch for re-testing.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

The IS is missing steps to reproduce..

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2502009-before.png212.74 KB
new 2502009-after.png230.37 KB

Figured out the STR myself, added them to the IS.

Manually tested.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/search/tests/modules/search_extra_type/src/Plugin/Search/SearchExtraTypeSearch.php
@@ -64,7 +64,7 @@ public function execute() {
+        'snippet' => SafeMarkup::format("Dummy search snippet to display. Keywords: @keywords\n\nConditions: @search_parameters", ['@keywords' => $this->keywords, '@search_parameters' => print_r($this->searchParameters, TRUE)]),

If this was 'snippet' => "Dummy search snippet to display. Keywords: {$this->keywords}\n\nConditions: " . print_r($this->searchParameters, TRUE), instead it would still work because Twig auto-escape would just kick in. I think we should reserve SafeMarkup::format() for joining markup that needs to be marked safe together. Here the whole point is that you want the result checkplain-ed...

That said... if it used <br/> instead of \n you'd need to use SafeMarkup::format... why are we using \n?

But meh this is test code and generally a search snippet would need to be marked as safe (see search_excerpt) so let's proceed here. Committed d7ab90d and pushed to 8.0.x. Thanks!

  • alexpott committed d7ab90d on 8.0.x 
    Issue #2502009 by joelpittet, Cottser, Wim Leers: Remove SafeMarkup::set...

Status: Fixed » Closed (fixed)

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