Phishing Project Error Logging

Question

Moving on to the next steps! Previous review was here. The idea here was to implement suggestions made by @hd/@Pimgd and then implement an effective way of tracking and logging results when an exception is thrown. With that said, my focus is still on security of my application, but now I also really want feedback on the data being sent when an error is triggered, and it's effectiveness or usefulness.

I ended up creating 2 custom exceptions for better naming of what error is happening - PDOEmptyResultException and PDOQueryException. If the latter is thrown, then something went wrong. If the former is thrown, then it's not so much an all hands on deck problem as a research and figure out what went wrong situation.

DBManager

/**
 * DBManager constructor.
 */
public function __construct() {
    try {
        $this->db = new PDO('mysql:dbname='.getenv('DB_DATABASE').';host='.getenv('DB_HOST'),getenv('DB_USERNAME'), getenv('DB_PASSWORD'));
    } catch(\PDOException $pdoe) {
        $trace = $pdoe->getTrace();
        $ip = $_SERVER['REMOTE_ADDR'];
        $message = $pdoe->getMessage();

        $headers = array('trace'=>$trace,'ip'=>$ip,'message'=>$message);

        Mail::send(['html' => 'emails.errors.pdoconnectexception'],$headers, function($m) {
            $m->from('[email protected]');
            $m->to('[email protected]')->subject('PDOQueryException WebbugRedirect');
        });
    }
    date_default_timezone_set('America/New_York');
}

/**
 * query
 * Public facing method for executing queries. It will return the result set back.
 *
 * @param   string          $sql        The query to be prepared and executed
 * @param   array           $bindings   An array of query parameters
 * @return  array                       Array of results from query
 * @throws  PDOQueryException
 */
public function query($sql,$bindings) {
    $result = $this->queryErrors($sql,$bindings);
    return $result;
}

/**
 * queryErrors
 * Calls the function to create and execute the prepared statement.
 *
 * @param   string      $sql        The query to be prepared and executed
 * @param   array       $bindings   An array of query parameters
 * @return  array                   Array of results from the prepared statement
 * @throws  PDOQueryException       Checks if result set is empty or null
 */
private function queryErrors($sql,$bindings) {
    $result = $this->createPreparedStatement($sql,$bindings);

    if(is_null($result)) {
        $message = "Prepared statement returned null.\nError Code: " .
            $this->db->errorCode() . "\nError Info: " . array_values($this->db->errorInfo());
        throw new PDOQueryException($message,$sql);
    } else if(empty($result)) {
        $message = "Prepared statement returned no results.";
        throw new PDOEmptyResultExecption($message);
    }

    return $result;
}

/**
 * bindParams
 * Creates an array of the binding parameters for a prepared statement.
 *
 * @param   array       $bindings   An array of query parameters
 * @return  array                   Array of results from the prepared statement
 */
private function bindParams($bindings) {
    $params = array();
    for($i = 0; $i < count($bindings); $i++) {
        $params[] = & $bindings[$i];
    }
    return $params;
}

/**
 * createPreparedStatement
 * Prepares the query($sql), binds the parameters, executes the query, then returns the result set.
 *
 * @param   string      $sql        The query to be prepared and executed
 * @param   array       $bindings   An array of query parameters
 * @return  array                   Array of results from the prepared statement
 * @throws  PDOQueryException       Checks if prepared statement was successful created and executed
 */
private function createPreparedStatement($sql,$bindings) {
    $stmt = $this->db->prepare($sql);

    if($stmt === false) {
        $message = "Failed to generate prepared statement.\nError Code: " .
            $this->db->errorCode() . "\nError Info: " . array_values($this->db->errorInfo());
        throw new PDOQueryException($message,$sql);
    }

    $params = $this->bindParams($bindings);
    $result = $stmt->execute($params);

    if($result === false) {
        $message = "Failed to execute prepared statement.\nError Code: " .
            $this->db->errorCode() . "\nError Info: " . array_values($this->db->errorInfo());
        throw new PDOQueryException($message,$sql);
    }

    return $stmt->fetchAll();
}

