Improvements To Member Registration Site Reg.php

Thanks for bringing it to my attention. And no, I haven’t read the final post there (as of now, post 11). But I am doing so now. you keep tabs too. Lol! :slight_smile:

Folks!

Here’s the updated script. It was edited by another volunteer as my code was messy with unnecessary stuffs (so I learnt).
If anyone wants to add anything then you are welcome.
It’s an account activation script: account_activation.php.
When the user clicks the “account activation” link, emailed to him, to verify his email address then this script takes over.
It checks whether the email address and account activation serial number is pending registration or not. If any of the 2 mismatch then user gets error and is forwarded to registration.php to register for an account. If they match then the script changes the appropriate row (“account_activations” column in mysql tbl) to “1” from “0”. “0” was set during registration by the registration script. “0” means account not activated yet. Pending registration.
Finally, script forwards the user to his homepage.php (logged-in page). User no longer needs to log-in. He gets auto logged-in via his username that related to the account activation link.
Simple.
Kindly, give me feed-back if the script can be improved or not for simplicty’s sake or security:


<?php
session_start();
include 'config.php';


if (!isset($_GET["email"], $_GET["account_activation_code"]) === true){
    $_SESSION['error'] = "Invalid Email Address! Invalid Account Activation Link! This email is not registered! Try registering an account if you do not already have one! <a href=\"register.php\">Register here!</a>";
    exit();
} else {
    $stmt = mysqli_prepare($conn, "SELECT usernames, account_activations FROM users WHERE emails = ? AND accounts_activations_codes = ?");
    mysqli_stmt_bind_param($stmt, 'si', $_GET["email"],  $_GET["account_activation_code"]);
    mysqli_stmt_bind_result($stmt, $username, $userActivationState);

    if (mysqli_stmt_execute($stmt) && mysqli_stmt_fetch($stmt)){
        if ($userActivationState != 0){
            echo "Since your account is already activated, why are you trying to activate it again ? Do not do that again and just login from <a href=\"login.php\">this webpage</a> next time! Make a note of that webpage, ok ?";
            exit;
        }

        $userActivationState = 1;
        $stmt = mysqli_prepare($conn, "UPDATE users SET account_activations = ? WHERE usernames = ?");
        mysqli_stmt_bind_param($stmt, 'is', $userActivationState, $username);
        if (mysqli_stmt_execute($stmt)){
            echo "<h3 style='text-align:center'>Thank you for your confirming your email and activating your account.<br /> Redirecting you to the login page ...</h3>";

            $_SESSION["user"] = $username;

            header("location:home.php");
            exit;
        }
    } else {
        $email = htmlspecialchars($_GET['email']);
        $code = htmlspecialchars($_GET['account_activation_code']);
        echo "Invalid Email Address or Invalid Account Activation Link! This Email $email was not pending registration with this Account Activation Code $code!
        Try registering an account if you have not already done so! <a href=\"register.php\">Register here!</a>";
        exit;
    }
}

PS - If you want to see what my own version looked like that got edited then head-over to post 133. I am glad I got a very short edited one. Now, gonna be busy trying to memorize the edited code. :slight_smile:

htmlspecialchars is used to escape outgoing html. It makes no sense at all to use it on incoming data. Take a look at the php input filters and validation functions.

And just a suggestion, set yourself up an account on github and start using it to manage your source code. Makes it considerably easier to track changes to your code.

2 Likes

Thanks. I was wondering why the editor used htmlspecialchars here. Might aswell ask him. :wink:

But I have no experience with github. Where do I signup ? I can google but a different section’s reg form might popup and I might waste time dealing with that. So, which one you use yourself ?
So github. It is like a forum only it will list all my updates to make it easy for others to contribute their snippets ?

PS - Remember, I’m a complete newbie and so sill don’t know all the major tools you pros use.
Only found out about NotePad++ a year ago. Else, would have simply used MS Note Pad instead and my site’s text editor to test my codes.

I was told this exactly 14 days from now by the editor:

Escape your inputs before echoing them out into your error message. The script tags aren’t really needed, but if you want to use them make sure you properly escape the input for use in scripts also.

What is your feed back now ? :slight_smile:

That is exactly what @ahundiak is telling you, escape outgoing html not incoming.

This https://en.wikipedia.org/wiki/Github and this: https://www.sitepoint.com/screencast-difference-git-github/ might help.

