matomo-org / matomo

Empowering People Ethically with the leading open source alternative to Google Analytics that gives you full control over your data. Matomo lets you easily collect data from websites & apps and visualise this data and extract insights. Privacy is built-in. Liberating Web Analytics. Star us on Github? +1. And we love Pull Requests!
https://matomo.org/
GNU General Public License v3.0
19.67k stars 2.62k forks source link

strtolower(): Argument #1 ($string) must be of type string #19222

Closed JasonMortonNZ closed 1 year ago

JasonMortonNZ commented 2 years ago

The following error has been popping up recently:

Error: {"message":"strtolower(): Argument #1 ($string) must be of type string, array given","file":"\/var\/www\/html\/core\/Date.php","line":1083,"request_id":"7a78b","backtrace":" on \/var\/www\/html\/core\/Date.php(1083)\n#0 \/var\/www\/html\/core\/Date.php(1083): strtolower(Array)\n#1 \/var\/www\/html\/core\/Period\/Range.php(255): Piwik\\Date->addPeriod(-29, Array)\n#2 \/var\/www\/html\/core\/Period.php(131): Piwik\\Period\\Range->generate()\n#3 \/var\/www\/html\/core\/Period\/Range.php(152): Piwik\\Period->getDateStart()\n#4 \/var\/www\/html\/core\/Period\/Range.php(543): Piwik\\Period\\Range->getDateStart()\n#5 \/var\/www\/html\/plugins\/ImageGraph\/ImageGraph.php(91): Piwik\\Period\\Range::getRelativeToEndDate(Array, 'last30', Object(Piwik\\Date), Object(Piwik\\Site))\n#6 [internal function]: Piwik\\Plugins\\ImageGraph\\ImageGraph->getReportMetadata(Array, Array)\n#7 \/var\/www\/html\/core\/EventDispatcher.php(141): call_user_func_array(Array, Array)\n#8 \/var\/www\/html\/core\/Piwik.php(845): Piwik\\EventDispatcher->postEvent('API.getReportMe...', Array, false, Array)\n#9 \/var\/www\/html\/plugins\/API\/ProcessedReport.php(220): Piwik\\Piwik::postEvent('API.getReportMe...', Array)\n#10 \/var\/www\/html\/plugins\/API\/API.php(272): Piwik\\Plugins\\API\\ProcessedReport->getReportMetadata('1', Array, 'yesterday', false, false)\n#11 [internal function]: Piwik\\Plugins\\API\\API->getReportMetadata('1,2', Array, 'yesterday', false, false, '1')\n#12 \/var\/www\/html\/core\/API\/Proxy.php(244): call_user_func_array(Array, Array)\n#13 \/var\/www\/html\/core\/Context.php(28): Piwik\\API\\Proxy->Piwik\\API\\{closure}()\n#14 \/var\/www\/html\/core\/API\/Proxy.php(335): Piwik\\Context::executeWithQueryParameters(Array, Object(Closure))\n#15 \/var\/www\/html\/core\/API\/Request.php(266): Piwik\\API\\Proxy->call('\\\\Piwik\\\\Plugins\\\\...', 'getReportMetada...', Array)\n#16 \/var\/www\/html\/plugins\/API\/Controller.php(45): Piwik\\API\\Request->process()\n#17 [internal function]: Piwik\\Plugins\\API\\Controller->index()\n#18 \/var\/www\/html\/core\/FrontController.php(631): call_user_func_array(Array, Array)\n#19 \/var\/www\/html\/core\/FrontController.php(169): Piwik\\FrontController->doDispatch('API', false, Array)\n#20 \/var\/www\/html\/core\/dispatch.php(32): Piwik\\FrontController->dispatch()\n#21 \/var\/www\/html\/index.php(25): require_once('\/var\/www\/html\/c...')\n#22 {main}","safemode_backtrace":"#0 [internal function]: Piwik\\Plugins\\Cloud\\Controller->safemode(Array)\n#1 \/core\/FrontController.php(631): call_user_func_array(Array, Array)\n#2 \/core\/FrontController.php(169): Piwik\\FrontController->doDispatch('Cloud', 'safemode', Array)\n#3 \/core\/FrontController.php(100): Piwik\\FrontController->dispatch('CorePluginsAdmi...', 'safemode', Array)\n#4 \/core\/FrontController.php(140): Piwik\\FrontController::(Array)\n#5 \/core\/FrontController.php(190): Piwik\\FrontController::(Object(TypeError))\n#6 \/core\/dispatch.php(32): Piwik\\FrontController->dispatch()\n#7 \/index.php(25): require_once('\/c...')\n#8 {main}"}

Steps to Reproduce (for Bugs)

URL: https://matomo.test/index.php?date=yesterday&format=JSON&idSite=1&idSites=1,2&method=API.getReportMetadata&module=API&period[$acunetix]=1&token_auth=XYZANONYMIZED

Referrer:

GET: {"date":"yesterday","format":"JSON","idSite":"1","idSites":"1,2","method":"API.getReportMetadata","module":"API","period":{"$acunetix":"1"},"token_auth":"XYZANONYMIZED","filter_limit":100}

Your Environment

sgiehl commented 2 years ago

This is another issue of "improper" handling of request parameters. In this case period is provided as an array, which we expect to be a string. Personally I would not suggest to fix all such incorrect handling were they occur, as it consumes a lot of time and actually doesn't improve much. The result for the customer will only change from a php error to an unsupported param exception.

We should maybe consider for Matomo 5, to introduce a proper request param handling for API methods. Each api method could define the types of the method parameters using type hints (or maybe doc comments where multiple types are allowed 🤔). Our API request processor could then check if the provided values can be converted/mapped to the defined types. If that isn't the case a general exception like Parameter X expects type Y, but Z provided. could be thrown. Maybe we should create a global issue for that. (ping @justinvelluppillai)

samjf commented 2 years ago

I think a global issue could be good for this. I might be completely out of context here but ideally it would return a 422 code because it is a user correctable error and avoid any exceptions. Would likely need light validations for such a implementation though. I think stopping such invalid input earlier might help with security too. Being able to hit methods such as preg_match etc can provide a vuln sometimes.