Parameters are not adapted or quoted in Query.__str__

[Dmitry Dygalo]

Now it is just string interpolation of the SQL template with parameters and in most cases produces invalid queries for the following reasons:

• No quoting
• No adaptation. So, some python objects will be used as is, not like their SQL equivalents

Yes, there are situations, when output of Query.__str__ is equal to actual query. But for debugging reasons, it will be better to see real query here. Also it is logical and expected behavior of this method - to show actual query.

[Dmitry Dygalo]

Initial draft

[Dmitry Dygalo]

Commit: ​https://github.com/Stranger6667/django/commit/b2e36668a6877fe29923493e5669402dd30d6421
This issue is more general case of #24991

[Tim Graham]

This looks like a duplicate of #17741 and #25092 which are "wontfix", however, I'm not sure I see any harm in your proposal besides the question of whether or not it can be implemented on other database backends.

[Bernd Wechner]

Replying to Dmitry Dygalo:

Now it is just string interpolation of the SQL template with parameters and in most cases produces invalid queries for the following reasons:

• No quoting
• No adaptation. So, some python objects will be used as is, not like their SQL equivalents

Yes, there are situations, when output of Query.__str__ is equal to actual query. But for debugging reasons, it will be better to see real query here. Also it is logical and expected behavior of this method - to show actual query.

Fascinating. I inadvertantly filed a duplicate of this it seems:

https://code.djangoproject.com/ticket/30132#ticket

and I would dearly love this fixed. In essence Query.__str__ should return exactly what is logged as per here:

​https://docs.djangoproject.com/en/2.1/faq/models/#how-can-i-see-the-raw-sql-queries-django-is-running

so we can reliably see and extract teh SQL of a query in a production environment in which DEBUG is not enabled!

I would gladly fix this and PR it, if I had the skills and I am close to that, as I am coding and debugging python, but single stepping into Query.__str__ didn't find me a quick easy answer and I bailed for now. So if you have any tips as to where the code is that Query.__str__ uses to generate SQL and where the code is that logs SQL to connection.queries I canbegin to look at it consider how this might be fixed.

It seems to me an experience Django coder could fix this in minutes and that there should be a regression test for this kind of code:

qs=model.objects.somequeryset
sql=str(qs.query)
raw_qs=model.objects.raw(sql)

I have a test bed in which I confirmed this failure here:

​​https://github.com/bernd-wechner/DjangoTutorial/blob/238787c83ef8515aeeb405577980e71ff35664e8/Library/views.py

Let me know how I might help get this fixed sooner, given ti was reported 3 years ago and is still not fixed!

[GitHub <noreply@…>]

In 845042b3:

Refs #25705 -- Fixed invalid SQL generated by SQLFuncMixin.as_sql() in custom_lookups tests.

Generated SQL was invalid because parameters are quoted by a driver.

[Mariusz Felisiak]

IMO we should close all related tickets as duplicates:

• #24803 was marked as a duplicate (empty strings in parameters),
• #24991 was marked as a duplicate (range types in parameters).

[Alex]

I've done some investigation.

The main issue comes to the Python DB API doesn't have a way to do this. The only way to see the query with the parameters binded correctly is after executing it.

As Mariusz commented ​here, only Mysql/MariaDB and Postgres have a way to do it via a mogrify function which is their own extension to the API.
Django is already using the postgres mogrify in its own compose_sql function in the ​search backend, ​schema queries and ​ensuring the role of the connection

In Oracle and SQLite, none of the extensions to the API they add allows do this.

I see two approaches:

• We fix this issue for the first 3 backends and leave it as it is in Oracle and SQLite.
• Use the mogrify function in the first three backends, and manually quote the parameters in the other two. Something similar was already attempted in ​this PR and it was rejected. The amount of effort needed to implement and maintain this, ​example on how cPython does it for SQLite, would probably be too much since this seems to be the only use case.

[Simon Charette]

My concerns with fixing this issue related to comments such as comment:5

It seems to me an experience Django coder could fix this in minutes and that there should be a regression test for this kind of code:

qs=model.objects.somequeryset
sql=str(qs.query)
raw_qs=model.objects.raw(sql)

We absolutely don't want to support this pattern in a context where we can't guarantee that the proper quoting is performed on all supported backends as that might result in SQL injection problems. In this sense I think that it's a good thing that sql.Query.__str__ doesn't attempt to perform the proper quoting to make it clear it should not be used for this purpose.

I'd much rather see us document sql.Query.sql_with_params(using: str = DEFAULT_DB_ALIAS) which could be used as

sql, params = qs.query.sql_with_params()
model.objects.raw(sql, params)

over spending time trying to dangerously emulate parameters quoting.

[Carlton Gibson]

@Simon do you think we should close this ticket as wontfix.

I can recall 2 or 3 (or 4?) occasions since say DjangoCon US 2018 where a (new) contributor has tried to pick this up, only to hit the same wall. The trail of breadcrumbs gets slightly clearer each time, but, is there a realistic fix for this issue, and if not can we not say so?

[Alex]

Replying to Simon Charette:

My concerns with fixing this issue related to comments such as comment:5

It seems to me an experience Django coder could fix this in minutes and that there should be a regression test for this kind of code:

qs=model.objects.somequeryset
sql=str(qs.query)
raw_qs=model.objects.raw(sql)

We absolutely don't want to support this pattern in a context where we can't guarantee that the proper quoting is performed on all supported backends as that might result in SQL injection problems. In this sense I think that it's a good thing that sql.Query.__str__ doesn't attempt to perform the proper quoting to make it clear it should not be used for this purpose.

I'm not sure I understand your point. I think the original commenter was suggesting that just as a simple (and probably not ideal) way to do the test. I wouldn't consider someone doing str(qs.query) to then pass in to raw() (which the docs already warn of SQL injections) a real use case that anyone would do.
Personally, the use case I've had with this issue is printing the query to then try to format it and maybe execute it in a sql editor connected that my database while testing stuff in local. For that case your suggestion of sql_with_params wouldn't cover it. I'm not against having that method either, but I don't see it as a replacement of a correctstr(qs.query).

Also my recommended choice was not trying to emulate parameter quoting, but to use the database library mogrify method in 3 of the 5 supported backends.

[Simon Charette]

@Carlton I think the request for this feature is a symptom of a different problem so maybe we could re-purpose this ticket instead?

---

@Alex

I wouldn't consider someone doing str(qs.query) to then pass in to raw() (which the docs already warn of SQL injections) a real use case that anyone would do.

I don't agree on that point. From helping users through various forums in most cases the reason why they were bringing up the issue of improper quoting was when they tried executing the query as is. The duplicates of this ticket seem to support this position (see #17741 and #25092 ​SO) and I don't think would be hard to find other examples.

It might not be your personal use case as you are well versed into why this a bad idea but I think it would be irresponsible to our user base from an API design (newcomers footgun) and maintenance burden perspective (think of the security and bug reports about mogrify not producing the exact same SQL as backend escaping normally does).

Also my recommended choice was not trying to emulate parameter quoting, but to use the database library mogrify method in 3 of the 5 supported backends.

You second alternative mentioned

Use the mogrify function in the first three backends, and manually quote the parameters in the other two.

I think that would qualify as a form of emulation or best-effort wouldn't it? What guarantees do we have that the quoting is appropriate for all datatypes that these backends support as parameters? Are we feeling confident, knowing that users will attempt to pipe str(qs.query) into raw and that Query.__str__ always use DEFAULT_DB_ALIAS for compilation #25947 (even if the proper backend can often only be determined as execution time) that we are exposing the right tools to users and we can commit to them being safe for the years to come?

IMO the usage of raw(str(qs.query)), and the main motive for this ticket, is a symptom of a lack of documented way for safely building and executing the SQL and parameters from a QuerySet object which makes me believe the focus should be on documenting queryset.qs.sql_with_params() first instead?

