Code

Opened 4 years ago

Closed 14 months ago

Last modified 14 months ago

#12923 closed Bug (worksforme)

Base last_executed_query() does not escape parameters

Reported by: pablobm Owned by: pablobm
Component: Database layer (models, ORM) Version: 1.2-beta
Severity: Normal Keywords: last_executed_query escaping
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The base implementation of last_executed_query() does not escape its parameters. For example, a query like the following is returned:

u'SELECT * FROM things WHERE name = unscaped"stuff'

When it should be something along the lines of:

u'SELECT * FROM things WHERE name = "unscaped\"stuff"'

Nevertheless, it can be argued that BaseDatabaseOperations does not make assumptions as to what quoting scheme each different database prefers. In this case, then the problem is that the MySQL backend (and probably all others except postgresql_psycopg2) suffer this problem, by not implementing their own versions of last_executed_query().

A specific third-party piece of software that is affected by this problem is django-devserver. This raises an exception in my computer when it tries to display recently executed SQL statements that contain characters such as quotes, as it relies on last_executed_query() to do so.

I'm attaching a patch for 1.2-beta, although 1.1 is affected too. It implements a MySQL-specific solution in the appropriate backend.

Attachments (2)

mysql-last_executed_query.patch (507 bytes) - added by pablobm 4 years ago.
Patch for DatabaseOperations on the MySQL backend
last_executed_query.patch (2.9 KB) - added by pablobm 4 years ago.
Patch for DatabaseOperations, with tests for MySQL and SQLite

Download all attachments as: .zip

Change History (11)

Changed 4 years ago by pablobm

Patch for DatabaseOperations on the MySQL backend

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

If we're going to address this problem, we should address it for all backends.

comment:2 Changed 4 years ago by pablobm

  • Owner changed from nobody to pablobm
  • Status changed from new to assigned

I've been working on this one today. I'm not entirely sure what the best approach would be, so I need a bit of advice at this point. The new patch provides a test, passing for the MySQL and SQLite backends.

I'm not entirely happy about the test. Testing the strings in such a literal fashion looks a bit brittle to me. However, it's the string that we are checking after all, so couldn't think of anything better.

Also, I have created a new method on the abstract backend. It's called construct_query() and makes the SQL template and the params into a "final" SQL query. It also receives the cursor so subclasses can use backend-specific methods. I provide generic quoting+escaping, later overriden on the MySQL subclass.

The method contruct_query() is called by last_executed_query(), therefore the Unicode conversions in the latter method are kept.

Before going on to implement this for other backends, can I have some feedback on this one, please?

Changed 4 years ago by pablobm

Patch for DatabaseOperations, with tests for MySQL and SQLite

comment:3 Changed 4 years ago by pablobm

  • Patch needs improvement unset

Maybe I should have unticked the "patch needs improvement" box after offering a new patch? (Sorry, I'm not sure of how people roll here)

comment:4 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:5 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:6 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:7 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:8 Changed 14 months ago by claudep

  • Resolution set to worksforme
  • Status changed from assigned to closed

The current code either gets the query from the backend (which should be properly escaped) or provides the query and params separated ("QUERY = %r - PARAMS = %r"). Trying to reconstruct the backend query is probably bound to fail and is not Django's business. Therefore I don't think this issue is still valid.

comment:9 Changed 14 months ago by aaugustin

For what it's worth, reconstructing the backend query is doable in about 5 lines of code. I attached a patch that did that to another ticket one or two years ago. However, the technique I used was rejected by the BDFL and the ticket closed as wontfix.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.