5
\$\begingroup\$

The following code list all files in a folder and allows the user to download one of them. It is working but I think it could be done better.

Is there a way to reduce the number of forms to only one and to improve the way I retrieve the name of the file to download?

My second concern is about the security of my getFile function.

function listDir($directoryName)
{
    $directory = opendir($directoryName);
    $files = array();
    while ($elem = readdir($directory))
    {
        if ($elem != '.' && $elem != '..' && $elem != '.htaccess')
        {
            $files[] = $elem;
        }
    }
    closedir();
    if (!empty($directory))
    {
        sort($files);
        echo "<ul>";
        foreach ($files as $file)
        {
            $filePath = $directoryName.'/'.$file;
            echo "<form method='post' action=".htmlentities($_SERVER['PHP_SELF']).">";
            echo "<li>($fileDate)";
            echo "<input type='hidden' name='$file' value='$filePath'>";
            echo " <button type='submit' name='getFile'>Télécharger</button> ";
            echo "$file</li>";
            echo "</form>";
        }
        echo "</ul>";
    }
}

function getFile($file)
{
    if (file_exists($file))
    {
        header('Content-Description: File Transfer');
        header('Content-Type: application/octet-stream');
        header('Content-Disposition: attachment; filename='.basename($file));
        header('Expires: 0');
        header('Cache-Control: must-revalidate');
        header('Pragma: public');
        header('Content-Length: ' . filesize($file));
        ob_clean();
        flush();
        readfile($file);
        exit;
    }
}

if (isset($_POST['getFile']))
{
    $keys = array_keys($_POST);
    $file = $_POST[$keys[0]];
    getFile($file);
}

listDir('data');
\$\endgroup\$
1
  • \$\begingroup\$ Your getFile function is incredibly insecure. The number one rule is to never trust the user. I could POST getFile=/var/etc/passwd or getFile=../my/secret/file or something. \$\endgroup\$
    – Schism
    Jul 8 '14 at 5:06
1
\$\begingroup\$

Let's post a dry review first, before jumping in to the conceptual stuff:

  • You are allowing your end-users download any file on the server. All I need to do is to change the path to /etc/passwd on any form and submit, and I get your server's password file! And if I don't have permissions to that, no biggy, I can still download all of your owned files on the server.
  • Invalid HTML markup - <form> isn't allowed directly inside of <ul>. Only <li> is.
  • Separate logic and presentation - You shouldn't mix the part where the user sees your form, and the part where he downloads the file.

A better approach?

A better approach would be a PHP file which is capable of serving files. That PHP file needs to make sure that the file being requested is found in a very specific list of allowed directories (i.e. I allow from images/ and from static/ but not from anywhere else.

Then, when you download a file, all you need to do is to link (With an <a>) to

http://example.com/path/to/file_server.php?filename=somefile.jpg

And have the PHP file_server handle the serving of the requested file.

Getting Fancy

You can even use .htaccess and mod_rewrite to rewrite URLs like this:

http://example.com/static/somefile.jpg

to

http://example.com/path/to/file_server.php?filename=somefile.jpg

Making the fact that PHP is the one sending the file completely transparent to the user!

\$\endgroup\$
1
  • \$\begingroup\$ Taking into account your advice, I replaced my getFile with this script (improved with $finfo = new finfo; $ctype = $finfo->file($file, FILEINFO_MIME_TYPE); for the MIME) \$\endgroup\$
    – lbeziaud
    Jul 8 '14 at 12:50

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.