Closed Bug 91969 (launchexe) Opened 18 years ago Closed 17 years ago

[FIX]Launch file disabled on .EXE (executeable) download

Categories

(Core :: Security, enhancement, P1)

x86
Windows 2000
enhancement

Tracking

()

VERIFIED FIXED
mozilla1.4beta

People

(Reporter: jpt.d, Assigned: bzbarsky)

References

()

Details

(Whiteboard: [se-radar])

Attachments

(2 files, 13 obsolete files)

7.10 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review
1.53 KB, patch
timeless
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review
Launch file disabled on EXE (executeable) download
We disabled the automatic launch of .EXE files due to security reasons.

marking invalid.... (please reopen if you disagree)

Status: UNCONFIRMED → RESOLVED
Closed: 18 years ago
Resolution: --- → INVALID
I believe that I should be the one to make that determination. There is as much
security risk opening up a zip file and executing something as there is with the
browser executing something at my request. It is my responsibility to take
measures for possibly infected files.

Because I will run the thing I download anyways there is no point in mozilla to
automatically not allowing the option. 

I also do believe that there are security issues that must be delt with here as
well - but giving choice is a big part of what mozilla is about... 

Would it be evil to provide an option to allow this action? 

Microsoft even provides a way to execute things, warning you first wether or not
it is signed. I do not care for this way either, but a warning about possible
virii is a good solution to this problem.

So in summary, alternatives to the absolute no executing of executeables:
a) Provide an option to enable the activity
b) Provide a warning about possible consequences.
Status: RESOLVED → UNCONFIRMED
Resolution: INVALID → ---
marking NEW
Assignee: mpt → mstoltz
Status: UNCONFIRMED → NEW
Component: User Interface Design → Security: General
Ever confirmed: true
QA Contact: zach → ckritzer
You make a good point. THe issue isn't settled; the current solution errs on the
side of caution. Let's use this bug to discuss the issue. Some talking points:

1) What types of content, if any, should the browser execute automatically,
rather than just saving to disk?

2) What sort of warning should be provided?

Personally, I don't think we should offer the option of automatically running a
downloaded executable. Users often ignore warning dialogs. For the user to go to
the directory where the file was downloaded and double-click it makes the
consequences very clear. Users understand that an executable downloaded to their
disk and run by double-clicking it or from the command line can do anything it
wants to the user's machine. I'm not sure that most users equate clicking OK in
a dialog (one of many dialogs they see every day) with running a program
locally. This makes it easier for a malicious site to induce a naive user to
download a file and run it without realizing what they've done.

You're right about giving choice, though. That is what Mozilla is good at. I'm
not against creating a hidden pref to enable the Run button. It wouldn't be
enabled by default, though.

