Feedback please

[SpaceLama]

Hello all,

 

I have been working on a blog. Started programming about 2 months ago and at the moment my little project is done. This means my scripts works. So i can register, login, add message to a categorie, create a categorie, place comments and update messages. As i said i am just a beginner so it is always possible that i am using commands wrong or that i should work with other functions. That is why i need some feedback from the pro's!

 

login.php:

<?php
require_once('header.php');
echo '<br/>';
echo '<br/>';

// check if the form has been posted. 
if($_SERVER['REQUEST_METHOD'] == 'POST'){
   if(!$_POST['name'] == '' AND !$_POST['password'] == ''){
      $name = mysql_real_escape_string(ucwords(strtolower($_POST['name'])));

    
    // Unify all names 
        if (!empty($name)){   
    // Set after the ' within a name on a captial. Example: o'brian becomes O'Brian.     
            if(strpos($name, "'")) {	
              $pos = strpos($name, "'") + 1;
              }
    // Set after an - also an capital within the name. Example: peter-paul becomes Peter-Paul.
            if (strpos($name, '-')) {	
              $pos = strpos($name, '-') + 1;
              }
    // If there aint - or ' in the name then just first letter capital.
            if (!preg_match('/[-\']/', $name)){
              $pos = $name;
              }
            $name = substr_replace($name, strtoupper($name[$pos]), $pos, 1);
        }
  // if the right characters has been used go into the block  
    if (preg_match ('/^[A-Za-z. -]+$/', $name)){
    
    // encrypt password
      $salt1 = $name;
      $salt2 = 'zub67u3ff63qnxr9mhgdmuzka3heqy';
      $e_pass = sha1(md5($salt1.mysql_real_escape_string($_POST['password']).$salt2));       
       
      // Check if the name and password match with database
        $sql = "SELECT COUNT(*) FROM persons AS UserCheck WHERE name = '".$name.
                                "' AND password = '".$e_pass."'";
        $result = mysql_query($sql);
      // check if $result has been set. If not then mysql_query didnt worked
        if(!$result){
            trigger_error(mysql_error().' <br/>In query: '.$sql);
        }
        else {
            $pass_count = mysql_result($result, 0);
            if ($pass_count == 1) { 
            // set cookies
                $expire=time()+3600;
                setcookie("name","$name",$expire);
                  
                header('Location: ' .$config_basedir);
                                   
              } elseif ($pass_count == 0 AND !$_POST['password'] == '') {
                  echo 'Inlog gegevens kloppen niet.<br/>';
              }
        } 
    } else {
        echo 'U kunt geen cijfers of aparte symbolen gebruiken!<br/>';
    }  
  } else {
      echo 'U dient alle velden in te vullen!';
  }
}   
?>
<form action="" method="post">
<table>
<tr>
  <td>Username</td>
  <td><input type="text" name="name" size="20" maxlenght="20"></td>
</tr>
<tr>
  <td>Password</td>
  <td><input type="password" name="password" maxlength="20"></td>
</tr>
<tr>
  <td></td>
  <td><input type="submit" name="submit" value="Registreer"></td>
</tr>
</table>
</form>
<br/>
<br/>
<br/>
<br/>
<br/>
<?php
require('footer.php');
?>

 

One other script that works different then the above one. Btw this script contains also $_SESSION... just added this into it, but havent put it too all scripts yet. That is my new project: to give the user a option to login automaticly with cookie or just login with session.

 

updatemsg.php

<?php
require ('header.php');

// check if there is an user
if(isset($_COOKIE['name']) OR (isset($_SESSION['name']))){
    // do nothing
} 
else {
    header('Location: ' . $config_basedir);
}

