Help with custom function

[pedro84]

Hi there,

 

I wrote small function that provides to my site users a email validation. User must click the link thast has been sent with activation email. I'm very tired and can't make one thing.

 

This is the function:

function user_activation ()
{

$password = $_GET['hash'];
$timestamp = base64_decode($_GET['timestamp']);
$query = mysql_query("UPDATE users
SET status=1 WHERE (password = '$password') AND (timestamp = '$timestamp')") or die (mysql_error());
$activated = mysql_affected_rows();
if ($activated = 1){
	 return true;
	 } else {
	 return false;
	 }

}


if (user_activation())
{
    echo 'Your account has been activated! Please click here to login';
}elseif (!user_activation()){
echo 'Error! Your account has already been activated!';
}

 

Ok. Let's say that user's account has been activated early. How to change error text? Second thing, how to put any error text here?

 

Cheers!

Pedro

[Bauer418]

Well, first of all, why are you using elseif (...) when you could just use else?

 

Second of all, it's probably not smart to use the hash of a user's password as a validation method, even if it's only going to be shown to the user.  The way I do it when I need an email activation, is that I have a separate table called verifications, which contains the following columns:

verification_id VARCHAR(64)

verification_user_id INTEGER

verification_email VARCHAR(225)

verification_time DATETIME

When a user registers, a new row is created.  verification_id is a randomly generated, 64 character string sort of like a password hash, but it's completely unrelated to the password.  user_id is their user_id in my users table.  verification_email is the email address that needs to be verified, and verification_time is the time that the verification request was initiated.  The link for the user then looks something like:

index.php?act=home.verify&k={64 byte string}&u={user_id}&e={email}

And it goes to the table to match their data together.  It also checks to see how old the verification is (it won't allow requests from over 24 hours ago to be validated).

[pedro84]

Thanks for reply, but I think your proporsal is totally the same I did. It's impossible to breake md5. Why password you ask? It is safe to use the timestamp and passwrod hash to to locate the user because it is very unlikely that another person will register at exactly the same time with exactly the same password as another user.

 

There's only need to split error messages.

 

PS I don't know why there was elseif. Maybe was testing or something.

 

Cheers,

Pedro

[Bauer418]

md5 is not almost impossible to break, especially with technology these days.  Running a simple md5 on a user's password without some sort of salt or some other encryption is hardly considered secure anymore.  While it used to be an acceptable technique, it no longer is.  Especially considering that md5 does incur collisions from time to time in which two entirely different strings end up with the same hashed value.  I was simply pointing out what I have read and used in my own applications, not to be condescending or anything else.

 

Anyway, back on subject, I don't think I quite grasp your problem, can you describe in more detail?

[pedro84]

Both of us are right. Saying that md5 is impossible to break I meant that I use id with salt. That's more secure.

 

About my problem. Currently is as following:

<?php
include ("init.php");

function user_activation ()
{
   $password = $_GET['hash'];
   $timestamp = base64_decode($_GET['timestamp']);
$query = mysql_query("UPDATE users SET status=1 WHERE (password = '$password') AND (timestamp = '$timestamp')") or die (mysql_error());
}	

if (user_activation ()){
echo 'Your account has been activated.';
} else {
echo 'Your account has been already activated.';
}



?>

 

This is the problem generating part of the script. The problem is that echo. No matter if status = 1 (no data has been added to datasbase) or status = 0 (data has been added) I got:

echo 'Your account has been already activated.';

 

That's the only problem now;)

[pedro84]

Anyone?

[darkfreaks]

<?php
function user_activation ()
{
   $password = $_GET['hash'];
   $timestamp = base64_decode($_GET['timestamp']);
$query = mysql_query("UPDATE users SET status=1 WHERE
(password = '$password') AND (timestamp = '$timestamp')")
or die (mysql_error());

$num=mysql_num_rows($query);

if($num>1) { echo "Your Account has already been activated!";}
else {user_activation (); 
echo "Your account has been activated!";

}
}?>

[marklarah]

Hi

 

The line

echo 'Your account has been activated.';

Is confusing to php.

 

"." before a "'" means to php means implode text/variables, so your best off there not having a full stop or something.

 

 

Hope that helps.

 

 

Mark

[Northern Flame]

you have to make the variables not established in the

function globals. try making $_GET a global variable

in the function

[Bauer418]

you have to make the variables not established in the

function globals. try making $_GET a global variable

in the function

$_GET is a superglobal and is already used in the global spec in a function, class, or anywhere else.

 

<?php
include ("init.php");

function user_activation () {
     $password = $_GET['hash'];
     $timestamp = base64_decode($_GET['timestamp']);
     $query = mysql_query("UPDATE users SET status=1 WHERE (password = '$password') AND (timestamp = '$timestamp')") or die (mysql_error());
     return mysql_affected_rows($query) >= 1;
}

if (user_activation ()) {
     echo 'Your account has been activated.';
} else {
     echo 'Your account has been already activated.';
}
?>

[pedro84]

Hi,

 

Thanks for reply. I used every way and even code you posted and Im getting:

 

Warning: mysql_affected_rows(): supplied argument is not a valid MySQL-Link resource in D:\EasyPHP 2.0b1\www\system\activate.php on line 10
Your account has been already activated

[Bauer418]

My bad, it shouldn't be mysql_affected_rows($query), it should just be mysql_affected_rows()

 

<?php
include ("init.php");

function user_activation () {
     $password = $_GET['hash'];
     $timestamp = base64_decode($_GET['timestamp']);
     $query = mysql_query("UPDATE users SET status=1 WHERE (password = '$password') AND (timestamp = '$timestamp')") or die (mysql_error());
     return mysql_affected_rows() >= 1;
}

if (user_activation ()) {
     echo 'Your account has been activated.';
} else {
     echo 'Your account has been already activated.';
}
?>

[darkfreaks]

<?php
include ("init.php");

function user_activation () {
     $password = $_GET['hash'];
     $timestamp = base64_decode($_GET['timestamp']);
     $query = mysql_query("UPDATE users SET status=1 WHERE (password = '$password') AND (timestamp = '$timestamp')") or die (mysql_error());
     return mysql_affected_rows($query) >= 1;
}

if (user_activation ()) {
     echo 'Your account has been activated.';
} else {
     echo 'Your account has been already activated.';
}
?>

[pedro84]

My bad, it shouldn't be mysql_affected_rows($query), it should just be mysql_affected_rows()

 

<?php
include ("init.php");

function user_activation () {
     $password = $_GET['hash'];
     $timestamp = base64_decode($_GET['timestamp']);
     $query = mysql_query("UPDATE users SET status=1 WHERE (password = '$password') AND (timestamp = '$timestamp')") or die (mysql_error());
     return mysql_affected_rows() >= 1;
}

if (user_activation ()) {
     echo 'Your account has been activated.';
} else {
     echo 'Your account has been already activated.';
}
?>

 

Ok. Now works great. That's what I meant. Ok..now question. What did I make wrong?

 

Thanks for everyone for reply.

[Bauer418]

More importantly is something I just realized...you don't clean $_GET['hash'] at all before throwing it to MySQL.  People could easily inject their own SQL queries into your site by simply changing the value of the hash parameter in your URL query string.

[pedro84]

Clean GET? How make it?

[Bauer418]

mysql_real_escape_string($_GET['hash']) would help quite a bit.  And just ensure that it contains the data that you want, even if you need to run preg_match(), such as

 

preg_match('/^[a-z0-9]{32}$/i', $_GET['hash']);

[darkfreaks]

<?php
function clean($text) {

mysql_real_escape_string($text);
trim($text);


return $text;
}?>

 

this would be easier

[pedro84]

<?php
function clean($text) {

mysql_real_escape_string($text);
trim($text);


return $text;
}?>

 

this would be easier

 

I need to change variable $text, right?

[Bauer418]

<?php
function clean($text) {

mysql_real_escape_string($text);
trim($text);


return $text;
}?>

 

this would be easier

Yes but that doesn't validate a proper md5 hash.  The method I posted, though preg_match is slightly more cpu-intensive than trim, won't let a string that is obviously not an md5 hash go into the query.

[pedro84]

The last question. I should post it in the activation.php file? What will this function do?

[darkfreaks]

will trim spaces and stop injection

[pedro84]

Oki. Thank you. I understand now.

 

So could I make something like that:

<?php

$text = $my_variable;

function clean($text)

?

 

 

Or is it possible to change variable when calling function, ex:

function clean($my_variable)