280265 - Timestamps should be quoted

Status: RESOLVED DUPLICATE of bug 257315

(Bugzilla :: Bugzilla-General, defect)

Description

[Vlad Dascalu]

Bug 257315 caused some timestamps to no longer be quoted, which makes it
impossible to create new bugs on landfill (among other things)

Comment 1

[Vlad Dascalu]

Attachment: patch v1

I think we don't need to use SqlQuote, because:

-> $timestamp is already trick_tainted (by not been derived from user input)
-> it contains digits and safe chars, so calling dbh->quote on it is not
required.
-> simply adding quotes around it makes the solution simple and elegant,
although placeholders would be nicer.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 172754 [details] [diff] [review]
patch v1

>         "VALUES($bugid, $whoid, $timestamp, " . SqlQuote($comment) . ", " . 
>         $privacyval . ", " . SqlQuote($work_time) . ")");
> 
>-    SendSQL("UPDATE bugs SET delta_ts = $timestamp WHERE bug_id = $bugid");
>+    SendSQL("UPDATE bugs SET delta_ts = '$timestamp' WHERE bug_id = $bugid");

  This $timestamp is already quoted, above, unless I'm mistaken:

  $timestamp = ($timestamp ? SqlQuote($timestamp) : "NOW()");

  However, the SqlQuote should probably be moved outside of the ternary so that
it also covers the NOW().

>-$sql .= "$::userid, $timestamp, $timestamp, ";
>+$sql .= "$::userid, '$timestamp', '$timestamp', ";

  Yeah, and it's OK to not SqlQuote this one because it comes from the DB.

>-        SendSQL("UPDATE bugs SET delta_ts = $timestamp," .
>+        SendSQL("UPDATE bugs SET delta_ts = '$timestamp'," .

  Same for this one (it's the same variable, even).

>-        SendSQL("UPDATE bugs SET delta_ts=$timestamp WHERE bug_id=$i");
>+        SendSQL("UPDATE bugs SET delta_ts='$timestamp' WHERE bug_id=$i");

  Technically, this one is passed into the function from an arbitrary location,
so *maybe* it should be SqlQuoted. But it wasn't before, so this is probably
OK. We'll just get a taint error if this function gets a tainted timestamp,
which is probably fine.

>-            SendSQL("UPDATE bugs SET delta_ts = $timestamp, keywords = " .
>+            SendSQL("UPDATE bugs SET delta_ts = '$timestamp', keywords = " .

  And this one also comes from the DB, so we're OK.

  So only the first one needs to be fixed.

Comment 3

[Tomas Kopal]

Attachment: V2

Bugger, good catch. I need to spend more time testing it next time, sorry :-(.

I went through the whole patch which caused this and found a few more places
where quotes are missing. I have also converted few SqlQuote() calls to simpe
quotes after verifying that they are coming directly from the DB (even in
functions, if all calls are getting the passed parameter from the DB).

Comment 4

[Max Kanat-Alexander]

Comment on attachment 172788 [details] [diff] [review]
V2

You know, actually, I'd rather you leave any SqlQuotes there that are already
being done. They don't hurt, and they take very little time.

Just quote the values that are current unquoted and cause Bugzilla to break.

Comment 5

[Dave Miller [:justdave]]

*** This bug has been marked as a duplicate of 257315 ***

Comment 6

[Dave Miller [:justdave]]

clearing target of DUPLICATE/WONTFIX/INVALID/WORKSFORME so they'll show up as
untriaged if they get reopened.