cc'ing law though I think he's on vacation, he's the person to actually make
this change.  
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.4
Target is now 0.9.5, Priority P1.
Priority: -- → P1
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Target is now 0.9.5, Priority P2.
Priority: P1 → P2
*** Bug 99740 has been marked as a duplicate of this bug. ***
time marches on. Retargeting to 0.9.6.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
my god, can't you just enable it for now? i want to throw my computer across 
the room every time it happens. :) I would try to fix it myself or something, 
but I have no idea where to even begin.
performance, footprint, feature work, and re-architecture bugs will be addressed
in 0.9.8
Target Milestone: mozilla0.9.6 → mozilla0.9.8
Bill, this is the issue we talked about on Friday. I assume this is a dupe but I
leave it to you.
Assignee: mstoltz → law
Status: ASSIGNED → NEW
will this apply to all executable files (eg .cmd, .bat, .exe, .com, .vbs, .vba, etc)
how about it just comes with a default list of extensions and you can change (or
disable) them? :)
I don't see why you just don't enable the button. Something that should have
taken maybe three minutes for someone who knows the source code has taken near 6
months. I feel like uninstalling Mozilla or using IE to download files everytime
this happens to me. If Mozilla is about choice and is going to be used by
intelligent people, why not let them use their intelligence in determining what
should be opened... If they chose to download a file, why not let them open it
instead of using your idea of safety and caution to govern their choice?
This feature got axed so I'm not sure it's in or out.  Moving to 9.9 till I can 
sort it out.
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Is it really a lot of work to create an (undocumented, I don't care ;))
preference setting in the meantime?

This is annoying the heck out of me ;(

Please fix this and make it optional. I see the point of having the user
doubleclick on this locally but I'm also well aware of the fact that disabling
the button is annoying a lot of (expert) users...
yeah, I understand the need to dumb the browser down to the lowest level of user
experience, but.. wait.. no, I don't understand that. PLEASE make it an option.
thanks. :)
Please confine any pleading/arguing/whatever-you-want-to-call-it to the
newsgroups.  Every time you post a comment in a bug, Bugzilla sends numerous
messages to all the developers involved, and just reading it all *greatly
reduces* the amount of time they have to do real work.  If you want to make a
convincing argument in a bug, attach a good patch, or convince someone else to
do so, in private. Patches settle most bugs.  
Mozilla already lets XPI code run after a single click in a dialog.  As long as 
the UI for running downloaded .exe files is better than the UI for XPIs, fixing 
this bug won't make Mozilla less secure.
Spam: Dropping off my list for MachV/mozilla1.0.  If you have issues, please 
make your case by stating which of my mozilla0.9.9 or mozilla1.0 bugs you think 
are of lesser importance.

Please note that *some* of these might be fixed elsewhere if there is work 
being done in the same area of code.  In most of those cases, I will have 
marked such bugs by putting the *real* bug in the "depends on" field.
Keywords: helpwanted
Target Milestone: mozilla0.9.9 → Future
Bill, what is considered the correct fix for this bug?  Is it just a warning
message that can be turned off when you choose open for exe files?  IE6 kicks
our ass in this area.  We make users download the file and find it on their hard
drive.  IE presents a simple dialog asking the user if they want to open the
file or save it to their computer.  (IE6 doesn't even warn users, although the
dialog has a More Info button that talks about viruses...)
Correct.  This feature got booted from the MachV plan, is all, and I have too 
many bugs that didn't.  See the MachV plan for more details.

Note the "helpwanted" keyword.
Attached patch Patch V 1.0 (obsolete) — Splinter Review
This patch was supplied by Manoj Meheta <manoj_r_mehta@yahoo.co.uk>.  I simply
made the diff for him.
Well, sadly this patch doesn't seem to work in my Win32 tree.  The Launch File
button is still disabled with this patch applied.  (Note to self: test patch
before attaching to random bugs). ;-(  Manoj, I think you need to patch
http://lxr.mozilla.org/seamonkey/source/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js#189
or something.
Blocks: 106094
Attached patch Correct patch (obsolete) — Splinter Review
(should we update the qa field?)
Is this patch still workable?  1.0 should not be released without a way for me
to open an .exe file after a download.  I grow tired of having to click 'open
location in explorer' and then having to hunt down my file every time.  This
would also probably make the people who scream about download manager is useless
in the nightlies, enjoy it a bit more.
Is there anyone willing to compile the latest mozilla releases with this bug
fixed, for Windows XP??

Or will the mozilla developers eventually add a check box in Preferences to
enable the launch file button for executables?
*** Bug 149752 has been marked as a duplicate of this bug. ***
You marked my duplicate bug as 'resolved' - This problem is still there in today
(2002060611)'s build. Why can't you guys just fix this? Disabling a button isn't
security, it's idiot-proofing, and I for one don't want an idiot-proofed
browser. Do you want to make your users think you're calling them idiots?
Because that's what you're doing. I haven't seen any good reasons to keep this
button disabled - if someone wants to disable it, why not make it a checkbox in
preferences, or something?
Kevin Gadd:
>You marked my duplicate bug as 'resolved' - This problem is still there in today
Sure, it's a dupe of this bug. And you still see it because this bug has the
state " _NEW_ "
*** Bug 150193 has been marked as a duplicate of this bug. ***
*** Bug 152412 has been marked as a duplicate of this bug. ***
*** Bug 152682 has been marked as a duplicate of this bug. ***
If people know what they're doing here... they can patch their
nsProgressDialog.js. Of course, people that do this are the rare minority, so I
seriously doubt there will be big security holes, as people that are doing this
KNOW WHAT THEY ARE DOING.
Attached patch.diff for linux systems with an already installed mozilla.
patch.diff works by patching nsProgressDialog.js to the right code. To execute
the patch, chdir into your mozilla directory and type patch -p0 <
(/path/to/patch/)patch.diff.

THE -p0 ARGUMENT TO PATCH _IS_ NECESSARY!
Another feature that might be helpful here is a minor feature that IE shows but
that doesn't seem to be present in Mozilla: when you click "show location" in
IE, the file explorer opens AND selects the file you just downloaded.  Is it
possible to reproduce this action in Mozilla?  Would this work on Linux as well?
Re: Comment 38

Weird. One old version of mozilla, (pre 1.0) can't remember which one it was, DID select the file that you were supposed to have. I wonder where the feature went... or it could've been my weird windows tweaks.. but now it doesn't do it anymore.
Reply to #38 (Martin). Mozilla does this too, it opens up Explorer and
highlights the file just downloaded. But you cannot directly press enter in the
explorer window because though the downloaded file is highlighted (in blue), it
isn't selected. The first file in the Explorer window gets selected. Maybe the
call to launch Explorer needs to be modified so that the downloaded file is both
highlighted and selected.
i'm using the latest nightly for 1.1a+ (2002070813) and the windows fix listed
here does not work for me. Octalc0de, could you post a new fix or email one to
me? Thanks.
Whoops. Sorry, forgot about the download manager. Currently, I'm trying to find where to tweak it... but the current workaround is use my patch, and tell mozilla to use the Progress Dialog for file downloads. (Edit->Preferences->Navigator(?)->Download)

Will post once I find the file.
Can't seem to find the .js file that controls the download manager 'dialog box'. I might have to root around in the source... but not everyone wants/can/has the time to compile mozilla. If anyone finds a .js file that controls the download manager in versions 1.1a++, email or post.
*** Bug 156729 has been marked as a duplicate of this bug. ***
Given the fact that people might not have the time/knowledge/whatever to enable
the launch button on the download dialog, I am creating a new attachment with
the modified file. I am running Moz on windows, therefore my file has been
modified as per the step by step directions for patching windows versions of
mozilla. Linux users, sorry, I can't help you here, but you can extrapolate the
change you need to make from the directions provided in attachment #3 [details] [diff] [review]. Download
and save this file to

<installation-drive>\Program
Files\mozilla.org\Mozilla\components\nsProgressDialog.js

or <moz-install-directory>\Mozilla\components\nsProgressDialog.js

Enjoy
Yes, Linux users can use the .js file posted, and they can also extrapolate from
my file posted. I've tried the directions on a Linux build, and they work fine.
However, for Linux users, using the diff file might be easier.
reassigning to me, I'd really like to see this one fixed.
Assignee: law → blaker
Nominating for buffy:

* This was originally planned for machv (see bug 106094).
* There is huge gain here in making it easier for users to launch installers and
such that they download off the web.  IE kills us in this area.
* The patch is simple (just add a warning dialog).

jatin, who has to make the decision on wording here?  Seems like it should be legal.
Keywords: nsbeta1
Attached patch patch (obsolete) — Splinter Review
Attachment #68288 - Attachment is obsolete: true
Attachment #72241 - Attachment is obsolete: true
Attachment #89724 - Attachment is obsolete: true
Attachment #90244 - Attachment is obsolete: true
Attachment #91159 - Attachment is obsolete: true
+        onunload="Shutdown();">

where are you defining this function?
I am against this bug. See my discussion with trudelle on .browser or .ui (don't
remember). I completely agree with mstoltz in comment 4. Make it a hidden pref,
default off.
Even if you fix this bug and turn it on in the default build, make it preffable.
I don't want this in Beonex Communicator. Otherwise, I'd consider fixing this
bug a regression.

> IE kills us in this area.

No, IE kills its users in this area. ;-)
re comment 20: Yes, it is far too easy to launch XPIs at the moment.
Nav triage team: nsbeta1-
Keywords: nsbeta1nsbeta1-
Your objection has been noted. Please either prevent damning evidence that the
security risk posed by IE greatly outweighs the convenience of this feature, or
take your discussion back to the newsgroups.  Feel free to turn this off in Beonex.
> Please either prevent damning evidence that the
> security risk posed by IE greatly outweighs the convenience of this feature

<http://securityresponse.symantec.com/avcenter/vinfodb.html/>
If you're dumb enough to DOWNLOAD one of those viruses... I hardly see what the
difference between 'clicky clicky' and opening the folder, and opening the exe.

Also, if you've got anti-virus programs, and the program is a 'known' virus,
it should intercept it.
A list of viruses is not damning evidence, especially given that most of those
are e-mail based.  Please present some proof about *this specific feature* or
please stop commenting here.
> especially given that most of those are e-mail based.

Doesn't Mailnews use the same dialog (consider esp. IMAP)?
> Please present some proof about *this specific feature* or
> please stop commenting here.

What "proof" do you expect? Running downloaded exes is the most dangerous thing
you can do with a computer, and I provided proof for that.

You told me to take the discussion to the newsgroups. That's what I did, mid of
Dec 2001, in n.p.m.browser, thread "Comments on MachV plans", and I just added
another post to the thread. In addition to that, mstoltz explained very well in
comment 4 why this is so dangerous. I don't see any of the arguments being
invalidated. "This is more convient" and "IE does it this way" are no arguments,
esp. when it comes to security. A patch doesn't beat arguments either.
> mid of Dec 2001, in n.p.m.browser, thread "Comments on MachV plans"

Rereading it, there's lots of old and irrelevant stuff in it. For your
convience, just read <news://news.mozilla.org/3D41D60A.8090606@beonex.com>
[Google doesn't carry it] (and Trudelle's posts, if you want).
I don't think forbidding users to execute downloaded exe's is an acceptable
solution.  There are a number of software sites that describe to IE users how to
run the software they're downloading.  The obscurity of the Windows file system
should not be the deterrent we're relying on to prevent people from launching files.

How many users know how to navigate to the directory of the file they
downloaded?  Not many, we decided, hence the addition of "Show File Location". 
Why make them click that and *then* double click?  I think if they want to
launch the file, they will, and we're just slowing them down.  And I think
comment 4 gives users way too much credit; I'd guess most users don't launch
stuff from or interact with Windows Explorer folder windows on a regular base.

* Continuing to disable launch for exe's downloaded via e-mail might be good.

* I agree that IE makes it way too easy to launch exe's, since all you have to
do  is click a download and press enter.  Here, you'd have to save the file,
*then hit Launch*, then dismiss a dialog.  I'm not convinced that every user
dismisses dialogs without reading them, as nobody has pointed me to studies
confirming that, but in any case, clicking Launch is a very precise and
deliberate action.  It's not the default button, it's a very charged word (i.e.,
it doesn't sound like "Cancel" or "Close"), and in the download manager it's
even less conspicuous.

BenB: I've heard your arguments, there is no need to reiterate them.  I'm hoping
to bring Mitch back into the discussion.
> BenB: I've heard your arguments, there is no need to reiterate them.

*You* asked me to restart the discussion, and you continue to argue. I just told
my opinion ('please wontfix'). I can't be turned on and off however you please.

But I agree that a newsgroup is more appropriate for discussion. I started a new
thread in n.p.m.browser: <news://news.mozilla.org/3D41EE48.1040305@beonex.com>.
This is completly ridiculous.  Having launch disabled does ABSOLUTELY NOTHING 
to make it harder for a virus to attack someones computer, and people who 
think it does are fooling themselves.  People who want to run .exe files will 
do so whether or not they have to dig for the file or just launch after it's 
downloaded.  This is such a pain when, on Windows for example, your download 
folder might be set to "arrange by name" or "arrange by size" and then you 
have to dig that much more just to open the file you want because the files 
are scattered.  BTW, I have never seen my file highlighted when I click "open 
location".  No other browser or download manager behaves this way and there's 
a reason.  It's because they wouldn't be working the way they were intended 
to.  That's why I'd rather use IE or a download manager, and (one) of the 
reasons that Mozilla is not my (and probably many others) default browser and 
won't be.  This is basic functionality and it's SAD that the most basic and 
easiest things in Mozilla are the hardest to do.
This so called "feature" is very annoying, and if possible I would like to see
an option in preferences somewhere. Even if it was turned off by default, it
would be a definate plus.
*** Bug 162973 has been marked as a duplicate of this bug. ***
I would also like to voice my concern over this bug - it appears to the casual
user (viz.: me) that it is an error in Mozilla, not a 'security feature', seeing
how every other browser that exists can transparently open executable files
without you  even needing to specify a place to save them to, much less
requiring you to navigate to that folder because the seemingly obvious thing to
do ("Launch File") is disabled. I feel strongly that the ability to have Moz
save the file to a temp folder and then run it from there is necessary for all
but the most inept of users, and should be an undocumented prefs.js setting if
nothing else. Protecting users from themselves seems to me to be a rather silly
strategy for ensuring security. I also could appreciate the idea of a 'blocked
extension' list that you can modify to disable automatic launching of certain
filetypes, however, that would be more difficult to implement than merely giving
Mozilla the same support for executable files everything else has. In any event,
this is not a strength of Mozilla, and certainly isn't a feature. I would love
to see an undocumented (or even better, a documented one, possibly under the
Security tab) that allows Mozilla to handle executable files like every other
browser does.
I don't want anyone or anything, especially a web browser, to protect me from myself. Especially over something this stupid. If I want to install some program, but not keep the installer around, which is more convient? IE (click link, click Open (in 6, or open from current and OK), step through installer) or Moz (click link, find a place to save it, click save, find the installer (or click reveal), run it, step through it, and finally delete the installer)

Frankly, between Moz and this annoying bug, and Crazy Browser (IE with tabs and a popup filter), I'd rather use Crazy Browser.

Make save the default option if you want (so you actually have to click open or launch or whatever). Or maybe a hidden preference. Whatever. Just fix it. If you aren't running virus protection, you diserve what you get anyway.
I too would like to see at least a setting under Advanced Preferences to enable
launching executables, despite just about every other browser already providing
the convenience instead of making users jump through hoops. However, I would
much rather have exe launching enabled by default. 

I've been editing nsProgressDialog.js to let me launch executables, but this
workaround is not acceptable because users should not have to hunt for any sort
of text file and do a search & replace just to get a function that they expect
as a default. Also, everytime I install a nightly build I end up having to do
the damn .js editing all over again.

Whether it takes one click to open a potentially malicious exe or 2,3+ clicks
because mozilla puts a few hoops in front of the user, the end result is going
to be the same: the user will open an exe that they *CHOSE* to download. It's
not mozilla's fault either way if something bad happens from running the exe, so
why bother with the extra windows and extra clicks?

Please fix this for 1.2.
QA Contact: ckritzer → bsharma
Come on already, this "feature" had been targetted for 0.9.5, and it's still
around in 1.2. I consider this "feature" to be THE most annoying thing in Moz,
together with the unnecessary abbreviation of links in my Personal Toolbar. Many
people before me have pointed out how silly this "feature" is in preventing
users from opening viruses and trojans.

If you really want to help your users to surf safely, point out the risks of
opening unknown exes and such. This just annoys the hell out of experienced
users and does nearly nothing to protect the truly dimwitted.
Hi guys,

I've been following this bug for some time now, and here's what I think (I
believe this was mentioned a while ago):

Make an option somewhere in preferences to disable this "security" thing. Make
it unchecked by default. Only those that are technical enough will be able to
find it. If you want it that badly, if someone checks this option to disable the
security, give them a warning message that says something about the file
containing a virus and all. IE does this, but it's easy to disable it. 

I think it's about time to get rid of it. It's not really a security thing,
either. Yeah, sure, it doesn't let someone open the file they just downloaded,
but that's a false sense of security. Any user will still be able to go to the
place where they downloaded the file and run it. What security is there? If you
think that this makes Mozilla safer, it doesn't. Plain in simple. In my opinion,
this is probably the worst "feature" in Mozilla. In fact, this feature probably
only makes new users think skeptically of Mozilla. I'm sure that IE is somewhere
on the user's computer, and if something's wrong with Mozilla, IE is a great
alternative.
IE 6.0 plus a bunch of patches on WinXP has the same behavior as Mozilla: if I
type
http://ftp.mozilla.org/pub/mozilla/nightly/latest/mozilla-win32-installer-sea.exe
into the address bar and press enter, I get a file picker immediately.
I think it should be easier to install programs from web sites.  Some things
that might help security without impacting other aspects of usability too much:

1. Make the dialog for running an executable the same shape as the XPInstall
dialog rather than the "What do you want to do with this file?" dialog.  (This
is the most important change, IMO.)

2. Label the button "Install and Run" or "Launch" rather than "Run" or "Open". 
("Install and Run" comes from non-button text in an earlier version of Internet
Explorer. Blake suggested "Launch" in comment 61.)

3. Follow the same precautions as in the XPInstall dialog (e.g. bug 162020).

4. The first time the user clicks "Install" or "Save as...", show a dialog with
a longer warning.  The text might concentrate on how to determine whether a site
is trustworthy.  The dialog would have a "show this every time" checkbox and
might only be shown the first time by default.

5. Replace "A web site" in the XPInstall dialog with the domain name.

6. Include a "Save as..." button (bug 47805 for XPInstall dialog).

7. Make it harder to run executables from mail messages than from web sites.
Summary: Launch file disabled on EXE (executeable) download → Launch file disabled on .EXE (executeable) download
Attachment 92811 [details] [diff] (created by Blake Ross) seems to solve all these issues. It
warns you that it could be a virus, and it enables the launch file. Except...
it's not getting anywhere. Could somebody set the Request flag for review of the
patch? (I don't have any reviewer email addrs.)
Attachment #92811 - Flags: superreview?
Attachment #92811 - Flags: review?(timeless)
Comment on attachment 92811 [details] [diff] [review]
patch

I'm flattered, but I will not be held responsible for this.

mstoltz: if you want this, please sign on the dotted line :)

here's a strict review of the code itself:
@@ -467,6 +467,19 @@
4/4/2 space indentation

Index: progressDlg/locale/en-US/nsProgressDialog.properties
This message is not acceptable to me, and if I were the security module owner
or a lawyer or the ui owner or anyone else with a stake I would reject it. The
message appears childish and unless the user really understands that it's
mozilla and not a webpage that's triggering the message, the user could decide
it was a joke from a webpage and randomly accept the prompt.

bz: i'm only poking you so that you're aware that this patch exists.

I quickly skimmed the bug but didn't pay much attn to jesse's comments (they're
long and my mozilla-win32-talkback nightly from last night decided that
painting wasn't important which makes it very hard to read anything). I don't
think I saw any mention of allowing the user to do something like scan the file
before running it.  WinZip has an option which lets you do this, and certain
other apps do too.

Has anyone considered asking security companies (mcafee, symantec, ...) what
they think is the best way to address this problem?
Attachment #92811 - Flags: superreview?(bzbarsky)
Attachment #92811 - Flags: superreview?
Attachment #92811 - Flags: review?(timeless)
Attachment #92811 - Flags: review?(mstoltz)
> allowing the user to do something like scan the file before running it

I don't think that scanning binaries is such a good idea. There is a serious
class of problems that cannot be protected by virus scanners, because
- the app is rare or
- the hostile functions are unknown so far or
- the app is too common (try classifying kazaa as trojan :-( )
so it would give a false sense of security. Good virus scanners scan downloaded
binaries *anyways*, automatically.
This patch needs to be tested on mac.  If I recall correctly, the mac version of
isExecutable() lies horribly.

Past that, I have washed my hands of the security issues in file handling on
Win32.  The whole thing is insecure by design (of the OS, mostly).
>I don't think that scanning binaries is such a good idea.+
It's a good idea. I don't use a background scanner because you get performance
problems and i know what i download. A virus scanning option for the DM is a
good idea because you get additional security.

>so it would give a false sense of security.
That's wrong. Everyone (should) knows that a virus scanner can not find all
viruses and also that you get wrong alerts.

But there is AFAIK another RFE for this feature..
Let's not exaggerate. I always hear that Moz is not directed at the general
user, so IMO a warning (which can be disabled) is more than enough. We cannot
protect everyone from his own carelesness.
Someone more familiar with this code should do the review, not I. Anyway, I
still think the launch button should be disabled, rather than putting up an
oft-ignored dialog.
Comment on attachment 92811 [details] [diff] [review]
patch

that's three hand washings (bz, mstoltz and me). mstoltz is opposed to the
button being enabled and I'm going to treat that as a review.
Attachment #92811 - Flags: superreview?(bzbarsky)
Attachment #92811 - Flags: review?(mstoltz)
Attachment #92811 - Flags: review-
I'm going to try not to sound too frustrated :D

IMHO mstoltz's 'review' was not a review but a personal opinion :\ This bug has
been open for almost two years now and we are still getting nowhere ;( Just a
lot of chit chat about protecting users and other security blahblah. Users do
stupid things to their computers and to theirselfs, so what? A lot of people get
annoyed by this lack of functionality and I seriously doubt that this 'feature'
has saved even one computer on this planet. Anyway, I really don't want to
reitterate the points that have already been made 20 times in this bug. Just
venting some frustration here. Alas OS sure does have it's drawbacks.

Anyway. IMO 'review-' isn't a keyword, but if I missed something could someone
please fill me in on the meaning of 'review-'?
From what I've seen of accepted patches getting "r+" and "sr+", I would guess
that "review-" is "review-minus": a thumbs-down decision. My apologies if I'm
mistaken.

I don't think I'll ever be able to voice my frustration with this bug
adequately. I've been trying to think of a way to automate downloading and
post-install patching of nightly win32 binaries. As of now this is my only
option since it seems that despite many users' frustration, this bug is going
nowhere and the powers that be are content to blame it all on the OS' lack of
security. (Even though any other OS which a user has root access to will let
that user, logged in as root, wreck the system by running a program just as well
as windows. No browser ever saved a computer from annihilation by user error)

At this point I'd at least be amused if the final decision ended up with a
dialog box that says, "I'm sorry Dave, I'm afraid I can't do that."
How about a dialog box that says, "I'm sorry, the following people have decided
that you're not responsible enough to execute programs," followed by the names
of dissenters here? Let all those greatful users know who to thank for
protecting them.

I'm sorry, I'm just a little frustrated as well, and on many different levels
(practical and idealistic).

The mindsets that would prevent this from going through are much more fitting to
other, less lauded projects.
Let me put it this way: This bug would be one of the very first reasons I'd have
to leave Mozilla, and if it persists long enough, I'm sure I WILL quit, just
because I'll get very irritated from editing that stupid .js file every time I
install a beta or a new release.
How hard is it to make a configuration option (heck, have launch disabled by
default) or a dialog that must be watched for a certain time the first time it
pops up?

Just face it: Mozilla is no where near mainstream yet, and having this so-called
feature won't bring in any new blood. Just annoy the hell out of more
experienced users because it is limiting them in the usability of the browser.
Please stop venting.  I raised a technical question about the patch.  No one has
addressed it.  The patch can't land in its current state until that question is
addressed.  I have no religious views on the issue one way or another, as I said
in comment 80.  But I'd like to be either told that isExecutable() does the
right thing on the mac or why it does not matter in this case (the latter may
well be the case).
Apparently isExecutable() works fine enough to _disable_ the button, now 
doesn't it?

Let's please drop the excuses, give the patch review+, and let's get it in the 
tree. I'm sick of waiting here while we debate the merits of protecting users 
from themselves. Users aren't stupid, especially users who will install an 
alternate browser. The submitted patch is a fine solution, especially judging 
from the fact that some method is currently used on the mac to detect whether 
the file is executable or not - let's use it and give users back their choice --
 where it belongs. I don't want three developers to sit up on their high horse 
and pooh-pooh a fix to a bug (23 votes - that's a hint.) that should've been 
fixed a LONG time ago.
Flags: blocking1.3b?
I'm sorry to say this, and this may sound a little offensive, but: one person
should not be allowed to make a decision that something is not going to be
included in the release of an open-source, publicly developed project that has
100s of developers! With 23 votes and over 1 year of inactivity, it's about time
to include this patch in the final build.
> Apparently isExecutable() works fine enough to _disable_ the button, now 
> doesn't it?

Does it?  I have no idea.  If it does, that's great and would fall under the
"explain why it does not matter" option.

You seem to misunderstand how "review+" works.  It means, "I can vouch that this
change is correct."  Now I could spend a few dozen hours convincing myself that
it is.  Or someone who cares about the change could spend some time convincing
me that it is (by checking whether isExecutable does the right thing on the
Mac).  The former is not likely to happen, because as I already said I really
don't care one way or another about this change.

Max, you are completely wrong. "Open Source" does not mean "mob rule".  It means
"anyone can see the source and suggest changes."  Whether those changes get
accepted is up to the module owner (for example, a change to change some
fundamental behavior in layout would require approval from the layout module
owner even if it had 3000 votes; this case is no different).  In this case, I'm
the closest thing to a module owner, I think, unless someone is volunteering to
take the job.  Since the patch relaxes as security precaution, it needs approval
(which is not the same as code review, mind you) from Mitch, who is the person
responsible for the security of Mozilla.

So to reiterate:

1) The patch looks perfectly reasonable from a code standpoint
2) It needs testing on mac before it lands (should be easy with all the people
   here who want this patch, right? The patch can be applied with PatchMaker to
   a nightly to test in case anyone cares).
3) It needs approval from Mitch before it lands.

These are not "excuses", they are just facts of life.  Deal with it.  If you
don't like mozilla.org's policies or the decisions of module owners, you're free
to start your own project based on the code or to request that
drivers@mozilla.org change module owners (in the latter case, you need good
reasons; "not accepting an untested patch" is not a good reason).
Thanks for the clarification. I did not mean any offense to the module owner or
any other developer of Mozilla. I'm sorry if it sounded that way. I think I was
confused by the explanation of 'review-' in comment 82. 

As far as I understand, the main issue at this point is point 2 in comment 88.
Before the patch can land, the issue with isExecutable() on Mac must be resolved
(if there is any). Once this is done, the patch is ready to be inserted, if the
module owner accepts. I guess all this "chit-chat" is here because no one has
been able to do a test of isExecutable() on Mac? This brings up another point:
how do you test it, properly?
Well, a simple two-step test would be:

1) Download a file you know is executable.  Launch it.  Does the dialog come up?
   Does the file execute?
2) Download a file you know is not executable (eg Word document).  Launch it.
   Does the dialog come up?  Does it get opened in Word?

I'd love to hear what the results are.
The problem is, I don't have a Mac :) I can tell you that it works properly on
Windows, though (but I have a feeling people already know this). Sorry that I
didn't mention it in my previous email. 
I have been waiting for this 'bug' to be resolved for a tremendously long time,
but don't you think the warning in the patch, "You're launching %S, which could
be a virus. Beware!" is a bit silly?

Not that I personally care, but users aren't going to take that line too
seriously, which makes it almost pointless to include.

Something like "Warning! Executable files may contain viruses or other malicious
code that could harm your system. Use caution when opening this file. Are you
sure you want to continue?"

A "Do not show this warning again" box might be nice too...
Kevin, the wording should really include the filename... past that, your
suggested version does seem better.  Can you incorporate the filename into it as
well?
"Warning! Executable files may contain viruses or other malicious
code that could harm your system. Use caution when opening this file. Are you
sure you want to execute %s?"

Will that work, or should the file name appear towards the beginning of the message?
Could we keep the current behavior as default, and add a (possibly hidden) pref
to enable the Launch button? As I explained in comment 4, even the most naive
user probably understands the implications of double-clicking an executable
file. Not so for warning dialogs; most of those get ignored. We have a
responsibility to protect naive users, although we should give power users the
ability to remove protection they feel is unnecessary for them. I stand by my
conviction that warning dialogs do not provide real security.

What we could really use here is input from some UI experts - not users for whom
"Mozilla would be great if it weren't for this."
When it all comes down to it, nothing provides real security.  Protecting users
from themselves is an impossible job.  I see this interface 'feature' as an
inconvenience and nothing else.

IE's most problematic security problem is that programs can try to run
themselves without the user's input at all.  In Mozilla, users must select to
download files, and thus probably intend to download them to begin with.  If the
file they download contains a virus, this disabled launch button will do nothing
to protect them.  They're still not going to scan the program or check it before
double-clicking.

At least there's a chance that a warning will be read.
Kevin, that looks pretty reasonable (%S, not %s, but other than that)...  Maybe
remove the "use caution" sentence?  It seems superfluous.

Mitch, can you corral a UI expert into commenting?  Are there any still
associated with the project?
I'd suggest:

"Warning! Executable files may contain viruses or other malicious
code that could harm your computer. Use caution when opening this file. Are you
sure you want to launch %s?"

changes: "system" -> "computer" -- "System" is a more technical term.
         "execute" -> "launch"  -- Mac users don't execute applications, they
launch them. "Launch" is more XP-friendly.
I like Simon's changes.

I think the "use caution" just helps to reiterate the potential seriousness of
the message, but it could probably be omitted if you think it sounds better that
way...
If "even the most naive user probably understands the implications of
double-clicking an executable file", then why make the pref hidden? I agree,
warning dialogs do not provide real security. But then, neither does a disabled
Launch button with an enabled Show File Location button right beside it. The
user who probably understands the implications, probably would rather have to
click only once to launch the exe, than to click twice (because of show file
location). 

I like Kevin's suggestion in comment 92 for having a "do not show this again"
check box. At least that way, users who know what they're doing don't have to
click a bunch of times through their file management tool of choice to get to
prefs.js, open their editor, edit, then save, then restart mozilla.

Pardon the bit of sarcasm, but if a UI Expert comes in and tells us this is all
wrong, are we non-experts screwed, again? Ok, seriously though, do we really
need to wait for a UI Expert? I would hope that between the developers and users
already discussing it here, a solution could be worked out.
> then why make the pref hidden

Because a user will not necessarily equate clicking the "Launch" button with
double-clicking a file....
How should isExecutable() work on a Mac? I have one right next to me, so I could try testing this, but I would need a test case...  In Mac Classic, *nothing* downloaded is directly executable - the whole resource fork thing... basically, an application must have the file type code APPL... these codes are stripped from files when traveling the internet, and thus you cannot download a unencoded executable to a Mac running OS 9 or prior.  Some web browsers (semi-recent IE, maybe Netscape 4) are able to decode MacBinary (.bin) and BinHex (.hqx) files on download... Mozilla does not appear to do this, so IsExecutable() should always be false, as the file isn't executable until some other tool (Stuffit Expander, usually) extracts it. If Mozilla did, then it would have to check after it is decoded...  Under Mac OS X, I have no idea what is required to make an item executable or if an executable file is downloadable... File type APPL still works, at least for classic programs. Many (most?) OS X programs are really directories with the .app suffix, but I believe the app suffix alone isn't enough to be considered an application.  Basically, I'd love to test it, but I can't find a test case where isExecutable() should be true. 
Pete, that was my impression from reading the code too... if that's how it is in
real life, then the patch is fine by me (with the revised wording we've come up
with and modulo Mitch's reservations).

Anyone care to update the wording and rediff?  ;)
> Because a user will not necessarily equate clicking the 
> "Launch" button with double-clicking a file....

But it's a preference. how about the preference says "Enable Launch button -
when you click the Launch button, it is as if you are double-clicking the file
you just downloaded"? *something*? you act as if there's no way whatsoever that
any user could ever comprehend the fact that Launch runs the executable. and
there is.
Konqueror is stupid and did not seem to want to submit any newlines, even the
ones I typed myself... For the sanity of the reader, my previous comment is
reproduced below - hopefully sanely. My apologies for the spam.

How should isExecutable() work on a Mac? I have one right next to me, so I could
try testing this, but I would need a test case... 

In Mac Classic, *nothing* downloaded is directly executable - the whole resource
fork thing... basically, an application must have the file type code APPL...
these codes are stripped from files when traveling the internet, and thus you
cannot download a unencoded executable to a Mac running OS 9 or prior.

Some web browsers (semi-recent IE, maybe Netscape 4) are able to decode
MacBinary (.bin) and BinHex (.hqx) files on download... Mozilla does not appear
to do this, so IsExecutable() should always be false, as the file isn't
executable until some other tool (Stuffit Expander, usually) extracts it. If
Mozilla did, then it would have to check after it is decoded...

Under Mac OS X, I have no idea what is required to make an item executable or if
an executable file is downloadable... File type APPL still works, at least for
classic programs. Many (most?) OS X programs are really directories with the
.app suffix, but I believe the app suffix alone isn't enough to be considered an
application.

Basically, I'd love to test it, but I can't find a test case where
isExecutable() should be true.
> In Mac Classic, *nothing* downloaded is directly executable

That's not true. What about a .pl file that opens and runs in MacPerl?
I suggest:

1. Making the Mac isExecutable() bug -- if one exists -- into its own bug. That
   issue should not necessarily block this bug .. and it sounds like it's
   potentially a bigger issue for Macs than it is for other environments.

2. Making the "Launch files without warning" be a normal preference, placed in
   Navigator->Downloads prefs. My reasoning is that (a) having a disabling
   checkbox right on the warning dialog may be too "handy" for naive users who
   tend to ignore such warnings -- this protects such users -- but (b) making
   it a hidden prefs.js preference is probably an unnecessary hassle, since
   only those users with half a clue are likely to even find the preference
   in Navigator->Downloads, understand what it means, or toggle it.  In short,
   naive users don't typically go looking in preferences unless instructed
   to do so.

That being said, I will say that, in my experience, new/naive users actually
DO read those warning dialogs when they pop up, and it is primarily the
experienced users who have reflexively learned to scan and dismiss them.  New
users don't yet understand that many such dialogs are implemented as
cautionary measures, and must read the dialog to understand why it came up.

Glad to see this bug moving towards some kind of resolution.
> > In Mac Classic, *nothing* downloaded is directly executable
> 
> That's not true. What about a .pl file that opens and runs in MacPerl?

Do you really want to go down that road? What about a .doc file that opens in
Microsoft Word and has a macro that does something stupid? That would work on
Mac and Windows, and is the same type of thing - a nonexecutable document being
parsed and run by something else, in this case MacPerl or MS Word.

Anyway, I said directly... the OS cannot execute a MacPerl document. It can
execute MacPerl and ask it to open the document. Same with a MacBinary. The OS
cannot open it. It can ask Stuffit Expander to. Stuffit Expander may then decode
it and decide if it should do something with the resulting file (although I
believe the only thing it'll do is expand further if it's a recognized
compressed file).

Perhaps, as far as open/launch/execute/whatever goes, everything should be
considered executable and display a warning? I'm sure there are hundreds of file
types, while not technically an executable (such as a perl script, or an
applescript, or whatever), could still contain code that gets executed by a
helper app...
Attached patch patch2 (obsolete) — Splinter Review
> Anyone care to update the wording and rediff?  ;)

