Skip to main content

✔ Using enivronmentenvironment variables

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

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 structrestructure will help both readability and debugging.

/**
 * createPreparedStatement
 * This method will prepare the query ($sql), the bind the perametersparameters, 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.
 */

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 recievingreceiving an e-mail that they didn't initiate through your application that isn't a newsletter might cause some concern for users).

✔ Using enivronment variables

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

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 structre will help both readability and debugging.

/**
 * createPreparedStatement
 * This method will prepare the query ($sql), the bind the perameters, 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.
 */

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 recieving an e-mail that they didn't initiate through your application that isn't a newsletter might cause some concern for users).

✔ Using environment variables

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

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.
 */

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).

deleted 8 characters in body
Source Link
ʰᵈˑ
  • 346
  • 1
  • 6

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 applicationlog, 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.)

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 application, 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.)

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.)

Source Link
ʰᵈˑ
  • 346
  • 1
  • 6

✔ Using enivronment variables

This is a great way to reduce the risk of credentails 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 structre will help both readability and debugging.

/**
 * createPreparedStatement
 * This method will prepare the query ($sql), the bind the perameters, 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 application, 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 recieving 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.