editing mysql table with html form

Question

My aim is to have a simple, form based CMS so the client can log in and edit the MySQL table data via an html form. The login is working, but the edit page isn't returning the values from the MySQL table, nor am I getting any errors.

I'm still amateur, and I first started the following code for a class project, but now plan to implement it for a live site. From what I understand I shouldn't have to declare the next/previous/etc. variables at the top, which I tried unsuccessfully to do so anyway. Does anything stand out to any of you?:

<?php

echo "<h2>Edit Special Offer</h2><hr>";

if (isset($_COOKIE["username"]))
    {
    echo "Welcome " . $_COOKIE["username"] . "!<br />";
    include "login.php";
    }
else
  echo "You need to log in to access this page.<br />";


if(isset($previous))
{
$query = "SELECT id, specialtitle, specialinfo
FROM special WHERE id < $id ORDER BY id DESC";
$result = mysql_query($query);
check_mysql();
$row = mysql_fetch_row($result);
check_mysql();
if ($row[0] > 0)
{
$id = $row[0];
$specialtitle = $row[1];
$specialinfo = $row[2];
}
}


elseif (isset($next))
{
$query = "SELECT id, specialtitle, specialinfo
FROM special WHERE id > $id ORDER BY id ASC";
$result = mysql_query($query);
check_mysql();
$row = mysql_fetch_row($result);
check_mysql();
if ($row[0] > 0)
{
$id = $row[0];
$specialtitle = $row[1];
$specialinfo = $row[2];
}
}



elseif (isset($add))
{
$query = "INSERT INTO special (specialtitle, specialinfo)
VALUES ('$specialtitle', '$specialinfo')";
$result = mysql_query($query);
check_mysql();
$id = mysql_insert_id();
$message = "Special Offer Added";
}


elseif (isset($update))
{
$query = "UPDATE special
SET specialtitle='$specialtitle', specialinfo='$specialinfo'
WHERE id = $id";
$result = mysql_query($query);
check_mysql();
$id = mysql_insert_id();
$message = "Monthly Special Updated";
}


elseif (isset($delete))
{
$query = "DELETE FROM special WHERE id = $id";
$result = mysql_query($query);
check_mysql();
$specialtitle = "";
$specialinfo = "";
$message = "Special Offer Deleted";
}
$specialtitle = trim($specialtitle);
$specialinfo = trim($specialinfo);
?>



<form method="post" action="editspecial.php">
<p><b>Special Offer</b>
<br><input type="text" name="specialtitle" <?php echo "VALUE=\"$specialtitle\"" ?>> </p>

<p><b>Special Info/Description</b>
<br><textarea name="specialinfo" rows="8" cols="70" >
<?php echo $specialinfo ?>
</textarea> </p>

<br>
<input type="submit" name="previous" value="previous">
<input type="submit" name="next" value="next">
<br><br>
<input type="submit" name="add" value="Add">
<input type="submit" name="update" value="Update">
<input type="submit" name="delete" value="Delete">
<input type="hidden" name="id" <?php echo "VALUE=\"$id\"" ?>>
</form>
<?php
if (isset($message))
{
echo "<br>$message";
}
?>

Login.php:

<?php

function check_mysql()
{
if(mysql_errno()>0)
{
die ("<br>" . mysql_errno().": ".mysql_error()."<br>");
}
}
$dbh=mysql_connect ("xxxxxxxxxxxxxxxxx","xxxxxxxx","xxxxxxxx");
if (!$dbh)
{
die ("Failed to open the Database");
}
mysql_select_db("xxxxxx");
check_mysql();
if(!isset($id))
{
$id=0;
}

?>

Answer 1

I don´t know what's happening in login.php, but you're using $id before it is set. That´s just in the first part.

Edit: To clarify, you are using $id in every query statement and setting it afterwards, my guess would be that $id is null and that is why nothing gets returned.

Edit 2: What else is happening in login.php? If you never read your $_POST variables, nothing will ever happen.

Edit 3: Like I already partly said in a comment, your if(isset($previous)) section, elseif (isset($update)) section and elseif (isset($delete)) sections will never do anything as $id is always 0.

After authenticating your user you need to get and filter the posted variables, $_POST['id'], $_POST['previous'], etc.

Answer 2

Be careful with your code there. Your not filtering your cookie value and you shouldn't be storing a username directly in there as it can be easily changed by the visitor. You should look into filter_input for filtering cookie data and eany form data that is being submitted - especially your $_POST['id']

this will save you a lot of heartache further down the line from attacks.

Your if else statements are checking if variables are set but you dont set next, previous, add etc

You are using submit buttons with those values so you would need to check for

if(isset($_POST['previous']))

instead of yours which is

