[Django] #36152: Postgres backend could quote `%` in column aliases instead of failing at the db level

[Django]

#36152: Postgres backend could quote `%` in column aliases instead of failing at
the db level
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: 5.1 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
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:
{{{#!diff
diff --git a/tests/annotations/tests.py b/tests/annotations/tests.py
index 29660a827e..6bc9bc9ed4 100644
--- a/tests/annotations/tests.py
+++ b/tests/annotations/tests.py
@@ -1171,6 +1171,9 @@ class NonAggregateAnnotationTestCase(TestCase):
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.

The Oracle backend
[https://github.com/django/django/blob/efec74b90868c2e611f863bf4301d92ce08067e8/django/db/backends/oracle/operations.py#L377
does this for you].
--
Ticket URL: <https://code.djangoproject.com/ticket/36152>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36152: Postgres backend could quote `%` in column aliases instead of failing at
the db level
-------------------------------------+-------------------------------------

Reporter: Jacob Walls | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Description changed by Jacob Walls:

Old description:

New description:

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
[https://github.com/django/django/blob/efec74b90868c2e611f863bf4301d92ce08067e8/django/db/backends/oracle/operations.py#L377
does this for you].

--

--
Ticket URL: <https://code.djangoproject.com/ticket/36152#comment:1>

[Django]

#36152: Postgres backend could quote `%` in column aliases instead of failing at
the db level
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 5.1
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Simon Charette):

If we are going to do that we'd need to be particularly careful here as
annotations have been [https://www.djangoproject.com/weblog/2022/apr/11
/security-releases/ 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 in on the
basis of augmenting `FORBIDDEN_ALIAS_PATTERN`.
--
Ticket URL: <https://code.djangoproject.com/ticket/36152#comment:2>