Problem/Motivation

search.module supports passing search keywords on the URL as GET arguments, and assumes these arguments to be strings. However, arrays can be passed to php scripts through get or post method as well as strings; so we need to check the type of the variable that comes from the user submission.

For example:
This message is given after typing the following in the address bar of the browser: example.com/search/node/?keys[]
Notice: Array to string conversion in search_view() (line 23 of ...\modules\search\search.pages.inc).

In Drupal 6 the message given is this:
warning: preg_match() expects parameter 2 to be string, array given in ...\modules\search\search.module on line 672.

Proposed resolution

Check for string format before operating on the search terms. Patch in #7 implements this fix for D7 and D8.

Remaining tasks

  1. Patch in #7 is RTBC.
  2. Patch needs backport to D6 and D7.

User interface changes

None.

API changes

None.

Original report by Chi

Arrays can be passed to php script through get or post method as well as string. So we need check type of the variable that comes from user and contains a search query.

I got this message after typing in address bar my browser: example.com/search/node/?keys[]
Notice: Array to string conversion in search_view() (line 23 of ...\modules\search\search.pages.inc).

In Drupal 6 this message looks as
warning: preg_match() expects parameter 2 to be string, array given in ...\modules\search\search.module on line 672.

CommentFileSizeAuthor
#7 check_search_query-1226480-2.patch2.26 KBchi
#4 test_only.patch1.7 KBcatch
#1 heck_search_query-1226480-0.patch580 byteschi

Comments

chi’s picture

chi commented
Priority: Normal » Major
Status: Active » Needs review
StatusFileSize
new heck_search_query-1226480-0.patch580 bytes

I believe it has major priority because any user can discover an internal path to site installation.

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: +Needs tests

Patch looks fine, do you think you could extend the existing tests or write a new one in search.test to cover this?

chi’s picture

chi commented

No, I can't. I haven't enough experience with drupal automated testing system.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new test_only.patch1.7 KB

OK, I wrote the test, thanks for the clear steps to reproduce. It's worth getting the hang of simpletest - trying to patch core tests is easier since they usually have some of the heavy lifting done for you (fortunately that was the case with this one so it's quite a short addition).

Uploading a patch with just the test - this should fail the test bot with an exception. After that fails please upload a combined patch with both the test and the fix.

catch’s picture

catch
he/him
Primary language English
commented
Title: Check search query for arrays » Invalid search query string causes PHP notice on D7 (warning in D6)
Issue tags: -Needs tests

Status: Needs review » Needs work

The last submitted patch, test_only.patch, failed testing.

chi’s picture

chi commented
Status: Needs work » Needs review
StatusFileSize
new check_search_query-1226480-2.patch2.26 KB
catch’s picture

catch
he/him
Primary language English
commented

What happens if you search for a number? Should it be is_scalar() or !is_array() rather than is_string()?

chi’s picture

chi commented

It should be is_string() or !is_array() but not is_scalar() . Because type of $_REQUST['keys'] can be only string or array.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs issue summary update

Tagging issues not yet using summary template.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

#9 is right, I wrote the test but haven't touched the actual bug fix, so marking RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes

grammar fix

mrmofo’s picture

mrmofo commented
Issue summary: View changes

Issue summary using template (by total newb).

mrmofo’s picture

mrmofo commented
Issue summary: View changes

Edit "API changes" to "none" and included Chi's original report.

xjm’s picture

xjm
she/her
Primary language English
commented

Issue summary added by mrmofo.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs issue summary update

See: #1242472-15: Invalid type of $_GET variables causes PHP warnings and notices when treated as strings .

Edit: Basically, we should cast all these values as strings immediately when we assign them from $_GET.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: Unassigned » xjm
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Closed (duplicate)

Marking as duplicate of #1242472: Invalid type of $_GET variables causes PHP warnings and notices when treated as strings to work on a more global solution.

xjm’s picture

xjm
she/her
Primary language English
commented
Assigned: xjm » Unassigned
clintthayer’s picture

clintthayer commented

Hey folks,

Sorry to comment on a closed issue but I'm trying to track down if there was a backport fix for D6 on this issue. Looked in the dup thread mentioned #15 and not seeing anything.

Any help would be great - thanks,
c

clintthayer’s picture

clintthayer commented

Nevermind... I believe this is my issue.

c

clintthayer’s picture

clintthayer commented

Sorry - forgot to add the link: http://drupal.org/node/1446372

clintthayer’s picture

clintthayer commented
Issue summary: View changes

Updated issue summary.