If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Extension fixup in nsExternalHelperAppService should only happen when executing

RESOLVED FIXED in mozilla1.6alpha

Status

Core Graveyard
File Handling
RESOLVED FIXED
15 years ago
a year ago

People

(Reporter: bz, Assigned: Biesinger)

Tracking

Trunk
mozilla1.6alpha
x86
Windows 2000
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

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.
Blocks: 120327

Comment 1

15 years ago
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

15 years ago
I see the problem described in the previous comment on Linux too.
OS: Windows 3.1 → All
Hardware: PC → All
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.
OS: All → Windows 3.1
Hardware: All → PC

Comment 4

15 years ago
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.

*** Bug 165810 has been marked as a duplicate of this bug. ***
*** Bug 166148 has been marked as a duplicate of this bug. ***
*** Bug 166273 has been marked as a duplicate of this bug. ***
QA Contact: sairuh → petersen

Comment 8

15 years ago
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

15 years ago
*** Bug 161485 has been marked as a duplicate of this bug. ***
I don't think that this is happening on Windows 3.1, we don't support it for ages.
OS: Windows 3.1 → Windows 2000
I'll take this...
Assignee: law → cbiesinger
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.
Hmm... that may work.  What about the case when mSuggestedFileName has no
extension?  What does Windows do with files that have no extension whatsoever?
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.
Created attachment 132217 [details] [diff] [review]
patch

so... this should do it...
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.6alpha
Attachment #132217 - Flags: review?(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....
Attachment #132217 - Flags: review?(bzbarsky) → review-
Whiteboard: biesi_testcase:http://192.168.1.1/~chb/testcase/mime2.php?do=1&type=application%2Fmsword&disposition=inline&filename=foo.bar&quote=1

Comment 17

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

Comment 18

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

Comment 19

14 years ago
*** Bug 194832 has been marked as a duplicate of this bug. ***
Depends on: 188865
Created attachment 133104 [details] [diff] [review]
patch v2
Attachment #132217 - Attachment is obsolete: true
Comment on attachment 133104 [details] [diff] [review]
patch v2

note that this patch requires the patch for bug 188865 to be checked in
Attachment #133104 - Flags: review?(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
Attachment #133104 - Flags: review?(bzbarsky) → review+
Created attachment 133820 [details] [diff] [review]
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.
Attachment #133104 - Attachment is obsolete: true
Comment on attachment 133820 [details] [diff] [review]
Updated to my comments.

darin, could you sr?  Especially the parts I just changed....
Attachment #133820 - Flags: superreview?(darin)

Comment 25

14 years ago
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
Attachment #133820 - Flags: superreview?(darin) → superreview+
Created attachment 133822 [details] [diff] [review]
Fix darin's issues

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

Removed that and used stringtail.
Attachment #133820 - Attachment is obsolete: true
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... ;)
the checked in patch looks good to me
...which means this can be marked fixed, I suppose
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
instructions for testcase: If you get an appended .doc extension when you choose
"Save page as", the test failed.

Whiteboard: biesi_testcase:http://192.168.1.1/~chb/testcase/mime2.php?do=1&type=application%2Fmsword&disposition=inline&filename=foo.bar&quote=1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.