Done. Wording changed, %s->%S, and some space indentation changed. (I just
edited the old patch; it should work).
Attachment #92811 - Attachment is obsolete: true
Whiteboard: [se-radar]
I think we all agree (incl. me and Mitch and timeless, see comment 51, comment 4
and comment 74, respectively) that a hidden pref is fine, and maybe even an
obscure pref in the prefs dialog (not in the warning dialog!) together with a
warning, as long as the default is off. So, there is not even a reason for you
to flame us that we make your life harder. [censored] If you want this bug to be
fixed, provide a good patch to implement what Mitch said, get it reviewed and
let's be done with it. [censored]

As for the wording of the warning, I suggest to add a note that it would also
put the privacy of the user's files at risk, not just the computer's technical
integrity. I'd suggest to add " or read, modify or transfer all your personal
files on the computer."  to "Executable files may contain viruses or other
malicious code that could harm your system"

> "execute" -> "launch"  -- Mac users don't execute applications, they
> launch them. "Launch" is more XP-friendly.

nod, but don't we also "Launch" data files? (The button doesn't work for me
anyways on Linux.)

> IE's most problematic security problem is that programs can try to run
> themselves without the user's input at all.  In Mozilla, users must select to
> download files, and thus probably intend to download them to begin with.

This is wrong. Mozilla can be made just as well to trigger a download of an
executable file, e.g. using onload. We have seen that in the past in mail, even.
Attachment #112201 - Flags: superreview?
Attachment #112201 - Flags: review?
I don't agree that a hidden preference is fine.  Fixing this and then not
telling anyone about it is pointless.  If it's going to be off by default, it
needs to be in the preferences dialog somewhere.
Please fix this.... it is really annoying
Alias: launchfile
A suggestion mkaply just made is to flag executables as red in the download
manager.... that should make it a little more obvious to people that something
is up with them....

