577329 - WebServices should filter email addresses same as the web UI as users are not always required to login

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[David Lawrence [:dkl]]

Attachment: Patch to filter login/email addresses in webservice data (v1)

Some webservices are available even to non-logged-in users and therefore email addresses should be filtered in the returned data. This is to make it more difficult to farm email addresses. I am attaching a patch to to use the email_filter() function in Bugzilla::Util whenever returning a user's login or email address.

Dave

Comment 1

[Reed Loden [:reed]]

Can't you also use Bugzilla::WebService::User to get e-mail addresses, as well?

Comment 2

[David Lawrence [:dkl]]

(In reply to comment #1)
> Can't you also use Bugzilla::WebService::User to get e-mail addresses, as well?

Not currently. You can call User.get as a non-logged-in user but you have to specify already the exact email address of the user to retrieve so you already know that information. When not logged in, you cannot retrieve any users based on user id or a wildcard match.

Dave

Comment 3

[Max Kanat-Alexander]

Comment on attachment 456319 [details] [diff] [review]
Patch to filter login/email addresses in webservice data (v1)

This is fine, but it will have to be documented.

It will also have to be relnoted, since in the case of Bug.get this changes the behavior of a stable API.

Comment 4

[Max Kanat-Alexander]

Because this is an API break and we froze yesterday, I'd like to target this to 4.2.

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to filter login/email addresses in webservice data (v2)

Updated patch for 4.2+ that filters more places and also adds items in POD history for the change.

dkl

Comment 6

[Max Kanat-Alexander]

I'm really not so keen on implementing this still, BTW. There are so many places that we have to track--imagine every place that emails can appear in %changes from update() returns, for example. I don't see a spammer risk here that's actually significantly mitigated by filtering (if they're going to code up a WS client, creating an account isn't a big deal, particularly since they can automate it using the WS client they wrote), and I would really like autocomplete to continue working for logged-out users.

Comment 7

[Max Kanat-Alexander]

So, given my arguments above, I still feel like this is a WONTFIX. LpSolit, do you agree?

Comment 8

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #7)
> So, given my arguments above, I still feel like this is a WONTFIX. LpSolit,
> do you agree?

I saw several people in other Bugzilla installations complaining that their email address is accessible by too many people. Of course, most of the complains come from installations which are still running Bugzilla 3.2 or older and so don't have the "don't display full email addresses to logged out users" feature. But I suppose that these same users would be irritated to know that despite they upgraded to 3.4 or newer, their email addresses are still accessible when using our WebServices. So I wouldn't say WONTFIX, but maybe a parameter to turn this feature on/off for our WebServices could make sense? This would be a single check in email_filter(). That's maybe a good compromise between breaking the API and keeping full email addresses away from logged out users.

(for some reason, I never got a bugmail with comment 7 in it. bmo is really buggy since yesterday)

Comment 9

[David Lawrence [:dkl]]

I agree with Frederic and like to add that currently it is inconsistent to not have email filtering in the WebServices when we already have it when viewing a web page when not logged in. Currently any anonymous user can farm email addresses by doing a simple search with Bug.search but cannot when doing the same with query.cgi. We should at least add a param check of some kind to email_filter to allow admins to turn the filtering on if they desire. There is currently not a param for allowing email addresses to be seen from web pages so maybe we should just do the same for webservices?

dkl

Comment 10

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #8)
> But I suppose that these same users would be irritated
> to know that despite they upgraded to 3.4 or newer, their email addresses
> are still accessible when using our WebServices. 

  I don't know, I wouldn't assume that. People are mostly concerned about their email addresses being exposed to spammers, and I'm not sure that this really exposes stuff to spammers.

> So I wouldn't say WONTFIX,
> but maybe a parameter to turn this feature on/off for our WebServices could
> make sense? This would be a single check in email_filter(). That's maybe a
> good compromise between breaking the API and keeping full email addresses
> away from logged out users.

  Okay. I do agree that the additional parameter wouldn't be a *lot* more complexity. But I do think that adding email_filter to the WS everywhere would itself be more complexity, and would put an additional maintenance and testing burden on us without actually being advantageous.

  I also don't really love the idea of having an API that acts that differently, and I don't love the idea of *ever* returning invalid user names to logged-out users who are using autocomplete.

Comment 11

[Max Kanat-Alexander]

> I agree with Frederic and like to add that currently it is inconsistent to
> not have email filtering in the WebServices when we already have it when
> viewing a web page when not logged in.

  I disagree, because the web pages are web pages, and the WebService is an API. One important difference here is that web pages are crawled for email addresses, and WebService APIs are not.

> Currently any anonymous user can farm
> email addresses by doing a simple search with Bug.search but cannot when
> doing the same with query.cgi.

  This is true, they could. However, if you're a spammer, what's a lower-effort attack vector--to create an account and use your pre-existing web crawler, or to write an entire Bugzilla WS client?

Comment 12

[Max Kanat-Alexander]

Comment on attachment 540453 [details] [diff] [review]
Patch to filter login/email addresses in webservice data (v2)

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

If we are going to implement this, then instead of this, add a new type called 'email' that generates a string that is email-filtered.

However, I really don't support doing poor autocompletes for logged-out users, and I'm still generally against this for the reasons above.

Comment 13

[David Lawrence [:dkl]]

Attachment: Patch to filter login/email addresses in webservice data (v3)

For your consideration. I implemented the way you proposed by creating a new webservice 'email' data type. Also I have added a parameter where the admin can turn on/off the filtering of email addresses. I put it in Advanced unless you can think of a better place for the param.

Thanks
dkl

Comment 14

[Reed Loden [:reed]]

Comment on attachment 577385 [details] [diff] [review]
Patch to filter login/email addresses in webservice data (v3)

>+  webservice_email_filter => 
>+    "Filter email addresses returned by the WebService API depending on"
>+    _ " if the user is logged in or not. This works similarly to how the"
>+    _ " web UI currentlyt filters email addresses."

currently*

Comment 15

[David Lawrence [:dkl]]

(In reply to Reed Loden [:reed] (very busy) from comment #14) 
> >+  webservice_email_filter => 
> >+    "Filter email addresses returned by the WebService API depending on"
> >+    _ " if the user is logged in or not. This works similarly to how the"
> >+    _ " web UI currentlyt filters email addresses."
> 
> currently*

Doh. Thanks I will fix that on checkin or the next patch depending on how many other problems are found.

dkl

Comment 16

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.

Comment 17

[Frédéric Buclin]

Comment on attachment 577385 [details] [diff] [review]
Patch to filter login/email addresses in webservice data (v3)

The patch doesn't apply cleanly:

patching file Bugzilla/WebService.pm
Hunk #1 succeeded at 13 with fuzz 2 (offset -7 lines).
Hunk #2 FAILED at 46.
patching file Bugzilla/WebService/Product.pm
Hunk #1 FAILED at 223.
patching file Bugzilla/WebService/User.pm
Hunk #1 succeeded at 160 (offset -7 lines).
Hunk #2 FAILED at 216.
Hunk #3 FAILED at 229.

Comment 18

[David Lawrence [:dkl]]

Attachment: Patch to filter login/email addresses in webservice data (v4)

Updated patch...

Comment 19

[Frédéric Buclin]

Comment on attachment 667492 [details] [diff] [review]
Patch to filter login/email addresses in webservice data (v4)

Looks good. A few things to fix, though.


>=== modified file 'Bugzilla/Config/Advanced.pm'

>+   name => 'webservice_email_filter', 

Maybe adding this parameter right below 'requirelogin' in "User Authentication" would make it more visible. This way, the admin can more easy understand the way they interact. If 'requirelogin' is enabled, then 'webservice_email_filter' plays no role because you must be logged in to interact with Bugzilla. So this new parameter is only meaningful when 'requirelogin' is off. I think its description should be updated accordingly, to remove some confusion.



>=== modified file 'Bugzilla/WebService.pm'

>+use Bugzilla::Util;

I don't understand why you call Bugzilla::Util from here as you don't alter the code at all in this file.



>=== modified file 'Bugzilla/WebService/Bug.pm'

You forgot to also fix _flag_to_hash() in this file:

    foreach my $field (qw(setter requestee)) {
        my $field_id = $field . "_id";
        $item->{$field} = $self->type('string', $flag->$field->login)
            if $flag->$field_id;
    }

Here, the type must be 'email' too.



>=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'

>+        $retval = Bugzilla::Util::email_filter($value);

Currently, this file has:

    use Bugzilla::Util qw(correct_urlbase trim disable_utf8);

If you were removing the list of subroutines from this line, you could simply call email_filter() directly, without prefixing it with Bugzilla::Util::.



>=== modified file 'template/en/default/admin/params/advanced.html.tmpl'

>+  webservice_email_filter => 
>+    "Filter email addresses returned by the WebService API depending on"
>+    _ " if the user is logged in or not. This works similarly to how the"
>+    _ " web UI currentlyt filters email addresses."

Besides the typo already reported by reed, please also make it clearer that this parameter is only useful if 'requirelogin' is off.


You must also fix the PARAMETERS section of the POD in WebService.pm to also list 'email' as a special type which returns a string, but whose result depends on the new parameter above.

Comment 20

[David Lawrence [:dkl]]

Attachment: Patch to filter login/email addresses in webservice data (v5)

Patch with suggested changes.

dkl

Comment 21

[Frédéric Buclin]

Comment on attachment 671629 [details] [diff] [review]
Patch to filter login/email addresses in webservice data (v5)

>=== modified file 'Bugzilla/WebService/Server/JSONRPC.pm'

>+        $retval = email_filter($value);

I just realized that email_filter() is not exported by default by Bugzilla::Util. There is no reason to not do it. Please add email_filter() to the export list of Bugzilla::Util, else the code above won't work.



>=== modified file 'Bugzilla/WebService/Server/XMLRPC.pm'

>+                $value = Bugzilla::Util::email_filter($value);

With email_filter() being exported by default as suggested above, you can remove the Bugzilla::Util:: prefix.



>=== modified file 'template/en/default/admin/params/auth.html.tmpl'

>+    " web UI currently filters email addresses. If <tt>requirelogin</tt>" _
>+    " is enabled then email addresses will never be filtered.",

Maybe I would reword it a bit to say that if requirelogin is enabled, then this parameter has no effect as users must be logged in to use Bugzilla. I think this is a bit clearer.


Otherwise looks good and works fine. r=LpSolit with these comments fixed on checkin.

Comment 22

[David Lawrence [:dkl]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunkmodified Bugzilla/Util.pm
modified Bugzilla/WebService.pm
modified Bugzilla/Config/Auth.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Product.pm
modified Bugzilla/WebService/User.pm
modified Bugzilla/WebService/Server/JSONRPC.pm
modified Bugzilla/WebService/Server/XMLRPC.pm
modified template/en/default/admin/params/auth.html.tmpl
Committed revision 8440.

Comment 23

[Frédéric Buclin]

Added to relnotes for 5.0rc1.

Comment 24

[Kohei Yoshino [:kohei]]

Will this API change be deployed to BMO production sometime soon?

Comment 25

[Simon Green]

(In reply to Kohei Yoshino [:kohei] from comment #24)
> Will this API change be deployed to BMO production sometime soon?

This is not the correct place to ask that question. Try #bmo on irc.mozilla.org. This bug is about the upstream Bugzilla product.

Comment 26

[David Lawrence [:dkl]]

(In reply to Kohei Yoshino [:kohei] from comment #24)
> Will this API change be deployed to BMO production sometime soon?

We have talk about it in the past and felt that it would impact too many people to just turn it on without proper warning. We are working on a plan on how to make sure we announce it properly and when to set the date to switch over.

Comment 27

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #26)
> (In reply to Kohei Yoshino [:kohei] from comment #24)
> > Will this API change be deployed to BMO production sometime soon?
> 
> We have talk about it in the past and felt that it would impact too many
> people to just turn it on without proper warning. We are working on a plan
> on how to make sure we announce it properly and when to set the date to
> switch over.

sorry for more bmo-specific chatter here, but this is incorrect.
we have no plans to enable this on bmo.
see bug 1078098 comment 6 (and the preceding comments).

Comment 28

[Kohei Yoshino [:kohei]]

(In reply to Byron Jones ‹:glob› from comment #27)
> we have no plans to enable this on bmo.
> see bug 1078098 comment 6 (and the preceding comments).

Awesome. Thanks!