416683 - binary downloads are deleted on completion when "Launch applications and unsafe files" is disabled

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect, P1)

Description

[Simon Bünzli]

Step to reproduce:
1. Download a Firefox installer (or any other file triggering the reduced save file dialog)
2. Wait for the download to complete

Expected result:
The download sits on the desktop.

Actual result:
When the download completes, both the .exe and the .exe.part files are deleted.

Regression between 2008020804 and 2008020904.

Comment 1

[Simon Bünzli]

Disabling browser.download.manager.scanWhenDone fixes the issue -> bug 408153?

Comment 2

[Simon Bünzli]

In case it matters: This is on XP SP2 with currently no AV software installed.

Comment 3

[Masatoshi Kimura [:emk]]

(In reply to comment #2)
> currently no AV software installed.
Have you ever installed any AV softwares or Windows Defender before?

Comment 4

[Simon Bünzli]

(In reply to comment #3)
> Have you ever installed any AV softwares or Windows Defender before?
IIRC I had one of the first versions of Microsoft AntiSpyware installed at one time, and the free Avira AntiVir running until about a year ago. Both were uninstalled through their own uninstallers.

Any possible left-overs I could check for?

Comment 5

[Jim Mathies [:jimm]]

Hey Simon,

There's a test program here - 

https://bugzilla.mozilla.org/show_bug.cgi?id=415005

that you can run on an exe - it'll mimic what fx does. Would you mind testing that out and seeing if you can reproduce the behavior?

Comment 6

[Simon Bünzli]

Running ScannerTimer on a copy of itself, the copy indeed vanishes!

And in case it matters, here's the output from that run:
> IAttachmentExecute:
> 47.788755 62.233887 0.006146
> 0.037994 0.659302 0.001956
> 0.018997 0.586946 0.001956
> 0.023467 0.582476 0.001956
> 0.016762 0.587505 0.001676
> 0.017041 0.586946 0.001956
> 0.017041 0.579403 0.001956
> 0.016762 0.586387 0.001956
> 0.016762 0.587784 0.001956
> 0.017041 0.628292 0.001956
> 0.017600 0.580241 0.001956
> 0.016762 0.586108 0.001956
> 0.017041 0.585549 0.001956
> 0.016762 0.579403 0.001956
> 0.017041 0.593092 0.001956
> 0.017041 0.739759 0.001956
> 0.020673 0.588343 0.001956
> 0.017321 0.620749 0.001956
> 0.017879 0.586387 0.001676
> 0.017321 0.584711 0.001676

Comment 7

[Masatoshi Kimura [:emk]]

What happens when you download a file using Internet Explorer?
(Please make sure [Internet Options] - [Advanved] - [Check for signatures on download programs] is checked before test)

Comment 8

[Simon Bünzli]

(In reply to comment #7)
IE won't even let me start the download...

Turns out that I have restricted IE's Internet zone to prevent "unsafe" files to be launched (Internet Options -> Security -> Custom Level... -> Miscellaneous -> Launch applications and unsafe files) which is the default setting at the highest security level.

This somehow explains the current behavior, makes it quite unexpected nonetheless (we should thus IMO either not allow "unsafe" downloads to be started or override IE's setting to prompt when the file is launched).

Comment 9

[Masatoshi Kimura [:emk]]

You're right. I have reproduced the issue.

Comment 10

[Shawn Wilsher :sdwilsh]

Jim, Rob, or Masatoshi - do you know if we can determine if it's an allowable download right away or not?

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Not sure that this blocks, since I'm pretty sure this is an uncommon setting and by the letter of the law we are respecting the user's request, but it's damned poor UE.

Comment 12

[Julian Hall]

I believe this is the default setting on fresh installs of Windows Server 2003.

Comment 13

[Rob Arnold [:robarnold]]

(In reply to comment #10)
> Jim, Rob, or Masatoshi - do you know if we can determine if it's an allowable
> download right away or not?
> 

IAttachmentExecute has a method called CheckPolicy which tells us if we should download the file, prompt the user to execute/save it or disable the download. From my extremely limited testing, this is returning the value of the pref in comment #8. We can call IAttachmentExecute::CheckPolicy and avoid calling Save. In this case, should we fall back to the enumerated virus scanners so that some scanning is still done?

Comment 14

[Shawn Wilsher :sdwilsh]

I'm inclined to say that if the OS policy is to not download the file, then we should not download the file.

Comment 15

[Jim]

The setting is an IE setting for "Launching applications and unsafe files". If this is set to disabled, the save of the file to disk will not succeed. It might not be obvious to a Firefox user that IE settings control the way Firefox behaves.

Comment 16

[Shawn Wilsher :sdwilsh]

It's actually a system setting that IE provides a UI for.  Perhaps Firefox should provide this UI as well.

Comment 18

[Shawn Wilsher :sdwilsh]

Re-nominating for blocking.  This may be more common that originally thought.  We should be checking if a download is allowed in nsIDownloadManager::addDownload and mark the download as blocked if it is in fact blocked.

Comment 19

[Peter van der Woude [:Peter6]]

We at least have to give the user a clue (warning) why the file is (going to be)removed and maybe an option to override this per download.

Comment 20

[Shawn Wilsher :sdwilsh]

right, which is exactly what comment 18 said we should do

Comment 21

[Skullsplitter]

Hi everyone.  I had to mention something about the bug under discussion.  On my machine (running xpsp2), the bug does not show up in FFb3.  In all the time I've been trying b3 and 4, nothing in the Internet Options settings have changed.  I first saw the problem in one of the nightlies from (maybe) January.  So I went back to FF2 at that point.  When I'd gotten around to b3, the problem was gone.  Came back in b4.   As I said, I hadn't changed anything in XP's Internet Options in all that time.     I *did* follow the suggestion mentioned further up, to do with IE Security settings.  But it changed nothing in FFb4... downloads still get deleted just after the download is finished.

Thanks for anything that can be done about this.  I'm really enjoying Firefox. B3 is really nice, and B4 seems faster than B3.  As far as I've been able to tell, Bug 416683 is the only problem that b4 exhibits in my case.

Cheers.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

Shawn: can we get a strings-portion patch by tonight? If so, I can try and sweet talk the l10n crew.

Comment 23

[OwenC]

Similar experiene to that of Skullsplitter (i raised bug 422469 btw) - fine in b3, broken in b4.

On my Vista machine "Launching applications and unsafe files" has no effect as the behaviour is displayed independent of this setting. I've since told IE to return its Security settings to the factory defaults in case I had something weird set up by mistake but no change in bug - behaviour still displayed.

IE7 will download files fine off the same IE settings.

Comment 24

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> Shawn: can we get a strings-portion patch by tonight? If so, I can try and
> sweet talk the l10n crew.
I doubt it (from me at least).  I'm not even sure if we can change this system setting with our UI.  Jim or Rob care to chime in here?

Comment 25

[Jim Mathies [:jimm]]

This is one of the global securty zone settings we now get tied into since we started using IAE. The approach in comment 13 seems to make the best sense.

Looks like you can detect it through the registry. Although it appears there is one copy of the setting for each security zone -

http://blogs.technet.com/askperf/archive/2007/11/27/managing-the-launching-applications-and-unsafe-files-setting.aspx

Comment 26

[Jim Mathies [:jimm]]

> I'm not even sure if we can change this system setting with our UI. 

These options are also available through control panel under Internet Options so users do not have to launch IE to get to them. If we wanted to provoid UI, I think there's a way to bring up the Internet Options panel up.  We could also try messing with the registry directly.  

Comment 27

[Shawn Wilsher :sdwilsh]

What string(s) would we need to try and launch it?  Note: this is a super P1-task since this is a string past the freeze and beltzner wants it ASAP.

Comment 28

[Jim Mathies [:jimm]]

Correct me if I'm wrong but I believe we have two dm blocked strings, one for parental controls, and one for virus software. Maybe add a 3rd related to "zone policy restrictions" or something like that and use the check policy call Rob suggested? 

Adding some sort of UI option to bring up the options panel - I'm not sure where that would go, but I found the way to do it here -

http://msdn2.microsoft.com/en-us/library/aa753680(VS.85).aspx

Comment 29

[Shawn Wilsher :sdwilsh]

Probably in the options panel somewhere, which implies that we need to have some xpcom component to wrap the call.

I have classes the rest of the day, and then work so I won't be around to help drive this.  If you really need to get in touch with me, call me.

Comment 30

[Skullsplitter]

The problem seems to be fixed at my end!  I think that something I installed may have been f*ing with my Zones settings in the Registry (xpsp2 here).  So I may have to stand corrected.

I changed both of the following manually:

HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Internet Settings\Zones\3

HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Internet Settings\Zones\3

To the right, there should be DWORDs named "1806".  One or both may need to be reset to 0 (zero).  For sure, they should not be at 3 (three), which is where mine were at.   An install of Zone Alarm or AVG may have messed with these.

This link is what helped me resolve the problem:

http://blogs.technet.com/askperf/archive/2007/11/27/managing-the-launching-applications-and-unsafe-files-setting.aspx

So FF3b4 has stopped deleting downloads on my machine.  Onward and upward! :)

Comment 31

[Jim Mathies [:jimm]]

Attachment: string change

the string change clipped out of the main patch.

Comment 32

[Jim Mathies [:jimm]]

Attachment: check policy patch

Ok, here's a first rev of this. Added the policy check right when a download starts, cancel it if it's destined to be deleted after the download, and identify affected downloads with a new string and the blocked icon.

Comment 33

[Jim Mathies [:jimm]]

Also FYI, launching the internet options panel programatically will bring up the same panel you get in IE, which has a ton of IE specific settings. I think that'll be confusing for average users. My feeling is we either create our own UI for specific settings or we simply don't mess with it and point users to the control panel version. 

Comment 34

[Shawn Wilsher :sdwilsh]

You might be better off having another toolkit peer and Edward look at this.  I'm not sure when I can get to it (grrr school)

Comment 35

[Jim Mathies [:jimm]]

Comment on attachment 309252 [details] [diff] [review]
check policy patch

np man. updated.

Comment 36

[Jim Mathies [:jimm]]

Comment on attachment 309252 [details] [diff] [review]
check policy patch

np man. updated.

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 309252 [details] [diff] [review]
check policy patch

I think you should make the constant names more descriptive, and possibly add a comment in the idl indicating what they are as well.

Comment 38

[Shawn Wilsher :sdwilsh]

Comment on attachment 309251 [details] [diff] [review]
string change

any word yet from the l10n folks Mike?

Comment 40

[Ed Lee :Mardak]

Comment on attachment 309252 [details] [diff] [review]
check policy patch

r=Mardak for the non-download scanner bits.

>+++ toolkit/components/downloads/public/nsIDownloadManager.idl	13 Mar 2008 22:20:31 -0000
>@@ -55,19 +55,20 @@ interface nsIDownloadManager : nsISuppor
>-  const short DOWNLOAD_BLOCKED          = 6;
>+  const short DOWNLOAD_BLOCKED_PC       = 6;
>   const short DOWNLOAD_SCANNING         = 7;
>   const short DOWNLOAD_DIRTY            = 8;
>+  const short DOWNLOAD_BLOCKED_SP       = 9;
Took me a bit to figure out why PC and SP.. (I think I figured them out..) parental control and security policy? How about..
>+  const short DOWNLOAD_BLOCKED_PARENTAL = 6;
>+  const short DOWNLOAD_BLOCKED_POLICY   = 9;
(Didn't help that the new thing is "Poli Cee" :p)

>+++ toolkit/components/downloads/src/nsDownloadManager.cpp	13 Mar 2008 22:20:32 -0000
>@@ -1356,25 +1356,39 @@ nsDownloadManager::AddDownload(DownloadT
>+  PRUint32 startState = nsIDownloadManager::DOWNLOAD_QUEUED;
Type is DownloadState
>+#if defined(XP_WIN) && !defined(__MINGW32__) && !defined(WINCE)
>+  if (mScanner) {
>+    AVCheckPolicyState res = mScanner->CheckPolicy(source, target);
>+    if (res == AVPOLICY_BLOCKED) {
>+      // This download will get deleted during a call to IAE's Save,
>+      // so go ahead and mark it as blocked and avoid the download.
>+      if (aCancelable)
>+        aCancelable->Cancel(NS_BINDING_ABORTED);
Use the nsDownload::Cancel() method which will check null and also break the mCancelable cycle.

>+++ toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties	13 Mar 2008 22:20:35 -0000
>@@ -76,16 +76,17 @@ doneScheme=#1 resource
> # LOCALIZATION NOTE (stateBlocked): 'Parental Controls' should be capitalized
> stateBlocked=Blocked by Parental Controls
> stateDirty=Blocked: Download may contain a virus or spyware
>+stateBlockedPolicy=Blocked by Internet Zone Policy
We might need to match the localization note like 'Parental Controls' for 'Internet Zone Policy' (where does it come from exactly?

>+++ toolkit/mozapps/downloads/content/downloads.css	13 Mar 2008 22:20:35 -0000
>@@ -19,17 +19,21 @@ richlistitem[type="download"][state="3"]
> richlistitem[type="download"][state="6"] {
>-  -moz-binding: url('chrome://mozapps/content/downloads/download.xml#download-blocked');
>+  -moz-binding: url('chrome://mozapps/content/downloads/download.xml#download-blocked-pc');
>+}
>+
>+richlistitem[type="download"][state="9"] {
>+  -moz-binding: url('chrome://mozapps/content/downloads/download.xml#download-blocked-sp');
Could you move the state=9 one after 8 to keep the ordering?

>+++ toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js	13 Mar 2008 22:20:36 -0000
>@@ -101,17 +101,17 @@ const DownloadData = [
>-    state: Ci.nsIDownloadManager.DOWNLOAD_BLOCKED,
>+    state: Ci.nsIDownloadManager.DOWNLOAD_BLOCKED_PC,
Could you add another test for the policy state?

Comment 41

[Ed Lee :Mardak]

(In reply to comment #40)
> >+++ toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js	13 Mar 2008 22:20:36 -0000
> >@@ -101,17 +101,17 @@ const DownloadData = [
> >-    state: Ci.nsIDownloadManager.DOWNLOAD_BLOCKED,
> >+    state: Ci.nsIDownloadManager.DOWNLOAD_BLOCKED_PC,
> Could you add another test for the policy state?
Just to be clear, you can just reuse the browser_basic_functionality file and add another row in the DownloadData array that is basically copy/paste except the state.

Comment 42

[Jim Mathies [:jimm]]

I'll address the details with a new patch tomorrow.

>We might need to match the localization note like 'Parental Controls' for
>'Internet Zone Policy' (where does it come from exactly?

One thing I'd like to bring up for general discussion is the string here, "Blocked by Internet Zone Policy". It fits with what's happening here, but I'm not convinced it's the most user friendly string. I'd welcome other suggestions.

Comment 43

[Ed Lee :Mardak]

Jim: Could you check if this fixes bug 420230? There's a data uri in the URL field of that bug, so load it and ctrl-s save on windows and see if it the file disappears when it finishes.

Comment 44

[Rob Arnold [:robarnold]]

Comment on attachment 309252 [details] [diff] [review]
check policy patch


>   const short DOWNLOAD_NOTSTARTED       = -1;
>   const short DOWNLOAD_DOWNLOADING      = 0;
>   const short DOWNLOAD_FINISHED         = 1;
>   const short DOWNLOAD_FAILED           = 2;
>   const short DOWNLOAD_CANCELED         = 3;
>   const short DOWNLOAD_PAUSED           = 4;
>   const short DOWNLOAD_QUEUED           = 5;
>-  const short DOWNLOAD_BLOCKED          = 6;
>+  const short DOWNLOAD_BLOCKED_PC       = 6;
>   const short DOWNLOAD_SCANNING         = 7;
>   const short DOWNLOAD_DIRTY            = 8;
>+  const short DOWNLOAD_BLOCKED_SP       = 9;

Can we swizzle the numbers so that the blocking states have adjacent numbers?

>-    case nsIDownloadManager::DOWNLOAD_BLOCKED:
>+    case nsIDownloadManager::DOWNLOAD_BLOCKED_PC:
>+    case nsIDownloadManager::DOWNLOAD_BLOCKED_SP:
>       mDownloadManager->SendEvent(this, "dl-blocked");

Should we think about having two blocking events (or three - one for when either happens)?

>+  (void)ae->SetClientGuid(GUID_MozillaVirusScannerPromptGeneric);
>+  (void)ae->SetSource(NS_ConvertUTF8toUTF16(aSource).get());
>+  if (!aTarget.IsEmpty())
>+    (void)ae->SetLocalPath(NS_ConvertUTF8toUTF16(aTarget).get());

What happens if the NS_ConvertUTF8toUTF16 calls fail? As long as we don't crash or get garbage, I don't really care.

>+  // Any failure means the file download/exec will be blocked by the system.
>+  // S_OK or S_FALSE imply it's ok.
>+  hr = ae->CheckPolicy();
>+
>+  if (FAILED(hr))
>+    return AVPOLICY_BLOCKED;
>+
>+  if (hr == S_FALSE)
>+    return AVPOLICY_PROMPT;
>+
>+  return AVPOLICY_DOWNLOAD;
>+}

The return value should be checked according to the spec and comment (i.e. can we check for S_OK/S_FALSE explicitly and block for anything else?)

>+enum AVCheckPolicyState
>+{
>+  AVPOLICY_DOWNLOAD,
>+  AVPOLICY_PROMPT,
>+  AVPOLICY_BLOCKED
>+};
>+
>...
>+  AVCheckPolicyState CheckPolicy(const nsACString &aSource, const nsACString &aTarget);

These really should not be part of the download scanner, but I don't know a better place to put them.

Comment 45

[Shawn Wilsher :sdwilsh]

(In reply to comment #44)
> Can we swizzle the numbers so that the blocking states have adjacent numbers?
I tried doing that once, and it can break people who have older builds sadly
> Should we think about having two blocking events (or three - one for when
> either happens)?
We give them the nsIDownload object, so if they need more info they can get the state property.

> What happens if the NS_ConvertUTF8toUTF16 calls fail? As long as we don't crash
> or get garbage, I don't really care.
I don't think it can

Comment 46

[Benjamin Smedberg]

Comment on attachment 309252 [details] [diff] [review]
check policy patch

>Index: toolkit/components/downloads/public/nsIDownloadManager.idl

>   // Download States
>   const short DOWNLOAD_NOTSTARTED       = -1;
>   const short DOWNLOAD_DOWNLOADING      = 0;
>   const short DOWNLOAD_FINISHED         = 1;
>   const short DOWNLOAD_FAILED           = 2;
>   const short DOWNLOAD_CANCELED         = 3;
>   const short DOWNLOAD_PAUSED           = 4;
>   const short DOWNLOAD_QUEUED           = 5;
>-  const short DOWNLOAD_BLOCKED          = 6;
>+  const short DOWNLOAD_BLOCKED_PC       = 6;
>   const short DOWNLOAD_SCANNING         = 7;
>   const short DOWNLOAD_DIRTY            = 8;
>+  const short DOWNLOAD_BLOCKED_SP       = 9;

These desperately need comments explaining them, even if you change the name as Mardak suggested.

Also, this desperately needs tests. I realize than an automated test might be hard to manage, because it depends on Windows policy state... but is there perhaps a standard "blocked" download URL we could test against? In any case, this needs a litmus test.

Comment 47

[Jim Mathies [:jimm]]

Is everybody cool with this style of commenting and the comments I've used? I'm not going to remix the values, re comment 45.

/**
* Download types:
*
* DOWNLOAD_TYPE_DOWNLOAD
*   Generic file download.
*/
const short DOWNLOAD_TYPE_DOWNLOAD    = 0;

/**
* Download States
*
* DOWNLOAD_NOTSTARTED
*   Download object created but not initialized.
* DOWNLOAD_QUEUED
*   Download object is initialized but data has yet to be
*   received.
* DOWNLOAD_DOWNLOADING
*   Download is currently active.
* DOWNLOAD_FINISHED
*   Download is complete.
* DOWNLOAD_FAILED
*   Download failed due to networking error.
* DOWNLOAD_CANCELED
*   Download was canceled by the user.
* DOWNLOAD_PAUSED
*   Download is paused by the user.
* DOWNLOAD_BLOCKED_PARENTAL
*   Download was blocked by parental controls proxies.
* DOWNLOAD_BLOCKED_POLICY
*   Download was blocked by zone policy settings (see bug #416683). The
*   The target file will most likely no longer exist.
* DOWNLOAD_SCANNING
*   Download is being scanned by virus scanners.
* DOWNLOAD_DIRTY
*   A virus was detected in the download. The target file will most likely
*   no longer exist.
*/
const short DOWNLOAD_NOTSTARTED       = -1;
const short DOWNLOAD_DOWNLOADING      = 0;
const short DOWNLOAD_FINISHED         = 1;
const short DOWNLOAD_FAILED           = 2;
const short DOWNLOAD_CANCELED         = 3;
const short DOWNLOAD_PAUSED           = 4;
const short DOWNLOAD_QUEUED           = 5;
const short DOWNLOAD_BLOCKED_PARENTAL = 6;
const short DOWNLOAD_SCANNING         = 7;
const short DOWNLOAD_DIRTY            = 8;
const short DOWNLOAD_BLOCKED_POLICY   = 9;

Comment 48

[Jim Mathies [:jimm]]

(Scratch the ending colon on "Download types:"...)

Comment 49

[Ed Lee :Mardak]

Probably keep it inline and in the order of the numbers. Personally, when I look at the idl, I'm trying to find what number is which state, so it's easier if it's ordered that way. Probaly only need to mention download state for the first state and just describe the download for the others. Also, it's /**\n<space>*<sp>Comment\n<sp>*/

(In reply to comment #47)
> /**
>  * Download state for uninitialized download object.
>  */
> const short DOWNLOAD_NOTSTARTED       = -1;
>
> /**
>  * Download is transferring data.
>  */
> const short DOWNLOAD_DOWNLOADING      = 0;
>
> /**
>  * Download successfully transferred to target destination.
>  */
> const short DOWNLOAD_FINISHED         = 1;
..

Elsewhere in the code.. "active" is notstarted, downloading, paused, queued, scanning.

Comment 50

[Shawn Wilsher :sdwilsh]

(In reply to comment #49)
> (In reply to comment #47)
> > /**
> >  * Download state for uninitialized download object.
> >  */
> > const short DOWNLOAD_NOTSTARTED       = -1;
Yes, I like this.  Not sure if we should bother keeping the numbers lined up at this point fyi.

Comment 51

[Jim Mathies [:jimm]]

>Jim: Could you check if this fixes bug 420230? There's a data uri in the URL
>field of that bug, so load it and ctrl-s save on windows and see if it the file
>disappears when it finishes.

Yes, the local machine zone apparently prevents data url sources. Other types
of local content aren't affected. For example a png dragged into the browser
then saved out to a different directory will work just fine.

I've been looking at the local security zone default settings here - 
http://msdn2.microsoft.com/en-us/library/ms537183.aspx#local
but none of these seem to match up with this type of content.

I suppose we could create an exception list for certain schemes since we
obviously trust Fx's own security measures on content like this. 

Comment 52

[Jim Mathies [:jimm]]

A little more info - I'm not sure it's a specific security zone restriction. The result on CheckPolicy is E_INVALIDARG for data uris. The docs on CP state though that anything that fails should be blocked. 

Comment 53

[Jim Mathies [:jimm]]

Attachment: string change

Updated to "Security Zone Policy" and added a localization note.

Comment 54

[Jim Mathies [:jimm]]

From bug 420230:
>This seems to be windows only. I wonder if it's related to bug 416683. Because
>when I toggle browser.download.manager.scanWhenDone to false, I can save okay
>on windows.

I should also point out that with this patch, using scanWhenDone to override this will stop working, since the CheckPolicy stuff isn't tied into that pref (and shouldn't be imho because this has nothing to do with virus scanning.)

Comment 55

[Jim Mathies [:jimm]]

Attachment: check policy patch v.2

Comment 56

[Jim Mathies [:jimm]]

Misc. comments:

>>+
>>+  if (hr == S_FALSE)
>>+    return AVPOLICY_PROMPT;
>>+
>>+  return AVPOLICY_DOWNLOAD;
>>+}
>
>The return value should be checked according to the spec and comment (i.e. can
>we check for S_OK/S_FALSE explicitly and block for anything else?)

(yes) Updated.

>>+  AVCheckPolicyState CheckPolicy(const nsACString &aSource, const nsACString >&aTarget);
>
>These really should not be part of the download scanner, but I don't know a
>better place to put them. 

I agree, however I also like having all the AE code stored in the same place. I think I'd rather rename the scanner to something more generic than create a new source file or ifdef this in someplace. (or alternatively, leave as is.)  

>What happens if the NS_ConvertUTF8toUTF16 calls fail? As long as we don't crash
>or get garbage, I don't really care.

I'd guess yes in cases where you run out of memory. In which case the CheckPolicy call would fail and the download would be blocked. I'll confirm a null here fails gracefully.

Comment 57

[Rob Arnold [:robarnold]]

Comment on attachment 310110 [details] [diff] [review]
check policy patch v.2

Looks good to me

Comment 58

[Mike Beltzner [:beltzner, not reading bugmail]]

Hrm. We can't take the string change at this late date without busting locales entirely. Is there any way to do this patch such that it uses a different string (or just doesn't use a string at all) and then add the string for RC1?

Alternatively, do we think this bug needs beta exposure, or safe to keep for RC1 (ie: are there tests we can run to make sure it's fixed, and are there tests included to make sure we don't regress anything else?)

Comment 59

[Shawn Wilsher :sdwilsh]

We could probably use one of the other blocked strings for this beta, and fix it after the beta if need be.

I do think this should probably get beta exposure.

Comment 60

[Ed Lee :Mardak]

Comment on attachment 310110 [details] [diff] [review]
check policy patch v.2

For Beta 5, instead of this..
>-  stateBlocked: "stateBlocked",
>+  stateBlockedParentalControls: "stateBlocked",
>+  stateBlockedPolicy: "stateBlockedPolicy",

we could easily do..
>-  stateBlocked: "stateBlocked",
>+  stateBlockedParentalControls: "stateBlocked",
>+  stateBlockedPolicy: "stateBlocked",

Good thing I added that level of indirection! ;)

Comment 61

[Ed Lee :Mardak]

Actually, maybe want to use stateDirty instead of stateBlocked for the policy.

stateBlocked=Blocked by Parental Controls
vs
stateDirty=Blocked: Download may contain a virus or spyware

or less lying would be stateFailed.. kinda.

Comment 62

[Shawn Wilsher :sdwilsh]

Let's use dirty for the string in beta5 - we can file a follow-up to get the right string fixed right away after beta5.

Jim - can you get a new patch that does that please?

Comment 63

[Jim Mathies [:jimm]]

yep, sorry I missed the discussion my cable internet has been out. I'll post here in a bit.

Comment 64

[Jim Mathies [:jimm]]

Attachment: check policy patch v.3

- temporarily removed the new string from the properties file and switched to stateDirty string.

Comment 65

[Shawn Wilsher :sdwilsh]

Comment on attachment 310541 [details] [diff] [review]
check policy patch v.3

you already have sr - just need to address his comments and this is good to land.

> /**
>  * Represents a download object.
>  *
>  * @note This object is no longer updated once it enters a completed state.
>- *       Completed states are the following:
>- *       nsIDownloadManager::DOWNLOAD_FINISHED
>- *       nsIDownloadManager::DOWNLOAD_FAILED
>- *       nsIDownloadManager::DOWNLOAD_CANCELED
Please don't remove this - it's important.

Comment 66

[Reed Loden [:reed]]

Comment on attachment 310541 [details] [diff] [review]
check policy patch v.3

(In reply to comment #65)
> (From update of attachment 310541 [details] [diff] [review])
> you already have sr - just need to address his comments and this is good to
> land.

Uh, no, he doesn't have sr. I see sr-.

Comment 67

[Shawn Wilsher :sdwilsh]

Whoops - wrong bug.  This wasn't the parental controls one...

Comment 68

[Jim Mathies [:jimm]]

> Please don't remove this - it's important. 

It wasn't up to date though. I added "(completed)" commenting on the states in the idl which seemed more organized. Shouldn't that suffice? 

Comment 69

[Shawn Wilsher :sdwilsh]

(In reply to comment #68)
> It wasn't up to date though. I added "(completed)" commenting on the states in
> the idl which seemed more organized. Shouldn't that suffice? 
Then please update it accordingly.  People may not look at the nsIDownloadManager interface to see what goes on with an nsIDownload.

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310098 [details] [diff] [review]
string change

>+# LOCALIZATION NOTE (stateBlockedPolicy): 'Security Zone Policy' should be capitalized
>+stateBlockedPolicy=Blocked by Security Zone Policy

gets my ui-r for post-beta5 with the following change:

stateBlockedPolicy=This download has been blocked by your Security Zone Policy

Comment 71

[Jim Mathies [:jimm]]

(this bug shouldn't be closed after the first patch lands.) 

Comment 72

[Jim Mathies [:jimm]]

Attachment: check policy patch v.3

updated commenting in nsIDownload.idl.

Comment 73

[Ed Lee :Mardak]

Jim: Did you want to request approval for beta 5? Also mention that string changes in attachment 310098 [details] [diff] [review] will be landed after beta 5.

Drivers might be wanting a testcase. Did you say that this would fix bug 420230? If so, the testcase wouldn't be too hard to make -- just have it download a data uri and make sure it wasn't deleted.

Comment 74

[Jim Mathies [:jimm]]

> Drivers might be wanting a testcase.

That's kind of tough to do since it's tied to a config setting in Internet Options. I don't think we have the ability to set something like that in tests.. but I might be wrong.

> Did you say that this would fix bug
> 420230? If so, the testcase wouldn't be too hard to make -- just have it
> download a data uri and make sure it wasn't deleted.

No, it does not. I'll make another patch for that after doing some investigation. The solution would be to create a filter for the data scheme under certain circumstances, but I'm concerned about the security implications of turning virus checking off for data brought down. So I wanted to do some research first into what measures we currently have in place for those.

Comment 75

[Jim Mathies [:jimm]]

Which brings up the question, should bug 420230 also block beta 5 since this patch will make it impossible to use the scanWhenDone pref as a work around?

Comment 76

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310999 [details] [diff] [review]
check policy patch v.3

a=beltzner, tests = yay!

Comment 77

[Jim Mathies [:jimm]]

The only patch that should land is "check policy patch v.3".

Comment 78

[Jim Mathies [:jimm]]

not yet, try server doesn't like.

Comment 79

[Jim Mathies [:jimm]]

argh, the patch didn't apply. 
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1206132209.1206132477.17772.gz

Comment 80

[Jim Mathies [:jimm]]

Attachment: check policy patch v.3

fixed patch bustage.

Comment 81

[Jim Mathies [:jimm]]

the only patch that should land is "check policy patch v.3".

Comment 82

[Ed Lee :Mardak]

Checking in toolkit/components/downloads/public/nsIDownload.idl;
/cvsroot/mozilla/toolkit/components/downloads/public/nsIDownload.idl,v  <--  nsIDownload.idl
new revision: 1.23; previous revision: 1.22
done
Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
/cvsroot/mozilla/toolkit/components/downloads/public/nsIDownloadManager.idl,v  <--  nsIDownloadManager.idl
new revision: 1.26; previous revision: 1.25
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.176; previous revision: 1.175
done
Checking in toolkit/components/downloads/src/nsDownloadProxy.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadProxy.h,v  <--  nsDownloadProxy.h
new revision: 1.22; previous revision: 1.21
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.cpp,v  <--  nsDownloadScanner.cpp
new revision: 1.16; previous revision: 1.15
done
Checking in toolkit/components/downloads/src/nsDownloadScanner.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadScanner.h,v  <--  nsDownloadScanner.h
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js,v  <--  DownloadProgressListener.js
new revision: 1.39; previous revision: 1.38
done
Checking in toolkit/mozapps/downloads/content/download.xml;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/download.xml,v  <--  download.xml
new revision: 1.53; previous revision: 1.52
done
Checking in toolkit/mozapps/downloads/content/downloads.css;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.css,v  <--  downloads.css
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.141; previous revision: 1.140
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js,v  <--  browser_basic_functionality.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js,v  <--  browser_bug_411172.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_411172_mac.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_411172_mac.js,v  <--  browser_bug_411172_mac.js
new revision: 1.4; previous revision: 1.3
done

Comment 83

[Ed Lee :Mardak]

Orange fix so the download states are in descending order:

diff -r1.5 browser_basic_functionality.js
44a45
> // Downloads are sorted by endTime, so make sure the end times are distinct
121,122c122,123
<     startTime: 1180493839859230,
<     endTime: 1180493839859231,
---
>     startTime: 1180493839859229,
>     endTime: 1180493839859229,
BEdBook:~/mozilla Ed$ cvs commit -m 'Bustage fix for bug 416683' !$
cvs commit -m 'Bustage fix for bug 416683' toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js
Checking in toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_basic_functionality.js,v  <--  browser_basic_functionality.js
new revision: 1.6; previous revision: 1.5
done

Comment 84

[Ed Lee :Mardak]

More bustage fix for the other files

RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js,v
retrieving revision 1.5
diff -r1.5 browser_bug_411172.js
44a45
> // Downloads are sorted by endTime, so make sure the end times are distinct
84,85c85,86
<     startTime: 1180493839859230,
<     endTime: 1180493839859231,
---
>     startTime: 1180493839859229,
>     endTime: 1180493839859229,
RCS file: /cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_411172_mac.js,v
retrieving revision 1.4
diff -r1.4 browser_bug_411172_mac.js
44a45
> // Downloads are sorted by endTime, so make sure the end times are distinct
84,85c85,86
<     startTime: 1180493839859230,
<     endTime: 1180493839859231,
---
>     startTime: 1180493839859229,
>     endTime: 1180493839859229,
EdBook:~/mozilla Ed$ cvs commit -m 'More bustage fix for bug 416683. r=sdwilsh' !$
cvs commit -m 'More bustage fix for bug 416683. r=sdwilsh' toolkit/mozapps/downloads/tests/browser/browser_bug_411172{,_mac}.js
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_411172.js,v  <--  browser_bug_411172.js
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/mozapps/downloads/tests/browser/browser_bug_411172_mac.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/tests/browser/browser_bug_411172_mac.js,v  <--  browser_bug_411172_mac.js
new revision: 1.5; previous revision: 1.4
done

Comment 85

[Mike Connor [:mconnor]]

remaining bits are not blocking b5, moving off list.

Comment 86

[Shawn Wilsher :sdwilsh]

This bug is fixed as is - filed bug 424547 to fix the strings.

Comment 87

[Jim Mathies [:jimm]]

I'm thinking of adding a pref for this so users can disable security zone related checks and fall back on IOfficeAntiVirus to scan files. Currently we're enforcing the results of CheckPolicy on all downloads for security purposes but in light of bugs like bug 425946 I'm wondering if we should give users the ability to turn this stuff off completely.

Comment 88

[rstat1]

I'd say that wou(In reply to comment #87)
> I'm thinking of adding a pref for this so users can disable security zone
> related checks and fall back on IOfficeAntiVirus to scan files. Currently we're
> enforcing the results of CheckPolicy on all downloads for security purposes but
> in light of bugs like bug 425946 I'm wondering if we should give users the
> ability to turn this stuff off completely.
> 
I'd say that would be a very good idea.

Comment 89

[Martin W. Kirst]

(In reply to comment #87)
> I'm thinking of adding a pref for this so users can disable security zone
> related checks and fall back on IOfficeAntiVirus to scan files. Currently we're
> enforcing the results of CheckPolicy on all downloads for security purposes but
> in light of bugs like bug 425946 I'm wondering if we should give users the
> ability to turn this stuff off completely.
> 

+1 I'm would also appreciate this very much!
As I also commented in bug 425946, I'm using FF to keep my IE
in 'full blocked mode'. In any case should consider not just an
option pane in preferences but also some quick links in content
menu from download window. This would speed up the process of adding
sites to the right zone.
I like 'Firefox - rediscover the web'. So please, don't pay of usability
just for some <1% more security. (Antivirus shields are always active,
even when you click a .exe, And people who don't have anti virus tools,
they will not gain more security with this.)

Regards
 Martin

Comment 92

[musorchtchik]

I have discovered that this issue is dependant on the risk level that is assigned to ".partial" file type (I use IE not Mozilla, if you use Mozilla you probably have another extention). If this file type has high risk level and the setting "Launching programs and unsafe file" is set to Disabled, then binary (and any other files with high risk level) downloads are really deleted. By default all file types have moderate risk level except those that are explicitlty defined (in built-in list by Microsoft). So does ".partial" file type. By if you set the setting "Default risk level for file attachments" (under Windows Components\Attachment manager in GPO) to Enabled (High risk) you will have ".partial" file type of high risk level.
So, all you need is to put ".partial" file type in the setting "Inclusion list for moderate risk file types" (under Windows Components\Attachment manager in GPO)