https://github.com/

1 Like

Sam,

I know what Ahundiak is saying. He is saying I should use htmlspecialchars to escape outgoing html and I should not use it on incoming data. But, I said that, that part of the code was not built by me but another volunteer. 14 days ago, when he edited, he stated this:

Escape your inputs before echoing them out into your error message. The script tags aren’t really needed, but if you want to use them make sure you properly escape the input for use in scripts also.

I just answered Ahundiak’s question with the editor’s 14 day old statement (advice) to me.
he escaped the input before outputting and gave a reason why he did it.

My original code looked like this at the end:


else 
	{
		//Give error that this email address (from where the user is clicking the account activation and email confirmation link) is not pending registration. Provide the unregistered user the registration link.
		echo "<script>alert('Invalid Email Address or Invalid Account Activation Link! This Email $confirming_email was not pending registration with this Account Activation Code $account_activation_code! Try registering an account!')</script>";
		echo "Invalid Email Address or Invalid Account Activation Link! This Email $confirming_email was not pending registration with this Account Activation Code $account_activation_code! 
		Try registering an account if you have not already done so! <a href=\"register.php\">Register here!</a>";
    	$conn->close();
		exit();	
	}

The kind volunteer changed it to this:


} else {
        $email = htmlspecialchars($_GET['email']);
        $code = htmlspecialchars($_GET['account_activation_code']);
        echo "Invalid Email Address or Invalid Account Activation Link! This Email $email was not pending registration with this Account Activation Code $code!
        Try registering an account if you have not already done so! <a href=\"register.php\">Register here!</a>";
        exit;
    }

Oh well, here’s my original code and here is the editor’s update …

My original security adding attempt code (unworthy code):


<?php
session_start();
include 'config.php';


//Grab User's (account activator's) email and account activation code from account activation link's url. Check for email and account activation code details in the account activation link's url.
if(!isset($_GET["email"], $_GET["account_activation_code"]) === TRUE)
{
	$_SESSION['error']="Invalid Email Address! Invalid Account Activation Link! This email is not registered! Try registering an account if you do not already have one! <a href=\"register.php\">Register here!</a>";
	exit();
}
else
{
	$confirming_email = trim(mysqli_real_escape_string($conn,$_GET["email"])));
	$account_activation_code = trim(mysqli_real_escape_string($conn,$_GET["account_activation_code"])));
		
	/*
	Check User's Confirmed Email and Account Activation Code against the "users" tbl to see if it has already been registered or not. 
	Do this by selecting the Confirmed Email and Account Activation code to check against Mysql DB if they match or not.
	*/
	$stmt = mysqli_prepare($conn, "SELECT emails, accounts_activations_codes FROM users WHERE emails = ? AND accounts_activations_codes = ?");
	mysqli_stmt_bind_param($stmt, 'si', $confirming_email, $account_activation_code);
	mysqli_stmt_execute($stmt);
	
	/* 
	If the account activation code matches with the confirming Email in the same row in the MySql DB then check if user has already activated his account or not.
	Check if the associated row is 0" or "1". Must be "0" to indicate account activation is pending.
	*/
	if (mysqli_stmt_insert_id($stmt)) 
	{	
		while($row = mysqli_fetch_assoc($result)) 
			{	
		        $db_username = $row["usernames"];
				$db_confirmed_email = $row["emails"];
				$db_account_activation = $row["account_activations"];
				
				//If "account_activation" row shows "not equal to 0 (is: 1)", then show error indicating account has already been activated. Then re-direct user to Log-in Page.
				if($db_account_activation != 0)	
				{
					echo "<script>alert('Since your account is already activated, why are you trying to activate it again ? Do not do that again and just login!')</script>";
					echo "Since your account is already activated, why are you trying to activate it again ? Do not do that again and just login from <a href=\"login.php\">this webpage</a> next time! Make a note of that webpage, ok ?";
					$conn->close();
				}
				else
				{
					//Dump the account confirming User's details onto the same row in the "users" table.
					if (mysqli_stmt_insert_id($stmt)) 
					{	
				        // Are lines 42 to 48 (next 5 lines) really necessary ?
						$stmt = mysqli_prepare($conn, "SELECT usernames, emails, account_actvations FROM users WHERE usernames = ? AND emails = ? AND account_activations_codes = ?");
						mysqli_stmt_bind_param($stmt, 'ssi', $username, $email, $account_activations_code);
						mysqli_stmt_execute($stmt);
						$result = mysqli_stmt_get_result($stmt);
						
						// Update 'account_activation' row to '1' to indicate account and email has now been confirmed.
						$stmt = mysqli_prepare($conn, "UPDATE users SET account_activations = ? WHERE emails = ? AND account_activation_codes = ?";
						mysqli_stmt_bind_param($stmt, 'isi', 1, $db_confirmed_email, $account_activations_code);
						//Execute the statement.
						mysqli_stmt_execute($stmt);
						
						//If statement execution a success then create a session under the user's Username.
						if (mysqli_stmt_insert_id($stmt)) 
						{
							echo "<h3 style='text-align:center'>Thank you for your confirming your email and activating your account.<br /> Redirecting you to the login page ...</h3>";
		
							$_SESSION["user"] = $db_username;
					
							//Redirecting the newly account activated user to his/her account homepage by identifying the user by his/her session name (username).
							header("location:home.php");
						}		
					}
				}
			}		
	}
	else 
	{
		//Give error that this email address (from where the user is clicking the account activation and email confirmation link) is not pending registration. Provide the unregistered user the registration link.
		echo "<script>alert('Invalid Email Address or Invalid Account Activation Link! This Email $confirming_email was not pending registration with this Account Activation Code $account_activation_code! Try registering an account!')</script>";
		echo "Invalid Email Address or Invalid Account Activation Link! This Email $confirming_email was not pending registration with this Account Activation Code $account_activation_code! 
		Try registering an account if you have not already done so! <a href=\"register.php\">Register here!</a>";
    	$conn->close();
		exit();	
	}
}

