? Success

User tests: Successful: Unsuccessful:

avatar zero-24
zero-24
23 Oct 2015

This PR implements the hide of the SQL Table Prefix from error messages if the webseite is not in debugging mode.

In debugging mode we get the full error message.

How to test

  • produce a SQL Error message in the frontend
  • see that the prefixes get shown
  • apply this patch
  • see the prefixes are replaced with #__
  • enable debug for your user
  • see the prefixes are back

This implements the good idea by @ryandemmer from here: #8129 Thanks.

avatar zero-24 zero-24 - open - 23 Oct 2015
avatar zero-24 zero-24 - change - 23 Oct 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 23 Oct 2015
Labels Added: ?
avatar phproberto
phproberto - comment - 23 Oct 2015

I think is better that you have reusable functions like showErrorMessage(). It will avoid duplicated code and allow us to centralise error messages control there.

avatar zero-24
zero-24 - comment - 23 Oct 2015

@phproberto what do you think about $this->getErrorNum (returns the error num) and $this->getErrorMsg which returns the cleaned up error message?


protected function getErrorNum()
{
    return (int) mysql_errno($this->connection);
}

protected function getErrorMsg()
{
    $errorMessage = (string) mysql_error($this->connection);

    // Replace the Databaseprefix with `#__` if we are not in Debug
    if (!$this->debug)
    {
        $errorMessage = str_replace($this->tablePrefix, '#__', $errorMessage);
    }

    return $errorMessage . ' SQL=' . $this->query;
}

// Get the error number and message before we execute any more queries.
$errorNum = $this->getErrorNum();
$errorMsg = $this->getErrorMsg();
avatar phproberto
phproberto - comment - 23 Oct 2015

@zero-24 those $this->getErrorNum() & $this->getErrorMsg() functions are perfect :+1:

avatar zero-24
zero-24 - comment - 23 Oct 2015

Just done @phproberto Thanks for your tip can you review?

avatar zero-24 zero-24 - change - 23 Oct 2015
Category Libraries MS SQL Postgresql SQL
avatar zero-24 zero-24 - change - 23 Oct 2015
Easy No Yes
avatar infograf768
infograf768 - comment - 24 Oct 2015

Could you post easy ways to get different error messages?

avatar infograf768
infograf768 - comment - 24 Oct 2015

Found. Just modified some queries in the model.
Getting now the Error fine (test on msqli).
Just a remark:
When debug is off no prefix is displayed at all = fine.
When debug is on, it only partly works:
If a table does not exist in the query, prefix is displayed only for the table concerned, example when debug is on, the prefix is displayed, but only in
exception 'RuntimeException' with message 'Table 'master.jos_contento' doesn't exist SQL=
in the remaining SQL part the prefix is absent
[...] FROM #__contento AS a LEFT JOIN #__categories [...]

If the message does not concern a non-existing table, then the prefix is always absent.

avatar PhocaCz PhocaCz - test_item - 24 Oct 2015 - Tested successfully
avatar PhocaCz PhocaCz - test_item - 24 Oct 2015 - Tested successfully
avatar PhocaCz
PhocaCz - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on ffb8ef8

Hi, tested and the patch works OK for me.

Jan


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar coolman01 coolman01 - test_item - 24 Oct 2015 - Tested successfully
avatar coolman01
coolman01 - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on ffb8ef8

Works as described. Good call, hiding that bit of information.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar Bakual
Bakual - comment - 24 Oct 2015

@zero-24 Can you have a look at the comment from JM?

avatar pos-it pos-it - test_item - 24 Oct 2015 - Tested unsuccessfully
avatar pos-it pos-it - test_item - 24 Oct 2015 - Tested unsuccessfully
avatar sudo-web
sudo-web - comment - 24 Oct 2015

I have tested this item :red_circle: unsuccessfully on ffb8ef8

Only half of the Test was Successfully:
In Normale Mode the all prefixes been replaced with #__
in debug mode I would expect that I see all prefixes again - that was not the case:
expact Table 'db_name.tableprefix_bannerse' doesn't exist SQL=UPDATE tableprefix_bannerse SET clicks = (clicks + 1) WHERE id = 1
got Table 'db_name.tableprefix_bannerse' doesn't exist SQL= UPDATE #__bannerse SET clicks = (clicks + 1) WHERE id = 1


Testet on Joomla! 3.4.5 changed line 43 in components/com_banners/models/banner.php from ->update('#__banners') to ->update('#__bannerse')


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar zero-24
zero-24 - comment - 24 Oct 2015

@infograf768 @Bakual @sudo-web @coolman01, @PhocaCz, @pos-it please retest.

Now it should work if you enable the Debug the raw SQL should shown :smiley:

