Code needs a second pair of eyes

[gelf]

I've written a user registration system for my site, but many readers seem to have problems either signing up or logging into the system. I've been through the code again and again and I can't see any problems. There also seems to be no pattern, in browsers, cookies etc that could be causing the problem. To be honest, I think some of the session control stuff is beyond my current coding abilities.

I was wondering if one of you kind php geniuses here would be willing to look over my php code and see if you can find any problems with it. There's probably too much code to paste into a message here, so we may need to communicate over email.

Thanks

[stymiee]

Post it here. You'll get many more eyes to review it for you.

[gelf]

Well I'm not really sure which page would be the most appropriate one to post, but I guess I can start with the signup.php page. It's loosely based on article I remember seeing on this site:

              <?php
if (!isset($submitok)):
    // Display the user signup form
?>  
  <p>Please use the following form to sign up for a registered account
              here at DVDLard. Registration allows many advances site facilities
              such as review ratings, browsing histories and more.</p>
            <p>Your password will be privately emailed directly to you.</p>
            <form method=post action="<?=$PHP_SELF?>">
              <table border=0 cellpadding=0 cellspacing=5>
                <tr>
                  <td align=right>
                    <p>User ID</p>
                  </td>
                  <td>
                    <input name=newid type=text maxlength=100 size=25>
                    <font color=orangered size=+1><TT><B>*</B></TT></font> </td>
                </tr>
                <tr>
                  <td align=right>
                    <p>Full Name</p>
                  </td>
                  <td>
                    <input name=newname type=text maxlength=100 size=25>
                    <font color=orangered size=+1><TT><B>*</B></TT></font> </td>
                </tr>
                <tr>
                  <td align=right>
                    <p>E-Mail Address</p>
                  </td>
                  <td>
                    <input name=newemail type=text maxlength=100 size=25>
                    <font color=orangered size=+1><TT><B>*</B></TT></font> </td>
                </tr>
                <tr valign=top>
                  <td align=right>
                    <p>Other Notes</p>
                  </td>
                  <td>
                    <textarea wrap name=newnotes rows=5 cols=30></textarea>
                  </td>
                </tr>
                <tr>
                  <td align=right colspan=2>
                    <hr noshade color=black>
                    <input name="reset" type=reset value="Reset Form">
                    <input type=submit name="submitok" value="   OK   ">
                  </td>
                </tr>
              </table>
            </form>
            <?php
