Remove SafeMarkup::set in NodeSearch::prepareResults()

Very straight forward. Thank you @cwells for the steps to reproduce.

Before:

After:

Thanks for documenting the before-and-after testing!

+++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
@@ -328,10 +328,9 @@ protected function prepareResults(StatementInterface $found) {
+      $built = $this->renderer->render($build);
+      $comment_count = $this->moduleHandler->invoke('comment', 'node_update_index', array($node, $item->langcode));
+      $rendered = SafeMarkup::format('@built @comment_count', ['@built' => $built, '@comment_count' => $comment_count]);

I'm slightly hesitant about this pattern. Both comment_node_update_index() and the $this->renderer->render() here are doing early renders. Is it possible to resolve this by removing those early renders instead?

I realize the scope of that would be a lot bigger than the scope of this issue, and at least by using the @ placeholders we are ensuring it's already either in the SafeMarkup list or sanitized. Setting to NR for more feedback; I'll also ping @effulgentsia.

Another issue where this pattern was proposed: #2296929: Remove system_requirements() SafeMarkup::set() use That one was slightly different because the strings are being assembled right there in the same code path, but I still have the concern. In both cases we're using SafeMarkup::format() for something that's kinda leaking beyond just putting variables in a text string safely.

In this case, lower down we have this, so not sure the early render can be avoided:

'snippet' => search_excerpt($keys, $rendered, $item->langcode),

we're using SafeMarkup::format() for something that's kinda leaking beyond just putting variables in a text string safely

The fact that we have a space in there makes me think it's a legit use of SafeMarkup::format(). In fact, should we have a followup for whether to use t(), since could an alternate language want something different than a space or want to reverse the order?

Note that HEAD's tablesort_header() function has SafeMarkup::format('@cell_content@image', ..., which is a straight concatenation with no space, which we might want to change to some other concatenation pattern if we feel it's an abuse of that method. I didn't find any other similar examples in HEAD.

@effulgentsia maybe this proposal has some promise #2501975: Determine how to update code that currently joins strings in SafeMarkup::set()?

The fact that we have a space in there makes me think it's a legit use of SafeMarkup::format().

So back to RTBC;) ?

Yep.

Wow that's a slow test.

Drupal\locale\Tests\LocaleUpdateTest had one fail:

Updates for Contrib module one

LocaleUpdateTest.php 144

Drupal\locale\Tests\LocaleUpdateTest->testUpdateImportSourceRemote()

Thanks @effulgentsia and @joelpittet for looking into that more.

I'm not sure I agree with this statement:

The fact that we have a space in there makes me think it's a legit use of SafeMarkup::format().

In general, my thought is that if we need to do SafeMarkup::format(@thing_1 @thing_2), something is being done wrong somewhere else before that. I spoke to @alexpott about it briefly this morning and he had a similar thought.

Then I thought about this point from @effulgentsia:

In fact, should we have a followup for whether to use t(), since could an alternate language want something different than a space or want to reverse the order?

On first read, that raises a red flag that there should absolutely be a format_plural() call if it's making a comment count (which the screenshots seem to support). So I tried to dig into it more to see if @comment_count was properly formatting plurals further down. And so I read comment_node_update_index(). At which point my jaw kind of dropped because it appeared that we're rendering the entire comment thread apparently to just get a count of how many comments there are? Granted, this is a very rare operation and part of something that's already batched or on cron and expensive (updating search indexes). But it still surprised me. I also didn't understand how this ended up being a comment "count" in the first place.

@Cottser pointed out this:

In this case, lower down we have this, so not sure the early render can be avoided:

'snippet' => search_excerpt($keys, $rendered, $item->langcode),

That's a good point. (See in NodeSearch::prepareResult(s) for the whole deal.) But if the comment count really is just a count, why would we need to run search_excerpt() on it? Why couldn't the comment count just be appended to it later?

