account activation script! Looking for feedback!

[spiritssight]

Hello All,

I am writing to ask for your feedback with the following script that I have writen, the script does one job and that is to activate an account.

Any feedback on making this script better and more sercue would be great, I don't know much about validation so help in that area would be great.

If this code can be simplified that would be great also!!!

Thanks ahead of time!

Here is the code:
activate.php

<?php
include '/home/dev/www/user-sys/activation_process.php';

include '/home/dev/www/user-sys/activation_form.php';
?>

activation_form.php

<?php

echo '
<div class="activation_form">
<center>
<form action="/user-sys/activate.php" method="post">

<label for="user_id">User ID:</label><br />
<input class="activate_form" type="text" name="user_id" size="25" /><br />

<label for="activation_code">Activation Code:</label><br />
<input class="activate_form" type="text" name="activation_code" size="25" /><br />

<label for="ate:">Todays Date: (yyyy-mm-dd)</label><br />
<input class="activate_form" type="text" name="activation_date" size="25" /><br />

<input type="submit" name="activate" value="Activate Account" /><input type="reset" name="clear" value="Clear" />
</form>
</center>
</div>
';
?>

activation_process.php

<?php
if(isset($_POST['activate']))
{
include '/home/dev/www/lib/db_config_cr-dev.php';
include '/home/dev/www/lib/db_conn-select.php';

$user_id = $_POST["user_id"];
$activation_code = $_POST["activation_code"];
$activation_date = $_POST["activation_date"];

$query_s = "SELECT * FROM user_preferences WHERE user_id = '$user_id'";
$result_s = mysql_query($query_s) OR die("Sorry". mysql_error());
$record = mysql_fetch_array($result_s);
$count = mysql_affected_rows();
if($count)
{
$user_timezone = $record["timezone"];
date_default_timezone_set ($user_timezone);
}
else
{
$default_timezone = "UTC";
date_default_timezone_set ( $default_timezone );
}

$query_s = "SELECT * FROM activation WHERE user_id = '$user_id' AND activation_code = '$activation_code' ";
$result_s = mysql_query($query_s) OR die("Sorry". mysql_error());
$record = mysql_fetch_array($result_s);
$count = mysql_affected_rows();

// If result matched $userid and $activation_code, table row must be 1 row
if($count && $activation_date == date("Y-m-d"))
{
$default_timezone = "UTC";
date_default_timezone_set ( $default_timezone );

$exp_date = $record["expire_date"];
$expiration_date = strtotime($exp_date);
$todays_date = date("Y-m-d");
$today = strtotime($todays_date);

if ($expiration_date >= $today)
{
$query_i = "UPDATE access_credentials SET user_access_status = \"A\" WHERE user_id = \"$user_id\"";
$result_i = mysql_query($query_i) OR die("Sorry was unable to update table!<br />" . mysql_error());
$count = mysql_affected_rows();
if($count)
{
echo "Account is now activated";
}
}
else
{
echo "Sorry you did not enter vaild infomation on the activation form, please try again!";
}
}
include '/home/dev/www/lib/db_close.php';
}
?>


Sincerely,
Christopher

[bradgrafelman]

Your code is vulnerable to SQL injection attacks; user-supplied data should never be placed directly into SQL query strings. Instead, first sanitize it with a function such as mysql_real_escape_string().
mysql_affected_rows() doesn't apply to SELECT queries.
Don't use 'SELECT *' queries (unless you really mean to SELECT all columns, e.g. if doing a database dump) - only SELECT the columns you actually need data from.
Is there an index on user_id? Is it a UNIQUE index? Looks like it should be to me.
No need to count the number of rows - just try to retrieve the first row SELECTed. If the fetching function (such as mysql_fetch_array() in your code) doesn't find a row, it'll return boolean false.
MySQL error messages should never printed to the user if an error occurs.
You can get rid of the two lines before the UPDATE query that calculate the current Unix timestamp; just compare the calculated Unix timestamp of the expiration date to the result of the function time().
What is in the 'db_close.php' script? Is it nothing more than a call to mysql_close()? If so, this is unnecessary - there's no need to call this function at your script's end.

