63601 - Recommend filename when downloading attachment

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, enhancement, P2)

Description

[timeless]

We should recommend a filename. showattachment.cgi is a bad recommendation.

Comment 1

[timeless]

Attachment: This patch probably won't work

Comment 2

[Peter Bowen]

Attachment: Use info from SQL query for filename, and include orig. filename

Comment 3

[timeless]

Attachment: Concept-Patch that won't do anything

Comment 4

[dror2]

attachments should show their file name (instead of just date/time or "id=xxx").

Comment 5

[Jacob Steenhagen]

Attachment: PATCH to suggest a filename in the form of "BUG#-FILENAME.EXT"

Comment 6

[Jacob Steenhagen]

Example for the above patch...  When I created the file I named it 63601.dif.
If you were to try and download it (and if it weren't text/plain) it would
suggest the name of 63601-63601.dif (BUG#-FILENAME).

Comment 7

[Stephan Niemz]

*** Bug 20383 has been marked as a duplicate of this bug. ***

Comment 8

[Stephan Niemz]

Please also look at bug 20383 which I marked duplicate, because it has
a small patch attached, too.

Comment 9

[Stephen Lee]

An alternative technique that works on at least some web servers is to make the 
link of the form:
"/showattachment.cgi/originalfilename.txt?attach_id=12345"

The following patch does this, and also adds a GetAttachLink to centralise the 
process in a similar way to GetBugLink

Comment 10

[Stephen Lee]

Attachment: Patch to tweak the form of attachment links.

Comment 11

[Jacob Steenhagen]

*** Bug 82527 has been marked as a duplicate of this bug. ***

Comment 12

[Jacob Steenhagen]

Bug 82527 also came with a patch (attachment 35950 [details] [diff] [review]).

Comment 13

[Jacob Steenhagen]

*** Bug 85706 has been marked as a duplicate of this bug. ***

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

->New product Bugzilla

Comment 15

[Dave Miller [:justdave]]

*** Bug 102140 has been marked as a duplicate of this bug. ***

Comment 16

[Matthew Tuck [:CodeMachine]]

Bug 102140 has a patch for this.

Comment 17

[Dave Miller [:justdave]]

And this bug has several.  They all seem to take a different approach to it. 
Daniel, can you reupload your patch to this bug?

Comment 18

[Daniel Raichle]

Attachment: Use orginal filename (without complete path). Tested with Netscape 4.71, IE 5.0 and IE 5.5.

Comment 19

[Gervase Markham [:gerv]]

Comment on attachment 51499 [details] [diff] [review]
Use orginal filename (without complete path). Tested with Netscape 4.71, IE 5.0 and IE 5.5.

Looks sensible to me. I didn't know we stored the original filename in the DB.

r=gerv.

Gerv

Comment 20

[David Lawrence [:dkl]]

This does not seem to work with wget. It still saves the filename improperly.
Example if I do:

wget http://bugzilla.redhat.com/bugzilla/showattachment.cgi?attach_id=33499

it properly downloads the file and places it in the current directory but the
filename itself is:

showattachment.cgi?attach_id=33499

Comment 21

[Gervase Markham [:gerv]]

Comment on attachment 51499 [details] [diff] [review]
Use orginal filename (without complete path). Tested with Netscape 4.71, IE 5.0 and IE 5.5.

>! $filename =~ s/^.*[\/\\]//;

This truncates a path down to a name, right? What about MacOS, which uses a ":"?

>! print qq{Content-type: $mimetype; name="$filename"\nContent-disposition: file; filename=$filename\n\n$thedata};

Does the filename need to be in quotes in Content-disposition?

Gerv

Comment 22

[Gervase Markham [:gerv]]

Daniel: could you respond to the review comments? Thanks :-)

Gerv

Comment 23

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 24

[David D. Kilzer (ddk)]

I'm looking at attachment 51499 [details] [diff] [review], "Use orginal filename (without
complete path). Tested with Netscape 4.71, IE 5.0 and IE 5.5." for
this comment.

For reference, RFC 2183 covers the Content-Disposition MIME header.