else:
    // Process signup submission
    if ($newid=="" or $newname=="" or $newemail=="") {
        error("One or more required fields were left blank.\\n".
              "Please fill them in and try again.");
    }
    
    // Check for existing user with the new id
    $sql = "SELECT COUNT(*) FROM USER WHERE USERID = '$newid'";
    $result = mysql_query($sql);
    if (!$result) { 
        echo("A database error occurred in processing your ".
              "submission.\\nIf this error persists, please ".
              "contact [email="steve@dvdlard.co.uk"]steve@dvdlard.co.uk[/email].");
  $error=1;
    }
    if (mysql_result($result,0,0)>0) {
        echo("A user already exists with your chosen userid.".
              "Please try another.");
  $error=1;
    }
    
 if (!$error)
 {
    $newpass = substr(md5(time()),0,6);
    $newpassencrypt = md5($newpass);
 
    $sql = "INSERT INTO USER SET
              USERID = '$newid',
              PASSWORD = '$newpassencrypt',
              FULLNAME = '$newname',
              EMAIL = '$newemail',
              NOTES = '$newnotes'";
    if (!mysql_query($sql))
        error("A database error occurred in processing your ".
              "submission.\\nIf this error persists, please ".
              "contact [email="steve@dvdlard.co.uk"]steve@dvdlard.co.uk[/email].");
              
    // Email the new password to the person.
    $message = "Hi!
Blah, Blah welcom message to user
";
    ?>
            <p><strong><font size="4">User registration successful!</font></strong></p>
            <p>Your userid and password have been emailed to <strong>
              <?=$newemail?>
              </strong>, the email address you just provided in your registration
              form. To log in, click <a href="index.php">here</a> to return to
              the login page, and enter your new personal userid and password.</p><br>
            <?php
endif;
?>

[Jake1]

I see two problems.

One is your using reguler $variable variables for incoming data, instead of $_POST['varible'] data instead which is better/safer. So for example instead of...

  if ($newid=="" or $newname=="" or $newemail=="") { 

should be....

  if ($_POST['newid'] =="" or $_POST['newname']=="" or $_POST['newemail']=="") { 

Also you should checking incoming data for single quotes when you use them in queries.

Hope it helps

[gelf]

Thanks for that I'll give it a try. I'm not sure if the problems might actually be in the login.php page though so here's the code from that page:

//Include Common Commands Script
include("common.php");
 
if ($strMethod=="checklogin") 
{ 
$CookieSession=$_COOKIE['user'];
$strQuery = mysql_query("SELECT * FROM USER WHERE USERID = '$CookieSession'"); 
if($cookie = mysql_num_rows($strQuery) > 0) 
{ 
//cookie session = stored session, login 
$result = mysql_fetch_array($strQuery); 
$uname=$result["USERID"];
$pass=$result["PASSWORD"];
$fullname=$result["FULLNAME"];
$hotlistnum=$result["HOTLISTNUM"];
if($CookieSession==$uname) 
{
$currentdate = getdate();
$formattedDate = $currentdate[year]."-".$currentdate[mon]."-".$currentdate[mday];
$sql = ("Update USER SET LastLogin = '$formattedDate' 
       where USERID = '$CookieSession'"); 
mysql_query($sql);
session_register("uname");
session_register("fullname");
session_register("hotlistnum");
header("Location: $url"); 
}
}
}
include("contentheader.php");
?>
<table width="100%" border="0" cellspacing="5" cellpadding="5">
  <tr valign="top"> 
    <td>
      <table width="100%" border="0" cellpadding="1" cellspacing="1" bgcolor="#000066">
        <tr>
          <td background="/Images/Gradients/header3.gif" valign="top" height="40"><p><strong><font color="#FFFFFF"><img src="Images/Icons/Join.gif" width="130" height="40" align="right">Site
                  Logon</font></strong><font color="#FFFFFF">
                  </font>        </p></td>
        </tr>
        <tr valign="top">
          <td align="left" bgcolor="F1F1F1">
            <p>Please use the following form to log into the site. If you are
              not a <a href="signup.php">registered</a> user you can sign up
              by <a href="signup.php">clicking here</a>. Registration allows
              many advances site facilities such as review ratings, browsing
              histories and more.
            <form name="form1" method="post" action="login_check.php?strMethod=<?=$strMethod?>&url=<?=$url?>">
              <p>&nbsp;</p>
              <table width="100%" border="0" cellspacing="0" cellpadding="0">
                <tr>
                  <td width="100">
                    <p><strong>User Name</strong>: </p>
                  </td>
                  <td><input name="uname" type="text" id="uname">
                  </td>
                </tr>
                <tr>
                  <td><p><strong>Password</strong>: </p>
                  </td>
                  <td><input name="pass" type="password" id="pass">
                  </td>
                </tr>
                <tr>
                  <td width="100"><p><strong>Remember Me</strong>:</p>
                  </td>
                  <td><input name="strSetCookie" type="checkbox" id="strSetCookie" value="1" checked>
                      <font size="2">(Requires cookies enabled on browser. We
                      strongly recommend that you turn this option off if your
                      on a public machine e.g. Cybercafe, Public Library etc))</font></td>
                </tr>
                <tr>
                  <td width="100">&nbsp;</td>
                  <td><input type=submit name="submitok" value="   OK   ">
                  </td>
                </tr>
                <tr>
                  <td>&nbsp;</td>
                  <td><p>* <a href="loginforgot.php">Forgotten your username
                        or password? Have them emailed here.</a></p>
                  </td>
                </tr>
              </table>
            </form>
            <p>
          </td>
        </tr>
      </table>
    </td>
    <td width="175">

[Chillijam]

You seem to be posting your form data to login_check.php, and also, you are passing get variables in the "action" field of the form.

I would try to keep your data transer method consistent. Instead of

<form name="form1" method="post" action="login_check.php?strMethod=<?=$strMethod?>&url=<?=$url?>">

try

<form name="form1" method="post" action="login_check.php">
<input type=hidden name="strMethod" value="<?=$strMethod?>">
<input type=hidden name="url" value="<?=$url?>">

It may not help with your problem, but it will make it easier to remember whether you use post or get for a particular parameter.