Closed Bug 491759 Opened 11 years ago Closed 10 years ago

Clear geolocation token when exiting private browsing

Categories

(Firefox :: Private Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta3-fixed
status1.9.1 --- .8-fixed

People

(Reporter: Gavin, Assigned: Natch)

References

()

Details

(Keywords: privacy, Whiteboard: [geo])

Attachments

(3 files, 7 obsolete files)

All the token could potentially reveal that you used a site that used geolocation, which is pretty minor, but it probably doesn't really hurt to also clear it in this case.
Duplicate of this bug: 491754
Attached patch patch v.1 (obsolete) — Splinter Review
Assignee: nobody → doug.turner
Attachment #376097 - Flags: review?(gavin.sharp)
Comment on attachment 376097 [details] [diff] [review]
patch v.1

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js

>   removeDataFromDomain: function PBS_removeDataFromDomain(aDomain)
>   {
>+
>+    // clear any and all network geolocation provider sessions
>+    var psvc = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService);
>+    try {
>+        var branch = psvc.getBranch("geo.wifi.access_token.");
>+        branch.deleteBranch("");
>+    } catch (e) {}

Should be able to use:
this._prefs.deleteBranch("geo.wifi.access_token.");

instead, r=me with that.
Attachment #376097 - Flags: review?(gavin.sharp) → review+
(In reply to comment #2)
> Created an attachment (id=376097) [details]
> patch v.1

Does this actually remove it? I don't think that's the way to do it, I think you want to listen to "private-browsing" in the geolocation module and remove it there. Seems like I'm wrong though, given your r+...
Er, this patch is just in the wrong bug. It was meant for 491756.
Attachment #376097 - Attachment is obsolete: true
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #376167 - Flags: review?(gavin.sharp)
Flags: wanted-firefox3.5?
Whiteboard: [geo]
Component: General → Private Browsing
QA Contact: general → private.browsing
Attachment #376167 - Flags: review-
Comment on attachment 376167 [details] [diff] [review]
patch v.1

>diff --git a/dom/src/geolocation/NetworkGeolocationProvider.js b/dom/src/geolocation/NetworkGeolocationProvider.js
>+    observe: function (aSubject, aTopic, aData) {
>+        if (aTopic == "private-browsing") {

Please check aData here and only delete the pref branch if it's either "enter" or "exit".  In the future, we might have other values for aData representing other things, and I prefer to keep checking aData everywhere if possible for now.

Other than that, it looks great to me.
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #376167 - Attachment is obsolete: true
Attachment #376167 - Flags: review?(gavin.sharp)
Comment on attachment 376248 [details] [diff] [review]
patch v.2

incorporates ehsan.akhgari's comment.
Attachment #376248 - Flags: review?(gavin.sharp)
http://hg.mozilla.org/mozilla-central/rev/c1b04224d6cf
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
dougt: Can you suggest a good way for qa to verify this?  Thanks.
yes.

1) new profile
2) enter into private browsing mode
3) do a geolocation request
4) edit private browsing mode
5) go to about:config
6) filter "geo.wifi.access_token"
7) ensure that nothing is in preferences
Flags: in-litmus?
Verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090508 Minefield/3.6a1pre. I followed the steps in Comment 12 to verify.
Status: RESOLVED → VERIFIED
Comment on attachment 376248 [details] [diff] [review]
patch v.2

