Modify

Opened 19 months ago

Closed 6 months ago

#11027 closed defect (fixed)

Cross-db compatibility issue (SQLite can't insert multiple rows at once)

Reported by: rjollos Owned by: ChrisNelson
Priority: high Component: TracJsGanttPlugin
Severity: major Keywords:
Cc: hasienda Trac Release:

Description (last modified by rjollos)

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 b class TicketRescheduler(Component): 
    29042904                            values.append(t['id'])
    29052905                            values.append(to_utimestamp(self.pm.start(t)))
    29062906                            values.append(to_utimestamp(self.pm.finish(t)))
    2907                     cursor.execute('INSERT INTO schedule' + \
    2908                                        ' (ticket, start, finish)' + \
    2909                                        ' VALUES %s' % valuesClause,
    2910                                    values)
    2911 
     2907                    cursor.execute("""
     2908                        INSERT INTO schedule (ticket, start, finish)
     2909                        VALUES %s""" % valuesClause, values)
    29122910
    29132911                    # Finally, add history records to schedule_change
    29142912                    # for newly scheduled tickets.

Attachments (0)

Change History (29)

comment:1 Changed 19 months ago by rjollos

  • Description modified (diff)

comment:2 Changed 19 months ago by rjollos

  • Description modified (diff)

comment:3 Changed 19 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
estimate_pad = 0.0
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
prim_developer.options = na|allison|aracicot|chander|gcorradini|pmc2|rfsl|winkey
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 19 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 19 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: Changed 19 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.

I can't reproduce your specific issue. Maybe some more information is required. Could you post the [components] section from your trac.ini?

comment:7 Changed 19 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.admin.themeadminmodule = enabled
themeengine.api.themeenginesystem = enabled
themeengine.web_ui.themeenginemodule = enabled
timingandestimationplugin.* = enabled
trac.web.auth.loginmodule = disabled
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 19 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: Changed 19 months ago by hasienda

Replying to 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!

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 19 months ago by jun66j5

Replying to hasienda:

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 19 months ago by ChrisNelson

Replying to hasienda:

Replying to 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!

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.

comment:12 Changed 6 months ago by ChrisNelson

  • Priority changed from normal to high
  • Severity changed from normal to major

comment:13 Changed 6 months ago by ChrisNelson

  • Status changed from new to accepted

comment:14 Changed 6 months ago by ChrisNelson

I don't really see how this can be subject to SQL injection. The valuesClause is a list of triples each of which consists of:

  • The numeric ticket ID from Trac
  • The result of parsing two date strings, each is either a valid date or None
Last edited 6 months ago by ChrisNelson (previous) (diff)

comment:15 follow-up: Changed 6 months ago by ChrisNelson

Considering

  • tracjsganttplugin/0.11/tracjsgantt/tracpm.py

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

what does that """ do here? Doesn't """ just makes multi-line strings? If so, the after is semantically equivalent to the former.

StackOverlfow suggests I can leave off the " + \" and rely on implicit concatenation, as in C.

comment:16 in reply to: ↑ description ; follow-up: Changed 6 months ago by ChrisNelson

Replying to rjollos:

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. ...

I don't see how I violate the Rules. I should not use "string formatting" to put values in the query. And I don't.

        valuesClause = ','.join(('(%s,%s,%s)',) * len(toInsert))
        values = []
        for t in tickets:
            if t['id'] in toInsert:
                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)

I add valuesClause to the SQL with string formatting (%s) like the example from the Rules about a table name. valuesClause may be

  • '(%s,%s,%s)'
  • '(%s,%s,%s),(%s,%s,%s)'

and so on. (It can't be '' because we don't get to this code unless len(toInsert) is > 0.) So for two tickets to insert, I get

        cursor.execute('INSERT INTO schedule' + \
                           ' (ticket, start, finish)' + \
                           ' VALUES %s' % '(%s,%s,%s),(%s,%s,%s)',
                       values)

and values has six values in it (id, ts, ts, id, ts, ts). What's wrong with that?

