Closed
Bug 651093
Opened 13 years ago
Closed 13 years ago
nsFilePicker does not honor privacy settings when saving a file
Categories
(Core :: Widget: Win32, defect)
Tracking
()
VERIFIED
FIXED
mozilla6
People
(Reporter: jimm, Assigned: jimm)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 5 obsolete files)
17.20 KB,
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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•13 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•13 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•13 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•13 years ago
|
||
Assignee: nobody → jmathies
![]() |
Assignee | |
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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•13 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•13 years ago
|
Attachment #533948 -
Flags: review?(sdwilsh)
![]() |
Assignee | |
Comment 8•13 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•13 years ago
|
Attachment #533946 -
Flags: superreview?(gavin.sharp)
Comment 9•13 years ago
|
||
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•13 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.
Comment 11•13 years ago
|
||
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•13 years ago
|
||
![]() |
Assignee | |
Comment 13•13 years ago
|
||
slightly clearer comment that includes a note about private browsing overriding the setting.
Attachment #534536 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•13 years ago
|
Attachment #534539 -
Flags: review?(gavin.sharp)
![]() |
Assignee | |
Comment 14•13 years ago
|
||
Attachment #533948 -
Attachment is obsolete: true
Attachment #533948 -
Flags: review?(sdwilsh)
Attachment #534543 -
Flags: review?(sdwilsh)
![]() |
Assignee | |
Comment 15•13 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•13 years ago
|
Attachment #534543 -
Attachment is obsolete: true
Comment 16•13 years ago
|
||
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•13 years ago
|
Attachment #533946 -
Attachment is obsolete: true
![]() |
Assignee | |
Comment 17•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/26440e8180a2
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Target Milestone: --- → mozilla6
Comment 18•13 years ago
|
||
dev-doc-needed: https://developer.mozilla.org/en/nsIFilePicker should be updated to mention addToRecentDocs property.
Updated•13 years ago
|
Keywords: dev-doc-needed
Comment 19•13 years ago
|
||
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•13 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.
Description
•