Document why RawSQL requires parameters

[David Wolever]

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

[petedmarsh]

PR: ​https://github.com/django/django/pull/6916/files

[Simon Charette]

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 to possible injections.

[Tim Graham]

Sounds reasonable to me. If we decide to keep the status quo, let's update the documentation with that rationale.

[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!

[Tim Graham <timograham@…>]

In 7bf3ba0d:

Fixed #26899 -- Documented why RawSQL params is a required parameter.

[Tim Graham <timograham@…>]

In 8f7008c4:

[1.9.x] Fixed #26899 -- Documented why RawSQL params is a required parameter.

Backport of 7bf3ba0d0c700670d13d7683eec7bd3eb3d4dd1f from master

[Tim Graham <timograham@…>]

In 5cc6190:

[1.10.x] Fixed #26899 -- Documented why RawSQL params is a required parameter.

Backport of 7bf3ba0d0c700670d13d7683eec7bd3eb3d4dd1f from master

[David 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(…).

[Josh Smeaton]

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.

[David 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."
        )

[Josh Smeaton]

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.