[PHP] Critique My User Registration Script

Conrad11

Conrad11

Conrad11

Soldato
Joined
12 Jun 2005
Posts
5,361
Hi there,

This is the first time i have done any user registration stuff, and was wondering whether someone could critique my script and tell me what could be improved and if there are any security flaws in it?

Looking for things that could increase speed possibly and increase security, especially on the "check logged in user" part.

You can download the script here: download script clicky!

Thanks for any input.
 
Not bad. Some areas could be cleaned though:

Your two check_login.php files are identical save one integer check, why not take it as a function argument? login.php has an empty PHP section at the top which is just wasting parse time, no matter how small. Same with register.php, recover_details.php and, bizarrely, p_recover_details.php is blank. :P

Can't find any security holes. As far as I can see, in remove_magic_quotes() you can replace the array iteration and successive recursive function application with array_map() which should be significantly faster.

AddUser() could return boolean with the string outputted on the other side, but that's just pedantry for flexibility. Also for pedantry, echo 'x' followed by a blank die() can be replaced with die('x'). AGAIN for pedantry, real men use single quotes for string literals as it's faster. As a last little naggle, personally I'd put some of settings.php in general.php, with settings.php just containing error_reporting(), database setup, the salt definition and root path.

Overall though, nothing major that I can see.
 
First of all, thanks for looking at it :)

psyr33n said:
Your two check_login.php files are identical save one integer check, why not take it as a function argument? login.php has an empty PHP section at the top which is just wasting parse time, no matter how small. Same with register.php, recover_details.php and, bizarrely, p_recover_details.php is blank. :P
Click to expand...

Good point about the function, thanks.

Yeah i know about the empty sections, what I was originally going to do was put the validation results (if the validation failed) back onto the register or login page, but I scrapped that. When I actually implement I won't actually do that, i know its back practise.

Must have forgot to implement the recover details bit :s.

psyr33n said:
Can't find any security holes. As far as I can see, in remove_magic_quotes() you can replace the array iteration and successive recursive function application with array_map() which should be significantly faster.
Click to expand...

Thats just one of the standard things i stick in all my php projects, i got that function from robmiller's guide, which is rather old now. Thanks i will look into that.

psyr33n said:
AddUser() could return boolean with the string outputted on the other side, but that's just pedantry for flexibility. Also for pedantry, echo 'x' followed by a blank die() can be replaced with die('x'). AGAIN for pedantry, real men use single quotes for string literals as it's faster. As a last little naggle, personally I'd put some of settings.php in general.php, with settings.php just containing error_reporting(), database setup, the salt definition and root path.
Click to expand...

Fair point about the AddUser function and the die function.

What do you mean by faster?, also what is meant by "string literal"?


=======


My main concern was that maybe i shouldn't be querying the database every time the check_login is included? And should i store the md5(password) in the session data or just the plaintext password without the salt? Also, I am right in assuming that i don't need to escape the session data before i query with it, assuming its been escaped when entered into the session data.

Thanks again.
 
Conrad11 said:
What do you mean by faster?, also what is meant by "string literal"?
Click to expand...

Literals in programming language are the syntax used to hard-code a value into the code, so a string literal is what you use to specify a string in your code, e.g.
Code: 
$string1 = [B]"some literal"[/B];
$string2 = [B]'some other literal'[/B];
Similarly, when you assign a number or boolean value to your variable in a hard-coded way, you're using numeric and boolean literals respectively.

When PHP parses and executes a script, if it encounters a string literal that uses double quotes, it must parse the literal itself looking for special characters and embedded variables, for example:
Code: 
$number = 1
$string = "Here's a number: $number\r\n";

If you don't have any special characters or embedded variables in the literal, then it's faster to use single quotes because PHP then won't have to parse it in the first place.
 
Last edited:
I had a quick look at your stuff and nothing major presented itself. The only gripe I have is how you seem to validate the user's input when registering.

From what I can see you run a query on the database with certain details and then have if statements to see if a user already exists or the inputted value is valid. This is potentially wasteful of resources as you are running a query BEFORE you have even checked the data is valid.

Example, say a username was less than 3 characters. Your code goes:

Run query based on user input
Get result
if [user input exists OR user input not valid]
error

The username being too short is enough to cause an error, but you check the database anyway.

It would make far more sense to check the data is valid BEFORE running a query. Make sure all of the user's data is actually valid, if so THEN check the DB.
 
Worrying about a single database query that selects a single integer in an event so rare as user registration is premature optimisation.

Also, as if `array_map` is actually faster—it's not like it doesn't compile into the same instructions as manually looping through the array.

Lots of these criticisms are just irrelevant based on the inevitable use of the code, and at the end of the day if your site ever does get popular you're probably always going to see more benefit by moving to memcached than you would by doing all of these tiny optimisations.
 
Last edited:
It's hardly optimizing "too early" if it is something that should be obvious though.

Besides, he did ask for advice/criticisms etc. I never actually said it was wrong or bad or that he has to do anything. It is completely up to him to heed or ignore what I said.
 
robmiller said:
Worrying about a single database query that selects a single integer in an event so rare as user registration is premature optimisation.
Click to expand...

Well you can do it before or after the validation check. It makes more sense to do it afterwards and involves no extra effort or convolution of code, so do it afterwards.

You're right about array_map though.
 
robmiller said:
Also, as if `array_map` is actually faster—it's not like it doesn't compile into the same instructions as manually looping through the array.
Click to expand...

Always wondered about this. Was told by an elf (online buddy) that it was speedier but I have obviously never tested it. Regardless, it makes the code leaner.

robmiller said:
Lots of these criticisms are just irrelevant based on the inevitable use of the code, and at the end of the day if your site ever does get popular you're probably always going to see more benefit by moving to memcached than you would by doing all of these tiny optimisations.
Click to expand...

Well it was a bit inevitable. Highlighting a few things which make little difference but are perhaps better practice is critique nonetheless, even if largely useless. :(
 
I'm genuinely not trying to have a go, it's just better to concern yourself with the bigger picture when it comes to optimisation IMO.
 
Cheers guys, I was more worried about things like SQL injection/Cross-site attacks and things like that, or if i took a blatent "long way around" something etc.

I was actually pre-reply aware of all the validation of the fields points you brought up, I just quickly put them in there so that you wouldn't moan there was no validation.

With regards to validation, I will be using javascript first and then fallback on php should javascript not be enabled, by adding a token to submit from.

I will be doing validation first before querying the database.

Thanks for all the replies/discussion, and if you notice anything else, give me a shout.
 
Why would you use Javascript, when PHP is capable of doing validation regardless of how the user has their browser set up? It is a genuine question as I can't see why you would bother. Anyone care to explain? :p
 
robmiller said:
use echo instead of print it's way faster maaan
Click to expand...

I use printf and sprintf myself.

And the javascript thing is more for user experience than anything else. It's much nicer to have a seamless form check using javascript rather than reloading the page to do it in php. plus the server load wotsit, too
 
You must log in or register to reply here.
Back
Top Bottom