// check if the number id is a number and nothing else
if(isset($_GET['id'])) {
  if(!is_numeric($_GET['id'])){
    header('Location: ' . $config_basedir);
  }
  else {
    $validentry = $_GET['id'];
  }
}
else {
  header('Location: ' . $config_basedir);
}
if($_SERVER['REQUEST_METHOD'] == 'POST'){
  $sql_cat = "SELECT id FROM categorieen AS CategorieMatch WHERE categorie = '".mysql_real_escape_string($_POST['upcategorie'])."'";
  $result = mysql_query($sql_cat);
  if(!$result) {
     trigger_error(mysql_error().' <br/>In query: '.$sql_cat);
  }
   $row = mysql_fetch_assoc($result);
    
  $sql_update = "UPDATE berichten SET cat_id = '". $row['id'] . 
          "', onderwerp = '" . 
          $_POST['uponderwerp']  ."', bericht = '"
  . $_POST['upbericht'] . "' WHERE b_id = '" .
          $validentry . "';";
  if(!mysql_query($sql_update)){
    echo 'Error: Could not connect:' . mysql_error();
  }
  else{  
  header('Location:' . $config_basedir . '/viewmsg.php?id='. 
  $validentry);
  }
}
else {

  $fillsql = "SELECT * FROM berichten WHERE b_id = " . $validentry . ";";
  $fillres = mysql_query($fillsql);
  if (!$fillres){
    echo 'Error: Could not retrieve data:' . mysql_error();
  } 
  $fillrow = mysql_fetch_assoc($fillres);
  ?>  

<form action="" method="post">
<table>
<tr>
  <td><?php echo 'Naam: '; ?></td>
  <td><?php 
            if (isset($_COOKIE['name'])){ echo $_COOKIE['name']; }
            if (isset($_SESSION['name'])){ echo $_SESSION['name'];} 
      ?>
  </td>
</tr>
<!-- Maak een select keuze menu tussen categorien -->
<tr>
  <td>Categorie</td>
  <td><select name="upcategorie">
<?php  
$sql = "SELECT * FROM categorieen"; 
$query = mysql_query($sql);
if (!$query){
    echo 'Error: Could not retrieve data:' . mysql_error();
} 
while ($cat = mysql_fetch_assoc($query)) {
    echo "<option value='".$cat['categorie']."'";
    
    if($cat['id'] == $fillrow['cat_id']){
      echo  ' selected';
      }
      echo '>' .$cat["categorie"]. '</option>';    
}
?> 
</select></td>
</tr>
<tr>
  <td>Onderwerp</td>
  <td><input type="text" name="uponderwerp" size="50" MAXLENGTH="50" value="<?php echo $fillrow['onderwerp'];?>"></td>
</tr>
<tr>
  <td>Bericht</td>
  <td><textarea name="upbericht" rows="10" cols="50" value=" "><?php echo $fillrow['bericht'];?></textarea></td>
</tr>
<tr>
  <td></td>
  <td><input type="submit" name="submit" value="Updaten! "></td>
</tr>
</table>
</form>
<?php } 
require('footer.php')?>

[Muddy_Funster]

My feedback = Post in the right section of the forum! 

[the182guy]

Security:

 

Use exit() after your header redirects because it is possible for a non-logged in user to bypass the login system by simply ignoring the redirect.

 

Have a look into SQL Injection, your UPDATE SQL is vulnerable.

 

In a live/production environment you should remove all of the mysql_error() calls which are shown to the user because this an information disclosure security issue. Show a meaningful but non technical error message that doesn't give any details that woud help an attacker. Also don't just die() with the message, handle it properly and display it within your template.

 

Well done for asking for feedback though - a lot of new coders don't do this and end up with bad code standards/poor practices and vulnerabilities.

[PFMaBiSmAd]

I would add to what the182guy posted. On a live site, you should continue to use mysql_error() to LOG any query errors that are occurring.

 

In a real application, when an error occurs, you should produce and display a USER error message (Sorry, you currently cannot log in to this site due to an error). You should also log all the available information about each error so that you can find and fix what is causing the problem. If you use trigger_error in your error handling logic, php will take the information you supply as the first parameter to trigger_error and produce an user generated error message that includes the file and line number (of the trigger_error statement) that can be logged by setting the log_errors setting to ON.