possible dlattach bug (and fix) in 1.0.x

[linear]

Apologies if this is covered elsewhere--I had zero luck searching for relevant terms.

My users pointed out that all attachements, even images that the browser can neatly handle, are causing a download dialog when the link is clicked. I checked it out, and sure enough, the header

Code Select Expand

Content-Disposition: attachment; filename="example.png" is coming from the server in every case.

I found the relevant code in Sources/Display.php, in the Download() function. From the 1.0.7 distribution files, lines 805-820 are where the response headers get built for an attachment and/or avater download:

Code Select Expand

// Send the attachment headers.
header('Pragma: ');
header('Cache-Control: max-age=' . (525600 * 60) . ', private');
if (!$context['browser']['is_gecko'])
header('Content-Transfer-Encoding: binary');
header('Expires: ' . gmdate('D, d M Y H:i:s', time() + 525600 * 60) . ' GMT');
header('Last-Modified: ' . gmdate('D, d M Y H:i:s', filemtime($filename)) . ' GMT');
header('Accept-Ranges: bytes');
header('Set-Cookie:');
header('Connection: close');

if (!isset($_REQUEST['image']))
{
header('Content-Disposition: attachment; filename="' . $real_filename . '"');
header('Content-Type: application/octet-stream');
}


The problem is with line 816 which tests $_REQUEST['image']. This only gets set true for avatar downloads in line 759. Ideally, we'd set it true for attachments that are images as well.

Lines 824-826 following amount to a test for "imageness," conditionally setting the mime-type header as needed. Why not let's set the $_REQUEST['image'] as well, then move this up above where we test for it?

So the patch is to add the line that sets $_REQUEST['image'], which necessitates adding braces, and moving that block up above the conditional setting of the Content-Disposition header. I also go ahead and set the content-disposition header with the value inline, in order to see the real filename instead of index.php.

My revised code follows:

Code Select Expand

if (filesize($filename) != 0)
{
$size = @getimagesize($filename);
if (!empty($size) && $size[2] > 0 && $size[2] < 4)
{
header('Content-Type: image/' . ($size[2] != 1 ? ($size[2] != 2 ? 'png' : 'jpeg') : 'gif'));
  header('Content-Disposition: inline; filename="' . $real_filename . '"');
$_REQUEST['image'] = true;
}
}

if (!isset($_REQUEST['image']))
{
header('Content-Disposition: attachment; filename="' . $real_filename . '"');
header('Content-Type: application/octet-stream');
}



So I acknowledge I'm taking a leap of faith here--the code tests $_REQUEST['image'], so I'm pretty much assuming that sending the content-disposition: attachment header is only something you want to do for non-image attachments. But it's only getting set in the block that handles avatars. Image attachments weren't caught.

Thanks for the great software.

Edit: made code compliant with SMF formatting guidelines.

[linear]

On further testing, the content-disposition: inline header isn't seeming to have much of any effect on the browsers I have to test on. But it's also not hurting anything as far as I see. The RFCs "suggest" how to handle it, but browsers/user agnets are not required to comply with the suggestions.