a191=beltzner
Attachment #376248 - Flags: approval1.9.1+
Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090515 Shiretoko/3.5b5pre. Will add a litmus test case for this as well.
https://litmus.mozilla.org/show_test.cgi?id=7718 added to Litmus
Flags: in-litmus? → in-litmus+
This should better be an automated test. Request its addition.
Flags: in-testsuite?
Target Milestone: --- → Firefox 3.6a1
adding tests for this in bug 493122.
Depends on: 493122
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090515 Minefield/3.6a1pre (official nightly)
(In reply to comment #16)
> Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac
> OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090515 Shiretoko/3.5b5pre. Will add a
> litmus test case for this as well.

How was this verified, I cannot seem to verify this and in fact the automated test I'm trying to add to bug 493122 is failing. Note: you must check that the access_cookie is set before private browsing mode as sometimes there is no cookie at all.
As already said, I cannot verify this too. Even when the access token is created in PB mode. Leaving PB mode the token still exists. That happens on trunk and recent Shiretoko builds.

=> Reopen bug.
Status: VERIFIED → REOPENED
Keywords: verified1.9.1
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
requesting blocking to get this sorted out.
Flags: blocking-firefox3.5?
@whimboo, could you let me know if geolocation was running when you exited.  I think what you may be seeing is if geolocation shuts down before the PB exist, we don't get a chance to clean up.
I don't think that this should be done in the geolocation service.  The best place for this code to live is inside the private browsing service, which would last for the whole duration of the browsing session.

Doug, can you please provide an alternate patch which undoes attachment 376248 [details] [diff] [review] and moves this code to nsPrivateBrowsingService instead?
Attached patch keep clean-up code together (obsolete) — Splinter Review
(In reply to comment #24)
> I don't think that this should be done in the geolocation service.

OTOH, I'm opposed to spreading even more geolocation code around our code base if we can help it.
Attachment #376248 - Attachment is obsolete: true
Attachment #378006 - Flags: review?(doug.turner)
Attachment #378006 - Attachment is obsolete: true
Attachment #378007 - Flags: review?(doug.turner)
Attachment #378006 - Flags: review?(doug.turner)
Comment on attachment 378007 [details] [diff] [review]
keep clean-up code together even tighter

If I'm reading the code correctly, this still doesn't solve the case in which the following sequence of events happen:

1. A geolocation token is saved.
2. Browser is restarted.
3. Private browsing mode is started (without first visiting a page with geolocation service).
4. A page with geolocation is opened, the previously stored token should still be used.

Is that right?
(In reply to comment #27)
You're indeed right. This patch now even also solves the case where you set cookies to expire at the end of the session, use Geolocation, crash Firefox, restart and quit (previously, this could have persisted the token as well).
Attachment #378007 - Attachment is obsolete: true
Attachment #378012 - Flags: review?(doug.turner)
Attachment #378007 - Flags: review?(doug.turner)
Comment on attachment 378012 [details] [diff] [review]
keep clean-up code together and always ready

>+    QueryInterface:    XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference])

When applying this patch, please manually add a comma at the end of this line...
Simon, could you start a tryserver build? So I could check the behavior right before the review of Doug.

(In reply to comment #23)
> @whimboo, could you let me know if geolocation was running when you exited.  I
> think what you may be seeing is if geolocation shuts down before the PB exist,
> we don't get a chance to clean up.

Steps I did were the following:
1. New profile
2. Start Private Browsing mode
3. Open http://people.mozilla.org/~dougt/geo.html
4. Leave Private Browsing mode

After step 4 the access token was still existent. I believe this will be fixed by Simon's patch.
Attachment #378012 - Flags: review+
Comment on attachment 378012 [details] [diff] [review]
keep clean-up code together and always ready

Looks great.  r=me on the privatebrowsing part.  :-)
i like the approach a bunch better.  can we make it so that WifiGeoCleanupService doesn't get started each time the browser starts up, but instead only starts when geo starts?
(In reply to comment #32)
> i like the approach a bunch better.  can we make it so that
> WifiGeoCleanupService doesn't get started each time the browser starts up, but
> instead only starts when geo starts?

Doesn't that cause problems such as comment 27 again?
depends how it is done.  i am just worry about having a startup hit for something that might not at all be used.
we could test to see if we are in private browsing mode and just clear the access token when we startup geolocation, i suppose.  This will resolve comment 27, correct?
(In reply to comment #35)
> we could test to see if we are in private browsing mode and just clear the
> access token when we startup geolocation, i suppose.  This will resolve comment
> 27, correct?

Based on my understanding of the geolocation stuff, yes, it should.
Actually that won't work.  In comment 27, replace step 3 with "Used Forget About This Site on a history entry" to get a set of steps which would lead to the geolocation token not being cleared when it should be.
I don't think this blocks, though it's definitely wanted.
Flags: wanted-firefox3.5+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
The clearing *must* work whether geolocation has been used this session or not. Suppose a user uses the geo feature, shuts down the browser, restarts it and goes directly into pb mode, the access_token must be cleared then as well. So there must be a startup service for this, if only just to observe pb notifications. Alternatively, this can be handled directly in the pb service, as are other items (eg clearing nsIConsoleService).
(In reply to comment #34)
> i am just worry about having a startup hit for
> something that might not at all be used.

The easiest solution would of course be to never store the token in the preferences but only inside the WifiGeoPositionProvider (which would be the same behavior as setting network.cookie.lifetimePolicy to 2).
Lets see if this has any Ts impact by pushing to try.  If it does, maybe we should move this into the nsBrowserGlue that already gets started up?
pushed to try; awaiting results.

gavin, can you take a look at this patch?
(In reply to comment #42)
> pushed to try; awaiting results.

Note: In case you haven't read comment #29, you might want to push again...

(In reply to comment #41)
> should move this into the nsBrowserGlue that already gets started up?

nsBrowserGlue.js same as nsPrivateBrowsing.js are both Firefox specific, while your provider and the "private-browsing" notification itself are not (SeaMonkey might have its own implementation).
Attachment #376248 - Flags: approval1.9.1+ → approval1.9.1-
Comment on attachment 376248 [details] [diff] [review]
patch v.2

Revoking approval. We're cutting back on potential churn here. We can try again for 3.5.1

You can leave the non-working patch in 191 for now, but can't carry over approvals.
Flags: wanted1.9.1.x?
Duplicate of this bug: 496595
Depends on: 497032
I am more or less unhappy with having to startup a new service to monitor these events.  However, i really like how everything is managed in one place.

Instead of creating a startup listener, could we start this service the first time geo is use?  If so, I think everyone would be happy.
(In reply to comment #46)
> Instead of creating a startup listener, could we start this service the first
> time geo is use?

Comment #39 explains why this isn't really an option. On Trunk, you really want to use conventional cookies (which already behave as expected, except for the missing chrome-only flag). And for the Branch, let me ask you again: why can't you just always clear that token when your service shuts down (which AFAICT it also does when enabling/disabling Private Browsing mode)?
so, I am not sure that we should be starting this up exclusively to monitor something that might never be used.  I agree to wait on the chrome cookies.

The token is suppose to live for more or less a 2 week period in normal usage.  When a person enters or exists private browsing mode, getting a new token is fine.
Then why don't you add the cleaning directly to nsPrivateBrowsingService.js to PBS_observe right after where we clear the Error Console?

(In reply to comment #48)
> When a person enters or exists private browsing mode, getting a new token is
> fine.

And the paranoid me keeps asking: And why isn't it otherwise fine? ;-)
simon, that would be fine, i suppose.  I did like your idea of keeping all of our stuff together.

Right, paranoia is fine.  Basically, google uses this token for detection of DDoS style attacks.   I suppose we could restore the access token when existing out of private browsing mode if one existed.  However, I didn't think it was that big of a deal.
I don't see how the token helps against DDoS attacks - if it really is distributed, you'd get multiple IDs anyways, and it's unlikely that such attackers would use an unmodified Firefox client to perform the attack rather than a client that avoids sending the token altogether.
Hi there,

My name is Jonathan McPhie, I'm a PM on Google's location platform.

The client identifier that gets sent with a location request to Google Location Service is used to protect our service against common attacks such as DoS.  While we understand that this is not a perfect signal, we have found it to be very valuable in helping us to protect our service and users from malicious use.  

As a reminder, the client identifier is not used to identify individual users of the service, and Firefox resets this identifier periodically.

If you have further questions, please feel free to contact me.

Thanks,
Jonathan
This got lost...
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
Whiteboard: [geo] → [geo] [needs review dougt]
natch, for the trunk, i think we want to wait for chrome-only cookies.  We should mark this bug dependent on that work. Anyone have a bug number?
status1.9.1: --- → ?
Flags: wanted1.9.1.x?
Attachment #378012 - Flags: review?(doug.turner)
Depends on: 511933
Flags: wanted-firefox3.6?
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6+
Comment on attachment 378012 [details] [diff] [review]
keep clean-up code together and always ready

I think we need to land this patch on trunk, get it baked and land it on 1.9.2, and then file a new bug to revert this change from trunk and use a chrome cookie for implementing the token on trunk.  Doug, what do you think?
Attachment #378012 - Flags: review?(doug.turner)
ehsan, i would rather just do the chrome-only cookies approach.  is that something we can do for 3.6?
(In reply to comment #56)
> ehsan, i would rather just do the chrome-only cookies approach.  is that
> something we can do for 3.6?

I'm not sure, Dan is the person to ask here.
Pretty late in the game for 3.6, and I don't have cycles to put into it atm - but if someone picked it up, I could certainly help out.
Priority: -- → P2
Doug, bug 511933 was minused as a blocker for 3.6, so I think we must take the less ideal approach in attachment 378012 [details] [diff] [review] for 3.6.  What do you think?
Ehsan, in comment #34 i mentioned that I am worried about startup perf.  Loading another component that may never be used doesn't sound like the best idea.  Do we have any data that shows that this doesn't hurt anyone (esp. mobile)?
(In reply to comment #60)
> Ehsan, in comment #34 i mentioned that I am worried about startup perf. 
> Loading another component that may never be used doesn't sound like the best
> idea.  Do we have any data that shows that this doesn't hurt anyone (esp.
> mobile)?

You pushed a try server build in comment 42, what results did you get?
As an interim fix it should be possible to just deleteBranch in the pb service (since it's already an app-startup service) here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#356
(In reply to comment #62)
> As an interim fix it should be possible to just deleteBranch in the pb service
> (since it's already an app-startup service) here:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js#356

Hmm, I'm fine with taking that fix for 3.6, and then reverting it when chrome cookies are available.  Doug, what do you think?

Natch: if Doug feels comfortable with it as well, would you mind writing a quick patch for it with a quick review from me guaranteed? :-)
sounds like a fine approach.
If you guys could file a bug for changing this behavior if & when we get chrome-only cookies, and make it dependent on the chrome-only bug, that would be awesome.
Filed bug 524790.
No longer depends on: 511933
Whiteboard: [geo] [needs review dougt] → [geo]
Whiteboard: [geo] → [geo] [needs new patch]
Attachment #378012 - Flags: review?(doug.turner)
Attached patch proposed patch w/test (obsolete) — Splinter Review
This is the proposed patch with a test.

The observer is being removed from the geolocation module so there's no need for it to implement nsISupportsWeakRef
Assignee: doug.turner → highmind63
Attachment #378012 - Attachment is obsolete: true
Attachment #408703 - Flags: review?(doug.turner)
Comment on attachment 408703 [details] [diff] [review]
proposed patch w/test

Keep forgetting Ehsan's new bug mail address ;)
Attachment #408703 - Flags: review?(ehsan)
Whiteboard: [geo] [needs new patch] → [has patch][needs review ehsan, dougt][geo]
Comment on attachment 408703 [details] [diff] [review]
proposed patch w/test

The patch looks good to me overall, but I have a few comments.

>diff --git a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>--- a/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>+++ b/browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
>@@ -360,16 +360,20 @@ PrivateBrowsingService.prototype = {
>+        try {
>+          this.__prefs.deleteBranch("geo.wifi.access_token.");

s/__prefs/_prefs/

>+        } catch (ex) {}
>+


>diff --git a/dom/tests/mochitest/geolocation/test_geoPrivateBrowsing.html b/dom/tests/mochitest/geolocation/test_geoPrivateBrowsing.html

Please move this test to the privatebrowsing module.

Also, is there any reason why you wrote this as a mochitest?  I think it could easily be converted into an xpcshell test, which is quicker to run and hence preferred if the test doesn't touch the UI.
Attachment #408703 - Flags: review?(ehsan)
Attachment #408703 - Flags: review?(doug.turner)
Whiteboard: [has patch][needs review ehsan, dougt][geo] → [has patch][needs new patch][geo]
Whiteboard: [has patch][needs new patch][geo] → [needs new patch][geo]
Made the test an xpcshell test, never really done those before, but works just as well.
Attachment #408703 - Attachment is obsolete: true
Attachment #409960 - Flags: review?(ehsan)
Attachment #409960 - Flags: review?(doug.turner)
Whiteboard: [needs new patch][geo] → [geo] [needs review ehsan, dougt]
Attachment #409960 - Flags: review?(ehsan) → review+
Comment on attachment 409960 [details] [diff] [review]
address comments [checkin: comment 72]

>diff --git a/browser/components/privatebrowsing/test/unit/test_geoClearCookie.js b/browser/components/privatebrowsing/test/unit/test_geoClearCookie.js
>+ * ***** END LICENSE BLOCK ***** */

Please add a short comment here specifying what this test does (see the other unit tests in that directory for samples).

>+const accessToken = '{"location":{"latitude":51.5090332,"longitude":-0.1212726,"accuracy":150.0},"access_token":"2:jVhRZJ-j6PiRchH_:RGMrR0W1BiwdZs12"}'
>+function run_test() {

Something that we do in PB tests is to run each test twice, once with the js PB service and once with the C++ wrapper.  For this test, all you need to do is to rename run_test to run_test_on_service, use PRIVATEBROWSING_CONTRACT_ID instead of "@mozilla.org/privatebrowsing;1", and define a run_tests function like this:


// Support running tests on both the service itself and its wrapper
function run_test() {
  run_test_on_all_services();
}

Please see test_transition_nooffline.js for a real sample.

>+  if (!("@mozilla.org/privatebrowsing;1" in Components.classes)) {
>+    do_check_true(true);
>+    return;
>+  }

No need to do this, because if we're in this directory, then the PB service should exist (and we have tests which make sure it does!).

>+  var prefBranch = Components.classes["@mozilla.org/preferences-service;1"]
>+                             .getService(Components.interfaces.nsIPrefBranch2);
>+  var pb = Components.classes["@mozilla.org/privatebrowsing;1"]
>+                     .getService(Components.interfaces.nsIPrivateBrowsingService);

Use PRIVATEBROWSING_CONTRACT_ID like I said above, and also please stick to Cc and Ci.  These are all just nits to make this test look more like the rest of pb test suite.  :-)

r=me with the above nits addressed.

The rest should be a very quick and easy review for Doug.  Thanks for the patch, Natch!
Whiteboard: [geo] [needs review ehsan, dougt] → [geo] [needs review dougt]
Attachment #409960 - Flags: review?(doug.turner) → review+
http://hg.mozilla.org/mozilla-central/rev/c19d7df7b28a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago10 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [geo] [needs review dougt] → [geo] [needs 1.9.2 landing]
Target Milestone: Firefox 3.6a1 → Firefox 3.7a1
Ehsan, I didn't get a chance to address your nits from comment 71, and the cset you pushed doesn't seem to address them either...
(In reply to comment #73)
> Ehsan, I didn't get a chance to address your nits from comment 71, and the cset
> you pushed doesn't seem to address them either...

Oh, I'm so sorry I didn't pay enough attention...  Could you please attach a patch which addresses the nits based on the current trunk?  I'll push it and then push both patches combined on 1.9.2.
Done.
Attachment #409960 - Attachment description: address comments → address comments [checkin: comment 72]
Comment on attachment 411131 [details] [diff] [review]
update to comments

r=me.  Thanks!
Attachment #411131 - Flags: review+
Pushed both patches combined as http://hg.mozilla.org/releases/mozilla-1.9.2/rev/14191c0ab88d.
Whiteboard: [geo] [needs 1.9.2 landing] → [geo]
Updated roll-up patch for 1.9.1
Attachment #411233 - Flags: approval1.9.1.6?
Comment on attachment 411233 [details] [diff] [review]
refreshed for 191

Would like to see more testing/baking with nightly users, will have to catch this one next time.
Attachment #411233 - Flags: approval1.9.1.6? → approval1.9.1.7?
Comment on attachment 411233 [details] [diff] [review]
refreshed for 191

Approved for 1.9.1.8, a=dveditz for release-drivers
Attachment #411233 - Flags: approval1.9.1.8? → approval1.9.1.8+
You need to log in before you can comment on or make changes to this bug.