As for the talk of a pref, I'm not sure we want to go in that direction.  There
is certainly no reason to have UI for this pref, imo.  If we _do_ allow
launching EXEs, I think we should enable it by default.  Distributors who do not
like it (eg BenB) can turn the pref off before shipping their products and be ok
with things, I think.  My $0.03 on the policy debate here..
Alias: launchfile → launchexe
Comment on attachment 112201 [details] [diff] [review]
patch2

-> review:? to mstoltz
-> sr:? to bzbarsky
Attachment #112201 - Flags: superreview?(bzbarsky)
Attachment #112201 - Flags: superreview?
Attachment #112201 - Flags: review?(mstoltz)
Attachment #112201 - Flags: review?
Comment on attachment 112201 [details] [diff] [review]
patch2

Looks pretty good to me. ;)

One issue; this seems to be made against an old version of downloadmanager.xul;
the part at the beginning that adds "Shutdown()" is no longer needed (and will
in fact cause the patch not to apply).	I assume this is an artifact of editing
the patch by hand; that hunk should just be removed.
Attachment #112201 - Flags: superreview?(bzbarsky) → superreview+
Making executables red is a good idea - again, much more intuitive than a
dialog. Why don't we do that?

I am not the right person to review this patch. Please find someone who knows
this code better.
Comment on attachment 112201 [details] [diff] [review]
patch2

