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

[RTM] Quote reserved words in database queries #8813

Merged
merged 5 commits into from Jan 19, 2018

Conversation

Projects
None yet
3 participants
@ausi
Member

ausi commented Nov 20, 2017

As discussed in contao/core-bundle#1106 and the Contao Mumble call, we want to escape all column names that are reserved keywords.

In Contao 4 we will use \System::getContainer()->get('connection')->getDatabasePlatform()->quoteSingleIdentifier($name);. In Contao 3.5 (this pull request) we only quote rows.

There is no need to fix the install tool in 3.5, as it uses quoted column names already.

@leofeyer Do you have an automated workflow to port this pull request to Contao 4.4, or should I do it by hand?

@leofeyer leofeyer added the defect label Nov 21, 2017

@leofeyer leofeyer added this to the 3.5.32 milestone Nov 21, 2017

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Nov 21, 2017

Member

I have a semi-automated way (creating split branches and then rebasing), so you don't have to do it by hand. But are the code changes actually the same in Contao 3 and Contao 4?

In Contao 4 we will use \System::getContainer()->get('connection')->getDatabasePlatform()->quoteSingleIdentifier($name);. In Contao 3.5 (this pull request) we only quote rows.

Member

leofeyer commented Nov 21, 2017

I have a semi-automated way (creating split branches and then rebasing), so you don't have to do it by hand. But are the code changes actually the same in Contao 3 and Contao 4?

In Contao 4 we will use \System::getContainer()->get('connection')->getDatabasePlatform()->quoteSingleIdentifier($name);. In Contao 3.5 (this pull request) we only quote rows.

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Nov 21, 2017

Member

But are the code changes actually the same in Contao 3 and Contao 4?

Most of them are the same. The quoteColumnName method has to be different. Can you create a branch in the core-bundle where you merge these changes, and I add the additional modifications for Contao 4?

Member

ausi commented Nov 21, 2017

But are the code changes actually the same in Contao 3 and Contao 4?

Most of them are the same. The quoteColumnName method has to be different. Can you create a branch in the core-bundle where you merge these changes, and I add the additional modifications for Contao 4?

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Nov 21, 2017

Member

Sadly this is not how it works. I can only create the split branches after the PR has been merged into Contao 3.5 and a new version has been tagged. So I guess we have to do it manually. 😢

Member

leofeyer commented Nov 21, 2017

Sadly this is not how it works. I can only create the split branches after the PR has been merged into Contao 3.5 and a new version has been tagged. So I guess we have to do it manually. 😢

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Nov 21, 2017

Member

K, I do it :)

Member

ausi commented Nov 21, 2017

K, I do it :)

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Nov 21, 2017

Member

Thank you very much.

Member

leofeyer commented Nov 21, 2017

Thank you very much.

leofeyer added a commit to contao/core-bundle that referenced this pull request Dec 27, 2017

