Problem with nesting if statements: Submit button won't submit the data to the database

Hello everyone!

I’ve been trying to debug this piece of code for days now. I’ve tried nesting the conditions in various ways, and then making them independent. The result would be the same. I’d get to the reset password form (through code and token validation), and when I hit Submit on the form, after which it says “The page does not exists.” - my first validation step, as if it exits on Submit. And the password isn’t updated in the database either.

When I test the execution of individual pieces of code, they do what they are supposed to do.

And then sometimes it starts redirecting me to the login page, when I type in my reset password page in the browser. And then the problem disappears on its own (- I think, because I don’t make any changes to the code). And I don’t understand why.

I thought I understood the logic of nesting if statements. But obviously, I don’t. I will need to add the code, which will remove the token ($code) from the database, after it’s been used, but without the understanding, I’m not even sure where it will need to go.

Any input would be valuable.

Thanks. :slight_smile:

<?php

// Initialize the session
session_start();

// Check if the user is logged in, if not then redirect to login page
if(!isset($_SESSION["loggedin"]) || $_SESSION["loggedin"] !== true){
    header("location: login.php");
    exit;
}
// Include config file
require_once "config.php";

// Check if the link contais code
if(!isset($_GET["code"])){
  exit ("The page does not exists.");

  //Check if the link contains the generated code
} elseif(isset($_GET["code"])){
        $code = $_GET["code"];
        $sql = "SELECT email FROM reset_password WHERE code = :code";
        if($stmt = $pdo->prepare($sql)){
            $stmt->bindParam(":code", $param_code, PDO::PARAM_STR);
            $param_code = $code;
            if($stmt->execute()){
              if($stmt->rowCount() == 0){
              exit ("The page does not exists (again).");
              }
            }
        }
}

// Define variables and initialize with empty values
$new_password = $confirm_password = "";
$new_password_err = $confirm_password_err = "";

// Processing form data when form is submitted
if($_SERVER["REQUEST_METHOD"] == "POST"){

    // Validate new password
    if(empty(trim($_POST["new_password"]))){
        $new_password_err = "Please enter the new password.";
    } elseif(strlen(trim($_POST["new_password"])) < 8){
        $new_password_err = "Password must have atleast 8 characters.";
    } else{
        $new_password = trim($_POST["new_password"]);
    }

    // Validate confirm password
    if(empty(trim($_POST["confirm_password"]))){
        $confirm_password_err = "Please confirm the password.";
    } else{
        $confirm_password = trim($_POST["confirm_password"]);
        if(empty($new_password_err) && ($new_password != $confirm_password)){
            $confirm_password_err = "Password did not match.";
        }
       }

    // Check input errors before updating the database
    if(empty($new_password_err) && empty($confirm_password_err)){
        // Prepare an update statement
        $sql = "UPDATE users SET password = :password WHERE id = :id";

        if($stmt = $pdo->prepare($sql)){
            // Bind variables to the prepared statement as parameters
            $stmt->bindParam(":password", $param_password, PDO::PARAM_STR);
            $stmt->bindParam(":id", $param_id, PDO::PARAM_INT);

            // Set parameters
            $param_password = password_hash($new_password, PASSWORD_DEFAULT);
            $param_id = $_SESSION["id"];

            // Attempt to execute the prepared statement
            if($stmt->execute()){
                // Password updated successfully. Destroy the session, and redirect to login page
                $sql = "DELETE FROM reset_password WHERE code = :code";
                session_destroy();
                echo header("location: login.php");
                exit();
            }

            // Close statement
            unset($stmt);
        }
    }

    // Close connection
    unset($pdo);
}


