2

Im new to database and i have written a LOT of PHP code that accesses a database using MySQL.

I didnt take into account SQL injection attacks so i have to re-write all that PHP code to use mysql prepared statements.

After looking at videos on how to used prepared SQL statements, to perform just ONE SQL command requires a whole lot of "prepared" statements. My existing code has lots of different SQL statements all over the place, it would be a nightmare to change all that code to pack and unpack all the required preparation for each "prepared" statement command.

Is there some kind of wrapper i can use to prevent turning one line of regular SQL into 6 or 7 lines of prepared statements?

For example use to do this line line of SQL

SELECT * from users where userid=10

needs many more lines of prepared SQL statements, especially if there are lots of other SQL statements too it now becomes very complex.

Is there was some sort of one line wrapper that i can call that accepts the template SQL string, plus the parameters, which also executes the command and returns the result in just one line of wrapper for different types of MYSQL statements it would be great and the code would be much less confusing looking and error prone.

For example

 $users=WrapAndExecute($db,"SELECT * from users where userid=?","s",$userid);

 $data=WrapAndExecute($db,"UPDATE table SET username=?,city=?","ss",$name,$city);

 $result=WrapAndExecute($db,"DELETE from table where id=?","s",$userid);

 $result=WrapAndExecute($db,"INSERT into ? (name,address) VALUES(?,?)","ss","users",$name,$address);

Each of those lines above would create a prepared statement template, do the bind, execute it and return the result that a regular MYSQL statement would. This would create minimal impact on existing code.

Anybody knows how to do this or if some easy php library or class already exists to do this, that i can just import and start using it?

