saved search using using old form fields accepted, but results in "missing query"

RESOLVED FIXED in Bugzilla 2.18

Status

()

defect
RESOLVED FIXED
16 years ago
7 years ago

People

(Reporter: ajschult784, Assigned: gerv)

Tracking

unspecified
Bugzilla 2.18
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment)

Reporter

Description

16 years ago
I had a script that created queries and would then construct a GET URL that
would save the search in bugzilla.mozilla.org.  The script still seems to work
and the resulting saved search is placed in the footer, but attempting to use it
results in an "Missing Query" error page that states that 

"The query named foo does not exist."

A sample URL to save such an invalid query is:
http://bugzilla.mozilla.org/buglist.cgi?cmdtype=doit&bug_id=232368&remember=1&remtype=asnamed&newqueryname=foo

the old syntax should either be rejected in the first place or result in a valid
saved search.
Indeed.  Although we don't guarantee our API won't change, and arguably don't
need to warn anyone when it does or convert old-style links to new-style ones,
the behavior here is strange.  When the "newquery" parameter is unspecified,
Bugzilla creates a row for the search in the "namedqueries" table with the
"newquery" field set to an empty string.  Then, when you try to run the search,
Bugzilla complains that the search doesn't exist.

We don't allow users to run searches without terms, so Bugzilla shouldn't allow
users to save such a search.  For robustness, however, it also shouldn't say
that a search without terms doesn't exist when it clearly does.  Bugzilla should
both prevent users from saving termless searches and run any existing termless
searches, displaying the default "buglist_parameters_required" user error message.

Optionally, it could also hunt for search terms in the URL when the "newquery"
parameter is unspecified.
Assignee: justdave → gerv
The issue here is the difficulty of easily detecting searches without terms. The
URL can have any old garbage in, so you can't look at its length. And we have no
canonical list of which parameters in it are search-restricting ones and which
aren't.

We currently protect solely against buglist.cgi getting called without any
parameters; but you can still get a list of all bugs in the DB simply by
appending ?a=b or even ?debug=1.

We could change the error message; the current one comes from this line of code:
    $result || ThrowUserError("missing_query", {'queryname' => $name});
If we used "defined($result)" instead of just $result, that would avoid the
error message, but then presumably it would run the query.

Myk: thoughts?

Gerv
>The issue here is the difficulty of easily detecting searches without terms.

In this case it's just about the easily-detected variety, not all possible
termless queries, as the queries saved and then not found by this bug are the
empty string.

>We could change the error message; the current one comes from this line of
>code:
>    $result || ThrowUserError("missing_query", {'queryname' => $name});
>If we used "defined($result)" instead of just $result, that would avoid
>the error message, but then presumably it would run the query.

Right, but if you add a check for emptiness you can deal with that, i.e.:

defined($result) || ThrowUserError("missing_query", {'queryname' => $name});
$result
  || ThrowUserError("buglist_parameters_required", {'queryname' => $name});

Then do the same thing when the user is saving a query and newquery is undefined
or blank.
Posted patch Patch v.1Splinter Review
Myk, is this what you wanted? The wording change is to make it more general;
"url" and "link" weren't being used so I removed those too.

Gerv
Attachment #142613 - Flags: review?(myk)
Attachment #142613 - Flags: review?(justdave)
Attachment #142613 - Flags: review?(justdave) → review+
Flags: approval?
Comment 4 asks a direct question of Myk, which he hasn't answered yet.  I'm
interested in his opinion, so I'll wait for his review.
Flags: approval?
Comment on attachment 142613 [details] [diff] [review]
Patch v.1

Yup, this looks good and behaves well (it doesn't let you create an empty
query, but if you have an empty query it doesn't let you run it). r=myk
Attachment #142613 - Flags: review?(myk)
Flags: approval+
Fixed.

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.246; previous revision: 1.245
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.50; previous revision: 1.49
done

Gerv
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → Bugzilla 2.18

Comment 8

16 years ago
Comment on attachment 142613 [details] [diff] [review]
Patch v.1

>Index: buglist.cgi
> 
>+        $::FORM{'newquery'} || ThrowUserError("buglist_parameters_required", 
>+                                              

Why are we adding more uses of $::FORM to callsites?

>+    You may not search, or create saved searches, without any search terms.

That's an awful lot of commas there :-)
> You may not search, or create saved searches, without any search terms.

This is the right number of commas. "or create saved searches" is a subordinate
clause. Remove it, and the sentence still makes sense. Hence two commas.

Gerv
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.