433613 - Inline Content-Disposition filename not used when passing data to a plug-in

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[Phil Hayward]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b5) Gecko/2008032620 Firefox/3.0b5

The Content-Disposition filename parameter appears to be ignored when the content disposition is set to "inline".  The filename is used properly when the disposition is set to "attachment".  

It's likely that this affects other file types, but I am not sure, since I haven't tested it.


Reproducible: Always

Steps to Reproduce:
1. Open URL that will provide a header like: Content-Disposition:inline; filename=somefile.pdf
2. After the content is loaded in the viewer, save the file.
3. Note that the basename from the URL is used instead of the filename specified in the Content-Disposition header.
Actual Results:  
Filename from URL present in the Save As dialog

Expected Results:  
Filename from Content-Disposition header present in the Save As dialog

I presume (without having checked the source code - I'm new here and wouldn't have a clue where to start digging) that the file is being saved in a temporary disk cache before being passed to the helper application. The name of this temporary file is taken from the URL, rather than the Content-Disposition, probably due to the recommendations in RFC 2183 that Content-Disposition filenames not be accepted blindly.  

Could we possibly have Firefox save the file with the Content-Disposition filename, after doing some checking that the filename is safe for the filesystem, and that it won't overwrite anything?  If it fails the check, fall back to the URL for the filename.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This should work. We have code in nsExternalAppHandler::LaunchWithApplication explicitly designed to make this work.

Do you have an example url where it doesn't work?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Or is the issue that you're viewing in a PDF plug-in and then saving from within that plug-in, so that we're not talking about an external app at all?  I guess that would make some sense given the dependency on attachment vs inline...

Over to plug-ins; it would in fact make some sense to use the Content-Disposition filename when passing data to the plug-in.  Please let me know if I'm diagnosing this wrong (i.e. if you're really getting an external app, not a plug-in running in the browser window, or if you're saving using the browser's menus, not the plug-in's).

Comment 3

[Phil Hayward]

Your second comment is correct.  The issue is seen with the Adobe reader plugin.  When you change over to Content-disposition:attachment, it opens in an external app with the correct filename.

Here's a test URL:  http://pjhayward.net/pdf/testpdf.php

Here's the headers the web server sent:
Date: Thu, 12 Feb 2009 04:12:53 GMT
Server: Apache
content-disposition: inline; filename=aCompletelyDifferentFileName.pdf
X-Powered-By: PHP/4.4.9
Content-Length: 1242
Keep-Alive: timeout=2, max=200
Connection: Keep-Alive
Content-Type: application/pdf

Saving from within the plugin comes up with testpdf.pdf.  
I'm running Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6, if that matters.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Eran's going to give this a stab.

Comment 5

[Sven Ludwig]

In a large project we experience this issue, which for our customer is not regarded as minor, but has quite some relevance. Tested with Firefox 3.6.13 with Adobe Acrobat Plugin 9.3.0.148 and verified HTTP-Headers with HttpFox. The filename given in Content-Disposition is not used by the plugin as reported above (also regardless whether there are any quotation marks "" or '' around the actual filename or not).

Also see http://greenbytes.de/tech/tc2231/#inlwithasciifilenamepdf which refers to this bug here.

Can a fix be expected to come out in the near future, or maybe within 2011?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sven, if you want to take a stab at fixing this I'm happy to point you to the relevant code.  I'm not sure what Eran's up to, and I definitely won't have time to look at this in the next while; too busy with other issues.  But I'm happy to do some hand-holding here as needed.

Comment 7

[Javi Rueda :javirid]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:5.0) Gecko/20100101 Firefox/5.0

It worked fine to me, using http://greenbytes.de/tech/tc2231/inlwithfnattach.asis I browse to there and "Save as..." in the main menu. Firefox presented to me the dialog and in the "filename" field, the name was the same as in the HTTP header (checked with HTTP Live Headers).

Same with Firefox 3.6.18 on my netbook using Ubuntu 10.04. The behavior seen at Comment 3 doesn't meet what I saw at this same machine. The plugin tries to save the file with the name passed with the HTTP header.

Comment 8

[Julian Reschke]