http://www.ietf.org/rfc/rfc2183.txt

Note that "file" is not a standard Content-Disposition value (the
only ones defined in the RFC are "inline" and "attachment").  However,
this non-standard value may force various versions of MSIE to actually
respect the desire of the programmer and force a save-file dialog box
for file being sent.

http://www.google.com/search?q=site%3Asupport.microsoft.com+content-disposition

Also the filename parameter should NOT be enclosed in quotes.  One of
the Microsoft knowledgebase articles mentions this.  While it is not
explicitly stated in the RFC, the BNF notation has quoted values for
the dates, but not for the filename parameter.  (The patch is correct.
I was looking at the "name" parameter for the Content-Type header.)

Finally, the patch has commented-out code and does not use the $filesize
parameter that it calculates itself.  It needs to add a "size=$filesize"
parameter to the Content-Disposition line.

Comment 25

[Matthew Tuck [:CodeMachine]]

I think the original filename would be a bad idea.  The filenames of things I
upload are typically useless because I expected them to never be used.  They
could also be the same between different attachments on the same or different
bugs.

I think much more useful would be attachment number or probably better bug
number-attachment number, then perhaps followed by the original filename.

Comment 26

[timeless]

I agree. Please don't use original filename.  Make a field and let people set
it. And provide a default.

Comment 27

[Stephen Lee]

Other than as a candidate for a downloaded filename, is there any need to have 
the original filename in the DB?

If the current filename field was editable on the Edit Attachment screen, "bad" 
filenames could be fixed.

Then add "Upload as filename: (leave blank for same as local filename) [      ]"
onto the Create Attachment form, allowing someone to call it Bug123-Patch2.txt 
or similar.

Comment 28

[Nahor]

I think the original filename (minus the path) should be kept because some
software may request a specific filename (config file for instance).
If there is a conflict on the filename, then the save dialog box should display
the overwrite confirmation. It's then up to the downloader to decide what he
wants to do (add a number, add the bug number, replace the current file,...)

Comment 29

[Brian 'netdragon' Bober]

Whether you want to use the orignal file name on attachments or not can be a
preference. 

If you turn the pref on, filenames would appear as:

Bug#-Attachment#-origfilename.origextension

Otherwise:

Bug#-Attachment#.origextension

Which would leave only one issue on whether someone might want to quickly toggle
this setting. Perhaps, There could be a box at the bottom of the attachment list
to temporarily toggle this setting.

Comment 30

[Stephen Rasku]

I think it should default to the uploaded filename unless overridden by the
uploader.

I have run into this problem today.  I uploaded a program called program.gz to
bugzilla but the person downloading it thought it had failed because the file
size was too small and the filename was showattachment.cgi.  If the name was
program.gz, he would have understood what he had to do.

Comment 31

[Jouni Heikniemi]

The fact that most original filenames may not be perfect is not interesting -
what is relevant is the fact that when the name _is_ important, it usually is
correct.

One typical use of our installation of Bugzilla is to create requests for
drawing graphics (for web sites). When those graphics are ready, they're
attached. Now, since the filenames for the image files are usually relevant
because they are linked to from html files, it's quite a pain trying to download
the files and give them correct names according to some info in the attachment
comments. It would be much easier to use the original file name directly.

I can't see why there should be something like bug number prefixes in the
filename: if the file isn't unique, then it isn't; browsers usually ask the save
location and file name anyway. Saving attachments is fairly rare; I don't think
name conflicts are worth extra hassle.

Comment 32

[Bradley Baetz (:bbaetz)]

*** Bug 161404 has been marked as a duplicate of this bug. ***

Comment 33

[Dave Miller [:justdave]]

Attachment: same as previous, updated for attachment.cgi, add filename editing

This patch takes the previous one and updates it for attachment.cgi (previous
one was against showattachment.cgi), and also adds the capability to edit the
filename to the attachment editing page.

This is live at http://landfill.bugzilla.org/bz63101/show_bug.cgi?id=365
(yeah, I know I screwed up the bug number when I created the landfill install,
but oh well)

