Opened 9 years ago

Closed 7 years ago

# OperationalError: near "t": syntax error (Trac 0.11.6)

Reported by: Owned by: anonymous Ryan J Ollos highest TracMetrixPlugin blocker 0.11

### Description

#7610: OperationalError: near "t": syntax error

Reporter: frederic.olivie@… | Owner:

Type: defect | Status: new

Priority: normal | Milestone: not applicable

Component: general | Version: 0.11rc2

Severity: normal | Keywords:

#### How to Reproduce

While doing a GET operation on /pdashboard, Trac issued an internal error.

Request parameters:

{'imagename': None}


User Agent was: Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.16) Gecko/20080702 Firefox/2.0.0.16

#### System Information

20061115 (prerelease) (Debian 4.1.1-21)]
 Trac 0.11rc2 Python 2.4.4 (#1, Apr 16 2008, 17:56:48)  [GCC 4.1.2 setuptools 0.6c8 SQLite 3.3.8 pysqlite 2.3.2 Genshi 0.6dev-r884 mod_python 3.2.10 Subversion 1.4.2 (r22196) jQuery: 1.2.3

#### Python Traceback

Traceback (most recent call last):
File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/web/main.py", line 423, in
_dispatch_request
dispatcher.dispatch(req)
File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/web/main.py", line 197, in dispatch
resp = chosen_handler.process_request(req)
File "build/bdist.linux-x86_64/egg/tracmetrixplugin/web_ui.py", line
122, in process_request
File "build/bdist.linux-x86_64/egg/tracmetrixplugin/web_ui.py", line
165, in _render_view
File "build/bdist.linux-x86_64/egg/tracmetrixplugin/model.py", line 94,
in get_ticket_resolution_group_stats
File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/db/util.py", line 51, in execute
return self.cursor.execute(sql)
File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/db/sqlite_backend.py", line 58, in
execute
args or [])
File "/usr/lib/python2.4/site-
packages/Trac-0.11rc2-py2.4.egg/trac/db/sqlite_backend.py", line 50, in
_rollback_on_error
return function(self, *args, **kwargs)
OperationalError: near "t": syntax error



### comment:1 Changed 9 years ago by anonymous

Priority: normal → highest normal → blocker

### comment:2 Changed 8 years ago by Bhuricha Deen Sethanandha

I can't reproduce this error. Please use the trunk version and try again.

### comment:3 Changed 8 years ago by Ryan J Ollos

Resolution: → worksforme new → closed

This is probably no longer valid with a stable release of Trac.

### comment:4 follow-up:  6 Changed 7 years ago by paul.amor@…

Resolution: worksforme closed → reopened OperationalError: near "t": syntax error (Trac 0.11rc2) → OperationalError: near "t": syntax error (Trac 0.11.6)

I can confirm that this issue definitely still exists when installed from the trunk, though the line numbers of the trace are slightly different the route is the same. If you need further info please let me know

### comment:5 Changed 7 years ago by Ryan J Ollos

Owner: changed from Bhuricha Deen Sethanandha to Ryan J Ollos reopened → new

Could you please test again with the latest version of the Trunk? If you still see the error, please send the debug trace and details of your installation. Thanks.

### comment:6 in reply to:  4 Changed 7 years ago by Ryan J Ollos

I can confirm that this issue definitely still exists when installed from the trunk

In particular, which database are you using?

Ticket will be closed in one month if no response.

### comment:7 Changed 7 years ago by jnelson@…

