I've just had a discussion with my teammate about the location of user uploaded images in an image gallery. I would like a broader insight on the methods we suggest.

My teammate wrote a controller + action that calls file_get_contents on an image file placed in a folder that's not available for public browsing (i.e., outside public_html on the server), and echoes it via a header. This is secure, but since we use Zend Framework, it's also crawling slow - each call to the image controller costs us approx 500ms of lag due to the bootstrap's queries being executed. It's annoying since the picture gallery view displays over 20 images at the same time.

In short, the relevant code would be:

class ImageController extends Zend_Controller_Action {
    public function showAction () {
        $filename = addslashes($this->_getParam('filename'));
        if(!is_file($filename)) {
            $filename = APPLICATION_PATH.'/../public/img/nopicture.jpg';
        }
        $this->_helper->viewRenderer->setNoRender(true);
        $this->view->layout()->disableLayout();
        $img = file_get_contents($filename);
        header('Content-Type: image/jpeg');
        $modified = new Zend_Date(filemtime($filename));
        $this->getResponse()
             ->setHeader('Last-Modified',$modified->toString(Zend_Date::RFC_1123))
             ->setHeader('Content-Type', 'image/jpeg')
             ->setHeader('Expires', '', true)
             ->setHeader('Cache-Control', 'public', true)
             ->setHeader('Cache-Control', 'max-age=3800')
             ->setHeader('Pragma', '', true);
        echo $img;
    }
}

Then, in a view, we just call:

<img src="<?php echo $this->url(array('controller' => 'image', 'action' => 'show', 'filename' => PATH_TO_HIDDEN_LOCATION.'/filename.jpg')); ?>" />

I have a different approach: I prefer to keep the original images in a hidden location, but as soon as they are requested, copy them to a public location and provide a link to it (with an extra mechanism, run by cron, to wipe the public images directory every now and then in order not to waste space, and a robots.txt telling Google not to index the directory). The solution places files (a few at every given moment) in a publicly accessible directory (provided one knows the filename), but also requires only a view helper, thus not launching the bootstrap:

class Zend_View_Helper_ShowImage extends Zend_View_Helper_Abstract {
    public function showImage ($filename) {
        if (!file_exists(PUBLIC_PATH."/img/{$filename}")) {
            if (!copy(PATH_TO_HIDDEN_FILES."/{$filename}",PUBLIC_PATH."/img/{$filename}"))
                $url = PUBLIC_PATH.'/img/nopicture.jpg';
            else
                $url = PUBLIC_PATH."/img/{$filename}";
        } else {
            $url = PUBLIC_PATH."/img/{$filename}"
        }
        return "{$url}";
    }
}

With the aid of this helper, the call is very simple in the view:

<img src="<?php echo $this->showImage('filename.jpg'); ?>" />

Question: Does my approach pose a security threat, as my coleague states? What are the potential risks of this? And, most importantly, do the security threats, if any, outweigh the 10 seconds gain on page load?

In case it matters: we're working on a community portal with around 15K registered users, with the galleries being a very frequently used feature.

*The code I pasted is an edited, simplified version of what each of us has come up with - just to show the mechanics of both approaches.

I have a different approach: I prefer to keep the original images in a hidden location, but as soon as they are requested, copy them to a public location and provide a link to it

+1 for creativity.

Does my approach pose a security threat, as my coleague states? What are the potential risks of this? And, most importantly, do the security threats, if any, outweigh the 10 seconds gain on page load?

Sort of. Yes, if you have images only some people are allowed to see, and you're putting them into a publicly accessible directory, there is a change other people can see that image, which appears to be undesirable. I also don't think (might be wrong) that it will gain 10 seconds on a page load, as you'll have to copy the images, which is a rather intensive operation, more than using file_get_contents or readfile( ).

This is secure, but since we use Zend Framework, it's also crawling slow - each call to the image controller costs us approx 500ms of lag due to the bootstrap's queries being executed.

If I may suggest; nuke Zend Framework for this specific case. I'm using Zend Framework for a rather large website as well, so I know the bootstrap can take longer than you want. If you circumvent Zend Framework, opting for vanilla PHP, this would improve the performance significantly.

Also, use readfile( ), not file_get_contents( ). There's a big difference in that file_get_contents will load the whole file in memory before outputting, where readfile does this more efficiently.

When a user accesses /user/validate without the correct post parameters, my Zend application throws a zend exception. (I get the standard "An Error Occurred" message, framed within my layout). This is intentional.

I am now trying to test that behaviour with PHPUnit. Here's my test:

/**
 * @expectedException Zend_Exception
 */
public function testEmptyUserValidationParametersCauseException() {
    $this->dispatch('/user/validate');
}

When I run the test I get a message saying it failed, "Expected exception Zend_Exception". Any ideas?

I have other tests in the file which are working just fine...

Thanks!

The Zend_Controller_Plugin_ErrorHandler plugin handles exceptions for you and the default Error_Controller forces a 500 redirect which may mean the exception you are testing for no longer exists. Try the following unit test and see if it passes:

public function testUnknownUserRedirectsToErrorPage()
{
    $this->dispatch('/user/validate');
    $this->assertController('error');
    $this->assertAction('error');
    $this->assertResponseCode('500');
}

If this works then it shows that by the time you are rendering the error view the exception will no longer exist as it is contained in the code before the redirect.