Upload image using php 7 best practices to make it secure

Question

I have a script that uploads an image, inserts it into database, and displays it on the home page.

However it's not secure, how can i make this script more secure and make sure it allows only jpeg/jpg files.

UploadController.php

public function upload_image($filename, $image_name)
{

    $this->uploadfile = $this->directory . basename($filename);

    try{
        $sql = "INSERT INTO images (img, image_name) VALUES (?, ?)";
        $stmt = $this->connect->prepare($sql)->execute([$filename, $image_name]);
        return $stmt; 
    }
    catch(PDOExeception $e)
    {
        echo $e->getMessage();
    }

}

Home.php

<?php
require_once 'paritals/header.php';
require_once 'paritals/nav.php';


use Eli\Db as DB;
use ElCont\UploadController as Image;


    if(isset($_FILES['profile_img'])){

        $dbh = new DB();
        $connect = $dbh->connect();
        $image = new Image($connect);

        $filename = $_FILES["profile_img"]["name"];
        $realname = "public/uploads/" . basename($filename);
        $directory = $_SERVER['DOCUMENT_ROOT'] . "/public/uploads/";

        $path = $directory . basename($filename);

        $image_name = $_POST['image_name'];


        if($image->upload_image($realname, $image_name)){

            move_uploaded_file($_FILES["profile_img"]["tmp_name"], $path);

            $owl = $image->get_image();


        }

}?>

<div class="container">
    <div class="row">
        <div class="col-md-6 offset-md-4 p-5 ">
            <h1>Upload Image</h1>
            <form action="" enctype="multipart/form-data" method="post">
                <div class="form-group">
                    <label for="exampleInputFile">File input</label>
                    <input type="file" name="profile_img" class="form-control-file" id="exampleInputFile" aria-describedby="fileHelp">
                    <input type="text" name="image_name" placeholder="Enter Name Of File" class="mt-3">
                    <small id="fileHelp" class="form-text text-muted">This is some placeholder block-level help text for the above input. It's a bit lighter and easily wraps to a new line.</small>
                    <button type="submit" class="btn btn-primary">Submit</button>
                </div>

            </form>
        </div>
    </div>

<div class="row p-5">
<?php

$dbh = new DB();
$connect = $dbh->connect();
$image = new Image($connect);

$photo =  $image->get_images();
foreach($photo as $pic)

{?>


            <div class="col-md-4">
              <h1><?php echo $pic['image_name'];?></h1>
                <img src="<?php echo $pic['img'];?>" width="300" height="400">

            </div>


<?php }?>
       </div>
    </div>
</div>
<?php require_once  'paritals/footer.php';?>

Your Common Sense · Accepted Answer · 2018-06-17 11:35:39Z

For some reason you aren't following suggestions have been given to you in the other post. So again:

One of insecurities is revealing the internal error message to a site user. Remove that try and catch stuff from the database interaction and configure your server to report errors appropriately like you've been told.

Same goes for adding directory to the file name. Please remember, you already added it in the step before:

$realname = "uploads/" . basename($filename);

therefore, the following code makes no sense:

$this->uploadfile = $this->directory . basename($filename);

Do you realize that?
Besides, a function responsible for storing a filename in the database should be dong only that - storing a filename. Assigning the filename or error reporting should be done elsewhere.

It means your upload image method should be exactly like this:

public function store($filename, $image_name)
{
    $sql = "INSERT INTO images (img, image_name) VALUES (?, ?)";
    $this->connect->prepare($sql)->execute([$filename, $image_name]);
}

making it to do the only stuff it is intended for.

Regarding security, the main point is to verify the file's extension. Your web-server will judge the file type by its extension so should you.

$allowed = ['jpg', 'jpeg', 'png', 'gif'];
$extension = pathinfo($_FILES['file']['name'],PATHINFO_EXTENSION);
if (in_array($extension, $allowed)) {
    // process your file
} else {
    // tell a user the extension is not allowed
}

regarding general purpose best practices. Looks like your namespaces are inconsistent. It seems you want something like that

use Eli\Db as DB;
use Eli\Controller\UploadController as Image;

and also you are supposed to implement PSR-4 autoload so you won't have to include class definitions manually

thank you common sense, i need to be paying better attention sorry. — BARNOWL
– BARNOWL, Commented Jun 17, 2018 at 17:03

Ivan Vartanyan · Accepted Answer · 2018-06-17 09:20:50Z

1

The best way to handle uploads is to use open source, well designed and tested libraries such as Upload. But if you want to do it yourself you can use methods described here

answered Jun 17, 2018 at 9:20

Ivan Vartanyan

786 bronze badges

Add a comment |

Stack Exchange Network

Upload image using php 7 best practices to make it secure

2 Answers 2

You must log in to answer this question.

Linked

Hot Network Questions

Upload image using php 7 best practices to make it secure

2 Answers 2

You must log in to answer this question.

Linked

Related

Hot Network Questions