Why is my entry being deleted even though the condition is not met?

Question

0.00/5 (No votes)

See more:

Hello guys!

I would like this form to have 3 validation levels, 2 with the codes, and one with the expiry date.

I would like to delete the entry only if the token is expired. But currently it deletes the token even if it's not expired.

Any help will be greatly appreciated!

PHP

if(!isset($_GET["selector"]) || !isset($_GET["code"])){
  exit ("The page does not exists.");
}


if(isset($_GET["selector"]) || isset($_GET["code"])){
  $selector = $_GET["selector"];
  $code = $_GET["code"];
  $sql = "SELECT * FROM reset_password WHERE selector = :selector AND code = :code";
  if($stmt = $pdo->prepare($sql)){
      $stmt->bindParam(":selector", $param_selector, PDO::PARAM_STR);
      $stmt->bindParam(":code", $param_code, PDO::PARAM_STR);
      $param_selector = $selector;
      $param_code = $code;
      if($stmt->execute()){
        $result = $stmt->fetch(PDO::FETCH_ASSOC);
        $expires = $result['expires'];
        $email = $result['email'];
        //print_r($expires);
        //print_r($email);
        if($stmt->rowCount() == 0){
          exit ("The page does not exists again.");
        }
        elseif($now >= $expires){
          print_r($now);
          print_r($expires);
          $sql = 'DELETE FROM reset_password WHERE code = :code AND email = :email';
          if($stmt = $pdo->prepare($sql)){
            // Set parameters
            $param_code = $code;
            $param_email = $email;
            $stmt->bindParam(":code", $param_code, PDO::PARAM_STR);
            $stmt->bindParam(":email", $param_email, PDO::PARAM_STR);
          }
          // Attempt to execute the prepared statement
          if($stmt->execute()){

            exit ("Token expired.");
          }
        }
      }
    }
  }

What I have tried:

If I remove the PDO part that deletes the entry from the table, the

elseif

statement alone works as it should.

PHP

elseif($now >= $expires){
    print_r($now);
    print_r($expires);
    exit ("Token expired.");
}

Posted 7-Feb-21 2:53am

InnaTS

Updated 7-Feb-21 3:42am

Richard MacCutchan

v2

Add a Solution

Comments

Richard MacCutchan 7-Feb-21 9:20am

What are the values of $now and $expires at the time that it deletes incorrectly?

InnaTS 7-Feb-21 9:25am

$now = date("Y:m:d H:i:s");

$expires = date("Y:m:d H:i:s", strtotime("+1 hour")); - the value is supposed to be fetched from the database.

Richard MacCutchan 7-Feb-21 9:26am

Those are not valid datetimes, they are simple strings. There is something wrong in your database.

InnaTS 7-Feb-21 10:48am

You were right. I have changed datetime to varchar in my database, and it fixed the issue. Thank you!

Richard MacCutchan 7-Feb-21 10:56am

That is not the right way to correct this. You should always store dates/times as DateTime types not strings. And you should not convert them to strings in order to compare them.

1 solution

Add a Solution

Add your solution here

Treat my content as plain text, not as HTML

Preview 0

…

Existing Members

Sign in to your account

...or Join us

Download, Vote, Comment, Publish.

Your Email
Password
Forgot your password?

Your Email
This email is in use. Do you need your password?
Optional Password

I have read and agree to the Terms of Service and Privacy Policy
Please subscribe me to the CodeProject newsletters

When answering a question please:

Read the question carefully.
Understand that English isn't everyone's first language so be lenient of bad spelling and grammar.
If a question is poorly phrased then either ask for clarification, ignore it, or edit the question and fix the problem. Insults are not welcome.
Don't tell someone to read the manual. Chances are they have and don't get it. Provide an answer or move on to the next question.

Let's work to help developers, not make them feel stupid.

This content, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Luc Pattyn · Accepted Answer · 2021-02-07T03:42:00

Solution 1

Hi,

in the decision phase you select on code and selector, whereas in the deletion phase you select on code and email, possibly deleting more than you intended.

Also your nesting levels are off: the second

$stmt->execute()

gets called even when the second

if($stmt = $pdo->prepare($sql))

would have failed.

BTW1: from your post, it isn't clear what type $now and $expires are. Are they timestamps, strings, ...

BTW2: there is no need to use bindParam: as you already know the value bindValue would suffice.

BTW3: the line

if(isset($_GET["selector"]) || isset($_GET["code"])){

is not quite correct (should be AND, not OR), and completely superfluous as the conditions have been checked before.

:)

Posted 7-Feb-21 3:42am

Luc Pattyn

Updated 7-Feb-21 4:26am

v5

Comments

InnaTS 7-Feb-21 11:46am

Thanks Luc!

Re. BTW1, $expires was defined as a datetime in my database. I have changed it to varchar, and it fixed the problem.

Thanks a lot for your feedback on the other issues in my code. I'll fix them too.

Regarding nesting levels, there should only be one entry with the given e-mail/ code in my database, so theoretically only one entry could be deleted. I guess that's why I never took notice of possible problems with it. Could you give me a suggestion on how to fix it? I'm really not that experienced. :)

Luc Pattyn 7-Feb-21 11:56am

IMO the deletion query should use the same WHERE clause as the discovery query, hence selector, no email.

Your second $stmt->execute() belongs inside the if($stmt = $pdo->prepare($sql)) block.

Your fix is not good. The problem may have disappeared, the code is not OK.
As Richard pointed out, datetime values in a database should use the appropriate type. Strings are for arbitrary text, a datetime is a very structured thing; a database knows how to compare datetimes; whereas string comparison does not understand e.g. month names.

With a datetime field, you could replace most all of your code with a single query, saying delete all records where code and selector match AND expires is less than NOW.

:)

InnaTS 13-Feb-21 2:06am

Thanks again! You both were right, it was a comparison issue. I have placed both dates into strtotime() to make them comparable datetimes, and it fixed the problem. :)

strtotime($now) >= strtotime($expires)