Stop someone creating a new row in table.

[dachshund]

Hi,

 

The address of one of the features on my website is http://www.webaddress.com/features/view_feature.php?id=13

 

but if someone happens to put in http://www.webaddress.com/features/view_feature.php?id=14

 

it creates a new blank row with id 14 in my mysql table.

 

anyone know how to stop this?

 

thanks

[jesushax]

post your code for the page, we cant tell whats happening without it....

[Psycho]

Validate, validate, validate!

 

Never assume that data coming from the user is valid or safe. It doesn't matter if it is POST or GET data. If '14' is not a valid value for the ID then don't process it.

[fook3d]

Firstly, check a value isset(), then check the id value is numeric, then search the database for a valid result

 

<?php
// If no id, tell them to select one
if(!isset($_GET['id']))
{
echo 'You did not select a feature to view.';
// Change to your footer file/function
include "footer.html";
exit();
}

// Check the value is numeric..
if(isset($_GET['id']) and !eregi("[^0-9]", $_GET['id']))
{
echo 'The feature ID must be numeric.';
// Change to your footer file/function
include "footer.html";
exit();
}

// Now check the database for the value of the post
// If it doesn't exist, the page exit()'s
$this_q = sprintf
(
// The main query
"SELECT * FROM `table_name` WHERE `row_name` = ('%u')",
// The argument
$_GET['id']
);
$DoesExist = mysql_query($this_q) or die (mysql_error());
if(!mysql_num_rows($DoesExist))
{
echo 'This feature does not exist.';
// Change to your footer file/function
include "footer.html";
exit();
}

// Script continues here if no errors in the $_GET['id']
?>

 

A rough way i would do it.

[Wolphie]

As mjdamato said, always assume that data coming from the user is unsafe and invalid. Also, never give too much information as to why something went wrong. For example, if I had a log in form, and entered a username and password, and the username was correct but it was telling me the password was in-correct, then i'd know I would have the correct username. This is an advantage for people trying to exploit it, as such they would know the username. Therefore, if the username and/or password is in-correct display a notice saying "username or password is in-correct" or simply "log in failed".

 

<?php
if(isset($_GET['id']) && is_int($_GET['id'])) {
  $id = (int) mysql_real_escape_string(htmlspecialchars(htmlenities($_GET['id'])));
  $sql = sprintf("SELECT * FROM `db_name` . `table_name` WHERE `id` = '%d'", $id);
  $result = mysql_query($sql) or die('MySQL Error: ' . mysql_error());
  if(mysql_num_rows($result) > 0) {
    // do something
  }
  else {
    // print 'Invalid ID.';
  }
}
else {
  // print 'Invalid ID';
}
?>

 

fook3d:

exit is a language construct, not a function, and therefore parenthesis aren't necessary unless a parameter is required. Such as an error code, or exit('Script Terminated!');

[fook3d]

fook3d:

exit is a language construct, not a function, and therefore parenthesis aren't necessary unless a parameter is required. Such as an error code, or exit('Script Terminated!');

 

I assume you refer to

 

// Change to your footer file/function
include "footer.html";

 

Which can be either a file (Eg. footer.html) or a function (Eg. PageFooter()) along with other things, like class values ($variable->footer) etc.

[Wolphie]

No, i'm referring to your use of exit();

[dachshund]

Ok, thanks, here's my code, where should i add the new parts?

<?php
include "../template/header.php";
?>
<link href="../template/style.css" rel="stylesheet" type="text/css" />    
<tr>
<td width="760" valign="top">

      
  
<table align="center" bgcolor="#FFFFFF" width="100%" cellpadding="5" cellspacing="0">
    	<tr>
        <td align="left" valign="bottom" class="secondtitle">
        Interviews
        </td>
        </tr>
        <tr>
    		<td valign="top" align="left">
            
            
			<table width="100%" border="0" align="left" cellpadding="5">
                
<?php
$id=$_GET['id'];

$sql="SELECT * FROM interviews WHERE id='$id'";
$result=mysql_query($sql);
if ($rows=mysql_fetch_array($result)){
$sql = "update interviews set views=views+1 WHERE id='$id'";
mysql_query($sql);
}
else {
    $sql="INSERT INTO interviews(id, views) VALUES('$id', 1)";
mysql_query($sql);
}

?>

				<tr>
                    	<td width="100%" align="left" valign="top" class="contentmaintitle">
                        	<? echo $rows['title']; ?>                        </td>
                  </tr>
                    <tr>
                    	<td width="100%" align="justify" valign="top" class="maincontent">
                        	<? echo $rows['content']; ?>                        </td>
                    </tr>
                    <tr>
                    	<td width="100%" align="justify" valign="top" class="maincontent">
                        <a href="<? echo $rows['link']; ?>"><? echo $rows['link']; ?></a>                        </td>
                    </tr>
                    <tr>
                    	<td width="100%" align="left" valign="top" class="wordsby">
                       		<? echo $rows['wordsby'];	?>                        </td>
                    </tr>
                                            <?php
