Guidance on simple database insert,update, processing script

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
tymlls05
Forum Commoner
Posts: 30
Joined: Tue Nov 01, 2005 1:30 pm

Guidance on simple database insert,update, processing script

Post by tymlls05 »

Hi,

I have recently made an effort to stop taking other peoples scripts and customizing them to my needs. Though, that was a great first step for me in learning the language. I have started writing php from notepad so that I may see every line of code for what it is and learn more about debugging errors.

Anyways, I am here to post a simple script (and database) I wrote yesterday so that it may be critiqued. I feel like I have great ideas for scripts but also feel like my coding designs are inefficient. Maybe this post belongs in theory and design? I apologize if it does. Hope you guys can tell me how I am not taking full advantage of the PHP language haha.

Here is my database:

Code: Select all

-- phpMyAdmin SQL Dump
-- version 3.1.3.1
-- http://www.phpmyadmin.net
--
-- Host: localhost
-- Generation Time: Aug 27, 2009 at 10:09 AM
-- Server version: 5.1.33
-- PHP Version: 5.2.9
 
SET SQL_MODE="NO_AUTO_VALUE_ON_ZERO";
 
--
-- Database: `apps`
--
 
-- --------------------------------------------------------
 
--
-- Table structure for table `rentroll`
--
 
CREATE TABLE IF NOT EXISTS `rentroll` (
  `uid` int(11) NOT NULL AUTO_INCREMENT,
  `address` varchar(50) NOT NULL,
  `due` varchar(20) NOT NULL,
  `paid` varchar(20) NOT NULL,
  `datepaid` varchar(50) NOT NULL,
  `tenant` varchar(50) NOT NULL,
  `appname` text NOT NULL,
  PRIMARY KEY (`uid`)
) ENGINE=MyISAM  DEFAULT CHARSET=latin1 AUTO_INCREMENT=57 ;
 
--
-- Dumping data for table `rentroll`
--
 
INSERT INTO `rentroll` (`uid`, `address`, `due`, `paid`, `datepaid`, `tenant`, `appname`) VALUES
(1, '502 S. Address 1', '1250', '0', '', ' . ', ''),
(2, '542 N. Address 1', '1200', '0', '', ' . ', ''),
(3, '5452 W. Address 1', '12000', '0', '', ' . ', ''),
(4, '909 Address Ave.', '0', '0', '0', ' . ', '');
Here is my php page.

Code: Select all

<style>
    body {
        font-family:arial;
    }
    A:link, A:visited {
        color:#000;
    }
    td {
        vertical-align:top;
    }
</style>
 
<?
 
$con=mysql_connect(localhost,user_root,user_password);
mysql_select_db("apps", $con);
 
function protect($string) {
    $string = mysql_real_escape_string($string);
    return $string;
}
 
date_default_timezone_set('America/Texas');
 
if ($_GET['print']) {
    echo 
        '<style>
            input{display:none;}
            .end {display:none; width:1px;}
        </style>
        <body onload="javascript&#058;window.print();"><center><a onclick="history.back()">RETURN</a>';
} else {
        echo'<center><a href="?print=1">PRINT</a><p>';
}
 
// get the content from the table.
$get = mysql_query("SELECT * FROM rentroll ORDER BY address ASC") or die(mysql_error());
$count_rows = mysql_num_rows($get);
// display each message title, with a link to their content
echo    '<center><table style="width:768px; text-align:center; vertical-align:top; border:1px solid #000;">
            <tr>
                <td colspan=6><font style="font-size: 30px; margin:3px; font-family:tahoma;"><b>RENT ROLL<br>STAR OF SAN ANGELO</font>
                    <form action="rent_roll.php" method="post"><input name=addnew value="Add New" type=submit></form>
                </td>
            </tr>
            <tr>
                <td style="border-bottom:3px double #999;">Address</td>
                <td style="border-bottom:3px double #999;">AMT Due</td>
                <td style="border-bottom:3px double #999;">AMT Paid</td>
                <td style="border-bottom:3px double #999;">Date Paid</td>
                <td style="border-bottom:3px double #999; width:200;">Tenant</td>
                <td class=end></td>
            </tr>
        ';
