Context Navigation

← Previous Ticket
Next Ticket →

#23329 closed Bug (fixed)

Regression in security patch for querystring manipulation in admin

Reported by:	Markus Holtermann	Owned by:	Simon Charette
Component:	contrib.admin	Version:	1.5
Severity:	Release blocker	Keywords:
Cc:	Simon Charette, Markus Holtermann, cmawebsite@…	Triage Stage:	Accepted
Has patch:	yes	Needs documentation:	no
Needs tests:	no	Patch needs improvement:	no
Easy pickings:	no	UI/UX:	no

Description (last modified by Markus Holtermann)

At least on 1.5.9 the following modified Test failed:

Explanation: the model "Recommendation" inherits from "Title". "Recommendation" has a ModelAdmin registerd, "Title" does not. Due to the restrictiveness of the new to_field_allowed function, one cannot open the popup for "Recommendation" anymore.

tests/regressiontests/admin_views/tests.py

diff --git a/tests/regressiontests/admin_views/tests.py b/tests/regressiontests/admin_views/tests.py
index e7efca2..08f90d8 100644

                class AdminViewBasicTest(TestCase):
         with self.assertRaises(DisallowedModelAdminToField):
             response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'name'})
+        # Specifying a field that is not refered by any other model directly registered
+        # to this admin site but registered through inheritance
+        response = self.client.get("/test_admin/admin/admin_views/recommendation/", {TO_FIELD_VAR: 'id'})
+        self.assertEqual(response.status_code, 200)
         # Specifying a field referenced by another model should be allowed.
         response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'id'})
         self.assertEqual(response.status_code, 200)

Change History (23)

comment:1 by Markus Holtermann, 10 years ago

Description:	modified (diff)

comment:2 by Simon Charette, 10 years ago

Shouldn't TO_FIELD_VAR be 'title_ptr_id' in this case? Which should be the default to_field if you have ForeignKey pointing to Recommendation.

comment:3 by Simon Charette, 10 years ago

Cc:	Simon Charette added

comment:4 by Markus Holtermann, 10 years ago

Ok, here's a more detailed description:

# models.py
class Purchase(models.Model):
    date_added = models.DateTimeField(_('Date (added)'), blank=False, default=now)


class Ticket(models.Model):
    purchase = models.ForeignKey(Purchase)


class VenueTicket(Ticket):
    name = models.CharField(_('Name'), max_length=250, blank=True)

# admin.py
class PurchaseAdmin(admin.ModelAdmin):
    list_display = ('id', 'date_added', )

admin.site.register(Purchase, PurchaseAdmin)


class VenueTicketAdmin(admin.ModelAdmin):
    list_display = ('id', 'purchase', 'name', )
    raw_id_fields = ('purchase', )

admin.site.register(VenueTicket, VenueTicketAdmin)

If one clicks on the magnifier next tho the purchase field in the VenueTicketAdmin /admin/attendees/purchase/?t=id&pop=1 is being opened. But since there is no model that references the purchase which is also registered with a ModelAdmin, the check in options.py fails.

This works for me (original code: https://github.com/django/django/commit/2a446c896e7c814661fb9c4f212b071b2a7fa446#diff-3c42de3e53aba87b32c494f995a728df):

    def to_field_allowed(self, request, to_field):
        opts = self.model._meta

        try:
            field = opts.get_field(to_field)
        except FieldDoesNotExist:
            return False

        # Make sure at least one of the models registered for this site
        # references this field.
        registered_models = self.admin_site._registry
        for related_object in opts.get_all_related_objects():
            if ((related_object.model in registered_models or any(issubclass(c, related_object.model) for c in registered_models)) an
                    field == related_object.field.rel.get_related_field()):
                return True

        return False

comment:5 by Markus Holtermann, 10 years ago

Cc:	Markus Holtermann added

comment:6 by Simon Charette, 10 years ago

Owner:	changed from nobody to Simon Charette
Severity:	Normal → Release blocker
Status:	new → assigned
Triage Stage:	Unreviewed → Accepted

Thanks for the detailed report, this is definitely a regression. I'll put together a PR with the suggested changes.

comment:7 by Simon Charette, 10 years ago

Has patch:	set

Here's a PR against the master branch.

comment:8 by Collin Anderson, 10 years ago

Cc:	cmawebsite@… added

comment:9 by Tim Graham, 10 years ago

Needs documentation:	set

Needs release notes, otherwise looks good to me.

comment:10 by Reiner Gerecke, 10 years ago

I have a somewhat related problem, only in my case, I'm using a through model that is not registered explicitly in the admin. You can find a minimal example here: https://gist.github.com/squiddy/2913590c6867e302b548

comment:11 by Ross McFarland, 10 years ago

running in to some variant of this problem. i don't have a solution, but i do have a simple work-around.

i just went to the admin for the target model and overrode to_field_allowed to explicitly allow the problem field. in my case that looks like the following:

    def to_field_allowed(self, request, to_field):
        return to_field == 'item_ptr' or \
            super(BioAdmin, self).to_field_allowed(request, to_field)

where i have Bio which inherits from Item.