[PHP] Critique My User Registration Script

[Conrad11]

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.

 

[Conrad11]

First of all, thanks for looking at it

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.

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.

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.

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.

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.

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]

Ah right - gotcha, thanks.

 

[Conrad11]

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.

 

[Conrad11]

Reduce load on the server?

 

[Conrad11]

Just remember to check your data on the server every time, regardless of if you have JS do it or not.

I already knew that :s.....just looked at what i said, I was thinking that if javascript would check it, but it's probably possible to bypass the javascript validation.....lol...must think before i post!!