1

Am fairly new to PHP and am making a basic CRUD style management system. I Have an update page and it displays data from a News table, and populates a form with it. The current picture ?(reference) is pulled through and displayed on the form. However if a user wants to change the picture they can press a 'delete' button and then I have written some PHP to display a upload button, set the values in the database for the image to null and hide the delete button, allowing the user to upload a new picture.

The Delete button only removes the reference (path) to the picture from the database, it doesn't delete the actual picture.

This is the HTML control to show the image and delete button. It also shows how the delete button works:

 <td align="right">Image 1:</td>
 <td align="left"><img src="uploads/newsimages/<?php echo $row["Image"]; ?>" width="230" border="0">&nbsp;<a href="UpdateNews.php?change=imagex&cid=<?php echo $row["NewsID"]; ?>">delete</a></td>

As you can see, when clicked it sets change=imagex and cid= the current news id.

There is then an if statement I have written, but it doesn't seem to only get activated when the delete button is clicked. Because I always get an error that 'cid' is undefined. It is as follows:

<?php
if (isset($_GET['change'] = "image1") {
    $query = "UPDATE  Table_Name SET Image = '' WHERE NewsID =".$_GET['cid']." ";
}

?>

I am pretty sure my lack of PHP knowledge is letting me down and I am trying to go about this the wrong way, because however I alter the if statement it always gives me an error. First it was cid is undefined so I changed to id but i already use that for something else, another query/function. I hope that all amde sense, can anyone tell me where Im going wrong?

Bohdi
  • 1,295
  • 4
  • 28
  • 62
  • Do you just want to delete the file on the server? – David Sep 06 '12 at 18:58
  • Have you checked the HTML you're outputting to make sure it's all correct? – andrewsi Sep 06 '12 at 18:58
  • 4
    **warning** your code is vulnerable to sql injection attacks. – Daniel A. White Sep 06 '12 at 18:58
  • 2
    Psst, `=` is assignment, `==` is general equality. Also, [read up on prepared statements](http://en.wikipedia.org/wiki/Prepared_statement) to protect yourself from the mentioned SQL injection vulnerability. – Charles Sep 06 '12 at 19:00
  • Yea am aware its not a very secure query, I can write it more secure, it was just for basic testing and functionalit. Thanks for the heads up though :) – Bohdi Sep 06 '12 at 19:00

3 Answers3

4

You are missing a parenthesis + you have to specify individually:

if (isset($_GET['change'] = "image1") {

Change to:

if (isset($_GET['change']) && $_GET['change'] == "image1") {
user1477388
  • 20,790
  • 32
  • 144
  • 264
  • Also, '==' - which you got, but is an important part of the error. – FrankieTheKneeMan Sep 06 '12 at 19:00
  • Take heed of Danial A. White's comment. Never use input data, whether by `$_GET` or `$_POST`, directly inside an SQL statement. Always filter the data so you know exactly the type of information that is going to be requested. Try using `mysql_real_escape_string($_GET['cid'])` and also make sure it's an integer with `is_int($_GET['cid'])`. – user1477388 Sep 06 '12 at 19:05
  • Its a MSSQL Database, not a MySQL one. So I don't think mysql-real_escape will work, and as far as I know, there isn't a MSSQL equivalent? I do secure most of my queries with string formatting, but not when I am just testing functionality. Lazy, I know. :p Thanks though :) – Bohdi Sep 06 '12 at 19:40
  • You're welcome. Try reading this just for reference http://stackoverflow.com/questions/574805/how-to-escape-strings-in-mssql-using-php – user1477388 Sep 06 '12 at 19:47
2

Some more things to consider:

1) Don't use unsanitized values directly from $_GET in a mysql query

WHERE NewsID =".$_GET['cid']."

It is very easy to exploit this with some funky sql injection (see http://xkcd.com/327/ ).

If you are using numeric values for cid, you should cast your $_GET value to integer to prevent sql injection:

$cid = (int)$_GET['cid];
$query = '(...)WHERE NewsID = '.$cid.' limit 1';

Or even better:

$cid = (int)(array_key_exists('cid', $_GET) ? $_GET['cid'] : 0);
if ($cid) {
  $query = (...)
}

If you need this kind of sanitizing in different places, you should think about writing a helper function for it to keep your code readable.

2) Don't use GET requests to change data on your server

Imagine a google bot browsing your site and following all those links that you use to delete images. Other scenarios involve users with prefetch plugins for their browsers (e.g. Fasterfox). Also, GET requests may be cached by proxies and browsers, so that the request won't hit the server if you click the link.

The HTTP specification comes with numerous request methods, the most important ones are:

  • GET to fetch content from the server
  • PUT to store new information on the server
  • POST to update existing information on the server

To update your news record (by removing the image) the appropriate method would be POST. To send a POST request, you can use the <form method="POST"> tag.

Pierre
  • 874
  • 6
  • 8
0

try this

<?php
if (isset($_GET['change']) && $_GET['change'] == "image1") {
    $query = "UPDATE  Table_Name SET Image = '' WHERE NewsID =".$_GET['cid']." ";
}

?>
Abid Hussain
  • 7,724
  • 3
  • 35
  • 53