if(isset($previous))

I can't see where you set your database details either unless you have an included file somewhere that you haven't posted. (don't post the real ones of course but i can't see anything)

Answer 3

"Welcome " . $_COOKIE["username"] . "!<br />"; [and many other places]

HTML-injection leading to cross-site security holes. You need to use htmlspecialchars every time you output a text value to HTML.

"INSERT INTO special (specialtitle, specialinfo) VALUES ('$specialtitle'  [and many other places]

SQL-injection leading to database vandalism. You need to use mysql_real_escape_string every time you output a text value to an SQL string literal.

if (isset($_COOKIE["username"]))

Cookies are not secure, anyone can set a username cookie on the client-side. Don't use it for access control, only as a key to a stored or session user identifier.

You also appear to be using register_globals to access $_REQUEST values as direct variables. This is another extreme no-no.

Between all these security snafus you are a sitting duck for Russian hackers who will take over your site to push viruses and spam.

Answer 4

Please please please do a little bit more learning before attempting to build this thing. You can do it the way you are doing it, but with just a small amount of extra knowledge about OO programming, and maybe about the Pear db classes you will have 3x cleaner code.

If you really choose not to, at the very least, pull each of your save, update, delete, etc procedures out into functions instead of just inlining them in your code. put them in a separate file, and include it in that page.

It may not be useful to you, but I am going to dump a generic table access class here in the page for you. It requires a simple db class API, but if you use this or something like it your life will be 5x easier.

If you don't understand this code when you look at it, that's ok, but please just come back and ask questions about the stuff you don't understand. That is what stackoverflow is for. This is an older class that should just do basic stuff. Sorry it's not better I just wanted to dig something out of the archives for you that was simple.

<?php
// Subclass this class and implement the abstract functions to give access to your table
class ActiveRecordOrder
{

    function ActiveRecordOrder()
    {
    }

    //Abstract function should return the table column names excluding PK
    function getDataFields()
    {}

    //Abstract function should return the primary key column (usually an int)
    function getKeyField()
    {}

    //abstract funtion just return the table name from the DB table
    function getTableName() 
    {}

    /*
    This function takes an array of fieldName indexed values, and loads only the
    ones specified by the object as valid dataFields.
    */
    function loadRecordWithDataFields($dataRecord)
    {
        $dataFields = $this->getDataFields();
        $dataFields[] = $this->getKeyField();
        foreach($dataFields as $fieldName)
        {
            $this->$fieldName = $dataRecord[$fieldName];
        }
    }

    function getRecordsByKey($keyID, &$dbHandle)
    {
        $tableName = $this->getTableName();
        $keyField  = $this->getKeyField();
        $dataFields = $this->getDataFields();
        $dataFields[] = $this->getKeyField();

        $results = $dbHandle->select($tableName, $dataFields, array($keyField => $keyID));

        return $results;
    }

    function search($whereArray, &$dbHandle)
    {
        $tableName = $this->getTableName();
        $dataFields = $this->getDataFields();
        $dataFields[] = $this->getKeyField();
        return  $dbHandle->select($tableName, $dataFields, $whereArray);
    }

    /**
    * Since it is *hard* to serialize classes and make sure a class def shows up
    * on the other end. this function can just return the class data.
    */
    function getDataFieldsInArray()
    {
        $dataFields = $this->getDataFields();
        foreach($dataFields as $dataField)
        {
            $returnArray[$dataField] = $this->$dataField;
        }
        return $returnArray;
    }

    /**
    * Added update support to allow to update the status
    * 
    * @deprecated - use new function saveObject as of 8-10-2006 zak
    */
    function updateObject(&$dbHandle)
    {

        $tableName = $this->getTableName();
        $keyField = $this->getKeyField();
        $dataArray = $this->getDataFieldsInArray();

        $updatedRows = $dbHandle->updateRow(
            $tableName,
            $dataArray, 
            array( $keyField => $this->$keyField )
        );

        return $updatedRows;
    }   

    /**
    * Allows the object to be saved to the database, even if it didn't exist in the DB before.
    * 
    * @param mixed $dbhandle
    */
    function saveObject(&$dbhandle)
    {
        $tableName = $this->getTableName();
        $keyField = $this->getKeyField();
        $dataArray = $this->getDataFieldsInArray();

        $updatedRows = $dbHandle->updateOrInsert(
            $tableName,
            $dataArray, 
            array( $keyField => $this->$keyField )
        );

        return $updatedRows;
    }   
}

Answer 5

I revise my question. My client may need this functionability faster than I can master the methods you've all laid out(for which I am grateful).

What if I installed phpMyEdit, and secured the edit form with .htaccess? Would that provide adequate security?