1
\$\begingroup\$

I have an Ajax request from a page where the user is being asked to enter their date of birth. The request URL file has the code below and works all good. However, I'll be rewriting all MySQL/MySQLi functions used on the same website and this is generally how I use prepared statements. Should I avoid using die();? Is the code below acceptable and secure at some level?

// If received the birthday of logged in user
if(isset($_REQUEST['new_dob']) && isset($_REQUEST['userId'])) {
  $newdob = $_REQUEST['new_dob'];
  $userid = $_REQUEST['userId'];

  $check_info = "SELECT date_of_birth FROM users WHERE id = ? ";
  $check_mate = $conn -> prepare($check_info);
  if ( false === $check_mate ) {
    die('prepare() failed: ' . htmlspecialchars($conn->error));
  }
  $check_mate -> bind_param("i", $userid);
  $check_exe = $check_mate -> execute();
  if ( false === $check_exe ) {
    //No dob record at all for this userid
    die('execute() failed: ' . htmlspecialchars($conn->error));

  } else {
    $check_mate -> bind_result($date_of_birth);
    $check_mate -> fetch(); 
    if ($newdob != $date_of_birth) {
        $update_flag = 1;
        $check_mate -> close();
    } else if ($newdob == $date_of_birth || $newdob == '--' || $newdob == '0000-00-00' || $newdob == false || $newdob == NULL) {
        exit;
    }
  }
  if($update_flag == 1) {
    $update_dob = "UPDATE users SET date_of_birth = ? WHERE id = ? ";
    $update_exe = $conn -> prepare($update_dob);
    if ( false === $update_exe ) { 
      die('prepare() failed: ' . htmlspecialchars($conn->error));
    }
    $update_exe -> bind_param("si", $newdob, $userid);
    $update_result = $update_exe -> execute();
    if ( false === $update_result ) {
      //No record at all for this userid
      die('execute() failed: ' . htmlspecialchars($conn->error));
    } else {
        $updated = 1;
        $update_exe -> close();
    }
  } else {
    // set dialog as completed
  }
}
\$\endgroup\$
2
\$\begingroup\$

Should I avoid using die();?

I would (at least in the places you are using it). It makes your code really hard to reuse. And it can easily lead to problems once your requirements change.

Personally, I would just use exceptions, and let them bubble up to wherever you want to handle the problem (I usually follow MVC loosely and do this in the controller).

Security

Yes, your code should be secure. You are using prepared statements, and you are using them correctly. Additionally, you prevent XSS, which is also done correctly.

Two small things:

  • You should not echo database error messages directly to the enduser. It may contain information that can help in further attacks, and it is not very user friendly.
  • You should not use REQUEST, but POST instead. Requests that change state should always be submitted via POST, and using REQUEST thus provides a fall-back to GET, which might aid attackers in exploiting other existing vulnerabilities such as CSRF or XSS. Example: Let's say your request is vulnerable to CSRF. Without the fallback, I could only use POST, meaning I can only distribute my payload via websites I control or websites that allow JavaScript to be posted. But if you do allow the fallback to GET, now I can use any website where I can eg control image urls, which is easier to find. Same for XSS, where it's distributing links vs getting the victim to visit my website.

Readability

I found your code a bit hard to read, resulting from a combination of these issues:

Naming

  • don't shorten variable names. If I have to guess and/or remember what you meant, it will make the code harder to read. Examples: dob (date of birth?), mate (no idea), exe (execute)
  • consistency: Same things should be named the same, and all variable names should follow the same pattern. If you use snake_case, use it everywhere (eg newdob vs date_of_birth; one uses snake_case, one doesn't, one shortens dob, one doesn't; same with new_dob vs userId).
  • again naming consistency: Here, you use the same pattern for different things: update_exe/update_result vs update_dob. All three deal with dob stuff, but only the ar contains it in its name (instead of eg update_query which would follow the pattern of the other two names).
  • one more naming consistency issue: For the select query, you use this pattern: check_info (the query), check_mate (bind result for param as well as result, and check result), check_exe (execute result). For the update query, you use update_dob (the query), update_exe (not execute result, but prepare result, as well as bind result), update_result (execute result). It would be better to be consistent here.

Formatting

  • I wouldn't put spaces around ->. Spaces indicate an interaction of two entities (eg when using == or .), but that's not what's happening. There is only one entity, and its method is being called. This confused me a couple of times while reading your code.
  • inconsistent spacing, eg in your if statements.
  • inconsistent function use: you use exit and die interchangeably in very similar situations. I would pick one and stick with that.

Nested ifs and lack of function

For my taste, your code is too nested, and it does too many different things for its length. An alternative might look like this (a bit simplified):

$result = processBirthday($_POST['newBirthday'], $_POST['userId']);
[...]

function processBirthday($newBirthday, $userId) {
    if (!isValidDate($newBirthday)) {
        return "not valid";
    }

    $oldBirthday = getBirthday($userId);
    if ($newBirthday == $oldBirthday) {
        return "same date";
    }
    updateBirthday($newBirthday, $userId);        
}

function isValidDate($newBirthday) {
    return !($newdob == '--' || $newdob == '0000-00-00' || $newdob == false || $newdob == NULL);
}

The getBirthday and updateBirthday functions now do the actual db interaction. This way, you can easily reuse those functions in other parts of your application, and your code is a lot more readable.

Other

  • double checks: if ($newdob != $date_of_birth) { ... } else if ($newdob == $date_of_birth) { ... } here you don't need the second check. Either it's the same, or it's not.
  • one-time variables. If these kinds of variables have good names they may sometimes increase the readability of code. But often times, they just make code harder to read. So instead of $update_result = $update_exe -> execute(); if ( false === $update_result ) I would just write if ( false === $update_exe->execute()) or just if (!$update_exe->execute()).
\$\endgroup\$
1
  • \$\begingroup\$ You're definitely right about all the inconsistencies on naming and formatting I've used and all other points. I'll be using your answer as a guide/checklist from now on. Thank you for such a detailed answer, so much appreciated. \$\endgroup\$
    – Ekin
    Aug 23 '15 at 18:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.