#25705: Parameters are not adapted or quoted in Query.__str__
-------------------------------------+-------------------------------------
Reporter: Dmitry Dygalo | Owner: Alex
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by 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
{{{#!diff
diff --git a/django/db/backends/utils.py b/django/db/backends/utils.py
index ab0ea8258b..9874b362b3 100644
--- a/django/db/backends/utils.py
+++ b/django/db/backends/utils.py
@@ -9,6 +9,7 @@
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,6 +95,8 @@ def _execute_with_wrappers(self, sql, params, many,
executor):
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,6 +110,8 @@ def _execute(self, sql, params,
*ignored_wrapper_args):
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()
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index f00eb1e5a5..9e7cc03527 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -18,6 +18,7 @@
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,6 +146,8 @@ class RawQuery:
"""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,8 +194,8 @@ def params_type(self):
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,7 +343,7 @@ def __str__(self):
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):
"""
diff --git a/django/db/utils.py b/django/db/utils.py
index e45f1db249..a5f821cee7 100644
--- a/django/db/utils.py
+++ b/django/db/utils.py
@@ -276,3 +276,12 @@ def get_migratable_models(self, app_config, db,
include_auto_created=False):
"""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
{{{#!diff
diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py
index 9e7cc03527..f952c1bb86 100644
--- a/django/db/models/sql/query.py
+++ b/django/db/models/sql/query.py
@@ -345,12 +345,12 @@ def __str__(self):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/25705#comment:21>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/01070190c6629f16-f7715432-d850-45f5-9822-09631c2811a8-000000%40eu-central-1.amazonses.com.
