PREG_QUOTE and SQL String Binding

[JakeAi]

The function \CodeIgniter\Database\Query matchNamedBinds() uses preg_quote to escape some characters. This is a hindrance with SQLSRV as the backslashes are taken literally. Is there a way to override this function in the driver since it's being loaded as a new query in many base functions?

Adding the line (below) fixes the issue.

$escapedValue = str_replace('\\-', '-', $escapedValue);

protected function matchNamedBinds(string $sql, array $binds)
	{
		foreach ($binds as $placeholder => $value)
		{
			$escapedValue = $this->db->escape($value);
			// In order to correctly handle backlashes in saved strings
			// we will need to preg_quote, so remove the wrapping escape characters
			// otherwise it will get escaped.
			if (is_array($value))
			{
				foreach ($value as &$item)
				{
					$item = preg_quote($item, '|');
				}
				$escapedValue = '(' . implode(',', $escapedValue) . ')';
			}
			else
			{
				$escapedValue = preg_quote(trim($escapedValue, $this->db->escapeChar), '|');
			}
			// preg_quoting can cause issues with some characters in the final query,
			// but NOT preg_quoting causes other characters to be intepreted, like $.
			$escapedValue = str_replace('\\.', '.', $escapedValue);

			//THIS LINE FIXES IT
			$escapedValue = str_replace('\\-', '-', $escapedValue);
			//THIS LINE FIXES IT

			$sql = preg_replace('|:' . $placeholder . '(?!\w)|', $escapedValue, $sql);
		}
		return $sql;
	}

[JakeAi]

There is a line that removes \. and replaces with ".". Why not get the preg_quote char list, and remove hyphens and periods and use preg_replace?? Proposed change below. OR, make it overwritable.

$escapedValue = preg_replace('/([+*?\[^\]$(){}=!<>|:])/', "\\\\$1",trim( $escapedValue,$this->db->escapeChar));

	protected function matchNamedBinds(string $sql, array $binds)
	{
		foreach ($binds as $placeholder => $value)
		{
			$escapedValue = $this->db->escape($value);

			// In order to correctly handle backlashes in saved strings
			// we will need to preg_quote, so remove the wrapping escape characters
			// otherwise it will get escaped.
			if (is_array($value))
			{
				foreach ($value as &$item)
				{
					$item = preg_quote($item, '|');
				}

				$escapedValue = '(' . implode(',', $escapedValue) . ')';
			}
			else
			{
				$escapedValue = preg_replace('/([+*?\[^\]$(){}=!<>|:])/', "\\\\$1",trim( $escapedValue,$this->db->escapeChar));
			}


			$sql = preg_replace('|:' . $placeholder . '(?!\w)|', $escapedValue, $sql);
		}

		return $sql;
	}

[lonnieezell]

I have no problem with the second solution. If you wanted to make a PR for it that would be great. What issues were you running into, exactly, with the SQLSRV syntax? I'm not familiar enough with that variant.

[JakeAi]

By default the database driver was escaping hyphens. When we use part numbers it would do 006038\-8.000. SQL Server takes this literally and doesn't need escaped. Everything seems to work on my end. But I haven't done any extreme testing yet. Here are the lines I changed plus my driver. I won't create a PR until I test it more.

https://github.com/JakeAi/MinnichRW/blob/master/system/Database/BaseConnection.php#L244
https://github.com/JakeAi/MinnichRW/blob/master/system/Database/Query.php#L426
https://github.com/JakeAi/MinnichRW/tree/master/system/Database/SQLSRVNEW

[lonnieezell]

I'm definitely looking forward to the PR. I haven't dug back through the logic enough yet, but why would the BaseConnection's escapeChar list need to change? Seems like overriding that in SQLSRV's Connection class would handle what was needed?

[JakeAi]

Not sure entirely, as I am still trying to learn all of the logic. I am trying to make it change in the driver as much as possible but the function matchNamedBinds (in query.php) keeps escaping characters, and they won't work in SQL Server. The \- is interpreted as literal and not escaping. Unless you know where I can override this in a driver, I would need to change system files.
"o1"."ItemCode" LIKE '%006533\-0.750%' ESCAPE '!'

[lonnieezell]

I just pushed a change that seems to fix things for both MySQL and PostgreSQL though it's far from ideal. I think this might help your issue out, also.

[JakeAi]