Comment 34

[Bradley Baetz (:bbaetz)]

Comment on attachment 94297 [details] [diff] [review]
same as previous, updated for attachment.cgi, add filename editing

Waht is content-disposition: file? Where is it defined?

You need to check taht this doesn't break IE - see bug 27248 comment 17 +
friends.

Comment 35

[Dave Miller [:justdave]]

bbaetz: see comment #24 on this bug.

However, see also 
http://support.microsoft.com/default.aspx?scid=KB;EN-US;q182315&

MSIE less than 5.0 is broken with regards to this.  Apparently using "file" as a
disposition works in IE 4.x, but the real ones don't.

Almost everything on the planet that uses IE requires IE 5 these days so it's
probably safe to just leave it broken in IE4 and do it per the RFC (which means
changing the disposition to "attachment")

Comment 36

[Dave Miller [:justdave]]

Contrary to comment 24 from ddk, I don't see anything in the RFC about a "size"
parameter to the Content-Disposition: header.  Should that be moved out to
Content-Length: ?  (or just Length: I forget which is it...)

Comment 37

[Dave Miller [:justdave]]

Comment on attachment 94297 [details] [diff] [review]
same as previous, updated for attachment.cgi, add filename editing

needs-work, per previous comments.  I'll wait for a little more discussion on
the bug before I update the patch again.

Comment 38

[Dave Miller [:justdave]]

Attachment: Uses "attachment" per rfc, adds Content-Length header

OK, did my research in the RFC, and there is no "size" parameter to the
Content-Disposition header defined.  However, there is a Content-Length header,
which the RFC says is REQUIRED any time we actually know how much data we're
going to send (which we do in this case).

Those changes are made in this patch as well as changing the disposition to
"attachment" instead of "file".  This is correct per the RFC, but doesn't work
in IE4.  As noted in previous comments, Microsoft admits this is a bug in IE4
and claims to have fixed it in IE5.  The end result is that people using IE4
will have the same behaviour they always got from Bugzilla before we added this
header.  People using a browser that follows the standards will be politely
asked to confirm the real filename.

Comment 39

[Dave Miller [:justdave]]

Attachment: fix typoes

oops.  had a blank line in the wrong place, and forgot contributors

Comment 40

[Dave Miller [:justdave]]

I tried this out on Mozilla OS X CFM 2002071608 and MSIE 5.1.1 OSX, it Does The
Right Thing on both. :)

Comment 41

[Bradley Baetz (:bbaetz)]

Wasn't the problem with MSIE4 (bug 27248) that trying to _view_ a page which had
this content-disposition header in it would force a save-as dialog to appear? I
don't care if a particular browser won't use our suggested filename, but if it
means that you can't view the attachment....

Comment 42

[Dave Miller [:justdave]]

OK, so the answer is, we need to browser-sniff and if it's MSIE < 5 we don't
include the headers....

Comment 43

[Dave Miller [:justdave]]

Attachment: adds browser-sniffing

This patch adds browser detection, and doesn't display the Content-Disposition
header if the user's browser identifies as an MSIE older than 5.0

Comment 44

[Bradley Baetz (:bbaetz)]

Comment on attachment 94808 [details] [diff] [review]
adds browser-sniffing

r=bbaetz x2, as long as someone has tested on windows ie

Comment 45

[Dave Miller [:justdave]]

I can't, I don't have access to Windows.  Jouni?

Comment 46

[Stephen Rasku]

I have IE 4.0.  I can test it if it's live somewhere.

Comment 47

[Dave Miller [:justdave]]

it's live at http://landfill.bugzilla.org/bz63601/show_bug.cgi?id=365

(well, that whole installation, but that bug has a few attachments on it to test
with)

Comment 48

[Stephen Rasku]

Looks good for IE5 on Windows 95.

Comment 49

[Gervase Markham [:gerv]]

In Moz 1.1b on Win2K, right clicking an attachment and doing "Save Link Target
As..." suggests attachment.cgi.html. Clicking attachments Moz can't display Does
The Right Thing; obviously, clicking ones it can display displays them.

