Opened 12 months ago

Closed 11 months ago

Last modified 11 months ago

#23329 closed Bug (fixed)

Regression in security patch for querystring manipulation in admin

Reported by: Markush2010 Owned by: charettes
Component: contrib.admin Version: 1.5
Severity: Release blocker Keywords:
Cc: charettes, Markush2010, 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 Markush2010)

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
    a b class AdminViewBasicTest(TestCase): 
    567567        with self.assertRaises(DisallowedModelAdminToField):
    568568            response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'name'})
    569569
     570        # Specifying a field that is not refered by any other model directly registered
     571        # to this admin site but registered through inheritance
     572        response = self.client.get("/test_admin/admin/admin_views/recommendation/", {TO_FIELD_VAR: 'id'})
     573        self.assertEqual(response.status_code, 200)
     574
    570575        # Specifying a field referenced by another model should be allowed.
    571576        response = self.client.get("/test_admin/admin/admin_views/section/", {TO_FIELD_VAR: 'id'})
    572577        self.assertEqual(response.status_code, 200)

Change History (23)

comment:1 Changed 12 months ago by Markush2010

  • Description modified (diff)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 12 months ago by charettes

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 Changed 12 months ago by charettes

  • Cc charettes added

comment:4 Changed 12 months ago by Markush2010

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 Changed 12 months ago by Markush2010

  • Cc Markush2010 added

comment:6 Changed 12 months ago by charettes

  • Owner changed from nobody to charettes
  • Severity changed from Normal to Release blocker
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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

comment:7 Changed 12 months ago by charettes

  • Has patch set

Here's a PR against the master branch.

comment:8 Changed 12 months ago by collinanderson

  • Cc cmawebsite@… added

comment:9 Changed 11 months ago by timgraham

  • Needs documentation set

Needs release notes, otherwise looks good to me.

comment:10 Changed 11 months ago by squiddy

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 Changed 11 months ago by ross

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.

comment:12 Changed 11 months ago by charettes

@squiddy I should be able to look into handling the through issue tomorrow, thanks for the minimal example project.

@ross, does this fix solves your issue?

comment:13 Changed 11 months ago by charettes

  • Needs documentation unset

I just pushed an updated patch that correctly allow m2m fields references and release notes.

@squiddy could you make sure the updated patch solves your issue?

comment:14 Changed 11 months ago by Simon Charette <charette.s@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 3cbb7590cb0ece38f665b516db30cd5a9431f8c8:

Fixed #23329 -- Allowed inherited and m2m fields to be referenced in the admin.

Thanks to Trac alias Markush2010 and ross for the detailed reports.

comment:15 Changed 11 months ago by Simon Charette <charette.s@…>

In 4883516bea2aebff38b193f4c9707928040d0f8a:

[1.7.x] Fixed #23329 -- Allowed inherited and m2m fields to be referenced in the admin.

Thanks to Trac alias Markush2010 and ross for the detailed reports.

Backport of 3cbb7590cb from master

comment:16 Changed 11 months ago by Simon Charette <charette.s@…>

In e3453b61c6269d7868ceb404abaea5ad2569778f:

[1.6.x] Fixed #23329 -- Allowed inherited and m2m fields to be referenced in the admin.

Thanks to Trac alias Markush2010 and ross for the detailed reports.

Backport of 3cbb759 from master

comment:17 Changed 11 months ago by Simon Charette <charette.s@…>

In 4c96bd8fb31d2325112ba92ed3cbdc3ff1bbfabc:

Fixed #23329 -- Allowed inherited and m2m fields to be referenced in the admin.

Thanks to Trac alias Markush2010 and ross for the detailed reports.

Backport of 3cbb759 from master

comment:18 Changed 11 months ago by Simon Charette <charette.s@…>

In 4685026840f0e2b895f980b6a33ad1b282aa7852:

[1.4.x] Fixed #23329 -- Allowed inherited and m2m fields to be referenced in the admin.

Thanks to Trac alias Markush2010 and ross for the detailed reports.

Backport of 3cbb759 from master

comment:19 Changed 11 months ago by Simon Charette <charette.s@…>

In 342ccbddc1f2362f867e030befaeb10449cf4539:

Fixed #23431 -- Allowed inline and hidden references to admin fields.

This fixes a regression introduced by the 53ff096982 security fix.

Thanks to @a1tus for the report and Tim for the review.

refs #23329.

comment:20 Changed 11 months ago by Simon Charette <charette.s@…>

In 9c4fb019cb76eb3314357a18e225a63e113dc1fd:

[1.7.x] Fixed #23431 -- Allowed inline and hidden references to admin fields.

This fixes a regression introduced by the 53ff096982 security fix.

Thanks to @a1tus for the report and Tim for the review.

refs #23329.

Backport of 342ccbddc1 from master

comment:21 Changed 11 months ago by Simon Charette <charette.s@…>

In a7af6ad96a35634383c2d73fa049127e85a886a6:

[1.6.x] Fixed #23431 -- Allowed inline and hidden references to admin fields.

This fixes a regression introduced by the 53ff096982 security fix.

Thanks to @a1tus for the report and Tim for the review.

refs #23329.

Backport of 342ccbd from master

comment:22 Changed 11 months ago by Simon Charette <charette.s@…>

In d9d4d62d8539fc3b72c979c04d11e160bc8fff9d:

[1.5.x] Fixed #23431 -- Allowed inline and hidden references to admin fields.

This fixes a regression introduced by the 53ff096982 security fix.

Thanks to @a1tus for the report and Tim for the review.

refs #23329.

Backport of 342ccbd from master

comment:23 Changed 11 months ago by Simon Charette <charette.s@…>

In 065caafa70b6c422f73e364a4c241b6538969d7b:

[1.4.x] Fixed #23431 -- Allowed inline and hidden references to admin fields.

This fixes a regression introduced by the 53ff096982 security fix.

Thanks to @a1tus for the report and Tim for the review.

refs #23329.

Backport of 342ccbd from master

Note: See TracTickets for help on using tickets.
Back to Top