164816 - Extension fixup in nsExternalHelperAppService should only happen when executing

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We should move the code that was introduced while fixing bug 116938 so that the
extension fixup does not occur for "save as".  It should only happen for the
"execute" branch of that code.

Comment 1

[Alexander Skwar]

I tried to download a "PDF" file from
http://iso-top.biz/pdf_wrapper.php?ReNr=4711&KdNr=0815&Firma=Fa&Name=Name&Adresse1=Adr1&PLZ=01234&Ort=City&Datum=2002-08-27&Versandart=1&Art%5B1%5D%5Bname%5D=Knoppix

I did this by left clicking on a link.  The pdf_wrapper script sends the
following headers:

2 Date: Tue, 27 Aug 2002 14:22:21 GMT
3 Content-Length: 3629
4 Content-Type: application/octet-stream
5 Server: Apache/1.3.20 (Linux/SuSE) mod_throttle/3.0 mod_ssl/2.8.4
OpenSSL/0.9.6b PHP/4.2.2 mod_perl/1.26 mod_layout/1.0 mod_fastcgi/2.2.2 mod_dtcl
DAV/1.0.2
6 X-Powered-By: PHP/4.2.2
7 Content-Disposition: attachment; filename=iso-top.de_Rechnung-4711.pdf

So, it is application/octet-stream and has a Content-Dispoition header with a
suggested filename.  Mozilla now suggests to save the file as
iso-top.de_Rechnung-4711.pdf.php.

That's wrong, because of the trailing .php.  IMO, extension fixup should only
happen when Mozilla actually calls a helper app to execute a file.  A simple
left click is not enough to determine this; the extension should only be messed
up, when the external app is invoked.  

When I right click on the link and select Save Link Target As..., Mozilla
correctly suggests to save the file as iso-top.de_Rechnung-4711.pdf, without a
trailing .php.

I tried this with Mozilla 1.1 on Windows NT.

I'm filing this as a comment to this bug per bug 147679, comment 6.

Comment 2

[Tom Mraz]

I see the problem described in the previous comment on Linux too.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Very impressive.. which build?  Please file a separate bug on that if it's a
current build (_not_ 1.0) and cc me, since this bug is about code that is not
even compiled on non-windows platforms.  Needless to say, I cannot reproduce the
issue in comment 1 on Linux, since the code responsible never runs.

Comment 4

[Tom Mraz]

I've retested it with current Linux trunk build and it's OK (it wasn't with some
older build which I've used before.) I apologize for the confusion and spam.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Vaskin Kissoyan]

There is a workaround that works for IE 6, NS7, and Moz 2002101508

If you know the filename during run-time in which the original link is generated
you can add the filename after the executable script such as:

script.cgi/iso-top.de_Rechnung-4711.pdf

script.cgi should still have Content-disposition as mentioned here, although
this seems to over-ride it.

Comment 9

[Max Alekseyev]

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

Comment 10

[Simon Paquet [:sipaq]]

I don't think that this is happening on Windows 3.1, we don't support it for ages.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

I'll take this...

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

ok... suggestion:
nsExternalAppHandler::mSuggestedFileName will contain just the filename as the
server sent it
mTempFileExtension will be empty if the temp file extension matches the mime
info, and will be the mime info's primary extension otherwise.

That way, the helper app dialog will get the original filename; for the leaf
name of the temp file, mSuggestedFileName + mTempFileExtension will be used.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Hmm... that may work.  What about the case when mSuggestedFileName has no
extension?  What does Windows do with files that have no extension whatsoever?

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

When trying to open them, windows shows the "Open file with" dialog, allowing
one to pick an application, not allowing one to remember the chosen program.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

so... this should do it...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 132217 [details] [diff] [review]
patch

>Index: nsExternalHelperAppService.cpp
nsExternalAppHandler::ExtractSuggestedFileNameFromChannel(nsIChannel* aChannel)

This could use renaming to indicate what it does now....

>+  // If the disposition header didn't work, try the extension from nsIURL

s/extension/filename/


>+ * to the MIME type (which was used to calculate mTempFileExtension).  This prevents
>+ * a cgi-script named foobar.exe that returns application/zip from being named
>+ * foobar.exe.

Add "and executed as an executable file" to the end there?

>  It also blocks content that a web site might provide with a
>+ * content-disposition header indicating filename="foobar.exe" from being downloaded
>+ * to a file with extension .exe.

"and executed".

I've been thinking... we may want to extract the suggested filename before even
doing the MIME lookup.	We already look for that extension; we may as well
combine the code and extract the filename in DoContent(), so that we don't have
to keep reparsing that http header....

Comment 17

[Henrik Gemal]

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

Comment 18

[Henrik Gemal]

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

Comment 19

[Henrik Gemal]

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

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 133104 [details] [diff] [review]
patch v2

note that this patch requires the patch for bug 188865 to be checked in

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133104 [details] [diff] [review]
patch v2

>Index: nsExternalHelperAppService.cpp
>+ * Also gives back whether the channel requested extenal handling (i.e.

"external"

>+   * permission...o.t. just use our temp file

"otherwise"

>+#ifdef XP_WIN
>+  // Make sure extension is correct.
>+  EnsureSuggestedFileName();
>+#endif

This will munge extensions on non-Windows on launch... I think that
EnsureSuggestedFileName() should be called unconditionally here, and the ifdef
should move into that code.

r=bzbarsky with that

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to my comments.

Changes I made:

1)  Put the ifdef around the Append() call when creating the salted name.  That
way it won't get fixed up on non-Windows systems.  Call EnsureSuggestedFilename
unconditionally so that we get the right mTempFileExtension on all systems,
since we use it for other things.

2)  Replaced the not-yet-existing RFindCharInReadable with some code that works
today.