?>

The volunteer/editor’s update of my code:


<?php
session_start();
include 'config.php';


if (!isset($_GET["email"], $_GET["account_activation_code"]) === true){
    $_SESSION['error'] = "Invalid Email Address! Invalid Account Activation Link! This email is not registered! Try registering an account if you do not already have one! <a href=\"register.php\">Register here!</a>";
    exit();
} else {
    $stmt = mysqli_prepare($conn, "SELECT usernames, account_activations FROM users WHERE emails = ? AND accounts_activations_codes = ?");
    mysqli_stmt_bind_param($stmt, 'si', $_GET["email"],  $_GET["account_activation_code"]);
    mysqli_stmt_bind_result($stmt, $username, $userActivationState);

    if (mysqli_stmt_execute($stmt) && mysqli_stmt_fetch($stmt)){
        if ($userActivationState != 0){
            echo "Since your account is already activated, why are you trying to activate it again ? Do not do that again and just login from <a href=\"login.php\">this webpage</a> next time! Make a note of that webpage, ok ?";
            exit;
        }

        $userActivationState = 1;
        $stmt = mysqli_prepare($conn, "UPDATE users SET account_activations = ? WHERE usernames = ?");
        mysqli_stmt_bind_param($stmt, 'is', $userActivationState, $username);
        if (mysqli_stmt_execute($stmt)){
            echo "<h3 style='text-align:center'>Thank you for your confirming your email and activating your account.<br /> Redirecting you to the login page ...</h3>";

            $_SESSION["user"] = $username;

            header("location:home.php");
            exit;
        }
    } else {
        $email = htmlspecialchars($_GET['email']);
        $code = htmlspecialchars($_GET['account_activation_code']);
        echo "Invalid Email Address or Invalid Account Activation Link! This Email $email was not pending registration with this Account Activation Code $code!
        Try registering an account if you have not already done so! <a href=\"register.php\">Register here!</a>";
        exit;
    }
}

There is still a lot of unnecessary stuff, mostly due to the logical errors. Some code parts will never be executed.

Instead of asking random people to fix your code, you have to test it. Yourself.
there are different scenarios covered in this code, but I am sure you never tried any of them beside, probably, a correct one.

You should sit behind your computer and test every scenario and see if it works as intended.

3 Likes

Actually, I was wrong. He is outputting the escaped values. The code is just so convoluted with all the else statements that I misunderstood what was going on.

1 Like

Ok. I will be doing this tonight.
But, where exactly is the logical error that you state ?
Best to sort it out asap and not spend hrs testing a script that has these errors. Do you agree ?

Anyone else can point-out any logical errors in my code ?

You have this the wrong way around, you use the testing to find the errors, that’s the whole point of testing.