if ($_POST['update']) {
    $grabitem = mysql_query("SELECT * FROM `rentroll` WHERE `uid`=$_POST[uid]") or die(mysql_error());  
    // keeps getting the next row until there are no more to get
    while($item = mysql_fetch_array($grabitem)) {
        // Print out the contents of each row into a table
        $euid=$item['uid'];
        $eaddress=$item['address'];
        $edue=$item['due'];
        $epaid=$item['paid'];
        $edpaid=$item['datepaid'];
        $etenant=$item['tenant'];
        $eappname='Rent Roll Star of San Angelo';
    }
    echo '  <tr style="background-color:lightgray;">
                <td style="width:160; height:100%;">
                    <form action="rent_roll.php" method="post">
                        <input type=hidden name="uid" value="' .$euid. '">
                        <input type=hidden name="updated" value="1">
                        <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:12px" name="address">'. $eaddress. '</textarea>
                </td>
                <td style="width:150;">
                    <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:18px; font-weight:bold;" name="due">' .$edue. '</textarea>
                </td>
                <td style="width:150; border:1px solid #000;">
                        <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:18px; font-weight:bold;" name="paid">' .$epaid. '</textarea>
                </td>
                <td style="width:150;border:1px solid #000;">
                        <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:14px; font-weight:bold;" name="datepaid" >' .$edatepaid. '</textarea>
                </td>
                <td style="width:140; border:1px solid #000;">
                        <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:12px; font-weight:bold;" name="tenant">' .$etenant. '</textarea>
                </td>
                <td style="width:40px; border:1px solid #000;" class=end>
                        <input type=hidden name="upadte" value="1"><input type=submit value=update></form>
                </td>
            </tr>';
}
 