And also, if it is just the "2 comments" bit in the screenshots as a unit, then the formatting wrapping it is not part of a translation -- it's part of the search snippet output. Which, ideally, should be themeable output. And therefore in an actual Twig template.

This seems like a huge stack of stuff, and it's possible we might decide cleaning up part or all of it is too disruptive in or otherwise not appropriate for beta. If that turns out to be the case and if I do understand what's going on correctly, a postponed followup issue to put it in a template with @todo in the code might be an option. However, I'd also prefer not to simply move technical debt around, so I'd like to investigate and understand this whole code flow better before I make a recommendation.

Leaving at RTBC (since my review isn't actionable at this point and I don't want to push back a second time if I'm on the wrong track here), but assigning to myself to make it clear to other committers that I'd like to take a look at this more. I might also end up reaching out to the comment and search maintainers about the issue once I understand better what's going on here.

Thanks for your patience. :)

Forgot to mention, @Cottser, thanks for documenting the test failure on the issue prior to the retest.

Testbot terminated. Back to RTBC to try and get this back in the queue.

Working on this at the Drupal North sprint

Rerolled the patch from #3.

The conflict was that $this->renderer->render($build) had been changed to $this->renderer->renderPlain($build)

I worked on this with bohemier please make sure he's attributed.

Adding a note at the top of the issue summary to credit @bohemier.

There's no existing tests for this

It seems like the comment count is figured out elsewhere. It works this way.

Working with cilefen on this at DrupalGovCon.

The patch in comment #25 is actually probably good.

I think the confusion is over the naming of $comment_count.

Patch coming.

Re #18 - it is not the comment count, it is the rendered comments to be snippet-ed in the search output. The count is evidently found some other way.

$comment_count renamed to $comments

Added an XSS test with a script in the comment subject.

I'll send this back to RTBC, thanks for the fixes and see where @xjm is at with the latest iteration and use of SafeMarkup::format()

Isn't there an assertEscaped() we could use instead of assertRaw?

No, we can't because it gets wrapped in a <strong> in the search results.

Reading search_excerpt I'm not sure that we need to worry about whether the $text input is safe. It removes all html. I think it's docs need to be improved and probably by this patch. Also I'm not sure why it's doing any SafeMarkup::checkPlain() inside it since it has stripped all the tags - but that is a separate issue.

  // Prepare text by stripping HTML tags and decoding HTML entities.
  $text = strip_tags(str_replace(array('<', '>'), array(' <', '> '), $text));
  $text = Html::decodeEntities($text);

+++ b/core/modules/search/src/Tests/SearchCommentTest.php
@@ -152,6 +158,14 @@ function testSearchResultsComment() {
+    // Verify the evil comment subject is escaped in search results.
+    $this->drupalPostForm('search/node', $edit, t('Search'));
+    $this->assertRaw('alert(&#039;<strong>hello</strong>&#039;);');

this does not actually assert that the (script) html tag was stripped out.
(I did confirm the the escaped quotes make an alert not work even if it is in a script tag.

and I looked at SearchExcerptTest and did not see any tests which verify that html tags are stripped out.

"Needs work" to
i) improve the docs of search_excerpt() as @alexpott suggests in #37 ... and
ii) to improve the test, maybe by adding a test to SearchExcerptTest asserting html tags are stripped and/or to SearchCommentTest asserting there is no script tag.
iii) update the comment count comment. maybe from // Fetch comment count for snippet. maybe to // Fetch comments for snippet.

The rest of the patch looks ok.

Do we still want the issue @xjm asks for in #18?
a) To have the search snippet (or the "X comments") output be themeable output. And in an actual Twig template.

unassigning from xjm because it was set that way back in june when this was also remaining at rtbc but xjm had an action item for it. I think we are not waiting on that anymore.

Examining this at DrupalCorn sprint.

Setting to needs review for testbot.

Patch to address i) and iii) in #38. Will attempt ii) after caffeine.

Regarding ii) in #38, there is actually a test in testSearchExcerpt() that implicitly tests for stripping of HTML tags.

