Opened 15 months ago
Last modified 11 months ago
#34831 assigned Cleanup/optimization
Search in admin could allow issuing a query with many OR'd clauses
Reported by: | Natalia Bidart | Owned by: | Yves Weissig |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
As a logged in user, when performing searches in the django admin, a request with a lot of terms separated by spaces could cause the server to slow down because each term is OR'd, potentially building a bad query.
The root cause of this issue is a for
loop in /django/contrib/admin/options.py
:
if search_fields and search_term: orm_lookups = [construct_search(str(search_field)) for search_field in search_fields] for bit in smart_split(search_term): if bit.startswith(('"', "'")): bit = unescape_string_literal(bit) or_queries = [models.Q(**{orm_lookup: bit}) for orm_lookup in orm_lookups] queryset = queryset.filter(reduce(operator.or_, or_queries)) use_distinct |= any(lookup_needs_distinct(self.opts, search_spec) for search_spec in orm_lookups)
The value of the string search_term
comes from the frontend input search field and is then split with spaces using the smart_split
.
Thanks Wenchao Li of Alibaba Group for the report.
Change History (5)
comment:1 by , 15 months ago
comment:2 by , 14 months ago
Happy to have a look at this Natalia Bidart.
Is limiting the amount of OR terms the agreed upon solution? Is there a discussion in the forum I'm missing?
comment:3 by , 14 months ago
This issue was only discussed on the private security mailing list before being made into a ticket. I proposed the split limit and others agreed. I would guess the sensible limit is somewhere between 30 and 100 terms. Ideally we’d do some benchmarking before deciding on an exact number.
comment:4 by , 14 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Cool, thanks Adam Johnson, I'll have a look.
comment:5 by , 11 months ago
I did benchmark with MySQL 8.0 and this model:
class Posts(models.Model): title = models.CharField(max_length=100, db_index=True) slug = models.TextField(db_index=True) body = models.TextField(db_index=True) tags = models.CharField(max_length=100, db_index=True) created_at = models.DateTimeField(auto_now_add=True, db_index=True) class PostAdmin(admin.ModelAdmin): search_fields = ["title", "slug", "tags", "body", "created_at"] list_display = ["title", "tags"]
10 terms: 673.32 ms
25 terms: 649.94 ms
50 terms: 615.97 ms
100 terms: 543.81 ms
Notice the query is actually improving in time when increasing the terms. I think this is ecause the query is only OR'd for each different field, for a single field the terms are actually AND'd, which limit the results a lot.
Here's a query with 2 terms (Hello World) :
SELECT ••• FROM `posts_posts` WHERE ((`posts_posts`.`title` LIKE '%Hello%' OR `posts_posts`.`slug` LIKE '%Hello%' OR `posts_posts`.`tags` LIKE '%Hello%' OR `posts_posts`.`body` LIKE '%Hello%' OR `posts_posts`.`created_at` LIKE '%Hello%') AND (`posts_posts`.`title` LIKE '%World%' OR `posts_posts`.`slug` LIKE '%World%' OR `posts_posts`.`tags` LIKE '%World%' OR `posts_posts`.`body` LIKE '%World%' OR `posts_posts`.`created_at` LIKE '%World%')) ORDER BY `posts_posts`.`id` DESC
This was first reported as a security issue but the security team concluded that this can be discussed and fixed in the open, given that the user has to be already authenticated to issue the query (and the admin is considered secured).
But, this issue could happen accidentally from legitimate users, so the suggested fix is to limit the amount of terms that can OR'd in the resulting query.