Phishing Project Webbug Implementation

Question

Here comes the next round. I've implemented some of the suggestions from the previous review. That being said, though, there are a few things that I have on the horizons but haven't yet put in that were suggested. I am currently working on implementing dependency injection so that the database object is not redundantly instantiated. I am also working on implementing query iteration instead of returning the result set all at once.

As with previous reviews, I'm primarily looking to make sure that my implementation stays secure and efficient. I've added in a class that contains 2 static methods for generating logs on the server when something goes wrong. These functions generate the log and send an email. If the email fails, the log is still created.

DBManager

/**
 * DBManager constructor.
 */
public function __construct() {
    if($this->db == null) {
        try {
            $this->db = new PDO(getenv('DB_TYPE').':dbname='.getenv('DB_DATABASE').'1;host='.getenv('DB_HOST'),
                getenv('DB_USERNAME'), getenv('DB_PASSWORD'));
        } catch(\PDOException $pdoe) {
            ErrorLogging::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
        }
    }
}

/**
 * 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  \PDOException
 */
public function query($sql,$bindings) {
    if(is_null($this->db)) {
        throw new \PDOException();
    }
    $result = $this->executePreparedStatement($sql,$bindings);
    return $result;
}

/**
 * executePreparedStatement
 * 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  QueryException          Checks if prepared statement was successful created and executed
 */
private function executePreparedStatement($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 QueryException($message,$sql);
    }

    $result = $stmt->execute($bindings);

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

    //fetchAll() is used for now. Based off a previous suggestion, iteration will be implemented at a later date.
    return $stmt->fetchAll();
}

public function getErrorCode() {
    if(is_null($this->db)) {
        throw new \PDOException();
    }
    return $this->db->errorCode();
}

public function getErrorInfo() {
    if(is_null($this->db)) {
        throw new \PDOException();
    }
    return $this->db->errorInfo();
}

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,$urlId,$projectId);
        } else {
            $this->webbugExecutionWebsite($username);
        }
    } catch(QueryException $pdoe) {
        ErrorLogging::logQueryError(__CLASS__,__FUNCTION__,$pdoe,$db,array($urlId,$projectId));
    } catch(\PDOException $pdoe) {
        ErrorLogging::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
    }
}

/**
 * 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($urlId,$projectId) {
    if(!empty($_SERVER['REMOTE_ADDR'])) {
        $db = new DBManager();
        $ip = $_SERVER['REMOTE_ADDR'];
        $host = gethostbyaddr($_SERVER['REMOTE_ADDR']);
        $sql = "SELECT PRJ_ProjectName FROM gaig_users.projects WHERE PRJ_ProjectId=?;";
        $bindings = array($projectId);
        try {
            $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);
        } catch(QueryException $pdoe) {
            ErrorLogging::logQueryError(__CLASS__,__FUNCTION__,$pdoe,$db,array($urlId,$projectId));
        } catch(\PDOException $pdoe) {
            ErrorLogging::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
        }
    }

    return null;
}

/**
 * webbugExecutionEmail
 * Email specific execution of the webbug tracker.
 *
 * @param   string      $username           Username of user passed from webbugRedirect
 */
private function webbugExecutionEmail($username,$urlId,$projectId) {
    $db = new DBManager();
    $data = $this->webbugRootExecution($urlId,$projectId);
    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]);
        try {
            $db->query($sql,$bindings);
        } catch(QueryException $pdoe) {
            ErrorLogging::logQueryError(__CLASS__,__FUNCTION__,$pdoe,$db,array($urlId,$projectId));
        } catch(\PDOException $pdoe) {
            ErrorLogging::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
        }
    }
}

ErrorLogging - Mailing commented out for testing purposes

/**
 * logConnectError
 * Emails devs that a connection error has occured and then generates .log file
 *
 * @param   string          $class          Class Name
 * @param   string          $method         Function Name
 * @param   string          $message        Error Message
 * @param   array           $trace          Exception Trace
 */
public static function logConnectError($class, $method, $message, $trace) {
    $path = self::createFile($class);

    $headers = array('server'=>$path[1],'method'=>$method,'path'=>$path[2]);
    $mail = false;/*Mail::send(['html' => 'emails.errors.pdoconnectexception'],$headers, function($m) {
        $m->from('');
        $m->to('')->subject('CRITICAL: PDOConnectException');
    });*/

    date_default_timezone_set('America/New_York');
    $message = "Error in $class $method \n Error logged at: " . date('m/d/Y h:i:s a') .
        "\n Email sent to Devs: $mail \n Error logged on IP: $path[0] \n Error Message: $message" .
        "\n Error Trace: \n" . json_encode($trace) . "\n ------------------------------------- \n";
    error_log($message,3,$path[2]);
}

/**
 * logQueryError
 * Emails devs that a connection error has occured and then generates .log file
 *
 * @param   string          $class          Class Name
 * @param   string          $method         Function Name
 * @param   QueryException  $exception      Exception object to retrieve trace, message, and SQL
 * @param   DBManager       $db             Database object to retrieve errors
 * @param   array           $params         urlId and projectId
 */
