Php uploads incorrect files also

Question

Whats my problem with this code:

As you can see, I allow only image files from the $fajl_types array. But if I select a txt or some other file, it will also be uploaded. I get the error, that incorrect file type, but it will be uploaded anyway.

What am II doing wrong? Should I put a if(count($error) == 0 ) before the move_upload_file function or what?

$error = array();
$fajl_types = array(
                    'png'  => 'image/png',
                    'jpe'  => 'image/jpeg',
                    'jpeg' => 'image/jpeg',
                    'jpg'  => 'image/jpeg',
                    'gif'  => 'image/gif',
                    'bmp'  => 'image/bmp'
                );

if(isset($_POST['send_kapcsolat'])) {
    if(empty($_POST['hiba_nev'])) {
        $error[] = "Name required";
    }
    if(empty($_POST['hiba_email'])){
        $error[] = "Email required.";
    }

    if(isset($_FILES['hiba_file']) && $_FILES["hiba_file"]['size'] != 0 )
    {
        if($_FILES["hiba_file"]["size"] > 5242880 ) { 
            $error[] = "File size is max 5 mb."; 
        }
        $filename = $_FILES["hiba_file"]['name'];
        $ext = pathinfo($filename, PATHINFO_EXTENSION);
        if(!array_key_exists($ext, $fajl_types)) { 
            $error[] = "Incorrect file type"; 
        } 
        $path = "hiba/" . date( "Y-m-d-H:i:s" ) . '-' . rand(1, 9999) . '-' . $_FILES["hiba_file"]['name'];
        if(move_uploaded_file($_FILES["hiba_file"]['tmp_name'], $path ))
        {
            $hiba_file = basename($path);
        }
    }
    else
    {
        $hiba_file = "";
    }

    if(count($error) == 0 )
    {
        $hiba_nev = mysqli_real_escape_string($kapcs, $_POST['hiba_nev']);
        $hiba_email = mysqli_real_escape_string($kapcs, $_POST['hiba_email']);
        $hiba_uzenet = mysqli_real_escape_string($kapcs, $_POST['hiba_uzenet']);
        $hiba_status = (int)0;
        $hiba_date = date("Y-m-d-H:i:s");

        $sql = 
        "
            INSERT INTO hiba
            (
                hiba_nev,
                hiba_email,
                hiba_uzenet,
                hiba_file,
                hiba_status,
                hiba_date
            )
            VALUES
            (
                '".$hiba_nev."',
                '".$hiba_email."',
                '".$hiba_uzenet."',
                '".$hiba_file."',
                '".$hiba_status."',
                '".$hiba_date."'
            )
        ";
        print_r($sql);
    }
}

You are not exiting your code when you find that the type is wrong, you just set an error message and then go right ahead and move the file anyway! — RiggsFolly, Feb 18 '18 at 12:06
Your script is wide open to [SQL Injection Attack](http://stackoverflow.com/questions/60174/how-can-i-prevent-sql-injection-in-php) Even [if you are escaping inputs, its not safe!](http://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string) Use [prepared parameterized statements](http://php.net/manual/en/mysqli.quickstart.prepared-statements.php) — RiggsFolly, Feb 18 '18 at 12:07
_Should I put a if(count($error) == 0 ) before the move_upload_file function_ **That sounds like a plan!** So you answered your own question, make me wonder why you didn't just try that and save yourself the time involved in asking a question — RiggsFolly, Feb 18 '18 at 12:10
_Small Note:_ `$sql` is a string and therefore an `echo $sql;` would be better than a `print_r($sql)`;` as `$sql` is not an array. — RiggsFolly, Feb 18 '18 at 12:13

lyyka · Answer 1 · 2018-02-18T12:25:37.913

0

Below

if(!array_key_exists($ext, $fajl_types)) { $error[] = "Incorrect file type"; }

Check if count of errors is 0 and then execute the rest of the code until the end of the main If branch that checks if files are set.

The error is that if statement

if(move_uploaded_file($_FILES["hiba_file"]['tmp_name'], $path ))

Uploads your files and is executed even if the errors occur.

edited Feb 18 '18 at 12:25

answered Feb 18 '18 at 12:13

lyyka

450
3
11

And isnt that a problem, that now i have two count($error)==0 in the code? – KissTom87 Feb 18 '18 at 12:20
No, because you must set some value to variable $hiba_file before executing sql. The if statement does not require much resources so you will be fine I think. – lyyka Feb 18 '18 at 12:24

score 0 · Answer 2 · answered Feb 18 '18 at 12:18

You should put this code in:

if(move_uploaded_file($_FILES["hiba_file"]['tmp_name'], $path ))
{
$hiba_file = basename($path);
}

this if:

if(count($error) == 0 )

Because right now you upload your file even if error does exist,but you should do it only if error count is 0.

Php uploads incorrect files also

2 Answers2