mysql_close();
?>
</table>			</td>
        </tr>
   </table> 

<?php
include "../template/footer.php";
?>

 

 

[.josh]

1) you need to sanitize your variable

 

$id=$_GET['id']; ...

$sql = "update interviews set views=views+1 WHERE id='$id'";

 

You should feel very lucky that someone ONLY made id=14

 

2) You need a permissions system in place.  A login script with permission levels for only certain people to even access that page in the first place. 

 

 

 

[dachshund]

ok, how do i do that?

[.josh]

As far as sanitizing goes: You expect for $id to have a number in it, so check to make sure it's a number.  Also use mysql_real_escape_string() on your stuff.

 

As far as log in and user levels go...there are a ton of tutorials on how to do that out there.

[dachshund]

would it not just be possible to place the code fook3d posted into my code?

 

 

[.josh]

his posted code checks to make sure it is numerical but it does nothing to stop someone from entering in a number that doesn't exist in your database and therefore triggering your insert...which is the concern of your original post.

[dachshund]

oh right ok, thanks

[Psycho]

Unless I am misunderstanding what you are trying to accomplish, you do not need the SELECT or INSERT queries at all!

 

It appears to me that your code is simply trying to update the number of times a record is viewed. If the record does not exist why do you need to add one? You just need to validate that the value passed is a integer and run a single query. If the int doesn't exist in the database the statement will do nothing

 

<?php

$id=$_GET['id'];
if ($id == (int)$id) {
   $sql = "UPDATE interviews SET views=views+1 WHERE id='$id'";
   mysql_query($sql);
}

?>/code]

[fook3d]

No, i'm referring to your use of exit();

 

I see, guess that is a personal preference thing, for me, i find codes easier to read where every "if" statement is seperate, others prefer to use "else if" too, whereas i don't like using them. Doubt it makes much difference which way you use it to be honest. Maybe you could tell me if there is a reason to use "else if" over "if's and exits" ?

 

his posted code checks to make sure it is numerical but it does nothing to stop someone from entering in a number that doesn't exist in your database and therefore triggering your insert...which is the concern of your original post.

 

<?php
// If it doesn't exist, the page exit()'s
$this_q = sprintf
(
// The main query
"SELECT * FROM `table_name` WHERE `row_name` = ('%u')",
// The argument
$_GET['id']
);
$DoesExist = mysql_query($this_q) or die (mysql_error());
if(!mysql_num_rows($DoesExist))
{
echo 'This feature does not exist.';
// Change to your footer file/function
include "footer.html";
exit();
}
?>

 

Sure does block the insert query in the page if that is above it, all data is checked before this also, so should be pretty secure

[dachshund]

Thanks mjdamato

 

That is basically what i'm trying to do, but when I use that code it doesn't seem to be able to GET the information properly, and just displays a blank page.

 

Any ideas why?

[Psycho]

Just add some debugging code to see what is going on:

 

<?php

$id=$_GET['id'];
echo "ID passed on query string [".$id."]<br>";

if ($id == (int)$id) {
echo "ID is an int<br>";
    $sql = "UPDATE interviews SET views=views+1 WHERE id='$id'";
echo "Query: $sql<br>";
    mysql_query($sql) or die (mysql_error());
}

?>

[dachshund]

ok,

 

comes back

 

ID passed on query string [12]

ID is an int

Query: UPDATE interviews SET views=views+1 WHERE id='12'

 

 

[Psycho]

Well, after you run the update you will need to run the SELECT if you want to display the data. You will also want to create content to display for the error conditions (ID not an int, ID not in database, etc.)

 

<?php

$id=mysql_real_escope_string($_GET['id']);

if ($id !== (int)$id) {
    echo "Invalid ID";
} else {
    $sql = "UPDATE interviews SET views=views+1 WHERE id='$id'";
    mysql_query($sql) or die (mysql_error());

    $sql="SELECT * FROM interviews WHERE id='$id'";
    $result = mysql_query($sql) or die (mysql_error());
    $rows=mysql_fetch_array($result)

}

?>

[dachshund]

Ok almost there, but when I try that it always returns "Invalid ID" even if it isn't.

[Psycho]

Try this

 

 

if ($id != (int)$id) {

[dachshund]

ah that's worked, it stops new rows being produced, but for some reason it doesn't echo "Invalid ID", it's just blank.

[Psycho]

I'm guessing it may have something to do with the fact that you have your code within a table. If you try to put text between table tags that are not also within TD tags strange things may happen. Check the HTML output to see if that message is there and then determine how to format that erro message to correctly display on the page.