public static function logQueryError($class, $method, $exception, $db, $params) {
    $path = self::createFile($class);

    $trace = $exception->getTrace();
    $errorcode = $db->getErrorCode();
    $errorinfo = $db->getErrorInfo();
    $ip = $_SERVER['REMOTE_ADDR'];
    $message = $exception->getMessage();
    $sql = $exception->getQuery();

    $headers = array('trace'=>$trace,'errorcode'=>$errorcode,'erorrinfo'=>$errorinfo,'ip'=>$ip,'message'=>$message,'sql'=>$sql,'params'=>$params);
    $mail = false;/*Mail::send(['html' => 'emails.errors.pdoqueryexception'],$headers, function($m) {
        $m->from('');
        $m->to('')->subject('PDOQueryException WebbugRedirect');
    });*/

    date_default_timezone_set('America/New_York');
    $message = "Error in $class $method \n Error logged at: " . date('m/d/Y h:i:s a') .
        "\n Email sent to Devs: $mail \n Error logged on IP: $path[0] \n Error Message: $message" .
        "\n Error Trace: \n" . json_encode($trace) . "\n ------------------------------------- \n";
    error_log($message,3,$path[2]);
}

/**
 * createFile
 * Executes common processes for logQueryError and logConnectionError.
 *
 * @param   string          $class          Class Name
 * @return  array
 */
private static function createFile($class) {
    $remote_addr = $_SERVER['REMOTE_ADDR'];
    $server_addr = getenv('SERVER_ADDR');
    $index = strpos($class,'\\');

    $path = '../storage/logs/' . basename($class) . '_' . date('m-d-Y') . '.log';
    if(!file_exists($path)) {
        $file = fopen($path,'w');
        fclose($file);
    }

    return array($remote_addr,$server_addr,$path);
}

As always, thanks for the feedback! Looking forward to some responses!

Answer 1

So having participated in previous reviews, I can say I like some of the evolution of this code, but have a few concerns as well.

1)

I still am not understanding the reason for the DBManager class to really exist. My expounded a lot on this in last review, so won't go into that here. Even with this latest review, I don't see what value this class is adding.

2)

You use this sort of validation pattern throughout your DMManager class:

if(is_null($this->db)) {
    throw new \PDOException();
}

I think it is good to fail loudly here, however I do not think you should not be throwing a PDOException and you should not be throwing an exception without any message at all. The PDO classes should be throwing PDO* exceptions, not your custom class. After all, who is to say PDO is the underlying cause to why you don't have a valid PDO instance to work with. Also, if you truly want to test that you have a valid PDO object, the validation might look like:

if(empty($this->db) || $this->db instanceof PDO === false) {
    throw new \Exception('You need a message here');
}

Ultimately, if you go with a dependency injection approach, you could actually remove this validation from each method, as you could then assure that valid PDO object was passed in constructor and set on object such that all other methods can reliably utilize it.

3)

I think doing away with custom PDO Exception classes is the right approach, but the way you have now split up business and logging logic seems a little strange. You are at the end of the day now utilizing PHP's error_log() functionality vs. email as previously done. This seems to be an improvement. I think however that you moved too much of the business logic into these logging methods, as now the logging methods have a dependency on the DBManger object and are very limited in terms of use beyond DBManager use cases.

In previous review, I suggested central logging mechanism (of which error_log() is certainly one example) vs. having the DBManager class directly email errors out. My stance really hasn't changed in that the DBManager class should know nothing about the specific implementation of the logging mechanism, only what logging mechanism to use. For this reason, I would suggest that these two methods might actually be methods on the DBManager class, because what they are doing is providing the business logic for forming the error message and then handing the logging of the message to the central logging mechanism. They could actually help bring meaning to the existence of that DBManager class if you need to provide some error handling behavior above and beyond what PDO gives you.

4)

I don't understand the createFile() method at all. If you are relying on a central logging mechanism like error_log(), you should not have to deal with creation of new log files, this should be handled by logging mechanism. I also don't understand the return value for this method.What do remote and server address variables have to do with anything?

I can see that you are trying to log to individual log files by class and date. I question the strategy of doing the by-class approach in that you might find it more diffivult to debug your application if you can't see a seeing of interconnected errors in a single log file vs. trying to go through a bunch of different files comparing timestamps and such. I would think the by-date functionality would typically be handled by whatever log rotation logic you have on the server, I don't know that I would want to enforce this in some class somewhere.

5)

If you take a look at the PhishingController class, you will note a lot of repeated code. It seem like really there are maybe 3-4 lines of code that is truly specific to each method (to generate SQL and parameters). Outside of that the code commonly instantiates the DB, queries the DB, and handles DB errors. This could likely be refactored to a single method that these other method utilize to do the common work.

I like that you are trying to catch multiple exception types and trigger different behaviors, but I wonder if this class is where that logic should be. If you take the comments above around possibly moving the error logging logic in DBManager class, then you could similarly have all this logic about when to log query error vs. connect error in DBManager class and not in this class.

Think about it for a second, what happens if you get one of your two defined error modes that you are concerned with? Should the DBManager class own reporting and logging these errors or the PhishingController class which is using DBManager? From the PhishingController class perspective, why should it log DB errors? All it should care about is whether the query executed successfully or not, not whether the error get logged successfully. The only question you should ask yourself for the PhishingController class, is what behavior does it need to provide to it's caller in the cases where the underlying query fails. As shown now, the class just eats the underlying exceptions and an returns null. This may be totally appropriate behavior, or you may decide that this class should in turn throw its own exception or rethrow the underlying exception if this method call is going to be critical and the caller needs to be aware of problems with underlying dependencies.