OK.  Sounds like we have a possible plan of action here:

1)  Mark executables as red (actually, put some sort of class on the rows and
have the theme style them as red).

2) Don't have a warning dialog.

3) Enable the Launch button by default.

4) Have a hidden pref with no UI to disable it for executables.

Anyone up for doing that?  Or seeing major problems with it?
Attachment #112201 - Flags: superreview+
Attachment #112201 - Flags: review?(mstoltz)
This sounds good to me, although that is simply because the proposed plan of
action is suited to my personal use/tastes (i.e. the launch button will work).

From a "pro-security" perspective, however, I think it's fair to say that simply
making the executables red is not going to sufficiently warn the user of the
danger of executing such files (derivative Moz distributions notwithstanding).

Nonetheless, I would be glad to see this implemented as a solution "soon", with
possible new RFEs after the fact to address an additional warning dialog.
Attached patch working-patch (obsolete) — Splinter Review
Ack! I changed the files and created a new patch that works against the current
code. But since we seem to have a new agenda now (no warning msg, red marks,
launch default=on), this doesn't seem to be too useful. Anyway, I'm putting
this up if anyone wants it.
Attachment #112201 - Attachment is obsolete: true
This patch uses a confirmCheck so you can disable the popup.

I haven't updated nsProgressDlg yet.
From my perspective, the warning dialog is a reasonable alternative that at
least solves the irritation factor of the current code. The "red" thing might be
nice but it's not as conventional as a warning dialog.

