php delete from sql and directory form

Question

I have resolved this issue of the warning by going back to if(!file_exists($file_to_delete)(as I already know in that folder is an is only images I just needed it so user could not get to other directories) I have also made a check on the id that its numeric & exists in db and sanitised the query's I believe could you please have a look though the new code below and see if ok or if and further problems exist

Many thanks

Heres my code

<?php
// Include Databse
include ("common.php");

// VARIBLES	
$delete = $_POST['delete'];
$id = $_POST['id'];
$filename = $_POST['filename'];
$ext = end(explode('.',$filename));

// Check if form has been submitted
if (isset ($delete))
{
// Check filename is not empty
if(empty($filename)) {
    $status = "Please enter a FILENAME" ;
	$error = true;
	$filecheck = false;
}

else {
	$filecheck = true;
}

if ($filecheck)
{
//Check user stays in correct directory & check image ext
if(!preg_match('/^\/?[\w\s-_]+\.(jpe?g|gif|png|bmp)$/',strtolower($filename))) 
{
	$error = true;
	$status = "Please check FILENAME";
} 

else {
    $file_to_delete = 'images/' . $filename;
}

// Check file_to_delete is set
if ($file_to_delete)
{
// Checks the file exists
if(!file_exists($file_to_delete))
{
$status = "File not found please check FILENAME";
$error = true;
$idcheck = false;
}

else 
{
$idcheck = true;	
}
}

// Check $idcheck is set
if($idcheck)
{
// Check ID is not empty
if(empty($id)) {
    $status = "Please enter a ID " ;
	$error = true;
	$filecheck = false;
}

//Check if ID is not numeric
else if(!is_numeric($id))
{
	$error = true;
	$status = "Please check ID";
}

else
{
// Check ID exists in database
$query = "SELECT id FROM `test` WHERE `id` = :id" ;
$stmt = $db->prepare($query);
$stmt->bindParam(":id", $id);
$stmt->execute();

//if ID exists.
if($stmt->rowCount() > 0)
{
	$error = false;
}

else {
	$error = true;
	$status = "Please check ID";
}

}
}
}

if (!$error)
{
// Run Query & Delete File Information From Database
$query = "DELETE FROM `test` WHERE `id` = :id" ;
try { 
        $stmt = $db->prepare($query);
		$stmt->bindParam(':id', $id);
        $stmt->execute(); 
    } 
    catch(PDOException $ex) 
    {  
        die("Failed to delete image: Please report issue to admin"); 
    }
	
// Delete File From Directory
unlink($file_to_delete);


$status = "File Deleted";

}
}
?>

<?php 
$query = "SELECT id,photo FROM test";
      
try 
 { 
// Run Query To Show The Current Data In Database
        $stmt = $db->prepare($query); 
        $stmt->execute(); 
    } 
    catch(PDOException $ex) 
    {   
        die("Failed to run query: Please report issue to admin"); 
    } 
         
$rows = $stmt->fetchAll(); 
?>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Delete Image</title>
<style type="text/css">
.table {
	text-align: center;
}
.table {
	font-weight: bold;
}
</style>
</head>

<body>
<form action="delete.php" method="post" enctype="multipart/form-data" class="table">
Please enter the Filename and ID of the image you wish to delete
  <table width="178" align="center">
    <tr class="table">
      <td width="144" class="table">Filename</td>
      <td width="30" class="table">ID </td>
    </tr>
    <tr>
      <td><input name="filename" type="text" value="<?php echo $filename; ?>" />      </td>
      <td><input name="id" type="text" id="id" value="<?php echo $id; ?>" size="3" maxlength="4" />      </td>
    </tr>
  </table>
  <p><?php echo $status; ?><br />
    <input type="submit" value="Delete Selected Image" name="delete" />
  </p>
  <p>IMAGE DETAILS </p>
  <table width="400" align="center" class="table">
    <tr>
      <th width="61">ID</th>
      <th width="185">Filename</th>
      <th width="138">Image</th>
    </tr>
  </table>
  <table width="400" align="center" class="table"> 
    <?php foreach($rows as $row): ?> 
        <tr> 
            <td width="61"><?php echo $row['id']; ?></td>

            <td width="185"><?php echo $row['photo']; ?></td>
            <td width="138" height="138">
<img src="images/<?php echo $row['photo'] ; ?>" width="138" height="138" /></td> 
        </tr>
    <?php endforeach; ?> 
</table> </p>
  <p><br />
  <br />
  </p>
</form>
</body>
</html>

stj · Accepted Answer · 2015-06-13 23:20:49Z

1

There are various options, and if you're really concerned about security, you shouldn't let end users specify a filename at all. Instead, you may want to hand out randomly generated md5 strings or something alike. You can store a mapping between such md5 string and a filename in the database, which you seem to be using.

If you really have to have users specify the actual filenames, you could make sure that they only contain characters you consider safe. The fewer characters you allow, the better. For example, if you can restrict filenames to a-z, A-Z, 0-9, _ and - plus a file extension, you could validate as follows:

if (! preg_match("/^[a-zA-Z0-9_\-]+\.[a-zA-Z0-9]+$/", $filename)) {
  throw new Exception("invalid filename pattern");
}

This way users cannot specify a filename that crosses directory bounds.

To restrict filenames to certain extension, you could use something like this:

if (! preg_match("/\.(jpe?g|png|gif)$/i", $filename)) {
  throw new Exception("invalid file extension");
}

You can additionally check the directory name of the assembled filename and raise an error if the directory name is not what you expect:

if (dirname("images/" . $filename) !== "images") {
  throw new Exception("cannot leave directory");
}

When concerned about security and deleting the wrong files, you should also be worried about SQL injection. Your script is vulnerable, because you're inserting a user-specified value into an SQL query unchecked:

$query = "DELETE FROM `test` WHERE `id` = $id" ;

What will happen if the user posts an id value of 1 OR true? Right, all your images will be deleted from the database!

answered Jun 13, 2015 at 23:20

stj

9,11721 silver badges34 bronze badges

Sign up to request clarification or add additional context in comments.

8 Comments

RicPurcell Over a year ago

Ok thanks will try this tomorrow and look into securing the sql query further any ideas how could do it better?

stj Over a year ago

Always check and sanitize user input, especially before inserting it into SQL queries. Use bind parameters in SQL statements instead of string concatenation, or use a framework.

RicPurcell Over a year ago

how would I go about doing this as I have tried $query = "DELETE FROM test` WHERE id = :id" ; try { $stmt->bindParam(':id', $id); $stmt = $db->prepare($query); $stmt->execute();` But get error Fatal error: Call to a member function bindParam() on a non-object in ../delete.php on line 100

stj Over a year ago

The code you show uses $stmt before it is initialized. You have to reverse the two lines $stmt->bindParam(':id', $id); and $stmt- = $db->prepare($query);.

RicPurcell Over a year ago

thank you worked perfect understand not so bind parameters need to go after prepare statement could you also tell me how to get rid of Warning: getimagesize(): Filename cannot be empty in /../delete.php on line 67 aswell as my $status = "Please check filename" I don't want the user to be able to see that warning only the $status = "Please check filename"

|

Just Lucky Really · Accepted Answer · 2015-06-13 23:23:01Z

0

To check that the file is an image:

$imagetest=false;
$ext = end(explode('.',$filename));
switch(strtolower($ext)) {
    case 'jpeg':
        $imagetest=true;
        break;
    case 'jpg':
        $imagetest=true;
        break;
    case 'gif':
        $imagetest=true;
        break;
    case 'png':
        $imagetest=true;
        break;
    case 'bmp':
        $imagetest=true;
        break;
    default:
        $imagetest=false;
}

And to ensure the user the user can't go outside of the image dir, validate the input, so it's only a filename, then add the path:

if(!preg_match('/^[\w,\s-]+\.[A-Za-z]+$',$filename)) {
    $imagetest=false;
} else {
    $filepath='/to/your/path/image/' . $filename;
}

And then you could replace your file_exists() with:

if(!getimagesize($filepath)) {
    $imagetest==false;
}

This will both check that the file exists, and that it is an image.

edited Jun 13, 2015 at 23:23

answered Jun 13, 2015 at 23:15

Just Lucky Really

1,4312 gold badges19 silver badges38 bronze badges

1 Comment

Marc B Over a year ago

filename extensions aren't reliable. if you're using getimagesize(), then you can directly check what kind of image it is there...

RicPurcell · Accepted Answer · 2015-06-15 21:25:58Z

I have done it like so... works perfect just how I wanted thank you all for your help much appriated now to create the image slideshow itself

<?php
// Include Databse
include "common.php";

// validation errors
$error = array();

// Check if form has been submitted
if (isset ($_POST['delete']))
{
// get the filename & id. See php.net/basename for more info
    $filename = basename($_POST['filename']);
	$id =($_POST['id']);
    
// get file extension, see php.net/pathinfo for more info
    $ext = pathinfo($_POST['filename'], PATHINFO_EXTENSION);

// allowed file extensions
    $allowedExtensions = array('jpeg','jpg','gif','png','bmp');

// Check filename is not empty
    if(empty($filename))
    {
        $error[] = "Please enter a Filename";
    }

// Check valid file extension used
   else if(!in_array($ext, $allowedExtensions))
    {
        $error[] = "Please check Filename";
    }
	
// Check ID is not empty    
if(empty($_POST['id']))
    {
        $error[] = "Please enter a ID";
    }
	
	else if(is_numeric($id))
{
// Check ID exists in database
$query = "SELECT id FROM `test` WHERE `id` = :id" ;
$stmt = $db->prepare($query);
$stmt->bindParam(":id", $id);
$stmt->execute();

if(!$stmt->rowCount() == 1)
{
	$error[] = "Please check ID";
}
}

else {
	$error[] = "ID is not numeric";
}

// delete file from database if there are no errors
    if (empty($error))
    {
// path to the image
    $file_to_delete = 'images/' . $filename;
	
// Checks the file exists and that is a valid image
        if(file_exists($file_to_delete) && getimagesize($file_to_delete))
        {
// Delete File From Directory
            unlink($file_to_delete);
        }
        else
        {
            $error[] = "File not found please check Filename";
        }

if (empty($error))
{
// Run Query To Delete File Information From Database
        try
        { 

            $query = "DELETE FROM `test` WHERE `id` = :id";
            $stmt = $db->prepare($query); 
            $stmt->execute(array('id' => intval($_POST['id']))); 
        } 
        catch(PDOException $ex) 
        {  
            die("Failed to run query: Please report issue to admin"); 
        }


        $status = "File Deleted";
    }
}
}

// Run Query To Show The Current Data In Database
try 
{ 
    $query = "SELECT id,photo FROM test ORDER BY id";
    $stmt = $db->prepare($query); 
    $stmt->execute(); 
} 
catch(PDOException $ex) 
{   
    die("Failed to run query: Please report issue to admin"); 
} 
         
$rows = $stmt->fetchAll(); 
?>

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<title>Delete Image</title>
<style type="text/css">
.table {
    text-align: center;
}
.table {
    font-weight: bold;
}
</style>
</head>

<body>
<form action="delete2.php" method="post" enctype="multipart/form-data" class="table">
Please enter the Filename and ID of the image you wish to delete
  <table width="178" align="center">
    <tr class="table">
      <td width="144" class="table">Filename</td>
      <td width="30" class="table">ID </td>
    </tr>
    <tr>
      <td><input name="filename" type="text" value="<?php echo $filename; ?>" /></td>
      <td><input name="id" type="text" id="id" value="<?php echo $id; ?>" size="3" maxlength="4" /></td>
    </tr>
  </table>
  <p>
    <?php 
// Show validation errros here
    if(!empty($error)):
    echo  implode('<br />', $error);
	echo $status;
	endif; 
    ?>
    <br />
    <input type="submit" value="Delete Selected Image" name="delete" />
  </p>
  <p>IMAGE DETAILS </p>
  <table width="400" align="center" class="table">
    <tr>
      <th width="61">ID</th>
      <th width="185">Filename</th>
      <th width="138">Image</th>
    </tr>
  </table>
  <table width="400" align="center" class="table"> 
    <?php foreach($rows as $row): ?> 
        <tr> 
            <td width="61"><?php echo $row['id']; ?></td>

            <td width="185"><?php echo $row['photo']; ?></td>
            <td width="138" height="138">
                <img src="images/<?php echo $row['photo'] ; ?>" width="138" height="138" />
            </td> 
        </tr>
    <?php endforeach; ?> 
</table> </p>
  <p><br />
  <br />
  </p>
</form>
</body>
</html>

Dennis Lindsey · Accepted Answer · 2015-06-16 03:56:24Z

0

@Stretch had a good answer, but it can be done in less code if you don't mind a longer regular expression test.

if(!preg_match('/^\/?[\w\s-_]+\.(jpe?g|gif|png|bmp)$/', strtolower($filename)) || !getimagesize($filename)) {
    $imagetest = false;
} else {
    $imagetest = true;
}

Also, be sure to use prepared statements correctly if you're going to be using PDO for your database queries. You should never directly insert user-supplied data into your query strings, always build your query and then add their data as a bound value.

$sql = "DELETE FROM `test` WHERE `id` = :id";
$q = $conn->prepare($sql);
$q->execute(array('id', (int)$id));

edited Jun 16, 2015 at 3:56

answered Jun 13, 2015 at 23:42

Dennis Lindsey

113 bronze badges

5 Comments

RicPurcell Over a year ago

how would I do this I have tried $query = "DELETE FROM test` WHERE id = :id" ; try { $stmt->bindParam(':id', $id); $stmt = $db->prepare($query); $stmt->execute(); ` But get error "Fatal error: Call to a member function bindParam() on a non-object in /home/u271859679/public_html/gallery/display/New/delete.php on line 100 "

Dennis Lindsey Over a year ago

You're calling the bindParam() method on $stmt before you instantiate the $stmt object.

RicPurcell Over a year ago

thank you worked perfect understand now so bind parameters need to go after prepare statement could you also tell me how to get rid of Warning: getimagesize(): Filename cannot be empty in /../delete.php on line 67 aswell as my $status = "Please check filename" I don't want the user to be able to see that warning only the $status = "Please check filename"

Just Lucky Really Over a year ago

@DennisLindsey Ah yeah, your right about that regex. In this guys case, he would be better to use regex ... I just simplified the code I use, which actually does something different depending on what image type it is Lol ...

Dennis Lindsey Over a year ago

@user3396325 You can either A) turn off PHP debug globally (which you should do in a production environment anyway), or B) add an '@' in front of getimagesize() @getimagesize($filename)

Collectives™ on Stack Overflow

php delete from sql and directory form

4 Answers 4

8 Comments

1 Comment

Comments

5 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

8 Comments

1 Comment

Comments

5 Comments

Your Answer

Sign up or log in

Post as a guest

Related