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

Query error with parameterised queries #106

Closed
Tracked by #123
craigfrancis opened this issue Jan 16, 2022 · 9 comments
Closed
Tracked by #123

Query error with parameterised queries #106

craigfrancis opened this issue Jan 16, 2022 · 9 comments

Comments

@craigfrancis
Copy link
Contributor

Following up on Issue 69, as this is a different issue (I don't want to mix up the two things).

The SQL error, when checking $pdo->prepare():

------ ------------------------------------------------------------------ 
 Line   index.php                                                         
------ ------------------------------------------------------------------ 
 5      Query error: You have an error in your SQL syntax; check the      
        manual that corresponds to your MySQL/MariaDB server version for  
        the right syntax to use near ':id AND 2=2 LIMIT 0' at line 1      
                                                                          
        Simulated query: SELECT * FROM user WHERE id = :id AND 2=2 LIMIT  
        0 (1064).                                                         
 8      Query error: You have an error in your SQL syntax; check the      
        manual that corresponds to your MySQL/MariaDB server version for  
        the right syntax to use near '? AND 2=2 LIMIT 0' at line 1        
                                                                          
        Simulated query: SELECT * FROM user WHERE id = ? AND 2=2 LIMIT 0  
        (1064).                                                           
------ ------------------------------------------------------------------

This output used the new debug mode in RuntimeConfiguration (thanks for adding).

It's happening on both named and question mark parameter queries.

The MysqliQueryReflector::simulateQuery() method is used, and it looks like the query is being sent to the database without parameters being provided (or replaced).

It's interesting that later, when $stmt->execute() is being tested, the values are being replaced in QueryReflection::replaceParameters().

I wonder if this check can be done during $stmt->execute(), when you have some parameters to use... or it might be possible to generate some values, even if it's the number 0, or a value that's appropriate for the field type (considering it's not actually returning any records with a LIMIT 0).

@staabm
Copy link
Owner

staabm commented Jan 17, 2022

for some reason the placeholder replacement does not work on your machine.
I cannot reproduce either in CI nor on my windows laptop.

could you dig a bit thru

/**
* @param array<string|int, scalar|null> $parameters
*/
private function replaceParameters(string $queryString, array $parameters): string
{
$replaceFirst = function (string $haystack, string $needle, string $replace) {
$pos = strpos($haystack, $needle);
if (false !== $pos) {
return substr_replace($haystack, $replace, $pos, \strlen($needle));
}
return $haystack;
};
foreach ($parameters as $placeholderKey => $value) {
if (\is_string($value)) {
// XXX escaping
$value = "'".$value."'";
} elseif (null === $value) {
$value = 'NULL';
} else {
$value = (string) $value;
}
if (\is_int($placeholderKey)) {
$queryString = $replaceFirst($queryString, '?', $value);
} else {
$queryString = str_replace($placeholderKey, $value, $queryString);
}
}
return $queryString;
}

to get a feeling why it fails in your setup?

@staabm
Copy link
Owner

staabm commented Jan 17, 2022

I was able to reproduce this problem, when

    -
        class: staabm\PHPStanDba\Rules\SyntaxErrorInQueryMethodRule
        tags: [phpstan.rules.rule]
        arguments:
            classMethods:
                - 'rex_sql::setQuery#0'
                - 'rex_sql::setDBQuery#0'
                

is configured for a method, which actually takes a prepared statement and query-string separately

@craigfrancis
Copy link
Contributor Author

I think we are on the same page... and Pull Request 119 has stopped the error.

From my understanding, the checking of $pdo->prepare() does not call replaceParameters(), so the SQL is sent to the database with the placeholders (causing it to error).

In comparison $stmt->execute() (which is checked after this error has happened), does use replaceParameters(), and is fine.

I'm just wondering, could the parameters simply be replaced with the number 0? something like this:

$queryString = preg_replace('/(\?|\b:[a-zA-Z0-9_]+\b)/', '0', $queryString);

It's already using LIMIT 0, so it won't return any records... but I must confess I've not really checked it, and this approach might cause other issues.

e.g. While these are a bit contrived, WHERE question LIKE "%?" or WHERE meta_data LIKE "%:tag_name%". Because I'm not really doing proper parsing of the SQL string, those characters would be incorrectly replaced because they look like placeholders.

As an aside, I think there might have a similar issue with countPlaceholders() and extractNamedPlaceholders().

I wonder, if this is a problem, does it needs to do something similar to the pdo_parse_params() function, rather than simply using the BINDCHR regex (used later as PDO_PARSER_BIND)... as that supports escaped question marks (??), comments (/* What about X? */ or -- To fix?), and quoted string values (if I'm reading this correctly, using ANYNOEOF?).

@staabm
Copy link
Owner

staabm commented Jan 17, 2022

You are right that the current parsing is a bit naive and can easily break in edge cases.

We might finally use a proper sql parser.


Regarding your inital problem: I guess your phpstan-dba config is wrongly mixing SyntaxErrorInPreparedStatementMethodRule and SyntaxErrorInQueryMethodRule.

Actually for pdo support you don't need additional rules configuration. The default config should work for pdo-only source analysis.

I'm just wondering, could the parameters simply be replaced with the number 0

Thats what SyntaxErrorInPreparedStatementMethodRule is doing when variables types are known:

public static function simulateParamValueType(Type $paramType): ?string

@staabm staabm mentioned this issue Jan 17, 2022
13 tasks
@craigfrancis
Copy link
Contributor Author

Just running a quick test (need to go in a bit)... I've used a script that only contains:

<?php

$pdo = new PDO('mysql:dbname=test;host=localhost', 'test', 'test', [PDO::ATTR_EMULATE_PREPARES => false]);

$stmt = $pdo->prepare('SELECT * FROM user WHERE id = ?');

And that triggered the 'Query error' in SyntaxErrorInQueryMethodRule.

Taking a guess, that might be due to the provided dba.neon file.

@staabm staabm mentioned this issue Jan 17, 2022
@staabm
Copy link
Owner

staabm commented Jan 17, 2022

Just running a quick test (need to go in a bit)... I've used a script that only contains:

<?php

$pdo = new PDO('mysql:dbname=test;host=localhost', 'test', 'test', [PDO::ATTR_EMULATE_PREPARES => false]);

$stmt = $pdo->prepare('SELECT * FROM user WHERE id = ?');

And that triggered the 'Query error' in SyntaxErrorInQueryMethodRule.

Taking a guess, that might be due to the provided dba.neon file.

I think this example works on latest master. I think it was fixed with #119

at least running your example in a unit test seems to not produce a error, see #124

@craigfrancis
Copy link
Contributor Author

Yep, when I used #119, that fixed the error... and I suspect those checks are run when it get's to the $stmt->execute() (sorry, gtg, and I'm probably not going to be available for a few days).

@staabm
Copy link
Owner

staabm commented Jan 23, 2022

Does the initial issue still reproduce or can we close the issue?

@craigfrancis
Copy link
Contributor Author

Yes, thank you (once I realised that composer was not updating).

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

No branches or pull requests

2 participants