Secure image upload class

Question

Last night I made this image uploading class. It was the first time I made a file uploader to be used on a real site, so I thought I would share it here and on Github to get some reviews, hoping if I can perfect it with your help, I will use the class in all my future projects.

<?php
/**
 * A small, secure & fast image uploader class written in all-static
 * class to give an extra boost in performance.
 * @author     Simon _eQ <https://github.com/simon-eQ>
 * @license    Public domain. Do anything you want with it.
 * Don't ever forget to use at your own risk.
 */


class BulletProof
{
    /**
     * User defined, allowed image extensions to upload. ex: 'jpg, png, gif'
     * @var
     */
    static $allowedMimeTypes;

    /**
     * Set a max. height and Width of image
     * @var
     */
    static $allowedFileDimensions;

    /**
     * Max file size to upload. Must be less than server/browser
     * MAX_FILE_UPLOAD directives
     * @var
     */
    static $allowedMaxFileSize;

    /**
     * Set the new directory / folder to upload new image into.
     * @var
     */
    static $directoryToUpload;

    /**
     * MIME type of the upload image/file
     * @var
     */
    static $fileMimeType;

    /**
     * A new name you have chosen to give the image
     * @var
     */
    static $newFileName;


    /**
     * Set of rules passed by user to choose what/where/how to upload.
     * @param array $allowedMimeTypes
     * @param array $allowedFileDimensions
     * @param $allowedMaxFileSize
     * @param $directoryToUpload
     */
    static function options(array $allowedMimeTypes,
                            array $allowedFileDimensions,
                                  $allowedMaxFileSize,
                                  $directoryToUpload)
    {
        /**
         * Globalize the score of the directives, so we can call them from another method.
         */
        self::$allowedMimeTypes = $allowedMimeTypes;
        self::$allowedFileDimensions = $allowedFileDimensions;
        self::$allowedMaxFileSize = $allowedMaxFileSize;
        self::$directoryToUpload = $directoryToUpload;
    }


    /**
     * Native and possible PHP errors provided by the $_FILES[] super-global.
     * @return array
     */
    static function commonFileUploadErrors()
    {
        /**
         * We can use the key identifier from $_FILES['error'] to match this array's key and
         * output the corresponding errors. Damn I'm good!
         */
        return array(
            UPLOAD_ERR_OK           => "...",
            UPLOAD_ERR_INI_SIZE     => "File is larger than the specified amount set by the server",
            UPLOAD_ERR_FORM_SIZE    => "Files is larger than the specified amount specified by browser",
            UPLOAD_ERR_PARTIAL      => "File could not be fully uploaded. Please try again later",
            UPLOAD_ERR_NO_FILE      => "File is not found",
            UPLOAD_ERR_NO_TMP_DIR   => "Can't write to disk, as per server configuration",
            UPLOAD_ERR_EXTENSION    => "A PHP extension has halted this file upload process"
        );
    }


    /**
     * There are many reasons for a file upload not work, other than from the information we
     * can get of the $_FILES array. So, this function tends to debug server environment for
     * a possible cause of an error (if any)
     * @param null $newDirectory optional directory, if not specified this class will use tmp_name
     * @return string
     */
    static function checkServerPermissions($newDirectory = null)
    {
        $uploadFileTo = $newDirectory ? $newDirectory : init_get("file_uploads");

        /**
         * if the directory (if) specified by user is indeed dir or not
         */
        if(!is_dir($uploadFileTo))
        {
            return "Please make sure this is a valid directory, or php 'file_uploads' is turned on";
        }

        /**
         * Check http header to see if server accepts images
         */
        if(stripos('image', $_SERVER['HTTP_ACCEPT']) !== false)
        {
            return "This evil server does not seem to accept images";
        }


        /**
         * Check if given directory has write permissions
         */
      //  if(!substr(sprintf('%o', fileperms($uploadFileTo)), -4) != 0777)
      //  {
      //      return "Sorry, you don't have her majesty's permission to upload files on this server";
      //  }

    }


