Creating a secure and efficient DB connection and queries (using one class)

Question

Currently, I'm working with a PHP database connection class that is in one single file with a construct. Every time I need this connection, I instantiate the class and the construct creates the connection with only having to modify the user, password, DBname and host.

It works fine and I haven't faced any problems with this class. Although I want to start taking security seriously in my web applications, I've been looking for a way to implement security (SQL Inj, XSS, DoS, etc.) and use updated functions, prepared statements, escaping, etc. My problem is that I haven't found a single answer about this exactly because I don't call the mysqli_connect function in index.php. I just create the object for the Connection class and use it.

<?php class Connection { private $connection; private $host='localhost'; private $user='user'; private $pass='pass'; private $database='dbname'; private $n=0; function __construct() { $this->connection = mysqli_connect($this->host, $this->user, $this->pass, $this->database); mysqli_query($this->connection,"SET NAMES 'utf8'"); } function sql($sql) { if (is_array($sql)) { foreach ($sql as $s) { $res = mysql_query($s); if (mysql_errno()) echo mysql_error().'<br>'; $this->n++; } } else { $res = mysqli_query($this->connection,$sql); if (mysql_errno()) echo mysql_error().'<br>'; $this->n++; } return $res; } function __destruct() { if (is_resource($this->connection) ) mysql_close($this->connection); } function getLink() { return $this->connection; } } 

So if I want to Create, Read, Update, Delete Something I do this in another file: User.php (Which i want to use the new method I'm asking for and use prepared statements for the Queries here and escape functions)

<?php // Declare a variable private $user; // Setter function setUser($val){ $this->user = $val; } // Create Function to CreateUser function CreateUser(){ $con = new Connection(); $sql = "INSERT INTO user (DATA) VALUES(DATA)"; $con->sql($sql); } // Same for all CRUD i just change the Query of course. ?> <?php // At the index.php i do this: include_once 'User.php'; $user = new User(); if(isset($_POST['createUserButton'])){ // $The variable i set in my User Class $user = $_POST['user']; // Then the Setter $user->setUser($user); // Then execute the Function $user->CreateUser(); } ?> 

I'm trying to translate this class using:

• New versions of functions
• Using try/catch
• Security

I'm just trying to have as many points of view from more experienced developers. I'm a few months old at this and I'm trying to look at different methods to code so I can create my own.

Answer 1

You say that you use prepared statements, but your code doesn't actually reflect that.

sql just executes any query, without binding or escaping anything. You really need to use prepared statements though. Your current sql function isn't usable in practice and it's pure existence will likely lead to vulnerabilities (other programmers will likely use it as it exists, and it can't be used securely).

Apart from that:

• don't mix different APIs. Currently, you use mysqli and mysql.
• don't use mysql, it's been deprecated for years and has finally been removed in PHP7.
• your formatting is off. Use correct indentation, always use curly brackets, use newlines for statements (eg after an if), and use spaces around =. Any IDE can fix these things for you.
• be consistent with your naming. Your functions should all start with a lower-case character.
• Also, same things should be named the same. So if you have a field called connection, the method retrieving it should be getConnection, not getLink.
• Also try to be more explicit with your variable names and don't shorten names. val should be value (but really, it should be user or username), res should be result, n should be queryCount, and so on.
• It's very unclear to me what n is used for. I would remove it, or document it.
• sql isn't really useful. It's just a wrapper around mysql_query which automatically echoes errors (meaning that the calling code has no possibility to recover or show a different error message; throw an exception instead), and provides the possibility to execute multiple queries, which doesn't actually seem needed.
• You don't want to create a new database connection for each method (for performance reasons), so instead, create it once and then pass the db object to methods that need it.

Answer 2

Things that I would consider critical design flaws:

• You are trying to establish working with the DB in an OOP manner, yet you are falling back to using procedural PHP database interactions throughout. Why not work with MySQLi in OO fashion?
• This is not even working code in that you do things like call mysql_close() on a resource opened with mysqli_connect(). You simply SHOULD NOT be working with two different DB resource types interchangeably in your code, especially ones representing the same database resource.
• Your singleton pattern (if that is what you are attempting) is flawed. The instance storage variable must be private static; the constructor must be private; and the the method to invoke instance should be be static. There is nothing to prevent your code from spawning thousands or hundreds of thousands of connections to your DB, making your entire application grind to a halt.

• You are adding an unnecessary level of abstraction around an existing database library (MySQLi in this case) and adding no additional value to it. Typically one might write database-related classes to do things like:

  • connection management (like a Singleton or connection pool implementation)
  • adding "model" capabilities (i.e. ORM, active record pattern, etc.)
  • adding fluent, natural language query capabilities ($db->select('field')->from('table')->where('id = ?') or similar).

You are doing none of that. You are just obfuscating the underlying MySQLi implementation from the caller, while simultaneously leaking implementation details outside the class, as the caller has to know that you dealing with a mysqli result set being returned from sql() and then work with that result set, splitting DB interaction logic in two places.

• There really is no security here to speak of. Potentially dirty data are set to the class methods and you do no data validation, no data escaping/sanitization, no prepared statements, etc.

Some more minor concerns:

• I would echo thoughts from @tim around syntax and styling issues.
• You talk about try-catch, but there is nothing there to review. I would encourage further investigation in this area as you will find it useful once you are used to working with exceptions.
• This class (or really any class that is not specifically designed to do so) should not echo output directly to standard out. Why would a DB connection class generate output? Once you learn exceptions you will better learn how to pass pertinent messaging about exceptional application states up the call stack, to where you can better position your application's display/rendering logic to return error messaging that is more meaningful to the end user.
• This code only considers happy path. You just assume all DB connections, queries, etc. work the way they are expected. You should strive to understand all the possible returns from function/method calls and provide appropriate code to handle the range of outcomes (happy path, null/void returns, value vs. false returns, exceptions thrown, etc.).