Last edited 6 months ago by ChrisNelson (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 6 months ago by ChrisNelson

Replying to ChrisNelson:

... So for two tickets to insert, I get

        cursor.execute('INSERT INTO schedule' + \
                           ' (ticket, start, finish)' + \
                           ' VALUES %s' % '(%s,%s,%s),(%s,%s,%s)',
                       values)

and values has six values in it (id, ts, ts, id, ts, ts). What's wrong with that?

To carry this one step further, since the % will be processed before the function call, this evaluates to

        cursor.execute('INSERT INTO schedule' + \
                           ' (ticket, start, finish)' + \
                           ' VALUES (%s,%s,%s),(%s,%s,%s)',
                       values)

and we're using the DB API to fill in the %s's with the elements of values.

comment:18 follow-up: Changed 6 months ago by jun66j5

INSERT statement with multiple rows is not cross-db compatibility. Instead, should use cursor.executemany().

$ sqlite3
SQLite version 3.3.6
Enter ".help" for instructions
sqlite> CREATE TABLE test (id INTEGER, col1 TEXT);
sqlite> INSERT INTO test (id, col1) VALUES (1, '111'), (2, '222');
SQL error: near ",": syntax error
sqlite> INSERT INTO test (id, col1) VALUES (1, '111');
sqlite> SELECT * FROM test;
1|111

comment:19 in reply to: ↑ 18 Changed 6 months ago by ChrisNelson

Replying to jun66j5:

INSERT statement with multiple rows is not cross-db compatibility. ...

THANK YOU!

comment:20 Changed 6 months ago by ChrisNelson

  • Summary changed from Cross-db compatibility issues and possibility of SQL injection to Cross-db compatibility issue (SQLite can't insert multiple rows at once)

comment:21 Changed 6 months ago by ChrisNelson

Short form:

  • Must use executemany() to INSERT multiple rows for cross-db compatibility (not supported in SQLlite)
  • Cannot use executemany() to SELECT (not supported at all)

comment:22 Changed 6 months ago by ChrisNelson

Considering, for example, some MySQL help, (and pyodbc) the structure of my values has to change from

(r1v1, r1v2, ..., r1vn, r2v1, r2v2, ...)

to

[ (r1v1, r1v2, ... r1vn), (r2v1, ... r2vn) ...]`

Easy enough.

comment:23 Changed 6 months ago by ChrisNelson

In 13868:

Use executemany() when inserting multiple rows. Refs #11027.

Required for cross-db compatibility.

Light testing shows this still works for me in PostgreSQL so it
doesn't seem I broke anything. I need SQLite feedback, though.

comment:24 in reply to: ↑ 15 ; follow-up: Changed 6 months ago by hasienda

Replying to ChrisNelson:

what does that """ do here? Doesn't """ just makes multi-line strings? If so, the after is semantically equivalent to the former.

Mostly readability. You can achieve the same by positioning multi-line SQL statements aligned like done in the triple-quoted string example.

StackOverflow suggests I can leave off the " + \" and rely on implicit concatenation, as in C.

Definitely. You'll find plenty of examples relying on string concatenation inside braces, while your style has been rather surprising to me.

Anyway, the main point has been about multiple inserts, what we obviously failed to make clear in ticket description as well as early comments. Sorry for that.

comment:25 in reply to: ↑ 24 ; follow-up: Changed 6 months ago by ChrisNelson

Replying to hasienda:

Replying to ChrisNelson: ...

StackOverflow suggests I can leave off the " + \" and rely on implicit concatenation, as in C.

Definitely. You'll find plenty of examples relying on string concatenation inside braces, while your style has been rather surprising to me. ...

Perhaps emacs's Python mode indents

#py
f('Foo' + \
  ' Bar')

better than

#py
f('Foo'
  ' Bar')

? I'd have to check.

comment:26 in reply to: ↑ 25 ; follow-up: Changed 6 months ago by ChrisNelson

Replying to ChrisNelson:

... Perhaps emacs's Python mode indents

#py
f('Foo' + \
  ' Bar')

better than

#py
f('Foo'
  ' Bar')

? I'd have to check.

I suppose the definition of "better" is open to discussion. emacs gives me:

f('Foo' + \
      ' Bar')

g('Stuff'
  ' and nonsense')

I rather like

f('Foo' + \
      ' Bar',
  theSecondArg)

comment:27 in reply to: ↑ 26 Changed 6 months ago by hasienda

Replying to ChrisNelson:

I suppose the definition of "better" is open to discussion. emacs gives me:

Agreed. At that time it was a minor consent, that SQL statements formatted like

INSERT into table (col1, col2)
VALUES (val1, val2)

improved source readability compared to semantically identical oneliners and line wrapping on PEP8 style, subject to alterations by personal tast, sure. OTOH the '+ \' is allowed but technically obsolete, definitely. Just that.

comment:28 Changed 6 months ago by ChrisNelson

Any chance of getting this validated in SQLite? I don't have a system to do it on. I may just close it and hope for the best.

comment:29 Changed 6 months ago by ChrisNelson

  • Resolution set to fixed
  • Status changed from accepted to closed

No feedback in two weeks. I'm going to call this good. We can reopen if it's still an issue.

Add Comment

Modify Ticket

Action
as closed The owner will remain ChrisNelson.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.