I also have this with a 0.11.7 install of Trac, using the TracMetrixPlugin installer offered from the Download link on the Trac page (http://trac-hacks.org/wiki/TracMetrixPlugin).

Have since upgraded Trac to 0.12 with no change on the error, although now it's unclear from the site that this plugin is 0.12 compatible.

### comment:8 Changed 7 years ago by jnelson@…

(Please disregard obviously dumb comment about 0.12 compatibility)

As an update, have just finished installing 0.12 version of this plugin. On doing the trac-admin upgrade call, the only spew I got was "Database is up to date, no upgrade necessary." Did not get "TracMetrixPlugin needs an upgrade" as I was told to expect. Still getting same error message as stated in this bug.

### comment:9 Changed 7 years ago by jnelson@…

Traceback and system information:

Python Traceback
Most recent call last:
File "build/bdist.linux-i686/egg/trac/web/main.py", line 513, in _dispatch_request
File "build/bdist.linux-i686/egg/trac/web/main.py", line 235, in dispatch
File "build/bdist.linux-i686/egg/tracmetrixplugin/web_ui.py", line 181, in process_request
File "build/bdist.linux-i686/egg/tracmetrixplugin/web_ui.py", line 228, in _render_view
File "build/bdist.linux-i686/egg/tracmetrixplugin/model.py", line 95, in get_ticket_resolution_group_stats
File "build/bdist.linux-i686/egg/trac/db/util.py", line 66, in execute
File "build/bdist.linux-i686/egg/trac/db/sqlite_backend.py", line 78, in execute
File "build/bdist.linux-i686/egg/trac/db/sqlite_backend.py", line 56, in execute
File "build/bdist.linux-i686/egg/trac/db/sqlite_backend.py", line 48, in _rollback_on_error

System Information:
User Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.472.63 Safari/534.3

Trac	0.12
Genshi	0.6
pysqlite	2.4.0
Python	2.5.2 (r252:60911, Jan 20 2010, 21:48:48) [GCC 4.2.4 (Ubuntu 4.2.4-1ubuntu3)]
pytz	2007k
RPC	1.0.6
setuptools	0.6c8
SQLite	3.4.2
Subversion	1.4.6 (r28521)
jQuery	1.4.2


### comment:10 Changed 7 years ago by jnelson@…

I figured it out. Your code can't handle a tick mark in a bug resolution. I had a resolution value "Won't fix", and that was breaking it.

### comment:11 Changed 7 years ago by Ryan J Ollos

Issue confirmed when replacing default Trac resolution wontfix with won't fix. Thanks for tracking this down ... it was a tricky issue, no doubt.

### comment:12 Changed 7 years ago by Ryan J Ollos

(In [9166]) Replace possible single-quotes in resolution strings with double single-quotes, as required for SQLite queries (see http://www.sqlite.org/faq.html#q14). Refs #3684.

### comment:13 Changed 7 years ago by Ryan J Ollos

Resolution: → fixed new → closed

(In [9167]) Merged [9166] into 0.12 branch. Fixes #3684.

### comment:14 Changed 7 years ago by Odd Simon Simonsen

Resolution: fixed closed → reopened

Just noticed this change by accident, but had to speak up as this is bad code practice...:

Trac and the underlying connector does all the needed escaping for you if used correctly - and 'correctly' is not by using string interpolation - depending in making right strings for all platforms will quite likely leave holes for SQL injection attacks.

The method cursor.execute is defined as cursor.execute(self, sql, args=None), and you identify any arg in sql as a plain %s regardless of type - type is looked up by the underlying mechanisms that translates correctly between underlying db connectors (usually the same, but may be different). It will take care of mapping, escaping and all that is needed. Review any SELECT in the main Trac code base to see more examples of usage with arguments.

So, it should be like this (simplified):

cursor.execute("SELECT * FROM ticket WHERE resolution=%s", [type.name])


BTW, type is a Python built-in name, and re-defining it as loop key is bad practice and may well get you in trouble - the newly defined type will exist for the duration of the method and if anything else further down depends on the original type they will be disappointed and barf errors...

I don't use the plugin, and haven't really seen more code than the changeset linked to above. However, I'm reopening the ticket as it really needs fixing - sorry to butt in... :-)

### comment:15 Changed 7 years ago by Ryan J Ollos

Status: reopened → new

Osimons, thank you for the feedback! I'm glad this is all handled more elegantly in Trac than the ugly fix that I put in place, for lack of knowing any better. I'll commit a fix shortly and appreciate if you can review that as well.

### comment:16 Changed 7 years ago by Odd Simon Simonsen

See trac:wiki:TracDev/DatabaseApi for much more details about DB layer.

### comment:17 Changed 7 years ago by Ryan J Ollos

Status: new → assigned

I spent some time reading t:wiki:TracDev/DatabaseApi#RulesforDBAPIUsage and studying the Trac source code, and have refactored the get_ticket_resolution_group_stats method accordingly. There is a lot more work to do on this plugin however, as just the two mis-uses that Osimons mentioned are extensive.

Osimons, if you have a chance to take a quick look at the changes, I'd appreciate any feedback.

I'm a bit puzzled by this use case, from get_ticket_group_stats in roadmap.py (Trac 0.11.7):

str_ids = [str(x) for x in sorted(ticket_ids)]
cursor.execute("SELECT status, count(status) FROM ticket "
"WHERE id IN (%s) GROUP BY status" %
",".join(str_ids))


This seems to go against the rule of don't use string formatting for anything other than dynamically specifying the names of tables and columns. However, I don't see any example in the Trac code where a list of integers is passed as an argument in a call to execute, so I can't see another way to accomplish this.

### comment:18 Changed 7 years ago by Ryan J Ollos

(In [9463]) Refactoring of get_ticket_resolution_group_stats following suggestions by Osimons. Refs #3684.

### comment:19 Changed 7 years ago by Ryan J Ollos

Note to self: merge these changes into the 0.12 branch after feedback.

### comment:20 follow-ups:  22  23 Changed 7 years ago by Odd Simon Simonsen

For the string interpolation example, what you need to do is make the string look like... IN (%s, %s, %s, %s) - with one %s for each value. Then all the actual values goes into the args list for replacement by cursor.execute(). Something like this:

cursor.execute("SELECT status, count(status) FROM ticket "
"WHERE id IN (%s) GROUP BY status" %
",".join(['%s']*len(ticket_ids)), sorted(ticket_ids))


The roadmap.py example you found does not do it quite correctly, but that is likely also a concious decision as any input for this particular execute is just guaranteed to be existing ticket ids (safe numbers auto-generated by Trac) and therefore no user input that ever needs escaping. The moment you add resolution= (like in your changeset), that is a value that is input by users and must be escaped.

The good practice is always to escape properly though, so that you or other maintainer later do not need to research the origin of each possible input. Escape and forget :-)

### comment:21 Changed 7 years ago by Ryan J Ollos

(In [9466]) Format SQL string so that it is escaped properly. Refs #3684.

### comment:22 in reply to:  20 Changed 7 years ago by Ryan J Ollos

The good practice is always to escape properly though, so that you or other maintainer later do not need to research the origin of each possible input. Escape and forget :-)

It's starting to make a lot of sense now. Thanks for your advice and detailed explanation! Hopefully the code is looking better.

Looks like there are a couple more SQL query strings and uses of type as a variable name, so I'll take care of those tomorrow and then close this ticket.

### comment:23 in reply to:  20 ; follow-up:  24 Changed 7 years ago by Ryan J Ollos

One more quick question. Does your use of sorted have any significance such as speeding up the query? I couldn't immediately see a reason for it, but I'm also very new to working with databases, so its all new to me.

### comment:24 in reply to:  23 Changed 7 years ago by Odd Simon Simonsen

One more quick question. Does your use of sorted have any significance such as speeding up the query? I couldn't immediately see a reason for it, but I'm also very new to working with databases, so its all new to me.

I just left it in seeing the code you found had it. I doubt you'd notice any difference, so the net cost of sorting first is likely negative. However, if you turn on SQL debug logging then the output in order will be more "developer friendly" when reading :-)

### comment:25 Changed 7 years ago by Ryan J Ollos

Resolution: → fixed assigned → closed

(In [9563]) Refactored SQL queries so that strings were properly formatted for escaping by the underlying API. Fixes #3684.

### Modify Ticket

Change Properties
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.