Elegant and secure way to create a dynamic query

Question

I want to create a dynamic query to display:

• all the invoices if the user has moderator rank
• only his invoices if the user is a simple member

code

 public function getAllBill($user_rank, $user_id){
 $sql = 'SELECT * FROM invoice_user
 INNER JOIN users on users.user_id = invoice_user.user_id';
 $sql_clause = 'WHERE user_id = :user_id)';
 $sql_group = 'GROUP BY invoice_number ORDER BY invoicedate DESC';
 if ($user_rank === 'member') {
 $final_sql = $sql.' '.$sql_clause.' '.$sql_group;
 $parameters = array(':user_id' => $user_id);
 } 
 else if ($user_rank === 'moderator'){
 $final_sql = $sql.' '.$sql_group;
 $parameters = array();
 }
 else {
 $error = TRUE;
 }
 if (!isset($error)) {
 $request = $this->bdd->prepare($final_sql);
 $request->execute($parameters);
 return $request->fetchAll();
 } else return array();
}

script

<?php 
$table = getAllBill($_SESSION ['rank'], $_SESSION ['id']);
foreach ($table as $row) {
 # code...
}

I would like to know if there is a more elegant way to do this, but especially to know if everything is safe knowing that the rank and the ID come from the user session ($_SESSION ['rank'], $_SESSION ['id'])

Does it pose a problem not to put my parameter table in a condition?

PS : I use MySQL

• AlexV: "... but especially to know if everything is safe knowing that the rank and the ID come from the user session ($_SESSION ['rank'], $_SESSION ['id'])" --> Why not add getAllBill($_SESSION ['rank'], $_SESSION ['id']) as an example how you would call this function then?
• Rocstar: @AlexV I just added the call to the function in my post, I would especially like to know if there is no risk of SQL injection
• ArtisticPhoenix: Use a SHOW COLUMNS FROM table query or DESCRIBE table then you can use the list of columns to whitelist incoming column names. I forget exactly what these return, but you can use something like array_intersect_key, to remove fields that don't have an actual column, such as a field like this "' OR 1 -- (classic SQL injection) You also have an error in your SQL right here $sql_clause = 'WHERE user_id = :user_id)'; no opening (
• ArtisticPhoenix: @Rocstar - if you allow the columns to be dynamic then yes there is possibility of SQL injection because you cannot parameterize the columns. If it's just arguments then you can prepare those with no issue. For example if you allow them to pick what column a sort is done on SELECT * FROM ... SORT BY $foo that can absolutely be injected by altering the keys passed to the query building bit.

Answer 1

If you allow users to pass in arguments that are column names (for example array keys that you turn into column names). Then the keys could be modified to allow SQL Injection. It's not clear in the question if you plan to do this. If you just want to have dynamic arguments (with known columns) then preparing the query should suffice as all the user data can be parameterized without issue.

As preparing the values is not an issue, I will assume you want to make the columns and/or tables dynamic.

Problem

Imagine you create a dynamic Select query like this, where the keys from the input args are used for column names and the table name is dynamic as well (fully dynamic):

class foo{
 function Select($table, $args){
 $query = "SELECT * FROM {$table}";
 $where = [];
 $params = [];
 foreach($allowedArgs as $col => $arg){
 $where[] = "{$col} = :{$col}";
 $params[$col] = $arg;
 }
 if(!empty($where)) $query .= ' WHERE '.implode(' AND ', $where);
 print_r($query);
 }
}

Output would be something like this:

 //------- intended behaviour
 $foo->Select('someTable', ['ID' => 32, 'user' => 'someUser']);
 "SELECT * FROM someTable WHERE ID = :ID AND user = :user"
 //after executing with parameters
 "SELECT * FROM someTable WHERE ID = '32' AND user = 'someUser'"
 //------- unintended behaviour
 $foo->Select('someTable', ['ID' => 32, '1 OR 1 --' => null]);
 "SELECT * FROM someTable WHERE ID = :ID AND 1 OR 1 -- = :1 OR 1 --"
 //the DB would execute this:
 "SELECT * FROM someTable WHERE ID = 32 AND 1 OR 1"

To explain the SQLInjection:

• The first 1 completes the AND
• Then the OR 1 is always true.
• Everything after the SQL comment -- is ignored.

So this would return all records in that table regardless of the other arguments. This is how basic SQLInjection works.

Here $table is also susceptible to SQLInjection, so if you did (this is a bad example but you get the point):

 $foo->Select('admin WHERE 1 --', []);
//"SELECT * FROM admin WHERE 1 --"

Solution

However, you can query the DB schema, by using DESCRIBE {$table} and SHOW TABLES, to build a whitelist of column names and tables. And than, store that data so you can filter out any bad "stuff" like this:

//eg. results of DESCRIBE 
Field Type Null Key Default Extra 
 ID int(10) unsigned NO PRI NULL auto_increment
//eg. SHOW TABLES
Tables_in_{database}
 someTable

From this you can build something like:

class foo{
 //get the schema
 public static $fields;
 public static $tables;
 public function ckTable($table){
 if(!self::$tables){
 $smtp = $this->db->query("SHOW TABLES");
 self::$tables = $stmt->fetchAll(PDO::FETCH_COLUMN); //['someTable', "admin", ...]
 }
 if(!in_array($table, self::$tables)) throw new UnknownTable($table);
 }//end ckTable()
 public function getFields($table){
 $this->ckTable($table);
 //only query this once per request
 if(!self::$fields[$table]){
 self::$fields[$table] = [];
 $smtp = $this->db->query("DESCRIBE $table");
 $fields = [];
 while($row = $smtp->fetch(PDO::FETCH_ARRAY)){
 //technically we only need the Keys or the "Field" for this
 //but I felt like showing how you can parse the type too... 
 preg_match('/^(?P<type>\w+)(?:\((?P<size>\d+)\))?/', $row['Type'], $match);
 $fields[$table][$row['Field']] = [
 'type' => $match['type'],
 'size' => isset($match['size']) ? (int)$match['size'] : false,
 'key' => $row['Key']
 ];
 }//end while
 }//end if self::$fields
 return self::$fields[$table];
 }//end getFields()
 }//end foo

Regex Sandbox

Which should give you something like this:

 //-note- we're returning only $fields['someTable'] in the example
 $fields = [
 'someTable' => [
 'ID' => ['type' => 'int', 'size' => 10, 'key' => 'PRI']
 ], [...]
 ], [...]
 ];

