Closed Bug 91969 (launchexe) Opened 23 years ago Closed 22 years ago

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

Categories

(Core :: Security, enhancement, P1)

Product:
Core
Component:
Security
Platform:
x86
Windows 2000
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.4beta

People

(Reporter: jpt.d, Assigned: bzbarsky)

References

(
URL
)

Details

(Whiteboard: [se-radar])

Attachments

(2 files, 13 obsolete files)

updated-patch-to-old-tree
7.10 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review
This should fix it
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: 23 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: nsbeta1 → nsbeta1-
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: 23 years ago → 22 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. ***
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: