7 ms·

> The docs for how you were supposed to do SQL were just full of the antipattern of building queries by string formatting and concatenation This is still to th

> The docs for how you were supposed to do SQL were just full of the antipattern of building queries by string formatting and concatenation This is still to this day the recommended way to construct a "WHERE foo IN (a,b,c,...)" query in PHP. It's insane that there's no way to pass an array of values into a parameterised query for that use case.

path411 5y ago
What do you mean? You can Google php parameterize query and get atleast 2 different methods of doing this. You shouldn't ever touch string concat in any language when doing queries
- sellyme 5y ago
  > You can Google php parameterize query and get atleast 2 different methods of doing this There's no way of doing this with a single parameter. You need to parameterise every single individual item in the IN clause to do it that way, which is a horrific solution when it's of a completely unknown length. Still better than string concatenation in many cases, but that the language has no in-built way of doing it is one of the many reasons PHP code is so often vulnerable to injection attacks. There's so much friction to writing secure code.
ec109685 5y ago
Better is to concatenate question marks and then pass the params as an array.
- tored 5y ago
  Yes, something like this $user_ids = [1, 6, 46, 3, 17]; $count = count($user_ids); $in = '(' . implode(', ', array_fill(0, $count, '?')) . ')'; $sql = "SELECT user_id, email FROM user WHERE user_id IN {$in}"; $stmt = $pdo->prepare($sql); $stmt->execute($user_ids); var_dump($stmt->fetchAll());
  - throwawayboise 5y ago
    I realize this is just an example but if you're in control of all the values then there's nothing unsafe about concatenating them into a query. The problem comes with values that are submitted via a web form, API, or some other external, untrusted source.
    - sellyme 5y ago
      Yep. Fortunately a lot of the use cases of IN() are with controlled values - often IDs obtained from a previous query - so string concatenation is safe (and a lot less hassle than the alternatives). Unfortunately that gets people into the habit of using string concatenation, which is not a great habit to have.
    - tored 5y ago
      Not sure if I follow you completely, but in my example $user_ids could come from an external source because I’m only concatenating question marks (?), then bind $user_ids with execute and that is safe. What I also always do before passing the $user_ids to execute if it comes from an external source is to validate that everyone of them is an integer with filter_var.
    - wk_end 5y ago
      This is true but it’s still better to default to the Safe Thing, as long as there isn’t a good reason not to. How long until those values you know you control carelessly get turned into values you don’t control somewhere along the way?
      - sumtechguy 5y ago
        I used to mess with other devs by injecting bits of code from other sources (when blink in html worked it was one of my favorite ones). The correct way is to bind your params and do not trust that the data you got from some other system is 'OK'. What may be fine in one system could be an escape code in another. I speculate that the reason this is such an issue is because the interface at the ODBC level is basically security wise broken. It works 'OK' for getting/putting the data but it has 2 modes of execution. One of those paths is not great for security, the other has a usage issue. 'Binding' can be a real pain as it takes at least 1 call per variable parameter. Then managing the buffers correctly. So just building up the strings is an easy way to skip a lot of steps. So many take it. But that path leads to security vulins.
