User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:0.9.4+) Gecko/20011005 BuildID: 2001100503 It would greatly improve computer security and stability if Mozilla used the default anti-virus scanner to scan for viruses when downloading e-mail or other files from the internet. This goes one extra step to keeping everyone's computer virus-free and it helps keep some people who open every e-mail attachment they get out of trouble. Reproducible: Always Steps to Reproduce: n/a For instance when GetRight is done downloading a file it scans the file for viruses with the default anti-virus scanner, before you can open the file. Doing something similar to this would be great.

sounds like a good idea.

spam: over to File Handling. i have not changed the assigned developer [or the other fields for that matter], so if anyone realizes that a bug should have a more appropriate owner, go ahead and change it. :)

->future/helpwanted

->default owner

restoring target milestone

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

Bug 163357 is a dupe, but wants to have a manual 'scan for Virus' button in the download manager.

possible ways a virus can go to come into a system (win32): 1- being downloaded and run 2- being run as attachement 3- being run as script 4- being installed as additional software (XPI -> what is only a special case of 3) the case 1 is the most frequent one, 2 is second and 3 and 4 are not that frequent in non-MS-programms as i mentioned in bug 163357 - there should be a way to scan incomming downloads easily, and the easiest way is to add a button to Download-Manager/Download-Properties to run the default virus-scan - i suggested that in the Preferences there should be the option to specify them either by specifying the widely-spread ones (McAffee($), AntiVir(free),...) or giving a commandline for the unknown ones --- and this solution maybe done in a soon TargetMilestone, what is sooner than 'Future' :-)

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

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

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

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

especially do this on encrypted mails (see RFE Bug 263761)

are there any broadly used and standardized APIs beyond Microsoft's MAPI, VS API, ESE API (see "Extensible Storage Engine API" or the Exchange virus-scanning API 2.0. http://support.microsoft.com/default.aspx/kb/328841/EN/? and http://www.trendmicro.com/en/products/email/smex/evaluate/vsapi.htm)? How far is ICAP as per http://www.faqs.org/rfcs/rfc3507.html and http://www.i-cap.org/home.html (see http://issues.apache.org/bugzilla/show_bug.cgi?id=27193 for server-side discussion)

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

There's an extension that does this : http://downloadstatusbar.mozdev.org/downscan/

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

Everyone seems to agree with this, So do I. I think it should be in the most obvious place <Options -> Advanced -> Security, and just let you enter your AV scan filename/path/varibles for a silent scan.> And might furthermore ask the user on install if he wants firefox to scan for AV software and auto configure it. up until now, FireFox merely _Warns_the_user_ for downloading from secure sites... It is intolerant in todays world to try make a secure browser without scannind downloaded files, IMHO :)

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

This is on the Firefox 3 PRD; adding appropriate dependencies as well as the PRD line item number into the status whiteboard.

