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

RESOLVED FIXED in Bugzilla 4.2

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: LpSolit, Assigned: LpSolit)

Tracking

({regression})

Bugzilla 4.2
regression
Bug Flags:
approval +
approval4.2 +
blocking4.2.1 +

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Depends on: 616341
Keywords: regression
(Assignee)

Comment 1

7 years ago
Created attachment 615048 [details] [diff] [review]
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.
Assignee: query-and-buglist → LpSolit
Status: NEW → ASSIGNED
Attachment #615048 - Flags: review?(mkanat)
Attachment #615048 - Flags: review?(dkl)
(Assignee)

Comment 2

7 years ago
Created attachment 615051 [details] [diff] [review]
patch, v1.1

Better patch: only set $sharer_id if $cgi->param('sharer_id') is set.
Attachment #615048 - Attachment is obsolete: true
Attachment #615048 - Flags: review?(mkanat)
Attachment #615048 - Flags: review?(dkl)
Attachment #615051 - Flags: review?(mkanat)
Attachment #615051 - Flags: review?(dkl)
(Assignee)

Updated

7 years ago
Flags: blocking4.2.1+
(Assignee)

Updated

7 years ago
Target Milestone: --- → Bugzilla 4.2
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.
(Assignee)

Comment 4

7 years ago
(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 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
Attachment #615051 - Flags: review?(dkl) → review+
(Assignee)

Comment 6

7 years ago
(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.
Flags: approval4.2+
Flags: approval+
(Assignee)

Updated

7 years ago
Attachment #615051 - Flags: review?(mkanat)
(Assignee)

Comment 7

7 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified buglist.cgi
modified Bugzilla/Search.pm
Committed revision 8200.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified buglist.cgi
modified Bugzilla/Search.pm
Committed revision 8076.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.