1
\$\begingroup\$

I'm using this method from my Downloads() class in a back-end admin system, where these can upload some files, which must be .rar or .zip I'm not to concern about filesize, yet. Eventually, these compressed files, will be holding only .mp3 files.

Well, here's the code. As you can see, I'm checking mimetype against an array of the ones allowed, which I got them from Stack Overflow, but I'm still not sure if I'm not missing any.

Then, I'm checking against the file extension in case mimetype is faked (not sure how accurate this check is).

Are those conditionals OK? I mean, I wouldn't want the file to be uploaded, but the query not to be done, or vice-versa.

//Handle file upload process
    public function uploadFile($titulo,$file) {
        $accepted_types = array(
                        'application/zip',
                        'application/x-zip-compressed',
                        'multipart/x-zip',
                        'application/x-compressed',
                        'application/x-rar-compressed',
                );
        if (isset($file['name'])) {
            $filename = str_replace(' ', '_',$file["name"]);
            $tmp_name = $file["tmp_name"];
            $type = $file["type"];
            $name = explode(".", $filename);
            foreach($accepted_types as $mime_type) {
                if($mime_type == $type) {
                        $okay = true;
                        break;
                } 
            }
            //Check mimeType
            $continue = (strtolower($name[1]) == 'zip') || (strtolower($name[1]) == 'rar') ? true : false;
            if(!$continue) {
                    return false;
            }
            $finalPath = DOWNLOAD_PATH.$filename;
            if (file_exists($finalPath)) {
                $newname=  Utility::file_newname(DWONLOAD_PATH,$filename);
                $finalPath= DWONLOAD_PATH.$newname;
            }

            if (move_uploaded_file($tmp_name, $finalPath)) {
                $fileSize = filesize($finalPath);
                $query = "INSERT INTO descargas (titulo,size,src,filename) VALUES
                    ('$titulo','$fileSize','$finalPath','$filename')";
                if ($this->mysqli->query($query)) {
                    return true;
                } else {//The file's been moved, but theere was an error in the query
                    unlink($finalPath);
                    return false;
                }
            }
        }
    }
\$\endgroup\$
  • \$\begingroup\$ Check the second half of my answer to this question it deals with various ways to check a file's mime-type \$\endgroup\$ – Elias Van Ootegem Dec 16 '13 at 16:23
  • \$\begingroup\$ Thank you very much, @EliasVanOotegem I think i'm gonna go with checking the n bytes of the file. Seems to be the most accurate. By the way, what do you think about the last conditional in the method? \$\endgroup\$ – Leonardo Chaia Dec 16 '13 at 17:21
  • \$\begingroup\$ There's a couple of issues with that bit of your code: you're using mysqli, but you're not using prepared statements. I'd play it safe, and use a prep. stmt. I'd also not use a die: you're using mysqli::query() or die, so the else will never get executed. Besides, or die shouldn't be used, except when debugging... \$\endgroup\$ – Elias Van Ootegem Dec 17 '13 at 7:40
  • \$\begingroup\$ @EliasVanOotegem Thank you very much for your answer, I'm using prep stmt(in other methods of same project), but just when the user sends that from an input, is that OK? by the way, I stopped using or die() since you told me not to in another question. But, i haven't removed them from the res of my code. Thank you very much again. \$\endgroup\$ – Leonardo Chaia Dec 17 '13 at 16:05
  • \$\begingroup\$ Well, I've taken the time to put together a more general review of the code you posted, with some tips and tricks I hadn't discussed in these comments \$\endgroup\$ – Elias Van Ootegem Dec 18 '13 at 7:32
5
\$\begingroup\$

