? Success

User tests: Successful: Unsuccessful:

avatar Devportobello
Devportobello
26 Mar 2015

Concept

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.

Testing instructions

Try to make an SQL error

avatar Devportobello Devportobello - open - 26 Mar 2015
avatar joomla-cms-bot joomla-cms-bot - change - 26 Mar 2015
Labels Added: ?
avatar alikon
alikon - comment - 26 Mar 2015

maybe on a live site sounds good
but on a dev enviroment ?
p.s.
tested on postgresql don't work see the comment

avatar sovainfo
sovainfo - comment - 26 Mar 2015

Suggest to base it on error_reporting. When set to "None" hide the prefix, otherwise report it.

avatar Devportobello
Devportobello - comment - 27 Mar 2015

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...

avatar Bakual
Bakual - comment - 27 Mar 2015

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.

avatar wilsonge
wilsonge - comment - 27 Mar 2015

I don't disagree @Bakual but as we've seen in the 3.4.1 release these things do happen and we've had to patch them as they come. You're right this is a fallback problem. But it is needed imho

avatar sovainfo
sovainfo - comment - 27 Mar 2015

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.

avatar mbabker
mbabker - comment - 27 Mar 2015

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.

avatar Devportobello
Devportobello - comment - 27 Mar 2015

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)

avatar sovainfo
sovainfo - comment - 27 Mar 2015

Obviously the extension providing that bad sql should have a try/catch and not forwarding the error. Or only forwarding depending on error_reporting.

avatar alikon
alikon - comment - 27 Mar 2015

is needed imho mostly for bad coded extensions and yes make it as simple as possibile @Bakual

avatar zero-24 zero-24 - change - 27 Mar 2015
Category Libraries SQL UI/UX
avatar wilsonge
wilsonge - comment - 29 Mar 2015

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)

avatar mbabker
mbabker - comment - 29 Mar 2015

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.

avatar 810
810 - comment - 29 Mar 2015

Parse error: syntax error, unexpected 'version' (T_STRING) in /public_html/libraries/joomla/database/driver/mysqli.php on line 2

avatar Devportobello
Devportobello - comment - 30 Mar 2015

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.

avatar mbabker
mbabker - comment - 30 Mar 2015

So the right fix would be to update the plugin to manage errors correctly, like #6612 does.

avatar Devportobello
Devportobello - comment - 30 Mar 2015

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.

avatar alikon alikon - reference | a137838 - 1 Apr 15
avatar alikon alikon - reference | cf9f71d - 5 Apr 15
avatar alikon alikon - reference | 2792eb5 - 5 Apr 15
avatar alikon alikon - reference | 4bba407 - 6 Apr 15
avatar alikon alikon - reference | c43b313 - 6 Apr 15
avatar alikon alikon - reference | 93904ea - 8 Apr 15
avatar alikon alikon - reference | aa9fd15 - 8 Apr 15
avatar alikon alikon - reference | 55d8945 - 8 Apr 15
avatar alikon alikon - reference | c0f1194 - 8 Apr 15
avatar alikon alikon - reference | 3fa79ac - 8 Apr 15
avatar alikon alikon - reference | 5a3328c - 8 Apr 15
avatar alikon alikon - reference | 7c3e174 - 8 Apr 15
avatar alikon alikon - reference | 7334f30 - 9 Apr 15
avatar alikon alikon - reference | 84c0e0f - 9 Apr 15
avatar alikon alikon - reference | ef433d0 - 9 Apr 15
avatar alikon alikon - reference | cd3c761 - 9 Apr 15
avatar Devportobello
Devportobello - comment - 1 Jun 2015

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');
}
avatar Devportobello Devportobello - change - 1 Jun 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-06-01 15:03:51
Closed_By Devportobello
avatar Devportobello Devportobello - close - 1 Jun 2015
avatar Devportobello Devportobello - close - 1 Jun 2015
avatar Devportobello Devportobello - head_ref_deleted - 26 Oct 2015

Add a Comment

Login with GitHub to post a comment