769285 - Support concurrent private/public geolocation wifi access tokens

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Josh Matthews [:jdm]]

Currently the PB service stores wifi access tokens in the preferences, and clears them on the private-browsing notification. We need to support storing private ones in memory and clearing that list on last-pb-context-exited.

Comment 1

[Josh Matthews [:jdm]]

Doug, what is the motivation behind clearing the access token on exiting PB mode? It seems like only the geolocation provider would ever see the token, and I don't really see why we would be worried about reusing a token obtained while in PB mode once PB mode is exited.

Comment 2

[Doug Turner (:dougt)]

We are and should be pretty aggressive about purging what settings get set in pb mode.


google are the folks that set this token.  So, the threat you could imaging is that you:

 enter pb
 go to maps.google.com
 use geo
 they set a geo provider cookie
 then you start searching for on google.com
 exit pb mode

Now if google was evil and violated their Privacy Policy, they could use the cookie set in PB mode with all of your searches.  We purge this cookie to make this threat impossible.

JUST TO BE CLEAR: this isn't happening today, and Google doesn't do this according to their ToS, and having this protection is just belts and suspenders.

Comment 3

[Saurabh Anand [:sawrubh]]

@Ehsan,Josh Can you refer https://bugzilla.mozilla.org/show_bug.cgi?id=769283#c1. Since this is on the same pattern as that other one, I'd like to take up this too, if Chris hasn't yet started working on it.

Thanks.

Comment 4

[Chris Lord [:cwiiis]]

(In reply to Saurabh Anand [:sawrubh] from comment #3)
> @Ehsan,Josh Can you refer
> https://bugzilla.mozilla.org/show_bug.cgi?id=769283#c1. Since this is on the
> same pattern as that other one, I'd like to take up this too, if Chris
> hasn't yet started working on it.
> 
> Thanks.

Hi Saurabh - I'm currently stuck working on some layout stuff and haven't looked at these yet. You can safely assume that if I've not commented on the bug, you can take it. Thanks!

Comment 5

[Josh Matthews [:jdm]]

Attachment: Use separate access tokens for private and public geolocation requests.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 664773 [details] [diff] [review]
Use separate access tokens for private and public geolocation requests.

>+GonkGPSGeolocationProvider::Watch(nsIGeolocationUpdate* aCallback, const bool& aPrivate)

Note to self: get rid of the cost ref before pushing.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 664773 [details] [diff] [review]
Use separate access tokens for private and public geolocation requests.

Review of attachment 664773 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/NetworkGeolocationProvider.js
@@ +141,5 @@
>      // check to see if we have an access token:
>      let accessToken = "";
>      try {
>        let accessTokenPrefName = "geo.wifi.access_token." + url;
> +      if (this.lastRequestPrivate)

I don't understand why you're using preferences when we are in private browsing mode.  It seems like you could just remember the access token in-memory.

::: dom/system/gonk/GonkGPSGeolocationProvider.cpp
@@ +499,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +GonkGPSGeolocationProvider::Watch(nsIGeolocationUpdate* aCallback, const bool& aPrivate)

does this idl change really translate into a |const bool& aPrivate|

::: xpcom/system/nsIGeolocationProvider.idl
@@ +53,2 @@
>     */
> +  void watch(in nsIGeolocationUpdate callback, in boolean requestPrivate);

bump the uuid.

Comment 8

[Josh Matthews [:jdm]]

I don't believe Fennec uses the network geolocation provider. Mark, can you confirm?

Comment 9

[Doug Turner (:dougt)]

fennec does *not*.

Comment 10

[(no longer active)]

Josh, can you drive this bug please?  Thanks!

Comment 11

[Josh Matthews [:jdm]]

Attachment: Use separate access tokens for private and public geolocation requests.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 684787 [details] [diff] [review]
Use separate access tokens for private and public geolocation requests.

Review of attachment 684787 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/src/nsPrivateBrowsingService.js
@@ +442,5 @@
>          // clear all auth tokens
>          let sdr = Cc["@mozilla.org/security/sdr;1"].
>                    getService(Ci.nsISecretDecoderRing);
>          sdr.logoutAndTeardown();
>      

extra white space

::: dom/system/NetworkGeolocationProvider.js
@@ +82,5 @@
> +  this.lastRequestPrivate = false;
> +
> +  let obsSvc = Cc["@mozilla.org/observer/service;1"]
> +                 .getService(Components.interfaces.nsIObserverService);
> +  obsSvc.addObserver(privateBrowsingObserver, "last-pb-context-exited", false);

Services.obs.addObserver ?

Comment 13

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7c45205a61a0

Comment 14

[Josh Matthews [:jdm]]

Backed out for android: https://hg.mozilla.org/integration/mozilla-inbound/rev/9659f41bda78

Comment 15

[(no longer active)]

(In reply to comment #14)
> Backed out for android:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9659f41bda78

Hmm, should you just add a requestPrivate argument to AndroidLocationProvider::Watch?

Comment 16

[Josh Matthews [:jdm]]

Yes. I also need to update an xpcshell test.

Comment 17

[Josh Matthews [:jdm]]

Attachment: Latest patch

Comment 18

[(no longer active)]

Attachment: Fixed patch

Comment 19

[Josh Matthews [:jdm]]

This patch needs to remove browser/components/privatebrowsing/test/unit/test_geoClearCookie.js, since it's broken and obsolete now.

Comment 20

[(no longer active)]

Attachment: per-window PB test

Comment 21

[Josh Matthews [:jdm]]

https://hg.mozilla.org/mozilla-central/rev/adefa1214235
https://hg.mozilla.org/mozilla-central/rev/739f20de3c1e

Comment 22

[Alex Lakatos[:AlexLakatos]]

(In reply to Josh Matthews [:jdm] from comment #19)
> This patch needs to remove
> browser/components/privatebrowsing/test/unit/test_geoClearCookie.js, since
> it's broken and obsolete now.
We've got a similar test under Mozmill automation:
http://hg.mozilla.org/qa/mozmill-tests/file/default/tests/functional/testPrivateBrowsing/testGeolocation.js
Does that mean ours is obsolete as well and should be removed?

Comment 23

[Josh Matthews [:jdm]]

Yes. There is no way for external tests to access the token from private geolocation requests.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

What do you mean with 'external tests'? Don't we expose it to any public interface?

Comment 25

[Josh Matthews [:jdm]]

No. It's a variable internal to a JS file that implements an XPCOM component.