Is there a better way how to force file download than using this simple PHP script?

Question

I've written this simple script to force download some files (jpg, mp3, usually those which are loaded in browser by default). I was wondering whether there's any way this could be improved upon, which means:

• making it more secure
• making it use less cpu (filesize(), fopen(), fpassthru() are the usual suspects here)
• making it simpler maybe

Here it goes:

<?php

if (!isset($_GET['file'])){
   die('no file requested');
}

else{

   if (substr($_GET['file'], 0, 1) == '.'){
      die('trying to leave this directory? :)');
   }

   $path = './'.$_GET['file'];
   if (file_exists($path) && is_readable($path)){
    $size = filesize($path);
    header('Content-Type: application/octet-stream');
    header('Content-Length: '.$size);
    header('Content-Disposition: attachment; filename='.$_GET['file']);
    header('Content-Transfer-Encoding: binary');
    $file = fopen($path, 'rb');
    fpassthru($file);
    exit;
   }

}

?>

Usage: /?file=sample.jpg

Answer 1

A small improvement I've spotted, take care to enclose the filename in double quotes to handle the case when file name contains spaces, like so:

header('Content-Disposition: attachment; filename="'.$_GET['file'].'"');

Answer 2

Always use === in your comparisons.

if (substr($_GET['file'], 0, 1) == '.'){
  die('trying to leave this directory? :)');
}

Should be:

if (substr($_GET['file'], 0, 1) === '.'){
  die('trying to leave this directory? :)');
}

Also look into other ways to sanitize $_GET['file'] properly at the moment I could pass in something like /../ and I could access other files.

From the docs you can save on a fopen() call by using readfile() instead see fpassthru()

A better way to use variables in a string is to use curly syntax so this line

header('Content-Disposition: attachment; filename='.$_GET['file']);

Would be better as

header("Content-Disposition: attachment; filename={$_GET['file']}");

I think that covers the three bullet points :)

Answer 3

You should never accept a user to send a path, only accept file names as input, only you in the server side will know where directory go to get that file, so use basename() for that:

$_GET['file'] = basename( $_GET['file'] ); //permit only filename

Answer 4

It's much better to avoid using PHP and allow your web server to manage this. If you're using Apache, the following lines in your .htaccess file will do the trick quite nicely.

AddType application/octet-stream .jpg 
AddType application/octet-stream .mp3

This is much less server intensive and more secure.

Answer 5

what about just puting a link to that file?

for example:

<a href="/file.mp3">download file</a>

as far as i know it depends of the user configs if it will download it or use it in the browser

Answer 6

I agree with most of these other answers, there are just a few things I thought I'd point out.

I would never directly use user input. Always sanitize and validate it. You've already validated, you just need to sanitize. An easy way to do this if your PHP version is >= 5.2 is to use filter_input().

$file = filter_input( INPUT_GET, 'file', FILTER_SANITIZE_STRING );

$path = "./$file";
if( file_exists( $path ) && is_readable( $path ) ) {
    //etc...
    header( "Content-Disposition: attachment; filename=$file" );
}

You should use is_file() instead of file_exists(). The difference is that the latter doesn't assume its parameter is an actual file and will also return TRUE for directories. However, neither is actually necessary in this context. is_readable() also checks if a file exists, so this is redundant and unnecessary. Just use is_readable().

if( is_readable( $path ) ) {

Depending on the size of these files, imploding the input into an array and iterating over that will allow you to download multiple files, though at this point you will want to offer links to said files instead of automatically generating a save file dialog.

$filestring = filter_input( INPUT_GET, 'file', FILTER_SANITIZE_STRING );
$files = implode( ',' $filestring );
foreach( $files AS $file ) {
    //this is where having a function, as Jack mentioned, would be handy
}

My final suggestion. Don't use an if/else statement if you can get away with just using the if. Your if statement returns early, therefore the else statement is implied. There is no need to explicitly use an else statement and force yourself to indent your code unnecessarily. This will help you to avoid the Arrow Anti-Pattern.

Answer 7

making it simpler maybe not sure making it more secure You can try

$maxTime = 10; // Max download interval per IP address
$path = __DIR__ . "/test"; // make it fixed
$allowedFile = array("jpg","png");

$memcache = new Memcache();
$memcache->connect('localhost', 11211);

try {

    if (! isset($_GET['file'])) {
        throw new Exception('No file requested');
    }

    $file = basename(filter_var($_GET['file'], FILTER_SANITIZE_STRING, FILTER_FLAG_STRIP_HIGH | FILTER_FLAG_STRIP_LOW));
    $location = $path . DIRECTORY_SEPARATOR . $file;
    $key = sha1($_SERVER['REMOTE_ADDR'] . $location);

    $runtime = $memcache->get($key);

    if ((time() - $runtime) < $maxTime) {
        throw new Exception("Die! Die! Die! Mr Flood");
    }

    if (! in_array(pathinfo($location, PATHINFO_EXTENSION), $allowedFile)) {
        throw new Exception("File Type Not allowed");
    }

    if (! file_exists($location)) {
        throw new Exception("File does not exsits");
    }

    if (! is_readable($location)) {
        throw new Exception("Acess denied to file");
    }

    if (! is_file($location)) {
        throw new Exception("Not a valid File");
    }

    $memcache->set($key, time());
    $size = filesize($location);
    header('Content-Type: application/octet-stream');
    header('Content-Length:' . $size);
    header(sprintf('Content-Disposition: attachment; filename="%s"', $file));
    header('Content-Transfer-Encoding: binary');
    readfile($location);
} catch ( Exception $e ) {
    die("<h2>{$e->getMessage()}</h2>");
}