[Django] #23329: Regression in security patch for querystring manipulation in admin
Django
-------------------------------+--------------------
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
{{{#!diff
diff --git a/tests/regressiontests/admin_views/tests.py
b/tests/regressiontests/admin_views/tests.py
index e7efca2..08f90d8 100644
--- a/tests/regressiontests/admin_views/tests.py
+++ b/tests/regressiontests/admin_views/tests.py
@@ -567,6 +567,11 @@ 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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Markush2010):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:1>
Django
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:2>
Django
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* cc: charettes (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:3>
Django
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Markush2010):
Ok, here's a more detailed description:
{{{#!python
# 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)
}}}
{{{#!python
# 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):
{{{#!python
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:4>
Django
Reporter: Markush2010 | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Markush2010):
* cc: Markush2010 (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:5>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* owner: nobody => charettes
* status: new => assigned
* severity: Normal => Release blocker
* stage: Unreviewed => Accepted
Comment:
Thanks for the detailed report, this is definitely a regression. I'll put
together a PR with the suggested changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:6>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* has_patch: 0 => 1
Comment:
Here's a [https://github.com/django/django/pull/3096 PR] against the
master branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:7>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by collinanderson):
* cc: cmawebsite@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:8>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_docs: 0 => 1
Comment:
Needs release notes, otherwise looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:9>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:10>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:11>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by charettes):
@squiddy I should be able to look into handling the `through` issue
tomorrow, thanks for the minimal example project.
@ross, does [https://github.com/django/django/pull/3096/files this fix]
solves your issue?
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:12>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_docs: 1 => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:13>
Django
Reporter: Markush2010 | Owner: charettes
Component: contrib.admin | Version: 1.5
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette <charette.s@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"3cbb7590cb0ece38f665b516db30cd5a9431f8c8"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:14>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"4883516bea2aebff38b193f4c9707928040d0f8a"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:15>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"e3453b61c6269d7868ceb404abaea5ad2569778f"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:16>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"4c96bd8fb31d2325112ba92ed3cbdc3ff1bbfabc"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:17>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"4685026840f0e2b895f980b6a33ad1b282aa7852"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:18>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"342ccbddc1f2362f867e030befaeb10449cf4539"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:19>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"9c4fb019cb76eb3314357a18e225a63e113dc1fd"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:20>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"a7af6ad96a35634383c2d73fa049127e85a886a6"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:21>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"d9d4d62d8539fc3b72c979c04d11e160bc8fff9d"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:22>
Django
Reporter: Markush2010 | Owner: charettes
Type: Bug | Status: closed
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"065caafa70b6c422f73e364a4c241b6538969d7b"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23329#comment:23>