4 Likes

What is indeed best for you is to start making yourself familiar with “your” code.

What are you doing now is a road to nowhere. It’s like cheating in a game. Your goal now is not the working code but understanding. The ability to write a code yourself. You cannot make it that way for the rest of your life - asking other people writing a code for you. You are supposedly learning here. So you shouldn’t ask how to fix this code. You should try to understand what does this code do. And of course in the process you may ask other people. But getting the working code on a golden plate will do you no good, as you will have no idea how it works.

Errors I mentioned are no rocket science. It’s an elementary school logical statements like “if Joe goes to the cinema and there is Star Wars in display, then Joe is going to watch Star Wars”.

So I suppose you at least have an idea what should your program do with different scenarios, ie user already activated or wrong activation code or anything else programmed in the cod above. So just sit and test every scenario. I hope you’re able to do it without asking a hundred people on a dozen forums to give you a hand.

5 Likes

Like I said. I don’t see any logical errors in my script. But if you think there is then you’re welcome to point it/them out. I’m not asking you to hand me over the code. Just point out to me where the logical error is.
And as for the testing. I will do that tonight. And, check all the error codes (if any do popup).

But you don’t see errors, you run your script and find out what errors there are.

1 Like

Hint: start by seeing what happens if no email is submitted.

2 Likes

Depends.
If your goal is to get a working PHP application for free, by means of making other people writhe all the code for you, under the pretext you are “learning PHP”, then surely it would be best to make these people fix all errors asap.

You always see things in a negative way and jump to conclusions and accusations. I never asked you to build my code or script. Nor fix it or edit it.

You said I have logical errors in my script. Remember ? I told you I can’t see any. In such a situation, if you keep insisting there are then any Tom, Dick and Harry would then have no choice but to ask you where the “supposed error” is if they can’t see it themselves. I have done that. That is human nature.

Where did I ask you to fix my code for me where your supposed logical error exists ? I only said to point-out the lines where you claim the errors are in order to bring it to my attention so that I can then review it. That way, I get a chance to see the error for myself and fix it. I can also make note of my error so that it does not happen again. If that is not learning for a student from work experience then I don’t know what could be called “work experience”.

You are reading into too many things, pal. You should learn to trust people and see things in a positive way. Innocent until proven guilty. Live by that. Rather than believe: Guilty unless proven innocent.
Look at how others here have replied. Gandalf458 and Ahundiak. Hinted how I should spot any “logical errors” without jumping to any silly or childish conclusions. Some even suggest what error codes to place in my files. That is work experience for me, mate. In my book, every little I can get bit by bit from seniors is called work experience.
You should think along their lines. Other members’ lines. These are matured people not only in programming but human intelligence too and for sure know how to communicate appropriately and promptly with fellow humans without offending them.

Anyway, I hope you’ll see things in a different light after this. And, no offence.

Take care and have a nice weekend!

I tested my update: account_activation.php.

I did not bother getting the account activation link emailed as to do that I need to register again.
I just checked mysql db what was pending (from a registration a fortnight or so ago that is still pending) and grabbed the user’s email and account activation code and then loaded the account activation link:

http://www.localhost/id/activate_account_edited.php?email=EDITED@gmail.com&hash=$2y$10$AtFLJ6DuE/rq.AXFq3zFfOd2r.NIi7LeT4/dXHRn8dF

And instead of seeing a notice that the accouint has been activated (email verified), I see an error instead:

Notice: A session had already been started - ignoring session_start() in C:\xampp\htdocs\eid\config.php on line 21

I hit the logout.php and destroyed the session and then reloaded the link

http://www.localhost/id/activate_account_edited.php?email=EDITED@gmail.com&hash=$2y$10$AtFLJ6DuE/rq.AXFq3zFfOd2r.NIi7LeT4/dXHRn8dF

hoping to get the mssg that the account has been activated. But same result over and over again.

Anyway, FYI colshrapnel, I built the following code few wks ago all by myself and it was working. But, it did not have sql prevention. Hence, updating it to the script you saw mentioned on one of my previous posts.


<?php
session_start();
require "conn.php";

    //Grab account activator's email and account activation code from account activation link's url.
	
