[Django] #26899: RawSQL requires parameters even if they are empty
Django
----------------------------------------------+--------------------
Reporter: wolever | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
----------------------------------------------+--------------------
The `params` argument to `RawSQL` isn't optional so an empty tuple must be
provided if no arguments are needed:
{{{
>>> RawSQL("some_column")
...
TypeError: __init__() takes at least 3 arguments (2 given)
}}}
It seems reasonable to make `params=()` the default: `RawSQL(sql,
params=(), output_field=None)`, or possibly `params=None` and `self.params
= params if params is not None else ()`.
See also:
https://github.com/django/django/blob/master/django/db/models/expressions.py#L607
--
Ticket URL: <https://code.djangoproject.com/ticket/26899>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: wolever | Owner:
| petedmarsh
Type: Uncategorized | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by petedmarsh):
* owner: nobody => petedmarsh
* needs_docs: => 0
* status: new => assigned
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:1>
Django
Reporter: wolever | Owner:
| petedmarsh
Type: Uncategorized | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by petedmarsh):
PR: https://github.com/django/django/pull/6916/files
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:2>
Django
Reporter: wolever | Owner:
| petedmarsh
Type: Uncategorized | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by charettes):
I like to think of `params` being an explicit argument as a feature that
forces developers to think twice about not interpolating user provided
data into their SQL and expose their application into possible injections.
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:3>
Django
Reporter: wolever | Owner:
| petedmarsh
Type: Uncategorized | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by timgraham):
Sounds reasonable to me. If we decide to keep the status quo, let's update
the documentation with that rationale.
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:4>
Django
Reporter: wolever | Owner:
| petedmarsh
Type: Uncategorized | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by petedmarsh):
I've updated the PR to add a note to the documentation as described above,
happy to re-word or bring back the code changes, just let me know!
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: wolever | Owner:
Type: | petedmarsh
Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* component: Database layer (models, ORM) => Documentation
* has_patch: 0 => 1
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: wolever | Owner:
Type: | petedmarsh
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"7bf3ba0d0c700670d13d7683eec7bd3eb3d4dd1f" 7bf3ba0d]:
{{{
#!CommitTicketReference repository=""
revision="7bf3ba0d0c700670d13d7683eec7bd3eb3d4dd1f"
Fixed #26899 -- Documented why RawSQL params is a required parameter.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: wolever | Owner:
Type: | petedmarsh
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"8f7008c48769b2b4340581b6b0f6c41d99c2c1df" 8f7008c4]:
{{{
#!CommitTicketReference repository=""
revision="8f7008c48769b2b4340581b6b0f6c41d99c2c1df"
[1.9.x] Fixed #26899 -- Documented why RawSQL params is a required
parameter.
Backport of 7bf3ba0d0c700670d13d7683eec7bd3eb3d4dd1f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: wolever | Owner:
Type: | petedmarsh
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"5cc619078860c13ae910b26cd2ab5c11508c023f" 5cc6190]:
{{{
#!CommitTicketReference repository=""
revision="5cc619078860c13ae910b26cd2ab5c11508c023f"
[1.10.x] Fixed #26899 -- Documented why RawSQL params is a required
parameter.
Backport of 7bf3ba0d0c700670d13d7683eec7bd3eb3d4dd1f from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: wolever | Owner:
Type: | petedmarsh
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by wolever):
I'm glad the rationale is documented, but I can't help but feel like
keeping this parameter required is both frustrating for folk who aren't
performing injectable queries and more importantly inconsistent with the
other mechanisms Django offers to drop into raw SQL. Specifically,
`QuerySet.raw(…)` doesn't require an empty params, and neither does
`QuerySet.extra(…)`.
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: wolever | Owner:
Type: | petedmarsh
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by jarshwah):
I can understand your frustration. There's certainly an inconsistency
here, which is unfortunate, but I think I'd prefer to make the raw and
extra params a necessary argument if I was writing them from scratch given
the argument made earlier. It's way too easy to slip in a % (format) and
just ignore the params argument altogether. All parameters should be
passed via params, whether they're potentially injectable or not.
I consider the overhead involved in adding an empty params argument a
decent tradeoff to have a reminder of good security practises built into
the API and not just within the docs. RawSQL will probably not be used
very much within projects so it shouldn't lead to any/much API fatigue.
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: wolever | Owner:
Type: | petedmarsh
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by wolever):
That's absolutely fair!
I just want to make sure that the (potentially many?) use cases which
don't involve any parameters are being considered (ex, adding an
`array_agg` to a `groupby`, selecting a column from an intermediary many-
to-many table, a subselect that doesn't require parameters, and I'm sure
you can imagine a bunch more).
Anyway, if this has been considered and parameters are the final decision,
the one last request I would make (and PR I could provide): provide a
default value for the `params` argument which raises an explanatory
exception (it took me 15 minutes to figure out why `RawSQL("some_column")`
was raising a `__init__ takes 3 arguments but 2 were provided` error).
{{{
def __init__(self, query, params=Undefined, …):
if params is Undefined:
raise TypeError(
"params must always be provided to RawSQL to reduce the risk
of SQL injection. "
"Use `params=()` if your query does not require any
parameters."
)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: wolever | Owner:
Type: | petedmarsh
Cleanup/optimization | Status: closed
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by jarshwah):
I think that change is fairly reasonable. If we want to encode good
practises into the API it's only fair that correct usage is guided by it
too. If you put a patch together then just "Refs #26899" to begin the
commit.
--
Ticket URL: <https://code.djangoproject.com/ticket/26899#comment:13>