Then when you make your dynamic query:

 class foo{
 //eg inputs (with sql injection) 
 //$table = 'someTable';
 //$args = ['ID' => 1, '1 OR 1 --' => '']
 public function Select($table, array $args){
 $fields = $this->getFields($table); //retuns ['ID' => [...]]
 $allowedArgs = array_intersect_key($args, $fields); //results in ['ID' => 1], removing key '1 OR 1 --'
 //escaping with backtic ` is a good idea, it adds a tiny bit of security too.
 //eg. "SELECT * FROM `admin WHERE 1 --`" this is a sql error
 //that said it's mostly for reserved words and spaces in table names etc..
 $query = "SELECT * FROM `{$table}`";
 $where = [];
 $params = [];
 foreach($allowedArgs as $col => $arg){
 //backtic column names too
 $where[] = "`{$col}` = :{$col}"; // 'ID = :ID'
 $params[$col] = $arg;
 }
 if(!empty($where)) $query .= ' WHERE '.implode(' AND ', $where);
 $this->db->prepare($query);
 return $this->db->execute($params);
 } //end Select()
 }//end foo

Output would be something like this:

$foo->Select('someTable', ['ID' => 1, '1 OR 1 --' => '']);
//note the SQLInjection key is removed by array_intersect_key
$query = "SELECT * FROM `someTable` WHERE `ID` = :ID"
//in PDO the ':' for execute arguments are optional
$params = ['ID' => 1];

array_intersect_key() returns an array containing all the entries of array1 which have keys that are present in all the arguments.

So by having the schema dynamically pulled from the DB we can filter out bad Keys that would become column names in the SQL. Also because it's dynamic (you should store it once per request, so do the describe query once) if we modify the schema we don't have any issues with our code. It just adapts based on the arguments and the schema.

The only problem with this (the tables being dynamic) is they could potentially access other tables you don't want them too. But only if they are valid tables. If that is a concern you can always create the table array manually or explicitly remove those tables you don't want end users having access to, with unset etc...

Additional Error checking

You can also throw an exception by comparing the count of $args and $allowedArgs Like this:

class foo{
 //eg inputs (with sql injection) ['ID' => 1, '1 OR 1 --' => '']
 public function Select($table, array $args){
 $fields = $this->getFields($table);
 $allowedArgs = array_intersect_key($args, $fields);
 if(count($args) != count($allowedArgs)){
 $diff = array_diff_key($args, $allowedArgs);
 throw new UnknownColumn(implode(',', array_keys($diff)));
 }
 ....
 }//end Select()
}//end foo

The above would throw a UnknownColumn (assuming that exception exists) that says something like this:

//$foo->Select('someTable', ['ID' => 1, '1 OR 1 --' => '']);
Exception UnknownColumn: "1 OR 1 --" IN somefile.php ON line 123

If you did some table that doesn't exist (as I showed above) you would get an exception from ckTable() which is chained through getFields:

//$foo->Select('1 OR 1', ['ID' => 1, '1 OR 1 --' => '']);
Exception UnknownTable: "1 OR 1" IN somefile.php ON line 123

Obviously you can add more complexity such as if it's and AND or an OR. Argument groups such as exclusive ORs. Type checks, size checks etc. Basically whatever you want as you will have all the schema data at your fingertips.

I actually have a DB class that basically does this to build dynamic Insert and Update queries. For example in the above I know that ID is the Pkey of the table so if its present in the arguments, I know that is an update of row with {x} ID. If not then it's an insert etc... Unfortunately my DB class has some dependencies and I can't share it without removing some of my employers requirements. It does a lot of other things too, like handling multiple databases, different comparison operators, logical groups etc. I should refactor it and put it on Git (one of these days).

Anyway, I should re-iterate you can also do this with a simple array (instead of getting the schema) that you manually built. Essentially a whitelist the columns or tables. But, if you edit the schema you will likely have to modify your code.

Cheers!

PS. I didn't test any of this, I like PDO so I used it for the code. But it should be pretty close.

Answer 2

For improved readability and for reduced processing & memory, I recommend an early return and no single-use variables.

public function getAllBill($user_rank, $user_id) {
 if (!in_array($user_rank, ['member', 'moderator'])) {
 return [];
 }
 $parameters = [];
 $sql = 'SELECT *
 FROM invoice_user
 INNER JOIN users on users.user_id = invoice_user.user_id';
 if ($user_rank === 'member') {
 $sql .= ' WHERE user_id = :user_id';
 $parameters[':user_id'] = $user_id;
 }
 $sql .= ' GROUP BY invoice_number
 ORDER BY invoicedate DESC';
 $request = $this->bdd->prepare($sql);
 $request->execute($parameters);
 return $request->fetchAll();
}

You are properly implementing a prepared statement so security is good. Now your code is free from convolution and excess variables. (Snippet: untested)