In theory, the PhishingController should only be executing and should have as little logic in it as possible. If there are any suggestions that could be made to extrapolate classes out of these methods below, that would be appreciated.

PhishingController

/**
 * webbugRedirect
 * Handles when webbugs get called. If request URI contains the word 'email', executes email webbug otherwise executes website webbug
 *
 * @param   string      $id     Contains UniqueURLId that references specific user and specific project ID
 */
public function webbugRedirect($id) {
    $urlId = substr($id,0,15);
    $projectId = substr($id,15,16);
    $db = new DBManager();
    $sql = "SELECT USR_Username FROM gaig_users.users WHERE USR_UniqueURLId=?;";
    $bindings = array($urlId);
    try {
        $result = $db->query($sql,$bindings);
        $username = $result[0]['USR_Username'];
        if(strpos($_SERVER['REQUEST_URI'],'email') !== false) {
            $this->webbugExecutionEmail($username);
        } else {
            $this->webbugExecutionWebsite($username);
        }
    } catch(PDOQueryException $pdoe) {
        $trace = $pdoe->getTrace();
        $errorcode = $db->getErrorCode();
        $errorinfo = $db->getErrorInfo();
        $ip = $_SERVER['REMOTE_ADDR'];
        $message = $pdoe->getMessage();
        $sql = $pdoe->getQuery();
        $params = array($urlId,$projectId);

        $headers = array('trace'=>$trace,'errorcode'=>$errorcode,'erorrinfo'=>$errorinfo,'ip'=>$ip,'message'=>$message,'sql'=>$sql,'params'=>$params);

        Mail::send(['html' => 'emails.errors.pdoqueryexception'],$headers, function($m) {
            $m->from('');
            $m->to('')->subject('PDOQueryException WebbugRedirect');
        });
    } catch(PDOEmptyResultExecption $pdoe) {
        /*
         * 1) user was deleted
         * 2) user failed to be created
         * 3) uniqueURLId failed to insert to database on first email sent
         */
    }

/**
 * webbugRootExecution
 * Common values for webbug execution. Returns array of values to calling method.
 * 
 * @param   int         $strLocation        Index of UniqueURLId in REQUEST_URI
 * @return  array|null                      Returns null if IP is hidden or not given, otherwise gives needed input
 */
private function webbugRootExecution($strLocation) {
    if(!empty($_SERVER['REMOTE_ADDR'])) {
        return null;
    }

    $db = new DBManager();
    $ip = $_SERVER['REMOTE_ADDR'];
    $host = gethostbyaddr($_SERVER['REMOTE_ADDR']);
    $reqpath = $_SERVER['REQUEST_URI'];
    $projectID = substr($reqpath, $strLocation);
    $projectID = ltrim($projectID, '0');
    $projectID = rtrim($projectID, '.');
    $projectID = intval(strval($projectID));
    $sql = "SELECT PRJ_ProjectName FROM gaig_users.projects WHERE PRJ_ProjectId=?;";
    $bindings = array($projectID);
    $result = $db->query($sql,$bindings);
    $projectName = $result[0]['PRJ_ProjectName'];
    $date = date("Y-m-d");
    $time = date("H:i:s");
    return array($ip,$host,$projectName,$date,$time);
}

/**
 * webbugExecutionEmail
 * Email specific execution of the webbug tracker.
 *
 * @param   string      $username           Username of user passed from webbugRedirect
 */
private function webbugExecutionEmail($username) {
    $db = new DBManager();
    $data = $this->webbugRootExecution(29);
    if(!is_null($data)) {
        $sql = "INSERT INTO gaig_users.email_tracking (EML_Id,EML_Ip,EML_Host,EML_Username,EML_ProjectName,
    EML_AccessDate,EML_AccessTime) VALUES (null,?,?,?,?,?,?);";
        $bindings = array($data[0],$data[1],$username,$data[2],$data[3],$data[4]);
        $db->query($sql,$bindings);
    }
}

As always, thanks for the feedback!

Answer 1

DB Manager thoughts:

1)