fouc 5y ago
Probably good idea to use a framework or a database library in PHP. Laravel has Model::whereIn('foo', [a, b, c, ...])
- billisonline 5y ago
  I'm shocked no one else gave this answer earlier in the thread. If you're using PDO directly in 2021, you're absolutely doing it wrong. You don't need to use all of Laravel, or even all of Eloquent for that matter. If you don't want to depend on a framework or use an ORM, you can use "illuminate/database" (https://packagist.org/packages/illuminate/database https://packagist.org/packages/illuminate/database) for a secure wrapper around PDO. No need to reinvent the wheel.
  - sellyme 5y ago
    > If you're using PDO directly in 2021, you're absolutely doing it wrong. This is somewhat the point. If using the language's standard libraries is "absolutely doing it wrong", that's an indictment of the language.
    - billisonline 5y ago
      > If using the language's standard libraries is "absolutely doing it wrong" You are being deliberately obtuse. Other comments in this thread offer correct examples of using PDO to avoid SQL injection. I didn’t mean it was impossible to write safe database code using the standard library—obviously, PHP is a Turing-complete language, it can be done!—I just meant it’s awkward, and verbose, and developers are unlikely to do it consistently throughout an application. Hence this type of concern is best abstracted into a library. To your point about “indicting a language,” most languages have footguns like this. The worst you can say about PHP is that the documentation should do more to discourage new users from working with PDO directly. (And I mean the official documentation—the language maintainers can’t be held responsible for the kind of unofficial tutorials the article complains about.) But regardless of what the official docs say, most PHP development today is done using frameworks like Laravel, Symfony, and Zend framework that do not suffer from SQL injection issues.
yourenotsmart 5y ago
If there's anything more annoying than the lingering bad examples of PHP code online, it's people lying about what PHP "recommends", like you do. Common database drivers like PGSQL, MySQL, SQLite etc. don't accept arrays of values for parameterized queries. This is at C level, in their own client libraries, and their own communication protocols. This means there's nothing specific to PHP about this problem. Many higher level libraries, including for PHP, abstract over this problem and do offer arrays by binding per query. So where are statements like yours coming from? Probably just being eager to say something bad about PHP without checking your facts too much.
- sellyme 5y ago
  > This means there's nothing specific to PHP about this problem. I don't particularly care if languages that I am not using have the same problem. It's up to people who use those languages to make those criticisms. > Many higher level libraries, including for PHP, abstract over this problem and do offer arrays by binding per query. Feel free to link an in-built PHP library that supports this feature, that would be far more useful than just obliquely suggesting that one exists.
  - TrispusAttucks 5y ago
    PDO + Prepared Statements https://www.php.net/manual/en/pdo.prepared-statements.php https://www.php.net/manual/en/pdo.prepared-statements.php
    - sellyme 5y ago
      PDO explicitly does not support the functionality being discussed [1]: > For example, you cannot bind multiple values to a single parameter in the IN() clause of an SQL statement. You're required to roll your own implementation, something that thankfully isn't particularly difficult, but unfortunately also seems to be enough of a barrier that a lot of programmers don't bother. [1]: https://www.php.net/manual/en/pdo.prepare.php https://www.php.net/manual/en/pdo.prepare.php
hattmall 5y ago
That's what the docs should show because that's how it works. The docs should give you the streamlined barebones implementation. It's trivial to write your own function to use parameterized queries and add in all the type checking etc you need. It's only a few lines of code.
- sellyme 5y ago
  > It's trivial to write your own function to use parameterized queries and add in all the type checking etc you need. It's only a few lines of code. It's fairly trivial to do this, but now you're potentially adding thousands of parameters per query in circumstances where the contents of the IN() are variable in count. This is not ideal for a number of reasons. Additionally, a language should be designed such that the easiest possible way to do something is at least moderately secure. If you need to attach some boilerplate code on top of the standard libraries every single time you use them for it to be safe, then there is no reason for that boilerplate to not be in the standard libraries.
remram 5y ago
Can't you at least build a "IN (?, ?, ?)" string, if you're going to build a string dynamically?
- sellyme 5y ago
  Not only is it the case that you can do that, it's the case that you should do that. Which leads to the question of why it's not in-built functionality. Making secure code harder to write than insecure code is a great way to ensure that lots of people write insecure code.
  - adzm 5y ago
    You end up writing code with 16 ? parameters and filling the empty ones with -1 or something ;)
    - kijin 5y ago
      No, you use array_fill() and implode() to generate exactly as many placeholders as you need.
