edit data

[andibakker]

Hi, I am very new to php and am writing a little system with a lot of maintenance screens. I want to base all of my edit screens on the code below. Please review and check it for security and make suggestions.
Thanks

<?php
  session_start();
  include('dbc.php');
	include('functions.php');

if ($_SERVER['REQUEST_METHOD'] == 'GET')
//display the form for user update
{
      $drivercode = $_GET['id'];	
			$driverresult = @mysql_query(
		   "SELECT driverCode, driverName, driverLink FROM driver where driverCode = $drivercode");
			 if (!$driverresult) 
			 {
			    exit('<p>' . 'Error fetching this driver : ' . mysql_error() . '</p>');
				}

		list($drivercode, $drivername, $driverlink) = @mysql_fetch_array($driverresult);

		// set up the defaults to the retrieved database values for display
		$defaults['drivercode'] = $drivercode;
		$defaults['drivername'] = htmlentities($drivername);
		$defaults['driverlink'] = htmlentities($driverlink);					   

		display_form(array(), '', $defaults);
}
else {
		 // the request is a post 
		 // set up the defaults to the posted value

		 $defaults['drivercode'] = isset($_POST['id']) ? htmlentities($_POST['id']) : ''; 
		 $defaults['drivername'] = isset($_POST['drivername']) ? htmlentities($_POST['drivername']) : ''; 
     $defaults['driverlink'] = isset($_POST['driverlink']) ? htmlentities($_POST['driverlink']) : '';

		 // validate for errors 
		 $errors = validate_driver();
		 if (count($errors)) 
		 {
		   //there were errors, so redisplay the form with errors
			 display_form($errors,'', $defaults);
		 }
		 else {
		   //the user input has been validated - we can now validate against the database
			 // $msg = validate_driver_db();
		   //process the form and display the form with user input and success message

			 $upddrivercode = $defaults['drivercode'];
			 $upddrivername = $defaults['drivername'];
			 $upddriverlink = $defaults['driverlink'];
			 $updatedriver = "UPDATE driver 
			                  SET drivername = '$upddrivername', 
												    driverlink = '$upddriverlink'
												WHERE drivercode = '$upddrivercode'";

         if (@mysql_query($updatedriver)) 
			   {
			    $msg = 'Driver successfully updated !';
		     }
			   else
			   {
			    $msg = 'error updating the driver ' . mysql_error();
			   }
			 display_form(array(),$msg, $defaults);			 
		 }
}

function display_form($errors, $msg, $defaults) 
{

?>
<form action="<?php echo htmlentities($_SERVER['PHP_SELF'], ENT_QUOTES, 'utf-8'); ?>" method="post">
  Driver Name : <input type="text" name="drivername" value='<?php echo $defaults['drivername'] ?>'/> <?php print_error('drivername', $errors) ?> <br>
  Driver WebPage : <input type="text" name="driverlink" value='<?php echo $defaults['driverlink'] ?>'/>
	<input type="hidden" name="id" value='<?php echo $defaults['drivercode'] ?>'/>  
  <br />
  <input type="submit" name="updatedriver" value="Update Driver" />
  <input type="reset" name="reset" value="Reset" />
	<br />
	<a href="driver.php">Back to Driver Maintenance</a>
	</br>
	<?php print $msg; ?>
  </form>
<?php 
}

?>

[bradgrafelman]

Some general comments:

Your code is vulnerable to SQL injection attacks; user-supplied data should never be placed directly into SQL queries. Instead, it must first be sanitized with a function such as [man]mysql_real_escape_string/man.

Get rid of all of the '@' error suppressors; when debugging, you want all errors to be reported. When the site goes live, display_errors shouldn't even be turned on anyway.

Speaking of hiding errors, don't forget that you'll want to remove calls to [man]mysql_error/man when the site goes live; displaying MySQL error messages to users isn't desirable (besides, it was never meant for them anyway).

[Bjom]

Your code is vulnerable to SQL injection attacks; user-supplied data should never be placed directly into SQL queries. Instead, it must first be sanitized with a function such as mysql_real_escape_string().

Very true and the solution given works perfectly fine and is easy to do.

Another approach: Snce you are interested in improving on your code anyway,you might want to switch to the mysqli extension and use prepared statements. Prepared statements have built in functionality like the mysql_real_escape_string() and other niceties.

Bjom