Cleaned up the formatting of the original post.
Now that you have your code “working”, post the latest version and we will help get you to the next level.
Ok so here is my new code and I cleaned it up to make it easier to read. The only problems I am getting is that the Username length checker doesn’t work right and the Alphanumeric doesn’t work either, also when I remove these two codes to test if my information enters into the database it does not go into the table it just goes to index.php
if (isset($_POST["_Submit"])) {
$Username = mysqli_real_escape_string(strip_tags(stripslashes($_POST['_Username'])));
$Password = mysqli_real_escape_string(strip_tags(stripslashes($_POST['_Password'])));
$ConfirmPassword = mysqli_real_escape_string(strip_tags(stripslashes($_POST['_ConfirmPassword'])));
$Email = mysqli_real_escape_string(strip_tags(stripslashes($_POST['_Email'])));
function is_alphanumeric($Username)
{
return (bool)preg_match("/^([a-zA-Z0-9])+$/i", $Username);
}
if (!$_POST['_Username']||!$_POST['_Password']||!$_POST['_ConfirmPassword']||!$_POST['_Email']) {
echo "<b>Please fill in all required fields.</b>";
} else {
$userExist = mysqli_query($conn, "SELECT * FROM Users WHERE Username='$Username' OR OriginalName='$Username'");
$userExist = mysqli_num_rows($userExist);
if ($userExist > 0) {
echo "<div id='Error'>That username already exists.</div>";
} else if ($ConfirmPassword != $Password) {
echo "<div id='Error'>Your password and confirm password does not match.</div>";
} else if (strlen($Username)>15||strlen($Username)<3) {
echo "<div id='Error'>Your username needs to be 3 to 15 characters!</div>";
} else if (!is_alphanumeric($Username)) {
echo "<div id='Error'>Only A-Z and 1-9 is allowed, or there is profanity in your username.</div>";
} else {
$_ENCRYPT = $Password;
$IP = $_SERVER['REMOTE_ADDR'];
mysqli_query($conn, "INSERT INTO Users (Username, Password, Email, IP) VALUES ('$Username', '$_ENCRYPT', '$Email', '$IP')");
$_SESSION['Username']=$Username;
$_SESSION['Password']=$_ENCRYPT;
header("Location: index.php");
}
}
}
}
Addon - I also believe the code is not pulling the information from the form.
Ok, on to the “real” problems…
-
Hoping the name of a button to be submitted in order for your script to work will completely fail in certain cases. You need to check the REQUEST METHOD.
-
You need to use Prepared Statements. That will also eliminate all that escaping code.
-
Do not ever slap functions in the middle of a code block.
-
You need to trim the POST array and then check for empty on required fields.
-
Do not check if a username is already taken. Set a unique constrain on the column, then attempt the insert and capture the duplicate error if any. This is one of the few times you should use Try/Catch.
-
Get rid of the variable for nothing $_ENCRYPT and use password_hash and password_verify.
-
Never ever put variables in your queries. Use Prepared Statements
-
Kill the script after a header redirect with die or exit. In this case nothing follows so it doesn’t really matter.
-
Use a proper IDE with syntax highlighting and you wont miss simple errors like the extra curly brace in your code.
-
I would suggest you use a more easily readable code format style. I use Whitesmiths myself.
Thank you benanamen for the help, I will post a reply as soon as I get a chance to do these few things and hopefully get my code working and in good condition.
You need to set php’s error_reporting to E_ALL and display_errors to ON, preferably in the php.ini on your system, so that php will help you by reporting and displaying all the errors it detects. You will save a TON of time while learning, developing, and debugging code/queries.
The reason it doesn’t seem that you are getting any form data is because the …_escape_string() statements you have in your code are incorrectly coded and you would be getting php errors to alert you to the problem.
However, when you switch to use prepared queries (and the php PDO extension), the …_escape_string() statements are no longer need and can be removed.
Here’s a list of issues with this code (repeats some of the items already mentioned in the thread) -
- Detect that a post method form was submitted before referencing any of the form data.
- If they are not already, your form and form processing code should be on the same page. This will let you display the validation errors when you re-display the form and repopulate the form field values with the submitted data so that the user doesn’t need to keep reentering the same data.
- Use an array to hold validation errors. The array is also an error flag. If the array is empty, there are no errors. If the array is not empty, there are errors.
- Forget about using striplsashes (not needed after php 5.5, released in 2012) and strip_tags (< and > should be allowed in usernames, passwords, and are actually valid in the name part of email addresses.)
- Forget about using any …_escape_string() function. You are also using them incorrectly. This is just a lot of code that can still allow sql injection if you haven’t set the character set that php is using to match the character set your database tables are using. You should be using prepared queries, which will result in the simplest code and the simplest sql query syntax. (If you did have a case where you were using …_escape_string() on data, you would do it right before using the data in a query. By doing it before validating the data, you are altering the values and changing what is getting validated.)
- You should be using the much simpler and more consistent php PDO extension.
- Trim all the input data at once so that you can detect if all white-space characters were entered. You can do this will one line of code (and a simple call-back function.)
- Validate all the input data at once (your nested conditional logic is only validating one input at a time, requiring multiples form submissions to validate all the inputs.) For a registration script, in addition to first validating that ‘required’ fields are not empty, you would validate the content/format/length… of the data as appropriate for the type of data.
- If here are no validation errors, use the submitted data. Any comparison (the password and confirm password) or other use of the input data must come after it has been validated.
- Rather than try to run initial SELECT query(ies) to find if data already exists, which has a race condition with concurrent users (or the same user submitting the form multiple times) that will allow same values to be used, just insert the data and detect any duplicate key error from the query (requires that columns holding unique values be set up as unique indexes - you should be doing this anyway so that the database enforces uniqueness.) If you detect a duplicate key error, if you have multiple columns that are unique, you can run a SELECT query to find which column values where duplicates. This will also reduce the amount of queries, since you only run a/one SELECT query if a duplicate key was detected.
- Php’s password_hash() and password_verify() should be used for hashing and testing the hashed password.
- The only value your login code should store in a session variable is the user id (auto-increment column in database table.) If you are doing this for real, your registration code would not automatically login the user. You would have an email verification system to activate the user’s account as the next step in the process and then require the user to enter the username and password to login.
- Any header() redirect needs an exit; statement after it to prevent the remainder of the code on the page from running.
- You need error handling for all your database statements (connection, query, prepare, execute.) The easiest way of adding error handling, without needing to add program logic, is to enabled exceptions for errors for the database extension you are using and let php catch the exceptions where it will use its error_reporting, display_errors and log_errors settings to control what happens with the actual error information. The exception to this is item #10 in this list, where your code needs to catch the exception and handle any duplicate key error.
Doing these things will greatly simplify the code/queries and improve the User eXperiance (UX.) They will also result in code that either works or it will tell you why it isn’t working.
Do a tutorial on prepared statements in mysqli and pdo.
After you’re sure you know how to do them properly, just stop using manually written sql strings for queries 97% of the time by using an ORM or Query Builder.
I found these to be very noob friendly for small projects: http://j4mie.github.io/idiormandparis/
The less actual sql syntax you have to write, the happier you’ll be.
I’ll agree and disagree with this. You should not use an exception for something that you expect to happen, that isn’t an exception, it is guiding logic by exception. You should check if the username exists AND have the constraint on the column for data integrity.
Why is that? I enjoy SQL and live in databases. ORM’s while useful, are also bloated and do their own thing. You can’t really tune an ORM query.
That’s why i said 97% instead of 100%.
I’ve seen countless posts over the years where people have a very large form and they’re manually writing the SQL (whether it be bound parameters or string concatenation) and they’re running into bugs caused by a tiny mistake in the SQL syntax or missing one of the many fields from their form due to a typo. In those situations it’s so much easier to pass the entire post array to a class and hit ->save()
Sorry boss, but that is incorrect. You do not check if the username exists. By doing so you create a race condition whereby simultaneous requests will all get the OK to insert but only one request will actually be entered into the DB, all the others will fail even though the username check passed. The correct and error proof method is to set a unique constraint on the column, attempt the insert and catch the duplicate error if there is one.
In your opinion it is not correct. It is incorrect to use exceptions, that aren’t actually exceptions, to drive application logic.
A duplicate constraint error is not an exception?
Please tell me then how you will avoid creating a race condition with your method.
It is not an opinion. Code execution proves it out.
http://wiki.c2.com/?DontUseExceptionsForFlowControl
I didn’t say don’t use the constraint, I said don’t use it as the flow control method. What race condition are we talking about, the likelihood that two [or more] people are going to use the same value with in milliseconds?
Constraint wasn’t an issue.
A race condition is not based on likelihood. It is in fact simultaneous requests for the same thing which absolutely can and does happen. Sure likelihood of it happening in a low traffic app is small, but it can still happen. Do you really want to advocate building in an application bug based on likelihood?
The specific race condition is inserting a value that someone else is also trying to insert. We are not talking about a dirty vs clean record. A race condition of someone entering a value and validating it does not have presence is not an large issue. Many apps use AJAX to validate a username before it is submitted, it’s the same concept. Same thing when you want to buy a domain name. They don’t submit the name then wait to see if it exists first. They check to see if it is available, and if it is they allow the process.
You are advocating an anti pattern and I am not. Simple as that. The part where you say the anti pattern is more correct is wrong in itself. But, we can agree to disagree.
Large issue or not depends on the scale of your application. At worst it is less then a favorable user experience. The point is, checking first builds in the capability for a race condition. The constraint stops dirty data of course, so it relates to the user experience.
Sure, many apps do the username check first by whatever method. Many apps also use MySQL_* and put variables in the queries. That doesn’t make it ok.
In any of those cases, a potential race condition affecting the user experience is built into the app. There is no way it cannot be. There can only be one person with a particular username or domain name no matter how many people passed the availability check. The first request to make it to the server gets the name. In PHP, an Exception is thrown all by itself without a programmer throwing it. How it is handled is something else. Without seeing the code I couldn’t say, such as the case of domain names.
Unless I am mistaken, what you are talking about as anti-pattern or Exceptions for program flow doesn’t apply. That is the programmer throwing the exceptions. That is not the case here.
A duplicate constraint violation is an Exception in PHP all by itself so your previous statement about “actually aren’t exceptions” is incorrect.
Let’s just take it to code, as it always says what is what. We already agree on the duplicate constraint in place. Lets take 100 people that want the username “BOB” which is not currently in the DB. A username check will tell ALL 100 people they can have the name “BOB”. They all submit at the same time. The race is on. Only one of them (race winner) gets the username registered to them, the other 99 have a failure EVEN THOUGH the app said the name was available.
Tell me, how would you, @astonecipher solve this problem?
$sql = "SELECT COUNT(*) FROM Member WHERE email = ?";
$stmt = $this->_pdo->prepare($sql);
$stmt->execute([$data['email']]);
if($stmt->rowCount() == 1)
return -1;
$sql = "INSERT INTO Member (first_name, last_name, email) VALUES (?,?,?)";
$stmt = $this->_pdo->prepare($sql);
$stmt->execute([$data['first_name'],
$data['last_name'],
$data['email']]
);
return $this->_pdo->lastInsertId();
I don’t see a race condition being an issue for this. And it is the same practice used in high availability systems with 1m + users a week that I have worked on, though not in PHP.
There are two different types of used name checks. One is the kind is like what the Ajax and Domain name checks do. They check the name and give the user an Available/Not Available response without attempting the insert. (A double process to complete, 1.check username & respond to user. 2. Attempt insert on final submit) Easily a candidate for a race condition.
The other is like what your posted code is doing, which is a “single process” doing everything in one submit. It is still vulnerable to a race condition.
Lets be clear, I am talking about the UX (User Experience). Even with your code concurrent requests can still pass the username check (first query) and fail the second query if another request for the same username wins the race to the insert query. There are no locks on the DB when the first query runs so it wouldn’t be a problem for another request to get between the two query’s.
You didn’t show what you do with the -1 as far as the UX, nevertheless, the second query still fails for all concurrent requests (users) but one. One and only one insert can happen per unique username even though the first query says it is available. It basically tells a half truth. The first query says “The name is available as of the moment I asked the DB”, but it has no idea if another request has already got to the insert query before your request.
Additionally, especially on a high load DB, I know you would agree that one query is always better than two.
Take your code and do a concurrent request load test and see what happens. If I get time, I may do one in case this comes up again sometime. Code doesn’t lie.
I was thinking of doing so, but don’t have the ability from work. If I have the inclination, I will do so later.
FYI: A couple methods for concurrency testing are curl_multi_init, HttpRequestPool and ApacheBench.
I am sure there are probably plenty more options as well.