Open Web Location dialog leaks URL/search entered in private browsing mode

VERIFIED FIXED in Firefox 3.7a1

Status

()

Firefox
Private Browsing
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: myk, Assigned: Away for a while)

Tracking

(4 keywords)

Trunk
Firefox 3.7a1
dev-doc-complete, privacy, verified1.9.1, verified1.9.2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3.5 -
blocking-firefox3.6 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, status1.9.1 .4-fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

9 years ago
The Open Web Location dialog persists the last URL or search terms you enter across changes to the status of private browsing, which means it can reveal private URLs or search terms that you enter while browsing privately and don't want to reveal to others after you leave that mode.

Steps to Reproduce:

1. enter private browsing mode;
2. open the Open Web Location dialog;*
3. enter anything into the text field and press the Open button;
4. exit private browsing mode;
5. open the Open Web Location dialog again.

* There are (at least) two ways to open the Open Web Location dialog.  One is to be in full screen mode on Windows or Linux (View > Full Screen).  The other is to remove the location bar from the navigation toolbar on all OSes using View > Toolbars > Customize...  In either case, selecting File > Open Location will then open the Open Web Location dialog instead of focusing the location bar.

Expected Results: the text you entered into the text field is no longer there.

Actual Results: the text you entered into the text field is still there.

This seems significant enough to warrant some status wrt 3.5.  Requesting blocking so drivers see it and can determine whether it warrants blocking or wanted status.
Flags: blocking-firefox3.5?
Indeed, the code in openLocation.js needs to avoid setting a pref in private browsing mode.

(We made "Clear private data..." clear this data in bug 397196.)
Version: unspecified → 3.5 Branch
Confirmed in latest 1.9.2 trunk

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090620 Minefield/3.6a1pre

and 3.5 RC2

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090616 Firefox/3.5

Nice find Myk
Version: 3.5 Branch → unspecified
Not gonna block final on this, as it's only through the Open Web Location dialog which isn't widely used. Should try to get it on the stability/security release cycle.
Flags: wanted1.9.1.x?
Flags: blocking-firefox3.6+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
(Assignee)

Comment 4

9 years ago
Taking...
Assignee: nobody → ehsan.akhgari
Status: NEW → ASSIGNED
Keywords: privacy
Version: unspecified → Trunk
(Assignee)

Comment 5

9 years ago
We need to persist the pref to an in-memory variable for use inside the private browsing mode, just like the download last dir location, so that the usability of this dialog inside the private browsing mode does not regress.
(Reporter)

Comment 6

9 years ago
Note: bug 287329 talks about making the dialog behave more like the location bar.  Perhaps it would be easier to update the dialog to behave more like the bar than to maintain the existing functionality.
(Assignee)

Comment 7