Quote reserved words in database queries (see #1262)
Description
-----------

This is the port of contao/core#8813.

Also see #1106 and contao/installation-bundle#70

Commits
-------

84c4746 Quote reserved words in database queries
7bf64b1 Use static instead of \Database as self-reference.
@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Dec 27, 2017

Member

Before merging this we should check if this fix needs to be applied here as well (I think it does):

contao/core-bundle@4de1d20

Member

leofeyer commented Dec 27, 2017

Before merging this we should check if this fix needs to be applied here as well (I think it does):

contao/core-bundle@4de1d20

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Dec 27, 2017

Member

Yes, I think so. Needs to be applied in both Mysql.php and Mysqli.php.

Member

ausi commented Dec 27, 2017

Yes, I think so. Needs to be applied in both Mysql.php and Mysqli.php.

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Dec 27, 2017

Member

And this one as well: contao/core-bundle@541a8ca

Member

leofeyer commented Dec 27, 2017

And this one as well: contao/core-bundle@541a8ca

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Dec 28, 2017

Member

Unfortunately, there are even more problems. Sometimes the identifier is not a column but an expression such as CONCAT(firstname, ' ', lastname), which leads to a "Column not found" exception.

Of course we could add something like this:

// Not an identifier
if (!preg_match('/^[a-z0-9_$.]+$/', $strName))
{
	return $strName;
}

However this would leave the column names inside the CONCAT() function unquoted.


EDIT: The regex should be A-Za-z0-9_$. as mentioned below.

Member

leofeyer commented Dec 28, 2017

Unfortunately, there are even more problems. Sometimes the identifier is not a column but an expression such as CONCAT(firstname, ' ', lastname), which leads to a "Column not found" exception.

Of course we could add something like this:

// Not an identifier
if (!preg_match('/^[a-z0-9_$.]+$/', $strName))
{
	return $strName;
}

However this would leave the column names inside the CONCAT() function unquoted.


EDIT: The regex should be A-Za-z0-9_$. as mentioned below.

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Dec 28, 2017

Member

@contao/developers I don't think that quoting identifiers is the correct solution as it leads to unforeseen issues. We have to discuss this topic again.

I am going to revert the changes in Contao 4.4.11 meanwhile.

Member

leofeyer commented Dec 28, 2017

@contao/developers I don't think that quoting identifiers is the correct solution as it leads to unforeseen issues. We have to discuss this topic again.

I am going to revert the changes in Contao 4.4.11 meanwhile.

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Dec 28, 2017

Member

I still think quoting identifiers is the right solution and your fix with the regular expression is correct. Although we should allow uppercase letters too: /^[a-z0-9_$.]+$/i. Is there a specific reason why you included the $ character?

However this would leave the column names inside the CONCAT() function unquoted.

Which is correct IMO. If you as a developer pass such a string to the model you have to do the quoting yourself, because you are passing an SQL expression and not a column name.

Member

ausi commented Dec 28, 2017

I still think quoting identifiers is the right solution and your fix with the regular expression is correct. Although we should allow uppercase letters too: /^[a-z0-9_$.]+$/i. Is there a specific reason why you included the $ character?

However this would leave the column names inside the CONCAT() function unquoted.

Which is correct IMO. If you as a developer pass such a string to the model you have to do the quoting yourself, because you are passing an SQL expression and not a column name.

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Dec 29, 2017

Member

Apparently the dollar sign is a permitted character in an unquoted identifier:

https://dev.mysql.com/doc/refman/5.7/en/identifiers.html

Member

leofeyer commented Dec 29, 2017

Apparently the dollar sign is a permitted character in an unquoted identifier:

https://dev.mysql.com/doc/refman/5.7/en/identifiers.html

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jan 4, 2018

taca
www/contao44: update to 4.4.12
From release announce:

4.4.9 (2017/12/14)

* Fixes several minor PHP 7.2 related issues.


4.4.10 (2017/12/27)

* Fixes a few minor issues including a problem with the comments bundle.


4.4.11 (2017/12/28)

* Reverts the identifier quoting changes.

  MySQL 8 and MariaDB 10.2

    In MySQL 8.0.2 and MariaDB 10.2.7 rows has been added as reserved
    word. Since we are using a field named rows in the tl_layout
    table, Contao is no longer compatible with the mentioned database
    versions.

    To avoid renaming the field in Contao and potentially causing
    theme import issues, we have added identifier quoting in version
    4.4.10(*1). Unfortunately this led to several unforeseen
    problems(*2), so we had to revert the changes in version 4.4.11.

    (*1) contao/core-bundle#1262
    (*2) contao/core#8813 (comment)


4.4.12 (2018/1/3)

* Optimizes adding pages to the search index and fixes a few minor issues.

@leofeyer leofeyer removed this from the 3.5.32 milestone Jan 15, 2018

@leofeyer leofeyer closed this Jan 15, 2018

@leofeyer leofeyer changed the base branch from master to 3.5 Jan 15, 2018

@leofeyer leofeyer reopened this Jan 15, 2018

@DanielSchwiperich

This comment has been minimized.

Show comment
Hide comment
@DanielSchwiperich

DanielSchwiperich Jan 17, 2018

Contributor

@ausi the PR also needs (if get recontinued) to take the creation into account:
https://github.com/madeyourday/contao-core/blob/e33738ab880063afc6453c17305479f58f98cf16/system/modules/core/drivers/DC_Table.php#L688

creating a new layout will otherwise not works because of the rows field in tl_layout
Same for contao/core-bundle#1262 if gets rereverted.

There is also another issue regarding mariadb, will create an issue for it in core-bundle

Contributor

DanielSchwiperich commented Jan 17, 2018

@ausi the PR also needs (if get recontinued) to take the creation into account:
https://github.com/madeyourday/contao-core/blob/e33738ab880063afc6453c17305479f58f98cf16/system/modules/core/drivers/DC_Table.php#L688

creating a new layout will otherwise not works because of the rows field in tl_layout
Same for contao/core-bundle#1262 if gets rereverted.

There is also another issue regarding mariadb, will create an issue for it in core-bundle

@ausi

This comment has been minimized.

Show comment
Hide comment
Member

ausi commented Jan 17, 2018

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Jan 18, 2018

Member

As discussed in Mumble on January 18th, 2018, we want to merge this with the changes from contao/core-bundle@4de1d20 and contao/core-bundle@541a8ca.

Member

leofeyer commented Jan 18, 2018

As discussed in Mumble on January 18th, 2018, we want to merge this with the changes from contao/core-bundle@4de1d20 and contao/core-bundle@541a8ca.

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Jan 18, 2018

Member

And add the regex check /^[a-z0-9_$.]+$/i

Member

ausi commented Jan 18, 2018

And add the regex check /^[a-z0-9_$.]+$/i

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Jan 18, 2018

Member

I looked again and think we must not include the . so the correct regex is: /^[a-z0-9_$]+$/i

Member

ausi commented Jan 18, 2018

I looked again and think we must not include the . so the correct regex is: /^[a-z0-9_$]+$/i

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Jan 18, 2018

Member

What if it is tl_content.singleSRC?

Member

leofeyer commented Jan 18, 2018

What if it is tl_content.singleSRC?

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Jan 18, 2018

Member

Same as with CONCAT(...), the string tl_content.singleSRC is an expression and not a column name. The qouting would have to be done before. Like so 'tl_content.'.\Database::quoteColumnName('singleSRC');

Member

ausi commented Jan 18, 2018

Same as with CONCAT(...), the string tl_content.singleSRC is an expression and not a column name. The qouting would have to be done before. Like so 'tl_content.'.\Database::quoteColumnName('singleSRC');

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Jan 18, 2018

Member

Ok.

I wonder if the method should be called quoteIdentifier() instead of quoteColumnName(). The MySQL manual speaks of "unquoted identifiers" and the Doctrine method is called quoteIdentifier(), too. Any objections if I change it?

Member

leofeyer commented Jan 18, 2018

Ok.

I wonder if the method should be called quoteIdentifier() instead of quoteColumnName(). The MySQL manual speaks of "unquoted identifiers" and the Doctrine method is called quoteIdentifier(), too. Any objections if I change it?

@leofeyer leofeyer added this to the 3.5.33 milestone Jan 18, 2018

@ausi

This comment has been minimized.

Show comment
Hide comment
@ausi

ausi Jan 18, 2018

Member

Any objections if I change it?

No, seems reasonable 👍
(but we should not forget to rename it in the Contao 4 pull request too)

Member

ausi commented Jan 18, 2018

Any objections if I change it?

No, seems reasonable 👍
(but we should not forget to rename it in the Contao 4 pull request too)

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Jan 19, 2018

Member
		// The identifier is quoted already
		if (strncmp($strName, '`', 1) === 0)
		{
			return $strName;
		}

		// Not an identifier
		if (!preg_match('/^[A-Za-z0-9_$]+$/', $strName))
		{
			return $strName;
		}

This is actually only required in Contao 4.4 as we are only quoting rows in Contao 3.5.

Member

leofeyer commented Jan 19, 2018

		// The identifier is quoted already
		if (strncmp($strName, '`', 1) === 0)
		{
			return $strName;
		}

		// Not an identifier
		if (!preg_match('/^[A-Za-z0-9_$]+$/', $strName))
		{
			return $strName;
		}

This is actually only required in Contao 4.4 as we are only quoting rows in Contao 3.5.

@leofeyer leofeyer merged commit db8de81 into contao:3.5 Jan 19, 2018

@leofeyer

This comment has been minimized.

Show comment
Hide comment
@leofeyer

leofeyer Jan 19, 2018

Member

Thank you @ausi.

Member

leofeyer commented Jan 19, 2018

Thank you @ausi.

@@ -95,6 +95,8 @@ protected function get_error()
*/
protected function find_in_set($strKey, $varSet, $blnIsField=false)
{
$strKey = static::quoteIdentifier($strKey);
if ($blnIsField)
{
return "FIND_IN_SET(" . $strKey . ", " . $varSet . ")";

This comment has been minimized.

@ausi

ausi Jan 19, 2018

Member

In this case we should probably use "FIND_IN_SET(" . $strKey . ", " . static::quoteIdentifier($varSet) . ")"; because $varSet is then an identifier too.

@ausi

ausi Jan 19, 2018

Member

In this case we should probably use "FIND_IN_SET(" . $strKey . ", " . static::quoteIdentifier($varSet) . ")"; because $varSet is then an identifier too.

@@ -85,6 +85,8 @@ protected function get_error()
*/
protected function find_in_set($strKey, $varSet, $blnIsField=false)
{
$strKey = static::quoteIdentifier($strKey);
if ($blnIsField)
{
return "FIND_IN_SET(" . $strKey . ", " . $varSet . ")";

This comment has been minimized.

@ausi

ausi Jan 19, 2018

Member

Same here.

@ausi

ausi Jan 19, 2018

Member

Same here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment