Code

Opened 7 months ago

Closed 7 months ago

#21056 closed Cleanup/optimization (fixed)

AdminSite app_list may be reverse()'d into an invalid URL endpoint

Reported by: Keryn Knight <django@…> Owned by: Tim Graham <timograham@…>
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Because of the way the regular expression for the app_index view is set, it allows pretty much anything to be reversed to a valid URL, even if that URL will generate a 404 when visited. This is in contrast to any of the views defined in the ModelAdmin's own get_urls, because they are included by way of the app_label for that ModelAdmin's Model class, and are usually resolved using the named url which is a combination of the app label + model name.

>>> from django.core.urlresolvers import reverse
>>> reverse('admin:index') # this is ok!
'/admin/'
>>> reverse('admin:app_list', kwargs={'app_label': 'auth'}) # this is ok!
'/admin/auth/'
>>> reverse('admin:app_list', kwargs={'app_label': 'test_anything_is_allowed'}) # chances are this isn't right.
'/admin/test_anything_is_allowed/'

As the registry already maintains a list of ModelAdmins, it would probably be reasonably simple to iterate over those and get all distinct app labels, and compile one regex that ORs them altogether, reducing the ability to accidentally create invalid links.

Attachments (0)

Change History (7)

comment:1 Changed 7 months ago by DanRJohnson

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to DanRJohnson
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 7 months ago by DanRJohnson

I can confirm this issue. Very straightforward to reproduce this issue.

comment:3 Changed 7 months ago by DanRJohnson

  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 7 months ago by DanRJohnson

  • Owner DanRJohnson deleted
  • Status changed from assigned to new

comment:5 Changed 7 months ago by Keryn Knight <django@…>

As Dan has removed himself, but verified the problem [thanks!], I've whipped up a first pass at fixing it; Pull request is here, tested locally using:

PYTHONPATH=..:$PYTHONPATH python ./runtests.py --settings=test_sqlite

Essentially it saves the app labels until it's mounted the various individual modeladmins, and then compiles a regex of the form: ^(?P<app_label>app1|app2|app3|app4)/$ so that only those which are valid are resolvable. It also stops the app index being registered if there aren't any apps mounted, though I cannot imagine a reasonable scenario in which that is the case.

comment:6 Changed 7 months ago by timo

  • Has patch set

comment:7 Changed 7 months ago by Tim Graham <timograham@…>

  • Owner set to Tim Graham <timograham@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 170f72136758add6c9c0c59240cfce73d5672cc2:

Fixed #21056 -- AdminSite.app_index no longer blindly accepts any app-labelish input.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.