1011343 - sync logs contain FxA email address

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

With the change to FxA, we now log the email address of the user:

1400141271123	Sync.Service	INFO	Logging in user [e-mail]

(but with the real email address).  In the old sync we just logged a hash of the username (the hash is what is stored as the username, so no special action was taken to log the hash).

Given this is PII, it seems we should not log this.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Is this a "regression" from bug 1006943 then?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp (email [e-mail]) from comment #1)
> Is this a "regression" from bug 1006943 then?

Nope - it's been doing that since FxA landed.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Since it's not a regression and we're already getting close to the end of Beta for FF30, I'm going to untrack this and let the backlog do it's job. We can look at a nomination for uplift when ready and make the decision then for how high to take it at that time based on where we're at in the cycle.

Comment 4

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #0)
> With the change to FxA, we now log the email address of the user:
> 
> 1400141271123	Sync.Service	INFO	Logging in user [e-mail]
> 
> (but with the real email address).  In the old sync we just logged a hash of
> the username (the hash is what is stored as the username, so no special
> action was taken to log the hash).
> 
> Given this is PII, it seems we should not log this.

What's the desired endstate here? Should we log a hash (is there still a hash), or replace it with e.g. [e-mail], or something else?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #4)

> What's the desired endstate here? Should we log a hash (is there still a
> hash), or replace it with e.g. [e-mail], or something else?

The desired state is the correct balance between "no PII" and "enough debug info to be useful" - beyond that it's up to you ;)  (But to be helpful, I'd say completely omitting it is fine - we could just ask the user for their email addy)

Comment 6

[:Gijs (he/him)]

So... what are the STR here? I spent the last 30 minutes trying to understand the log level goop and which of the two (both?) logs that match the string from comment #0 are involved here, but I've not managed to get it to show up. If I can't get this logged, I also can't really check that I've fixed it...

I've enabled logOnSuccess, and I synced several times, but neither the error nor the success log seem to have any login info. I've also tried to replace some of the seemingly relevant "Debug" prefs with "Info" (.identity, .status, .authentication, ...), to no avail.

Comment 7

[:Gijs (he/him)]

OK, so it seems for the log level prefs to take effect, you need a restart, and the tps/ thing isn't built by default, so I just need to change the other instance. Patch coming up.

Comment 8

[:Gijs (he/him)]

Attachment: don't log username in sync log,

Comment 9

[:Gijs (he/him)]

FWIW, the other case that I noticed was http://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/tps/resource/auth/sync.jsm#65 but I don't know by whom / under what circumstances / how that gets called.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8430733 [details] [diff] [review]
don't log username in sync log,

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

Thanks!

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #9)
> FWIW, the other case that I noticed was
> http://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/
> tps/resource/auth/sync.jsm#65 but I don't know by whom / under what
> circumstances / how that gets called.

TPS is yet-another-test-environment.  Sync is full of surprises ;)

Comment 12

[Richard Newman [:rnewman]]

(In reply to :Gijs Kruitbosch from comment #6)

> I've enabled logOnSuccess, and I synced several times, but neither the error
> nor the success log seem to have any login info. I've also tried to replace
> some of the seemingly relevant "Debug" prefs with "Info" (.identity,
> .status, .authentication, ...), to no avail.

FWIW, you want to bump those prefs up to "Trace", not down to "Info". And, as you discovered, restart Firefox.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/75131819a6fe

Comment 14

[:Gijs (he/him)]

Does this want uplifting, Mark?

Comment 15

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gijs Kruitbosch from comment #14)
> Does this want uplifting, Mark?

I think so, yeah.  Thanks!

Comment 16

[:Gijs (he/him)]

Comment on attachment 8430733 [details] [diff] [review]
don't log username in sync log,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new sync
User impact if declined: we log personally identifiable information to disk if the user toggles a hidden pref
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c0a148e23e67

Comment 18

[Marco Mucci [:MarcoM]]

Hi Tracy, will it be possible to have this bug verified by end of day this Friday?  Our current Iteration ends on Monday June 9.

Comment 19

[Tracy Walker [:tracy]]

(In reply to Marco Mucci [:MarcoM] from comment #18)
> Hi Tracy, will it be possible to have this bug verified by end of day this
> Friday?  Our current Iteration ends on Monday June 9.

Absolutely, I'll look at it today, if possible.  

But otherwise leaving myself needinfo'd as a reminder.

Comment 20

[Tracy Walker [:tracy]]

I no longer see the log line with email addy in latest nightly. I had seen it with logs prior to the landing.