I say, if the warning dialog is mostly done, lets roll. Good stuff Mike!
Attached patch combination-patch (obsolete) — Splinter Review
Presto! This one combines attachment 112340 [details] [diff] [review] (working-patch) and attachment
112344 [details] [diff] [review] (don't ask again).

This features a "don't ask me again" prompt for both the dialog box and the
download manager.
Attachment #112340 - Attachment is obsolete: true
Attachment #112344 - Attachment is obsolete: true
Comment on attachment 112443 [details] [diff] [review]
combination-patch

Poking people (the same ones..... should I go to somebody else?) for review and
super-review. Sorry if I shouldn't, but we do have some dissenters to the 'red
text' method, and I feel that a warning dialog is best.
Attachment #112443 - Flags: superreview?(bzbarsky)
Attachment #112443 - Flags: review?(mstoltz)
Comment on attachment 112443 [details] [diff] [review]
combination-patch

Talked to mkaply, and I'm thinking that the dialog box is fine if we also do
the red.  That way the first time the red and the dialog get associated
together, hopefully...

>+       const kDontAskAgainPref  = "browser.download.progressDnlgDialog.dontAskForLaunch";
>+        var prefe = Components.classes["@mozilla.org/preferences-service;1"]

"prefe"?  Fix the typo?  ;)

>+                              .getService(Components.interfaces.nsIPrefBranch);
>+        var dontAskAgain = pref.getBoolPref(kDontAskAgainPref); 