?>

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Reset Password</title>
</head>
<body>
    <div class="wrapper">
        <h2>Reset Password</h2>
        <p>Please fill out this form to reset your password.</p>
        <form action="<?php echo htmlspecialchars($_SERVER["PHP_SELF"]); ?>" method="post">
            <div class="form-group <?php echo (!empty($new_password_err)) ? 'has-error' : ''; ?>">
                <label>New Password</label>
                <input type="password" name="new_password" class="form-control" value="<?php echo $new_password; ?>">
                <span class="help-block"><?php echo $new_password_err; ?></span>
            </div>
            <div class="form-group <?php echo (!empty($confirm_password_err)) ? 'has-error' : ''; ?>">
                <label>Confirm Password</label>
                <input type="password" name="confirm_password" class="form-control">
                <span class="help-block"><?php echo $confirm_password_err; ?></span>
            </div>
            <div class="form-group">
                <input type="submit" class="btn btn-primary" value="Submit">
                <a class="btn btn-link" href="login.php">Cancel</a>
            </div>
        </form>
    </div>
</body>
</html>

Your intent is/should be - if a validated (trimmed, non-empty) $_GET[‘code’] value exists in the database and matches the currently logged in user, enable the display of the reset password form and allow the corresponding form processing code to be executed. You should also have a time limit on how long a reset code is good for.

Rather than to keep adding nested logic, use a program variable to remember the success/failure of the ‘code’ match query. Initialize it to a false value and only set it to a true value if the query matches a row of data. You can test this variable at the point of displaying the form and when processing the form data.

Your code has a lot of unnecessary logic and variables, making it harder to see the forest for the trees. A top down list -

  1. If you just store the user’s id in a login session variable, you only need one variable. It will either be set or it won’t be set. You don’t need an additional ‘loggedin’ variable.
  2. You need to validate all inputs before using them. In the case of $_GET[‘code’], it’s not enough just to test if it’s set, you must trim, then test if it is not empty. Also, if the code has an expected format, such as length, character range, … you should validate that too, before using the value. For what you are doing, any validation error message should be generic, so as to not give hackers useful information. You would log the specific information about each validation failure.
  3. Don’t copy variables to other variables without any purpose. If you are changing the meaning of the value in a variable, such as when trimming it, use a new variable. Otherwise, just use the original variable.
  4. Trim all the data at the same time and only do it once. I/someone will post an example showing how to do this.
  5. There’s no good reason to test the result of the ->prepare() and ->execute() calls if your code isn’t doing anything when they fail. If you use exceptions for database statement errors, and in most cases let php catch and handle the exception, you don’t need any logic in your code. Your code will only see error free execution of database statements, since execution will transfer to the nearest correct type of exception handling, which will be php. The exception to this rule is when inserting/updating user summited data and a duplicate or out of range error can occur. In this case, your code should catch the exception, detect if the error number is for something your code is designed to handle, then setup and display a message for the visitor telling them what was wrong with the data that they submitted. For all other error numbers, just re-throw the exception and let php handle it.
  6. Forget about the bindParam/bindValue statements. Instead, just supply an array of the input values to the ->execute([…]) call.
  7. If you don’t fetch all the data from a prepared query, the next query you execute will fail with an out of sync error. If you are selecting data, you should always fetch it. For a query who’s purpose is to test if a value exists, you should use a SELECT COUNT(*) … query, then fetch and test the count value.
  8. Use arrays for sets of data. Keep the submitted form data as an array, then operate on elements of the array. Use an array to hold validation errors, with the array index being form field name. This error array is also an error flag. If the array is empty, there are no errors and you can use the submitted form data. You can test and display the content of this array at the appropriate point(s) in the html document.
  9. You should name the session user id login variable ‘user_id’ or similar. A lot of things have ids, so, $_SESSION[‘id’] doesn’t clearly indicate what it is for.
  10. You didn’t actually execute the DELETE … query. Perhaps you have not finished that part of the code.
  11. Upon on successful completion of the post method form processing code, you should redirect to the exact same url of the current page to cause a get request. If you want to display a one-time success message, store it in a session variable, then test, display, and clear that session variable at the appropriate point in the html document. You should provide navigation link(s) to allow the visitor to go else where.
  12. If you leave the whole action=’…’ attribute out of the form, the form will submit to the same page. This will also cause the browser to automatically propagate any existing get parameters in the url.