if(isset($_GET["email"], $_GET["account_activation_code"]) === true) 
{
	$confirmed_email = trim($_GET["email"]);
	$account_activation_code = trim($_GET["account_activation_code"]);
	
	$confirmed_email = mysqli_real_escape_string($conn,$confirmed_email);
	$account_activation_code = mysqli_real_escape_string($conn,$account_activation_code);
		
	
    //Grab User details from table "pending_users". Search data with confirmed Email Address.
	
    $query = "SELECT * FROM pending_users WHERE Email = '".$confirmed_email."'";
    $result = mysqli_query($conn,$query);
	$numrows = mysqli_num_rows($result);
	if($numrows != 0)
    {		
        while($row = mysqli_fetch_assoc($result)) 
	    {	  
	        $db_id = $row["Id"];
	        $db_username = $row["Username"];
	        $db_password = $row["Password"];
	        $db_email = $row["Email"];
			$db_forename = $row["Forename"];
			$db_surname = $row["Surname"];
			$db_account_activation_code = $row["Account_Activation_Code"];
		    $db_account_activation = $row["Account_Activation"];		    
	    
		    if($db_account_activation != 0)	
		    {
	            echo "<center>Since your account is already activated, why are you trying to activate it again ?</center>";
		        $conn->close();
		        exit();  
		    }
		    else 
		    {            
			    echo "Your email $confirmed_email has now been confirmed!";
				
				$user = $db_username;
	            $userid = $db_id;
                $_SESSION["user"] = $user;
			    
		        $conn->query("UPDATE pending_users SET Account_Activation 1 WHERE Email = '".$confirmed_email."'");		
		        echo "Activating your account! Wait to be auto-logged-in to your account as that will be the sign that your account has been activated.";
		
		        //Create table under $user to hold user account activity data.

                $sql = "CREATE TABLE $user(
                id INT(6) UNSIGNED AUTO_INCREMENT, PRIMARY KEY, 
                Username varchar(30) NOT NULL,
		        Email varchar(50) NOT NULL,
                Forename varchar(30) NOT NULL,
	            Surname varchar(30) NOT NULL,
	            Password varchar(32) NOT NULL,
	            Profile_Pic (longblob) NOT NULL,
	            Bio varchar(250) NOT NULL,
	            Status varchar(100) NOT NULL)";
	 
	            if($conn->query($sql)===TRUE)
		        {
	                echo "<center>table $user created!</center>";
                }
		        else 
		        {
                    echo "<center>table $user creation failed!</center>";
		            $conn->close();
		            exit();
	            }
	
    
	            //Copy $user's registration data from table "pending_users" to table users.
	
                $sql_1 = "INSERT INTO users(Username,Password,Email,Forename,Surname,Account_Activation_Code) VALUES('$username','$password','$email','$forename','$surname','$account_activation_code')";

	            if($conn->query($sql_1)===TRUE)
	            {
	                echo "<center>inserted data into table $user!</center>";
                }
	            else
	            {	
		            echo "<center>inserting data into table $user failed!</center>";
		            $conn->close();
		            exit();
	            }

	            //Copy $user's registration data from table "pending_users" to table $user.
	
                $sql_2 = "INSERT INTO $user(Username,Password,Email,Forename,Surname,Account_Activation_Code) VALUES('$username','$password','$email','$forename','$surname','$account_activation_code')";

	            if($conn->query($sql_2)===TRUE)
	            {
	                echo "<center>inserted data into table $user!</center>";
                }
	            else
	            {	
		            echo "<center>inserting data into table $user failed!</center>";
		            $conn->close();
		            exit();
	            }
	
                //Redirect newly activated user to account homepage.

                header("location:home.php");
            }
		}
	}
	else
    {
        echo "<script>alert('Invalid Email Address! Invalid Account Activation Link! This email is not registered! Try registering it!')</script>";
        $conn->close();
    }	
}
?>

As you can see, the old version with the sql prevention none, it was lengthy.

It is interesting that you are vastly eloquent and resourceful when it takes to talking other people into doing something for you. But when it takes to doing something with your own hands, your attitude changes dramatically: it becomes very passive, all you can do is to state your problem, without any attempt to solve it.

There are several tests you had to perform. You did one attempt, that failed (entirely due to your laziness and negligence). Not trying to fix a silly typo in your lazy test. Not trying to “bother” with a proper test. Not trying other tests. All you can do is come here and tell us your problem. Come on, can you do something all by yourself? This is not even coding. It’s just running your own application, according to your own design.

Why some stranger should tell you all that?
Do you care for your program at all?