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

RESOLVED WONTFIX

Status

()

defect
--
minor
RESOLVED WONTFIX
11 years ago
6 years ago

People

(Reporter: phil, Assigned: srishsensation)

Tracking

(Blocks 1 bug, {student-project})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
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?
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).
Status: UNCONFIRMED → NEW
Component: File Handling → Plug-ins
Ever confirmed: true
Product: Firefox → Core
QA Contact: file.handling → plugins
Summary: Inline Content-Disposition filename ignored for PDF files → Inline Content-Disposition filename not used when passing data to a plug-in
Whiteboard: [good first bug]
(Reporter)

Comment 3

10 years ago
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.
Eran's going to give this a stab.
Assignee: nobody → erangross1

Comment 5

8 years ago
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?
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.
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

8 years ago
(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
(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

Updated

8 years ago
Blocks: 609667
OS: Windows 2000 → All
Hardware: x86 → All
Assignee: erangross1 → nobody
(Assignee)

Comment 10

6 years ago
Interested in fixing this bug
It's yours!  ;)
Assignee: nobody → srishsensation
(Assignee)

Comment 12

6 years ago
Here is a proposed patch for this bug,if anyone can please review it for me
http://pastebin.mozilla.org/2275833
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.
(Assignee)

Comment 14

6 years ago
Posted patch proposed patch1 (obsolete) — Splinter Review
Please review this and let me know if I need to make any more changes.:)
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?
Attachment #734267 - Flags: feedback?(joshmoz)
> 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.
(Assignee)

Comment 17

6 years ago
Posted patch proposed patch2 (obsolete) — Splinter Review
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.
You probably want to just create a single patch, not a patch on top of your other patch...

Comment 19

6 years ago
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?
Attachment #734267 - Flags: feedback?(joshmoz) → feedback-
Josh, what are the compat worries here?
(Assignee)

Comment 21

6 years ago
Posted patch patchSplinter Review
Attachment #734267 - Attachment is obsolete: true
Attachment #735723 - Attachment is obsolete: true
(Assignee)

Comment 22

6 years ago
Josh, Can you please explain me what compatibility issues you mean here ,and what is that I should to fix in here ?
Srishti, you want to use the needinfo field when asking someone in particular a question....
Flags: needinfo?(joshmoz)
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX

Comment 25

6 years ago
(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.
Flags: needinfo?(joshmoz)
You need to log in before you can comment on or make changes to this bug.