Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I got this message after typing in address bar of my browser: example.com/?q[]
Warning: trim() expects parameter 1 to be string, array given in request_path() (line 2436 of .../includes/bootstrap.inc).
Comment | File | Size | Author |
---|---|---|---|
#12 | 1242472-12.patch | 1.25 KB | xjm |
#9 | invalid_type_of_GET_variable-1242472-9.patch | 1.17 KB | galooph |
#7 | invalid_type_of_GET_variable_test-1242472-7.patch | 704 bytes | galooph |
#6 | invalid_type_of_GET_variable_test-1242472-6.patch | 736 bytes | galooph |
#5 | invalid_type_of_GET_variable_test-1242472-5.patch | 674 bytes | galooph |
Comments
Comment #1
StevenPatzComment #2
galooph CreditAttribution: galooph commentedCast $path as a string - $path = trim((string) $path, '/');
Comment #3
catchFix looks right, should be simple enough to add tests for this though.
Comment #4
galooph CreditAttribution: galooph commentedThanks catch.
Just trying to work out where's best to add the test. Should I add another test module in /modules/simpletest/tests or add a test to one of the existing test modules? If the latter, which test module would you recommend I extend?
Comment #5
galooph CreditAttribution: galooph commentedFirst stab at a test. This one should fail, then I'll submit the test together with the fix to see if it passes.
Comment #6
galooph CreditAttribution: galooph commentedRight, worked out how to run tests locally :-)
Next try at a failing test.
Comment #7
galooph CreditAttribution: galooph commentedThird time lucky for a failing test.
Comment #9
galooph CreditAttribution: galooph commentedThe test correctly failed, so now adding in the patch to fix the problem.
Comment #10
Chi CreditAttribution: Chi commentedComment #11
xjmThere's some trailing whitespace in the second hunk, but this looks fine other than that.
Comment #12
xjmAttached patch is a reroll of #9 with the following changes:
Comment #13
xjmOh, and it's got tests, so untagging. :)
Comment #14
galooph CreditAttribution: galooph commentedThanks for the tweaks, xjm - still finding my feet on core patches :-)
Comment #15
Dave ReidShouldn't this be fixed a little farther up in request_path()?
Should maybe have an is_string() check on $_GET['q']. We shouldn't let $_GET['q'] = 'Array' which would happen with the current patch.
Comment #16
xjm@Dave Reid's review is relevant to these related issues as well:
#1242060: Invalid value of page GET variable causes PHP warning
#1242480: Invalid type of $_GET[sort] variable causes PHP warning
#1226480: Invalid search query string causes PHP notice on D7 (warning in D6)
Edit: Basically, we should cast all these values as strings immediately when we assign them from
$_GET
.Comment #17
xjmComment #18
xjmSo we thought #15 might resolve all four of these issues, but I realize it doesn't (the other three tests fail). I do think @Dave Reid is correct that we should fix these issues systematically upstream rather than type-checking and typecasting in the specific places that have been found to throw errors.
I'm going to mark the others as duplicates and look into a more complete solution.
Comment #19
Chi CreditAttribution: Chi commented@xjm There are some reasons against combining these issues raised in that comment.
But since we are looking for a more complete solution I've marked as duplicate
#1242488: Invalid type of $_GET[target] variable causes PHP warning
Also we have rename the issue or file new one.
Comment #20
xjmFor reference, an all-inclusive list of specific
$_GET
parameters used in core. A lot of these should only ever be strings.$_GET['q']
$_GET['destination']
$_GET['language']
or custom value$_GET['page']
$_GET['sort']
$_GET['order']
$_GET['parent']
$_GET['token']
$_GET['weight']
$_GET['render']
$_GET['link']
$_GET['name']
$_GET['trigger_actions_on_watchdog']
$_GET['format']
$_GET['theme']
$_GET['translation']
$_GET['target']
$_GET['pass-reset-token']
$_GET['name']
Comment #21
xjmComment #22
sunGarbage in, garbage out. Unless there's a security issue involved, this won't fix. Fix the calling code instead. Or, as in the case of the OP, enter a proper URL into your browser's address bar.
Comment #23
Dave ReidYep, I'm ok with that.
Comment #24
Chi CreditAttribution: Chi commentedThere is related issue: #1253858: Invalid type of $_POST['foo'] variables causes PHP warnings and notices when treated as strings
Comment #25
galooph CreditAttribution: galooph commentedI've just watched Crell's Core Conversation update on http://groups.drupal.org/wscci and it looks like this will solve the issue.
Comment #26
xjmComment #27
Chi CreditAttribution: Chi commentedThis case is different because we are getting garbage inside.
Reopening, since we have just got a security issue involved and it is being exploited by passing array to the $_GET['q'] variable.
Unfortunately there is no way to make attackers enter a proper URL to the address bar. At present, much of Drupal sites are logging error messages like follows.
Warning: trim() expects parameter 1 to be string, array given в функции user_pass_validate()
Comment #28
Chi CreditAttribution: Chi commentedReverting the original title since for $_POST variables there is a separate issue. #1253858: Invalid type of $_POST['foo'] variables causes PHP warnings and notices when treated as strings
Comment #35
catchThis is being resolved in #3162016: [Symfony 6] Retrieving a non-string value from "Symfony\Component\HttpFoundation\InputBag::get()" is deprecated, marking as duplicate.