You want to catch exceptions if the pref is not set here... (I know it's in
all.js; doesn't matter).

>+	if (file.isExecutable() && !dontAskAgain) {

These are all indented differently.  Please fix that.  Reverse the order of
those checks, so we don't bother with the isExecutable() check if dontAskAgain
is set.

>+          if (checkbox.value != dontAskAgain)
>+            pref.setBoolPref(kDontAskAgainPref, checkbox.value);

You need to catch exceptions on failure to set the pref.

Same comments on the other place you added this code.

Please ask mkaply@us.ibm.com for the review instead of asking mitch, ok?
Attachment #112443 - Flags: superreview?(bzbarsky)
Attachment #112443 - Flags: superreview-
Attachment #112443 - Flags: review?(mstoltz)
Attached patch fixed-combination-patch (obsolete) — Splinter Review
Changed all that needed to be changed - small question though: how do we call
an alert again? Are we allowed to just call alert("msg");? Or do we not need an
alert to inform the user that the savePref failed?

Currently it's just a blank catch statement.
Attachment #112443 - Attachment is obsolete: true
Comment on attachment 112448 [details] [diff] [review]
fixed-combination-patch

No need to let the user know saving the pref failed.

>+          try {
>+              if (checkbox.value != dontAskAgain)
>+                pref.setBoolPref(kDontAskAgainPref, checkbox.value);
>+            } catch (ex) {}

Indentation?  (tabs?  if so, please remove them)

With that, looks great
Attachment #112448 - Flags: superreview+
Attachment #112448 - Flags: review?(mkaply)
not tabs - bad indentation. 

I'll put that in on a next revision - I have no idea where I can call a function
to change the color of the download manager list. I don't know where the font is
named, and I don't know how to call that to change it.
You basically want to put a class on the relevant treerows and then the various
themes can style it as needed.  downloadmanager.xul/downloadmanager.js are where
I would look...
Flags: blocking1.3b? → blocking1.3b-
Comment on attachment 112448 [details] [diff] [review]
fixed-combination-patch

I say we should try to get this in as soon as possible. This would help all the
people venting about this issue, and I recommend we start a new bug about
trying to mark the executables in red (I can't find the CSS files, and I still
don't understand how the download manager adds rows to the download manager,
even after looking at the code for a while, so someone else is free to do
that).

-> approval1.3b ?
-> approval1.0.x ? << I feel that this should not just be isolated to the new
builds, as this is a major functionality increase.
Attachment #112448 - Flags: approval1.3b?
Attachment #112448 - Flags: approval1.0.x?
octal: please don't request approval if the patch does not have review yet
whoop - sorry, didn't realize that. I need some sleep.
Comment on attachment 112448 [details] [diff] [review]
fixed-combination-patch

r=mkaply with indentation changes
Attachment #112448 - Flags: review?(mkaply) → review+
Attached patch indented-fixed-combination-patch (obsolete) — Splinter Review
fixed the indentation issue.

since now I've got reviewer approval once I do this, I'll request approval once
this is in.
Attachment #112448 - Attachment is obsolete: true
Attachment #112554 - Flags: superreview?(bzbarsky)
Attachment #112554 - Flags: review?(mkaply)
Attachment #112554 - Flags: approval1.3b?
Attachment #112554 - Flags: approval1.0.x?
Comment on attachment 112554 [details] [diff] [review]
indented-fixed-combination-patch

+		  pref.setBoolPref(kDontAskAgainPref, checkbox.value);
+		} catch (ex) {}

That's still mis-indented (this is why curly-braces on one-line ifs should be
used  ;) ).

Whoever checks that in should make sure to fix it.

Marking mkaply's r=mkaply too.
Attachment #112554 - Flags: superreview?(bzbarsky)
Attachment #112554 - Flags: superreview+
Attachment #112554 - Flags: review?(mkaply)
Attachment #112554 - Flags: review+
only line 96, to be specific (that code occurs twice):


+             var okToProceed = promptService.confirmCheck(window, title, msg,
dontaskmsg, checkbox);
+             try {
+               if (checkbox.value != dontAskAgain)
+                 pref.setBoolPref(kDontAskAgainPref, checkbox.value);
+               } catch (ex) {}

into:


+             var okToProceed = promptService.confirmCheck(window, title, msg,
dontaskmsg, checkbox);
+             try {
+               if (checkbox.value != dontAskAgain)
+                 pref.setBoolPref(kDontAskAgainPref, checkbox.value);
+             } catch (ex) {}
.

Yep, curly brackets on one-line IF would've helped...
Comment on attachment 112448 [details] [diff] [review]
fixed-combination-patch

usetting requests for obsoleted patch.
Attachment #112448 - Flags: approval1.3b?
Attachment #112448 - Flags: approval1.0.x?
Comment on attachment 112554 [details] [diff] [review]
indented-fixed-combination-patch

Too late for 1.3beta. Feature and featurish work needs to be completed in alpha
or early in beta, not in the last days of the cycle.
Attachment #112554 - Flags: approval1.3b? → approval1.3b-
taking this to keep it on my radar.

If we're going to be waiting till 1.4a to land it, would someone be willing to
investigate the issue of marking relevant rows in the DM with a class that can
then be styled by the theme?  Alternately, we could style the launch button
itself depending on the item that's selected... (is this too weird to fly as
UI?)  The nice thing there is that the progress dialog and download manager can
have a uniform UI then.
Assignee: blaker → bzbarsky
Keywords: helpwanted
Priority: P2 → P1
Summary: Launch file disabled on .EXE (executeable) download → [FIXr]Launch file disabled on .EXE (executeable) download
Target Milestone: Future → mozilla1.4alpha
okee- the roadmap's not making much sense for me here. On the image, it shows
that from 1.3beta - 1.3final, the tree is managed by drivers@mozilla.org.
However, the table at the bottom shows two freeze dates, 22-Jan-2003, and
12-Feb-2003. What's the second freeze date for if the tree is already frozen for
approval-based checkins only?
The first freeze is for shipping 1.3b, then we get feedback on that and perhaps
identify additional bugs that must be fixed with another freeze before shipping
the final 1.3.  The latter freeze is much more frozen.
*** Bug 81703 has been marked as a duplicate of this bug. ***
Blocks: 78106
I just tried to apply this patch.  It does not apply, because

1)  The filenames are all screwy (not relative to the same directory)
2)  The line counts are off (if you hand-edit the patch, update the line counts
    accordingly!  Or just use patchmaker...)

In the process of figuring this out, I found two more problems:

+        var pref = Components.classes["@mozilla.org/preferences-service;1"]
+                              .getService(Components.interfaces.nsIPrefBranch);
+        try {
+          var dontAskAgain = pref.getBoolPref(kDontAskAgainPref); 

The "try" needs to be around the whole pref access, since attempts to get the
pref service could throw.  (This is in both relevant files.)

+          var promptService =
Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
+                                     
.getService(Components.interfaces.nsIPromptService);
+          if (!promptService)
+            break;

this needs a try/catch.  getService will never return null -- it will throw.

Please fix those issues and attach a patch that actually applies to the tree?
Assignee: bzbarsky → octalc0de
Status: NEW → ASSIGNED
Summary: [FIXr]Launch file disabled on .EXE (executeable) download → Launch file disabled on .EXE (executeable) download
Attached patch pm-fixed-combination-patch (obsolete) — Splinter Review
Presto! Patch created. Sorry for the delay, I somehow managed to delete my
local tree.
Attachment #112554 - Attachment is obsolete: true
Attachment #115422 - Flags: superreview?(bzbarsky)
Attachment #115422 - Flags: review?(mkaply)
Attachment #112554 - Flags: approval1.0.x?
Comment on attachment 115422 [details] [diff] [review]
pm-fixed-combination-patch

carring over the r=mkaply.
Attachment #115422 - Flags: superreview?(bzbarsky)
Attachment #115422 - Flags: superreview+
Attachment #115422 - Flags: review?(mkaply)
Attachment #115422 - Flags: review+
Patch checked in on the trunk (for 1.4a).

Please test tomorrow with Windows builds and resolve appropriately....
Error: invalid break
Source File: file:///usr/src/mozilla/dist/bin/components/nsProgressDialog.js
Line: 486, Column: 20
Source Code:
               break;
Errr, are the changes in
mozilla/embedding/components/ui/progressDlg/nsProgressDialog.js really
sufficient? Has someone tested the code?

If I remember correctly, another place must be changed, too, because the launch
button will be disabled otherwise.
I'm assuming the people who put up the patch tested it _very_ carefully (since I
have no way to test it, that's my only option).  If it does not work, I'll back
it out until it has received sufficient testing.  Please let me know ASAP.
Oh, and I just checked in a s/break/return/ for the issue Neil pointed out. 
Apparently people did _not_ test this.....

Can someone confirm whether this works with the progress dialog, please?
I can confirm that it _does not_ work.
http://lxr.mozilla.org/seamonkey/source/embedding/components/ui/progressDlg/nsProgressDialog.js#647
the launch button is only enabled if the file is not executable, and testing
confirms that.

Also, the patch only touched nsProgressDialog.js, doesn't it need to change
nsProgressDlg too?

(now that we enable launching, do we still want to force a filepicker for .exe
files isntead of the helper app dialog?)
> doesn't it need to change nsProgressDlg too

What needs to change in there?

Yes, the other issue needs to be fixed.  If it's not by about 10 hours from now,
I'll be backing out this patch.

> do we still want to force a filepicker for .exe files

Yes.
Note that the problem Neil pointed out likely caused blocker bug 194921.
>What needs to change in there?

oh, right, sorry; that file neesd no change.
patch not working is my mistake - sorry. [attachment is pre-checkin patch]
on rewriting the patch - I forgot to add this segment to the source code:
[attachment also rectifies the return/break issue].


Index: mozilla/embedding/components/ui/progressDlg/nsProgressDialog.js
===================================================================
RCS file:
/cvsroot/mozilla/embedding/components/ui/progressDlg/nsProgressDialog.js,v
retrieving revision 1.19
diff -u -r1.19 nsProgressDialog.js
--- mozilla/embedding/components/ui/progressDlg/nsProgressDialog.js	16 Aug
2002 05:23:
46 -0000      1.19
+++ mozilla/embedding/components/ui/progressDlg/nsProgressDialog.js	25 Feb
2003 21:50:
41 -0000
@@ -609,7 +645,7 @@
		 if ( enableButtons ) {
		     this.enable( "reveal" );
		     try {
-			 if ( this.target && !this.target.isExecutable() ) {
+			 if ( this.target ) {
			     this.enable( "launch" );
			 }
		     } catch(e) {
Attachment #115422 - Attachment is obsolete: true
Comment on attachment 115558 [details] [diff] [review]
updated-patch-to-old-tree

checked in the one part of this patch that was not in yet.....
Attachment #115558 - Flags: superreview+
Since Mozilla 1.3 is branched (i think) and this patch went into the trunk,
should nightlies be picking this up immediately?
Yes.  This was in today's nightlies (note again that it caused blocker bug
194921 which is still not confirmed to be resolved....
I'm experiencing extremely weird behavior when the progress dialog is enabled...

The Download Manager patch worked perfectly, but the progress dialog appears to
disappear every time it finishes a download, which is strange, as the code in
the patch hasn't even been accessed yet.

No errors appear in the javascript console.... !??? (It could just be me, didn't
have much chance to test it yesterday). Anyone can confirm or disprove?
The progress dialog has a checkbox for whether to stay open at the end of a
download.... what option do you have selected?
*** Bug 170372 has been marked as a duplicate of this bug. ***
*** Bug 196298 has been marked as a duplicate of this bug. ***
With Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.4a) Gecko/20030329,
the Launch file button of the progress dialog still doesn't work (works fine the
download manager, though).
This affects Phoenix as well.
*** Bug 200184 has been marked as a duplicate of this bug. ***
> This affects Phoenix as well.

Phoenix as well, or Phoenix _only_?
Phoenix _as well_. It works fine in Mozilla's download manager, but not with the
separate progress dialog (which is _also_ used in Phoenix).
OK.  Details?  Is the button enabled?  If so, does clicking on it just do nothing?
It's enabled. It just does nothing. When I click on the button, it seems to be
pressed, but the exe isn't launched.
 No error in the JS console.
And the dialog does not close when you click the button?

Please enable dump() output in debug preferences and repeat the test, running
with a text console... you should see an error message dumped out to that console...
I can also confirm that this is not working (and actually never worked) in
progress dialog (under Win XP, moz 2003033108). 

Boris, if you could explaine what should I do to give you the data I would do
this. I could not really get where to enable the dump() in the Debug menu...
Edit > Preferences > Debug.  First option under "Miscellaneous".
for me, the "Launch File" button is still greyed out when using both the
progress dialog and the download manager with the released version 1.3:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3) Gecko/20030312

i didn't realize this was supposed to be working, already...
Ahhh... I looked in Debug from main menu. Ok, I'll post the results after a minute.
See comment 145.  It's not supposed to work in 1.3.
I can confirm what is said in Comment 167, can you enable debug in Phoenix?!
Sould I see smth in Javascript console or where? When In javascript, then
nothing appears.
HEre's what I get using 20030329 WinXP

Error ``window is not defined'' [xs] in file
``file:///C:/PROGRA~1/mozilla.org/Mozilla/components/nsProgressDialog.js'', line
497, character 0.
Jason, thank you!

Eugene, this is the _text_ console.  On Unix, this is the terminal window, on
Windows you have to run "mozilla -console".  On Mac, no idea.

Could someone try this patch and let me know whether it helps?	Note that it
may not apply to builds from this morning or earlier; it should be trivial to
apply it by hand, though.
attachment 119139 [details] [diff] [review] fixes the whole problem. I just added it and have had no
problems executing exe files since, the security warning also works just as
expected.

Once committed, the bug should be marked FIXED.
taking so I don't forget about this....
Assignee: octalc0de → bzbarsky
Status: ASSIGNED → NEW
Target Milestone: mozilla1.4alpha → mozilla1.4beta
Attachment #119139 - Flags: superreview?(jaggernaut)
Attachment #119139 - Flags: review?(mkaply)
Summary: Launch file disabled on .EXE (executeable) download → [FIX]Launch file disabled on .EXE (executeable) download
No longer blocks: 106094
Comment on attachment 119139 [details] [diff] [review]
This should fix it

sr=jag
Attachment #119139 - Flags: superreview?(jaggernaut) → superreview+
Attachment #119139 - Flags: review?(mkaply) → review+
Fixed.
Status: NEW → RESOLVED
Closed: 18 years ago17 years ago
Resolution: --- → FIXED
There is a minor glitch on "launch file" and "Show file location" buttons on top
of the Download Manager.
When a download is finished, thjose buttons remain grayed out until the user
click in another row and then back again on the last row (the file just downloaded).
UI interface should update as far as the download is complete.

Paolo - that is not related to this bug. You are describing bug 171890.

this works. marking verified.
Status: RESOLVED → VERIFIED
*** Bug 200184 has been marked as a duplicate of this bug. ***
Duplicate of this bug: 333744
You need to log in before you can comment on or make changes to this bug.