Closed Bug 535571 Opened 11 years ago Closed 10 years ago

Adapt Search.pm to accept limit and offset parameters

Categories

(Bugzilla :: Query/Bug List, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 4.2

People

(Reporter: rosie.clarkson, Assigned: mkanat)

References

Details

Attachments

(1 file, 5 obsolete files)

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
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.
Blocks: 477601
Assignee: query-and-buglist → rosie.clarkson
Status: UNCONFIRMED → NEW
Ever confirmed: true
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);
Attached patch v2 (obsolete) — Splinter Review
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?
Attachment #418180 - Attachment is obsolete: true
Attachment #418340 - Flags: review?(mkanat)
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.
Attachment #418340 - Flags: review?(mkanat) → review-
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.
Attached patch v3 (obsolete) — Splinter Review
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!
Attachment #418340 - Attachment is obsolete: true
Attachment #419900 - Flags: review?(mkanat)
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.
Attachment #419900 - Flags: review?(mkanat) → review-
Oh, and thanks for finding out that that constant was in POSIX.pm! I wanted to know that! :-)
Attached patch v4 (obsolete) — Splinter Review
Cheers for the review. 

Having limit and offset in the $params makes total sense so here's the new patch!
Attachment #419900 - Attachment is obsolete: true
Attachment #420060 - Flags: review?(mkanat)
Attachment #420060 - Flags: review?(mkanat) → review+
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! :-)
Flags: approval?
Target Milestone: --- → Bugzilla 3.8
Whiteboard: [licensing issue, don't check in until resolved]
Target Milestone: Bugzilla 4.0 → Bugzilla 5.0
Hey Chris. Could we get a relicensed version of this patch also?
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...
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.

:-)
Attachment #471853 - Flags: review?(mkanat)
Whiteboard: [licensing issue, don't check in until resolved]
Flags: approval?
Attached patch v6Splinter Review
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.
Assignee: rosie.clarkson → mkanat
Attachment #420060 - Attachment is obsolete: true
Attachment #471853 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #511903 - Flags: review+
Attachment #471853 - Flags: review?(mkanat)
Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified buglist.cgi
modified Bugzilla/Search.pm
Committed revision 7704.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: approval+
Resolution: --- → FIXED
Target Milestone: Bugzilla 5.0 → Bugzilla 4.2
You need to log in before you can comment on or make changes to this bug.