I updated the assertion text to reflect this. I don't think we need another test since HTML tag stripping is already being tested.

    $text = 'The <strong>quick</strong> <a href="#">brown</a> fox &amp; jumps <h2>over</h2> the lazy dog';
    // Note: The search_excerpt() function adds some extra spaces -- not
    // important for HTML formatting. Remove these for comparison.
    $expected = 'The quick brown fox &amp; jumps over the lazy dog';
    $result = preg_replace('| +|', ' ', search_excerpt('nothing', $text));
    $this->assertEqual(preg_replace('| +|', ' ', $result), $expected, 'Entire string, stripped of HTML tags, is returned when keyword is not found in short string');

Updating steps to test.

Going through manual testing steps in summary, both HEAD and the patch in #41 result in the following markup from the search:

<div class="search-result__snippet-info">
      <p class="search-result__snippet">…               Textbefore &lt;script&gt;alert(&#039;XSS Body&#039;);&lt;/script&gt; <strong>textafter</strong>    
        
  
     
  
  
        

    
      
 …            Textbefore &lt;script&gt;alert(&#039;XSS Comment&#039;);&lt;/script&gt; <strong>textafter</strong>    
        
  

                  Delete        …</p>

thanks for those changes and updating the issue summary and manual testing.

all remaining tasks from the issue summary are done.

I looked through the patch again. and the thing I mentioned before, about asserting the script tag is not there, seems not strong enough to be stubborn about this.

Nice; this patch is looking much better than before. Phew! Thanks for the added test coverage and the manual testing.

1. +++ b/core/modules/search/src/Tests/SearchCommentTest.php
   @@ -152,6 +158,14 @@ function testSearchResultsComment() {
   +    $this->assertRaw('alert(&#039;<strong>hello</strong>&#039;);');
   

   We should also probably pair this with asserting that <script> isn't there. Edit: So @YesCT, yes, you are right about that. :)

2. +++ b/core/modules/search/src/Tests/SearchExcerptTest.php
   @@ -39,7 +39,7 @@ function testSearchExcerpt() {
   -    $this->assertEqual(preg_replace('| +|', ' ', $result), $expected, 'Entire string is returned when keyword is not found in short string');
   +    $this->assertEqual(preg_replace('| +|', ' ', $result), $expected, 'Entire string, stripped of HTML tags, is returned when keyword is not found in short string');
   

   This changed assertion text doesn't seem to actually describe what the assertion is asserting. So I'm concerned about that. Never mind. I understand now how this is what the assertion is asserting -- it's just a couple lines above. I'll look again and try to suggest a clarification.

This should cover the script tag check (just in case there was whitespace or something, it ensures the tag doesn't exist at all in the raw output.

I am going to review this.

so there are a few things happening.

when using this as the comment body:
goodbye<script>alert('goodbye');
while manually testing in both standard and minimal profile, @kgoel found that the search only finds one goodbye (the one outside the script tag), and the script tags and everything inside them are stripped out of the snippet.

but when running the test locally in the UI, and looking at the verbose output, @kgoel found that both goodbye's were in the snippet.

Using the example in the summary, as a comment body:
Textbefore <script>alert('XSS Comment');</script> textafter
and searching for XSS Comment
there are NO search results.

Is this the expected behavior.
Is this the behavior in head?

+++ b/core/modules/search/src/Tests/SearchCommentTest.php
@@ -152,6 +160,26 @@ function testSearchResultsComment() {
+    // Verify the evil comment subject is escaped in search results.
+    $this->drupalPostForm('search/node', $edit, t('Search'));
+    $this->assertRaw('alert(&#039;<strong>hello</strong>&#039;);');
+    $this->assertNoRaw('<script>');

this is not verifying the script tag is escaped.

it might be verifying that it is not not escaped (that the script tag is not there).

This is for the comment subject, where the script tag *is* escaped, so I think the assertRaw should assert <script>... etc.

.... only looked that far at the patch. will look more in a bit.

Using the example in the summary, as a comment body:
Textbefore

alert('XSS Comment');

textafter
and searching for XSS Comment
there are NO search results.

Is this the expected behavior.
Is this the behavior in head?

With Patch -
I have tried above test case scenario with goodbye

in the comment body and manually search for "goodbye" , the search only finds goodbye outside script tag.

In Head - same result as with patch.

Worked with YesCT on tests. We worked on test for searching the keyword near script tag and inside script tag. The first test scenario passes as expected but second is failing. We checked in HEAD - if search for a keyword inside script tag then it returns no result and with this patch manual testing, it doesn't return the keyword inside the script tag in the search result but running test file locally fails because it does find the keyword inside the script tag. We aren't sure if this is within the scope of this issue and what to assert.

Made the related issue for writing test for searching keyword inside script tag in comment body #2551135: Missing test coverage for search keyword inside script tag in comment body

+++ b/core/modules/search/src/Tests/SearchCommentTest.php
@@ -152,6 +168,35 @@ function testSearchResultsComment() {
+    // Verify contents inside script tag in evil comment body is stripped from
+    // search results.
+    $this->drupalPostForm('search/node', $edit, t('Search'));
+    // Search for content inside script tag should return no result.
+    $this->assertText('Your search yielded no results.');

so since we have the other issue, here we can just add an assertion that we have no script tag.

Addressed #59 in this patch.

Thanks for sticking with this issue and addressing that so quickly.

+++ b/core/modules/search/src/Tests/SearchCommentTest.php
@@ -194,8 +194,7 @@ function testSearchResultsComment() {
     // Verify contents inside script tag in evil comment body is stripped from
     // search results.
     $this->drupalPostForm('search/node', $edit, t('Search'));
-    // Search for content inside script tag should return no result.
-    $this->assertText('Your search yielded no results.');
+    $this->assertNoRaw('<script>');

the assert looks good, but the comment above is no longer correct for what we are trying to do here.

Now, we are just trying to verify that the script tag is not in the search results.

I'm going to make a few small changes to grammar in the comments.

This may also needs work to get tests for the *node* and not just comments.

needs review, just so the bot runs. still needs work. @kgoel is looking at if there is test coverage for searching in the node (not the comments).

Normally we would want issues related to the NodeSearch class to be in the search module component (even though the file is in the Node module, the Search module maintainers normally maintain this file). I didn't know about this issue until I saw #2551135: Missing test coverage for search keyword inside script tag in comment body today. Having it in "theme system" isn't great.

Anyway, I looked at the code changes and they look OK.

Does this test fail without them by the way, or are you just adding test coverage for something that was already true?

Discussed with @YesCT and @kgoel.

I think there is already sufficient test coverage per #41, #45, and #46. The potential bug for #2551135: Missing test coverage for search keyword inside script tag in comment body (where, per their explanation, manual testing and automated testing have different results) seems out of scope for this issue.

For this assertion:

+++ b/core/modules/search/src/Tests/SearchExcerptTest.php
@@ -39,7 +39,7 @@ function testSearchExcerpt() {
-    $this->assertEqual(preg_replace('| +|', ' ', $result), $expected, 'Entire string is returned when keyword is not found in short string');
+    $this->assertEqual(preg_replace('| +|', ' ', $result), $expected, 'Entire string, stripped of HTML tags, is returned when keyword is not found in short string');

I recommend reading the whole test method to see how that works.

For me this patch has had extensive manual testing and it has both existing and extended test coverage, so I think that is sufficient.

Thanks @jhodgdon.

Does this test fail without them by the way, or are you just adding test coverage for something that was already true?

The latter I believe; as far as I understand the additional test coverage is intended to ensure that no security regression is introduced by this patch.

Alrighty, so I think this is RTBC.

Sorry again @Berdir. :P

Yep search_excerpt() will strip all markup. Rendering safeness moot. SearchExcerptTest has plenty of excellent coverage. Committed 05b5f4c and pushed to 8.0.x. Thanks!