    /**
     * Upload given files, after a series of validations
     * @param $fileToUpload
     * @param $newFileName
     * @return bool|string
     */
    static function upload($fileToUpload, $newFileName = null)
    {

        /**
         * check if file's MIME type is specified in the allowed MIME types
         */
        $fileMimeType = substr($fileToUpload['type'], 6);
        if(!in_array($fileMimeType, self::$allowedMimeTypes))
        {
            return "This file type is not allowed.";
        }

        self::$fileMimeType = $fileMimeType;

        /**
         * show if there is any error
         */
        if($fileToUpload['error'])
        {
            $errors = self::commonFileUploadErrors();
            return $errors[$fileToUpload['error']];
        }

        /**
         * Check if size of the file is greater than specified
         */
        if($fileToUpload['size'] > self::$allowedMaxFileSize)
        {
            return "File size must be less than ".(self::$allowedMaxFileSize / 100)." Kbytes";
        }

        /**
         * Checking image dimension is an enhancement as a feature but a must & wise check from
         * a security point of view.
         */
        list($width, $height, $type, $attr) = getimagesize($fileToUpload['tmp_name']);

        if($width > self::$allowedFileDimensions['max-width'] ||
           $height > self::$allowedFileDimensions['max-height'])
        {
            return "Image must be less than ". self::$allowedFileDimensions['max-width']."pixels wide
                    and ". self::$allowedFileDimensions['max-height']."pixels in height";
        }

        /**
         * No monkey business
         */
        if($height <= 1 || $width <= 1)
        {
            return "This is invalid Image type";
        }

        /**
         * If user has provided a new file name, assign it. Otherwise,
         * use a default uniqid id, to avoid name collision
         */
        if($newFileName)
        {
            self::$newFileName = $newFileName;
        }
        else
        {
            self::$newFileName = uniqid();
        }

        /**
         * Upload file.
         */
        $newUploadDir = self::$directoryToUpload;
        $upload = move_uploaded_file($fileToUpload['tmp_name'], $newUploadDir.'/'.self::$newFileName.'.'.self::$fileMimeType);

        if($upload)
        {
           return true;
        }
        else
        {
            /**
             * If file upload fails, the debug server enviroment as a last resort before giving 'Unknown error'
             */
            $systemErrorCheck = self::checkServerPermissions($newUploadDir);
            return $systemErrorCheck ? $systemErrorCheck : "Unknown error occured. Please try again later";
        }


    }

} 

In between the time I finished the it, and right now, I have began to have doubts about some things I did/should have done like:

• Is making the methods static really appropriate for this class? There is no need for unit testing, so in my opinion, I would say, there is nothing with using static class here.
  • Should I create custom exceptions to handle the errors instead of returning error messages (note: I am also trying to make the script faster)?
• Should I be extending the SplFileInfo class handle verification the file types better?
• Is it better to use finfo_open(FILEINFO_MIME_TYPE); to check file type, or is $_FILES['file']['type'] or SplFileInfo'sgetFileExtension?

This is the usage of the class:

BulletProof::set(array('png', 'jpeg', 'gif', 'jpg'),
                 array('max-width'=>150, 'max-height'=>150),
                 30000,
                 'pictures/'
                );



if($_FILES){
   $upload =  BulletProof::upload($_FILES['profile_pic'], 'simon');

   if($upload !== true)
   {
    echo "ERROR: ".$upload;
   }
}

I am still thinking if BulletProof::upload() should take in all the augments, if user wants to upload different files with different types/sizes/dimensions instead of creating a global option style.

I have a lot of questions/doubts, but I am in a need of any type of review.

Answer 1

Update
After a quick look at your updated github code, one thing that struck me as quite odd was line 94:

if(!substr(sprintf('%o', fileperms($uploadFileTo)), -4) != 0777)

In order for a file to be uploaded to some directory, I'd not expect that dir to be chmodded to xwr. If anything, I would consider it somewhat insecure to allow uploaded files to be moved to a rwx-all directory. I'd disallow execution for anyone except the actual owner, and set it to 0755, and even that'd be liberal, 0655 would work, too
I'm at work now, and have quite a lot to do, but, to coin a phrase: "stay tuned" for future updates.

Your main questions are:

Is making the methods static really appropriate for this class? There is no need for unit testing, so imho, I would say, there is nothing [wrong] with using statics here.

I would have to disagree with you on that one. Since neigh on everything in your class is static, what's the point of having a class? Especially since you're trying to improve performance. Classes were never meant to improve performance of code, they were introduced to improve performance of development: a good class is reusable, easy to maintain and to some extend it documents itself (type-hinting, short, concise and to the point methods & method-names).
In PHP, which is by design, stateless, statics are often just globals in OO-drag. That's, in my view, evidence of bad design. Not only is it hard to unit-test (you know that already), it greatly reduces the chances of your code being re-usable. Thus effectively reducing its OO interface to noise/syntactic sugar or even clutter.