chipotle_coyote 5y ago
> It's insane that there's no way to pass an array of values into a parameterised query for that use case. Maybe I'm misunderstanding you, but assuming $params is an array in the following code, isn't this passing an array into a parameterized query for that use case? (Edited to note this is literally an example from the PHP documentation, and not one of the squiffy comments.) $place_holders = implode(',', array_fill(0, count($params), '?')); $sth = $dbh->prepare("SELECT id, name FROM contacts WHERE id IN ($place_holders)"); $sth->execute($params); In Python, using MySQLdb, I believe this would be something like place_holders = ','.join(['%s'] * len(params)) cursor.execute("DELETE FROM foo.bar WHERE baz IN (%s)" % place_holders, tuple(params)) Which, while more succinct, seems to be functionally exactly the same thing. I don't see what PHP is doing that's "worse" here offhand. One could argue, "Yes, but a Python programmer would use SQLAlchemy," which is probably true, but then you need to let the PHP programmer use Doctrine or Eloquent.
- sellyme 5y ago
  > isn't this passing an array into a parameterized query for that use case? Yes, I definitely should have been more specific there - what I'm referring to is passing it as one parameter, instead of potentially dozens or hundreds. There's a lot of ways to do this safely, but none of them are elegant. In the example presented here I believe it's the case that you can't do the ->execute($params) and bind some parameters explicitly, so if you had something like " AND status = ? AND due_date < ?" at the end of your query you have to chuck those variables into the same nondescript array. I prefer the looped bindParam() method for this reason, but that has its own challenges. Firstly, it requires some boilerplate (not a big deal, but no-one likes writing boilerplate), and more pressingly it still has the issue where it actually is each individual element of the array being parameterised, and spams the ever-loving crap out of any debug outputs. Obviously all of these issues are way less concerning than SQL injection vulnerabilities, but life would be so much easier if you could just do $sth->bindParam(1, $params); on a single question mark, and have that show up logically in things like debugDumpParams(). Even if you had to use special syntax to indicate when a parameter is expected to be an array, that would be a huge improvement. I'm sure there's technical reasons why this is more difficult to implement than it would initially seem, but I've seen enough string-concatenated queries on StackOverflow from people who just give up on getting the parameterisation to play nicely that I believe it's worth the effort to make doing things right as frictionless as possible.

path411 5y ago

What do you mean? You can Google php parameterize query and get atleast 2 different methods of doing this. You shouldn't ever touch string concat in any language when doing queries

sellyme 5y ago
> You can Google php parameterize query and get atleast 2 different methods of doing this There's no way of doing this with a single parameter. You need to parameterise every single individual item in the IN clause to do it that way, which is a horrific solution when it's of a completely unknown length. Still better than string concatenation in many cases, but that the language has no in-built way of doing it is one of the many reasons PHP code is so often vulnerable to injection attacks. There's so much friction to writing secure code.

ec109685 5y ago

Better is to concatenate question marks and then pass the params as an array.

tored 5y ago
Yes, something like this $user_ids = [1, 6, 46, 3, 17]; $count = count($user_ids); $in = '(' . implode(', ', array_fill(0, $count, '?')) . ')'; $sql = "SELECT user_id, email FROM user WHERE user_id IN {$in}"; $stmt = $pdo->prepare($sql); $stmt->execute($user_ids); var_dump($stmt->fetchAll());
- throwawayboise 5y ago
  I realize this is just an example but if you're in control of all the values then there's nothing unsafe about concatenating them into a query. The problem comes with values that are submitted via a web form, API, or some other external, untrusted source.
  - sellyme 5y ago
    Yep. Fortunately a lot of the use cases of IN() are with controlled values - often IDs obtained from a previous query - so string concatenation is safe (and a lot less hassle than the alternatives). Unfortunately that gets people into the habit of using string concatenation, which is not a great habit to have.
  - tored 5y ago
    Not sure if I follow you completely, but in my example $user_ids could come from an external source because I’m only concatenating question marks (?), then bind $user_ids with execute and that is safe. What I also always do before passing the $user_ids to execute if it comes from an external source is to validate that everyone of them is an integer with filter_var.
  - wk_end 5y ago
    This is true but it’s still better to default to the Safe Thing, as long as there isn’t a good reason not to. How long until those values you know you control carelessly get turned into values you don’t control somewhere along the way?
    - sumtechguy 5y ago
      I used to mess with other devs by injecting bits of code from other sources (when blink in html worked it was one of my favorite ones). The correct way is to bind your params and do not trust that the data you got from some other system is 'OK'. What may be fine in one system could be an escape code in another. I speculate that the reason this is such an issue is because the interface at the ODBC level is basically security wise broken. It works 'OK' for getting/putting the data but it has 2 modes of execution. One of those paths is not great for security, the other has a usage issue. 'Binding' can be a real pain as it takes at least 1 call per variable parameter. Then managing the buffers correctly. So just building up the strings is an easy way to skip a lot of steps. So many take it. But that path leads to security vulins.

