Add to commit credit: bohemier

Problem/Motivation

NodeSearch::prepareResults() calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code. See #37
  • 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. (done in #41) 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. Under "Manage" in the administration menu, select "Content", then "Add content" of type "Article". In the body field select "Full HTML", click on source and add the following: Textbefore <script>alert('XSS Body');</script> textafter
  3. Under the newly created Article node/1, add comment title : Titlebefore <script>alert('XSS Comment');</script> titleafter and in the comment body chose "Full HTML" and click on source and add Textbefore <script>alert('XSS Comment');</script> textafter
  4. Under "Configure" in the administration menu, select search pages and re-index site.
  5. select "Cron", then select "Run cron" so the search indexer runs (search module is enabled in standard install profile)
  6. From your site homepage search for "goodbye". You will see the node body is not escaped as it is supposed to
  7. From your site homepage search for "titleafter" . You will see escaped title in the comment title because title field uses plain text.

Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.

User interface changes

N/A

API changes

N/A

Files: 
CommentFileSizeAuthor
#64 interdiff.2501757.62.64.txt3.87 KBYesCT
#64 2501757-64.patch5.76 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,211 pass(es). View
#62 interdiff.txt637 byteskgoel
#62 2501757-62.patch5.43 KBkgoel
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,596 pass(es). View
#60 2501757-60.patch5.48 KBkgoel
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,595 pass(es). View
#60 interdiff.txt724 byteskgoel
#55 interdiff.txt3.25 KBkgoel
#55 2501757-55.patch5.57 KBkgoel
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,550 pass(es), 1 fail(s), and 0 exception(s). View
#49 interdiff.txt2.26 KBkgoel
#49 2501757-49.patch4.81 KBkgoel
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: failed to clear checkout directory. View
#46 remove_safemarkup_set-2501757-46.patch4.19 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,841 pass(es). View
#46 interdiff.txt771 bytesjoelpittet
#41 2501757.41.patch4.16 KBjvandyk
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,714 pass(es). View
#41 interdiff.2501757.40.41.txt1.02 KBjvandyk
#40 interdiff.2501757.37.40.txt1.25 KBjvandyk
#40 2501757.40.patch3.13 KBjvandyk
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,714 pass(es). View
#37 33-37-interdiff.txt1014 bytesalexpott
#37 2501757.37.patch2.5 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,857 pass(es). View
#33 remove_safemarkup_set-2501757-33.patch2.6 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,869 pass(es). View
#33 interdiff-2501757-32-33.txt1.44 KBcilefen
#32 remove_safemarkup_set-2501757-32.patch1.04 KBcmanalansan
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,811 pass(es). View
#29 remove_safemarkup_set-2501757-29.patch1.23 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,811 pass(es). View
#29 interdiff-2501757-25-29.txt1.09 KBcilefen
#25 remove_safemarkup_set-2501757-25.patch1.37 KBadamwhite
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,242 pass(es). View
#6 Search_for_ipsum_-_after.png181.08 KBjoelpittet
#6 Search_for_ipsum_-_before.png170.92 KBjoelpittet
#3 remove_safemarkup_set-2501757-1.patch1.04 KBcwells
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove_safemarkup_set-2501757-1.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

cwells’s picture

cwells’s picture

Status: Active » Needs review
Issue tags: +Twig, +D8 Accelerate, +SafeMarkup

String concatenation pattern: split out into two variables and use SafeMarkup::format to 'concatenate' them.

cwells’s picture

FileSize
1.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch remove_safemarkup_set-2501757-1.patch. Unable to apply patch. See the log in the details link for more information. View

Always something...

cwells’s picture

Issue summary: View changes
cwells’s picture

Issue summary: View changes
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
170.92 KB
181.08 KB

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

Before:


After:

xjm’s picture

Status: Reviewed & tested by the community » Needs review

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.

Cottser’s picture

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

'snippet' => search_excerpt($keys, $rendered, $item->langcode),
joelpittet’s picture

Assigned: cwells » effulgentsia
effulgentsia’s picture

Assigned: effulgentsia » Unassigned

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.

joelpittet’s picture

joelpittet’s picture

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

So back to RTBC;) ?

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