avatar sudo-web sudo-web - test_item - 24 Oct 2015 - Tested successfully
avatar sudo-web
sudo-web - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on ffb8ef8

Tested on two code places again.
Now it works as expectet! :smiley:


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar sudo-web
sudo-web - comment - 24 Oct 2015

One Additional Remark....
It hast not directly to do with this patch but in some scenarios the error output included the full name of the database.
Table 'db_name.#__bannerse' doesn't exist...
I would say that is a kind of security issue.
Should I open a new Issue for that?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar zero-24
zero-24 - comment - 24 Oct 2015

hmm you can't do anything with that information. For sure we can filter it out but I'm not sure if it makes sense.

All actions Joomla does goes to that database if you know the databse name or not. The prefix is that generic you need to access the tables as that should be unique. Maybe @Bakual can give a option here on filter it out or not ;)

avatar joomla-cms-bot
joomla-cms-bot - comment - 24 Oct 2015

This PR has received new commits.

CC: @coolman01, @PhocaCz, @pos-it, @sudo-web


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar zero-24 zero-24 - alter_testresult - 24 Oct 2015 - sudo-web: Tested successfully
avatar zero-24
zero-24 - comment - 24 Oct 2015

@sudo-web i have just fixed Travis and alterd your test reusult.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar joomla-cms-bot
joomla-cms-bot - comment - 24 Oct 2015

This PR has received new commits.

CC: @coolman01, @PhocaCz, @pos-it, @sudo-web


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar zero-24 zero-24 - alter_testresult - 24 Oct 2015 - PhocaCz: Not tested
avatar zero-24 zero-24 - alter_testresult - 24 Oct 2015 - coolman01: Not tested
avatar zero-24 zero-24 - alter_testresult - 24 Oct 2015 - pos-it: Not tested
avatar zero-24
zero-24 - comment - 24 Oct 2015

Fixed it up again :smiley:


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar infograf768
infograf768 - comment - 25 Oct 2015

@zero-24
Although I do not really think we need to get the prefix in debug mode, I managed to get it by changing the method to:

    protected function getErrorMessage()
    {
        $errorMessage = (string) mysqli_error($this->connection);

        // Replace the Databaseprefix with `#__` if we are not in Debug
        if (!$this->debug)
        {
            $errorMessage = str_replace($this->tablePrefix, '#__', $errorMessage);
        }
        else
        {
            $this->sql = str_replace('#__', $this->tablePrefix, $this->sql);
        }

        return $errorMessage . ' SQL=' . $this->sql;
    }
avatar infograf768
infograf768 - comment - 25 Oct 2015

My last post above was done before I saw the changes in the PR. Will test again the PR as is.

avatar infograf768
infograf768 - comment - 25 Oct 2015

tested PR and all works ok now in debug mode

avatar zero-24 zero-24 - alter_testresult - 25 Oct 2015 - infograf768: Tested successfully
avatar zero-24 zero-24 - change - 25 Oct 2015
Milestone Added:
Status Pending Ready to Commit
avatar zero-24 zero-24 - change - 25 Oct 2015
Milestone Added:
avatar zero-24
zero-24 - comment - 25 Oct 2015

Thanks setting RTC for 3.4.6

If You think we should filter the database out let me know. Than i can open a new PR that implement that :)


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/8130.

avatar joomla-cms-bot joomla-cms-bot - change - 25 Oct 2015
Labels Added: ?
avatar rdeutz rdeutz - change - 25 Oct 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-10-25 10:30:37
Closed_By rdeutz
avatar rdeutz rdeutz - close - 25 Oct 2015
avatar joomla-cms-bot joomla-cms-bot - close - 25 Oct 2015
avatar rdeutz rdeutz - reference | 42fd6c7 - 25 Oct 15
avatar rdeutz rdeutz - merge - 25 Oct 2015
avatar rdeutz rdeutz - close - 25 Oct 2015
avatar joomla-cms-bot joomla-cms-bot - change - 25 Oct 2015
Labels Removed: ?
avatar zero-24 zero-24 - head_ref_deleted - 25 Oct 2015
avatar Bakual
Bakual - comment - 25 Oct 2015

If You think we should filter the database out let me know.

We don't need that. An attacker doesn't need to know that anyway as Joomla will query that database by default anyway. So attackers just don't care as they want to query the default database anyway.

avatar zero-24 zero-24 - change - 28 Oct 2015
Milestone
avatar zero-24 zero-24 - change - 28 Oct 2015
Milestone Added:
avatar zero-24 zero-24 - change - 28 Oct 2015
Milestone Added:
avatar zero-24 zero-24 - change - 28 Oct 2015
Milestone

Add a Comment

Login with GitHub to post a comment