User registration code critique

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

User registration code critique

Post 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.
Last edited by Benjamin on Sat Jul 18, 2009 11:21 pm, edited 1 time in total.
Reason: Changed code type from text to php.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
Darhazer
DevNet Resident
Posts: 1011
Joined: Thu May 14, 2009 3:00 pm
Location: HellCity, Bulgaria

Re: User registration code critique

Post by Darhazer »

There is a possible SQL injection on line 14
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: User registration code critique

Post 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?
Last edited by Benjamin on Sat Jul 18, 2009 11:21 pm, edited 1 time in total.
Reason: Changed code type from text to php.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
Darhazer
DevNet Resident
Posts: 1011
Joined: Thu May 14, 2009 3:00 pm
Location: HellCity, Bulgaria

Re: User registration code critique

Post by Darhazer »

No, never rely on addslashes() for preventing SQL injection, use the db-specific functions, for MySQL it's mysql_real_escape_string()
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: User registration code critique

Post by social_experiment »

Ok, thank you for the critique, i will append the code then post it again.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
social_experiment
DevNet Master
Posts: 2793
Joined: Sun Feb 15, 2009 11:08 am
Location: .za

Re: User registration code critique

Post 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.
“Don’t worry if it doesn’t work right. If everything did, you’d be out of a job.” - Mosher’s Law of Software Engineering
User avatar
Korken
Forum Newbie
Posts: 10
Joined: Thu Jul 02, 2009 8:07 am
Location: Luleå, Sweden

Re: User registration code critique

Post 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
User avatar
Darhazer
DevNet Resident
Posts: 1011
Joined: Thu May 14, 2009 3:00 pm
Location: HellCity, Bulgaria

Re: User registration code critique

Post 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
User avatar
Korken
Forum Newbie
Posts: 10
Joined: Thu Jul 02, 2009 8:07 am
Location: Luleå, Sweden

Re: User registration code critique

Post by Korken »

Ah, my bad. Didn't see the 'i' at the end.

//Emil
Post Reply