HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

File extension detection and restriction on upload

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fileuploadrestrictionextensiondetectionand

Problem

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) +"File

Solution

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

Code Snippets

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

Context

StackExchange Code Review Q#140402, answer score: 2

Revisions (0)

No revisions yet.