if ($_POST['updated']) {
    $address =$_POST["address"];
    $udue =$_POST[due];
    $upaid=$_POST[paid];
    $udatepaid=$_POST[datepaid];
    $utenant=$_POST[tenant];
    if (!$address) $address=' . ';
    if (!$udue) $udue=' . ';
    if (!$upaid) $upaid=' . ';
    if (!$utenant) $utenant=' . ';
    $additem = mysql_query('UPDATE `rentroll` SET `address` = "'.$address.'",`due` = "'.$udue.'",`paid` = "'.$upaid.'",`datepaid` = "'.$udatepaid.'",`tenant` = "'.$utenant.'",`appname` = "'.$eappname.'" WHERE `uid` = "'.$_POST[uid].'" LIMIT 1 ;') or die(mysql_error());  
    echo '  <tr>
                <td colspan=6 align=left><meta http-equiv="refresh" content="2;url=?updated=1" />
                    <center>You have updated '.$uaddress .'</center>
                    <form action="rent_roll.php" method="post">
                        <input name=addnew value="Add New" type=submit>
                    </form>
                </td>
            </tr>';
} else {
    if ($_POST['addnew']||$_GET['addnew']) {
        $address =$_POST["address"];
        $udue =$_POST[due];
        $upaid=$_POST[paid];
        $udatepaid=$_POST[datepaid];
        $utenant=$_POST[tenant];
        if (!$address) $address=' . ';
        if (!$udue) $udue=' . ';
        if (!$upaid) $upaid=' . ';
        if (!$utenant) $utenant=' . ';
        if ($_POST['insert']) {
            $insert=mysql_query('INSERT INTO `rentroll` (`address`,`due`,`paid`,`datepaid`,`tenant`,`appname`) 
                        VALUES (\''.$address.'\',\''.$udue.'\',\''.$upaid.'\',\''.$udatepaid.'\',\''.$utenant.'\',\''.$eappname.'\')') or die(mysql_error());  
            echo '  <meta http-equiv="refresh" content="0;url=?addnew=1" /> You have added '.$uaddress; }
            echo '  <tr style="background-color:lightgray;">
                        <td style="width:160; height:100%;">
                            <form action="rent_roll.php" method="post">
                                <input type=hidden name="uid" value="' .$euid. '">
                                <input type=hidden name="addnew" value="1">
                                <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:12px" name="address">'. $eaddress. '</textarea>
                        </td>
                        <td style="width:150;">
                                <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:18px; font-weight:bold;" name="due">' .$edue. '</textarea>
                        </td>
                        <td style="width:150; border:1px solid #000;">
                                <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:18px; font-weight:bold;" name="paid">' .$epaid. '</textarea>
                        </td>
                        <td style="width:150;border:1px solid #000;">
                                <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:14px; font-weight:bold;" name="datepaid" >' .$edatepaid. '</textarea>
                        </td>
                        <td style="width:140; border:1px solid #000;">
                                <textarea type=text style="width:95%; height:80; border:1px solid #000; font-family:arial; font-size:12px; font-weight:bold;" name="tenant">' .$etenant. '</textarea>
                        </td>
                        <td style="width:40; border:1px solid #000;">
                                <input type=hidden name="insert" value="1"><input type=submit name=addnew value="GO"></form>
                        </td>
                    </tr>';
    }
}
 
for($count = 1; $count <= $count_rows; $count++){
    $row=mysql_fetch_array($get);
    $uid=$row['uid'];
    $address=$row['address'];
    $due=$row['due'];
    $due=number_format($due,2);
    $paid=$row['paid'];
    $paid=number_format($paid,2);
    $datepaid=$row['datepaid'];
    $tenant=$row['tenant'];
    $appname=$row['appname'];
    echo '
        <tr style="">
            <td style="width:160; height:100%;border:1px solid #000;"">
                <form action="rent_roll.php" method="post" name=update>
                    <input type=hidden name="uid" value="' .$uid.'">'.$address.'<p><br><p><br>
            </td>
            <td style="width:150;border:1px solid #000;">'.$due.'</td>
            <td style="width:150;border:1px solid #000;">'.$paid.'  </td>
            <td style="width:150;border:1px solid #000;">';if (!$datepaid) {echo'n/a';}else{echo $datepaid;} echo'</td>
            <td style="width:200; border:1px solid #000;">'.$tenant.'</td>
            <td style="width:40; border:1px solid #000;" class=end>
                    <input type=hidden name="appname" value="'.$appname.'"><input name=update type=submit value=update></form>
            </td>
        </tr>';
}
echo '</table>';
?>
User avatar
Darhazer
DevNet Resident
Posts: 1011
Joined: Thu May 14, 2009 3:00 pm
Location: HellCity, Bulgaria

Re: Guidance on simple database insert,update, processing script

Post by Darhazer »

First check your datatypes. You are using varchar for datepaid, probably date, timestamp is more suitable

Code: Select all

$grabitem = mysql_query("SELECT * FROM `rentroll` WHERE `uid`=$_POST[uid]") or die(mysql_error());
You have an SQL injection vulnelabirity in this line. you have to cast the $_POST[uid] to int
Also, $_POST['uid'] is the proper way
$_POST[uid] will check for a constant called uid and if such is not find, it will raise warning and then change it to $_POST['uid'].

Update statement is vulnerable to SQL injection as well. Although you have a function protect, you are not using it.

The output is vulnerable to XSS attacks.

You have to separate the add, edit and list in different functions.
User avatar
Benjamin
Site Administrator
Posts: 6935
Joined: Sun May 19, 2002 10:24 pm

Re: Guidance on simple database insert,update, processing script

Post by Benjamin »

:arrow: Moved to Coding Critique
Post Reply