From e180cbe24acab321964be7bb8156770cc3cfafa6 Mon Sep 17 00:00:00 2001 From: Emanuel Lupi Date: Wed, 12 Nov 2025 21:40:25 -0300 Subject: [PATCH 1/3] Add test for inserting a dollar prefixed string This paves the way for fixing a problem with update in INTPYTHON-827. --- django_mongodb_backend/test.py | 17 +++++++++++++---- tests/basic_/__init__.py | 0 tests/basic_/models.py | 8 ++++++++ tests/basic_/test_escaping.py | 20 ++++++++++++++++++++ 4 files changed, 41 insertions(+), 4 deletions(-) create mode 100644 tests/basic_/__init__.py create mode 100644 tests/basic_/models.py create mode 100644 tests/basic_/test_escaping.py diff --git a/django_mongodb_backend/test.py b/django_mongodb_backend/test.py index ee35b4e21..1460f239a 100644 --- a/django_mongodb_backend/test.py +++ b/django_mongodb_backend/test.py @@ -5,6 +5,7 @@ class MongoTestCaseMixin: maxDiff = None + query_types = {"SON": SON, "ObjectId": ObjectId, "Decimal128": Decimal128} def assertAggregateQuery(self, query, expected_collection, expected_pipeline): """ @@ -15,7 +16,15 @@ def assertAggregateQuery(self, query, expected_collection, expected_pipeline): _, collection, operator = prefix.split(".") self.assertEqual(operator, "aggregate") self.assertEqual(collection, expected_collection) - self.assertEqual( - eval(pipeline[:-1], {"SON": SON, "ObjectId": ObjectId, "Decimal128": Decimal128}, {}), # noqa: S307 - expected_pipeline, - ) + self.assertEqual(eval(pipeline[:-1], self.query_types, {}), expected_pipeline) # noqa: S307 + + def assertInsertQuery(self, query, expected_collection, expected_documents): + """ + Assert that the logged query is equal to: + db.{expected_collection}.insert_many({expected_documents}) + """ + prefix, pipeline = query.split("(", 1) + _, collection, operator = prefix.split(".") + self.assertEqual(operator, "insert_many") + self.assertEqual(collection, expected_collection) + self.assertEqual(eval(pipeline[:-1], self.query_types), expected_documents) # noqa: S307 diff --git a/tests/basic_/__init__.py b/tests/basic_/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/basic_/models.py b/tests/basic_/models.py new file mode 100644 index 000000000..faa68ce77 --- /dev/null +++ b/tests/basic_/models.py @@ -0,0 +1,8 @@ +from django.db import models + + +class Author(models.Model): + name = models.CharField(max_length=50) + + def __str__(self): + return self.name diff --git a/tests/basic_/test_escaping.py b/tests/basic_/test_escaping.py new file mode 100644 index 000000000..58d356be7 --- /dev/null +++ b/tests/basic_/test_escaping.py @@ -0,0 +1,20 @@ +"""Literals that MongoDB intreprets as expressions are escaped.""" + +from django.test import TestCase + +from django_mongodb_backend.test import MongoTestCaseMixin + +from .models import Author + + +class ModelCreationTests(MongoTestCaseMixin, TestCase): + def test_dollar_prefixed_string(self): + # No escaping is needed because MongoDB's insert doesn't treat + # dollar-prefixed strings as expressions. + with self.assertNumQueries(1) as ctx: + obj = Author.objects.create(name="$foobar") + obj.refresh_from_db() + self.assertEqual(obj.name, "$foobar") + self.assertInsertQuery( + ctx.captured_queries[0]["sql"], "basic__author", [{"name": "$foobar"}] + ) From 88f26310bd6a762db1192668b51bd288a74f976b Mon Sep 17 00:00:00 2001 From: Emanuel Lupi Date: Wed, 12 Nov 2025 22:13:32 -0300 Subject: [PATCH 2/3] INTPYTHON-827 Prevent Value strings from being interpreted as expressions --- django_mongodb_backend/expressions/builtins.py | 6 +++--- docs/releases/5.2.x.rst | 2 ++ tests/basic_/test_escaping.py | 17 +++++++++++++++++ tests/expressions_/test_value.py | 3 +++ .../model_fields_/test_embedded_model_array.py | 14 ++++++++++++-- 5 files changed, 37 insertions(+), 5 deletions(-) diff --git a/django_mongodb_backend/expressions/builtins.py b/django_mongodb_backend/expressions/builtins.py index 60400d940..85eab2b23 100644 --- a/django_mongodb_backend/expressions/builtins.py +++ b/django_mongodb_backend/expressions/builtins.py @@ -211,9 +211,9 @@ def when(self, compiler, connection): def value(self, compiler, connection, as_expr=False): # noqa: ARG001 value = self.value - if isinstance(value, (list, int)) and as_expr: - # Wrap lists & numbers in $literal to prevent ambiguity when Value - # appears in $project. + if isinstance(value, (list, int, str)) and as_expr: + # Wrap lists, numbers, and strings in $literal to avoid ambiguity when + # Value is used in aggregate() or update_many()'s $set. return {"$literal": value} if isinstance(value, Decimal): return Decimal128(value) diff --git a/docs/releases/5.2.x.rst b/docs/releases/5.2.x.rst index 4c20fe911..9f48a5032 100644 --- a/docs/releases/5.2.x.rst +++ b/docs/releases/5.2.x.rst @@ -17,6 +17,8 @@ Bug fixes - Prevented ``QuerySet.union()`` queries from duplicating the ``$project`` pipeline. +- Made :class:`~django.db.models.Value` wrap strings in ``$literal`` to + prevent dollar-prefixed strings from being interpreted as expressions. Performance improvements ------------------------ diff --git a/tests/basic_/test_escaping.py b/tests/basic_/test_escaping.py index 58d356be7..6ac4e8752 100644 --- a/tests/basic_/test_escaping.py +++ b/tests/basic_/test_escaping.py @@ -1,5 +1,8 @@ """Literals that MongoDB intreprets as expressions are escaped.""" +from operator import attrgetter + +from django.db.models import Value from django.test import TestCase from django_mongodb_backend.test import MongoTestCaseMixin @@ -18,3 +21,17 @@ def test_dollar_prefixed_string(self): self.assertInsertQuery( ctx.captured_queries[0]["sql"], "basic__author", [{"name": "$foobar"}] ) + + +class AnnotationTests(MongoTestCaseMixin, TestCase): + def test_dollar_prefixed_value(self): + """Value() escapes dollar prefixed strings.""" + Author.objects.create(name="Gustavo") + with self.assertNumQueries(1) as ctx: + qs = list(Author.objects.annotate(a_value=Value("$name"))) + self.assertQuerySetEqual(qs, ["$name"], attrgetter("a_value")) + self.assertAggregateQuery( + ctx.captured_queries[0]["sql"], + "basic__author", + [{"$project": {"a_value": {"$literal": "$name"}, "_id": 1, "name": 1}}], + ) diff --git a/tests/expressions_/test_value.py b/tests/expressions_/test_value.py index 4adbc1d3e..a27e2a68e 100644 --- a/tests/expressions_/test_value.py +++ b/tests/expressions_/test_value.py @@ -41,6 +41,9 @@ def test_int(self): def test_str(self): self.assertEqual(Value("foo").as_mql(None, None), "foo") + def test_str_expr(self): + self.assertEqual(Value("$foo").as_mql(None, None, as_expr=True), {"$literal": "$foo"}) + def test_uuid(self): value = uuid.UUID(int=1) self.assertEqual(Value(value).as_mql(None, None), "00000000000000000000000000000001") diff --git a/tests/model_fields_/test_embedded_model_array.py b/tests/model_fields_/test_embedded_model_array.py index 7b3d72ec8..8453f6379 100644 --- a/tests/model_fields_/test_embedded_model_array.py +++ b/tests/model_fields_/test_embedded_model_array.py @@ -327,8 +327,18 @@ def test_nested_array_index_expr(self): }, { "$concat": [ - {"$ifNull": ["Z", ""]}, - {"$ifNull": ["acarias", ""]}, + { + "$ifNull": [ + {"$literal": "Z"}, + {"$literal": ""}, + ] + }, + { + "$ifNull": [ + {"$literal": "acarias"}, + {"$literal": ""}, + ] + }, ] }, ] From 215d3bec9777ea5e5c4ad5c39ce6aa47b2b7c08a Mon Sep 17 00:00:00 2001 From: Emanuel Lupi Date: Wed, 12 Nov 2025 22:13:32 -0300 Subject: [PATCH 3/3] INTPYTHON-827 Prevent model update values from being interpreted as expressions --- django_mongodb_backend/compiler.py | 4 +- .../expressions/builtins.py | 6 +- django_mongodb_backend/test.py | 13 ++++ docs/releases/5.2.x.rst | 3 + tests/basic_/models.py | 8 +++ tests/basic_/test_escaping.py | 65 ++++++++++++++++++- tests/expressions_/test_value.py | 10 +++ 7 files changed, 104 insertions(+), 5 deletions(-) diff --git a/django_mongodb_backend/compiler.py b/django_mongodb_backend/compiler.py index 888362c6b..cb867221e 100644 --- a/django_mongodb_backend/compiler.py +++ b/django_mongodb_backend/compiler.py @@ -883,7 +883,9 @@ def execute_sql(self, result_type): f"{field.__class__.__name__}." ) prepared = field.get_db_prep_save(value, connection=self.connection) - if hasattr(value, "as_mql"): + if is_direct_value(value): + prepared = {"$literal": prepared} + else: prepared = prepared.as_mql(self, self.connection, as_expr=True) values[field.column] = prepared try: diff --git a/django_mongodb_backend/expressions/builtins.py b/django_mongodb_backend/expressions/builtins.py index 85eab2b23..c96c35595 100644 --- a/django_mongodb_backend/expressions/builtins.py +++ b/django_mongodb_backend/expressions/builtins.py @@ -211,9 +211,9 @@ def when(self, compiler, connection): def value(self, compiler, connection, as_expr=False): # noqa: ARG001 value = self.value - if isinstance(value, (list, int, str)) and as_expr: - # Wrap lists, numbers, and strings in $literal to avoid ambiguity when - # Value is used in aggregate() or update_many()'s $set. + if isinstance(value, (list, int, str, dict, tuple)) and as_expr: + # Wrap lists, numbers, strings, dicts, and tuples in $literal to avoid + # ambiguity when Value is used in aggregate() or update_many()'s $set. return {"$literal": value} if isinstance(value, Decimal): return Decimal128(value) diff --git a/django_mongodb_backend/test.py b/django_mongodb_backend/test.py index 1460f239a..88d6034d1 100644 --- a/django_mongodb_backend/test.py +++ b/django_mongodb_backend/test.py @@ -28,3 +28,16 @@ def assertInsertQuery(self, query, expected_collection, expected_documents): self.assertEqual(operator, "insert_many") self.assertEqual(collection, expected_collection) self.assertEqual(eval(pipeline[:-1], self.query_types), expected_documents) # noqa: S307 + + def assertUpdateQuery(self, query, expected_collection, expected_condition, expected_set): + """ + Assert that the logged query is equal to: + db.{expected_collection}.update_many({expected_condition}, {expected_set}) + """ + prefix, pipeline = query.split("(", 1) + _, collection, operator = prefix.split(".") + self.assertEqual(operator, "update_many") + self.assertEqual(collection, expected_collection) + condition, set_expression = eval(pipeline[:-1], self.query_types, {}) # noqa: S307 + self.assertEqual(condition, expected_condition) + self.assertEqual(set_expression, expected_set) diff --git a/docs/releases/5.2.x.rst b/docs/releases/5.2.x.rst index 9f48a5032..15e436a7d 100644 --- a/docs/releases/5.2.x.rst +++ b/docs/releases/5.2.x.rst @@ -19,6 +19,9 @@ Bug fixes pipeline. - Made :class:`~django.db.models.Value` wrap strings in ``$literal`` to prevent dollar-prefixed strings from being interpreted as expressions. + Also wrapped dictionaries and tuples to prevent the same for them. +- Made model update queries wrap values in ``$literal`` to prevent values from + being interpreted as expressions. Performance improvements ------------------------ diff --git a/tests/basic_/models.py b/tests/basic_/models.py index faa68ce77..a8bb652c8 100644 --- a/tests/basic_/models.py +++ b/tests/basic_/models.py @@ -6,3 +6,11 @@ class Author(models.Model): def __str__(self): return self.name + + +class Blob(models.Model): + name = models.CharField(max_length=10) + data = models.JSONField(null=True) + + def __str__(self): + return self.name diff --git a/tests/basic_/test_escaping.py b/tests/basic_/test_escaping.py index 6ac4e8752..491047637 100644 --- a/tests/basic_/test_escaping.py +++ b/tests/basic_/test_escaping.py @@ -7,7 +7,7 @@ from django_mongodb_backend.test import MongoTestCaseMixin -from .models import Author +from .models import Author, Blob class ModelCreationTests(MongoTestCaseMixin, TestCase): @@ -23,6 +23,69 @@ def test_dollar_prefixed_string(self): ) +class ModelUpdateTests(MongoTestCaseMixin, TestCase): + """ + $-prefixed strings and dict/tuples that could be interpreted as expressions + are escaped in the queries that update model instances. + """ + + def test_dollar_prefixed_string(self): + obj = Author.objects.create(name="foobar") + obj.name = "$updated" + with self.assertNumQueries(1) as ctx: + obj.save() + obj.refresh_from_db() + self.assertEqual(obj.name, "$updated") + self.assertUpdateQuery( + ctx.captured_queries[0]["sql"], + "basic__author", + {"_id": obj.id}, + [{"$set": {"name": {"$literal": "$updated"}}}], + ) + + def test_dollar_prefixed_value(self): + obj = Author.objects.create(name="foobar") + obj.name = Value("$updated") + with self.assertNumQueries(1) as ctx: + obj.save() + obj.refresh_from_db() + self.assertEqual(obj.name, "$updated") + self.assertUpdateQuery( + ctx.captured_queries[0]["sql"], + "basic__author", + {"_id": obj.id}, + [{"$set": {"name": {"$literal": "$updated"}}}], + ) + + def test_dict(self): + obj = Blob.objects.create(name="foobar") + obj.data = {"$concat": ["$name", "-", "$name"]} + obj.save() + obj.refresh_from_db() + self.assertEqual(obj.data, {"$concat": ["$name", "-", "$name"]}) + + def test_dict_value(self): + obj = Blob.objects.create(name="foobar", data={}) + obj.data = Value({"$concat": ["$name", "-", "$name"]}) + obj.save() + obj.refresh_from_db() + self.assertEqual(obj.data, {"$concat": ["$name", "-", "$name"]}) + + def test_tuple(self): + obj = Blob.objects.create(name="foobar") + obj.data = ("$name", "-", "$name") + obj.save() + obj.refresh_from_db() + self.assertEqual(obj.data, ["$name", "-", "$name"]) + + def test_tuple_value(self): + obj = Blob.objects.create(name="foobar") + obj.data = Value(("$name", "-", "$name")) + obj.save() + obj.refresh_from_db() + self.assertEqual(obj.data, ["$name", "-", "$name"]) + + class AnnotationTests(MongoTestCaseMixin, TestCase): def test_dollar_prefixed_value(self): """Value() escapes dollar prefixed strings.""" diff --git a/tests/expressions_/test_value.py b/tests/expressions_/test_value.py index a27e2a68e..634ffbc4f 100644 --- a/tests/expressions_/test_value.py +++ b/tests/expressions_/test_value.py @@ -23,6 +23,11 @@ def test_datetime(self): def test_decimal(self): self.assertEqual(Value(Decimal("1.0")).as_mql(None, None), Decimal128("1.0")) + def test_dict_expr(self): + self.assertEqual( + Value({"$foo": "$bar"}).as_mql(None, None, as_expr=True), {"$literal": {"$foo": "$bar"}} + ) + def test_list(self): self.assertEqual(Value([1, 2]).as_mql(None, None, as_expr=True), {"$literal": [1, 2]}) @@ -44,6 +49,11 @@ def test_str(self): def test_str_expr(self): self.assertEqual(Value("$foo").as_mql(None, None, as_expr=True), {"$literal": "$foo"}) + def test_tuple_expr(self): + self.assertEqual( + Value(("$foo", "$bar")).as_mql(None, None, as_expr=True), {"$literal": ("$foo", "$bar")} + ) + def test_uuid(self): value = uuid.UUID(int=1) self.assertEqual(Value(value).as_mql(None, None), "00000000000000000000000000000001")