As for the printing and copy-pasting into a shell use case we've reached an agreement that the only way to see the query with the parameters binded correctly is after executing it. Knowing that, and the fact Query.__str__ is still pretty legible even without the exact mogrifying that only leaves the copy-pasting into a shell use case. Should we make it easier to retrieve the exact SQL associated with a particular queryset instead? Something like QuerySet.executed_query: str | None that is only present on evaluated querysets.

These appears like solutions that prevent misuse of Query.__str__ and avoid maintaining correct and safe x-backend mogrifying solutions?

[Alex]

Replying to Simon Charette:

I wouldn't consider someone doing str(qs.query) to then pass in to raw() (which the docs already warn of SQL injections) a real use case that anyone would do.

I don't agree on that point. From helping users through various forums in most cases the reason why they were bringing up the issue of improper quoting was when they tried executing the query as is. The duplicates of this ticket seem to support this position (see #17741 and #25092 ​SO) and I don't think would be hard to find other examples.

If the worry is raw(str(qs.query)), then not having the quoting is a security issue.
In fact I can actually do an SQL injection abusing this lack of quoting.

qs = User.objects.filter(first_name='"test")--', is_staff=True).only('id')
>>> <QuerySet []>
list(User.objects.raw(str(qs.query)))
>>> [<User: test>]
list(User.objects.raw(str(qs.query)))
>>> 'SELECT "auth_user"."id" FROM "auth_user" WHERE ("auth_user"."first_name" = "test")-- AND "auth_user"."is_staff")'

