I was looking into this one and I was taking the opportunity to cleanup a little the controller.
I stumbled upon this:
https://github.com/elkarte/Elkarte/blob/v1.1.0-RC.1/sources/controllers/Attachment.controller.php#L443
// Make sure the mime type warrants an inline display.
if (isset($this->_req->query->image) && !empty($mime_type) && strpos($mime_type, 'image/') !== 0)
unset($this->_req->query->image);
// Does this have a mime type?
elseif (!empty($mime_type) && (isset($this->_req->query->image) || !in_array($file_ext, array('jpg', 'gif', 'jpeg', 'x-ms-bmp', 'png', 'psd', 'tiff', 'iff'))))
header('Content-Type: ' . strtr($mime_type, array('image/bmp' => 'image/x-ms-bmp')));
else
{
header('Content-Type: ' . (isBrowser('ie') || isBrowser('opera') ? 'application/octetstream' : 'application/octet-stream'));
if (isset($this->_req->query->image))
unset($this->_req->query->image);
}
1st condition looks good: if the request says "image", but the mime does not, believe the mime. (Apart that we do not send any header, that looks weird to me.)
The 2nd condition, instead I'm not sure what it wants to do.
If I read it correctly it says: if we have a mime and either request says image or the file extension is not that of an image, assume it is an image.
O_o
I agree we do not trust the request. Fine.
I wouldn't trust the file extension too, but... not consider the file an image if it has an image extension looks strange.
I went back to the commit from 2007 that first introduced this structure and couldn't get any hint because it was basically a rewrite of the previous check.
I was thinking to change it to something like:
elseif (!empty($mime_type) && in_array(str_replace('image/', '', $mime_type), array('jpeg', 'gif', 'png', 'bmp')))
so: we have mime and mime is one of the embeddable (jpeg, png, gif, bmp).