Lastly, if you're trying to improve performance, then static methods should be right out, really. As I said, they're mostly global functions in drag anyway, with 1 crucial difference: a call to a static method requires PHP to lookup the zend_object* pointer, and then use the zend_object_value to get to the zend_object_handlers in which it can finally look for the method you're calling. Which creates considerable overhead if you're interested in micro-optimizations.

So global functions will be faster, but even a non-static will be faster as you can see from this benchmark script. The reasons are a bit too complicated for me to explain here, but the bottom line is that an instance of any class references the zend_object_handlers directly, whereas a static call doesn't have an instance to begin with: the call doesn't start from a variable, so there's no zval * to start from. What is used instead is the zend_class_entry which was created at compilation time. The engine uses sifts through all sorts of things (global, calling scope, context specific stuff...), just to get to the zend_class_entry *, just to get to the method... That's hardly efficient, now is it?

Should I create costume [custom] exceptions to handle the errors instead of returning error messages

It all depends. Personally, I'm in favour of custom exceptions, that are grouped together neatly in a distinct namespace. Your code deals with file-uploads, and thus with request data. All code that deals with request data, be it in its "raw" for or not, should use request-specific exceptions IMO. That makes the entire code so much more clean, and reusable.
In the end, it's probably a matter of taste/opinion, but I find that if there's an error throw simply beats return false or return self::SOME_CONSTANT hands down, because the latter relies on the user of that code to check the return value. Getting people to do that is a struggle, even in languages like C, where the return value is all you have to go on, in some cases.

Should I be extending the SplFileInfo class handle verification the file types better

While one of the first examples on the doc pages on php.net shows a class extending the SplFileInfo class, I'm not too keen on that. I've always said that it's best to not extend from code you don't own. It's much the same as:

class MyDB extends \PDO
{
}

This code is completely harmless. It just changes the name of PDO to MyDB, and slows you down (every call to a method is a call to MyDB::parend::<method> now).
But it's too tempting to write code like:

class BadDB extends \PDO
{
    public function __construct(
                      $dsn = 'mysql:dbname=default;host=127.0.0.1',
                      $user = 'root',
                      $pass = 'yourPAss',
                      array $opts = array(
                          self::ATTR_ERRMODE => self::ERRMODE_EXCEPTION
                      )
    )
    {
        return parent::__construct($dsn, $user, $pass, $opts);
    }

    public function insert($query, array $data)
    {
        $stmt = parent::prepare($query);
        $stmt->execute($data);
        return $this->lastInsertID();
    }
}

You may be wondering what's wrong with that, well, I've been quite verbose on the matter, here, you'll find just a few reasons why this isn't a good idea.
The examples given here, and on the linked post aren't even the worst that can happen, imagine if I were to set the default options to include:

self::ATTR_EMULATE_PREPARES => false

And some poor fellow would use MyDB, test his code locally, using a version of MySQL that supported prepared statements, and then deploy the code on a server that is still running MySQL 4 (yes, they still exists, and these kinds of absurdities happen).

Anyway, back to SplFileInfo: I wouldn't extend from it. Your class has a clear task: process uploaded files. SplFileInfo, too, has a clear task: its job is to represent, and offer a clean way of working with files. Those are 2 different jobs. A class should have one and only one reason to change (or job). Combine 2 or more jobs, and you're dealing with a module.

Is it better to use finfo_open(FILEINFO_MIME_TYPE); to check file type, or is $_FILES['file']['type'] or SplFileInfo'sgetFileExtension Which one is safer, or better..

The short answer is SplFileGetInfo::getFileExtension is the safest bet, but it's not perfect still. I'll have a quick look 'round, but I think I have some code on this machine that deals with that...
Nope, it's a Symfony2 project, so it's not of much help here. However you may want to take a look at these answers here, some of the tips and tricks still apply today (there's one about checking the first n bytes of a file, to be absolutely sure the mime-type matches the extension, for example).

Answer 2

• Using static methods is not appropriate if your only good reason for doing so is performance. I can confidently say that outside of a benchmark, neither you nor anyone else will ever notice the difference in performance.

• HTTP_ACCEPT tells you what the client accepts. IE: what types of files you can return. Your script is the one that decides whether you accept images.