INTPYTHON-827 Fix Model.save() updates and QuerySet.update() for $-prefixed strings #445

WaVEV · 2025-11-12T16:35:39Z

Summary

This PR fixes unsafe handling of $-prefixed values during save() and update() operations in the Django MongoDB backend.
Values like {"$concat": [...]} or ("$name", "-", "$name") were previously interpreted as MongoDB operators instead of being stored literally.
The patch ensures that all such values are wrapped with $literal when appropriate, preventing unintended expression or operator injection.

Changes in this PR

Updated value compilation to wrap dict, tuple, string, and numeric values in $literal where needed.
Update SQLUpdateCompiler to wrap any direct value (i.e. string, dict, tuple, etc).

Test Plan

Added unit tests verifying that $-prefixed values are safely stored and not interpreted as expressions.
Tested scenarios for dicts, tuples, and Value() wrappers.
Verified that regular (non-$-prefixed) values are unaffected (basic unit test from Django)

Checklist

Checklist for Author

Did you update the changelog (if necessary)?
Is the intention of the code captured in relevant tests?
If there are new TODOs, has a related JIRA ticket been created?

Checklist for Reviewer @Jibola @aclark4life @timgraham

Does the title of the PR reference a JIRA Ticket?
Do you fully understand the implementation?
Have you checked for spelling & grammar errors?
Is all relevant documentation (README or docstring) updated?

Focus Areas for Reviewer

Pay attention to the logic handling $literal wrapping in value compilation.

timgraham · 2025-11-12T17:23:13Z

Did you forget to git add a new file with the test?

WaVEV · 2025-11-12T17:30:55Z

Did you forget to git add a new file with the test?

Yes

timgraham · 2025-11-12T19:28:17Z

django_mongodb_backend/compiler.py

+            if is_direct_value(value):
+                prepared = {"$literal": prepared}


Is it necessary to wrap all values? Are there problems with other types? Our logic in Value() only wraps list/int/str.

A unit test (test_update_dollar_prefix_in_value_expression) shows the issue. Because we could update passing an expression, and the expression could be Value("$update")

What I meant: Is it necessary to wrap all direct values?

understood, 🤔 . Will try to make a test and try to inject something.

well, 🤔. maybe only string should be wrapped, not any kind of value.

EDIT
it is possible:

def test_update_dict(self): obj = Author.objects.create(name="foobar") obj.name = Value({"$concat": ["$name", "-", "$name"]}) obj.save() obj.refresh_from_db() self.assertEqual(obj.name, {"$concat": ["$name", "-", "$name"]})

this tails fails and the name was update as foobar-foobar
I think we should scape any kind of literal

That seems sufficient to me.

Another case:

def test_update_dictvalue(self): obj = Author.objects.create(name="foobar", data={}) obj.data = {"$concat": ["$name", "-", "$name"]} obj.save() obj.refresh_from_db() self.assertEqual(obj.data, {"$concat": ["$name", "-", "$name"]})

If the field is a JSONField. Any kind of type that could be a MongoDB query, could be exploitable.
I think wrapping al values will bring us a safe net.

timgraham · 2025-11-12T19:28:57Z

django_mongodb_backend/test.py

            expected_pipeline,
        )
+
+    def assertUpdateQuery(self, query, expected_collection, expected_pipeline, new_values):


new_values -> expected_values?
expected_pipeline -> expected_condition?

timgraham · 2025-11-12T19:29:01Z

django_mongodb_backend/test.py

