0

I have to run a query based on data inside of an array. Locally, I tested with names.

$names = ['my name', 'another name'];

if (!$conn) {
    die("Connection failed: " . mysqli_connect_error());
}

$sql = "SELECT * FROM `clients` WHERE `name` IN ('".implode("','",$names)."') ORDER BY id DESC";

$result = $conn->query($sql);

while($row = $result->fetch_assoc()) {
    print_r($row);
}

$conn->close();

This returns the correct rows based on the names in my $names array, but some people are saying that using the implode function inside the query is dangerous. I'm not sure how else to go about this.

corporalpoon
  • 219
  • 6
  • 16
  • it is absolutely no different than if you concatenated the list prior to the statement, i.e. $daNames = implode($names); then $sql = "SELECT * FROM `clients` WHERE `name` IN ($daNames) ORDER BY id DESC"; I am sure that someone will tell you it is vulnerable but it isnt because of the implode. It is more due to the fact that if someone can counterfeit values going in, they may rewrite the remainder of the query. Prepared statements are the popular mitigation, if you are set to use your setup though, you can just remove special chars in your strings. – NappingRabbit Jan 03 '18 at 14:41
  • 1
    This *specific* iteration of it isn't, as you've hard-coded the possible values with ones that are safe - they don't contain characters like `'`. That said, presumably you won't be using hard-coded values in the future (you could skip the database entirely if they were fixed values, after all), and that's where you run into problems. Your question is a little like "is pointing this empty gun at my head and pulling the trigger dangerous?" - while it may *technically* be safe as-is, it's not a good habit. – ceejayoz Jan 03 '18 at 14:42
  • Thanks, everyone. I understand now. – corporalpoon Jan 03 '18 at 14:46

1 Answers1

3

Yes, it's vulnerable, because quotes are not escaped. Consider this input:

$names = ['my name', 'another name', '\') or true;--'];

It's not the implode that's a problem -- that has nothing to do with it. The problem is that you're building a query with string concatenation instead of prepared statements and bound parameters

Alex Howansky
  • 50,515
  • 8
  • 78
  • 98