With proper quoting, as the one done when executing normally, the query is SELECT "auth_user"."id" FROM "auth_user" WHERE ("auth_user"."first_name" = \'"test")--\' AND "auth_user"."is_staff")
So if raw(str(qs.query)) is a risk, then quoting the parameters would fix it.

Also my recommended choice was not trying to emulate parameter quoting, but to use the database library mogrify method in 3 of the 5 supported backends.

You second alternative mentioned

Use the mogrify function in the first three backends, and manually quote the parameters in the other two.

I think that would qualify as a form of emulation or best-effort wouldn't it? What guarantees do we have that the quoting is appropriate for all datatypes that these backends support as parameters? Are we feeling confident, knowing that users will attempt to pipe str(qs.query) into raw and that Query.__str__ always use DEFAULT_DB_ALIAS for compilation #25947 (even if the proper backend can often only be determined as execution time) that we are exposing the right tools to users and we can commit to them being safe for the years to come?

And as I said just after that, that alternative was rejected in the past, would be too much effort and I was only mentioning it as the only way to fix it in every backend. I should have been clearer that I wasn't pushing for this option.

IMO the usage of raw(str(qs.query)), and the main motive for this ticket, is a symptom of a lack of documented way for safely building and executing the SQL and parameters from a QuerySet object which makes me believe the focus should be on documenting queryset.qs.sql_with_params() first instead.

That idea was rejected 14 months ago, but it could be reconsidered in light of this discussion.
Also, I didn't actually find qs.query being documented, mostly this ​mention about pickling querysets.

As for the printing and copy-pasting into a shell use case we've reached an agreement that the only way to see the query with the parameters binded correctly is after executing it. Knowing that, and the fact Query.__str__ is still pretty legible even without the exact mogrifying that only leaves the copy-pasting into a shell use case. Should we make it easier to retrieve the exact SQL associated with a particular queryset instead? Something like QuerySet.executed_query: str | None that is only present on evaluated querysets.

I would be happy with this option. An alternative would be an error if the queryset hasn't been evaluated instead of None. But I'm not that big of a fan of the error idea.

These appears like solutions that prevent misuse of Query.__str__ and avoid maintaining correct and safe x-backend mogrifying solutions?

However, my initial investigation was not deep enough and I found more reasons to discard the mogrify option:

• In mysqlclient, mogrify was added in version ​2.2.0 and django supports ​1.4.3 onwards, so there would have to be an additional check and a difference in behavior.
• In the MySQL Connector/Python driver I couldn't find a mogrify method. Not the recommended driver, but still supported.
• In psicopg3, mogrify is only on the ​ClientCursor class. While that's the default in Django, server side is also ​supported. Again, an additional check. Also, I'm not sure if this means that these usages of mogrify I mentioned in the postgres backend don't work with server side parameter binding.

Django is already using the postgres mogrify in its own compose_sql function in the ​search backend, ​schema queries and ​ensuring the role of the connection

My proposal is to close this as a wontfix and work on the QuerySet.executed_query idea and maybe reconsider documenting sql_with_params
I'm happy to do both as long as I don't get yelled for reopenniing the wonfix sql_with_params ticket #34636

[Simon Charette]

So if raw(str(qs.query)) is a risk, then quoting the parameters would fix it.

I was not arguing that it is not possible to cause SQL injection today by using raw(str(qs.query)) but that the moment we do fix it by quoting parameters we must ensure to safely support this anti-pattern going forward unless we document sql_with_params as the blessed raw of doing that.

And as I said just after that, that alternative was rejected in the past, would be too much effort and I was only mentioning it as the only way to fix it in every backend. I should have been clearer that I wasn't pushing for this option.

Sorry I missed that, I thought you were pushing for this option.

That idea was rejected 14 months ago, but it could be reconsidered in light of this discussion.

I wasn't aware that documenting sql_with_params was rejected in #34636. I do think we should reconsider.

I would be happy with this option. An alternative would be an error if the queryset hasn't been evaluated instead of None. But I'm not that big of a fan of the error idea.

I don't have a strong opinion on the subject, either works for me!

My proposal is to close this as a wontfix and work on the QuerySet.executed_query idea and maybe reconsider documenting sql_with_params. I'm happy to do both as long as I don't get yelled for reopenniing the wonfix sql_with_params ticket #34636.

I'm happy to support you towards re-opening #34636 but we should likely follow the normal process of gathering a bit more consensus. It'd be great to hear from Mariusz and others what their thoughts are on the matter.

[Carlton Gibson]

FWIW I’ve wanted sql_with_params() to be public for other reasons, so I’d be keen there is there are no blockers

[Mariusz Felisiak]

Replying to Carlton Gibson:

FWIW I’ve wanted sql_with_params() to be public for other reasons, so I’d be keen there is there are no blockers

I don't mind documenting sql_with_params(), but would like to avoid encouraging users to use raw SQL queries. Also, sql_with_params() is not a panacea, it has it's limitation e.g. it always uses the default database connection. For most users it may be tricky to include parameters into an SQL string which can lead to SQL injection vectors. IMO, sql_with_params() is only for advanced users, who know the risks.

[Simon Charette]

Thank you for chiming in Mariusz.

I don't mind documenting sql_with_params(), but would like to avoid encouraging users to use raw SQL queries.. [snip]
For most users it may be tricky to include parameters into an SQL string which can lead to SQL injection vectors. IMO, sql_with_params() is only for advanced users, who know the risks.

I share this sentiment but I believe that there will always be a need for an escape hatch and if we don't provide a safe one users will follow the path of least resistance that sql.Query.__str__ provides. My hope is that with the documentation of sql_with_params we could even consider disallowing passing str(qs.query) to raw and cursor.execute completely to prevent its misuse. Something like

django/db/backends/utils.py

@@ -9 +9 @@
 
 from django.apps import apps
 from django.db import NotSupportedError
+from django.db.utils import QueryStr
 from django.utils.dateparse import parse_time
 
 logger = logging.getLogger("django.db.backends")
@@ -94 +95 @@
     def _execute(self, sql, params, *ignored_wrapper_args):
         # Raise a warning during app initialization (stored_app_configs is only
         # ever set during testing).
+        if isinstance(sql, QueryStr):
+            raise TypeError("str(qs.query) cannot be passed directly to execute(sql).")
         if not apps.ready and not apps.stored_app_configs:
             warnings.warn(self.APPS_NOT_READY_WARNING_MSG, category=RuntimeWarning)
         self.db.validate_no_broken_transaction()
@@ -107 +110 @@
     def _executemany(self, sql, param_list, *ignored_wrapper_args):
         # Raise a warning during app initialization (stored_app_configs is only
         # ever set during testing).
+        if isinstance(sql, QueryStr):
+            raise TypeError("str(qs.query) cannot be passed directly to execute(sql).")
         if not apps.ready and not apps.stored_app_configs:
             warnings.warn(self.APPS_NOT_READY_WARNING_MSG, category=RuntimeWarning)
         self.db.validate_no_broken_transaction()

django/db/models/sql/query.py

@@ -18 +18 @@
 
 from django.core.exceptions import FieldDoesNotExist, FieldError
 from django.db import DEFAULT_DB_ALIAS, NotSupportedError, connections
+from django.db.utils import QueryStr
 from django.db.models.aggregates import Count
 from django.db.models.constants import LOOKUP_SEP
 from django.db.models.expressions import (
@@ -145 +146 @@
     """A single raw SQL query."""
 
     def __init__(self, sql, using, params=()):
+        if isinstance(sql, QueryStr):
+            raise TypeError("Query strings cannot be safely used by RawQuery. Use sql_with_params().")
         self.params = params
         self.sql = sql
         self.using = using
@@ -191 +194 @@
 
     def __str__(self):
         if self.params_type is None:
-            return self.sql
-        return self.sql % self.params_type(self.params)
+            return QueryStr(self.sql)
+        return QueryStr(self.sql % self.params_type(self.params))
 
     def _execute_query(self):
         connection = connections[self.using]
@@ -340 +343 @@
         done by the database interface at execution time.
         """
         sql, params = self.sql_with_params()
-        return sql % params
+        return QueryStr(sql % params)
 
     def sql_with_params(self):
         """

django/db/utils.py

@@ -276 +276 @@
         """Return app models allowed to be migrated on provided db."""
         models = app_config.get_models(include_auto_created=include_auto_created)
         return [model for model in models if self.allow_migrate_model(db, model)]
+
+
+class QueryStr(str):
+    """
+    String representation of a SQL query with interpolated params that is
+    unsafe to pass to the database in raw form.
+    """
+    def __str__(self):
+        return self

Also, sql_with_params() is not a panacea, it has it's limitation e.g. it always uses the default database connection.

I don't think this is a big issue, the above discussions suggest allowing alias to be passed

django/db/models/sql/query.py

@@ -345 +345 @@
         sql, params = self.sql_with_params()
         return QueryStr(sql % params)
 
-    def sql_with_params(self):
+    def sql_with_params(self, alias=DEFAULT_DB_ALIAS):
         """
         Return the query as an SQL string and the parameters that will be
         substituted into the query.
         """
-        return self.get_compiler(DEFAULT_DB_ALIAS).as_sql()
+        return self.get_compiler(alias).as_sql()
 
     def __deepcopy__(self, memo):
         """Limit the amount of work when a Query is deepcopied."""

With your approval I'll re-open #34636, Alex feel free to pick it up.

[Alex]

Added a draft PR ​https://github.com/django/django/pull/18468 copying the description for posterity and easy access.

No tests done yet. I'll like to know first if the approach I did is valid.

Since the DB connections are per-thread as far as I know, this approach should not have any race condition issues. And since it's done before the prefetch, it should pick the Queryset query. Also, since queries are only logged if DEBUG is enabled, this approach requires that too.

My discarded approach was to to get it somehow in the return of self._result_cache = list(self._iterable_class(self)), or stored in the QuerySet.
However, it's much more complicated, because as far as I see the flow is something like
QuerySet when ​evaluating calls the ​IterableClass which then ​executes the query which then gets the ​connection cursor, which is wrapped in the previous mentioned one that can log the query.

So the a way would be to make all 4 SqlCompiler.execute_sql return the result and the SQL query. Then you have to adapt all 6 Iterable.__iter__ to also return the query. Then you can store it in the Queryset. Since these methods are part of the public API, this would be a breaking change in both methods. Also I'm not 100% sure how to get the query in the wrapper since the Cursor.execute is wrapped in a context manager which is the one logging the query.