Yep.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: remove_safemarkup_set-2501757-1.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

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()

cwells’s picture

I was wondering if it was just a slow test or if it was this https://qa.drupal.org/node/228 - pifr auto-retesting anything that's RTBC. With that said, I re-ran this test locally and it passed with this same patch.

Going to kick the testbot anyway, for assurance.

xjm’s picture

Assigned: Unassigned » xjm

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. :)

xjm’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: remove_safemarkup_set-2501757-1.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: remove_safemarkup_set-2501757-1.patch, failed testing.

adamwhite’s picture

Working on this at the Drupal North sprint

adamwhite’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,242 pass(es). View

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.

Cottser’s picture

Issue summary: View changes

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

Luckhardt Labs’s picture

Issue tags: +DrupalNorth2015

Updating the issue tag to include a hashless DrupalNorth2015 on behalf of the Drupal North sprinting group.

lauriii’s picture

Issue tags: +Needs tests

There's no existing tests for this

cilefen’s picture

FileSize
1.09 KB
1.23 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,811 pass(es). View

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

cmanalansan’s picture

Status: Needs review » Needs work

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.

cilefen’s picture

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.

cmanalansan’s picture

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

$comment_count renamed to $comments

cilefen’s picture

FileSize
1.44 KB
2.6 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,869 pass(es). View

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

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

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()

cilefen’s picture

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

cilefen’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.5 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 99,857 pass(es). View
1014 bytes

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);
YesCT’s picture

Assigned: xjm » Unassigned
Status: Needs review » Needs work
+++ 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.

jvandyk’s picture

Examining this at DrupalCorn sprint.

jvandyk’s picture

Status: Needs work » Needs review
FileSize
3.13 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,714 pass(es). View
1.25 KB

Setting to needs review for testbot.

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

jvandyk’s picture

FileSize
1.02 KB
4.16 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,714 pass(es). View

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');
AnnGreazel’s picture

Issue summary: View changes

Updating steps to test.

jvandyk’s picture

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>

YesCT’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
771 bytes
4.19 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 100,841 pass(es). View

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.

kgoel’s picture

Assigned: Unassigned » kgoel

I am going to review this.

kgoel’s picture

Issue summary: View changes
kgoel’s picture

FileSize
4.81 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: failed to clear checkout directory. View
2.26 KB
kgoel’s picture

Issue summary: View changes
kgoel’s picture

Issue summary: View changes
YesCT’s picture

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?

YesCT’s picture

+++ 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 &lt;script&gt;... etc.

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

kgoel’s picture

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

alert('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.

kgoel’s picture

FileSize
5.57 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,550 pass(es), 1 fail(s), and 0 exception(s). View
3.25 KB

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.

kgoel’s picture

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

The last submitted patch, 49: 2501757-49.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 55: 2501757-55.patch, failed testing.

YesCT’s picture

+++ 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.

kgoel’s picture

Status: Needs work » Needs review
FileSize
724 bytes
5.48 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,595 pass(es). View

Addressed #59 in this patch.

YesCT’s picture

Status: Needs review » Needs work

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.

kgoel’s picture

Status: Needs work » Needs review
FileSize
5.43 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 102,596 pass(es). View
637 bytes
YesCT’s picture

Status: Needs review » Needs work

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.

YesCT’s picture

Status: Needs work » Needs review
FileSize
5.76 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,211 pass(es). View
3.87 KB

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).

jhodgdon’s picture

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?

xjm’s picture

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.

xjm’s picture

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.

xjm’s picture

Assigned: kgoel » Berdir
Status: Needs review » Reviewed & tested by the community

Alrighty, so I think this is RTBC.

xjm’s picture

Assigned: Berdir » Unassigned

Sorry again @Berdir. :P

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 05b5f4c on 8.0.x
    Issue #2501757 by kgoel, cilefen, jvandyk, joelpittet, YesCT, alexpott,...

Status: Fixed » Closed (fixed)

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