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.

Files: 
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
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,245 pass(es). View
#2 remove_safemarkup_set-2502009-2.patch1.03 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,724 pass(es), 1 fail(s), and 0 exception(s). View

Comments

Cottser’s picture

Cottser’s picture

Status: Active » Needs review
FileSize
1.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,724 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs work » Needs review
FileSize
1.99 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,245 pass(es). View
1.11 KB

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

lauriii’s picture

The IS is missing steps to reproduce..

Wim Leers’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
212.74 KB
230.37 KB

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

Manually tested.

alexpott’s picture

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.