[spiritssight]

Your code is vulnerable to SQL injection attacks; user-supplied data should never be placed directly into SQL query strings. Instead, first sanitize it with a function such as mysql_real_escape_string().

Thanks, I know it was vulnerable, I just was unsure who to fix that, any other tips on make this better I would be happy to learn about and fix :-)
Ok, when I put the mysql_real_escape_string('$user_id') with \" in font and after the function it gives me issue, it does not look up in the database and gives me my else statement as soon as I take them away my script works again.


mysql_affected_rows() doesn't apply to SELECT queries.
oops, I knew this, I must of forget to fix it, seeing it worked I did not think about it, I was fixing them before I got this post but miss some in another script.

Don't use 'SELECT *' queries (unless you really mean to SELECT all columns, e.g. if doing a database dump) - only SELECT the columns you actually need data from.

bad habit, I also was told this about the insert to use the fields instead of *

Is there an index on user_id? Is it a UNIQUE index? Looks like it should be to me.

Yes I have set the index of all tables with user_id as a Unique index, I would how ever like to know how I can have it if I delete from one table it automatic does from the rest, I have 5 tables that all should connect to the parent table, one of these tables need to be allowed to delete with out affecting the rest, as this is the activation table that holds activation keys once activated it delete the key as its no longer good.

No need to count the number of rows - just try to retrieve the first row
SELECTed. If the fetching function (such as mysql_fetch_array() in your code) doesn't find a row, it'll return boolean false.
Good idea, is there a time where you would want to use the affect or num_rows besides if you know it may return more then one?

MySQL error messages should never printed to the user if an error occurs.
Are you talking about the mysql_error() with the use of die, I will remove all of this stuff once the scripts are ready to go live for the real site and not the development site I am going to need alot of help as these scripts get closer to being the public ver to do error handling so I get the errors and they get what they need to know

You can get rid of the two lines before the UPDATE query that calculate the current Unix timestamp; just compare the calculated Unix timestamp of the expiration date to the result of the function time().

I think I toke care of this before I got the post but I will at the end of this post repost the processing part of the script

What is in the 'db_close.php' script? Is it nothing more than a call to mysql_close()? If so, this is unnecessary - there's no need to call this function at your script's end.

Why do you not need this? I througt and see everyone using.

Thanks a lot, I am very grateful for all of your help!

See below for script with more of the fixes, if I missed something please point it out and if I can write something better please do advice me as I am learning slowly but surely :-)

Sincerely,
Christopher


<?php
if(isset($_POST['activate']))
{
include '/home/dev/www/lib/db_config_cr-dev.php';
include '/home/dev/www/lib/db_conn-select.php';

$user_id = $_POST["user_id"];
$activation_code = $_POST["activation_code"];
$activation_date = $_POST["activation_date"];

$query_s = "SELECT timezone, dls FROM user_preferences WHERE user_id = \"mysql_real_escape_string('$user_id')\"";
$result_s = mysql_query($query_s) OR die("Sorry". mysql_error());
$record = mysql_fetch_array($result_s);

if($record == 1)
{
$user_timezone = $record["timezone"];
date_default_timezone_set ( $user_timezone );
}

$query_s = "SELECT activation_code, expire_date FROM activation WHERE user_id = \"$user_id\" AND activation_code = \"$activation_code\" ";
$result_s = mysql_query($query_s) OR die("Sorry". mysql_error());
$record = mysql_fetch_array($result_s);

// If result matched $userid and $activation_code, table row must be 1 row
if($record && $activation_date == date("Y-m-d"))
{
if(isset($user_timezone)){date_default_timezone_set ( "UTC" );}

if ($record["expire_date"] >= time())
{
$query_u = "UPDATE access_credentials SET user_access_status = \"A\" WHERE user_id = \"$user_id\"";
$result_u = mysql_query($query_u)OR die("Sorry was unable to update table!<br />" . mysql_error());
$count = mysql_affected_rows();

if($count)
{
$query_d = "DELETE FROM activation WHERE user_id = \"$user_id\"";
$result_d = mysql_query($query_d)OR die("Sorry was unable to delete record from the table!<br />" . mysql_error());
$count = mysql_affected_rows();
if($count)
{ echo "Account is now activated"; }
}
else
{ echo "Your Account was already activated!";}
}
else
{ echo "Account activation has expired, please request another activation code!"; }

}
else
{
echo "Sorry you did not enter vaild infomation on the activation form, please try again!";
}

include '/home/dev/www/lib/db_close.php';
}
?>

