745320 - Shared queries do not work when tags are part of the query

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Frédéric Buclin]

Someone reported in the support mailing-list that if you create a saved search based on one of your tags and share this saved search with other users, then the other users won't get any result because the searching system will look for this tag in their list of tags. If sharer_id is passed in the URL and the user running the shared search is allowed to run it, then Search.pm should look for this tag in the original user's list of tags.

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

I defined a new 'sharer' key passed to Search->new which is only set if LookupNamedQuery() says it's fine for the current user to run this query. This way, it's not possible for a crafted query to try to set the sharer itself.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1.1

Better patch: only set $sharer_id if $cgi->param('sharer_id') is set.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 615051 [details] [diff] [review]
patch, v1.1

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

I'm hesitant about this. I'm concerned that somebody is going to mis-use "sharer" in the future, because validation has to happen outside of Search.pm. Perhaps Search.pm itself should be validating that this user can see the query.

Comment 4

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #3)
> Perhaps Search.pm itself should be validating that this user can see the query.

Search.pm doesn't know that the query it is building is a saved search. The reason I passed 'sharer' as a separate argument is to prevent someone from hacking the URL to define it. By default, we don't pass 'sharer' to Search.pm (buglist.cgi is the single place to use it), so this forces the developer to add it explicitly if he wants to use this argument.

Comment 5

[David Lawrence [:dkl]]

Comment on attachment 615051 [details] [diff] [review]
patch, v1.1

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

Looks good and fixes the issue in my testing. r=dkl

Comment 6

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #3)
> I'm hesitant about this. I'm concerned that somebody is going to mis-use
> "sharer" in the future

dkl made a good point on IRC: somebody can already mis-use "user", which is much more critical as $self->_user is used everywhere within Search.pm to determine privileges and group permissions. So "sharer" is not more vulnerable to this mis-use than "user".

One improvement could be to also pass the name of the shared search to Search->new and let it re-check that the user can really access this shared search. But that's IMO overkill for now, and definitely something for a separate bug, as an enhancement.

Comment 7

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified buglist.cgi
modified Bugzilla/Search.pm
Committed revision 8200.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified buglist.cgi
modified Bugzilla/Search.pm
Committed revision 8076.