Timestamps should be quoted

RESOLVED DUPLICATE of bug 257315

Status

()

Bugzilla
Bugzilla-General
--
blocker
RESOLVED DUPLICATE of bug 257315
14 years ago
13 years ago

People

(Reporter: Vlad Dascalu, Assigned: Tomas Kopal)

Tracking

({regression})

Details

Attachments

(2 attachments)

2.68 KB, patch
Max Kanat-Alexander
: review-
Details | Diff | Splinter Review
V2
12.47 KB, patch
Max Kanat-Alexander
: review-
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

14 years ago
Created attachment 172754 [details] [diff] [review]
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.
(Reporter)

Updated

14 years ago
Assignee: general → vladd
Status: NEW → ASSIGNED
Attachment #172754 - Flags: review?
(Reporter)

Updated

14 years ago
Flags: blocking2.20?
Target Milestone: --- → Bugzilla 2.20
(Reporter)

Updated

14 years ago
Severity: normal → critical

Comment 2

14 years ago
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.
Attachment #172754 - Flags: review? → review-
(Assignee)

Comment 3

14 years ago
Created attachment 172788 [details] [diff] [review]
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).
Attachment #172788 - Flags: review?
(Reporter)

Updated

14 years ago
Assignee: vladd → Tomas.Kopal
Status: ASSIGNED → NEW
(Reporter)

Updated

14 years ago
Attachment #172788 - Flags: review? → review?(mkanat)

Comment 4

14 years ago
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.
Attachment #172788 - Flags: review?(mkanat) → review-

Updated

14 years ago
Severity: critical → blocker
Keywords: regression

*** This bug has been marked as a duplicate of 257315 ***
Status: NEW → RESOLVED
Last Resolved: 14 years ago
No longer depends on: 257315
Resolution: --- → DUPLICATE
Flags: blocking2.20?
clearing target of DUPLICATE/WONTFIX/INVALID/WORKSFORME so they'll show up as
untriaged if they get reopened.
Target Milestone: Bugzilla 2.20 → ---
You need to log in before you can comment on or make changes to this bug.