[bradgrafelman]

$query_s = "SELECT timezone, dls FROM user_preferences WHERE user_id = \"mysql_real_escape_string('$user_id')\""; mysql_real_escape_string() is a PHP function, not a MySQL function, so you should use concatenation... e.g. $query = 'SELECT foo FROM bar WHERE foobar = \'' . mysql_real_escape_string($foobar) . '\'';
As for automatically deleting rows in other tables, you may be able to do this using foreign key constraints (here (http://dev.mysql.com/doc/refman/5.0/en/innodb-foreign-key-constraints.html) is a link to the MySQL manual). Basically, you'd used the "ON DELETE CASCADE" option to automatically delete the row from the other tables that have the same user_id. Note that I'm no SQL guru and that you shouldn't take my word on this. :p

Why do you not need this? I througt and see everyone using.As noted in the PHP manual, mysql_close() isn't necessary because non-persistent links are automatically closed when a PHP script ends. Thus, the only time you'd really need to use that function is if the script was going to take a lot of time processing data (or doing something else) and you didn't want to keep a connection open to the SQL server.

[spiritssight]

ok, thanks alot for the help, I got the escape (function) to work I tried every thing but that ". ... ." now do I need to do any thing when I pull the infomation from the database that was escaped?

I got to read about this automatic delete / update thing

now I understand about the mysql_close() thanks

script below just incase I missed something or you see something else :-) I like it when the code gets shorter and does better :-)

Sincerely,
Christopher


<?php
if(isset($_POST['activate']))
{
include '/home/dev/www/lib/db_config_cr-dev.php';
include '/home/dev/www/lib/db_conn-select.php';

$user_id = $_POST["user_id"];
$activation_code = $_POST["activation_code"];
$activation_date = $_POST["activation_date"];

$query_s = "SELECT timezone, dls FROM user_preferences WHERE user_id = \"". mysql_real_escape_string($user_id)."\"";
$result_s = mysql_query($query_s) OR die("Sorry". mysql_error());
$record = mysql_fetch_array($result_s);

if($record == 1)
{
$user_timezone = $record["timezone"];
date_default_timezone_set ( $user_timezone );
}

$query_s = "SELECT activation_code, expire_date FROM activation WHERE user_id = \"". mysql_real_escape_string($user_id)."\" AND activation_code = \"". mysql_real_escape_string($activation_code)."\"";
$result_s = mysql_query($query_s) OR die("Sorry". mysql_error());
$record = mysql_fetch_array($result_s);

// If result matched $userid and $activation_code, table row must be 1 row
if($record && $activation_date == date("Y-m-d"))
{
if(isset($user_timezone)){date_default_timezone_set ( "UTC" );}

if ($record["expire_date"] >= time())
{
$query_u = "UPDATE access_credentials SET user_access_status = \"A\" WHERE user_id = \"". mysql_real_escape_string($user_id)."\"";
$result_u = mysql_query($query_u)OR die("Sorry was unable to update table!<br />" . mysql_error());
$count = mysql_affected_rows();

if($count)
{
$query_d = "DELETE FROM activation WHERE user_id = \"". mysql_real_escape_string($user_id)."\"";
$result_d = mysql_query($query_d)OR die("Sorry was unable to delete record from the table!<br />" . mysql_error());
$count = mysql_affected_rows();
if($count)
{ echo "Account is now activated"; }
}
else
{ echo "Your Account was already activated!";}
}
else
{ echo "Account activation has expired, please request another activation code!"; }

}
else
{
echo "Sorry you did not enter vaild infomation on the activation form, please try again!";
}
}
?>

[spiritssight]

ok, one more thing with this or any of the script I post, if I have them indented wrong could you tell me so I can get better at having my code organized nicer!

Sincerely,
Christopher