300552 - Eliminate deprecated Bugzilla::DB routines from Search.pm

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Frédéric Buclin]

This one will be painful. :)

Comment 1

[Max Kanat-Alexander]

You may want to split this up by section of Search.pm.

Comment 2

[André Batosti]

Attachment: batosti_v1

Comment 3

[Max Kanat-Alexander]

Comment on attachment 202561 [details] [diff] [review]
batosti_v1

You don't need to change the ' marks to q{ everywhere. You don't need to change " to qq{.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 202561 [details] [diff] [review]
batosti_v1

Also, you're going to have all sorts of taint problems.

Remember that SqlQuote de-tainted things, but $dbh->quote does not.

Comment 5

[André Batosti]

Attachment: batosti_v1_fix

Comment 6

[Max Kanat-Alexander]

Comment on attachment 202687 [details] [diff] [review]
batosti_v1_fix

>+        trick_taint($chfieldfrom);
>+        my $sql_chfrom = $chfieldfrom ? $dbh->quote(SqlifyDate($chfieldfrom)):'';

  This is backwards. Instead, you need to trick_taint the things that have already been quoted and are being put into the DB. The way that you've done trick_taint here, it's far too broad. (This applies to other places in this patch, also.)

  You need to trick_taint *what is going into the DB*, not the input variable.

  You did this backwards in pretty much the whole patch.

Comment 7

[André Batosti]

Attachment: batosti_v2

Comment 8

[Max Kanat-Alexander]

Comment on attachment 203146 [details] [diff] [review]
batosti_v2

Joel is the person to ask for Search.pm stuff. Or justdave, if he has time.

Comment 9

[Frédéric Buclin]

joel, could you help us for this one?

Comment 10

[Max Kanat-Alexander]

Comment on attachment 203146 [details] [diff] [review]
batosti_v2

>-        my $sql_chfrom = $chfieldfrom ? &::SqlQuote(SqlifyDate($chfieldfrom)):'';
>-        my $sql_chto   = $chfieldto   ? &::SqlQuote(SqlifyDate($chfieldto))  :'';
>-        my $sql_chvalue = $chvalue ne '' ? &::SqlQuote($chvalue) : '';
>+        $chfieldfrom = SqlifyDate($chfieldfrom);
>+        trick_taint($chfieldfrom);
>+        my $sql_chfrom = $chfieldfrom ? $dbh->quote($chfieldfrom):'';

  No. trick_taint($sql_chfrom). Don't trick_taint the input variable. Isn't that what I *just* said?

>+        $chfieldto = SqlifyDate($chfieldto);
>+        trick_taint($chfieldto);
>+        my $sql_chto   = $chfieldto   ? $dbh->quote($chfieldto)  :'';

  Same here.

>+        trick_taint($chvalue);
>+        my $sql_chvalue = $chvalue ne '' ? $dbh->quote($chvalue) : '';

  Same here.

>@@ -353,7 +358,8 @@ sub init {
>+        trick_taint($deadlinefrom);
>+        $sql_deadlinefrom = $dbh->quote($deadlinefrom);

  Same here.

>@@ -362,7 +368,8 @@ sub init {
>+        trick_taint($deadlineto);
>+        $sql_deadlineto = $dbh->quote($deadlineto);

  Same here.

>@@ -373,7 +380,8 @@ sub init {
>-                my $q = &::SqlQuote($s);
>+                trick_taint($s);
>+                my $q = $dbh->quote($s);

  Same here.

>@@ -628,7 +640,8 @@ sub init {
>          },
>          "^deadline,(?:lessthan|greaterthan|equals|notequals),(-|\\+)?(\\d+)([dDwWmMyY])\$" => sub {
>              $v = SqlifyDate($v);
>-             $q = &::SqlQuote($v);
>+             trick_taint($v);
>+             $q = $dbh->quote($v);

  Same here.

>@@ -750,12 +767,13 @@ sub init {
>                                               COUNT(DISTINCT $table.bug_when) /
>                                               COUNT(bugs.bug_id)) +
>                                              bugs.remaining_time)))";
>+                 trick_taint($v);

  Why is it safe to trick_taint($v) here?? You have to quote it first.

>@@ -796,17 +814,17 @@ sub init {
>              my $field = $1;
>              if ($t eq "changedby") {
>                  $v = &::DBNameToIdAndCheck($v);
>-                 $q = &::SqlQuote($v);
>+                 $q = $dbh->quote($v);

  Why don't you trick_taint $q? (That goes for the other three $q's following this one, also.)

>@@ -1056,7 +1074,8 @@ sub init {
>                  if ($w eq "---" && $f !~ /milestone/) {
>                      $w = "";
>                  }
>-                 push(@list, &::SqlQuote($w));
>+                 trick_taint($v);
>+                 push(@list, $dbh->quote($w));

  You called trick_taint($v) and then you quote $w?? That doesn't make sense. 

  Also, you may only trick_taint things after you're absolutely certain that they're safe. So you have to quote it *first* and then call trick_taint.

>@@ -1097,7 +1116,7 @@ sub init {
>                                "ON $table.bug_id = bugs.bug_id " .
>                                "AND $table.fieldid = $fieldid " .
>                                "AND $table.bug_when $operator " . 
>-                               &::SqlQuote(SqlifyDate($v)) );
>+                               $dbh->quote(SqlifyDate($v)) );

  Does that work even though you didn't call trick_taint?

>@@ -1252,9 +1271,10 @@ sub init {
> # $suppstring = String which is pasted into query containing all table names
> 
>     # get a list of field names to verify the user-submitted chart fields against
>-    &::SendSQL("SELECT name, fieldid FROM fielddefs");
>-    while (&::MoreSQLData()) {
>-        my ($name, $id) = &::FetchSQLData();
>+    my $result = $dbh->selectall_arrayref(q{SELECT name, fieldid 
>+                                              FROM fielddefs});
>+    foreach my $row (@$result) {
>+        my ($name, $id) = @$row;
>         $chartfields{$name} = $id;
>     }

  You could replace all of that with selectall_hashref, couldn't you? Or with some sort of single DBI function...

>@@ -1289,7 +1309,7 @@ sub init {
>                 # already know about it), or it was in %chartfields, so it is
>                 # a valid field name, which means that it's ok.
>                 trick_taint($f);
>-                $q = &::SqlQuote($v);
>+                $q = $dbh->quote($v);

  No trick_taint?

>@@ -1517,24 +1537,23 @@ sub ListIDsForEmail {
>         }
>         $list = join(',', @list);
>     } elsif ($type eq 'substring') {
>-        &::SendSQL("SELECT userid FROM profiles WHERE " .
>-            $dbh->sql_position(lc(::SqlQuote($email)), "LOWER(login_name)") .
>-            " > 0 " . $dbh->sql_limit(51));
>-        while (&::MoreSQLData()) {
>-            my ($id) = &::FetchSQLData();
>-            push(@list, $id);
>-        }
>+        trick_taint($email);

  You're calling trick_taint before you quote it!!

>             $list = join(',', @list);
>         }
>     } elsif ($type eq 'regexp') {
>-        &::SendSQL("SELECT userid FROM profiles WHERE " .
>-            $dbh->sql_regexp("login_name", ::SqlQuote($email)) .
>-            " " . $dbh->sql_limit(51));
>-        while (&::MoreSQLData()) {
>-            my ($id) = &::FetchSQLData();
>-            push(@list, $id);
>-        }
>+        trick_taint($email);

  And here too!

>@@ -1548,10 +1567,12 @@ sub build_subselect {
>     my ($outer, $inner, $table, $cond) = @_;
>     my $q = "SELECT $inner FROM $table WHERE $cond";
>     #return "$outer IN ($q)";
>-    &::SendSQL($q);
>+    my $dbh = Bugzilla->dbh;
>+    my $sth = $dbh->prepare($q);
>+    $sth->execute;
>     my @list;
>-    while (&::MoreSQLData()) {
>-        push (@list, &::FetchOneColumn());
>+    foreach my $row ($sth->fetchrow_arrayref) {
>+        push (@list, @$row);

  You could do this whole thing with selectcol_arrayref, right?

  Also, you replaced FetchOneColumn with fetchrow_arrayref and then tried to make each row into an array--that doesn't make sense.

>@@ -1566,7 +1587,7 @@ sub GetByWordList {
>         my $word = $w;
>         if ($word ne "") {
>             $word =~ tr/A-Z/a-z/;
>-            $word = &::SqlQuote(quotemeta($word));
>+            $word = $dbh->quote(quotemeta($word));

  Can't you trick_taint $word now?

>@@ -1585,7 +1606,8 @@ sub GetByWordListSubstr {
> 
>     foreach my $word (split(/[\s,]+/, $strs)) {
>         if ($word ne "") {
>-            push(@list, $dbh->sql_position(lc(::SqlQuote($word)),
>+            trick_taint($word);
>+            push(@list, $dbh->sql_position(lc($dbh->quote($word)),
>                                            "LOWER($field)") . " > 0");

  Once again, quote first, trick_taint later.

Comment 11

[Olav Vitters]

Stealing bug as LpSolit commented you are busy + we are late according to the roadmap.

Comment 12

[Olav Vitters]

Attachment: Patch v3: Bugzilla HEAD 2006-06-14

Note: I tried to test everything, but I am not sure I did.

(In reply to comment #10)
> (From update of attachment 203146 [details] [diff] [review] [edit])
> >-        my $sql_chfrom = $chfieldfrom ? &::SqlQuote(SqlifyDate($chfieldfrom)):'';
> >-        my $sql_chto   = $chfieldto   ? &::SqlQuote(SqlifyDate($chfieldto))  :'';
> >-        my $sql_chvalue = $chvalue ne '' ? &::SqlQuote($chvalue) : '';
> >+        $chfieldfrom = SqlifyDate($chfieldfrom);
> >+        trick_taint($chfieldfrom);
> >+        my $sql_chfrom = $chfieldfrom ? $dbh->quote($chfieldfrom):'';
> 
>   No. trick_taint($sql_chfrom). Don't trick_taint the input variable. Isn't
> that what I *just* said?

The output from SqlifyDate will already be detained for every possible input passed to it. Tested by me + LpSolit. Removed all trick_taint calls that happen after SqlifyDate.

> >@@ -750,12 +767,13 @@ sub init {
> >                                               COUNT(DISTINCT $table.bug_when) /
> >                                               COUNT(bugs.bug_id)) +
> >                                              bugs.remaining_time)))";
> >+                 trick_taint($v);
> 
>   Why is it safe to trick_taint($v) here?? You have to quote it first.

Changed it to $q.

> >@@ -796,17 +814,17 @@ sub init {
> >              my $field = $1;
> >              if ($t eq "changedby") {
> >                  $v = &::DBNameToIdAndCheck($v);
> >-                 $q = &::SqlQuote($v);
> >+                 $q = $dbh->quote($v);
> 
>   Why don't you trick_taint $q? (That goes for the other three $q's following
> this one, also.)

That part of the code now uses login_to_id. That function already returns a detainted variable. The other two (didn't see three) used SqlifyDate, so no trick_taint added.


All other comments fixed.

Comment 13

[Olav Vitters]

Attachment: Patch v4: Bugzilla CVS 2006-06-19

Fixes the following from IRC:

<LpSolit>       -        my $sql_chfrom = $chfieldfrom ? &::SqlQuote(SqlifyDate($chfieldfrom)):'';
<LpSolit>       +        $chfieldfrom = SqlifyDate($chfieldfrom);
<LpSolit>       +        my $sql_chfrom = $chfieldfrom ? $dbh->quote($chfieldfrom):'';
<LpSolit>       this looks wrong to me
<LpSolit>       SqlifyDate($chfieldfrom) will return an non-empty string even if $chfieldfrom is empty
Jun 17 20:19:53 <LpSolit>       and so $sql_chfrom = $chfieldfrom ? $dbh->quote($chfieldfrom):'' is not equivalent to $sql_chfrom = $chfieldfrom ? &::SqlQuote(SqlifyDate($chfieldfrom)):''

Comment 14

[Frédéric Buclin]

Comment on attachment 226212 [details] [diff] [review]
Patch v4: Bugzilla CVS 2006-06-19

Looks good. Passes tests. Seems to work correctly.

Only a nit:
# $q  = sanitized version of user input (SqlQuote($v))

It should not say:
trick_taint(($dbh->quote($v)))

We should fix that on checkin to get it out of our radar when doing a grep on SqlQuote. r=LpSolit

Comment 15

[Frédéric Buclin]

Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.130; previous revision: 1.129
done