Closed Bug 164816 Opened 22 years ago Closed 21 years ago

Extension fixup in nsExternalHelperAppService should only happen when executing


(Core Graveyard :: File Handling, defect)

Windows 2000
Not set


(Not tracked)



(Reporter: bzbarsky, Assigned: Biesinger)





(1 file, 3 obsolete files)

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: exe-san
I tried to download a "PDF" file from

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
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

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.
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
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
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 should still have Content-disposition as mentioned here, although
this seems to over-ride it.
*** 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.
Attached patch patch (obsolete) — Splinter Review
so... this should do it...
Target Milestone: --- → mozilla1.6alpha
Attachment #132217 - Flags: review?(bzbarsky)
Comment on attachment 132217 [details] [diff] [review]

>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


>+ * 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:
*** Bug 207907 has been marked as a duplicate of this bug. ***
*** Bug 201243 has been marked as a duplicate of this bug. ***
*** Bug 194832 has been marked as a duplicate of this bug. ***
Attached patch patch v2 (obsolete) — Splinter Review
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.


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


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

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+
Attached patch Updated to my comments. (obsolete) — Splinter Review
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

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 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),

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?

Attachment #133820 - Flags: superreview?(darin) → superreview+
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
Closed: 21 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:
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.