303695 - Eliminate deprecated Bugzilla::DB routines from post_bug.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Teemu Mannermaa (:wicked)]

These lines need to rewritten to use DBI:

post_bug.cgi:127:my $sql_product = SqlQuote($cgi->param('product'));
post_bug.cgi:128:my $sql_component = SqlQuote($cgi->param('component'));
post_bug.cgi:132:    SendSQL("SELECT initialowner FROM components " .
post_bug.cgi:134:    $cgi->param(-name => 'assigned_to', -value =>
FetchOneColumn());
post_bug.cgi:159:        SendSQL("SELECT initialqacontact FROM components " .
post_bug.cgi:161:        $qa_contact = FetchOneColumn();
post_bug.cgi:180:    SendSQL("SELECT votestoconfirm FROM products WHERE id =
$product_id");
post_bug.cgi:181:    if (!FetchOneColumn()) {
post_bug.cgi:187:    SendSQL("SELECT defaultmilestone FROM products WHERE
name=$sql_product");
post_bug.cgi:188:    $cgi->param(-name => 'target_milestone', -value =>
FetchOneColumn());
post_bug.cgi:281:SendSQL("SELECT NOW()");
post_bug.cgi:282:my $timestamp = FetchOneColumn();
post_bug.cgi:283:my $sql_timestamp = SqlQuote($timestamp);
post_bug.cgi:293:    $sql .= SqlQuote($cgi->param($field)) . ",";
post_bug.cgi:310:    $sql .= SqlQuote($est_time) . "," . SqlQuote($est_time) . ",";
post_bug.cgi:317:    $sql .= SqlQuote($cgi->param('deadline'));
post_bug.cgi:339:        SendSQL("SELECT user_id FROM user_group_map
post_bug.cgi:343:        my ($permit) = FetchSQLData();
post_bug.cgi:345:            SendSQL("SELECT othercontrol FROM group_control_map
post_bug.cgi:347:            my ($othercontrol) = FetchSQLData();
post_bug.cgi:357:SendSQL("SELECT DISTINCT groups.id, groups.name, " .
post_bug.cgi:362:while (MoreSQLData()) {
post_bug.cgi:363:    my ($id, $groupname, $membercontrol, $othercontrol ) =
FetchSQLData();
post_bug.cgi:381:SendSQL($sql);
post_bug.cgi:388:    SendSQL("INSERT INTO bug_group_map (bug_id, group_id)
post_bug.cgi:398:SendSQL("INSERT INTO longdescs (bug_id, who, bug_when, thetext,
isprivate)
post_bug.cgi:399:         VALUES ($id, " . SqlQuote($user->id) . ",
$sql_timestamp, " .
post_bug.cgi:400:        SqlQuote($comment) . ", $privacy)");
post_bug.cgi:404:    SendSQL("INSERT INTO cc (bug_id, who) VALUES ($id, $ccid)");
post_bug.cgi:410:        SendSQL("INSERT INTO keywords (bug_id, keywordid)
post_bug.cgi:415:        SendSQL("SELECT name FROM keyworddefs WHERE id IN ( " .
post_bug.cgi:418:        while (MoreSQLData()) {
post_bug.cgi:419:            push (@list, FetchOneColumn());
post_bug.cgi:421:        SendSQL("UPDATE bugs SET delta_ts = $sql_timestamp," .
post_bug.cgi:422:                " keywords = " . SqlQuote(join(', ', @list)) .
post_bug.cgi:430:                SendSQL("INSERT INTO dependencies ($me,
$target) values " .

Comment 1

[Tiago Ratto]

Attachment: Patch v2

Comment 2

[Frédéric Buclin]

Comment on attachment 203376 [details] [diff] [review]
Patch v2

>+my $sql_product = $cgi->param('product');
>+trick_taint($sql_product);
>+
>+my $sql_component = $cgi->param('component');
>+trick_taint($sql_component);

That's a big r-! We detaint values because we pass them to the SQL query using placeholders. In your patch, you insert them directly. Moreover, I see no reason to keep 'sql_' in the variable names.


>+    my $sth = $dbh->prepare(qq{SELECT initialowner FROM components
>+                                WHERE id = ?});
>+    $sth->execute($component_id);
>+    my ($result) = $sth->fetchrow_array;

Combine them in a single line using $dbh->selectrow_array().


>+        ($qa_contact) = $dbh->selectrow_array(qq{
>+                                 SELECT initialqacontact
>+                                   FROM components
>+                                  WHERE id = ? }, undef,$component_id);

Nit: add a whitespace after commas.


>+    if (!$dbh->selectcol_arrayref(qq{SELECT votestoconfirm
>+                                       FROM products
>+                                      WHERE id = '$product_id'})) {

I really don't like having SQL queries in tests. First do the query, save the result in a variable and then use this variable in the test.

Moreover, use *placeholders* and stop using selectcol_arrayref() when selectrow_array() should be used!


>+    my $query =qq{SELECT defaultmilestone
>+                    FROM products
>+                   WHERE name = ?};
>+
>+    my ($result) = $dbh->selectrow_array($query, undef, $sql_product);

I prefer the query to be in selectrow_array() directly. If the product name is only used here, then that's the place where you should detaint its value.


> if (defined $cgi->param('cc')) {
>     foreach my $person ($cgi->param('cc')) {
>-        next unless $person;

Not only this change has nothing to do with your patch, but removing it is *wrong*, see bug 315969.


> foreach my $field (@used_fields) {
>+    my $value = $cgi->param($field);
>+    trick_taint($value);
>+    $query .= qq{'$value', };
> }

Use placeholders


>+$query .= q{?, ?, };

You use them here but not above?!


>+    $query .= qq{'$est_time', '$est_time')};

...


>+    $query .= q{NULL};

NULL values are inserted using 'undef'.


>+            my @result = $dbh->selectrow_array(q{
>+                                      SELECT othercontrol
>+                                        FROM group_control_map
>+                                       WHERE group_id = ?
>+                                         AND product_id = ?}, undef,
>+                                             $v,$product_id);
>+            my ($othercontrol) = @result;

Use $othercontrol directly


>+my $sth = $dbh->prepare(q{
>+                 SELECT DISTINCT groups.id, groups.name,
>+                                 membercontrol, othercontrol,
>+                                 description
>+                            FROM groups
>+                       LEFT JOIN group_control_map ON group_id = id
>+                             AND product_id = ?
>+                           WHERE isbuggroup != 0
>+                             AND isactive != 0
>+                        ORDER BY description});
>+$sth->execute($product_id);
>+
>+while (my @result = $sth->fetchrow_array) {

Use $dbh->selectall_arrayref() and then a foreach loop over results saved in the array.


>+trick_taint($timestamp);

No need to detaint $timestamp. This value comes from the DB.


>+trick_taint($query);

The query doesn't need to be detaint. Values passed to it have.


>+$dbh->do($query, undef, $user->id, $timestamp);

So only two values are inserted into the DB? ;)


>+    $dbh->do(q{INSERT INTO bug_group_map (bug_id, group_id) 
>+                    VALUES (?, ?)}, undef, $id, $grouptoadd);

When more than one value are passed, please enclose them in parens.


>+trick_taint($timestamp);

Not only $timestamp has already been detainted, but you don't need to do it for the same reason as above.


>+         $id,$user->id,$timestamp,$comment,$privacy);

Add whitespaces after commas.


>+        my @list = @{$dbh->selectcol_arrayref(q{SELECT name
>+                                                  FROM keyworddefs
>+                                                 WHERE id
>+                                                    IN (?)}, undef,
>+                                              join(',',@keywordlist))};

Doesn't work. Using placeholders will enclose the whole list. This is the kind of mistakes which is easily detected when being tested.

Moreover, write "WHERE id IN (foo)" on one line.


>+        $dbh->do(q{UPDATE bugs
>+                      SET delta_ts = ?,
>+                          keywords = ?
>+                    WHERE bug_id = ?}, undef, $timestamp,
>+                 join(',',@list),$id);

Parameters on one line when the list is not too long, which is the case here.

Comment 3

[Tiago Ratto]

Attachment: Patch v3

Comment 4

[Teemu Mannermaa (:wicked)]

Comment on attachment 204697 [details] [diff] [review]
Patch v3

>+trick_taint($product);

Move this before the if in line 216 so it's closer to where it's actually used.
 
>+        ($qa_contact) = $dbh->selectrow_array(q{
>+                                 SELECT initialqacontact
>+                                   FROM components
>+                                  WHERE id = ? },
>+                                  undef, $component_id);

Nit: No reason to format this differently than other similar SQL fragments (basicly, no new line after the q{ and no space after WHERE id = ?).

>+my @fields_values;
>+foreach my $field (@used_fields) {
>+    if (defined $cgi->param($field)) {
>+        my $value = $cgi->param($field);
>+        trick_taint($value);
>+        push (@fields_values, $value);
>+    }
>+}
>+

Umm, I see no reason to have this here and confuse the logic. Combine this foreach with the one doing similar loop around line 357.

>+my $query = qq{INSERT INTO bugs ($sql_used_fields, reporter, delta_ts,
>+                                 estimated_time, remaining_time,
>+                                 deadline)
>+                    VALUES ($sql_used_fields_values ?, ?, ?, ?, ? )};

Nit: No need to indent VALUES line or have newline before deadline.

>+    $est_time = $cgi->param('estimated_time');
>     Bugzilla::Bug::ValidateTime($est_time, 'estimated_time');
>-    $sql .= SqlQuote($est_time) . "," . SqlQuote($est_time) . ",";

Variable $est_time needs to be trick_tainted after ValidateTime is called. This is probably why post_bug.cgi crashes with:

Insecure dependency in parameter 20 of DBI::db=HASH(0x9e9d594)->do method call while running with -T switch at post_bug.cgi line 459.

>+                 SELECT DISTINCT groups.id, groups.name,
>+                                 membercontrol, othercontrol,
>+                                 description

Nit: No need to use three lines for these when there's room to combine.

>+$dbh->do($query, undef,
>+         (@fields_values, $user->id, $timestamp, $est_time,
>+         $est_time, $deadline));

Use different variable for remaining time even though it will have same value as $est_time.

Nit: No need to put newline after undef param.

>+    $dbh->do(q{INSERT INTO bug_group_map (bug_id, group_id)
>+                    VALUES (?, ?)},
>+             undef, ($id, $grouptoadd));

Nit: No need to put VALUES on a separate line when there's room on the first line.

>+$dbh->do(q{INSERT INTO longdescs
>+                       (bug_id, who, bug_when, thetext, isprivate)
>+                VALUES (?, ?, ?, ?, ?)},
>+         undef, ($id, $user->id, $timestamp, $comment, $privacy));

Nit: Put second line with first and don't indent VALUES.

>+        $dbh->do(q{INSERT INTO keywords (bug_id, keywordid) 
>+                        VALUES (?, ?)},
>+                 undef, ($id, $keyword));

Nit: Move VALUES line with the first line.

>+        my $list = $dbh->selectcol_arrayref(q{
>+                                      SELECT name
>+                                        FROM keyworddefs
>+                                       WHERE id IN (}.
>+                                       join(', ', @keywordlist).q{)});

Nit: No newline after q{ and add spaces around the dots. Also use normal quoting and not q{} around that lonely ).

>+        $dbh->do(q{UPDATE bugs
>+                      SET delta_ts = ?, keywords = ?
>+                    WHERE bug_id = ?},
>+                 undef, ($timestamp, join(', ', @$list), $id));

I don't think join() clause will work as a param. Move it inside the SQL fragment like it was in previous SELECT.

Nit: Combine SET clause with the first line.

>+                $dbh->do(qq{INSERT INTO dependencies ($me, $target)
>+                                 VALUES (?, ?)}, undef, ($id, $i));

Nit: No need to intent VALUES line.

Comment 5

[Gabriel Sales de Oliveira]

Attachment: v4-diff

fixed version.

Comment 6

[Frédéric Buclin]

Comment on attachment 216663 [details] [diff] [review]
v4-diff

>+my @fields_values;
>+my $sql_used_fields_values = "";
>+foreach my $field (@fields_values) {

Huh? you have to loop over @used_fields.


>+    $sql_used_fields_values .= q{?, };

Instead of this hack, you could write:
$sql_placeholders = "?, " x scalar(@used_fields);

The "x" notation means appending "?, " scalar(@used_fields) times. Of course, this goes outside the loop.


>+    if (defined $cgi->param($field)) {

fields from @used_fields are *all* defined, by definition.


>-$sql .= $user->id . ", $sql_timestamp, ";

Instead of removing this line, you could push() these values into @fields_values.


>+my $est_time;
>+my $deadline;
>+my $rem_time;

$rem_time is useless as it will always be equal to $est_time. Remove it.
Moreover, initialize these values in case you are not in the timetracking group:
my $est_time = 0;


> if (UserInGroup(Param("timetrackinggroup")) &&
>     defined $cgi->param('estimated_time')) {
> 
>+    $est_time = $cgi->param('estimated_time');
>     Bugzilla::Bug::ValidateTime($est_time, 'estimated_time');
>+    $rem_time = $est_time;

As said above, $rem_time is useless.


> } else {
>+    $est_time = 0;
>+    $rem_time = 0;
> }

If you have correctly set the default value for $est_time, this ELSE block is useless.
Moreover, you can now push() $est_time twice into @fields_values (outside the IF block, of course).


> } else {
>+    $deadline = undef;
> }

Here too, $deadline is already undefined when you are not in the timetracking group.
Moreover, you can now push() it into @fields_values.


> foreach my $b (grep(/^bit-\d*$/, $cgi->param())) {

>+            my ($othercontrol) = $dbh->selectrow_array(q{

Prepare this SQL call outside the FOREACH loop.


>+my $result = $dbh->selectall_arrayref(q{
>+                 SELECT DISTINCT groups.id, groups.name, membercontrol,
>+                                 othercontrol, description
>+                            FROM groups

$groups (if not already in use) is more explicit than $result.


>+                       LEFT JOIN group_control_map ON group_id = id
>+                             AND product_id = ?

Move ON group_id = id on its own line.


>+foreach my $row (@$result) {

Here too, $group is better than $row.


>+$dbh->do($query, undef, (@fields_values, $user->id, $timestamp, 
>+                         $est_time, $rem_time, $deadline));

If all additional values have been pushed in @fields_values, you now only have
($query, undef, @fields_values), which is more readable.


> foreach my $grouptoadd (@groupstoadd) {
>+    $dbh->do(q{INSERT INTO bug_group_map (bug_id, group_id) VALUES (?, ?)},
>+             undef, ($id, $grouptoadd));
> }

Prepare it outside the FOREACH loop.


> foreach my $ccid (keys(%ccids)) {
>+    $dbh->do(q{INSERT INTO cc (bug_id, who) VALUES (?,?)},
>+             undef, ($id, $ccid));
> }

Same comment here.


>     foreach my $keyword (@keywordlist) {
>+        $dbh->do(q{INSERT INTO keywords (bug_id, keywordid) VALUES (?, ?)},
>+                 undef, ($id, $keyword));
>     }

Same comment here.


>+        $dbh->do(q{UPDATE bugs SET delta_ts = ?, keywords = } . join(', ',
>+                 @$list) . "WHERE bug_id = ?", undef, ($timestamp, $id));

And now we can open a security bug for SQL injection!!! join(', ', @$list) must be moved outside the query and you must use a placeholder instead: keywords = ?


>             foreach my $i (@{$deps{$target}}) {
>+                $dbh->do(qq{INSERT INTO dependencies ($me, $target)
>+                           VALUES (?, ?)}, undef, ($id, $i));

Prepare it outside the FOREACH loop.

Comment 7

[Gabriel Sales de Oliveira]

Attachment: v5-version

maybe the identation is not the best... :/

Comment 8

[Frédéric Buclin]

Comment on attachment 217450 [details] [diff] [review]
v5-version

>+        my $list = $dbh->selectcol_arrayref(q{
>+                                    SELECT name 
>+                                      FROM keyworddefs 
>+                                     WHERE id IN (?)}, undef, $kw_ids);

You cannot use placeholders in IN(), because it will quote the whole list of keyword IDs, instead of quoting them one by one. You have to replace "?" by $kw_ids directly (this is a list of integers only, so there is no SQL injection possible here). This must be fixed on checkin.

Else the patch works correctly. r=LpSolit

Comment 9

[Frédéric Buclin]

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.142; previous revision: 1.141
done