IE 6, on the other hand, works correctly in all cases.

Gerv

Comment 50

[Bradley Baetz (:bbaetz)]

gerv: bz knows about that bug - if you do save-as from teh linked page then it
works most of the time.

bz is of the opinion that fixing that to work correctly all the time in moz in
Hard, because of the way we load stuff (moz sends a HEAD request first, but
apache doesn't send the headers for HEAD modes. Taht could be a bugzilla thing
we could fix at some point, but even if we did, moz still won't always work)

However, we still need an ie < 5 test before this goes in....

Comment 51

[Stephen Rasku]

On IE 4.0 on NT 4.0.

The patches display directly in the browser.  You are not prompted to save the
file.  I think this is OK.

However, the gz file saves as the file name "attachment" without a file type. 
In mozilla it suggests "AutoResponseV0.3.tar.gz".  The IE 4.0 behaviour doesn't
seem right.

Comment 52

[Gervase Markham [:gerv]]

So are we going with this, or not? I say we check it in, and see if anyone
complains.

Gerv

Comment 53

[Dave Miller [:justdave]]

Based on comment 51, this is okay as stands.  The "suggest filename" thing is
completely broken in IE4, thus the reason we disabled it if the browser is IE4.
 The fact that you can view the contents of it at all in IE4 means this patch
didn't break usability on IE4.  (otherwise it would prompt for a Save As..
dialog whether the attachment was viewable or not, so you'd never be able to
view it in your browser window, even if it was text, which was the IE4 bug).

a= justdave, will check it in shortly, assuming it hasn't bitrotted.

Comment 54

[Dave Miller [:justdave]]

Comment on attachment 94808 [details] [diff] [review]
adds browser-sniffing

oof.  it's bitrotted

I don't have time to mess with it right now, someone remind me tomorrow and
I'll refresh it.

Comment 55

[Dave Miller [:justdave]]

OK, the primary conflict was the "isprivate" field getting added to the
attachments table.  New patch coming up shortyly.  Joel, you should probably
have a look at it just to double-check that I didn't break the private
attachments thing.

Comment 56

[Dave Miller [:justdave]]

Attachment: Merge with tip

Comment 57

[Dave Miller [:justdave]]

This is still live at http://landfill.bugzilla.org/bz63601/show_bug.cgi?id=365
fwiw.

Comment 58

[Gervase Markham [:gerv]]

Comment on attachment 100095 [details] [diff] [review]
Merge with tip

I'm testing this with Mozilla and the Content-Disposition field is meaning that
it pops a "download" dialog when you click the link for all file types,
including text/plain and text/html . It also pops this dialog when you go to
"edit", because you are trying to load the attachment into the iframe.

This works in 1.0 and (according to justdave) 1.1. So Mozilla is broken at the
moment. 

r=gerv. Check it in, and then we can file a bug with Landfill as the test case.
They will be more likely to fix it if they know it'll affect Bugzilla ;-)

Gerv

Comment 59

[Dave Miller [:justdave]]

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.23; previous revision: 1.22
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.190; previous revision: 1.189
done
Checking in template/en/default/attachment/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.11; previous revision: 1.10
done

Comment 60

[Joel Peshkin]

I looked over the patch.  I don't think there is a substantial risk of breaking
private attachments.  We're just both adding fields to the same list of fields
to select.

Comment 61

[Dave Miller [:justdave]]

This issue is not dead.  See bug 171296.

Comment 62

[Joel Peshkin]

*** Bug 175122 has been marked as a duplicate of this bug. ***

Comment 63

[Stephen Lee]

Given all the problems with content-disposition headers, etc. is there likely 
still to be any merit in tweaking the attachment links in the manner suggested 
in comment 9, and implemented in obsolete attachment 32415 [details] [diff] [review]?

I'm currently tweaking our local bugzilla 2.16 installation, and wondering 
whether to redo a rough equivalent to that patch or to go with the bugzilla 
standard.

Comment 64

[Bradley Baetz (:bbaetz)]

*** Bug 207823 has been marked as a duplicate of this bug. ***