php file upload always uploading files even if errors

Question

I am trying to upload either pdf or jpg, jpeg files to a folder and the code is as follows:

//Get the uploaded file information
if(!$_FILES['medreport']['error'])
{
    $medreport = basename($_FILES['medreport']['name']);
    $medreport_extn = substr($medreport, strrpos($medreport, '.') + 1);//get the file extension of the file
    $medreport_size = $_FILES["medreport"]["size"]/1024;//size in KBs
    $tmp_path = $_FILES["medreport"]["tmp_name"];
    $report_folder = "../reports/";

    //Settings
    $max_allowed_file_size = 200; // size in KB
    $allowed_extensions = array("jpg", "jpeg", "pdf");

    //Validations
}

if($medreport_size > $max_allowed_file_size )
{
    $error[] = "Size of the report file should be less than $max_allowed_file_size KB";
}

//Validate the file extension
$allowed_ext = false;
for($i=0; $i<sizeof($allowed_extensions); $i++)
{
    if(strcasecmp($allowed_extensions[$i],$medreport_extn) == 0)
    {
        $allowed_ext = true;
    }
}

if(!$allowed_ext)
{
    $error[] = "The uploaded report file is not a supported file type. "."Only pdf, jpg and jpeg report file types are supported. ";
}

//replace filename with unixtime
$unixtime =time();
$medreport = $unixtime.mt_rand(0,9).'.'.$medreport_extn;

$report_path = $report_folder . $medreport;
if(is_uploaded_file($tmp_path))
{
    if(!copy($tmp_path,$report_path))
    {
        $error[] = 'Error while copying the uploaded report file';
    }
}

while trying to upload files with correct extension and size i am able to upload it.

But if i try to upload an over sized or incorrect format file, it displays my error message, but the file always get uploaded to the folder.

Why is it so ?? Please, What is wrong with my code??

Is the way, i am doing it is secure enough ?? the folder is owned by www-data and permission is 755. I have a .htaccess file too in the file upload folder to prevent executables as follows:

SetHandler none
SetHandler default-handler
Options -ExecCGI
php_flag engine off

The file always uploading is confusing me.

Presumably the dir is owned by www-data www-data (owner and group) which equates to the first 2 numbers on the permissions mask (75 atm). You're highlight unlikely for a guest user to ever need any permissions on anything on a web server so 750 should do. The security risk actually lies in having a directory that can be both read and written to by the web server - but you're mitigating that with the .htaccess file. — CD001
– CD001, Commented Oct 24, 2017 at 12:43
Since PHP runs on the server it can't know whether the file is too big, or a disallowed file type, until the file is on the server - so yes, it'll be uploaded irrespective unless it hits the limit set by upload_max_filesize or post_max_size for instance - in which case the upload will be aborted. — CD001
– CD001, Commented Oct 24, 2017 at 12:46
Depends on what you mean by others - every file served over http will be sent by Apache - which is the dir (and therefore file) owner. Everyone who logs in with SFTP should be in the www-data group - so will have read permissions. There is basically no instance where you should need to provide permissions for a guest user (i.e. logged into the machine as guest) on a web server. — CD001
– CD001, Commented Oct 24, 2017 at 12:49
For example mydomain.com/reports/15088490982.pdf will always give Forbidden error. You don't have permission to access /reports/15088490982.pdf on this server. — Pamela
– Pamela, Commented Oct 24, 2017 at 12:53
For a file it should work fine with 640 (files rarely need to be executable); Apache owns and serves the file. When you connect to a web server you're talking to the web server, it's the web server that's running as the *nix user and serving the files. Almost every directory on our server is 750, almost every file 640 - but that does require the file ownership to be correct in the first place. — CD001
– CD001, Commented Oct 24, 2017 at 13:02

jeroen · Accepted Answer · 2017-10-24 12:35:18Z

3

You are not using the errors you just found to check if you need to continue.

This:

if(is_uploaded_file($tmp_path))

Should be something like:

if(count($error) === 0 && is_uploaded_file($tmp_path))

And you should initialize your $error array at the start as an empty array if you are not doing that already.

answered Oct 24, 2017 at 12:35

jeroen

91.8k22 gold badges118 silver badges132 bronze badges

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

1 Comment

Pamela Over a year ago

Yep...You are absolutely correct. I had indded initialized the error array but i missed the count($error) === 0 part. Thank you for assisting me. Is the way i am doing it secure ?? what needs to be done to make it more secure ??

Collectives™ on Stack Overflow

php file upload always uploading files even if errors

1 Answer 1

1 Comment

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

1 Comment

Your Answer

Sign up or log in

Post as a guest

Related