Phishing Project Error Logging

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('someone@someone.com');
$m->to('someoneElse@someone.com')->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 exe

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('someone@someone.com');
            $m->to('someoneElse@someone.com')->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.

• `web

StackExchange Code Review Q#134208, answer score: 5