Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape quotes in $db->quoteName #13825

Merged
merged 1 commit into from Feb 2, 2017
Merged

Escape quotes in $db->quoteName #13825

merged 1 commit into from Feb 2, 2017

Conversation

csthomas
Copy link
Contributor

@csthomas csthomas commented Jan 31, 2017

Summary of Changes

Escape quote char in $db->quoteName() method.

It can be considered as a little security fix,
but joomla (with correct sanitise aliases/columns) is secure also without this patch.

I based on:

Testing Instructions

Code review or
Create and execute each query for specified database type:

$db = JFactory::getDbo();
$q1 = $db->getQuery(true)->select($db->quoteName('title', 'protected`title'))->from('#__content')->where('id=1');

// Without patch it generate error on mysql but it works on other databases
$db->setQuery($q1)->loadAssoc();
$db = JFactory::getDbo();
$q2 = $db->getQuery(true)->select($db->quoteName('title', 'protected"title'))->from('#__content')->where('id=1');

// Without patch it generate error on postgresql but it works on other databases
$db->setQuery($q2)->loadAssoc();
$db = JFactory::getDbo();
$q3 = $db->getQuery(true)->select($db->quoteName('title', 'protected]title'))->from('#__content')->where('id=1');

// Without patch it generate error on mssql but it works on other databases
$db->setQuery($q3)->loadAssoc();

Expected result - valid queries

  • on MySQL
    • SELECT title AS `protected``title` FROM #__content where id=1
  • on postgreSQL
    • SELECT title AS "protected""title" FROM #__content where id=1
  • on MSSQL
    • SELECT title AS [protected]]title] FROM #__content where id=1

Actual result - invalid queries

  • on MySQL
    • SELECT title AS `protected`title` FROM #__content where id=1
  • on postgreSQL
    • SELECT title AS "protected"title" FROM #__content where id=1
  • on MSSQL
    • SELECT title AS [protected]title] FROM #__content where id=1

Documentation Changes Required

No

@PhilETaylor

This comment was marked as abuse.

@beat
Copy link
Contributor

beat commented Feb 1, 2017

Reviewed source code, and looks ok.
Based on MSSQL official doc at Microsoft, second case seems ok too:
https://technet.microsoft.com/en-us/library/ms176027(v=sql.105).aspx#

@csthomas
Copy link
Contributor Author

csthomas commented Feb 1, 2017

Great, but as usual we need 2 tests, I think that you review can be used as success test.
You can mark it as success at https://issues.joomla.org/tracker/joomla-cms/13825

@marrouchi
Copy link
Contributor

I have tested this item ✅ successfully on 2305cf3

Tested on MySQL.


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

@alikon
Copy link
Contributor

alikon commented Feb 1, 2017

I have tested this item ✅ successfully on 2305cf3

on mssql
on postgresql


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

@wilsonge wilsonge merged commit 6f85b59 into joomla:staging Feb 2, 2017
@wilsonge wilsonge added this to the Joomla 3.7.0 milestone Feb 2, 2017
@wilsonge
Copy link
Contributor

wilsonge commented Feb 2, 2017

With a small degree of trepidation given previous attempts to modify this function. Merged

@csthomas csthomas deleted the secur branch February 2, 2017 14:34
@PhilETaylor

This comment was marked as abuse.

@csthomas
Copy link
Contributor Author

csthomas commented Feb 2, 2017

Then I will add a new PR with tests for quoteName for 3 dbs which will be tested by appveyor.

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.

None yet

7 participants