Page 1 of 1

User registration code critique

Posted: Sat Jul 18, 2009 5:50 am
by social_experiment
I created this code to use as a user registration. It will follow the normal "activation-by-email" protocol. Im using the code as part of a class.

Code: Select all

 
<?php
 
 function activate_user() {
   #check if activation id is set
   if ( isset($_GET['a_id']) ) {
    #equate the $_GET variable
    $a_id = $_GET['a_id'];
    #check the lenght of the string
    if ( strlen($a_id) == 32 ) {
     $connection_string = mysqli_connect($this->HOST, $this->DB_USER, $this->DB_PASS);
     $select_db = mysqli_select_db($connection_string, $this->DB);
     #check if there is a match for the $a_id
     $match_query = mysqli_query($connection_string, "SELECT * FROM user_table WHERE activate_id = '$a_id' AND status = 'N' ");
     #return the number of matches found
     $rows = mysqli_num_rows($match_query);
     #switch statement
     switch($rows) {
     #if a match is found
     case(1):
     $update_query = mysqli_query($connection_string, "UPDATE user_table SET status = 'Y' WHERE activate_id = '$a_id' ");
      if ( !$update_query ) { 
       echo 'There has been a problem with account activation. Please register again.';    
       #send an email to the administrator
       $err_date = date('Y-d-m H:i:s');
       $subj = 'Error with user registration.';
       $msg = "There has been an problem with user activation on $err_date.";
       $headers = "From: SITE";      
       @mail(ADDY, $subj, $msg, $headers);    
      }
      #query successful
      else {
       $update_a_id = mysqli_query($connection_string, "UPDATE user_table SET activate_id = '".md5($a_id)."' WHERE activate_id = '$a_id' ");
       if ( !$update_a_id ) { file_put_contents("err_log.txt", "Activation id has not been changed.", FILE_APPEND); }      
       echo 'Your account has been activated.';        
       echo "<a href=\"index.php\">Return to the main page</a>";
      }
     break;  
     default:
     $err_date = date('Y-d-m H:i:s');
     file_put_contents("err_log.txt", "At $err_date someone tried to access an allready activated account from ".$_SERVER['REMOTE_ADDR']."", FILE_APPEND);
     header('location: registration_form.php');
     exit();
     break;
     #end switch 
     }
    }
    #if activation id != 32 characters
    else {
     header('location: registration_form.php');
     exit();
    }
   }
   #if the id is not set.
   else {
    header('location: registration_form.php');
    exit();
   }
  }   
  
 }
?>
   
I am thinking of increasing the length of the the activation id. I have a question about testing for the length of the $_GET['a_id'], is it a valid test? My thinking is, even if an invalid id is inserted and pasted into a browser ( along with the rest of the required URL ) it will be tested against the db.

Thanks in advance for any help.

Re: User registration code critique

Posted: Sat Jul 18, 2009 8:04 am
by Darhazer
There is a possible SQL injection on line 14

Re: User registration code critique

Posted: Sat Jul 18, 2009 10:47 am
by social_experiment
There is a possible SQL injection on line 14

Code: Select all

 
<?php
$match_query = mysqli_query($connection_string, "SELECT * FROM user_table WHERE activate_id = '".addslashes($a_id)."' AND status = 'N' ");
?>
 
I changed the line to the code above, is this correct to stop possible SQL injection?

Re: User registration code critique

Posted: Sat Jul 18, 2009 11:42 am
by Darhazer
No, never rely on addslashes() for preventing SQL injection, use the db-specific functions, for MySQL it's mysql_real_escape_string()

Re: User registration code critique

Posted: Sun Jul 19, 2009 3:57 am
by social_experiment
Ok, thank you for the critique, i will append the code then post it again.

Re: User registration code critique

Posted: Sun Jul 19, 2009 8:30 am
by social_experiment

Code: Select all

<?php
$match_query = mysqli_query($connection_string, "SELECT * FROM user_table WHERE activate_id = '".mysqli_real_escape_string($connection_string, $a_id)."' AND status = 'N' ");
?>
Above is the changed code.

Re: User registration code critique

Posted: Mon Jul 20, 2009 5:09 pm
by Korken
Shouldn't the link identifier be at the end of mysql_query and mysql_real_escape_string?
-> http://se.php.net/manual/en/function.mysql-query.php
-> http://se.php.net/mysql_real_escape_string

But perhaps it works the other way around too.

//Emil

Re: User registration code critique

Posted: Mon Jul 20, 2009 5:17 pm
by Darhazer
Korken wrote:Shouldn't the link identifier be at the end of mysql_query and mysql_real_escape_string?
-> http://se.php.net/manual/en/function.mysql-query.php
-> http://se.php.net/mysql_real_escape_string

But perhaps it works the other way around too.

//Emil
He is using mysqli functions and they accept link as first argument

Re: User registration code critique

Posted: Mon Jul 20, 2009 5:22 pm
by Korken
Ah, my bad. Didn't see the 'i' at the end.

//Emil