1 Like

I reviewed your previous thread. The code you have in this thread is much improved, but still contains a lot of unnecessary typing. Remember these two acronyms - Don’t Repeat Yourself (DRY) and Keep It simple (KISS.) If you find yourself repeating the same things over and over, repeating logic that only differs in the variable it operates on, and are spending a lot of time typing implementation code, rather than working on the actual logic needed to accomplish a task, it is a sign you are doing things in the hardest way possible. Most of the points made in this (and the previous) thread will reduce the amount of code. The only actual increase here is in additional validation steps for the ‘code’ value.

See the following example code for this task -

<?php
// Initialize the session
session_start();

// Check if the user is logged in, if not then redirect to login page
if(!isset($_SESSION["user_id"]))
{
    header("location: login.php");
    exit;
}

// Include config file
require "config.php";

$errors = []; // an array to hold user/validation error messages
$post = []; // an array to hold a trimmed working copy of the form data
$get = []; // an array to hold a trimmed working copy of any get data

// a flag to enable the password reset logic (form, form processing) on the page
$enable_password_reset = false;

// trim the inputs
$get = array_map('trim',$_GET);

// validate the 'code' input
// Check if the link contains a code
if(!isset($get["code"]))
{
	// not set
	$errors['code'] = "The page does not exist.";
	// you can use a development/debugging function here to display/log more specific information
	// for demo purposes just echo a message, remove all occurrences when done testing
	echo 'code not set';
}
elseif($get["code"] == '')
{
	// empty string
	$errors['code'] = "The page does not exist.";
	// you can use a development/debugging function here to display/log more specific information
	echo 'code empty';
}
	// add any other validation tests here...
else
{
	// passed validation, use the input
	// to add a time limit check, you would select, fetch, and test the time limit value and display a message that the user needs to request a password reset again if it has gone past
	$sql = "SELECT COUNT(*) FROM reset_password WHERE code = ? AND user_id = ?";
	$stmt = $pdo->prepare($sql);
	$stmt->execute([
		$get['code'],
		$_SESSION['user_id']
	]);
	if(!$stmt->columnFetch()) // test the COUNT(*) value
	{
		// no matching row found
		$errors['code'] = "The page does not exist.";
		// you can use a development/debugging function here to display/log more specific information
		echo 'code not found in db';
	}
	else
	{
		// a row was found
		$enable_password_reset = true;
	}
}

// Processing form data when form is submitted
if($_SERVER["REQUEST_METHOD"] == "POST" && $enable_password_reset)
{
	// trim all the inputs
	$post = array_map('trim',$_POST);
	
	// Validate new password
	if($post['new_password'] == '')
	{
		$errors['new_password'] = 'Please enter the new password.';
	}
	elseif(strlen($post["new_password"]) < 8)
	{
		$errors['new_password'] = 'Password must have at least 8 characters.';
	}

    // Validate confirm password
	if($post['confirm_password'] == '')
	{
		$errors['confirm_password'] = 'Please enter the confirmation password.';
	}
	
	// if no errors, compare new and confirmation password
	if(empty($errors['new_password']) && empty($errors['confirm_password']) && $post['new_password'] !== $post['confirm_password'])
	{
		$errors['confirm_password'] = 'Confirm password does not match the password.';
	}
	
	// if no errors, use the inputs
	if(empty($errors))
	{
		// Prepare an update statement
		$sql = "UPDATE users SET password = ? WHERE id = ?";
		$stmt = $pdo->prepare($sql);
		$stmt->execute([
			password_hash($new_password, PASSWORD_DEFAULT),
			$_SESSION["user_id"]
		]);

		$sql = "DELETE FROM reset_password WHERE code = ? AND user_id = ?";
		$stmt = $pdo->prepare($sql);
		$stmt->execute([ $get['code'],$_SESSION['user_id'] ]);
		//session_destroy(); // are you really sure you don't want to just log the user out...
		unset($_SESSION['user_id']);
	}
	// if no errors, success
	if(empty($errors))
	{
		// redirect to the exact same url of this page to cause a get request - PRG Post, Redirect, Get.
		header("Refresh:0");
		exit;
	}
}
?>
<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Reset Password</title>
</head>
<body>
<?php
// display any errors that are not specifically displayed in the form
if(!empty($errors['code']))
{
	// add any css styling you want...
	echo "<p>{$errors['code']}</p>";
}
?>