9 years ago
(In reply to comment #6)
> Note: bug 287329 talks about making the dialog behave more like the location
> bar.  Perhaps it would be easier to update the dialog to behave more like the
> bar than to maintain the existing functionality.

That is out of scope for this bug.  But even if the open location dialog gets modified to use something like the URL bar, we'd still may want to auto-fill it with the same value, so that might not solve the issue completely.
(Assignee)

Comment 8

9 years ago
Created attachment 388058 [details] [diff] [review]
Patch (v1)

Patch + automated unit tests.
Attachment #388058 - Flags: review?(mconnor)
(Assignee)

Updated

9 years ago
Whiteboard: [needs r=mconnor]
Comment on attachment 388058 [details] [diff] [review]
Patch (v1)

I'm not sure I get the design here.  Why have the external JSM?  That said, if you're going to have that, why not always get/set via gOpenLocationLastURL and have the private browsing checks all in the getter and setter?
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
> (From update of attachment 388058 [details] [diff] [review])
> I'm not sure I get the design here.  Why have the external JSM?

This was done to make the code more re-usable for extensions, and also throughout the codebase if we need to do this from another location some day.

> That said, if
> you're going to have that, why not always get/set via gOpenLocationLastURL and
> have the private browsing checks all in the getter and setter?

The design of gOpenLocationLastURL follows that of gDownloadLastDir <http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/src/DownloadLastDir.jsm#57>.  This object has a single responsibility: to make sure that the contained path is not persistent after a private browsing mode switch.

We can modify gOpenLocationLastURL to handle it all in its getters/setters, if you think that's more appropriate, but I personally wouldn't touch that for the sake of compatibility with gDownloadLastDir at least.

That said, I'm willing to change the design if you still think it's necessary.
So if the idea is to be extensible/reusable, we shouldn't make the consumers need to be PB-aware, we should make the object smart and consumers simple.  If you put the intelligence in the object, consumers just get/set a value and the Right Thing happens.  That, to me, is an ideal case for any reuse...

(I'm not sure why you used "compatibility" as an example.  I guess you mean similarity of pattern, minimizing confusion?  I think gDownloadLastDir was the wrong impl, and I think we should change it too.)
gDownloadLastDir is used in two separate places that both read the same pref value. They need to refer to the same in-memory value while in PB mode.

Since that restriction doesn't apply in this case, I think we should just go with the simpler approach.
To be clear, by "simpler approach", I meant just putting the logic in openLocation.js and avoiding the module. But on second thought, I guess there can be two instances of that code running during a PB session, and if we care to maintain the current behavior in that case we'll need something like the JS module. It's not clear to me that we do, though...
(Assignee)

Comment 14

9 years ago
(In reply to comment #13)
> To be clear, by "simpler approach", I meant just putting the logic in
> openLocation.js and avoiding the module. But on second thought, I guess there
> can be two instances of that code running during a PB session, and if we care
> to maintain the current behavior in that case we'll need something like the JS
> module. It's not clear to me that we do, though...

One simple example of this is when the user has two windows open.  If we don't use a JSM, the last open dir in each window will be different, which is different to the behavior observed inside the private browsing mode.

(In reply to comment #11)
> So if the idea is to be extensible/reusable, we shouldn't make the consumers
> need to be PB-aware, we should make the object smart and consumers simple.  If
> you put the intelligence in the object, consumers just get/set a value and the
> Right Thing happens.  That, to me, is an ideal case for any reuse...

Hmm, I think you're right.  Let's reach a final decision on whether or not to use a JSM and I'll update the patch accordingly then.

> (I'm not sure why you used "compatibility" as an example.  I guess you mean
> similarity of pattern, minimizing confusion?  I think gDownloadLastDir was the
> wrong impl, and I think we should change it too.)

Yes, by compatibility I meant similarity.  I filed bug 508526 to make gDownloadLastDir smarter.
Let's do it in a JSM, so that PB works sanely across windows.  (Sorry, meant to comment on this earlier.)

Updated

9 years ago
Attachment #388058 - Attachment is obsolete: true
Attachment #388058 - Flags: review?(mconnor)
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
(Assignee)

Comment 16

8 years ago
Created attachment 394741 [details] [diff] [review]
Patch (v2)

Use a JSM with a smart object.
Attachment #394741 - Flags: review?(mconnor)
Comment on attachment 394741 [details] [diff] [review]
Patch (v2)

Looks good, sorry for the delay.
Attachment #394741 - Flags: review?(mconnor) → review+
(Assignee)

Comment 18

8 years ago
http://hg.mozilla.org/mozilla-central/rev/31ab7a791467
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs r=mconnor] → [needs 1.9.2 landing]
Target Milestone: --- → Firefox 3.7a1
(Assignee)

Updated

8 years ago
Attachment #394741 - Flags: approval1.9.1.4?
(Assignee)

Comment 19

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4e79c7e06cc6
Keywords: dev-doc-needed, fixed1.9.2
Whiteboard: [needs 1.9.2 landing]
Verified fixed on trunk with builds on Windows and OS X like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090831 Minefield/3.7a1pre ID:20090831030956
Status: RESOLVED → VERIFIED
Looking at this, it's unclear to me what requires documentation here. This seems to be a deeply internal change. Can anyone suggest what needs documenting?
status1.9.2: --- → beta1-fixed
Keywords: fixed1.9.2
Verified fixed on 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a2pre) Gecko/20090901 Namoroka/3.6a2pre (.NET CLR 3.5.30729) ID:20090901050855
Keywords: verified1.9.2
Comment on attachment 394741 [details] [diff] [review]
Patch (v2)

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #394741 - Flags: approval1.9.1.4? → approval1.9.1.4+
(Assignee)

Comment 24

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e811669e780e
status1.9.1: ? → .4-fixed
(Assignee)

Comment 25

8 years ago
(In reply to comment #21)
> Looking at this, it's unclear to me what requires documentation here. This
> seems to be a deeply internal change. Can anyone suggest what needs
> documenting?

I added the dev-doc-needed keyword on this bug in order to document the gOpenLocationLastURL object.
This is now documented here:

https://developer.mozilla.org/en/JavaScript_code_modules/openLocationLastURL.jsm
Keywords: dev-doc-needed → dev-doc-complete

Comment 27

8 years ago
random orange test failure: 

Error 0 (Unknown error: 0).TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js | Exception thrown - [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch2.clearUserPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_openlocation.js :: test :: line 91" data: no]
TEST-UNEXPECTED-FAIL | /builds/moz2_slave/mozilla-1.9.1-macosx-unittest-everythingelse/build/xpcshell/tests/test_privatebrowsing/unit/test_openLocationLastURL.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.clearUserPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: file:///builds/moz2_slave/mozilla-1.9.1-macosx-unittest-everythingelse/build/Shiretoko.app/Contents/MacOS/modules/openLocationLastURL.jsm :: anonymous :: line 91" data: no]
Unknown Error: command finished with exit code: 1

Comment 28

8 years ago
the above random orange is happening on linux and mac
Could this be caused because clearUserPref throws an error if the value of the pref hasn't been modified? It looks like that. See bug 487059.
(Assignee)

Comment 30

8 years ago
(In reply to comment #29)
> Could this be caused because clearUserPref throws an error if the value of the
> pref hasn't been modified? It looks like that. See bug 487059.

Yes, I landed a fix for the failing tests as <http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e62d7ebe4a8e>.
Tree is green now. I can also verify the fix on 1.9.1 with builds on OSX and Windows like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4pre) Gecko/20090907 Shiretoko/3.5.4pre ID:20090907040203
Keywords: verified1.9.1
Blocks: 524995
Depends on: 541404
You need to log in before you can comment on or make changes to this bug.