I am generally not understanding what you are trying to achieve by even having this class. It is not doing anything like managing database connections, abstracting the user from SQL formulation, adding transactional handling around queries, or other things that one might often implement a class such as this to do. As it stands now, you have in essence only added some custom exception types, while severely limiting the sorts of methods and ways of working the caller may desire since you have not implemented them in this wrapper class (How do we check number of rows in result set? How can we work with one record from result set at a time? Both very common operations.).

I honestly think you would be better just having a simple PDO provider class and let your calling code just interact with the PDO object itself (PDO is an abstraction after all).

2)

Without seeing code for the new Exception classes to understand if there really is some value being added by these (rather than simply more complexity for calling code to deal with), the value certainly is not clear here for having these. An exception specifically for getting an empty result set on a select query seems VERY odd, especially since there are many reasonable use cases when a DB might be queried and be expected to potentially return and empty result set. Now you may have a case where you would never expect a query to return an empty result set because of how the application is structured. In that case the logic calling the database should handle the empty result and perhaps throw an exception, but this should not be logic within the database class.

If you were truly doing something useful in this DB class like trying to abstract out the caller from knowing there is an underlying PDO object, then perhaps you would catch PDO Exceptions (which you do in many cases) and map them to exception types more meaningful to your application - terminal exceptions, retryable exceptions, etc. - not add in additional subclasses to PDOException.

3)

public function __construct() {
    try {
        $this->db = new PDO('mysql:dbname='.getenv('DB_DATABASE').';host='.getenv('DB_HOST'),getenv('DB_USERNAME'), getenv('DB_PASSWORD'));
    } catch(\PDOException $pdoe) {
        $trace = $pdoe->getTrace();
        $ip = $_SERVER['REMOTE_ADDR'];
        $message = $pdoe->getMessage();

        $headers = array('trace'=>$trace,'ip'=>$ip,'message'=>$message);

        Mail::send(['html' => 'emails.errors.pdoconnectexception'],$headers, function($m) {
            $m->from('[email protected]');
            $m->to('[email protected]')->subject('PDOQueryException WebbugRedirect');
        });
    }
    date_default_timezone_set('America/New_York');
}

• Most problematic is that when PDO object instantiation fails, the constructor succeeds. The caller would NEVER KNOW they have a worthless DBManager object. If this critical dependency fails, you should fail loudly to the caller. Most likely this should be by either rethrowing the underlying PDOException, or throw a new exception with type of your choosing if you are trying to abstract away the.
• Why does a DBManager class have anything to do with setting default time zones?
• Don't email critical system errors/exception - log them. Emails are fragile and prone to not be delivered, especially when your server might be having problems. Your application should have a strategy on how to centrally log errors/exceptions and not leave it up to individual classes to implement their own mechanisms for error logging.
• Thumbs up for abstracting out your DB credentials from the code!

4)

With regards to the methods used for querying:

• Only having a single chain of methods that execute for all query types seems problematic. The way one handles query execution, reading out results, recovering from problematic, expected results, can be much different between SELECT, INSERT, UPDATE, and DELETE queries.
• The query() method where the SQL statement and parameters for binding are provided by caller have no data validation whatsoever. You should immediately exit and not trying to move further in execution if you don't have valid values to work with.
• queryErrors() method seems meaningless based on earlier commentary regarding these custom exceptions.
• bindParams() method seems pointless. You are just building a duplicate of the array you already have (and then not actually doing any binding)?
• createPreparedStatement() seems inappropriately named. I would expect such a method to do what it says. Perhaps executePreparedStatement() would make more sense.
• By using fetchAll() after prepared statement is executed, you are really limiting the caller to HAVE to use the more memory intensive act of reading out the entire data set into a variable vs. allowing the caller to iterate the result set.

PhishingController Thoughts

• Same thoughts as above around error logging.
• webbugExecutionEmail() seems oddly named since it doesn't actually send an email, but rather seems to add someone to some email list in DB.
• I would pass DB object to this class ass a dependency, since it appears it cannot work without. This saves you from instantiating DB connections in every method.