3
\$\begingroup\$

I am trying to build a multiple file uploader with jQuery and Ajax. I would like to know how this code could be improved. I feel the client side validation of file types in my code are presently not good enough.

app.js

$(document).ready(function() {
    $('#upload').click(function () {
        $('input[type=file]').click();
        return false;
    });

    $("#uploadfile").change(function ()
    {
        //submit the form here
        var options = {
            url: 'processupload.php',
            beforeSubmit: beforeSubmit,
            success: afterSuccess,
            uploadProgress: OnProgress,
            resetForm: true
        };
        $('#fileupload').ajaxSubmit(options);
    });

    function beforeSubmit()
    {
        if (window.File && window.FileReader && window.FileList && window.Blob) {

            var fsize = $('#uploadfile')[0].files[0].size; //get file size
            var ftype = $('#uploadfile')[0].files[0].type; // get file type

            //allowed file types
            switch(ftype)
            {
                case 'image/png':
                case 'image/gif':
                case 'image/jpeg':
                case 'image/pjpeg':
                case 'text/plain':
                case 'text/html':
                case 'application/x-zip-compressed':
                case 'application/x-rar-compressed':
                case 'application/octet-stream':
                case 'application/pdf':
                case 'application/msword':
                case 'application/vnd.ms-excel':
                case 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet':
                case 'video/mp4':
                    break;
                default:
                    notify("Unsupported file type!");
                    return false
            }

            //Allowed file size is less than 27 MB
            if(fsize>29000000)
            {
                notify("Your total upload Size is: "+bytesToSize(fsize) +"<br/>File(s) is too big, it should be less than 27 MB.");
                return false
            }
        }
        else
        {
            //Output error to older unsupported browsers that doesn't support HTML5 File API
            notify("Please upgrade your browser, because your current browser lacks some new features we need!");
            return false;
        }
    }

    //progress bar function
    function OnProgress(event, position, total, percentComplete)
    {
        //Progress bar
        $('#overlay').show();
        $('#progressbar').width(percentComplete + '%'); //update progressbar percent complete
        $('#statustxt').html(percentComplete + '%'); //update status text
    }

    //function after sucessful file upload (when server response)
    function afterSuccess()
    {
        $('#statustxt').html('Files upload complete!');
        $('#overlay').delay(1500).fadeOut(); //hide progress bar

    }

    function bytesToSize(bytes) {
        var sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB'];
        if (bytes == 0) return '0 Bytes';
        var i = parseInt(Math.floor(Math.log(bytes) / Math.log(1024)));
        return Math.round(bytes / Math.pow(1024, i), 2) + ' ' + sizes[i];
    }

    function notify(msg) {
        $.alert({
            theme: 'black',
            icon: 'fa fa-exclamation-triangle',
            title: 'Oops!',
            content: msg
        });
    }
});

processupload.php

<?php

if(isset($_FILES["uploadfile"]) && $_FILES["uploadfile"]["error"]== UPLOAD_ERR_OK)
{
    ############ Edit settings ##############
    $UploadDirectory    = './storage/';
    #########################################

    /*
    Note : You will run into errors or blank page if "memory_limit" or "upload_max_filesize" is set to low in "php.ini". 
    Open "php.ini" file, and search for "memory_limit" or "upload_max_filesize" limit 
    and set them adequately, also check "post_max_size".
    */

    //check if this is an ajax request
    if (!isset($_SERVER['HTTP_X_REQUESTED_WITH'])){
        die();
    }


    //Is file size is less than allowed size.
    if ($_FILES["uploadfile"]["size"] > 29000000) {
        die("File size is too big!");
    }

    //allowed file type Server side check
    $allowedTypes = array('gif', 'png', 'jpeg', 'jpg', 'pdf', 'xls', 'xlsx', 'doc', 'docx', 'ppt', 'pptx', 'mp3', 'mp4', 'rar', 'zip', 'txt');

    $File_Name          = strtolower($_FILES['uploadfile']['name']);
    $File_Ext           = pathinfo($File_Name, PATHINFO_EXTENSION);
    if(!in_array($File_Ext, $allowedTypes)) {
        die('Unsupported File format!');
    }
    $Random_Number      = rand(0, 9999999999); //Random number to be added to name.
    $NewFileName        = $File_Name.$Random_Number.$File_Ext; //new file name

    if(move_uploaded_file($_FILES['uploadfile']['tmp_name'], $UploadDirectory.$NewFileName ))
       {
        die('Success! File Uploaded.');
    }else{
        die('error uploading File!');
    }

}
else
{
    die('Something wrong with upload! Is "upload_max_filesize" set correctly?');
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I feel the client side validation of file types in my code are presently not good enough.

Regarding security, this obviously doesn't matter, but regarding usability, it may be improved upon.

You are currently checking very different things server- and client-side. Server-side, you are checking the extension - which is the right thing to do, PHP type checks aren't very reliable - but client-side, you are checking the type.

This should be handled uniformly, to avoid the case that your client-side scripts allows an upload, but your server-side script forbids it (a valid .html file for example), or the case where your server-side script would allow an upload, but your client-side script forbids it (an audio/mp4 file for example).

You may want to consider performing extension- as well as filetype-checks on both sides.

You may also consider extracting your validation code, and share the same code (eg the types/extensions) across js and PHP to avoid discrepancies (some frameworks will do this for you).

Misc

  • Your braces style is not uniform, and I would suggest you follow PSR-2 (for methods/classes braces go on the next line, otherwise they go on the same line).
  • Your spacing is also not uniform.
  • variable names should start with a lower-case character.
  • either use camelCase or snake_case, but don't mix them for no reason.
  • return early to avoid nesting (eg the first if may be negated, so that the whole codeblock is not nested). This will also increase readability. You will first have your various checks, and then the actual uploading.

Returning early / guard clauses may look like this:

if(!isset($_FILES["uploadfile"]) 
    || $_FILES["uploadfile"]["error"] !== UPLOAD_ERR_OK) {
    die('Something wrong with upload! Is "upload_max_filesize" set correctly?');
}

if (!isset($_SERVER['HTTP_X_REQUESTED_WITH'])){
    die();
}

if ($_FILES["uploadfile"]["size"] > 29000000) {
    die("File size is too big!");
}

if (!isAllowedFile()) {
    die('Unsupported File format!');
}

$UploadDirectory    = './storage/';
// do the upload
\$\endgroup\$
2
  • \$\begingroup\$ can you please explain me the last point with an example? \$\endgroup\$
    – Ayan
    Sep 3, 2016 at 21:38
  • \$\begingroup\$ @Ayan sure, I added an example. Doing this really helps readability, as it's a lot easier to see when what things fail and what will happen then. I also added a new function in the example (isAllowedFile), which increases readability, and makes it possible to add that check as guard clause as well. \$\endgroup\$
    – tim
    Sep 3, 2016 at 21:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.