User tests: Successful: Unsuccessful:
If an SQL error occur, database driver throw a runtimeException containg the SQL with prefix replaced by table prefix.
[code] [error] SQL=[SQL]
eg:
1062 Duplicate entry '1' for key 1 SQL=INSERT INTO 'mysecretdatabaseprefix_table' SET 'key' = '1'
This fix show in exception message the sql before the string identifier was replaced.
eg:
1062 Duplicate entry '1' for key 1 SQL=INSERT INTO '#__table' SET 'key' = '1'
I think this is a tiny security improvment, by that we can't obtain the secret prefix.
Unfortunately, im unable to test on many drivers, moreover, PDO handle the error differently so i actually not modified this driver.
Try to make an SQL error
Labels |
Added:
?
|
Suggest to base it on error_reporting. When set to "None" hide the prefix, otherwise report it.
Thanks for feedback, i will update later.
Wich is better? check JConfig->error_reporting == 'none or 0'
or native error_reporting == 0
Moreover, there is some way where we can obtain some more info about database + prefix before the printed SQL " SQL=" eg:
1146 La table 'joomla_3_4.yh4ed_test' n'existe pas SQL= SELECT * FROM #__test
Maybe better to parse the final exception message...
I don't think it's wise to depend on the error_reporting for this. That would a unique use of this setting in the CMS. If you want to make it depend on a setting, then use debug.
But then, why do we need the prefix in the error message to begin with? Shouldn't you know it anyway?
Imho, there is something wrong in the code if you ever see an SQL query error message. We should catch those and show a human readable error message.
This PR isn't wrong, but it should only be the fallback in case the code fails to catch the error. Lets not make it more complex than really needed.I don't think it's wise to depend on the error_reporting for this. That would a unique use of this setting in the CMS. If you want to make it depend on a setting, then use debug.
But then, why do we need the prefix in the error message to begin with? Shouldn't you know it anyway?
Imho, there is something wrong in the code if you ever see an SQL query error message. We should catch those and show a human readable error message.
This PR isn't wrong, but it should only be the fallback in case the code fails to catch the error. Lets not make it more complex than really needed.
Disagree on abusing DEBUG for something that it wasn't meant to do. Error reporting is what was meant for this. That is why there are settings like None, Minimum, Maximum and Developer.
Agree that when set to None this error should not appear. Only the useless application error should be shown if at all. That is up to the application. It should be decided at which level of reporting the database error should appear and how. At minimum you could obfuscate as much as you like. At maximum expect it to report something usefull. You might even want to introduce something for the developer level.
Also agree on the poor implementation of error handling. That shouldn't stop you from doing the right thing.
My two cents...
Errors like database issues should never be reaching our global exception handler. All database activity should be wrapped in try/catch blocks and the caller should be managing the error, including deciding if the error is fatal or something that can be continued past with only a warning or something if necessary.
Our issues are that there are a lot of places where we either do not catch exceptions thrown in error conditions or we are setting the exact message of the exception to the error handler to display.
Personally, I disagree with coupling the database package to any aspect of the CMS configuration, especially as this creates long term ramifications should we reach a point where the CMS is consuming the Framework's database code which does not have this coupling. I think I also disagree with thrown exceptions having the generic prefix; an Exception is a developer tool to manage errors and the error messages should really only reach the end user as a last option, which is what our global handler acts as. I'd rather see some focus on cleaning up the application's error handling; ensuring that human friendly messages are always reaching the end user and doing more to ensure we don't have error conditions going uncaught.
Tell me if i'm on the wrong way but actually -from your idea- if i want to obfuscate the database prefix from an error who could occur anywhere, i need to try/catch any Exception throwed by database but how?
My PR come from i already seen en error like this: 1062 Duplicate entry 'x1' for key 2 SQL=INSERT INTO 'secretprefix_user_keys' SET 'user_id' = 'x2', 'series' = 'x1', 'uastring'x3', 'time' = x4, 'token' = 'x5'
(not the subject was J!3.2.7)
Obviously the extension providing that bad sql should have a try/catch and not forwarding the error. Or only forwarding depending on error_reporting.
Category | ⇒ | Libraries SQL UI/UX |
Yah. There are some situations like this where I think it's better we work around crap code in 3rd Party Extensions (although of course we should encourage them as much as possible not to do this stuff)
Truthfully, I don't think it's the core platform's responsibility to be playing cleanup around poorly coded extensions. In this example, what are the practical benefits, beyond masking the database table prefix because someone can't figure out how to manage errors in their code, to altering the error output based on the JDEBUG flag or error reporting is set to a predefined level?
Anything that executes a database connection should be wrapped in a try/catch (this includes execute, loadResult, loadObject, etc.) as a failed query will throw an exception containing details about the error (including the platform's error message and the query which caused the failure). These are details that the end user doesn't need to know nor should they be displayed; the exception's data is a developer's debugging tool. The caller should be catching exceptions and deciding how to proceed, and if that includes displaying a message to the user, they should be providing a proper message ("Could not perform function X due to an error" for example).
I'm also going to say in case someone gets the idea that no, we shouldn't change the exception to throw a generic and meaningless message. Now as a developer I have to jump through hoops to get useful debugging information.
Parse error: syntax error, unexpected 'version' (T_STRING) in /public_html/libraries/joomla/database/driver/mysqli.php on line 2
I'm not talking about 3rd Party Extensions, #issuecomment-86971135 was throwed from J! (seem cookie plugin - hijaking cookie identifier), so if someone could lead to sql error, he could know some database informations.
Well, i just wanted to got some feedback about if we need to obfuscate sql query on Exception.
The possibility to know some database info like prefix, database name when SQL error occur and if we need to obfuscate those informations, for me its a tiny security improvment.
Great job about plugin fix.
Well, this PR is not the goal wanted, closing.
Just in case, here is the right way, (handle the error in your script) #6612
... query ...
try
{
$list = $db->loadObjectList();
}
catch (RuntimeException $e)
{
$list = array();
JFactory::getApplication()->enqueueMessage(JText::_('JLIB_DATABASE_GENERIC_SQL_ERROR'), 'error');
JLog::add($e->getMessage(), JLog::ERROR, 'controller');
}
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-06-01 15:03:51 |
Closed_By | ⇒ | Devportobello |
maybe on a live site sounds good
but on a dev enviroment ?
p.s.
tested on postgresql don't work see the comment