+    def assertUpdateQuery(self, query, expected_collection, expected_pipeline, new_values):
+        """
+        Assert that the logged query is equal to:
+            db.{expected_collection}.aggregate({expected_pipeline})


update_many

timgraham · 2025-11-12T19:30:21Z

tests/queries_/tests.py

+class SaveUpdateDollarPrefixTests(MongoTestCaseMixin, TestCase):
+    def test_insert_dollar_prefix(self):
+        """Ensure $-prefixed values are correctly saved on insert."""
+        obj = Author.objects.create(name="$foobar")
+        refreshed = Author.objects.get(pk=obj.pk)
+        self.assertEqual(refreshed.name, "$foobar")
+


Not a regression test so should be a separate commit.

🤔 I though I need to test also the save process, but the insertion isn't a problem. So could we remove this test? or it is a nice to have?

Nothing wrong with more tests, just saying that additional test coverage should be added in a separate commit so it's easy to identify what a commit does and doesn't fix.

timgraham · 2025-11-12T19:31:18Z

tests/basic_/tests.py

+    def test_update_dollar_prefix(self):
+        """Ensure $-prefixed values are correctly saved on update."""
+        obj = Author.objects.create(name="foobar")
+        with self.assertNumQueries(1) as ctx:
+            obj.name = "$updated"
+            obj.save()
+        refreshed = Author.objects.get(pk=obj.pk)
+        self.assertEqual(refreshed.name, "$updated")
+        self.assertUpdateQuery(
+            ctx.captured_queries[0]["sql"],
+            "queries__author",
+            {"_id": obj.id},
+            [{"$set": {"name": {"$literal": "$updated"}}}],
+        )


Per https://docs.djangoproject.com/en/dev/internals/contributing/writing-code/coding-style/#python-style, "In test docstrings, state the expected behavior that each test demonstrates. Don’t include preambles such as “Tests that” or “Ensures that”."

timgraham · 2025-11-12T19:33:03Z

django_mongodb_backend/expressions/builtins.py

+    if isinstance(value, (list, int, str)) and as_expr:
+        # Wrap lists, numbers, & string in $literal to prevent ambiguity when Value
+        # appears in $project or $update.


The fix for Value could be a separate commit. Also, add a test in expressions_/test_value.py.

Ok, will separate the commits.

timgraham · 2025-11-12T19:34:26Z

django_mongodb_backend/expressions/builtins.py

+    if isinstance(value, (list, int, str)) and as_expr:
+        # Wrap lists, numbers, & string in $literal to prevent ambiguity when Value
+        # appears in $project or $update.


I know what you mean, but I think "$update" isn't a thing.

A rushed comment, there is nothing called that in our pipeline. should be rewritten.

All the str should be wrapped.

def test_query_injection(self): obj1 = Author.objects.create(name="Gustavo") obj2 = Author.objects.create(name="Walter") with self.assertNumQueries(1) as ctx: qs = list(Author.objects.annotate(a_value=Value("$name"))) print(ctx.captured_queries[0]["sql"]) print(qs)

the generated mql is:
db.basic__author.aggregate([{'$project': {'a_value': '$name', '_id': 1, 'name': 1}}])
and the results are:

qs[0].a_value == "Gustavo" qs[1].a_value == "Walter"

So the code was injected. the expected value is a constant value "$name"

timgraham · 2025-11-12T19:36:05Z

tests/basic_/tests.py

+from django.db.models import Value
+from django.test import TestCase
+
+from django_mongodb_backend.test import MongoTestCaseMixin
+
+from .models import Author
+
+
+class SaveUpdateDollarPrefixTests(MongoTestCaseMixin, TestCase):


You can add tests/basic_/tests.py for basic model operations tests. So I see three commits: add insert test, fix update, fix update with Value()

timgraham · 2025-11-12T19:37:38Z

tests/queries_/tests.py

+    def test_insert_dollar_prefix(self):
+        """Ensure $-prefixed values are correctly saved on insert."""
+        obj = Author.objects.create(name="$foobar")
+        refreshed = Author.objects.get(pk=obj.pk)


obj.refresh_from_db()

timgraham · 2025-11-12T19:38:08Z

tests/queries_/tests.py

+        """Ensure $-prefixed values are correctly saved on update."""
+        obj = Author.objects.create(name="foobar")
+        with self.assertNumQueries(1) as ctx:
+            obj.name = "$updated"


the assignment can be outside the with.

…tation

WaVEV requested review from Jibola, aclark4life and timgraham November 12, 2025 16:35

WaVEV marked this pull request as draft November 12, 2025 16:35

timgraham reviewed Nov 12, 2025

View reviewed changes

timgraham changed the title ~~Fix handling of $-prefixed string values during save~~ INTPYTHON-827 Fix Model.save() updates and QuerySet.update() for $-prefixed strings Nov 12, 2025

WaVEV force-pushed the INTPYTHON-827-Cannot-save-CharField-and-TextField-with-values-starting-with-$ branch from 6c2af4c to 68e4632 Compare November 12, 2025 21:57

WaVEV added 3 commits November 12, 2025 21:40

Add SaveDollarPrefixTests test case

30cf189

Fix: wrap string values in $literal to prevent expression misinterpre…

f41a94b

…tation

Fix Model.save() updates and QuerySet.update() for $-prefixed strings

c1b41ac

WaVEV force-pushed the INTPYTHON-827-Cannot-save-CharField-and-TextField-with-values-starting-with-$ branch from 68e4632 to c1b41ac Compare November 13, 2025 01:16

Edits.

51c8912

INTPYTHON-827 Fix Model.save() updates and QuerySet.update() for $-prefixed strings #445

Are you sure you want to change the base?

INTPYTHON-827 Fix Model.save() updates and QuerySet.update() for $-prefixed strings #445

Uh oh!

Conversation

WaVEV commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes in this PR

Test Plan

Checklist

Checklist for Author

Checklist for Reviewer @Jibola @aclark4life @timgraham

Focus Areas for Reviewer

Uh oh!

timgraham commented Nov 12, 2025

Uh oh!

WaVEV commented Nov 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timgraham Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaVEV Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaVEV Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WaVEV Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

WaVEV commented Nov 12, 2025 •

edited

Loading

timgraham Nov 12, 2025 •

edited

Loading

WaVEV Nov 13, 2025 •

edited

Loading

WaVEV Nov 12, 2025 •

edited

Loading

WaVEV Nov 13, 2025 •

edited

Loading