(In reply to comment #7)
> User Agent: Mozilla/5.0 (Windows NT 6.1; rv:5.0) Gecko/20100101 Firefox/5.0
> 
> It worked fine to me, using
> http://greenbytes.de/tech/tc2231/inlwithfnattach.asis I browse to there and
> "Save as..." in the main menu. Firefox presented to me the dialog and in the
> "filename" field, the name was the same as in the HTTP header (checked with
> HTTP Live Headers).
> 
> Same with Firefox 3.6.18 on my netbook using Ubuntu 10.04. The behavior seen
> at Comment 3 doesn't meet what I saw at this same machine. The plugin tries
> to save the file with the name passed with the HTTP header.

Wrong test case. You should try

http://greenbytes.de/tech/tc2231/#inlwithasciifilenamepdf

Comment 9

[Javi Rueda :javirid]

(In reply to Julian Reschke from comment #8)
> (In reply to comment #7)
> > User Agent: Mozilla/5.0 (Windows NT 6.1; rv:5.0) Gecko/20100101 Firefox/5.0
> > 
> > It worked fine to me, using
> > http://greenbytes.de/tech/tc2231/inlwithfnattach.asis I browse to there and
> > "Save as..." in the main menu. Firefox presented to me the dialog and in the
> > "filename" field, the name was the same as in the HTTP header (checked with
> > HTTP Live Headers).
> > 
> > Same with Firefox 3.6.18 on my netbook using Ubuntu 10.04. The behavior seen
> > at Comment 3 doesn't meet what I saw at this same machine. The plugin tries
> > to save the file with the name passed with the HTTP header.
> 
> Wrong test case. You should try
> 
> http://greenbytes.de/tech/tc2231/#inlwithasciifilenamepdf

Ok. Sorry. Confirming (to myself) on Firefox 6.0.2 on Windows 7 x86

Comment 10

[Srishti]

Interested in fixing this bug

Comment 11

[Boris Zbarsky [:bzbarsky]]

It's yours!  ;)

Comment 12

[Srishti]

Here is a proposed patch for this bug,if anyone can please review it for me
http://pastebin.mozilla.org/2275833

Comment 13

[Benjamin Smedberg]

Could you please attach patches to the bug and set the review flags using the normal bugzilla mechanism?

As a quick comment, I will also say that this really should have an automated test; we have some pretty good examples in dom/plugins/test/mochitest. It's not yet clear to me exactly what you're changing, since the NPAPI doesn't expose any filenames to plugins that I know about.

Comment 14

[Srishti]

Attachment: proposed patch1

Please review this and let me know if I need to make any more changes.:)

Comment 15

[Benjamin Smedberg]

Comment on attachment 734267 [details] [diff] [review]
proposed patch1

So if I'm reading this correctly, what you are doing is changing the name of the file that is eventually passed to NPP_StreamAsFile?

I'm surprised and disappointed that any plugins change their behavior based on this filename :-(

I'm on the edge about whether I want to accept this change or not... what I'd really like to do is rip out the filename handling altogether and always use a temporary name. But if the PDF plugin actually uses this now, I guess that would just harm users.

Josh, do you have an opinion?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I'm surprised and disappointed that any plugins change their behavior based on this
> filename :-(

Benjamin, see comment 0.  Some plug-ins offer a "save" option, and will use the filename they have as the default.

Comment 17

[Srishti]

Attachment: proposed patch2

There were a few extra white space corrections and the scope of the tempfilename had to be changed, these changes are added to the new patch ,please review it.

Comment 18

[Boris Zbarsky [:bzbarsky]]

You probably want to just create a single patch, not a patch on top of your other patch...

Comment 19

[Josh Aas]

Comment on attachment 734267 [details] [diff] [review]
proposed patch1

Been a while since I looked at this code... My gut says this patch has compatibility implications that make it not worthwhile. Nice find though!

The bug we're attempting to fix here might be the lesser of evils. We have some good contacts on the Reader plugin team, perhaps we could discuss a fix with them?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Josh, what are the compat worries here?

Comment 21

[Srishti]

Attachment: patch

Comment 22

[Srishti]

Josh, Can you please explain me what compatibility issues you mean here ,and what is that I should to fix in here ?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Srishti, you want to use the needinfo field when asking someone in particular a question....

Comment 24

[Benjamin Smedberg]

Every time we make any changes to plugin-facing code we end up risking that the plugins make some assumption about our current behavior and breaking. We are very risk-averse in this context. I think the judgement call here is that making any change here is not worth the potential risk.

Comment 25

[Josh Aas]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #24)
> Every time we make any changes to plugin-facing code we end up risking that
> the plugins make some assumption about our current behavior and breaking. We
> are very risk-averse in this context. I think the judgement call here is
> that making any change here is not worth the potential risk.

Agreed.