@tsteur opened this Issue on February 2nd 2022 Member

Error: {"message":"parse_url(): Argument #1 ($url) must be of type string, array given","file":"\/core\/Url.php","line":581,"request_id":"7ba36","backtrace":" on \/core\/Url.php(581)\n#0 \/core\/Url.php(581): parse_url(Array)\n#1 \/plugins\/LoginSaml\/Controller.php(167): Piwik\Url::isLocalUrl(Array)\n#2 [internal function]: Piwik\Plugins\LoginSaml\Controller->singleSignOn()\n#3 \/core\/FrontController.php(619): call_user_func_array(Array, Array)\n#4 \/core\/FrontController.php(168): Piwik\FrontController->doDispatch('LoginSaml', 'singleSignOn', Array)\n#5 \/core\/dispatch.php(32): Piwik\FrontController->dispatch()\n#6

Was initially going to create this issue in LoginSaml. However, was then thinking isLocalUrl shouldn't return true or fail when the parameter is an array and it could just return false.

Happens with PHP 8

Request was like:

index.php?action=singleSignOn&module=LoginSaml&target[%24foo]=1

@sgiehl commented on February 2nd 2022 Member

I actually wouldn't change that in Core. At some point we should consider to use type hints for as many methods as possible. And the method Url::isLocalUrl should only accept a string as parameter. So the right place to fix that is the place where the method is called with a parameter that isn't validated as string before.

@tsteur commented on February 2nd 2022 Member

Wouldn't that create in the end more work for everyone? Like we could simply say for everyone when array given, it is not a local URL. Similarly to when empty, it is not a local URL. It simply simplifies it for everyone. Not sure if enforcing a string for the sake of it is needed?

@sgiehl commented on February 4th 2022 Member

It won't produce more work actually. People should think about which parameters they pass through to certain methods. Using more stricter types makes the code a lot more stable over time. And if we now start implementing more of such workarounds it will later be a lot more work if we try to use more stricter types.
In that special case $_GET['target'] is passed to Url::isLocal. I would assume that call was done by some sort of security researcher, as we internally wouldn't pass such an array. So it would be totally fine to ignore the value if the parameter type doesn't match.
This could be simply achieved by using Common::getRequestVar('target', '', 'string') instead of directly using $_GET['target']

@tsteur commented on February 7th 2022 Member

@sgiehl we can also tweak it in the plugin and adjust it there. However, I still think it's fine/better to fix it in core. Not all code is equal and sometimes it's also good that you can just throw whatever input to a method that validates this input and then you know if it is valid or not. In the end things are easier for the developer because they don't need to worry about it and can do less wrong and it makes the app more stable / produces less fatal errors etc.

@sgiehl commented on February 8th 2022 Member

We definitely need to fix that in the plugin as directly accessing and using request variables for anything should only be done if it's really needed.
And regarding fixing that in core I would still disagree. There is a good reason why PHP started to more and more introduce type hints. A (good) developer should always think about what his variables can contain and what they are used for. And if a method only handles strings, he should take care to only pass through strings.
For sure it seems like it would make it more "easier" for a developer. But actually it more likely encourages developers to think less about their code and thus makes it more unstable. In my opinion it is better to have fatal errors in that case and fix the source of the issue instead of making all methods more loose and unstable in order to hide such coding faults.

Even though it might be a long way to go I would actually like to have strict_types=1 some day in the future.

@tsteur commented on February 10th 2022 Member

Like mentioned in previous comment, we can fix it in the plugin, it's fine. Generally, it's great to when devs don't have to think about the code etc and it just works and not everyone can know everything. When we fix it in the plugin, then I would expect as part of this issue we create a new guide on developer.matomo.org eg under the Develop -> Plugin Basics section on how to handle request parameters. So far we only have a small section in https://developer.matomo.org/guides/controllers#getting-query-parameters

This Issue was closed on February 15th 2022
Powered by GitHub Issue Mirror