Opened 11 months ago

# Cross-db compatibility issues and possibility of SQL injection

Reported by: Owned by: rjollos ChrisNelson normal TracJsGanttPlugin normal hasienda

As Steffen pointed out in the mailing list thread, the plugin doesn't follow the Trac rules for DB API usage, as described in t:TracDev/DatabaseApi#RulesforDBAPIUsage. This will result in cross-db compatibility issues and the possibility of SQL injection.

Here is an example fix (untested, as I don't understand the plugin well enough to execute this pathway or write a test):

• ## tracjsganttplugin/0.11/tracjsgantt/tracpm.py

diff --git a/tracjsganttplugin/0.11/tracjsgantt/tracpm.py b/tracjsganttplugin/0.
index 0f0c4ba..d5a90d7 100644
 a class TicketRescheduler(Component): values.append(t['id']) values.append(to_utimestamp(self.pm.start(t))) values.append(to_utimestamp(self.pm.finish(t))) cursor.execute('INSERT INTO schedule' + \ ' (ticket, start, finish)' + \ ' VALUES %s' % valuesClause, values) cursor.execute(""" INSERT INTO schedule (ticket, start, finish) VALUES %s""" % valuesClause, values) # Finally, add history records to schedule_change # for newly scheduled tickets.

### comment:1 Changed 11 months ago by rjollos

• Description modified (diff)

### comment:2 Changed 11 months ago by rjollos

• Description modified (diff)

### comment:3 Changed 11 months ago by mlm@…

hi,

from trac.ini

[trac-jsgantt]
option.caption = Resource
option.colorby = priority
option.comp = 0
option.datedisplay = mm/dd/yyyy
option.dur = 0
option.enddate = 1
option.expandclosedtickets = 1
option.format = month
option.formats = day|week|month|quarter
option.hoursperday = 8.0
option.lwidth = 500
option.omitmilestone = 0
option.openlevel = 0
option.res = 0
option.schedule = asap
option.showdep = 1
option.startdate = 1
option.usermap = 0

[TracPM]
blocking = text
blocking.label = Blocking
complete = select
complete.label = % Complete
complete.options = 0|5|10|15|20|25|30|35|40|45|50|55|60|65|70|75|80|85|90|95|100
complete.order = 3
date_format = %m/%d/%Y
days_per_estimate = 0.125
default_estimate = 4.0
due_assign = text
due_assign.label = Start (MM/DD/YYYY)
due_assign.order = 1
due_close = text
due_close.label = End (MM/DD/YYYY)
due_close.order = 2
estimatedhours = text
estimatedhours.label = Estimated Number of Hours
estimatedhours.order = 1
estimatedhours.value = 0
fields.estimate = estimatedhours
fields.finish = due_close
fields.parent = parents
fields.pred = blockedby
fields.start = due_assign
fields.succ = blocking
fields.worked = totalhours
goal_ticket_type = milestone
hours = text
hours.label = Add Hours to Ticket
hours.order = 2
hours.value = 0
hours_per_estimate = 1
loe = select
loe.label = Estimate of Effort
loe.options = low|medium|high
loe.value = low
parent_format = %s
parents = text
parents.label = Parent Tickets
prim_developer = select
prim_developer.label = Primary Developer
test_desc = textarea
test_desc.cols = 80
test_desc.format = wiki
test_desc.label = Testing Instructions
test_desc.rows = 10
totalhours = text
totalhours.label = Total Hours
totalhours.order = 4
totalhours.value = 0


### comment:4 Changed 11 months ago by rjollos

The [ticket-custom] section can probably be inferred from what you've posted, but could you add that as well, just so we can be sure?

### comment:5 Changed 11 months ago by anonymous

[ticket-custom]
billable = checkbox
billable.label = Billable?
billable.order = 3
billable.value = 1
blockedby = text
blockedby.label = Blocked By
blocking = text
blocking.label = Blocking
due_assign = text
due_assign.label = Start (MM/DD/YYYY)
due_assign.order = 1
due_close = text
due_close.label = End (MM/DD/YYYY)
due_close.order = 2
estimatedhours = text
estimatedhours.label = Estimated Number of Hours
estimatedhours.order = 1
estimatedhours.value = 0
hours = text
hours.label = Add Hours to Ticket
hours.order = 2
hours.value = 0
parents = text
parents.label = Parent Tickets
totalhours = text
totalhours.label = Total Hours
totalhours.order = 4
totalhours.value = 0


### comment:6 follow-up: ↓ 9 Changed 11 months ago by rjollos

Actually, I don't think there is anything wrong with the use of the Trac database API. I misread the code when opening this issue earlier, and the patch I've proposed doesn't actually result in any real changes.

Sorry about that, my brain apparently was not working earlier!

Anyway, #11028 is a problem on Trac 1.0 when the TicketRescheduler component is enabled, but that won't be an issue for you since you've shown in your screen capture that it's not enabled.

### comment:7 Changed 11 months ago by mlm@…

hi,

Tried to attach the complete trac.ini as a file, said it was spam - too many external links.

[components]
acct_mgr.htfile.htdigeststore = disabled
cmtekniktheme.* = enabled
crystalxtheme.* = enabled
githubsimple.* = enabled
mastertickets.* = enabled
skittlishtheme.* = enabled
themeengine.* = enabled
themeengine.api.themeenginesystem = enabled
themeengine.web_ui.themeenginemodule = enabled
timingandestimationplugin.* = enabled
tracjsgantt.* = enabled
tracjsgantt.tracjsgantt.tracjsganttchart = enabled
tracjsgantt.tracjsgantt.tracjsganttsupport = enabled
tracjsgantt.tracpm.projectsorter = enabled
tracjsgantt.tracpm.resourcescheduler = enabled
tracjsgantt.tracpm.simplecalendar = enabled
tracjsgantt.tracpm.simplesorter = enabled
tracjsgantt.tracpm.ticketrescheduler = disabled
tracjsgantt.tracpm.tracpm = enabled
tracopt.versioncontrol.git.* = enabled
tracopt.versioncontrol.svn.svn_fs.subversionconnector = enabled
tracopt.versioncontrol.svn.svn_prop.subversionmergepropertydiffrenderer = enabled
tracopt.versioncontrol.svn.svn_prop.subversionmergepropertyrenderer = enabled
tracopt.versioncontrol.svn.svn_prop.subversionpropertyrenderer = enabled
tracsubtickets.api.* = enabled
tracsubtickets.web_ui.* = enabled


### comment:8 Changed 11 months ago by rjollos

Just a hint, you can wrap the sections you post in a code block to get nicer formatting (I've been editing your comments to add this):

{{{
#!ini
<--- paste your content here, between the brackets
}}}


The use of the #!ini isn't necessary, and just gives syntax highlighting.

### comment:9 in reply to: ↑ 6 ; follow-ups: ↓ 10 ↓ 11 Changed 11 months ago by hasienda

Actually, I don't think there is anything wrong with the use of the Trac database API. I misread the code when opening this issue earlier, and the patch I've proposed doesn't actually result in any real changes.

Sorry about that, my brain apparently was not working earlier!

No, actually it seems like my fault, because I didn't read the source good enough too, and my thread comment triggered this ticket of yours.

Anyway I'm a bit surprised about the way taken there to insert multiple entries, because I'd have expected the uses of cursor.executemany, that would avoid the need 'valuesClause' and make code more obvious IMHO.

### comment:10 in reply to: ↑ 9 Changed 11 months ago by jun66j5

Anyway I'm a bit surprised about the way taken there to insert multiple entries, because I'd have expected the uses of cursor.executemany, ...

I think so, too. Should use cursor.executemany.

The inserting multiple rows feature is available in SQLite 3.7.11 or later. See http://sqlite.org/changes.html#version_3_7_11. However, the SQLite is 3.6.20 in CentOS 6.

$cat /etc/redhat-release CentOS release 6.4 (Final)$ rpm -q sqlite
sqlite-3.6.20-1.el6.x86_64


### comment:11 in reply to: ↑ 9 Changed 11 months ago by ChrisNelson

Actually, I don't think there is anything wrong with the use of the Trac database API. I misread the code when opening this issue earlier, and the patch I've proposed doesn't actually result in any real changes.

Sorry about that, my brain apparently was not working earlier!

No, actually it seems like my fault, because I didn't read the source good enough too, and my thread comment triggered this ticket of yours.

Anyway I'm a bit surprised about the way taken there to insert multiple entries, because I'd have expected the uses of cursor.executemany, that would avoid the need 'valuesClause' and make code more obvious IMHO.

I'd be interested in seeing a patch that demonstrated that. I'm a DB API noob and have never found satisfying documentation so I extrapolate from examples I find and stick with things that work for me. Sometimes, those examples or that working code is not ideal.

### Modify Ticket

Change Properties