<?php if($enable_password_reset): ?>
    <div class="wrapper">
        <h2>Reset Password</h2>
        <p>Please fill out this form to reset your password.</p>
        <form method="post">
            <div class="form-group <?php echo !empty($errors['new_password']) ? 'has-error' : ''; ?>">
                <label>New Password</label>
                <input type="password" name="new_password" class="form-control" value="<?php echo $post['new_password'] ?? ''; ?>">
                <span class="help-block"><?php echo $errors['new_password'] ?? ''; ?></span>
            </div>
            <div class="form-group <?php echo !empty($errors['confirm_password']) ? 'has-error' : ''; ?>">
                <label>Confirm Password</label>
                <input type="password" name="confirm_password" class="form-control">
                <span class="help-block"><?php echo $errors['confirm_password'] ?? ''; ?></span>
            </div>
            <div class="form-group">
                <input type="submit" class="btn btn-primary" value="Submit">
                <a class="btn btn-link" href="login.php">Cancel</a>
            </div>
        </form>
    </div>
<?php endif; ?>
</body>
</html>
1 Like

Hi phdr!

Wow, thanks a lot for reviewing my code, and most of all, for the valuable tips on how to structure it in general. I can see, I’ll have to implement those tips, and simplify the code in the other pieces of my login system, which will give me some needed practice. :slight_smile:

It’s amazing, how easy it is to expose your database or site to hackers, if you don’t know what you are doing. :see_no_evil: I’ve got lots of learning and practice to do. :slight_smile:

Thanks again for your help, I greatly appreciate it!!! :pray: :blush:

Hi phdr! I have now tried the suggested example of the code, but I get the exact same error - the password is not being updated in the database. Neither the ‘code’ is deleted from the database. I have 2 tables in the same database, one with the user info and another for password reset. They both have id, but obviously id for the same e-mail address would be different in the two tables. You suggest to name the variable ‘user_id’. In which table should I change it to ‘user_id’? I’m guessing that’s the part where things might go wrong?

I managed to fix the error in my old code, where it wouldn’t show my form, and now I get the exact same error in my old and your suggested code - the password won’t update.

Do you have php’s error_reporting set to E_ALL and display_errors set to ON, preferably in the php.ini on your system, but you can put them into your code if it looks like your code is actually running, so that php would help you by reporting and displaying all the errors it detects? Also, when you make the PDO database connection, are you setting the error mode to exceptions so that you would have error handling for all the database statements that can fail?

If you are getting one of the ‘page does not exist’ messages and from my code one of the extra echoed message with more information, it means that the expected input didn’t pass validation. Which extra message did you get from my code?

I don’t get the ‘page doesn’t exists’ error in your code. And I have also managed to fix it in my version.

I’ve just set php’s error_reporting set to E_ALL and display_errors set to ON in php.ini file. I still don’t get any error messages. It just looks like I refresh the page when I click Submit button, and nothing else happens.

I’ve always gotten errors like " Parse error : syntax error, unexpected end of file in /…Login/reset-password.php on line 131", if that’s what you mean?

When I commented out the $code validation part in my code, the password did update. But once the $code validation is added, it isn’t updated anymore.

Sponsor our Newsletter | Privacy Policy | Terms of Service