Annotation with `%` in alias fails at db level on Postgres and MySQL

[Jacob Walls]

When developing with postgres, if you are creating dynamic annotations, you have to know to reject % in the provided alias or transform it to %%, otherwise you end up with an unhelpful exception from the db adapter.

test case:

tests/annotations/tests.py

@@ -1171 +1171 @@
                 with self.assertRaisesMessage(ValueError, msg):
                     Book.objects.annotate(**{crafted_alias: Value(1)})
 
+    def test_annotation_contains_percent(self):
+        Book.objects.annotate(**{"contains_percent%": F("pk")}).first()
+
     @skipUnless(connection.vendor == "postgresql", "PostgreSQL tests")
     @skipUnlessDBFeature("supports_json_field")
     def test_set_returning_functions(self):

Gives on psycopg2:

IndexError: tuple index out of range

On psycopg 3, the errors are more helpful, either:

django.db.utils.ProgrammingError: only '%s', '%b', '%t' are allowed as placeholders, got '%"'

Or if the annotation indeed included %s:

django.db.utils.ProgrammingError: the query has 2 placeholders but 1 parameters were passed

But I still think we could easily fail at the ORM level instead of going to the backend and failing there just quote % to %%.

The Oracle backend ​does this for you.

[Simon Charette]

If we are going to do that we'd need to be particularly careful here as annotations have been ​a vector of SQL injection in the past.

I think we should instead include % in FORBIDDEN_ALIAS_PATTERN as by the time comes to escaping the SQL is mixed up with actual placeholders %s and now alias names. I wouldn't be surprised if the fact we allow % today could be exploited one way to leak some parameter that should be present in the WHERE clause for example.

The naive approach of adjusting SQLCompiler.get_select to escape percent signs could cause more harm than good as PEP-249 allows for Cursor.execute to be called with and without params: tuple | None and when None is provided no parameter interpolation takes place so the prior escaping would now assign the wrong alias.

In summary I think we should either wont-fix that one or accept it on the basis of augmenting FORBIDDEN_ALIAS_PATTERN as the complexity and security tradeoff are not worth the UX benefits it would provide (particularly because it has been broken for years already).

[Jacob Walls]

I'd be in favor of adjusting the regex. That's essentially what my code does, and I could remove some workarounds if we get it in core.

I wouldn't be surprised if the fact we allow % today could be exploited one way to leak some parameter that should be present in the WHERE clause for example.

I did try to fiddle with that before deciding whether to go to the security team first. This had been on my medium term tinker list for a while and finally opened an issue today because I saw a similar issue for oracle in ticket:36147 waiting for a reproducer. (I was surprised to see the annotation "just work" on Oracle -- this is how I noticed that backend replaces % for you.)

Do we need to worry about a deprecation path for non-postgres backends?

[Simon Charette]

Do we need to worry about a deprecation path for non-postgres backends?

I assume we would need a path yes, it's also crashing on MySQL though so not only a problem on Postgres

Traceback (most recent call last):
  File "/usr/local/lib/python3.12/site-packages/MySQLdb/cursors.py", line 200, in _mogrify
    query = query % args
            ~~~~~~^~~~~~
TypeError: not enough arguments for format string

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/django/source/django/db/backends/utils.py", line 105, in _execute
    return self.cursor.execute(sql, params)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/django/source/django/db/backends/mysql/base.py", line 76, in execute
    return self.cursor.execute(query, args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/MySQLdb/cursors.py", line 176, in execute
    mogrified_query = self._mogrify(query, args)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/lib/python3.12/site-packages/MySQLdb/cursors.py", line 202, in _mogrify
    raise ProgrammingError(str(m))
MySQLdb.ProgrammingError: not enough arguments for format string

The specialized logic for Oracle originated from 2249bd275cbae6b73716bf36f5f36def1bb4222e which was likely to support introspecting legacy tables columns containing % signs. Tat means we can't realistically deprecate the quote_name path entirely. I haven't looked into why things work on SQLite.

[Natalia Bidart]

I'm inclined to go with the "make FORBIDDEN_ALIAS_PATTERN more restrictive" option, accepting the ticket on that basis.

[Jacob Walls]

​PR

[Mariusz Felisiak]

Is not this a duplicate of #6343?

[Jacob Walls]

I see merit in handling the two issues separately.

#6343 refers to table and column names, which are rarely if ever driven by user input and potentially outside the developer's control for legacy databases (see the end of comment:4). This is a harder problem.

With this ticket's focus on aliases, which could be controlled by a user (or a user of a reusable app, e.g. supplying dynamic annotations), I'm seeing a consensus for deprecating the buggy behavior and a reasonable upgrade path, since there are no database entities to adjust.

[Sarah Boyce <42296566+sarahboyce@…>]

In 8ede411:

Fixed #36152 -- Deprecated use of "%" in column aliases.

Unintentional support existed only on SQLite and Oracle.

[Jacob Walls <jacobtylerwalls@…>]

In 2d453a2a:

Refs #36152 -- Suppressed duplicate warning when using "%" in alias via values().

[Jacob Walls <jacobtylerwalls@…>]

In fd705912:

Refs #36152, #35667 -- Used skip_file_prefixes in alias deprecation warning.

Follow-up to 8ede411a81b40ca53362e6788601193c7e56a0cf.