Right, seeing as I could go on and review your code in comments, I'd thought I'd post my comments + extra review in one go instead (after all, that's what this site is for).

Starting from the top:

public function uploadFile($titulo,$file) {

A couple of niggles and suggestions

  • Please try to adhere to the PHP-FIG coding standards. They're not official, but there are no official standards. PHP-FIG is subscribed to by all the main players, so it's best you do, too
  • The $file argument is central to the entire method. Why isn't it the first argument? Suppose you want $titulo (I'm assuming it means title or something) optional, and default to a string?
  • What is $file meant to be? What do you expect it to be? If I am to use your class, I'd like to be able to know what I have to pass to your methods just by looking at their signature.

So, basically, I'd recommend changing your method signature to:

public function uploadFile(array $file, $titulo = 'optional default argument')
{//brace goes on new line
}

Even that wouldn't be to my liking, though. I should know I'm expected to pass a file, so why not write:

public function uploadFile(\SplFileInfo $file, $titulo = null)
{
}

In this case, you can get rid of that if (isset($file['name'])) bit, and focus on the stuff that matters.

Next:

$accepted_types = array(
                'application/zip',
                'application/x-zip-compressed',
                'multipart/x-zip',
                'application/x-compressed',
                'application/x-rar-compressed',
);

Now every time this method gets called, the array above will be constructed. After the function returns, its ref-count will (or should) be zero, and it'll get GC. If you want to call this method more than once, or if you want to re-use this class for other file manipulations, you should turn this into a property:

class Downloads
{
    private $accepted_mimes = array(
                'application/zip',
                'application/x-zip-compressed',
                'multipart/x-zip',
                'application/x-compressed',
                'application/x-rar-compressed',
    );
    public function uploadFile(\SplFileInfo $file, $titulo)
    {
        //code here
    }
}

Better yet: define an associative array (this can be a private static, for once) that groups the mime-types into "modes", which you can then use per constant:

class Downloads
{
    const MIME_MODE_FILE = 1;
    const MIME_MODE_TEXT = 2;
    const MIME_MODE_FOO = 4;//powers of 2, for easy bitwise operations

    private static $mime_modes = array(
        self::MIME_MODE_FILE => array(
                'application/zip',
                'application/x-zip-compressed',
                'multipart/x-zip',
                'application/x-compressed',
                'application/x-rar-compressed',
        ),
        self::MIME_MODE_TEXT => array(
            //mime-types for text content
        )
    );
    private $mime_mode = null;
    private $accepted = null;

    public function __construct($mode = self::MIME_MODE_FILE)
    {
        if (!isset(static::$mime_modes[$mode])) throw new InvalidArgumentException('Invalid mime-mode for '.__CLASS__);
        $this->mime_mode = $mode;
        $this->accepted = static::$mime_modes[$mode];
    }
}

And take it from there. The reason why bitwise operators can come in handy here is that, if I wanted to use your Download class to process text files, but then call a method that was intended for use on non-text files, like archive decompression, you could check the $this->mime_mode property.
However MS Office documents are, in effect zip files. So a mime-type to accept those would have to accept zip files, too. Thus, the mime-mode to set might have to be Downloads::MIME_MODE_FILE | Downloads::MIME_MODE_TEXT
Of course, you'll have to work on the constructor in that case, but that's something you can do yourself. It's not that hard.

As for the actual checking of the mime-type: I've previously linked this answer and discussed various ways to do so. You've decided to play it safe and go for the checking of the first n bytes. That's fine, but never trust the result of a single check, though: always check both the given mime-type, and the bytes, always go "Double Dutch".

Lastly: mysqli_* usage where prepared statements shine in absence.
Sure, you say this code will only be called deep in the bowels of your back-end system, and no user input will be used. Call me stupid, but an uploaded file is user input in my book. I haven't got the faintest idea as to what $titulo is, nor do I know what the filenames might look like. Personally, I'd still play it safe and use a prepared statement.
If you feel as if prepared statements will only cause overhead then don't, because prepared statements can be used more than once. You could lazy-load a statement:

class Downloads
{
    //all of the consts, methods and properties I've previously listed +:
    private $mysqli = null;
    private $stmts = array();
    //then add:
    private function getStatement($queryString)
    {
        if (!isset($stmts[$queryString]))
        {
            $this->stmts[$queryString] = $this->mysqli->prepare($queryString);
        }
        return $this->stmts[$queryString];
    }
}

The prepared statement won't exist until, from the uploadFile method you call:

$stmt = $this->getStatement(
    'INSERT INTO descargas (titulo,size,src,filename) VALUES
      (?,?,?,?)'
);

The first time this call is executed, you'll create a prepared statement, the following times, you'll just get the same prepared statement returned to you, ready to be used again. So the overhead isn't as big as you might expect it to be.
Security-wise, however, you'll be better of. And in a way, in terms of traffic between PHP and MySQL, you'll bode well, too: the query string from which the statement is prepared is sent once, for all subsequent execute calls, only the parameters are sent over to the server, via a different protocol, so if you execute this query 20 times or more, you'll possibly end up sending less data to the DB.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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