651093 - nsFilePicker does not honor privacy settings when saving a file

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

When running in privacy mode, the file picker does not use the OFN_DONTADDTORECENT option, so files saved out will be added to Windows recent docs list.

STR:

1) open in privacy mode
2) display an image in the browser
3) right click the image -> save image as
4) save the image

Result: the image will be added to recent docs lists

Note on win7 you have to enable recent docs to see this: right click start menu button, select properties, select customize, scroll down until you find the "Recent Items" check box.

Comment 1

[Jim Mathies [:jimm]]

http://mxr.mozilla.org/mozilla-central/source/widget/public/nsIFilePicker.idl

Looks like the simplest solution here would be to upgrade init to take another option, or add a new method to turn on privacy mode after init is called. 

Personally I'd like for it to be in init so consumers are forced to set the value but I imagine this interface is used a lot by 3rd parties.

Alternatively we might be able to query internally for current privacy settings and just set this automatically when invoked.

Comment 2

[Jim Mathies [:jimm]]

Or, alternatively, per the request in bug 646351, just set OFN_DONTADDTORECENT on every call to show, and never populate recent docs info via the file picker. Some users might not appreciate that behavior. In Fx it's probably not a big deal, but in other applications it might be.

Comment 3

[Jim Mathies [:jimm]]

I'm leaning toward a new property on the interface. It seems the least intrusive of all the options and keeps default behavior for existing consumers.

Comment 4

[Jim Mathies [:jimm]]

Attachment: fix v.1

Comment 5

[Jim Mathies [:jimm]]

Attachment: addToRecentDocs patch v.1

Currently we default to adding all files to recent docs when invoking the Windows file chooser since we don't pass the OFN_DONTADDTORECENT flag. This addresses the problem by defaulting this setting to off, and also provides a new property on nsIFilePicker which allows callers to enable the functionality.

Comment 6

[Jim Mathies [:jimm]]

Attachment: content utils patch v.1

This enables the add to recent docs setting for the common case of saving content out to a local file *if* 'browser.download.manager.addToRecentDocs' is set (the default) and we're not in privacy mode.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 533946 [details] [diff] [review]
addToRecentDocs patch v.1

so much code to pass one little flags. ;D


>-    ofn.Flags = OFN_NOCHANGEDIR | OFN_SHAREAWARE | OFN_LONGNAMES | OFN_OVERWRITEPROMPT | OFN_HIDEREADONLY | OFN_PATHMUSTEXIST;
>+    ofn.Flags = OFN_NOCHANGEDIR | OFN_SHAREAWARE |
>+                OFN_LONGNAMES | OFN_OVERWRITEPROMPT |
>+                OFN_HIDEREADONLY | OFN_PATHMUSTEXIST |
>+                (mAddToRecentDocs ? 0 : OFN_DONTADDTORECENT);


nit:  I'd put a the `|= OFN_DONTADDTORECENT` on its own line:

// Windows only - 
// If mAddToRecentDocs is set, prevent the system from adding
// a link to the selected file in the file system directory that
// contains the user's most recently used documents.
if (mAddToRecentDocs)
  ofn.Flags |= OFN_DONTADDTORECENT;

Comment 8

[Jim Mathies [:jimm]]