3)  Made the illegal char substitutions before calling EnsureSuggestedFilename,
since it could be that things will start to match once illegal chars are
removed.  Plus, we needed to be doing said substitution on mTempFileExtension
and we weren't.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133820 [details] [diff] [review]
Updated to my comments.

darin, could you sr?  Especially the parts I just changed....

Comment 25

[Darin Fisher]

Comment on attachment 133820 [details] [diff] [review]
Updated to my comments.

>Index: uriloader/exthandler/nsExternalHelperAppService.cpp

>+    {
>+      // XXX RFindCharInReadable!!
>+      nsAutoString fileNameStr(aFileName);
>+      PRInt32 idx = fileNameStr.RFindChar(PRUnichar('.'));
>+      if (idx != kNotFound && (PRUint32)(idx + 1) != fileNameStr.Length())
>+        CopyUTF16toUTF8(Substring(fileNameStr, idx + 1, fileNameStr.Length() - idx - 1), aExtension);

maybe use StringTail here?

  CopyUTF16toUTF8(StringTail(fileNameStr, fileNameStr.Length() - idx - 1),
aExtension);

and isn't the second condition in the if statement unnecessary?  i mean,

  CopyUTF16toUTF8(StringTail(fileNameStr, 0), aExtension);

would be ok wouldn't it?  and wouldn't it result in an empty string, which
is equivalent to not calling CopyUTF16toUTF8?  i'm just thinking that you
can simplify things ;)


> void nsExternalAppHandler::EnsureSuggestedFileName()
> {
>   // Make sure there is a mTempFileExtension (not "" or ".").
>   // Remember that mTempFileExtension will always have the leading "."
>   // (the check for empty is just to be safe).
>   if (mTempFileExtension.Length() > 1)
>   {
...
>+    if (fileExt.Equals(mTempFileExtension, nsCaseInsensitiveStringComparator()))
>     {
>+      // Matches -> mTempFileExtension can be empty
>+      mTempFileExtension.Truncate();
>     }
>   }
>+  mTempFileExtension.Truncate();
> }

can't the inner Truncate() be left out?  and if so, then can't the branch
also be left out?

sr=darin

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix darin's issues

Whoa.  nice cath on that incorrect Truncate() call I accidentally added...

Removed that and used stringtail.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Fix checked in....  biesi, please look over the patch I just landed (the last
one I attached) and let me know if we need to undo one of my changes for 1.6a... ;)

Comment 28

[Christian :Biesinger (don't email me, ping me on IRC)]

the checked in patch looks good to me

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

...which means this can be marked fixed, I suppose

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

instructions for testcase: If you get an appended .doc extension when you choose
"Save page as", the test failed.