IE7 uses IOfficeAntiVirus Interface (http://msdn2.microsoft.com/en-us/library/ms537369.aspx) as indicated here: http://windowsvistablog.com/blogs/windowsvista/archive/2006/04/21/426003.aspx This should work for windows at least - not sure if this is much use on other platforms (the bug request that is) or even if api's exist for them.

OK, so it's not quite as easy as I originally thought. There is not a stock implementation of this in Windows. Windows defender includes an implementation, and virus scanners might (I still need to research this). So, I guess we need to see if this is interface is defined at runtime, and go from there. So much for the easy way out :/

Hey Shawn, where are we at on this? Looks like we could add a new rich list entry type with a "Virus Scanning..." message, launch this off in a seperate thread and update the list once that's complete?

Jim, that pretty much the idea I was thinking of. I, however, have little to no time to work on this (not to mention that I have no experience using windows API's or MS COM) before M7 or even M8.

Comment on attachment 274514 [details] [diff] [review] Full backend patch...thanks CVS >Index: toolkit/components/downloads/src/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v >retrieving revision 1.13 >diff -u -8 -p -r1.13 Makefile.in >--- toolkit/components/downloads/src/Makefile.in 27 Jun 2007 16:52:15 -0000 1.13 >+++ toolkit/components/downloads/src/Makefile.in 30 Jul 2007 21:28:04 -0000 >@@ -60,18 +60,23 @@ REQUIRES = xpcom \ > intl \ > windowwatcher \ > webbrowserpersist \ > appshell \ > dom \ > embed_base \ > alerts \ > storage \ >+ xulapp \ > $(NULL) > > CPPSRCS = \ > nsDownloadManager.cpp \ > $(NULL) > >+ rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >+ "UPDATE moz_downloads " >+ "SET name = ?1, source = ?2, target = ?3, startTime = ?4, endTime = ?5," >+ "state = ?6 " >+ "WHERE id = ?7"), getter_AddRefs(mUpdateDownloadStatement)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ Just to clarify to other reviewers - I had him move this up here because it needs to be above the observer calls. Shame on me for not catching it when I reviewed that patch... >+ case nsIDownloadManager::DOWNLOAD_FINISHED: >+ { >+ // Master pref to control this function. >+ PRBool showTaskbarAlert = PR_TRUE; >+ if (pref) >+ pref->GetBoolPref(PREF_BDM_SHOWALERTONCOMPLETE, &showTaskbarAlert); >+ >+ if (showTaskbarAlert) { >+ PRInt32 alertInterval = -1; >+ pref->GetIntPref(PREF_BDM_SHOWALERTINTERVAL, &alertInterval); you need to check if pref exists first! >+ >+ PRInt64 alertIntervalUSec = alertInterval * PR_USEC_PER_MSEC; >+ PRInt64 goat = PR_Now() - mStartTime; >+ showTaskbarAlert = goat > alertIntervalUSec; >+ >+ PRInt32 size = mDownloadManager->mCurrentDownloads.Count(); The problem with this of course is that downloads that are being scanned are not in this array. Perhaps we should have an alert for each completed download now? Talk to mconnor/beltzner about this please. > // We need to update mDownloadState before updating the dialog, because > // that will close and call CancelDownload if it was the last open window. >- nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID); > (void)mDownloadManager->FinishDownload(this, >+#ifdef XP_WIN >+ nsIDownloadManager::DOWNLOAD_SCANNING, >+#else > nsIDownloadManager::DOWNLOAD_FINISHED, >+#endif > "dl-done"); This isn't right. dl-done means we are done with the download. Perhaps another stage? That also means you will have to call FinishDownload when you change it to DOWNLOAD_FINISHED yourself. >Index: toolkit/components/downloads/src/nsDownloadScanner.cpp >+ * The Original Code is mozilla.org code. No, it's download manager code >+ * >+ * The Initial Developer of the Original Code is >+ * Netscape Communications Corporation. really?!?! >+ * Portions created by the Initial Developer are Copyright (C) 1998 >+ * the Initial Developer. All Rights Reserved. you wrote this in 1998? :p >+nsresult nsDownloadScanner::Init() return type, newline, function name please >+ hr = CoCreateInstance(CLSID_StdComponentCategoriesMgr, NULL, CLSCTX_INPROC, IID_ICatInformation, getter_AddRefs(catInfo)); nit: 80 chars line wrapping >+ if (hr != S_OK) >+ { nit: ew - { on same line as if please >+ //printf("Could not create category information class\n"); don't comment this out - use NS_WARNING >+ mDownload->AddRef(); Let's try NS_ADDREF(mDownload) instead please. Also, why don't you just use an nsCOMPtr as the member variable? >+ nsCOMPtr<nsIPrefBranch> prefBranch; >+ rv = prefService->GetBranch("browser.download.antivirus.", getter_AddRefs(prefBranch)); Please make this a constant >+ if (prefBranch) >+ rv = prefBranch->GetBoolPref("donotclean", &mIsReadOnlyRequest); Constant >+ >+ nsCOMPtr<nsILocalFile> file; >+ mDownload->GetTargetFile(getter_AddRefs(file)); You should check the return value of this - it can fail. >+ rv = file->GetPath(mPath); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIXULAppInfo> appinfo = >+ do_GetService(XULAPPINFO_SERVICE_CONTRACTID); Please use the two parameter version and check the result >+ nsCAutoString name; >+ appinfo->GetName(name); if you don't care about the return variable please cast to void. >+ rv = uri->GetSpec(origin); >+ NS_ENSURE_SUCCESS(rv, rv); I don't think we generally check this in DM code actually >+ CopyUTF8toUTF16(origin, mOrigin); >+ >+ PRBool isHttp(PR_FALSE), isFtp(PR_FALSE), isHttps(PR_FALSE); >+ nsCOMPtr<nsIURI> innerURI = NS_GetInnermostURI(uri); >+ rv = innerURI->SchemeIs("http", &isHttp); >+ rv = innerURI->SchemeIs("ftp", &isFtp); >+ rv = innerURI->SchemeIs("https", &isHttps); >+ mIsHttpDownload = isHttp || isFtp || isHttps; You assign to rv, but never check it... >+void nsDownloadScanner::Scan::Abort() >+{ >+ mDownload->Release(); >+ >+ // We can only do this internally >+ this->AddRef(); >+ this->Release(); Why, exactly, are you doing this? >+ PRInt16 downloadState = 0; We have a typedef of DownloadState for this.... >+ switch(mStatus) >+ { >+ default: >+ case AVSCAN_FAILED: >+ case AVSCAN_GOOD: >+ case AVSCAN_UGLY: >+ downloadState = nsIDownloadManager::DOWNLOAD_FINISHED; >+ break; >+ case AVSCAN_BAD: >+ downloadState = nsIDownloadManager::DOWNLOAD_BLOCKED; >+ break; >+ } nit: could you put default at the end of it's list, along with that whole block being below the AVSCAN_BAD bits? nit: brace after parens, space after switch, case/default one tab in >+ mDownload->Release(); NS_RELEASE

A couple notes from doing some testing - User friendly names are hard to come by. You can retrieve the user type class via OleRegGetUserType, however, you'll get stuff like this: Vista/Defender: Windows Defender IOfficeAntiVirus implementation XP/Defender: IOfficeAntiVirus implementation XP/Norton: Symantec Norton AntiVirus OfficeAntiVirus Class Since under most circumstances users are only going to have one or two of these installed, rather than go through the trouble of implementing a UI for choosing one, (and warning the user after an upgrade that they need to choose one or risk using an out of date scanner) I'd suggest we consider running files through all available IOfficeAntiVirus scan interfaces and add a simple check box in downloads that enables/disables the behavior. Overall though, I'm not convinced we need to implement this for every download or even attachments. Files downloaded by Firefox aren't exempt from virus scanning under say Norton. Was there a specific security related reason for implementing this? As far as attachments go, Norton supports attachment scanning, although I'm not sure if Thunderbird is safe. My guess though is that they do this through the proxy, so Thunderbird would be included under their security umbrella. Also based on some testing, Norton and Defender update through Windows Update, so the risk of being out of date is low with the bigger vendors.

(In reply to comment #28) > Overall though, I'm not convinced we need to implement this for every download > or even attachments. Files downloaded by Firefox aren't exempt from virus > scanning under say Norton. Was there a specific security related reason for > implementing this? As far as attachments go, Norton supports attachment > scanning, although I'm not sure if Thunderbird is safe. My guess though is that > they do this through the proxy, so Thunderbird would be included under their > security umbrella. It's for feature parity with IE7. They scan every download as well.

> User friendly names are hard to come by Ok, I guess we can't show that in the ui. > I'd suggest we consider running files through all available IOfficeAntiVirus > scan interfaces and add a simple check box in downloads that enables/disables > the behavior I looked into checking on the status of the antivirus software and it seems to be fairly involved (undocumented WMI and registry stuff). Hopefully the virus scans won't take too long (Windows Defender is quick at least). Thunderbird is not necessarily safe either; imap/pop connections over ssl cannot be run through the proxy I think (it would be like a man-in-the-middle scenario if they could). Also, we cannot rely on anti-virus updates to be pushed through windows update; my XP laptop has Norton AV 2005 and it does not get its updates through windows update (I have never seen them there).

Scans don't seem to take long at all. Using a scanning test app with Norton and Defender installed, the first run experienced some "startup time" for Norton but after that it flew through a 2MB file in about a second. This was on a VMWare image as well. An 82 MB installer finished in about 6 seconds.

Comment on attachment 275017 [details] [diff] [review] Rough draft >Index: toolkit/components/downloads/public/nsIDownloadManager.idl you need to rev the uuid > nsresult >-nsDownloadManager::FinishDownload(nsDownload *aDownload, DownloadState aState, >- const char *aTopic) { >+nsDownloadManager::FinishDownload(nsDownload *aDownload) >+{ would CompleteDownload be better name here now? > // We don't want to lose access to the download's member variables > nsRefPtr<nsDownload> kungFuDeathGrip = aDownload; We don't need access to our members anymore after we release this, so we don't need this bit >- NS_WARNING("Could not get downlaod's database schema version!"); >+ NS_WARNING("Could not get download's database schema version!"); I must really like typos... >+nsresult >+nsDownloadManager::SendEvent(nsDownload *aDownload, const char *aTopic) >+{ >+ return mObserverService->NotifyObservers(aDownload, aTopic, nsnull); >+} We don't ever care about the result of this, so why not make it a void function? > nsDownload::SetState(DownloadState aState) now, we'll need kungFuDeathGrip in here in case we call FinishDownload because that could remove our last reference and we'll lose access to our member variables >+ switch (aState) { >+ case nsIDownloadManager::DOWNLOAD_CANCELED: >+ (void)mDownloadManager->FinishDownload(this); hmm, FinishDownload should probably be a void function to now that I think about it... >+#ifdef XP_WIN >+ case nsIDownloadManager::DOWNLOAD_SCANNING: >+ { >+ nsresult rv = mDownloadManager->mScanner->ScanDownload(this); >+ // If we failed, then fall through to 'download finished' >+ if(!NS_FAILED(rv)) >+ break; >+ mDownloadState = aState = nsIDownloadManager::DOWNLOAD_FINISHED; We don't actually care about aState at this point anymore >+ pref->GetIntPref(PREF_BDM_SHOWALERTINTERVAL, &alertInterval); I know you just copied this code, but can you please put |if (pref)| on the line before it? We can crash here... >+ default: >+ break; you don't need a default here then >+ switch (aState) >+ { use mDownloadState, and { after ( please >+ case nsIDownloadManager::DOWNLOAD_DOWNLOADING: >+ (void)mDownloadManager->SendEvent(this, "dl-start"); >+ break; >+ case nsIDownloadManager::DOWNLOAD_FAILED: >+ (void)mDownloadManager->SendEvent(this, "dl-failed"); >+ break; >+ case nsIDownloadManager::DOWNLOAD_CANCELED: >+ (void)mDownloadManager->SendEvent(this, "oncancel"); >+ break; >+ case nsIDownloadManager::DOWNLOAD_PAUSED: >+ (void)mDownloadManager->SendEvent(this, "dl-paused"); >+ break; >+ case nsIDownloadManager::DOWNLOAD_SCANNING: >+ (void)mDownloadManager->SendEvent(this, "dl-scanning"); >+ break; >+ case nsIDownloadManager::DOWNLOAD_FINISHED: >+ (void)mDownloadManager->SendEvent(this, "dl-done"); >+ break; >+ case nsIDownloadManager::DOWNLOAD_BLOCKED: >+ (void)mDownloadManager->SendEvent(this, "dl-blocked"); >+ break; I don't think we need to add observer notifications for dl-paused (there is no corresponding dl-resumed). Also, oncancel should be dispatched to the progress listener, not the observer service which is what SendEvent does. >+ default: >+ break; don't need this again >+ (void)SetState( > #ifdef XP_WIN >+ nsIDownloadManager::DOWNLOAD_SCANNING >+#else >+ nsIDownloadManager::DOWNLOAD_FINISHED >+ ); hrm, for readability's sake, can you just do (void)SetState(state); >+ } > } > > mDownloadManager->NotifyListenersOnStateChange(aWebProgress, aRequest, > aStateFlags, aStatus, this); > > return UpdateDB(); We actually don't need this call anymore, since SetState calls it, and we don't do anything in there anymore. > /** >- * Removes download from "current downloads," updates download state, and >- * notifies observers. >+ * Removes download from "current downloads". This does not notify any >+ * observers Do not need to comment about what it doesn't do >Index: toolkit/components/downloads/src/nsDownloadScanner.cpp >=================================================================== >RCS file: toolkit/components/downloads/src/nsDownloadScanner.cpp >diff -N toolkit/components/downloads/src/nsDownloadScanner.cpp >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ toolkit/components/downloads/src/nsDownloadScanner.cpp 2 Aug 2007 21:37:12 -0000 >@@ -0,0 +1,304 @@ >+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ >+/* vim: se cin sw=2 ts=2 et : */ >+/* ***** BEGIN LICENSE BLOCK ***** >+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1 >+ * >+ * The contents of this file are subject to the Mozilla Public License Version >+ * 1.1 (the "License"); you may not use this file except in compliance with >+ * the License. You may obtain a copy of the License at >+ * http://www.mozilla.org/MPL/ >+ * >+ * Software distributed under the License is distributed on an "AS IS" basis, >+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License >+ * for the specific language governing rights and limitations under the >+ * License. >+ * >+ * The Original Code is mozilla.com code. eh, let's go with download manager code and usually there is the lines like this: * The Initial Developer of the Original Code is * Mozilla Corporation. >+ * >+ * Contributor(s): >+ * Rob Arnold <robarnold@mozilla.com> (Original Author) >+#include "nsIXULAppInfo.h" >+#include "nsXULAppAPI.h" do you need both of those? >+ There are 3 possible outcomes of the virus scan: >+ good => the file is clean >+ bad => the file has a virus >+ ugly => the file had a virus, but it was cleaned either include or use the constant names that you use here please >+ Future enhancements: nit: Possible future enhancements >+ * nit: extra * for bullet >+nsresult nsDownloadScanner::Scan::Start() you didn't fix this nit! You make me sad :( return type\nmethod name You actually do this in several places still >+{ >+ nsresult rv = NS_OK; please declare at first use - this isn't C ;) >+ nsCOMPtr<nsIPrefBranch> pref = >+ do_GetService(NS_PREFSERVICE_CONTRACTID); >+ if (pref) >+ rv = pref->GetBoolPref(PREF_BDA_DONTCLEAN, &mIsReadOnlyRequest); you don't do anything with rv, so don't assign to it >+ mDownload->SetState(downloadState); this has a return type - you dont' care what it is though, so please cast to void >+ if (hr != S_OK) { >+ NS_WARNING("Could not instantiate antivirus scanner"); >+ mStatus = AVSCAN_FAILED; >+ } >+ else >+ { nit: } else {

(In reply to comment #32) > (From update of attachment 275017 [details] [diff] [review]) > >Index: toolkit/components/downloads/public/nsIDownloadManager.idl > you need to rev the uuid Actually, you don't if you're only adding or removing constants on the interface. If you change the values you should, because code using different objects implementing the interface wouldn't know which value to use with which object. If you add, old implementations can't correctly have relied on its value before, and if you remove, new code won't use it and the old implementation will deal as though it hadn't been used (which should have been handled regardless whether the constant's present or not). See also bug 214672 comment 88.

Comment on attachment 275172 [details] [diff] [review] Fixes some minor style issues r=sdwilsh for DM stuff

Comment on attachment 275172 [details] [diff] [review] Fixes some minor style issues Jim, can you take a look at the windows stuff here?

> ULONG nReceived; > clsidEnumerator->Next(1, &mScannerCLSID, &nReceived); > if (nReceived == 0) { > NS_WARNING("No antivirus seems to be installed\n"); I might be missing a mozilla style loop in here someplace :) but afaict, your limiting the scan to Defender. Shouldn't we be saving clsidEnumerator and looping back around to scan using all scanners? We would keep calling Next until it returns something other than S_OK, or nReceived == 0. > hr = CoCreateInstance(mDLScanner->mScannerCLSID, NULL, CLSCTX_ALL, Not sure why we would use CLSCTX_ALL, I guess supporting remote instances is ok with all our threading and the like? Other than that, the com related stuff looks good to me. > #include <msoav.h> Aren't we going to run into trouble compiling this on sdk's less than 6.0? I don't use that sdk by default and had to manually define the interface and structures. We do the same thing with the newer Vista calls in shell services.

(In reply to comment #39) > > ULONG nReceived; > > clsidEnumerator->Next(1, &mScannerCLSID, &nReceived); > > if (nReceived == 0) { > > NS_WARNING("No antivirus seems to be installed\n"); > > I might be missing a mozilla style loop in here someplace :) but afaict, your > limiting the scan to Defender. Shouldn't we be saving clsidEnumerator and > looping back around to scan using all scanners? We would keep calling Next > until it returns something other than S_OK, or nReceived == 0. I was going to wait on implementing the loop until beta1 when we can get beta testers in to get this tested. > > > hr = CoCreateInstance(mDLScanner->mScannerCLSID, NULL, CLSCTX_ALL, > > Not sure why we would use CLSCTX_ALL, I guess supporting remote instances is ok > with all our threading and the like? I don't see why not; I could change it to inproc, but all seemed more flexible. I'm not quite sure how an out-of-proc server works for sharing data. > > #include <msoav.h> > > Aren't we going to run into trouble compiling this on sdk's less than 6.0? I > don't use that sdk by default and had to manually define the interface and > structures. We do the same thing with the newer Vista calls in shell services. > VC8 ships with the header and the interface I'm using should be available since Win95/NT 4/IE 5. That's very odd that it's not defined. Are you using VC7.1?

>VC8 ships with the header and the interface I'm using should be available since >Win95/NT 4/IE 5. That's very odd that it's not defined. Are you using VC7.1? Nope your right, just checked, I was mistaken.

P2 on the PRD, wanted, not blocking

Hopefully the last time I have to unbitrot. Also, cvs didn't eat the scanner part this time.

Comment on attachment 277417 [details] [diff] [review] unbitrotted patch (round 2) > // This has to be done in this exact order to not mess up our invariants > // 1) when the state changed listener is dispatched, it must no longer be > // an active download. > // 2) when the observer is dispatched, the same conditions for 1 must be > // true as well as the state being up to date. > (void)mCurrentDownloads.RemoveObject(aDownload); The comment above this isn't so accurate anymore... I also don't see it moved to anywhere else where you do this... >+#ifdef XP_WIN >+ mScanner = new nsDownloadScanner(); >+ if (!mScanner) >+ return NS_ERROR_OUT_OF_MEMORY; >+ rv = mScanner->Init(); >+ if (NS_FAILED(rv)) { >+ delete mScanner; >+ return rv; >+ } >+#endif If we fail to init, do we really want to not have a download manager, or should we just not scan? > if (NS_FAILED(aStatus)) { > // We don't want to lose access to our member variables > nsRefPtr<nsDownload> kungFuDeathGrip = this; > >- (void)mDownloadManager->FinishDownload(this, >- nsIDownloadManager::DOWNLOAD_FAILED, >- "dl-failed"); >+ SetState(nsIDownloadManager::DOWNLOAD_FAILED); cast to void or check result

This will be useful for testing the return values of different AV providers.

Compiled w/vc8 sp1 - release

Ok, I really don't want to unbitrot this again

Comment on attachment 277567 [details] [diff] [review] unbitrotted patch (round 3) >+ // This has to be done in this exact order to not mess up our invariants >+ // 1) when the state changed listener is dispatched, it must no longer be >+ // an active download. >+ // 2) when the observer is dispatched, the same conditions for 1 must be >+ // true as well as the state being up to date. OK, so 1 isn't quite accurate where it is anymore... When the state changed listener is dispatched, queries to the database and the download manager api should reflect what the nsIDownload object would return. So, if a download is done (finished, canceled, etc.), it should first be removed from the current downloads. We will also have to update the database *before* notifying listeners. At this point, you can safely dispatch to the observers as well. ^^ That comment should be sufficient. Our unit tests should cover this pretty well too, so make sure you run those.

Comment on attachment 277736 [details] [diff] [review] Latest working revision >Index: toolkit/components/downloads/src/nsDownloadManager.cpp >+ case nsIDownloadManager::DOWNLOAD_SCANNING: >+ { >+ nsresult rv = mDownloadManager->mScanner ? mDownloadManager->mScanner->ScanDownload(this) : NS_ERROR_NOT_INITIALIZED; >+ // If we failed, then fall through to 'download finished' >+ if (!NS_FAILED(rv)) NS_SUCCEEDED >Index: toolkit/components/downloads/src/nsDownloadScanner.cpp >+nsDownloadScanner::Scan::Run() >+ DownloadState downloadState = 0; >+ switch (mStatus) { >+ case AVSCAN_BAD: >+ downloadState = nsIDownloadManager::DOWNLOAD_BLOCKED >+ (void)mDownload->SetState(downloadState); This produces the correct UI, but doesn't remove the download. I think you need to add DOWNLOAD_BLOCKED to the switch in SetState() and have it behave the same as DOWNLOAD_CANCELED (call CompleteDownload()). >+nsDownloadScanner::FindCLSID() >+ clsidEnumerator->Next(1, &mScannerCLSID, &nReceived); >+ if (nReceived == 0) { >+ NS_WARNING("No antivirus seems to be installed\n"); This doesn't deserve a warning, just remove this (or make it a comment). r=me with those fixed. Can you file a bug about determining whether we should scan with all scanners, or just the first/last, etc.? Also need to file a bug on the UI to make it more generic, it currently says "blocked by Parental Controls" if your virus scanner causes it to be blocked.

I took the liberty of updating the patch to apply cleanly to current trunk, and addressing my review comments. Can you look over the changes and make sure you're OK with them? The change to SetState is the only real significant one.

Comment on attachment 277783 [details] [diff] [review] updated patch, review comments addressed Those changes seem fine by me

>+ downloadState = nsIDownloadManager::DOWNLOAD_BLOCKED If were going to use this, we should probably change the string displayed in the download manager. "Blocked by Parental Controls" Doesn't really apply here. Or add a new state, like DOWNLOAD_VIRUSDETECTED and create a new string for this state.

mozilla/toolkit/components/downloads/public/nsIDownloadManager.idl 1.19 mozilla/toolkit/components/downloads/src/Makefile.in 1.14 mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp 1.106 mozilla/toolkit/components/downloads/src/nsDownloadManager.h 1.35 mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp 1.1 mozilla/toolkit/components/downloads/src/nsDownloadScanner.h 1.1

Note to all: People download spyware via download manager more often than viruses. Viruses pray on browser exploits to get in, potentially bypassing download manager. The only time I can think of where users are willingly downloading files containing viruses is for warez. Whilst I welcome this bug and think it's a great safety net, we also need to be scanning downloads with any available anti-spyware as spyware is 10x the problem viruses are. I'm not aware of any API that exists on Windows for scanning for spyware. Even on Vista, Windows Defender may be disabled by the presence of Norton / McAffee. Firefox could do two things: a) Look for the installation of common, well known AS programs, and configure automatically to use one b) And provide UI in prefs to select the scanning executable, as a fallback

> Windows Defender may be disabled by the presence of Norton / McAffee. In my testing with Norton, this was not the case. On an XP image, with Norton installed, Defender (the primary purpose of which is to scan for spyware) was still available as one of the scanners listed through the interface.

I'm calling this fixed; collectively, we've done a plethora of testing against the vendors, here: https://bugzilla.mozilla.org/show_bug.cgi?id=396553#c16 In addition, bugs against its continual-scanning state have been filed, and hopefully fixed (see bug 401582; if that's truly not fixed, please do reopen and comment with your findings). Finally, I filed bug 408153 a little while ago, so that we could investigate implementing an API that would work with those vendors' products--usually the 2008 editions--that don't implement IOfficeAntiVirus.

How does everyone feel about splitting the scanner out for 3.1 as a service where everyone can get access to it? Similar to the service interface for parental controls where the interface is public, but fails if a native impl isn't available. I think there's a bug filed somplace for something like this but I currently can find it.

(In reply to comment #59) > How does everyone feel about splitting the scanner out for 3.1 as a service > where everyone can get access to it? Similar to the service interface for > parental controls where the interface is public, but fails if a native impl > isn't available. I think there's a bug filed somplace for something like this > but I currently can find it. We actually need to do that so exthandler can use it. Right now if you click "Open" or "Open With..." on a download, it opens it before the scan is complete.