fouc 5y ago

Probably good idea to use a framework or a database library in PHP. Laravel has Model::whereIn('foo', [a, b, c, ...])

billisonline 5y ago
I'm shocked no one else gave this answer earlier in the thread. If you're using PDO directly in 2021, you're absolutely doing it wrong. You don't need to use all of Laravel, or even all of Eloquent for that matter. If you don't want to depend on a framework or use an ORM, you can use "illuminate/database" (https://packagist.org/packages/illuminate/database https://packagist.org/packages/illuminate/database) for a secure wrapper around PDO. No need to reinvent the wheel.
- sellyme 5y ago
  > If you're using PDO directly in 2021, you're absolutely doing it wrong. This is somewhat the point. If using the language's standard libraries is "absolutely doing it wrong", that's an indictment of the language.
  - billisonline 5y ago
    > If using the language's standard libraries is "absolutely doing it wrong" You are being deliberately obtuse. Other comments in this thread offer correct examples of using PDO to avoid SQL injection. I didn’t mean it was impossible to write safe database code using the standard library—obviously, PHP is a Turing-complete language, it can be done!—I just meant it’s awkward, and verbose, and developers are unlikely to do it consistently throughout an application. Hence this type of concern is best abstracted into a library. To your point about “indicting a language,” most languages have footguns like this. The worst you can say about PHP is that the documentation should do more to discourage new users from working with PDO directly. (And I mean the official documentation—the language maintainers can’t be held responsible for the kind of unofficial tutorials the article complains about.) But regardless of what the official docs say, most PHP development today is done using frameworks like Laravel, Symfony, and Zend framework that do not suffer from SQL injection issues.

yourenotsmart 5y ago

If there's anything more annoying than the lingering bad examples of PHP code online, it's people lying about what PHP "recommends", like you do. Common database drivers like PGSQL, MySQL, SQLite etc. don't accept arrays of values for parameterized queries. This is at C level, in their own client libraries, and their own communication protocols. This means there's nothing specific to PHP about this problem. Many higher level libraries, including for PHP, abstract over this problem and do offer arrays by binding per query. So where are statements like yours coming from? Probably just being eager to say something bad about PHP without checking your facts too much.

sellyme 5y ago
> This means there's nothing specific to PHP about this problem. I don't particularly care if languages that I am not using have the same problem. It's up to people who use those languages to make those criticisms. > Many higher level libraries, including for PHP, abstract over this problem and do offer arrays by binding per query. Feel free to link an in-built PHP library that supports this feature, that would be far more useful than just obliquely suggesting that one exists.
- TrispusAttucks 5y ago
  PDO + Prepared Statements https://www.php.net/manual/en/pdo.prepared-statements.php https://www.php.net/manual/en/pdo.prepared-statements.php
  - sellyme 5y ago
    PDO explicitly does not support the functionality being discussed [1]: > For example, you cannot bind multiple values to a single parameter in the IN() clause of an SQL statement. You're required to roll your own implementation, something that thankfully isn't particularly difficult, but unfortunately also seems to be enough of a barrier that a lot of programmers don't bother. [1]: https://www.php.net/manual/en/pdo.prepare.php https://www.php.net/manual/en/pdo.prepare.php

hattmall 5y ago

That's what the docs should show because that's how it works. The docs should give you the streamlined barebones implementation. It's trivial to write your own function to use parameterized queries and add in all the type checking etc you need. It's only a few lines of code.

sellyme 5y ago
> It's trivial to write your own function to use parameterized queries and add in all the type checking etc you need. It's only a few lines of code. It's fairly trivial to do this, but now you're potentially adding thousands of parameters per query in circumstances where the contents of the IN() are variable in count. This is not ideal for a number of reasons. Additionally, a language should be designed such that the easiest possible way to do something is at least moderately secure. If you need to attach some boilerplate code on top of the standard libraries every single time you use them for it to be safe, then there is no reason for that boilerplate to not be in the standard libraries.

remram 5y ago

Can't you at least build a "IN (?, ?, ?)" string, if you're going to build a string dynamically?

sellyme 5y ago
Not only is it the case that you can do that, it's the case that you should do that. Which leads to the question of why it's not in-built functionality. Making secure code harder to write than insecure code is a great way to ensure that lots of people write insecure code.
- adzm 5y ago
  You end up writing code with 16 ? parameters and filling the empty ones with -1 or something ;)
  - kijin 5y ago
    No, you use array_fill() and implode() to generate exactly as many placeholders as you need.

chipotle_coyote 5y ago

> It's insane that there's no way to pass an array of values into a parameterised query for that use case. Maybe I'm misunderstanding you, but assuming $params is an array in the following code, isn't this passing an array into a parameterized query for that use case? (Edited to note this is literally an example from the PHP documentation, and not one of the squiffy comments.) $place_holders = implode(',', array_fill(0, count($params), '?')); $sth = $dbh->prepare("SELECT id, name FROM contacts WHERE id IN ($place_holders)"); $sth->execute($params); In Python, using MySQLdb, I believe this would be something like place_holders = ','.join(['%s'] * len(params)) cursor.execute("DELETE FROM foo.bar WHERE baz IN (%s)" % place_holders, tuple(params)) Which, while more succinct, seems to be functionally exactly the same thing. I don't see what PHP is doing that's "worse" here offhand. One could argue, "Yes, but a Python programmer would use SQLAlchemy," which is probably true, but then you need to let the PHP programmer use Doctrine or Eloquent.

sellyme 5y ago
> isn't this passing an array into a parameterized query for that use case? Yes, I definitely should have been more specific there - what I'm referring to is passing it as one parameter, instead of potentially dozens or hundreds. There's a lot of ways to do this safely, but none of them are elegant. In the example presented here I believe it's the case that you can't do the ->execute($params) and bind some parameters explicitly, so if you had something like " AND status = ? AND due_date < ?" at the end of your query you have to chuck those variables into the same nondescript array. I prefer the looped bindParam() method for this reason, but that has its own challenges. Firstly, it requires some boilerplate (not a big deal, but no-one likes writing boilerplate), and more pressingly it still has the issue where it actually is each individual element of the array being parameterised, and spams the ever-loving crap out of any debug outputs. Obviously all of these issues are way less concerning than SQL injection vulnerabilities, but life would be so much easier if you could just do $sth->bindParam(1, $params); on a single question mark, and have that show up logically in things like debugDumpParams(). Even if you had to use special syntax to indicate when a parameter is expected to be an array, that would be a huge improvement. I'm sure there's technical reasons why this is more difficult to implement than it would initially seem, but I've seen enough string-concatenated queries on StackOverflow from people who just give up on getting the parameterisation to play nicely that I believe it's worth the effort to make doing things right as frictionless as possible.