Phishing Project Assessment

Coming back for another look at my project after I've reworked a few of the key points made in my previous post here.

So far, I've rewritten most of my code to address SQL Injection. I've created a class called DBManager to connect to my database and then create prepared statements and execute them.

Right now there is some duplication as I will be adding in some error logging in the near future.

The focus here should be on Security of my code. That being said, I'm not opposed to anything you have to say.

DBManager

```
private $db;

//create database connection - find constructive way to log errors without database if connect fails
public function __construct() {
$this->db = new PDO('mysql:dbname='.getenv('DB_DATABASE').';host='.getenv('DB_HOST'),getenv('DB_USERNAME'), getenv('DB_PASSWORD'));
date_default_timezone_set('America/New_York');
$this->connectErrors();
}

//custom query generator - calls queryErrors to execute
//helper function to only expose what's necessary
public function query($sql,$bindings,$queryEnum) {
$result = $this->queryErrors($sql,$bindings,$queryEnum);
return $result;
}

//calls createPreparedStatement to execute, logs to database based on result
//delete and update not converted, but shows echo logging for some of the data that would be logged
private function queryErrors($sql,$bindings,$queryEnum) {
if($queryEnum == 1) {
//select
$result = $this->createPreparedStatement($sql,$bindings);
}
else if($queryEnum == 2) {
//insert
$result = $this->createPreparedStatement($sql,$bindings);
}
else if($queryEnum == 3) {
//delete
if(!$result = $this->db->query($sql)) {
echo "Sorry, the website is experiencing technical difficulties.";
echo "Error: Our query failed to execute and here is why: \n";
echo "Delete Query: " . $sql . "\n";
echo "Errno: " . $this->db->errno . "\n";
echo "Error: " . $this->db->er

✔ Using environment variables

This is a great way to reduce the risk of credentials being leaked through version control. Kudos.

✘ No PHPDoc comments

Although pedantic, your method comments aren't structured to a way that will generate documentation for you (or others). For example, having the following as your comment structure will help both readability and debugging.

/**
 * createPreparedStatement
 * This method will prepare the query ($sql), the bind the parameters, execute the query, then return the result set.
 *
 * @param   string      $sql        The query to be prepared and executed
 * @param   array       $bindings   An array of query parameters
 * @return  var                     Either a result set array, NULL, or string.
 */

✘ Functions aren't strictly using return

Having a mixture of return and echo within your methods is a weird approach. Normalise this to only have return. Also having a mixture of return types (null, array) in one method will cause some weird logic elsewhere in your code.

✘ Exposing details

Your queryErrors() (plus connectErrors()) method is exposing some details that may help an attacker. Consider moving the current output (raw $sql, $this->db->errno, $this->db->error) to a developer log, and returning a HTTP/1.1 503 Service Unavailable or HTTP/1.1 400 Bad Request to the client with a generic error page (perhaps with an error code (that isn't generated by the DB layer) to help you traceback to what went wrong if the end-user reports it.)

Also, you're outputting very technical errors ("Wrong SQL: invalid prepare statement") which will be no use to an end-user.

Overall, the application is secure against SQL Injections. Though I cannot see any restriction (perhaps it's elsewhere in the code) to stop users from enumerating through the users table - which (using the webbugEmailRedirect looks like it sends an e-mail. Users receiving an e-mail that they didn't initiate through your application that isn't a newsletter might cause some concern for users).

I would also suggest you space your code out a bit, splitting method calls and lines below by a line for readability. For example;

public function webbugEmailRedirect($id) {
    $urlId = substr($id,0,15);

    $db = new DBManager();
    $sql = "SELECT USR_Username FROM gaig_users.users WHERE USR_UniqueURLId=?;";
    $bindings = array($urlId);
    $bindingTypes = array('s');
    $result = $db->query($sql,$bindings,QueryEnum::SELECT);

    print_r(array_values($result));
    $username = $result[0]['USR_Username'];

    //check if user exists
    $this->webbugExecutionEmail($urlId,$username);
}

It may also be worth while, since query() (which calls queryErrors()) can return null or an array is to check the key [0] exists in $result before blindly using it.

StackExchange Code Review Q#134174, answer score: 9