phpmyadmin / error-reporting-server

phpMyAdmin server side component for the error reporting system
MIT License
19 stars 28 forks source link

Too long phpmyadmin version #141

Closed nijel closed 7 years ago

nijel commented 7 years ago
2017-05-09 08:15:42 Error: [PDOException] SQLSTATE[22001]: String data, right truncated: 1406 Data too long for column 'pma_version' at row 1 in /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Database/Statement/MysqlStatement.php on line 39
Request URL: /incidents/create
Client IP: [hidden]
Stack Trace:
#0 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Database/Statement/MysqlStatement.php(39): PDOStatement->execute(NULL)
#1 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Database/Connection.php(313): Cake\Database\Statement\MysqlStatement->execute()
#2 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Database/Query.php(213): Cake\Database\Connection->run(Object(Cake\ORM\Query))
#3 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/ORM/Table.php(1916): Cake\Database\Query->execute()
#4 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/ORM/Table.php(1810): Cake\ORM\Table->_insert(Object(Cake\ORM\Entity), Array)
#5 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/ORM/Table.php(1723): Cake\ORM\Table->_processSave(Object(Cake\ORM\Entity), Object(ArrayObject))
#6 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/ORM/Table.php(1446): Cake\ORM\Table->Cake\ORM\{closure}()
#7 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Database/Connection.php(680): Cake\ORM\Table->Cake\ORM\{closure}(Object(Cake\Database\Connection))
#8 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/ORM/Table.php(1447): Cake\Database\Connection->transactional(Object(Closure))
#9 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/ORM/Table.php(1724): Cake\ORM\Table->_executeTransaction(Object(Closure), true)
#10 /home/reports/error-reporting-server/src/Model/Table/IncidentsTable.php(187): Cake\ORM\Table->save(Object(Cake\ORM\Entity))
#11 /home/reports/error-reporting-server/src/Controller/IncidentsController.php(34): App\Model\Table\IncidentsTable->createIncidentFromBugReport(Array)
#12 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Controller/Controller.php(440): App\Controller\IncidentsController->create()
#13 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Http/ActionDispatcher.php(119): Cake\Controller\Controller->invokeAction()
#14 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Http/ActionDispatcher.php(93): Cake\Http\ActionDispatcher->_invoke(Object(App\Controller\IncidentsController))
#15 /home/reports/error-reporting-server/vendor/cakephp/cakephp/src/Routing/Dispatcher.php(60): Cake\Http\ActionDispatcher->dispatch(Object(Cake\Http\ServerRequest), Object(Cake\Http\Response))
#16 /home/reports/error-reporting-server/webroot/index.php(36): Cake\Routing\Dispatcher->dispatch(Object(Cake\Http\ServerRequest), Object(Cake\Http\Response))
#17 {main}
devenbansod commented 7 years ago

For this (and below mentioned ones), we can come up with a database migration (involving an ALTER TABLE ALTER COLUMN column_name ... statement).

Any suggestions welcome.

nijel commented 7 years ago

The question is whether this is desired or we should enforce some kind of validation. For example (here) the version probably could be sanitized to strip distro suffixes and include only information we really care about (see https://github.com/phpmyadmin/error-reporting-server/issues/102, though it mentioned sanitizing at rendering charts, but maybe it's better to do that earlier).

devenbansod commented 7 years ago

Okay. Yes, that sounds good. We can strip the phpMyAdmin versions to just have numeric versions (example regex code at http://ideone.com/pisa9Z) while saving to the database (or should we do the change in phpMyAdmin while sending the error-report itself ?) and maybe while displaying the stats too.

But I am really not very sure how to handle the other truncations with respect to error message.

Also, I just realized that the the fields stacktrace and full_report are already text (which has a limit of 65,535 bytes). How frequent do such long reports truncation occurs? If they are very frequent, may be we can try and reduce some unused information (if there is any), while sending it from phpMyAdmin itself.

nijel commented 7 years ago

Changing it at phpMyAdmin side is not really needed, we need to validate the data at the error reporting server anyway, so I think it's best to do the cleanup there. It might be good idea to migrate existing data as well to have consistent data in the database and then no conversion on displaying stats would be needed.

As for the other fields, the most errors seem to happen for the error_message, see attached log. I'm not really sure if the truncations are valid data, but limiting stacktrace and full report to 64k seems reasonable, so I' just truncate these strings at PHP level to avoid error from MySQL.

truncations.txt

devenbansod commented 7 years ago

About the error_message, should we put debug logs before saving the incident to see what type of error messages are creating this truncations? If all these messages are just exceeding the current limit of 100, we can adjust the limit by a bit. Otherwise, we can truncate all error_message content to 100 characters before saving.

I think we use json_encode while saving and json_decode with stacktrace and full_report, while rendering them on view. So, if we truncate their content to 64k and it does not form a valid JSON, we would have to separately handle those errors as well. Any suggestions for a better approach?

nijel commented 7 years ago

I think we can start with logging and looking at the problematic reports and then decide what to do with them. So I'd suggest to check length of the field before saving and if that does not fit, log the content and do not save the report. We can then review the logs and see if those reports are valid and worth addressing, they are result of somebody inserting false data or the request is result of bug in client.