nsFilePicker does not honor privacy settings when saving a file

VERIFIED FIXED in mozilla6

Status

()

Core
Widget: Win32
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla6
x86
Windows 7
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 533934 [details] [diff] [review]
fix v.1
Assignee: nobody → jmathies
(Assignee)

Comment 5

6 years ago
Created attachment 533946 [details] [diff] [review]
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.
Attachment #533934 - Attachment is obsolete: true
Attachment #533946 - Flags: review?(doug.turner)
(Assignee)

Comment 6

6 years ago
Created attachment 533948 [details] [diff] [review]
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

6 years ago
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;
Attachment #533946 - Flags: review?(doug.turner) → review+
(Assignee)

Updated

6 years ago
Attachment #533948 - Flags: review?(sdwilsh)
(Assignee)

Comment 8

6 years ago
(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.

Updated

6 years ago
Attachment #533946 - Flags: superreview?(gavin.sharp)
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.
Attachment #533946 - Flags: superreview?(gavin.sharp) → superreview+
(Assignee)

Comment 10

6 years ago
(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.
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).
(Assignee)

Comment 12

6 years ago
Created attachment 534536 [details] [diff] [review]
file picker patch v.2
(Assignee)

Comment 13

6 years ago
Created attachment 534539 [details] [diff] [review]
file picker patch v.2

slightly clearer comment that includes a note about private browsing overriding the setting.
Attachment #534536 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #534539 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

6 years ago
Created attachment 534543 [details] [diff] [review]
content utils patch v.2
Attachment #533948 - Attachment is obsolete: true
Attachment #533948 - Flags: review?(sdwilsh)
Attachment #534543 - Flags: review?(sdwilsh)
(Assignee)

Comment 15

6 years ago
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.
Attachment #534543 - Flags: review?(sdwilsh)
(Assignee)

Updated

6 years ago
Attachment #534543 - Attachment is obsolete: true
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.
Attachment #534539 - Flags: review?(gavin.sharp) → review+
(Assignee)

Updated

6 years ago
Attachment #533946 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/mozilla-central/rev/26440e8180a2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Target Milestone: --- → mozilla6

Comment 18

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

Updated

6 years ago
Keywords: dev-doc-needed
Documentation updated:

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

And mentioned on Firefox 6 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Comment 20

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.