Closed Bug 280265 Opened 20 years ago Closed 20 years ago

Timestamps should be quoted

Categories

(Bugzilla :: Bugzilla-General, defect)

defect
Not set
blocker

Tracking

()

RESOLVED DUPLICATE of bug 257315

People

(Reporter: goobix, Assigned: Tomas.Kopal)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 257315 caused some timestamps to no longer be quoted, which makes it impossible to create new bugs on landfill (among other things)
Attached patch patch v1 (deleted) — Splinter Review
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.
Assignee: general → vladd
Status: NEW → ASSIGNED
Attachment #172754 - Flags: review?
Flags: blocking2.20?
Target Milestone: --- → Bugzilla 2.20
Severity: normal → critical
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-
Attached patch V2 (deleted) — Splinter Review
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?
Assignee: vladd → Tomas.Kopal
Status: ASSIGNED → NEW
Attachment #172788 - Flags: review? → review?(mkanat)
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-
Severity: critical → blocker
Keywords: regression
*** This bug has been marked as a duplicate of 257315 ***
Status: NEW → RESOLVED
Closed: 20 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.

Attachment

General

Creator:
Created:
Updated:
Size: