-1

i've form which is like:

<form action="del.php" method="post" enctype="multipart/form-data">
Number of field to show:<input type="text" name="limit" /><br /><br /> 
Top category: 
<select name="topcat"> 
<option>tech</option>
<option>automobile</option>
</select><br /><br />
Base category: 
<select name="cat"> 
<option>mobile</option>
<option>computer</option>
</select> 

<input type="submit" />
</form>

i'm trying to write code to delete particular id from database selected from page: del.php :

<head>
<?php
require_once('globals.php');// for database connection
?>
</head>
<body>
<?php

        $cat = $_POST['cat'];
    $topcat = $_POST['topcat'];
    $limit = $_POST['limit'];

if(isset($_GET['delete']))
{

    $query = 'DELETE FROM '.$cat.' WHERE id = '.(int)$_GET['delete'];
    echo $query;
    $result = mysql_query($query);
}



$query = 'select id,headline from '. $cat.' order by date DESC limit 0,'.$limit;
$result = mysql_query($query);
while($row = mysql_fetch_assoc($result))
{

$headline=$row['headline'];

    echo '<div class="record" id="record-'.$row['id'].'">
                <a href="?delete='.$row['id'].'" class="delete">Delete</a>
                <strong>'.$headline.'</strong>
            </div>';
}
?>

</body>

Problem: when isset($_GET['delete'] get executed, it says undefined variable cat . but $cat is defined as parsed from previous form. any solution?

  • You don't need `enctype="multipart/form-data"` when there are no files to be submitted. – PurkkaKoodari May 12 '13 at 07:18
  • $cat can't be empty, please post your php error. – nacholibre May 12 '13 at 07:19
  • @Pietu1998: ok removed that and checked... problem still persist. – user2320375 May 12 '13 at 07:21
  • $cat is actually undefined cause if you submit your form its post request and you have access to the value of $cat.. but the delete value does not exist.. now since you are trying to access $_GET["delete"] value .. that means you have a get request on page and there is no post request due to which you never submit cat value via form therefore your $cat is undefined.. – Dinesh May 12 '13 at 07:22
  • What you are doing is terribly insecure. You are **wide open** to SQL injection attacks and **you will be hacked** if you haven't been already. Learn to use prepared/parameterized queries with PDO or similar to avoid this problem entirely. – Brad May 12 '13 at 07:24
  • @nacholibre: SCREAM: Error suppression ignored for ( ! ) Notice: Undefined index: cat in C:\wamp\www\site\del.php on line 42 Undefined index: topcat in C:\wamp\www\site\del.php on line 44 Undefined index: limit in C:\wamp\www\site\del.php on line 45 – user2320375 May 12 '13 at 07:24
  • 2
    @Brad - if we get a penny everytime we say about vulnerabilities of sql injection.. we would be millionaire by now.. – Dinesh May 12 '13 at 07:27
  • @Dinesh : but i can see the rows parsed from database based on cat variable. when i click delete button it shows error. – user2320375 May 12 '13 at 07:28
  • that doesn't matter.. – Dinesh May 12 '13 at 07:29
  • @brad and Dinesh I'm beginner in PHP, first let me get basic info, then with time sql injection can be rectified can tackled... – user2320375 May 12 '13 at 07:31
  • 2
    @user2320375, No no no no! Learn to write queries the right way first. You don't understand how bad of a problem this is. There are automated bots roaming the internet looking for bad code like yours. Not only that, but your code won't function properly as soon as you need to use something like an apostrophe in your string value. Fix it. Learn the right way first. It doesn't matter if you are a beginner. You're learning the wrong way. – Brad May 12 '13 at 07:33
  • @user2320375 Where is `delete` defined in your `form`? I don't see any `delete` in your `form`. – Yogesh Suthar May 12 '13 at 07:34
  • @YogeshSuthar: i'm using MooTools Js here :http://davidwalsh.name/animated-ajax-record-deletion-mootools – user2320375 May 12 '13 at 07:37
  • @Brad: Yes Sir.... thank you for suggestion sir will follow it day and night sir..... – user2320375 May 12 '13 at 07:37
  • @user2320375 If you are using `mootools` than you have to specify that in your question with your `mootools` used code. Whatever you have posted we can't help you much. Because its incomplete code. – Yogesh Suthar May 12 '13 at 07:40

2 Answers2

2

Here is a brief lesson

Lesson one:

A lesson about HTTP GET request:
Never Never Never Never... perform any update/delete using GET request its an invitations for abuser to screw up with your application for example: SQL injection POST request are safer than GET but that does not mean are invulnerable.. always sanitize!

htt://yourunsecureedwebsite/del.php?delete=10

Abusers send this request to web page:

htt://yourunsecureedwebsite/del.php?delete=10' or 1=1'

You are screwed. The entire table is deleted. So never never never do any modification to your database using GET request.

Lesson 2:
Always always always escape/sanitize the GET and POST request During olden times we used to rely on mysql_real_escape_string() to protect from the evil malicious request. But anything mysql_* had its own vulnerabilities so the PHP-keepers deprecated this functionality.

So Mysqli and PDO came to our rescue which has prepare() function who deals with this malicious code. No worries about the injection anymore. (I would personally add more sanitization along with this)

Now your code:

As I mentioned in the comments section, it does not look like the form not intended to delete anything it looks like just a search query form where you select num of fields and the category and when the from is submitted using POST request you display the data according to query. You are actually not sending any request about deleting the category.

Here are the steps to implement what you want: First when you do a post request always check whether the page was loaded with post request. In your case your first line should be:

if($_isset["submit"]){
    $cat = sanitize($_POST['cat']);
    $topcat = sanitize($_POST['topcat']);
    $limit = sanitize($_POST['limit']);
}

sanitize a custom function you will have to create to sanitize the POST data. (something to get you started).

Now databse part: Go through a small tutorial on how to implement MySQLi or PDO (nice tutorial and some knowledge about PDO)

If you also want to delete your category along with the form than you will have find a way to retrieve the id of that category and than delete it: this you can do in 2 ways: 1> you can send a hidden id field (not safe) 2> retrieve the id from database depending on what category you select.

Note: You can only do a GET request or POST request at a time. There are ways to append values to URL which can mimic the GET request but this is another big tutorial in your process of learning.

I would suggest going through tutorials on creating secure applications before producing any contents online cause there are people and cylons who love to mess around.

Dinesh

Community
  • 1
  • 1
Dinesh
  • 3,065
  • 1
  • 18
  • 19
-3

Use the if (isset()) structure for all posted content, and if you're having trouble, use an else to display a relevant notification for troubleshooting.

Presumably the $delete is being sent from a form checkbox so should hold an integer value and be POSTed rather than via GET. I'll leave that to you.

Ro Mc
  • 441
  • 1
  • 4
  • 9
  • -1. Don't post insecure code in an answer! People copy/paste this stuff all the time without any idea as to what they are doing. – Brad May 12 '13 at 07:28
  • `if (isset($_GET['delete']) AND isset($_POST['cat'])) {` will that ever be executed as true??? think before posting answers – Preetam May 12 '13 at 07:32
  • No worries, I was only demonstrating the use of sprintf in the example so that I didn't provide too much information at once. I'll fix it up. – Ro Mc May 12 '13 at 07:47