Anneliese Z. Anneliese Z. - 7 months ago 24
PHP Question

Prevent unbounded mysqli queries

I'm working for a company that uses this Mysqli php class to do mysql calls.
The problem is, the previous programmer wasn't great about preventing unbounded queries. So there are things dispersed throughout the code like the following:

$db -> where('id',$_POST['id']);
$db -> delete('table');


This code is supposed to only delete one record where
id = $_POST['id']
. However, if
$_POST['id']
is empty, we've got problems. It then deletes the entire table. One solution to this problem would be to find all the places in the code where delete or update functions are called and then make sure that the where variable is actually set.

if(isset($_POST['id']) && $_POST['id']!=''){
$db -> where('id',$_POST['id']);
$db -> delete('table');
}


But, that would take a lot of work because I know there are about 200 instances in the code. I'm hoping there might be a way to alter the following 2 functions to prevent them from executing unbound queries in the first place. Any help is appreciated!!

/**
* Update query. Be sure to first call the "where" method.
*
* @param string $tableName The name of the database table to work with.
* @param array $tableData Array of data to update the desired row.
*
* @return boolean
*/
public function update($tableName, $tableData)
{
if ($this->isSubQuery)
return;

$this->_query = "UPDATE " . self::$_prefix . $tableName ." SET ";

$stmt = $this->_buildQuery (null, $tableData);
$status = $stmt->execute();
$this->reset();
$this->_stmtError = $stmt->error;
$this->count = $stmt->affected_rows;

return $status;
}

/**
* Delete query. Call the "where" method first.
*
* @param string $tableName The name of the database table to work with.
* @param integer $numRows The number of rows to delete.
*
* @return boolean Indicates success. 0 or 1.
*/
public function delete($tableName, $numRows = null)
{
if ($this->isSubQuery)
return;

$this->_query = "DELETE FROM " . self::$_prefix . $tableName;

$stmt = $this->_buildQuery($numRows);
$stmt->execute();
$this->_stmtError = $stmt->error;
$this->reset();

return ($stmt->affected_rows > 0);
}

public function where($whereProp, $whereValue = 'DBNULL', $operator = '=', $cond = 'AND')
{
// forkaround for an old operation api
if (is_array($whereValue) && ($key = key($whereValue)) != "0") {
$operator = $key;
$whereValue = $whereValue[$key];
}
if (count($this->_where) == 0) {
$cond = '';
}
$this->_where[] = array($cond, $whereProp, $operator, $whereValue);
return $this;
}

Answer

You should catch the bad value when it's being passed to the where function, not later. That way it's easier to follow stack traces.

public function where($whereProp, $whereValue = 'DBNULL', $operator = '=', $cond = 'AND')
{
    if (is_null($whereValue) || trim($whereValue) == '') {
        throw new Exception('Cannot pass null or empty string as a condition to MysqliDb::where')
    }
    // ...
}

You could check through the _where protected property array inside of the delete function too, but it's not good practice to silently fail a method by doing a simple return out of the function. If you insist, though:

public function delete($tableName, $numRows = null)
{
    foreach ($this->_where as $w) {
        if (is_null($w[3]) || trim($w[3]) == '') {
            return;
            // or alternatively throw new Exception('...')
        }
    }
    // ...
}