Seems to work for me. I have also noticed that the function _escapeString() uses remove_invisible_characters(). In our part numbers, they start with 0's quite often. This function removes those, so I have an override of (below) in my driver.

protected function _escapeString(string $str) : string {
		//disable (false) url_encoded for whatever reason
		return str_replace( "'", "''", remove_invisible_characters( $str, false ) );
	}

Is the the best method? Because the regex in that function creates %0[0-8bcef] which turns our 0s into a potential "non displayable" and strips them. But it also breaks the LIKE statement because the % is part of a non-displayable.

This is what it looks like without the override. Just look at the where statement

SELECT "o1"."ItemCode" as "ItemCode", "o1"."ItemName" As "ItemName", "o1"."UserText" As "UserText", CAST(o2.Price as nvarchar(100)) As Price
FROM "OITM" "o1"
INNER JOIN "ITM1" "o2" ON "o1"."ItemCode" = "o2"."ItemCode"
WHERE   (
"o1"."ItemCode" LIKE '6533-0.750%' ESCAPE '!'
OR  "o1"."ItemName" LIKE '6533-0.750%' ESCAPE '!'
 )
AND "o1"."U_webitem" = 'Yes'
AND "o2"."pricelist" = '1'
AND "o1"."sellitem" = 'Y'
AND "o1"."frozenfor" = 'N'

and with the override

SELECT "o1"."ItemCode" as "ItemCode", "o1"."ItemName" As "ItemName", "o1"."UserText" As "UserText", CAST(o2.Price as nvarchar(100)) As Price
FROM "OITM" "o1"
INNER JOIN "ITM1" "o2" ON "o1"."ItemCode" = "o2"."ItemCode"
WHERE   (
"o1"."ItemCode" LIKE '%006533-0.750%' ESCAPE '!'
OR  "o1"."ItemName" LIKE '%006533-0.750%' ESCAPE '!'
 )
AND "o1"."U_webitem" = 'Yes'
AND "o2"."pricelist" = '1'
AND "o1"."sellitem" = 'Y'
AND "o1"."frozenfor" = 'N'

[lonnieezell]

Are you saying that this problem is still happening even after removing the preg_replace and converting it to strtr?

[JakeAi]

That is correct. I am getting the sql strings from SQL Server Profiler which gets the absolute final sql.

function remove_invisible_characters($str, $url_encoded = true)
	{
		$non_displayables = [];

		// every control character except newline (dec 10),
		// carriage return (dec 13) and horizontal tab (dec 09)
		if ($url_encoded)
		{
			$non_displayables[] = '/%0[0-8bcef]/';  // url encoded 00-08, 11, 12, 14, 15
			$non_displayables[] = '/%1[0-9a-f]/';   // url encoded 16-31
		}

		$non_displayables[] = '/[\x00-\x08\x0B\x0C\x0E-\x1F\x7F]+/S';   // 00-08, 11, 12, 14-31, 127

		do
		{
			$str = preg_replace($non_displayables, '', $str, -1, $count);
		} while ($count);

		return $str;
	}

By default the URL_Encoded is set to true. I didn't know that at first so in my driver i made it false. But the regex takes my '000653-00000', makes a like '%000653-00000%' but the remove_invisible_characters makes a regex '/%00/' and removes it from the front of my like string that the builder creates. So i think this function should be called earlier since the like statement contains % or default to false, and document it?

[lonnieezell]

Hm. Interesting. That's straight from CI3 and I'm not overly up to speed on what it's for or how it's used in the other drivers so I will have to do some research. Have you checked CI3's driver to see how they handle it?

[JakeAi]

@lonnieezell, I looked into CI3 and found a mismatch.

CI4 - https://github.com/bcit-ci/CodeIgniter4/blob/develop/system/Database/BaseConnection.php#L1401

	protected function _escapeString(string $str): string
	{
		return str_replace("'", "''", remove_invisible_characters($str));
	}

CI3 - https://github.com/bcit-ci/CodeIgniter/blob/develop/system/database/DB_driver.php#L1121

	protected function _escape_str($str)
	{
		return str_replace("'", "''", remove_invisible_characters($str, FALSE));
	}

[JakeAi]

@lonnieezell I posted a pull request which behaves exactly like CI3. When I removed the escapeString from the MYSQLI Driver I had the same issue, but not on CI3. Which came down to that function not having false passed through.

[lonnieezell]

I think we're good on this, then. I merged your PR. Closing this unless we run into other issues.