278018 - Eliminate deprecated Bugzilla::DB routines from buglist.cgi

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Let's get rid of all the SendSQL calls and so forth that show up in buglist.cgi.

Comment 1

[Max Kanat-Alexander]

Attachment: Work In Progress

Here's how far I was able to get tonight. It's not done yet, but everything
here is tested and works.

Comment 2

[Max Kanat-Alexander]

Attachment: Convert buglist.cgi to using DBI handles

OK, um, I actually managed to finish it!

Oh, and I could resist... I was going to have to write some code twice, so
instead I just moved it into a subroutine. I hope that you can forgive me. :-)

Comment 3

[Marc Schumann [:Wurblzap]]

Comment on attachment 170989 [details] [diff] [review]
Convert buglist.cgi to using DBI handles

Good and necessary patch; works. I just have a couple of nits or proposals.

>+my $dbh = Bugzilla->dbh;

It took me a while to get it... You're having one of these for each sub (where
needed), but you need that global one for the global code, right? If so, then
that's good.

> sub LookupNamedQuery {
>     my ($name) = @_;
>     Bugzilla->login(LOGIN_REQUIRED);
>-    my $userid = Bugzilla->user->id;
>-    my $qname = SqlQuote($name);
>-    SendSQL("SELECT query FROM namedqueries WHERE userid = $userid AND name = $qname");
>-    my $result = FetchOneColumn();
>+    my $dbh = Bugzilla->dbh;
>+    my $sth = $dbh->prepare("SELECT query FROM namedqueries" 
>+                          . " WHERE userid = ? AND name = ?");
>+    # $name is safe -- we only use it below in a placeholder and then 
>+    # in error messages (which are always HTML-filtered.
>+    trick_taint($name);
>+    $sth->execute(Bugzilla->user->id, $name);
>+    my $result = $sth->fetchrow_array;

I generally like having a comment when untainting. But using some value in a
placeholder is a reason to say it's safe to untaint only if we're talking about
SELECTs or DELETEs -- INSERTing a value by way of a placeholder may be unsafe
because the code reading the value may be sloppy.
Here, we're doing a SELECT, so it is safe to untaint -- how about giving the
SELECT as the untaint reason instead of the placeholder?

Nit: you could probably merge prepare, execute and fetchrow_array into
$dbh->selectrow_array("SELECT ...", {}, (Bugzilla->user->id, $name)) -- might
be easier to read.
Nit: There's a closing bracket missing in the comment.

>+sub InsertNamedQuery ($$$;$) {

Afaik we don't do that ($$$;$) notation any more.

>+    Bugzilla->login(LOGIN_REQUIRED);

You've done that outside the sub already, in both cases InsertNamedQuery is
being called. Is there a reason to have it here?

>+    $link_in_footer = 0 if (!defined($link_in_footer));

Nit: $link_in_footer ||= 0 may be more readable.

>+    # All the arguments are only passed to the DB in placeholders.

As said, this is not enough reason to untaint. We have no choice but to
untaint, though.

>+    trick_taint($query_name);
>+    trick_taint($query);
>+    trick_taint($userid);
>+    trick_taint($link_in_footer);

$link_in_footer is not tainted anyway. And I think $userid isn't, either.

>+    my $sth = $dbh->prepare("SELECT userid FROM namedqueries"
>+        . " WHERE userid = ? AND name = ?");
>+    $sth->execute($userid, $query_name);
>+    my $result = $sth->fetchrow_array;

Nit: may be merged into selectrow_array, as above.

>+        my $update_sth = $dbh->prepare("UPDATE namedqueries"
>+            . " SET query = ?, linkinfooter = ?"
>+            . " WHERE userid = ? AND name = ?");
>+        $update_sth->execute($query, $link_in_footer, $userid, $query_name);

Nit: this can be $dbh->do('UPDATE ...', {}, ($query, $link_in_footer, $userid,
$query_name)).

>+        my $insert_sth = $dbh->prepare("INSERT INTO namedqueries"
>+            . " (userid, name, query, linkinfooter)"
>+            . " VALUES (?, ?, ?, ?)");
>+        $insert_sth->execute($userid, $query_name, $query, $link_in_footer);

Similar here.

>+    my $count = $dbh->selectrow_array("SELECT COUNT(quip)"
>+                                    . " FROM quips WHERE approved = 1");

Nit: selectrow_array. (Don't hate me.)

>+    my $quip = 
>+        $dbh->selectrow_array("SELECT quip FROM quips"
>+                            . " WHERE approved = 1 LIMIT $random,1");

You're doing so fine using placeholders, why don't you here?

>+    my $groups = $sth->fetchall_arrayref({});

Nit: selectall_arrayref

>+        # Copy the name into a variable, so that we can trick_taint it for
>+        # the DB. We know it's safe, because we're using placeholders in 
>+        # the SQL.

Well yes, because it's a DELETE... Maybe I'm paranoid... I'm not feeling well
stating this so generally.

>+        my $sth = $dbh->prepare("DELETE FROM namedqueries"
>+            . " WHERE userid = ? AND name = ?");
>+        $sth->execute($userid, $qname);

Nit: $dbh->do is possible here.

>+        # qbuffer is safe because it will only be used in placeholders.

Same as above... Maybe "qbuffer may safely be untainted here because it will
only be used in a SELECT statement using placeholders"?

>         my $tofooter = 1;
[...]
>+        InsertNamedQuery($userid, $name, $::FORM{'newquery'}, $tofooter);

The $tofooter variable is used only once. Do you think putting a fixed 1 into
the InsertNamedQuery call is appropriate? I agree having a variable keeps
readability up... (The variable didn't come in with your code, I know.)

Comment 4

[Frédéric Buclin]

This makes many comments and nits to still get a r+.

I would still like to have a satisfactory explanation about "my $dbh =
Bugzilla->dbh;" appearing everywhere in the patch; mkanat?

Comment 5

[Marc Schumann [:Wurblzap]]

(In reply to comment #4)
> This makes many comments and nits to still get a r+.

Yes... The points important to me are
 - removal of ($$$;$)
 - having the comments not suggest a general safeness of placeholders
 - removing the redundant LOGIN_REQUIRED line (or knowing why it should stay)
 - removing the two redundant trick_taint commands

I didn't feel that deserves a r-... Maybe it does...

Max, can you please attach a new patch, and either request approval again or
move r+ forward, depending on how you feel about it?

> I would still like to have a satisfactory explanation about "my $dbh =
> Bugzilla->dbh;" appearing everywhere in the patch; mkanat?

Calling Bugzilla->dbh() is actually a way to get hold of the global DB handle,
so it's good to do it the way Max does. (It's done the same way in
editmilestones.cgi, btw.)

Comment 6

[Max Kanat-Alexander]

(In reply to comment #3)
> >+my $dbh = Bugzilla->dbh;
> 
> It took me a while to get it... You're having one of these for each sub (where
> needed), but you need that global one for the global code, right? If so, then
> that's good.

  Right. It's the correct way to do it. It improves modularity. Functions should
be able to be moved out of scripts without any modification of the function.

> I generally like having a comment when untainting. But using some value in a
> placeholder is a reason to say it's safe to untaint only if we're talking about
> SELECTs or DELETEs -- INSERTing a value by way of a placeholder may be unsafe
> because the code reading the value may be sloppy.

  Whoa, um, what? I'm only concerned about people trying to do SQL-insertion
attacks, which should be entirely stopped by placeholders. That's the point of
taint mode. Can you describe to me how a SQL-quoted variable can be malicious
inside of an INSERT statement?

> Nit: you could probably merge prepare, execute and fetchrow_array into
> $dbh->selectrow_array("SELECT ...", {}, (Bugzilla->user->id, $name)) -- might
> be easier to read.

  Good point. Perhaps I'll do that. :-) I'll also do all the other selectrow
ones that you pointed out.

> Nit: There's a closing bracket missing in the comment.

  Oh, didn't see that. I'll have to fix that. :-)
 
> >+sub InsertNamedQuery ($$$;$) {
> 
> Afaik we don't do that ($$$;$) notation any more.

  Um, what? Prototypes are good. They help catch coding errors. The reasons that
our old functions don't have them is because we do have coding errors.

  Do you have some reference somewhere that says that we don't do prototypes for
some good reason?

> >+    Bugzilla->login(LOGIN_REQUIRED);
> 
> You've done that outside the sub already, in both cases InsertNamedQuery is
> being called. Is there a reason to have it here?

  Modularity. A function itself cannot know how it is being called. It doesn't
hurt to call LOGIN_REQUIRED again, as far as I know. If it does hurt to call it
more than once during the same thread, then that's an architectural problem that
we should work out.

> >+    $link_in_footer = 0 if (!defined($link_in_footer));
> 
> Nit: $link_in_footer ||= 0 may be more readable.

  Agreed.

> $link_in_footer is not tainted anyway. And I think $userid isn't, either.

  I know that, but I can't guarantee that the caller will always do that.
Modularity, once again.

> Nit: this can be $dbh->do('UPDATE ...', {}, ($query, $link_in_footer, $userid,
> $query_name)).

  Sounds good to me. :-) I'll change all those other prepare/execute statements
that do INSERT or UPDATE into do statements where I can and it makes sense.

> >+    my $quip = 
> >+        $dbh->selectrow_array("SELECT quip FROM quips"
> >+                            . " WHERE approved = 1 LIMIT $random,1");
> 
> You're doing so fine using placeholders, why don't you here?

  Oh, it wasn't necessary. $random is a number, and we only prepare this SQL
once. But I suppose I could do it.

> >+    my $groups = $sth->fetchall_arrayref({});
> 
> Nit: selectall_arrayref

  OK. If selectall_arrayref can take the {} argument, I'll do it.

> >         my $tofooter = 1;
> [...]
> >+        InsertNamedQuery($userid, $name, $::FORM{'newquery'}, $tofooter);
> 
> The $tofooter variable is used only once. Do you think putting a fixed 1 into
> the InsertNamedQuery call is appropriate? I agree having a variable keeps
> readability up... (The variable didn't come in with your code, I know.)

  Yes, putting a fixed one into the call is appropriate. That's what the old
code does.

  I'll get around to putting up a new patch once I'm sure about the questions
that I've asked above.

Comment 7

[GavinS]

Thinking of a possible mod_perl future, should something as heavily used as
buglist.cgi use prepare_cached() for some of these DB accesses???

Comment 8

[Marc Schumann [:Wurblzap]]

(In reply to comment #6)
>   Whoa, um, what? I'm only concerned about people trying to do SQL-insertion
> attacks, which should be entirely stopped by placeholders. That's the point of
> taint mode. Can you describe to me how a SQL-quoted variable can be malicious
> inside of an INSERT statement?

When read again from the database by other parts of the code and being used as
part of an URL, for example.

The way I see taint mode is that it helps kicking us to make sure that a
variable contains something safe. We do the checks, write a code comment, and
trick_taint.

Otherwise DBI could generally untaint placeholder variables. Placeholders
protect us from SQL-insertion attacks. If that were the only danger, we wouldn't
need taint mode.

What I'm saying is that your code is, of course, safe. But somebody reading the
comments shouldn't get the impression that it is safe to untaint as long as one
uses placeholders. I think the trick_taint comments in reports.cgi and
editproduct.cgi are good examples.

>   Um, what? Prototypes are good. They help catch coding errors. The reasons that
> our old functions don't have them is because we do have coding errors.
>   Do you have some reference somewhere that says that we don't do prototypes
> for some good reason?
> 

It's the other way round: old functions do have some, new ones don't. I thought
there was some definite guideline on that, but I couldn't find it. I just found
bug 124174, comment 20, and bug 126955, comment 21.
Personally, I'm ambivalent on the issue.

> > >+    Bugzilla->login(LOGIN_REQUIRED);
> > 
> > You've done that outside the sub already, in both cases InsertNamedQuery is
> > being called. Is there a reason to have it here?
> 
>   Modularity. A function itself cannot know how it is being called. It doesn't
> hurt to call LOGIN_REQUIRED again, as far as I know. If it does hurt to call 

Accepted.

> > $link_in_footer is not tainted anyway. And I think $userid isn't, either.
> 
>   I know that, but I can't guarantee that the caller will always do that.

I think that's another point of taint mode -- a sub may well require to get
passed untainted values if it doesn't want to exhaustively check them for
naughtiness. As a (far-stretched) example: it may be a good thing if this
function crashed if it were passed a tainted $link_in_footer.

We're not getting into trouble with the four untaints here, of course.
But we need to keep in mind that we're not generally safe -- we're relying on
buglist templates to "FILTER html" the query name, for example. Not untainting
here but after the check which may result in
ThrowUserError('illegal_query_name') (line 395) is imo The Right Thing :)

Comment 9

[Marc Schumann [:Wurblzap]]

(In reply to comment #7)
> Thinking of a possible mod_perl future, should something as heavily used as
> buglist.cgi use prepare_cached() for some of these DB accesses???

This sounds like a good idea to me, but I don't know much about mod_perl. I
can't say whether it can (or should) be done. Maybe in another bug?

Comment 10

[Max Kanat-Alexander]

We can only use prepare_cached in mod_perl if we have a statement that will
return the same thing every time, for the rest of eternity.

Basically, mod_perl will give us one "persistent" Bugzilla->dbh for each Apache
thread. Just try to imagine how complex that makes things, and you'll see why
(1) it might be a good idea to cache some things and (2) it takes a lot of
thought to figure out what we can and can't cache.

Comment 11

[Max Kanat-Alexander]

Attachment: Version 2

OK, here's the newest version.

I addressed almost all the comments, and I made InsertNamedQuery a MUCH nicer
function, including some nice docs.

I didn't remove ($$$;$). I think it's useful, and nobody has yet contradicted
me in IRC.

The $random is not a placeholder, because DBI quotes it when it shouldn't be
quoted, if I make it a placeholder.

Comment 12

[Marc Schumann [:Wurblzap]]

Comment on attachment 172238 [details] [diff] [review]
Version 2

This looks very good now.
I'll put this through testing, and I think I can come up with a full review
real soon.

Comment 13

[Marc Schumann [:Wurblzap]]

Comment on attachment 172238 [details] [diff] [review]
Version 2

I like the way you're doing this now :)

>+        my $qname = $::FORM{'namedcmd'};

I'm sorry I missed this nit at first: maybe you can use $cgi->param('namedcmd')
here? I'm not too sure about this because if you do, we'd end up with a jumble
of $::FORM and $cgi->param in buglist.cgi. What's your opinion?

If you choose to replace some $::FORM occurrences, please put up a new patch
and carry r+ forward. If you don't, just ask for approval :)

Comment 14

[Max Kanat-Alexander]

I think we'll do the $::FORM stuff in another bug. I think there's a bug for
that, somewhere (probably blocking the mod_perl bug).

Comment 15

[Dave Miller [:justdave]]

yeah, the my $dbh in each sub is something we need for mod_perl, at least to be
explicit with it.  Variables declared outside of a sub don't survive well inside
subs under mod_perl, except in package (.pm) files.

Comment 16

[Shane H. W. Travis]

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.276; previous revision: 1.275
done