535571 - Adapt Search.pm to accept limit and offset parameters

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Rosie]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.9.1.5) Gecko/20091105 Fedora/3.5.5-1.fc12 Firefox/3.5.5
Build Identifier: 

Search.pm should be adapted to accept 'limit' and 'offset' parameters to construct its query. Formerly the limit would have been constructed by the caller using the sql_limit method. 

Reproducible: Always

Comment 1

[Rosie]

Attachment: v1 (patch not working with tainted values)

This patch adds the functionality for untainted values. However I've been testing it for use with the webservice (see bug 477601) where values are tainted. The query string created with tainted values wont run which makes sense.

I just don't understand tainting and the possible security errors to carry on with this at the moment. If someone can point me in the right direction that'd be great but I didn't want to submit the patch for review knowing it didn't work with tainted values.

Comment 2

[Max Kanat-Alexander]

Hey Rosie! Here's the info you're looking for:

  http://perldoc.perl.org/perlsec.html#Laundering-and-Detecting-Tainted-Data

In Bugzilla, to assure that a variable is an integer, you use detaint_natural($var);

Comment 3

[Rosie]

Attachment: v2

Ah thanks Max!

Hopefully this should be ok. If you specify an offset without a limit or if the limit/offset don't pass detaint_natural it just silently fails and doesn't limit the query. Is this behaviour OK or should I throw an error?

Comment 4

[Max Kanat-Alexander]

Comment on attachment 418340 [details] [diff] [review]
v2

Hey Rosie.

Unfortunately, it's not safe to call detaint_natural on "undef"--you'll cause Perl to throw a warning.

Perhaps if somebody specifies an offset without a limit, we can set the limit to INT_MAX  (don't know how to figure that out in Perl, and it wouldn't matter anyhow since it's the databases's INT_MAX we care about, so perhaps we should just invent a really high number, or use the 32-bit INT_MAX) and let them offset the list.

Comment 5

[Max Kanat-Alexander]

Oh, and I nearly forgot--in order to get this patch checked in, it would be ideal to have a part of Bugzilla use it--perhaps move the limit code from buglist.cgi to using this.

Comment 6

[Rosie]

Attachment: v3

Thanks for the review Max

I've changed Search.pm to use INT_MAX (I've used POSIX is that right?). I've also made buglist.cgi use the limit which I can split into a separate patch/bug if it needs to be.

Hope you had a good Christmas and new year!

Comment 7

[Max Kanat-Alexander]

Comment on attachment 419900 [details] [diff] [review]
v3

This looks great, but I think I'd rather have it parse limit and offset out of $params (instead of having them as separate input parameters for new()), which is less implementation for all the clients of Search.pm.

Comment 8

[Max Kanat-Alexander]

Oh, and thanks for finding out that that constant was in POSIX.pm! I wanted to know that! :-)

Comment 9

[Rosie]

Attachment: v4

Cheers for the review. 

Having limit and offset in the $params makes total sense so here's the new patch!

Comment 10

[Max Kanat-Alexander]

Comment on attachment 420060 [details] [diff] [review]
v4

>+#my $offset = $cgi->param('offset') || undef;

  It looks like you no longer need that variable.

>+    if (defined($offset) and !defined($limit)){ $limit = INT_MAX; }

  Nit: The open-brace needs a space before it. (In two other places, also.)

  Otherwise this looks excellent, and these can be fixed on checkin. Thanks, Rosie! :-)

Comment 11

[Max Kanat-Alexander]

Hey Chris. Could we get a relicensed version of this patch also?

Comment 12

[Chris Eveleigh]

hi Max,

sorry for the delay - been up to my neck sorting out updates for http://infrastructure.independent.gov.uk .. all done now though.

so yes, i'll do that now...

Comment 13

[Chris Eveleigh]

Attachment: patch with "re-use statement" removed

here ya go.  should be able to respond quickly to any further requests!  also should have a bit more time available if you want any of our patches updated for head or with other fixes/updates.

:-)

Comment 14

[Max Kanat-Alexander]

Attachment: v6

Hey Chris! Thank you for posting this patch. Sorry that it took me so long to get to it, I've been caught up in a LOT of things and this fell by the wayside.

I've re-written the patch for the modern Search.pm, which was pretty heavily re-written since the last version of the patch. I did this myself since I'm currently the "module owner" of Search.pm, meaning that I can grant myself review on any code that I write for Search.pm.

Comment 15

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified buglist.cgi
modified Bugzilla/Search.pm
Committed revision 7704.