Thanks

Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
Programmer2
  • 69
  • 11
  • I don't know what're you asking, but in doubt, here's the [PDO::prepare](https://secure.php.net/manual/en/pdo.prepare.php) PHP documentation. – Artemis Apr 12 '17 at 16:23
  • Sorry I didnt give enough information, i just updated my original post, thanks for your comment – Programmer2 Apr 12 '17 at 16:37
  • 1
    _needs many more lines of prepared SQL statements_ : more lines that give a much better, safer code IMHO. And not that much, it's easy to read/understand/maintain... I don't see **any reason** why one would not sweat a little (lot ?) to have some good/excellent code ^^ – OldPadawan Apr 12 '17 at 16:37
  • It looks like the [execute](https://secure.php.net/manual/en/pdostatement.execute.php) documentation's examples may be more helpful. Examples 2 & 3 appear to be the most succinct. – Uueerdo Apr 12 '17 at 16:37
  • There are plenty of PDO (and probably mysqli_) wrappers that does what you want. – junkfoodjunkie Apr 12 '17 at 16:39

4 Answers4

2

You don't need to change a query to a prepared statement if it has no PHP variables in it. If it has just constant expressions, it's safe from SQL injection.

$sql = "SELECT * from users where userid=10"; // Safe!
$stmt = $pdo->query($sql);
$data = $stmt->fetchAll();

You don't need to change a query that contains PHP variables, as long as the value of that variable is a constant specified in your code. If it doesn't take its value from any external source, it's safe.

$uid = 10;
$sql = "SELECT * from users where userid=$uid"; // Safe!
$stmt = $pdo->query($sql);
$data = $stmt->fetchAll();

You don't need to change a query that contains PHP variables, as long as you can filter the value to guarantee that it won't risk an SQL injection. A quick and easy way to do this is to cast it to an integer (if it's supposed to be an integer).

$uid = (int) $_GET['uid'];
$sql = "SELECT * from users where userid=$uid"; // Safe!
$stmt = $pdo->query($sql);
$data = $stmt->fetchAll();

That leaves cases where you are using "untrusted" values, which may have originated from user input, or reading a file, or even reading from the database. In those cases, parameters are the most reliable way to protect yourself. It's pretty easy:

$sql = "SELECT * from users where userid=?"; // Safe!

// two lines instead of the one line query()
$stmt = $pdo->prepare($sql);
$stmt->execute([$_GET['uid']]);

$data = $stmt->fetchAll();

In a subset of cases, you need one additional line of code than you would normally use.

So quit your whining! ;-)


Re your comment about doing prepared statements in mysqli.

The way they bind variables is harder to use than PDO. I don't like the examples given in http://php.net/manual/en/mysqli.prepare.php

Here's an easier way with mysqli:

$sql = "SELECT * from users where userid=?"; // Safe!

$stmt = $mysqli->prepare($sql);
$stmt->bind_param('i', $_GET['uid']);
$stmt->execute();
$result = $stmt->get_result();

$data = $result->fetch_all();

I don't like the stuff they do in their examples with bind_result(), that's confusing and unnecessary. Just use get_result(). So with mysqli, you need two more lines of code than you would with PDO.

I've written query wrappers for mysqli that emulate the convenience of PDO's execute() function. It's a PITA to get an array mapped to the variable-arguments style of bind_param().

See the solution in my answers to https://stackoverflow.com/a/15933696/20860 or https://stackoverflow.com/a/7383439/20860

Community
  • 1
  • 1
Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
  • Bill, thanks for that, but dont you have to also tell it the type of data each parameter, for example "ssddd" parameter 1 is string (s) parameter2 is also a string (s) parmater 3 is a decimal (d) etc – Programmer2 Apr 12 '17 at 16:47
  • 1
    No, not in the case of MySQL. It may be required in other PDO drivers for other brands of SQL database, but in MySQL's case, you can skip the `bindParam()` nonsense and just pass an array of parameter values to `execute()`. This is much simpler. – Bill Karwin Apr 12 '17 at 16:50
  • I saw in the SQL "prepared statements" tutorial the presenter was using 6 or 7 lines of extra PHP statements in order to execute just one line of regular PHP, I was like thats crazy, my code will be 7 times as big and more complicated to add all that! But it seems its not really like that – Programmer2 Apr 12 '17 at 16:50
  • I've seen lots of examples like that too. I have no idea who started that. They're making it more difficult than it needs to be. – Bill Karwin Apr 12 '17 at 16:51
  • Bill THANK GOD all that extra stuff is not necessary, i was just about to shoot myself LOL, I was like wow these people love to type and make complicated code!!! – Programmer2 Apr 12 '17 at 16:52
  • 1
    One thing that does demand a lot of extra code is checking the success/error result from each function call. But if you enable `PDO::ERRMODE_EXCEPTION` you can skip that, unless you want to catch and log errors in a specific way. See http://php.net/manual/en/pdo.error-handling.php In any case, error-checking would also not be significantly greater from using prepared statements. – Bill Karwin Apr 12 '17 at 16:54
  • Oh, and skip using the "mysqli" functions. I would only use it if I had to quickly port a bunch of legacy code that used the old deprecated "mysql" functions, because they're mostly one-for-one. Any new PHP apps should use PDO. – Bill Karwin Apr 12 '17 at 16:58
  • OMG, I never heard of PDO, all my code is in mysqli, I have around 80 php scripts full of mysqli code none has prepared statememts. Now I have the additional task of converting it all to PDO, maybe i do have to shoot myself after all – Programmer2 Apr 12 '17 at 17:08
  • Then don't convert them. If they're working with mysqli, leave them alone. You might like to start using PDO for new projects, though. – Bill Karwin Apr 12 '17 at 17:22
  • Here's a nicely written guide if you do decide to convert some of your older scripts: http://john.cuppi.net/pdo-and-mysql-code-migration-guide/ – Bill Karwin Apr 12 '17 at 17:24
  • If you check out this link http://php.net/manual/en/mysqli.prepare.php you can see instead of 1 mysql statement there is 7 statements for mysqli, so I can eliminate the one with the parameter type "sssddss" and the bind you're saying and make the rest a function call? – Programmer2 Apr 12 '17 at 17:28
  • Strange i thought PHP was UNTYPED language, yet the bind statement mysqli_stmt_bind_param requires you to tell it the type! – Programmer2 Apr 12 '17 at 17:43
  • PHP is untyped? Sort of. They do have `int` and `float` and `boolean` as native types. And just because PHP is flexible on types doesn't necessarily mean SQL is. But FWIW, you can generally use 's' for the parameter types in most cases. MySQL is fairly smart about converting strings containing numeric values. See https://dev.mysql.com/doc/refman/5.7/en/type-conversion.html – Bill Karwin Apr 12 '17 at 17:56
  • Ok, thanks I was just wondering how you set the $pdo variable is that like $pdo=new PDO("mysql:host=localhost....."); i have to figure out how $pdo will work with my mysqli code, thank for your help! – Programmer2 Apr 12 '17 at 17:58
  • @Programmer2 PHP is not a "strongly typed" language. – Jay Blanchard Apr 12 '17 at 18:03
  • The big issue is my code is using the old style $conn=mysqli_connect($dbhost,$dbuser,$dbpass); mysqli_select_db($conn,$db); somehow i have to convert that to pdo in all of my 80 php scripts in order to use prepared statements – Programmer2 Apr 12 '17 at 18:06
  • Well then you better get off Stack Overflow and get to work. ;-) – Bill Karwin Apr 12 '17 at 18:13
