From 732198ed5c2a127249970e0cd4218d093a38e9a2 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Thu, 23 Dec 2010 03:44:38 +0000 Subject: [PATCH] Fix a security issue in the admin. Disclosure and new release forthcoming. git-svn-id: http://code.djangoproject.com/svn/django/trunk@15031 bcc190cf-cafb-0310-a4f2-bffc1f526a37 --- django/contrib/admin/options.py | 41 ++++++++++++++++++++- django/contrib/admin/views/main.py | 8 +++- tests/regressiontests/admin_views/models.py | 7 +++- tests/regressiontests/admin_views/tests.py | 10 +++++ 4 files changed, 62 insertions(+), 4 deletions(-) diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index 5a35e4fbf5..0351ef65c1 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -11,7 +11,9 @@ from django.views.decorators.csrf import csrf_protect from django.core.exceptions import PermissionDenied, ValidationError from django.core.paginator import Paginator from django.db import models, transaction, router -from django.db.models.fields import BLANK_CHOICE_DASH +from django.db.models.related import RelatedObject +from django.db.models.fields import BLANK_CHOICE_DASH, FieldDoesNotExist +from django.db.models.sql.constants import LOOKUP_SEP, QUERY_TERMS from django.http import Http404, HttpResponse, HttpResponseRedirect from django.shortcuts import get_object_or_404, render_to_response from django.utils.decorators import method_decorator @@ -203,6 +205,43 @@ class BaseModelAdmin(object): qs = qs.order_by(*ordering) return qs + def lookup_allowed(self, lookup): + parts = lookup.split(LOOKUP_SEP) + + # Last term in lookup is a query term (__exact, __startswith etc) + # This term can be ignored. + if len(parts) > 1 and parts[-1] in QUERY_TERMS: + parts.pop() + + # Special case -- foo__id__exact and foo__id queries are implied + # if foo has been specificially included in the lookup list; so + # drop __id if it is the last part. However, first we need to find + # the pk attribute name. + model = self.model + pk_attr_name = None + for part in parts[:-1]: + field, _, _, _ = model._meta.get_field_by_name(part) + if hasattr(field, 'rel'): + model = field.rel.to + pk_attr_name = model._meta.pk.name + elif isinstance(field, RelatedObject): + model = field.model + pk_attr_name = model._meta.pk.name + else: + pk_attr_name = None + if pk_attr_name and len(parts) > 1 and parts[-1] == pk_attr_name: + parts.pop() + + try: + self.model._meta.get_field_by_name(parts[0]) + except FieldDoesNotExist: + # Lookups on non-existants fields are ok, since they're ignored + # later. + return True + else: + clean_lookup = LOOKUP_SEP.join(parts) + return clean_lookup in self.list_filter or clean_lookup == self.date_hierarchy + class ModelAdmin(BaseModelAdmin): "Encapsulates all admin options and functionality for a given model." diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 448c6377f3..0b98f11a49 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -1,6 +1,7 @@ from django.contrib.admin.filterspecs import FilterSpec from django.contrib.admin.options import IncorrectLookupParameters from django.contrib.admin.util import quote, get_fields_from_path +from django.core.exceptions import SuspiciousOperation from django.core.paginator import Paginator, InvalidPage from django.db import models from django.utils.encoding import force_unicode, smart_str @@ -188,13 +189,18 @@ class ChangeList(object): else: lookup_params[key] = True + if not self.model_admin.lookup_allowed(key): + raise SuspiciousOperation( + "Filtering by %s not allowed" % key + ) + # Apply lookup parameters from the query string. try: qs = qs.filter(**lookup_params) # Naked except! Because we don't have any other way of validating "params". # They might be invalid if the keyword arguments are incorrect, or if the # values are not in the correct type, so we might get FieldError, ValueError, - # ValicationError, or ? from a custom field that raises yet something else + # ValicationError, or ? from a custom field that raises yet something else # when handed impossible data. except: raise IncorrectLookupParameters diff --git a/tests/regressiontests/admin_views/models.py b/tests/regressiontests/admin_views/models.py index 12b92d80a4..df2f60c024 100644 --- a/tests/regressiontests/admin_views/models.py +++ b/tests/regressiontests/admin_views/models.py @@ -100,7 +100,7 @@ class ChapterXtra1Admin(admin.ModelAdmin): class ArticleAdmin(admin.ModelAdmin): list_display = ('content', 'date', callable_year, 'model_year', 'modeladmin_year') - list_filter = ('date',) + list_filter = ('date', 'section') def changelist_view(self, request): "Test that extra_context works" @@ -611,6 +611,9 @@ class Album(models.Model): owner = models.ForeignKey(User) title = models.CharField(max_length=30) +class AlbumAdmin(admin.ModelAdmin): + list_filter = ['title'] + admin.site.register(Article, ArticleAdmin) admin.site.register(CustomArticle, CustomArticleAdmin) admin.site.register(Section, save_as=True, inlines=[ArticleInline]) @@ -657,4 +660,4 @@ admin.site.register(Promo) admin.site.register(ChapterXtra1, ChapterXtra1Admin) admin.site.register(Pizza, PizzaAdmin) admin.site.register(Topping) -admin.site.register(Album) +admin.site.register(Album, AlbumAdmin) diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py index 0c9f03b933..75c6698db1 100644 --- a/tests/regressiontests/admin_views/tests.py +++ b/tests/regressiontests/admin_views/tests.py @@ -5,6 +5,7 @@ import datetime from django.conf import settings from django.core import mail +from django.core.exceptions import SuspiciousOperation from django.core.files import temp as tempfile from django.core.urlresolvers import reverse # Register auth models with the admin. @@ -348,6 +349,15 @@ class AdminViewBasicTest(TestCase): self.assertContains(response, 'Choisir une heure') deactivate() + def test_disallowed_filtering(self): + self.assertRaises(SuspiciousOperation, + self.client.get, "/test_admin/admin/admin_views/album/?owner__email__startswith=fuzzy" + ) + + try: + self.client.get("/test_admin/admin/admin_views/stuff/?color__value__startswith=red") + except SuspiciousOperation: + self.fail("Filters are allowed if explicitly included in list_filter") class SaveAsTests(TestCase): fixtures = ['admin-views-users.xml','admin-views-person.xml']