[RESOLVED] some data has disappeared - is this my coding ?!

VisionsOfCody

I've got kind of a strange problem with data that disappears from certain tables in the db

this happened on 3 different sites with 3 different admins but all based on a simple admin that i set up - now it's happened 3 times i can see there's a pattern to it but I'm stumped as to what it could be

the tables in question contain category names and ids which are then used in the products table for sorting the products into categories

what's happening is that the products table is fine but the table holding the categories just suddenly gets completely emptied apart from the default category with an id of zero - this would suggest that the emptying is being done by my php code because I've made it so that the user cannot delete the default category - also, in the products table the cat_id gets toggled to 0 so it must be coming from the code

here's the code used to delete categories - it's a click on a symbol in the page that triggers it

if($_GET['action'] == "delete"){
		$thisID = $_GET['deleteID'];


	$query2="UPDATE prods SET cat_id='0' , prod_online='0' WHERE (cat_id='$thisID')";
	mysql_query($query2);
	//echo "<br>".$query2;


	$query="DELETE FROM cat WHERE cat_id='$thisID' LIMIT 1";
	mysql_query($query);
	//echo "<br>".$query;


}

the html used to trigger the delete look like this :

<a href="page.php?action=delete&deleteID=11" onclick="return confirm('Etes-vous sûr de vouloir supprimer cette catégorie ?')"><img src="images/trash.gif" title="Supprimer cette catégorie" border="0" /></a>

I really cannot see where there could be a problem with the code - is there something really stupidly obvious I've missed here ?!!

thanks

leatherback

Sounds to me that somebody may be messing with your script!?

this: $thisID = $_GET['deleteID'];

Is open for external spicing of your code.

Say someone punches this in:

www.domain.com/page.php?action=delete&deleteID=%

What would happen?

Also.. how do you validate that people ar allowed to delete?

VisionsOfCody

arg !

access to the admin is password-controlled - in theory if you managed to work out the path to the admin pages (that i have made very tricky) then you'd be chucked out by the authorization session not being set - there's a check in the first line of code

also it would presumably delete all the categories including the default category which has an id of 0 - but this one was left intact

leatherback

Hm.. The code you showed:
- Does not check for a password
- Does not limit the deletion of row 0.

This means you have posted only a subset of the code. If the problem I showed you is NOT the problem, you might have to show the rest of the code. have you tried the example I gave you/ What happens? What happens if someone is logged in and does that?

Piranha

Search for "database injection" to see how you can solve at least part of this problem.

VisionsOfCody

ARG!!

Leatherback - I have just tried using that link you posted

I created a new category and then used the link with the new category id and even though I was chucked out by the authorization check THE BIT OF CODE STILL RAN AND DELETED THE CATEGORY !!!!

this means that my authorization thing has a flaw - how can i prevent the rest of the code in the page from executing if the 2 authorization sessions are not set ?

do you think I should also check for authorization in that bit of code I posted as well as for the page in general ?

and how could someone work out the exact path to the page in question ?

leatherback

You should ALWAYS check authorisation.

That being said:

I use a constant, which I set in the main processing pages:

define('_MAINPAGESET', true);

In each include file you place on top of the file:

if(! defined('_MAINPAGESET'))
  {
  die('Sorry, please do not try to load pages this way looser');
  }

which will effectively stop them from loading a page outside the main pages.

You should check the authorization routine, perhaps post a snippet?

People can figure out what a file is called if you have allowed them to retrieve an index of the folder? Or even just brute force..

VisionsOfCody

would this be safe

if(($_GET['action'] == "delete")&&($_SESSION['logged'] == "xxx")&&($_SESSION['key'] == "xxx")){

	$thisID = mysql_real_escape_string($_GET['deleteID']);

	$query2="UPDATE prods SET ville_id='0' , prod_online='0' WHERE (ville_id='$thisID')";
	mysql_query($query2);


	if($thisID!='0'){
		$query="DELETE FROM villes WHERE ville_id='$thisID' LIMIT 1";
		mysql_query($query);
	}


}

leatherback

Oh..

If(! isset($session['valid_login']))
  {
  die();
  // or
  header(location:login.php);
  }
else
  {
  // all other code here
  }

leatherback

VisionsOfCody wrote:

would this be safe

if(($_GET['action'] == "delete")&&($_SESSION['logged'] != "xxx")&&($_SESSION['key'] != "xxx")){

	$thisID = mysql_real_escape_string($_GET['deleteID']);

	$query2="UPDATE prods SET ville_id='0' , prod_online='0' WHERE (ville_id='$thisID')";
	mysql_query($query2);


	if($thisID!='0'){
		$query="DELETE FROM villes WHERE ville_id='$thisID' LIMIT 1";
		mysql_query($query);
	}


}

Instead of trusting what they post, you might want to check:

it is an id: Should be numeric:

if(is_numeric($thisID))
  {
// safe to proceed
  }
else
  {
// reload main page?
  }

VisionsOfCody

that is_numeric() is neat

my auth check was this

if(($_SESSION['logged'] != "xx")||($_SESSION['key'] != "xx")) {
	$page_location = "index.php";
	header ("Location: ". $page_location);
}


// rest of code here

but the bit of code still ran in that page which makes me think that i should have put an if/else instead of just an if - rather stupid of me :o

leatherback

I never rely on just one set. also..
!= means (I think) if you cannot ASSIGN the value to is.
try:

if(("XX" !== $_SESSION['logged']) || ("xx" !== $_SESSION['key'])) {

edit: By reversing the statement, you will get an error when it tries to assign something, instead of silently just doing what you do not intent it to do.

VisionsOfCody

excellent - I've learned loads of good stuff here - though it does mean I'm going to have to go through my sites and tighten up the code

thanks