Type:		Critical bugfix
Software:	shimmie2
Module:		image
Reference:	http://trac.shishnet.org/shimmie2/browser/branches/branch_2.2/ext/image/main.php
		http://trac.shishnet.org/shimmie2/browser/branches/branch_2.2/core/page.class.php
Verified:	Yes
Solved:		Yes, professional solution needs core changes
Official res.:	Unknown
Author:		Daniel Marschall
Date:		2008-10-18


The shimmie2 board 2.2 has a very, very awful bug that makes the board nearly unuseable.

If you reload the page, the thumb-images get interchanged. This happens with all browsers and all
servers. Also, sometimes a thumbnail will be loaded INSTEAD of the requested html-page. This is
a very, very big failure of the cache-control of the board.

The reason why contents are interchanged is following:

The board looks if the content has changed. If the content has not changed, then a 304 status
code will be sent to the user client by the script "ext/image/main.php". Actually NO content data
may be sended to the user client. But the core script "core/page.class.php" don't care which
status code was sent. So it DOES SEND the image data to the user client. The browsers are running MAD
because they get told "Content not modified, here is the new content". They will disrupt the
http-requests and the responses because the RFC-rule is violated.

A trivial bug fix is to just stop the script with an exit() if the 304 status code was sent.

But this should be solved by a more professional resolution.

$page->status_code should be implemented. And if this variable is set to 304, then
$page->display() should never send the "Content-type" header line or anything else. It should
just terminate. If this would be used, we wouldn't need the exit()'s at the cache-control-part.

The exit() at the 304 case is not professional because there might be other modules which
are counting image access or something else. These modules would be terminated because of exit().
In general case, exit() should be NEVER used in the object oriented image board shimmie2...

I have to append that the cache handler is totally crap and has to be replaced.
It doesn't even handle correct ETags.


Here is what I have changed:

~ ext/image/main.php

#	Comment out

	$page->set_data(file_get_contents($file));
			
	if(isset($_SERVER["HTTP_IF_MODIFIED_SINCE"])) {
		$if_modified_since = preg_replace('/;.*$/', '', $_SERVER["HTTP_IF_MODIFIED_SINCE"]);
	}
	else {
		$if_modified_since = "";
	}
	$gmdate_mod = gmdate('D, d M Y H:i:s', filemtime($file)) . ' GMT';

	// FIXME: should be $page->blah
	if($if_modified_since == $gmdate_mod) {
		header("HTTP/1.0 304 Not Modified");
	}
	else {
		header("Last-Modified: $gmdate_mod");
		header("Expires: Fri, 2 Sep 2101 12:42:42 GMT"); // War was beginning
	}

#	Add after this

	# http://nedmartin.org/site/caching

	// $file contains the file name of the page being displayed (the actual
	// content, not any templates you may be using). We take the last modified
	// date of this file.
	$mtime = filemtime($file);

	// send a unique 'strong' identifier. This is always the same for this 
	// particular file while the file itself remains the same.
	header('ETag: "'.md5($mtime.$file).'"');

	// $file contains the file name of the page being displayed (the actual
	// content, not any templates you may be using). We take the last modified
	// date of this file.
	$mtime = filemtime($file);

	// Create a HTTP conformant date, example 'Mon, 22 Dec 2003 14:16:16 GMT'
	$gmt_mtime = gmdate('D, d M Y H:i:s', $mtime).' GMT';

	// output last modified header using the last modified date of the file.
	header('Last-Modified: '.$gmt_mtime);

	// tell all caches that this resource is publically cacheable.
	header('Cache-Control: public');

	// this resource expires one month from now.
	header('Expires: '.gmdate('D, d M Y H:i:s', strtotime('+1 month')).' GMT');

	// check if the last modified date sent by the client is the the same as
	// the last modified date of the requested file. If so, return 304 header
	// and exit.
	if(isset($_SERVER['HTTP_IF_MODIFIED_SINCE']))
	{
		if ($_SERVER['HTTP_IF_MODIFIED_SINCE'] == $gmt_mtime)
		{
			header('HTTP/1.1 304 Not Modified');
			header('Status: 304 Not Modified'); // Added by DM
			
			exit(); // Note DM: Important, otherwise the pictures will be interchanged
		}
	}
	// check if the Etag sent by the client is the same as the Etag of the
	// requested file. If so, return 304 header and exit.
	if (isset($_SERVER['HTTP_IF_NONE_MATCH']))
	{
		if (str_replace('"', '', stripslashes($_SERVER['HTTP_IF_NONE_MATCH'])) == md5($mtime.$file))
		{
			header("HTTP/1.1 304 Not Modified");
			header('Status: 304 Not Modified'); // Added by DM

			// abort processing and exit
			exit(); // Note DM: Important, otherwise the pictures will be interchanged
		}
	}
		
	$page->set_data(file_get_contents($file));

#	Line 145


Notice: "+1 month" could also be staticly set to 100 years (but the dynamic timestamp is only
valid until 2038, not 2108+)


Further improvements:

As already mentioned by the developers in a comment, there should be a $page->status which
allows to set a status code, but I have to append that every call like this might be errorous:

header('HTTP/1.1 304 Not Modified');

If you want to be on the safe side, you should use a double call like this:

header('HTTP/1.1 304 Not Modified');
header('Status: 304 Not Modified');

This is more compatible with different PHP and/or Apache versions!