0

I were in the same boat, and I wrote such a wrapper that works exactly the way you want, save for it's being a class, not a function.

$user = $sdb->getRow("SELECT * from users where userid=?s", $userid);
$sdb->query("UPDATE table SET username=?s, city=?s", $name, $city);
$sdb->query("DELETE from table where id=?s", $userid);
$sdb->query("INSERT into ?n (name,address) VALUES(?s,?s)","users", $name, $address);

The above is a working code, as long as you have somewhere in your bootstrap file

$db = mysqli_connect(...);
...
require 'safemysql.class.php';
$sdb = new SafeMySQL('mysqli' => $db);

Note that none of the other suggestions could do anything like that.

Also note that if I were writing it today, I would have used PDO, as this class is duplicating a lot of functionality already exists in PDO.

Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
-1

Take a look at the PDO extension in PHP - http://php.net/manual/en/intro.pdo.php: it it secure against injections thanks to prepared statements; also, it allows you to connect to many different databases (e.g. MySQL, MSSQL, etc.).

You can then build your own wrapper as you wish to keep it clean; for example your own wrapper could be as follows: (following example will return user rows as objects)

// connect to DB
$GLOBALS['default_db'] = new DB('localhost','db_name','username','password') ;

// Get users and output results
$query = new DBQuery('SELECT * FROM users WHERE userid = ?',array(10)) ;
var_dump($query -> results()) ;
var_dump($query -> num_rows()) ;

// DB connection
class DB {

    public $connection;

    public function __construct($host , $dbname , $username , $password) {
        $this->connection = new \PDO('mysql:host=' . $host . ';dbname=' . $dbname , $username , $password);
    }

}

// Wrapper
class DBQuery {

    private $num_rows = 0;
    private $results  = array();

    public function __construct($query , $params = null , $class_name = null , DB $db = null) {

        if ( is_null($db) ) {
            $db = $GLOBALS['default_db'];
        }

        $statement = $db->connection->prepare($query);
        $statement->execute($params);

        $errors = $statement->errorInfo();
        if ( $errors[2] ) {
            throw new \Exception($errors[2]);
        }

        $fetch_style = ($class_name ? \PDO::FETCH_CLASS : \PDO::FETCH_OBJ);
        $this->results = $class_name ? $statement->fetchAll($fetch_style , $class_name) : $statement->fetchAll($fetch_style);
        $this->num_rows += $statement->rowCount();

        while ( $statement->nextrowset() ) {
            $this->results = array_merge($this->results,$class_name ? $statement->fetchAll($fetch_style , $class_name) : $statement->fetchAll($fetch_style));
            $this->num_rows += $statement->rowCount();
        }
    }

    public function num_rows() {
        return $this->num_rows;
    }

    public function results() {
        return $this->results;
    }

}
sammysaglam
  • 433
  • 4
  • 9
-1

Since a key requirement seems to be that you can implement this with minimal impact on your current codebase, it would have been helpful if you had told us what interface you currently use for running your queries.

While you could use PDO:

  • that means an awful lot of work if you are not already using PDO
  • PDO exceptions are horrible

Assuming you are using procedural mysqli (and have a good reason not to use mysqli_prepare()) its not that hard to write something (not tested!):

function wrapAndExecute()
{
   $args=func_get_args();
   $db=array_shift($args);
   $stmt=array_shift($args);
   $stmt_parts=explode('?', $stmt);
   if (count($args)+1!=count($stmt_parts)) {
        trigger_error("Argument count does not match placeholder count");
        return false;
   }
   $real_statement=array_shift($stmt_parts);
   foreach ($args as $k=>$val) {
      if (isnull($val)) {
         $val='NULL';
      } else if (!is_numeric($val)) {
         $val="'" . mysqli_real_escape_string($db, $val) . "'";
      }
      $real_statement.=$val . array_shift($stmt_parts);
   }
   return mysqli_query($db, $real_statement);
}

Note that this does not handle IS [NOT] NULL nicely nor a literal '?' in the statement nor booleans (but these are trivial to fix).

symcbean
  • 47,736
  • 6
  • 59
  • 94