Problem/Motivation

The following PHP warning occurs when invalid search keys are passed to the Terms parse mode.

Warning: Invalid argument supplied for foreach() in Drupal\search_api\Plugin\search_api\parse_mode\Terms->parseInput() (line 33

Steps to reproduce

  • Create a search page View with a Fulltext search filter with the Parse mode of "Multiple words" (terms). Set the Views Filter identifier as "search".
  • View the search page
  • Pass an invalid string as the keywords. Example: For a Views Filter identifier as "search", set the url to "/some-search-page?search=1%00%C0%A7%C0%A2%252527%252522"
  • The following PHP warning occurs:
    Warning: Invalid argument supplied for foreach() in Drupal\search_api\Plugin\search_api\parse_mode\Terms->parseInput() (line 33
CommentFileSizeAuthor
#8 3304560-8--keywords_parsing_invalid_utf8.patch2.88 KBdrunken monkey
#8 3304560-8--keywords_parsing_invalid_utf8--tests_only.patch568 bytesdrunken monkey
#8 3304560-8--keywords_parsing_invalid_utf8--interdiff.txt2.47 KBdrunken monkey
#6 search_api-3304560-6.patch1.46 KBrecrit
#5 search_api-3304560-5.patch854 bytesgertlor
#2 search_api-3304560-2.patch644 bytesrecrit

Comments

recrit created an issue. See original summary.

recrit’s picture

recrit commented
Status: Active » Needs review
StatusFileSize
new search_api-3304560-2.patch644 bytes

the attached patch adds an is_array() check after the preg_split()

Status: Needs review » Needs work

The last submitted patch, 2: search_api-3304560-2.patch, failed testing. View results

recrit’s picture

recrit commented

The failing test "IntegrationTest:: testInstallAndDefaultSetupWorking() line 116" is bit confusing since it is an empty search with no keys passed in. The change in this issue only affects the Terms parse_mode plugin. The parse_mode plugins are only called when there are keys.

Drupal\Tests\search_api_db_defaults\Functional\IntegrationTest

115  $this->submitForm([], 'Search');
116  $this->assertSession()->pageTextNotContains($title); // Failing Test
gertlor’s picture

gertlor commented
StatusFileSize
new search_api-3304560-5.patch854 bytes

We have a different approach/solution for this problem, I have a patch that validates the input on the SearchApiFulltext field and gives an error message when the input is invalid.

recrit’s picture

recrit commented
StatusFileSize
new search_api-3304560-6.patch1.46 KB

@Gertlor your patch #5 limits the coverage to only Search Api queries using Views. I suggest that we combine the 2 patches to also cover sites that may execute Search Api queries without views. The attached patch combines #5 and #2

recrit’s picture

recrit commented
Status: Needs work » Needs review
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Component: General code » Plugins
StatusFileSize
new 3304560-8--keywords_parsing_invalid_utf8--interdiff.txt2.47 KB
new 3304560-8--keywords_parsing_invalid_utf8--tests_only.patch568 bytes
new 3304560-8--keywords_parsing_invalid_utf8.patch2.88 KB

Thanks a lot for reporting this issue, and sorry it took me so long to respond!
I made the two fixes a bit more consistent, by avoiding to call preg_split() with invalid UTF-8 strings in the first place, but also making sure we don’t try to iterate over FALSE even if something slips through that check. I also added test coverage.
Please test/review!

The last submitted patch, 8: 3304560-8--keywords_parsing_invalid_utf8--tests_only.patch, failed testing. View results

  • drunken monkey committed 0273daf0 on 8.x-1.x authored by recrit 
    Issue #3304560 by recrit, Gertlor, drunken monkey: Fixed problems with "...
drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented
Status: Needs review » Fixed

Merged.
Thanks again!

Status: Fixed » Closed (fixed)

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