Skip to content

Conversation

@mbabker
Copy link
Contributor

@mbabker mbabker commented Dec 23, 2016

Summary of Changes

Including the SQL query in the Exception message is a mild information disclosure in that it displays to the user the failed SQL query and exposes information about the database structure. This PR removes the query from the Exception message retaining only the engine's error message.

The JDatabaseExceptionExecuting object has a $query property (accessible via getQuery()) that contains the failed SQL query. For debugging purposes, if you need access to the failed query, you should extract it from the Exception's property versus relying on the message.

Testing Instructions

Create a query failure that triggers the error page. Pre-patch, the error message will contain the query. Post-patch, it will not.

Documentation Changes Required

Note that the query is not exposed as part of the Exception message any longer, developers must read it from the JDatabaseExceptionExecuting object's $query property.

@fastslack
Copy link
Contributor

fastslack commented Dec 23, 2016

@test I used a cli script to test this and works as expected using MySQL database.

Executed script before patch:

$ ./Issue13356
Unknown column 'noexists' in 'where clause' SQL=SELECT * FROM #__users WHERE noexists = 0

Executed script after patch:

$ ./Issue13356
Unknown column 'noexists' in 'where clause'

Script used here: https://github.com/fastslack/joomla-cli-tools/blob/master/JoomlaTests/issue-13356/Issue13356

@bembelimen
Copy link
Contributor

I have tested this item ✅ successfully on 65ea823


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

* @since 3.4.6
*/
protected function getErrorMessage($query)
protected function getErrorMessage()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alikon
Copy link
Contributor

alikon commented Dec 24, 2016

same as #10949

@mbabker
Copy link
Contributor Author

mbabker commented Dec 30, 2016

Didn't remember that was there. Either way the change needs to be merged in sooner than later. I don't know why there's a bad test on that other PR because there is no code change to duplicate the error output.

@ralain
Copy link
Contributor

ralain commented Dec 30, 2016

I have tested this item ✅ successfully on 5386864


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

@alikon
Copy link
Contributor

alikon commented Dec 30, 2016

fully agree, i close mine in favour of this one.

p.s.
the bad test on the other one should be because of the tested query was executed twice

@alikon
Copy link
Contributor

alikon commented Dec 30, 2016

I have tested this item ✅ successfully on 5386864


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

@alikon alikon mentioned this pull request Dec 30, 2016
@jeckodevelopment
Copy link
Member

RTC


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 30, 2016
@jeckodevelopment jeckodevelopment added this to the Joomla 3.7.0 milestone Dec 30, 2016
@rdeutz rdeutz merged commit 4ca1d56 into joomla:staging Jan 2, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants