Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#12923 closed Bug (worksforme)

Base last_executed_query() does not escape parameters

Reported by: Pablo Brasero Owned by: Pablo Brasero
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 Pablo Brasero 7 years ago.
Patch for DatabaseOperations on the MySQL backend
last_executed_query.patch (2.9 KB) - added by Pablo Brasero 7 years ago.
Patch for DatabaseOperations, with tests for MySQL and SQLite

Download all attachments as: .zip

Change History (11)

Changed 7 years ago by Pablo Brasero

Patch for DatabaseOperations on the MySQL backend

comment:1 Changed 7 years ago by Russell Keith-Magee

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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

comment:2 Changed 7 years ago by Pablo Brasero

Owner: changed from nobody to Pablo Brasero
Status: newassigned

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 7 years ago by Pablo Brasero

Attachment: last_executed_query.patch added

Patch for DatabaseOperations, with tests for MySQL and SQLite

comment:3 Changed 7 years ago by Pablo Brasero

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 6 years ago by Luke Plant

Type: Bug

comment:5 Changed 6 years ago by Luke Plant

Severity: Normal

comment:6 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:7 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:8 Changed 4 years ago by Claude Paroz

Resolution: worksforme
Status: assignedclosed

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 4 years ago by Aymeric Augustin

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.

Note: See TracTickets for help on using tickets.
Back to Top