(In reply to comment #7)
> Comment on attachment 533946 [details] [diff] [review] [review]
> addToRecentDocs patch v.1
> 
> so much code to pass one little flags. ;D

This changes default behavior so I wanted to be sure there was a way to undo it if needed.

> 
> >-    ofn.Flags = OFN_NOCHANGEDIR | OFN_SHAREAWARE | OFN_LONGNAMES | OFN_OVERWRITEPROMPT | OFN_HIDEREADONLY | OFN_PATHMUSTEXIST;
> >+    ofn.Flags = OFN_NOCHANGEDIR | OFN_SHAREAWARE |
> >+                OFN_LONGNAMES | OFN_OVERWRITEPROMPT |
> >+                OFN_HIDEREADONLY | OFN_PATHMUSTEXIST |
> >+                (mAddToRecentDocs ? 0 : OFN_DONTADDTORECENT);
> 
> 
> nit:  I'd put a the `|= OFN_DONTADDTORECENT` on its own line:
> 
> // Windows only - 
> // If mAddToRecentDocs is set, prevent the system from adding
> // a link to the selected file in the file system directory that
> // contains the user's most recently used documents.
> if (mAddToRecentDocs)
>   ofn.Flags |= OFN_DONTADDTORECENT;

Will do.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 533946 [details] [diff] [review]
addToRecentDocs patch v.1

>diff --git a/widget/public/nsIFilePicker.idl b/widget/public/nsIFilePicker.idl

>  /**
>+  * Windows specific: the system's file picker can insert files chosen into
>+  * recent docs. By default nsIFilePicker does not allow this (default flag
>+  * value is false).
>+  *
>+  * @param   The new flag setting
>+  *
>+  * @return  Returns the current flag setting
>+  */
>+  attribute boolean addToRecentDocs;

I would write this comment as:

Whether or not to add the chosen file(s) to the system's recent documents list.
This attribute will be ignored on systems with no such concept. Defaults to
false.

and omit the @param and @return things, since they're useless.

I'm a little bit conflicted about whether it's a good idea to change the default behavior. On the one hand it simplifies concerns about getting this right in private browsing mode across the board, but on the other it's going to break useful behavior other apps might expect (particularly e.g. Thunderbird or Composer, where opening/saving files is probably more common). Perhaps we should leave the default behavior alone for the moment (just default mAddToRecentDocs to true and adjust the docs accordingly), and then in a followup we can revisit either changing the default, or adding pb mode support to the core filepicker code, or something else.

sr=me with those comments addressed.

Comment 10

[Jim Mathies [:jimm]]

(In reply to comment #9)
> I'm a little bit conflicted about whether it's a good idea to change the
> default behavior. On the one hand it simplifies concerns about getting this
> right in private browsing mode across the board, but on the other it's going
> to break useful behavior other apps might expect (particularly e.g.
> Thunderbird or Composer, where opening/saving files is probably more
> common). Perhaps we should leave the default behavior alone for the moment
> (just default mAddToRecentDocs to true and adjust the docs accordingly), and
> then in a followup we can revisit either changing the default, or adding pb
> mode support to the core filepicker code, or something else.
> 
> sr=me with those comments addressed.

That was my original plan, however the number of callers that would need to change in m-c was pretty large, a simple mxr estimate put it at around ~15 or so, some of which are in areas like certs, feeds, content, app settings, etc.. (I'm not sure we have access to privacy mode settings in all those locations.)

I ended up deciding to bail on that and just went with changing the default, since I'm pretty sure built-in os search picks up on most of this stuff even if you don't add it via the api. (So apps like tb may not be hurt much by the lack of exposure.)

If we really think this will kill something in an app like tb, I can start putting together patches for all the fx callers to fix the privacy leak. 

cc'ing Neil, he might be able to shed some light on how changing the default might effect Thunderbird.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Oh, I didn't quite realize this bug was about obeying private browsing mode specifically (rather than just a way to allow fixing bug 646351).

We can fix the privacy leak by having the filepicker code check the private browsing mode directly. The attribute can then just be an override (defaults to true, but if false then PB mode is ignored and we always pass OFN_DONTADDTORECENT).

Comment 12

[Jim Mathies [:jimm]]

Attachment: file picker patch v.2

Comment 13

[Jim Mathies [:jimm]]

Attachment: file picker patch v.2

slightly clearer comment that includes a note about private browsing overriding the setting.

Comment 14

[Jim Mathies [:jimm]]

Attachment: content utils patch v.2

Comment 15

[Jim Mathies [:jimm]]

Comment on attachment 534543 [details] [diff] [review]
content utils patch v.2

(In reply to comment #14)
> Created attachment 534543 [details] [diff] [review] [review]
> content utils patch v.2

Sorry for the spam shawn. Now that this isn't checking pb mode, I should probably move this to bug 646351, since we'll need a few of these there to finish out that bug.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 534539 [details] [diff] [review]
file picker patch v.2

>diff --git a/widget/public/nsIFilePicker.idl b/widget/public/nsIFilePicker.idl

>  /**
>+  * Whether or not to add the chosen file(s) to the system's recent documents
>+  * list. This attribute will be ignored on systems with no such concept.
>+  * Defaults to true except in cases when private browsing is enabled,
>+  * in which case the setting is ignored and files are not added.

Another comment tweak suggestion (I can't help myself):

Controls whether the chosen file(s) should be added to the system's recent documents list. This attribute will be ignored if the system has no "Recent Docs" concept, or if the application is in private browsing mode (in which case the file will not be added). Defaults to true.

Comment 17

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/26440e8180a2

Comment 18

[Nickolay_Ponomarev]

dev-doc-needed: https://developer.mozilla.org/en/nsIFilePicker should be updated to mention addToRecentDocs property.

Comment 19

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIFilePicker

And mentioned on Firefox 6 for developers.

Comment 20

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on Win 7 and WinXP using the steps from Comment 0 and a saved image while in Private Browsing Mode is not diaplayed in the Systems Recent files.

Setting status to Verified Fixed.