1058786 - Review Board shouldn't leak my full email address

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect, P1)

Description

[Ed Morley [:emorley]]

See:
https://reviewboard-dev.allizom.org/users/

As well as the per-user profile page.

Presuming an upstream issue, however IMO this still needs to be fixed before rolling it out more at Mozilla.

Comment 1

[Mark Côté [:mcote]]

Hm yeah, this would be a side effect of using email addresses as usernames in Review Board, which is not what it expects.

Ugh, this is going to be rough to fix, since the username is also clearly visible in the People field and when you hover over the submitter.

I am tempted to just prevent anonymous access, at least for launch.  In general our attempts at not publicly displaying email addresses on Bugzilla don't even make sense, given how easy it is to create an account, and since anyone committing code has their address visible on hg.m.o.  Sigh.

Changing this to block initial deployment.

Comment 2

[Ed Morley [:emorley]]

It's more the google search indexer and non-authenticated spammer email harvesters case that would be good to stop :-)

Comment 3

[Ed Morley [:emorley]]

(I found this page was leaking email addresses after googling my moco email; hg.m.o pages aren't indexed)

Comment 4

[Hal Wine [:hwine] use NI!]

Good point about indexing - likely doesn't make sense to crawl this server, and we sure don't need the extra load.

Comment 5

[Mark Côté [:mcote]]

In addition to preventing indexing, I think the full, proper solution here would be

* disable the user list for anonymous users
* disable profile pages for anonymous users
* display full names, not usernames, in the People field and anywhere else that usernames are currently displayed (I think this makes more sense from a UI perspective, but maybe it's tricky to list them properly)
* disable mouseovers for anonymous users

Sadly, I guess we'll have to do this before launch unless we turn off anonymous access altogether.

Comment 6

[Kendall Libby [:fubar] (he/him)]

I've updated robots.txt on staging, and dev to disallow indexing; prod will get it when puppet runs next.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Other short-term solution ideas:

1) Automatically set each profile to be "private", meaning that email addresses and full names are not visible for anyone except the user themselves and administrators. This is a field that is available under "Profile" in a user's account settings, but we could try to find a way of setting that instance-wide for all current and new accounts.
2) Re-assign usernames to not match email addresses. Not sure what we'd choose there.

For (2), users can still log in with their email addresses, but this just prevents it from being displayed as the user's monicker.

Comment 8

[Mark Côté [:mcote]]

(In reply to Mike Conley (:mconley) from comment #7)
> Other short-term solution ideas:
> 
> 1) Automatically set each profile to be "private", meaning that email
> addresses and full names are not visible for anyone except the user
> themselves and administrators. This is a field that is available under
> "Profile" in a user's account settings, but we could try to find a way of
> setting that instance-wide for all current and new accounts.
> 2) Re-assign usernames to not match email addresses. Not sure what we'd
> choose there.

I think we'd have to do both.  I've thought about this before; the main sticking point is that usernames are displayed as in the Reviewers|People field, unlike the Submitter field or at the top of the review sections.  However, mconley suggests that we could do away with that field entirely and just rely on the new reviewer fields in the Commits tab, which will eventually be integrated into the main view.

Since you can't change usernames in Review Board, they have to be set when the user is created, so using the Bugzilla user ID is probably a good choice, since they should be largely hidden.

Finally, the privacy setting actually seems to do roughly what we want.  Your real name and email address aren't displayed on your profile, but your real name *is* in the Submitter fields and in reviews.  I was worried that would be replaced with a useless number for unauthed users, but this is more like how Bugzilla works.  The only downside is that your email is never visible, even to logged-in users.  Ideally your email would only be hidden from anonymous users.  Still, this seems like the best solution, and the least amount of work.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Review for review ID: bz://1058786/mconley

/r/129 - rbmozui: Bug 1058786 - Remove TargetPeople field from review request. r=?

Pull down this commit:

hg pull review -r 939b1e2e6aa19dc1f7bba0913c1015d29016090e

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Let me know if you want me to redirect the review request.

Comment 11

[Mark Côté [:mcote]]

::: pylib/rbmozui/rbmozui/extension.py
(Diff revision 1)
> -        if (field):
> +        if (testing_done_field):

I know this was already in here, but is there some reason for the parens (and below in the new code)?

Comment 12

[Mark Côté [:mcote]]

Regardless, works properly for me.  Just need to change the commits tab to display full names instead of usernames, and then default all profiles to private upon creation, and we should be good for now.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

> I know this was already in here, but is there some reason for the parens (and below in the new code)?

Just my JS habits acting up. Thanks, fixed.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8479335 [details]
Review for review ID: bz://1058786/mconley

Checked in Part 1: https://hg.mozilla.org/hgcustom/version-control-tools/rev/a32509c60fb8

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8479335 [details]
Review for review ID: bz://1058786/mconley

/r/129 - rbbz: Bug 1058786 - Part 2: Bugzilla-linked accounts should use Bugzilla user IDs for Review Board usernames. r=?
/r/152 - rbbz: Bug 1058786 - Part 3: Bugzilla-linked accounts should be private by default. r=?

Pull down these commits:

hg pull review -r 1b9944332637523ec18ee8b3b1955d8fc6518cc9

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Unfortunately, I've made this needlessly complicated by just using the same bug 1058786 for all this stuff. :/ So I re-opened the old review request, and now I've got a bit of a history mess on my hands. Oh well.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Ohh, also note - I haven't actually tested this code, because I'm locked out of Bugzilla-dev, it seems. I'll try again on Monday.

Comment 18

[Mark Côté [:mcote]]

r+ by inspection.  As mentioned we don't really need the bugzilla user ID mapping table anymore, but that can be removed separately.  No harm leaving it for now.

Comment 19

[Mark Côté [:mcote]]

r+ by inspection.  As mentioned we don't really need the bugzilla user ID mapping table anymore, but that can be removed separately.  No harm leaving it for now.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Tested this locally for bugzilla-dev, and this seemed to do the right thing.

One drawback is now we have numbers for all of the usernames, which will show up in various places - like the "submitter" column in the dashboard. We'll probably need a new grid instead of the traditional dashboard, or put in a new default column that shows the fullname.

Thanks for the reviews!

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Part 2 and Part 3 landed:

https://hg.mozilla.org/hgcustom/version-control-tools/rev/f947c413e405
https://hg.mozilla.org/hgcustom/version-control-tools/rev/3a3552e26d3c

Comment 22

[Mike Conley (:mconley) (:⚙️)]

mcote: we should probably spin a release of rbbz and deploy onto reviewboard-dev to test these out, no? That way, we can see where else email addresses might leak.

Comment 23

[Mark Côté [:mcote]]

rbbz 0.1.20 and rbmozui 0.2.2alpha0 uploaded to pypi.  fubar will be upgrading them and review board itself to 2.0.6.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

mcote:

smacleod and I have developed a new plan for dealing with leakage like this. Tell us what you think of this.

Instead of monkeypatching Review Board, special-casing all over the place, etc., or switching to weird unmeaningful username values like Bugzilla IDs, we completely lock down Review Board and prevent outside access if not logged in.

HOWEVER

We make it so that if you follow a link that would normally take you to a squashed review request, we have Review Board intercept that request, notice that you're not logged in, and redirect you to hgweb for that commit / diff / commit range (corresponding to the Review Board review request object you were linked to).

So, for example, suppose I'm some squirrely student that just showed up all excited about Mozilla. This student has no Bugzilla account, but they're browsing the issue tracker, and they're pretty stoked. They see a bug that sounds interesting. It's one of gps's bugs, so there are about 12 commits in the squashed review request. They click on the link in Bugzilla to go to the review request. Review Board intercepts, notices that they're not logged in, and redirects them to hgweb for the reviews repository, showing them the set of commits for gps's push.

"Wow", the student says, "this gps guy works hard. Let's look at some more bugs". They then looks at a bug in Bugzilla that just has a review request linking to just a single diff. They click on it. Review Board intercepts, notices they're still not logged in, and also that there's just a single diff in the review request. They are then sent to hgweb for the reviews repository for that single commit.

The student is also idling in IRC, and sees smacleod send mconley a link to an individual commit review request. The student, out of curiosity, clicks the link, and Review Board intercepts it, notices that the link is for a single commit of part of a larger set of pushes, and links to the reviews repo's hgweb for that individual commit.

End result:

Student is able to view the source using a not so bad UI. All email addresses are hidden (except for the person who pushed, since their email address is recorded in the push to hg...but that's just how it is).

If anybody is logged in, of course, those above links will connect to the right review request as we currently do.

What do you think of this solution?

Comment 25

[Mark Côté [:mcote]]

Damn.  That's got some proper thinking in it.  Let me mentally digest overnight, but this sounds very workable.

Comment 26

[Mark Côté [:mcote]]

Can't think of any problems with this approach.  Ship it!

My suggested plan would be

1. Lock down reviewboard-dev right now, since everyone using it right now logs in.
2. Revert the username changes & deploy a new rbbz.  We'll just run some SQL statements on the server rather than hacking in some undo code into rbbz.
3. Work on the hgweb integration.

Thanks again for the suggestion.  This is a bit weird but overall way saner.

Comment 27

[Steven MacLeod [:smacleod]]

(In reply to Mark Côté [:mcote] from comment #26)
> 1. Lock down reviewboard-dev right now, since everyone using it right now
> logs in.
Done.

FYI: The configuration option is at https://reviewboard-dev.allizom.org/admin/settings/authentication/ , unchecked "Allow anonymous read-only access".

> 2. Revert the username changes & deploy a new rbbz.  We'll just run some SQL
> statements on the server rather than hacking in some undo code into rbbz.

Would you be up to taking care of this Mark?

Comment 28

[Steven MacLeod [:smacleod]]

Anonymous access has also been turned off for reviewboard.allizom.org and reviewboard.mozilla.org

Comment 29

[Gregory Szorc [:gps]]

Requiring a login to access review information seems like a very unMozillian thing to do. This is a regression from Bugzilla reviews, where anybody can read review comments on public bugs.

Sure, the implementation today is that RB mirrors comments back to Bugzilla. So we technically don't have an information availability regression. However, I've been asserting and will continue to assert that the requirement that RB reviews reference a Bugzilla bug is not necessary and will eventually go away. At that point, having a locked down RB would be an actual regression in information availability. For that reason, I would insist we have a long-term story for reopening ReviewBoard to anonymous viewing.

Can we not modify ReviewBoard's templates to not render email address for non-logged in users? Is that the long term plan?

Comment 30

[Mike Conley (:mconley) (:⚙️)]

(In reply to Gregory Szorc [:gps] from comment #29)
> Requiring a login to access review information seems like a very unMozillian
> thing to do. This is a regression from Bugzilla reviews, where anybody can
> read review comments on public bugs.
> 
> Sure, the implementation today is that RB mirrors comments back to Bugzilla.
> So we technically don't have an information availability regression.
> However, I've been asserting and will continue to assert that the
> requirement that RB reviews reference a Bugzilla bug is not necessary and
> will eventually go away. At that point, having a locked down RB would be an
> actual regression in information availability. For that reason, I would
> insist we have a long-term story for reopening ReviewBoard to anonymous
> viewing.
> 
> Can we not modify ReviewBoard's templates to not render email address for
> non-logged in users? Is that the long term plan?

Review Board actually defaults to not rendering email addresses for non-logged in users. The problem is that our current approach sets the Review Board username to be the Bugzilla user's email address. So everywhere where we show a username, we show an email address instead. Review Board doesn't know that these are email addresses - that's just the username selection we made.

The current approach on reviewboard-dev is to use the Bugzilla user ID number as the username... but that really doesn't do much for readability or meaningfulness.

Comment 31

[Mark Côté [:mcote]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> (In reply to Gregory Szorc [:gps] from comment #29)
> > Can we not modify ReviewBoard's templates to not render email address for
> > non-logged in users? Is that the long term plan?
> 
> Review Board actually defaults to not rendering email addresses for
> non-logged in users. The problem is that our current approach sets the
> Review Board username to be the Bugzilla user's email address. So everywhere
> where we show a username, we show an email address instead. Review Board
> doesn't know that these are email addresses - that's just the username
> selection we made.

To clarify, this is from the fact that Bugzilla's idea of a username *is* the email address, i.e., there is no separate username field.  There's not really any way that I can think of to generate a unique username automatically, aside from using the Bugzilla user ID, which I fully admit is not particularly useful.

The other option I've thought up would be to ask the user for a username when they first log onto Review Board.  Most people would use their (unique) IRC nick, I imagine, which would be nice, since we've just hacked that into Bugzilla with the :nick convention.  One problem is that you don't need to be logged into Review Board to be referenced--when you choose someone for review, you are querying the Bugzilla database, and users are automatically inserted into Review Board's user db if they aren't already there.  However, we could set the username to the user ID temporarily and have the user reset it to something useful when they log in to look at the review.  I originally dismissed this as being too much work, but maybe it's less work than the hgweb solution.

Comment 32

[Gregory Szorc [:gps]]

Oy.

I do insist we have a plan for eventually opening RB to unauthenticated reading.

I'm fine with sharing Bugzilla's auth database indefinitely. I think it is silly to create yet more user/auth data in ReviewBoard.

I'd support a username field in ReviewBoard on first login. But I think that data belongs in Bugzilla's database. Is it too much work for Bugzilla to grow a "public username/alias" field for each user that we can then use for RB's primary user identifier?

(I do hate asking this. I know the last thing we want at this point is scope bloat. But I think we might be digging ourselves into a huge hole if we launch RB with usernames being email address. Could we migrate later?)

Comment 33

[Mark Côté [:mcote]]

Attachment: Review for review ID: bz://1058786/mcote

Comment 34

[Mark Côté [:mcote]]

/r/199 - Bug 1058786 - Set meaningful username.

Pull down this commit:

hg pull review -r c83a978a74d1ce4fa729d553bd444b1cc68a3022

Comment 35

[Mark Côté [:mcote]]

Comment on attachment 8499954 [details]
Review for review ID: bz://1058786/mcote

Note that this *does* allow logging in with Review Board username.  Review Board does a bit of magic before calling the auth backend's authenticate() method.  It searches the email field, and if it finds a hit, it passes the username to the authenticate() method instead of the email address the user entered.  So we search for the given username, and, if found, user the user's email address to authenticate with Bugzilla.  If we don't get a hit, we presume it's a raw email address and pass that to Bugzilla.

I *think* this is fine... if not, we may need to patch Review Board core.

I have a script to migrate an existing db; I'll post that as a separate review.

Comment 36

[Mark Côté [:mcote]]

Attachment: fix_usernames.py

This script will migrate all existing users to the new scheme.

Comment 37

[Mark Côté [:mcote]]

Thinking about this more, in order to avoid having to put in a special UI, we might as well update username when we read a user from BMO, like we do with real name or email address.  If the user has changed the :ircnick part of their real name, then their username will be updated.

Note that user info is only updated when (a) a user logs into Review Board or (b) is returned by a user query (e.g. when someone is setting a reviewer).  We should fix this some how, eventually, through a cron job or something that runs periodically on page load or whatever, but for now, I'm comfortable just marking this as a known issue and requiring people to log out and back in again (or search for themselves) if they want a change to their BMO user info reflected in RB.

Comment 38

[Mark Côté [:mcote]]

/r/199 - Bug 1058786 - Set meaningful username.

Pull down this commit:

hg pull review -r 2f28e06981ca497e99ca919c2690801dde9abb10

Comment 39

[Mark Côté [:mcote]]

Attachment: fix_usernames.py

Updated with better irc-nick regular expression, which matches all valid characters (and notably doesn't include +, so we don't have to worry about someone clobbering someone else's placeholder).

Comment 40

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/198/#review152

Looks good for the most part, just a couple small nits.

::: pylib/rbbz/rbbz/auth.py
(Diff revision 2)
> -        if username.isdigit():
> +        # If a user logs in with an email address that is associated with
> +        # an existing user, then 'username' will contain that user's
> +        # username.  Otherwise, 'username' will contain whatever was
> +        # entered, which may be the Bugzilla login (email address) for a
> +        # user who does not yet have an entry in the Review Board user
> +        # table.

This comment is a little confusing and ambiguous with all the "user" and "username". Maybe something like:

        # If the user provides an email address when authenticating,
        # it is checked againsts Review Board's email field in the User
        # Model. If a match is found, the email will be translated into
        # the username field before being passed into this method's
        # 'username' argument.
        #
        # If a match is not found, 'username' will contain whatever was
        # entered, which may be the Bugzilla login (email adress) for a
        # user who does not yet have an entry in the Review Board database

::: pylib/rbbz/rbbz/models.py
(Diff revision 2)
> +BZ_IRCNICK_RE = re.compile(':([-0-9A-}]+)')

Just going to trust you that this is correct.

::: pylib/rbbz/rbbz/models.py
(Diff revision 2)
> +            except:

Can we catch only the exception where the username is not unique here?

::: pylib/rbbz/rbbz/models.py
(Diff revision 2)
> +                except:

Same here

::: pylib/rbbz/rbbz/models.py
(Diff revision 2)
>          else:

I've never seen Try..Except..Else in any other Python code... but it's exactly what we want here.

Comment 41

[Steven MacLeod [:smacleod]]

Comment on attachment 8501281 [details]
fix_usernames.py

Looks fine as a one off script. Since we're not touching the bugzilla user map we *should* be able to repair anything if there is a bug in the script.

Ship-it!

Comment 42

[Mark Côté [:mcote]]

(In reply to Steven MacLeod [:smacleod] from comment #40)
> https://reviewboard-dev.allizom.org/r/198/#review152
> 
> Looks good for the most part, just a couple small nits.
> 
> ::: pylib/rbbz/rbbz/auth.py
> (Diff revision 2)
> > -        if username.isdigit():
> > +        # If a user logs in with an email address that is associated with
> > +        # an existing user, then 'username' will contain that user's
> > +        # username.  Otherwise, 'username' will contain whatever was
> > +        # entered, which may be the Bugzilla login (email address) for a
> > +        # user who does not yet have an entry in the Review Board user
> > +        # table.
> 
> This comment is a little confusing and ambiguous with all the "user" and
> "username". Maybe something like:
> 
>         # If the user provides an email address when authenticating,
>         # it is checked againsts Review Board's email field in the User
>         # Model. If a match is found, the email will be translated into
>         # the username field before being passed into this method's
>         # 'username' argument.
>         #
>         # If a match is not found, 'username' will contain whatever was
>         # entered, which may be the Bugzilla login (email adress) for a
>         # user who does not yet have an entry in the Review Board database

Thanks, yeah, that is clearer.

> ::: pylib/rbbz/rbbz/models.py
> (Diff revision 2)
> > +            except:
> 
> Can we catch only the exception where the username is not unique here?
> 
> ::: pylib/rbbz/rbbz/models.py
> (Diff revision 2)
> > +                except:
> 
> Same here
> 

I really wanted to--I hate blanket excepts--but there doesn't seem to be a db-independent way to do this, e.g., a completely different exception will be raised if you're using sqlite versus MySQL.  I did a bit of searching (although admittedly not much) and didn't come across any good solutions.  Ideas more than welcome. :)

> ::: pylib/rbbz/rbbz/models.py
> (Diff revision 2)
> >          else:
> 
> I've never seen Try..Except..Else in any other Python code... but it's
> exactly what we want here.

:) It's almost as awesome as for..else.

Comment 43

[Mark Côté [:mcote]]

Something strange going on here.  The Django docs[1] say that django.db.IntegrityError should catch all the various db-specific integrity-error exceptions, but it doesn't for me, at least in sqlite.  I'll leave the blanket excepts, with a comment, for now, and maybe file a bug with Django.

[1] https://docs.djangoproject.com/en/1.6/ref/exceptions/#database-exceptions

Comment 44

[Mark Côté [:mcote]]

I did a version bump as well, to 0.2.  Still would like to solve the exception problem, but if it's not a real error with a duplicate username, the second save() call in the exception handler should still fail, so we don't die silently.

Releasing a new rbbz very shortly.

http://hg.mozilla.org/hgcustom/version-control-tools/rev/3e2c0365ced9

Comment 45

[Mark Côté [:mcote]]

Regex allows characters not supported by Review Board; will have a fix shortly.

Comment 46

[Mark Côté [:mcote]]

Attachment: Fixed ircnick regex

This should match Review Board's username regex.

Comment 47

[Mark Côté [:mcote]]

Attachment: fix_usernames.py (v2)

Comment 48

[Steven MacLeod [:smacleod]]

Comment on attachment 8504846 [details] [diff] [review]
Fixed ircnick regex

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

::: pylib/rbbz/rbbz/models.py
@@ +12,5 @@
> +# Anyone with those in their :ircnicks will have them truncated at the last
> +# legal character.  Not great, but we can later implement a UI for letting
> +# people change their usernames in Review Board.
> +BZ_IRCNICK_RE = re.compile(':([A-Za-z0-9_\-\.]+)')
> + 

Trailing Whitespace

Comment 49

[Mark Côté [:mcote]]

http://hg.mozilla.org/hgcustom/version-control-tools/rev/669991cb8243

Comment 50

[Mark Côté [:mcote]]

https://reviewboard-dev.allizom.org/r/198/#review162

> Just going to trust you that this is correct.

And apparently it wasn't. :)

(This comment also serving to test that review replies are now working correctly.)

Comment 51

[Gregory Szorc [:gps]]

The hidden reviewer part of this will get backed out in bug 1086986.