From ba31b0103442ac891fb3cb98f316781254e366c3 Mon Sep 17 00:00:00 2001 From: Jon Dufresne Date: Tue, 12 Jan 2021 05:37:56 -0800 Subject: [PATCH] Fixed #31747 -- Fixed model enumeration via admin URLs. Co-authored-by: Carlton Gibson --- django/contrib/admin/sites.py | 28 +++- docs/ref/contrib/admin/index.txt | 13 ++ docs/releases/3.2.txt | 14 ++ tests/admin_views/admin.py | 31 ++++- tests/admin_views/tests.py | 211 +++++++++++++++++++++++++++++++ tests/admin_views/urls.py | 10 ++ 6 files changed, 303 insertions(+), 4 deletions(-) diff --git a/django/contrib/admin/sites.py b/django/contrib/admin/sites.py index e200fdeb1e..600944ebc0 100644 --- a/django/contrib/admin/sites.py +++ b/django/contrib/admin/sites.py @@ -3,19 +3,23 @@ from functools import update_wrapper from weakref import WeakSet from django.apps import apps +from django.conf import settings from django.contrib.admin import ModelAdmin, actions from django.contrib.admin.views.autocomplete import AutocompleteJsonView from django.contrib.auth import REDIRECT_FIELD_NAME from django.core.exceptions import ImproperlyConfigured from django.db.models.base import ModelBase -from django.http import Http404, HttpResponseRedirect +from django.http import ( + Http404, HttpResponsePermanentRedirect, HttpResponseRedirect, +) from django.template.response import TemplateResponse -from django.urls import NoReverseMatch, reverse +from django.urls import NoReverseMatch, Resolver404, resolve, reverse from django.utils.functional import LazyObject from django.utils.module_loading import import_string from django.utils.text import capfirst from django.utils.translation import gettext as _, gettext_lazy from django.views.decorators.cache import never_cache +from django.views.decorators.common import no_append_slash from django.views.decorators.csrf import csrf_protect from django.views.i18n import JavaScriptCatalog @@ -63,6 +67,8 @@ class AdminSite: password_change_template = None password_change_done_template = None + final_catch_all_view = True + def __init__(self, name='admin'): self._registry = {} # model_class class -> admin_class instance self.name = name @@ -282,6 +288,10 @@ class AdminSite: urlpatterns += [ re_path(regex, wrap(self.app_index), name='app_list'), ] + + if self.final_catch_all_view: + urlpatterns.append(re_path(r'(?P.*)$', wrap(self.catch_all_view))) + return urlpatterns @property @@ -406,6 +416,20 @@ class AdminSite: def autocomplete_view(self, request): return AutocompleteJsonView.as_view(admin_site=self)(request) + @no_append_slash + def catch_all_view(self, request, url): + if settings.APPEND_SLASH and not url.endswith('/'): + urlconf = getattr(request, 'urlconf', None) + path = '%s/' % request.path_info + try: + match = resolve(path, urlconf) + except Resolver404: + pass + else: + if getattr(match.func, 'should_append_slash', True): + return HttpResponsePermanentRedirect(path) + raise Http404 + def _build_app_dict(self, request, label=None): """ Build the app dictionary. The optional `label` parameter filters models diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt index 87d1b4f3f5..040b2ada85 100644 --- a/docs/ref/contrib/admin/index.txt +++ b/docs/ref/contrib/admin/index.txt @@ -2905,6 +2905,19 @@ Templates can override or extend base admin templates as described in A boolean value that determines whether to show the navigation sidebar on larger screens. By default, it is set to ``True``. +.. attribute:: AdminSite.final_catch_all_view + + .. versionadded:: 3.2 + + A boolean value that determines whether to add a final catch-all view to + the admin that redirects unauthenticated users to the login page. By + default, it is set to ``True``. + + .. warning:: + + Setting this to ``False`` is not recommended as the view protects + against a potential model enumeration privacy issue. + .. attribute:: AdminSite.login_template Path to a custom template that will be used by the admin site login view. diff --git a/docs/releases/3.2.txt b/docs/releases/3.2.txt index f6c5ae5b62..45f42bc214 100644 --- a/docs/releases/3.2.txt +++ b/docs/releases/3.2.txt @@ -125,6 +125,14 @@ Minor features ` when searching a related model. +* The admin now installs a final catch-all view that redirects unauthenticated + users to the login page, regardless or whether the URLs is otherwise valid. + This protects against a potential model enumeration privacy issue. + + Although not recommended, you may set the new + :attr:`.AdminSite.final_catch_all_view` to ``False`` to disable the + catch-all view. + :mod:`django.contrib.admindocs` ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ @@ -600,6 +608,12 @@ backends. * Pagination links in the admin are now 1-indexed instead of 0-indexed, i.e. the query string for the first page is ``?p=1`` instead of ``?p=0``. +* The new admin catch-all view will break URL patterns routed after the admin + URLs and matching the admin URL prefix. You can either adjust your URL + ordering or, if necessary, set :attr:`AdminSite.final_catch_all_view + ` to ``False``, + disabling the catch-all view. See :ref:`whats-new-3.2` for more details. + :mod:`django.contrib.gis` ------------------------- diff --git a/tests/admin_views/admin.py b/tests/admin_views/admin.py index 4a72e3070f..1140f03496 100644 --- a/tests/admin_views/admin.py +++ b/tests/admin_views/admin.py @@ -15,10 +15,11 @@ from django.core.files.storage import FileSystemStorage from django.core.mail import EmailMessage from django.db import models from django.forms.models import BaseModelFormSet -from django.http import HttpResponse, StreamingHttpResponse +from django.http import HttpResponse, JsonResponse, StreamingHttpResponse from django.urls import path from django.utils.html import format_html from django.utils.safestring import mark_safe +from django.views.decorators.common import no_append_slash from .forms import MediaActionForm from .models import ( @@ -100,7 +101,19 @@ class ArticleForm(forms.ModelForm): model = Article -class ArticleAdmin(admin.ModelAdmin): +class ArticleAdminWithExtraUrl(admin.ModelAdmin): + def get_urls(self): + urlpatterns = super().get_urls() + urlpatterns.append( + path('extra.json', self.admin_site.admin_view(self.extra_json), name='article_extra_json') + ) + return urlpatterns + + def extra_json(self, request): + return JsonResponse({}) + + +class ArticleAdmin(ArticleAdminWithExtraUrl): list_display = ( 'content', 'date', callable_year, 'model_year', 'modeladmin_year', 'model_year_reversed', 'section', lambda obj: obj.title, @@ -1181,5 +1194,19 @@ class ArticleAdmin9(admin.ModelAdmin): return obj is None +class ActorAdmin9(admin.ModelAdmin): + def get_urls(self): + # Opt-out of append slash for single model. + urls = super().get_urls() + for pattern in urls: + pattern.callback = no_append_slash(pattern.callback) + return urls + + site9 = admin.AdminSite(name='admin9') site9.register(Article, ArticleAdmin9) +site9.register(Actor, ActorAdmin9) + +site10 = admin.AdminSite(name='admin10') +site10.final_catch_all_view = False +site10.register(Article, ArticleAdminWithExtraUrl) diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 6f47465c4a..297760f807 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -6470,3 +6470,214 @@ class GetFormsetsWithInlinesArgumentTest(TestCase): post_data = {'name': '2'} response = self.client.post(reverse('admin:admin_views_implicitlygeneratedpk_change', args=(1,)), post_data) self.assertEqual(response.status_code, 302) + + +@override_settings(ROOT_URLCONF='admin_views.urls') +class AdminSiteFinalCatchAllPatternTests(TestCase): + """ + Verifies the behaviour of the admin catch-all view. + + * Anonynous/non-staff users are redirected to login for all URLs, whether + otherwise valid or not. + * APPEND_SLASH is applied for staff if needed. + * Otherwise Http404. + * Catch-all view disabled via AdminSite.final_catch_all_view. + """ + def test_unknown_url_redirects_login_if_not_authenticated(self): + unknown_url = '/test_admin/admin/unknown/' + response = self.client.get(unknown_url) + self.assertRedirects(response, '%s?next=%s' % (reverse('admin:login'), unknown_url)) + + def test_unknown_url_404_if_authenticated(self): + superuser = User.objects.create_superuser( + username='super', + password='secret', + email='super@example.com', + ) + self.client.force_login(superuser) + unknown_url = '/test_admin/admin/unknown/' + response = self.client.get(unknown_url) + self.assertEqual(response.status_code, 404) + + def test_known_url_redirects_login_if_not_authenticated(self): + known_url = reverse('admin:admin_views_article_changelist') + response = self.client.get(known_url) + self.assertRedirects(response, '%s?next=%s' % (reverse('admin:login'), known_url)) + + def test_known_url_missing_slash_redirects_login_if_not_authenticated(self): + known_url = reverse('admin:admin_views_article_changelist')[:-1] + response = self.client.get(known_url) + # Redirects with the next URL also missing the slash. + self.assertRedirects(response, '%s?next=%s' % (reverse('admin:login'), known_url)) + + def test_non_admin_url_shares_url_prefix(self): + url = reverse('non_admin')[:-1] + response = self.client.get(url) + # Redirects with the next URL also missing the slash. + self.assertRedirects(response, '%s?next=%s' % (reverse('admin:login'), url)) + + def test_url_without_trailing_slash_if_not_authenticated(self): + url = reverse('admin:article_extra_json') + response = self.client.get(url) + self.assertRedirects(response, '%s?next=%s' % (reverse('admin:login'), url)) + + def test_unkown_url_without_trailing_slash_if_not_authenticated(self): + url = reverse('admin:article_extra_json')[:-1] + response = self.client.get(url) + self.assertRedirects(response, '%s?next=%s' % (reverse('admin:login'), url)) + + @override_settings(APPEND_SLASH=True) + def test_missing_slash_append_slash_true_unknown_url(self): + superuser = User.objects.create_user( + username='staff', + password='secret', + email='staff@example.com', + is_staff=True, + ) + self.client.force_login(superuser) + unknown_url = '/test_admin/admin/unknown/' + response = self.client.get(unknown_url[:-1]) + self.assertEqual(response.status_code, 404) + + @override_settings(APPEND_SLASH=True) + def test_missing_slash_append_slash_true(self): + superuser = User.objects.create_user( + username='staff', + password='secret', + email='staff@example.com', + is_staff=True, + ) + self.client.force_login(superuser) + known_url = reverse('admin:admin_views_article_changelist') + response = self.client.get(known_url[:-1]) + self.assertRedirects(response, known_url, status_code=301, target_status_code=403) + + @override_settings(APPEND_SLASH=True) + def test_missing_slash_append_slash_true_non_staff_user(self): + user = User.objects.create_user( + username='user', + password='secret', + email='user@example.com', + is_staff=False, + ) + self.client.force_login(user) + known_url = reverse('admin:admin_views_article_changelist') + response = self.client.get(known_url[:-1]) + self.assertRedirects(response, '/test_admin/admin/login/?next=/test_admin/admin/admin_views/article') + + @override_settings(APPEND_SLASH=False) + def test_missing_slash_append_slash_false(self): + superuser = User.objects.create_user( + username='staff', + password='secret', + email='staff@example.com', + is_staff=True, + ) + self.client.force_login(superuser) + known_url = reverse('admin:admin_views_article_changelist') + response = self.client.get(known_url[:-1]) + self.assertEqual(response.status_code, 404) + + @override_settings(APPEND_SLASH=True) + def test_single_model_no_append_slash(self): + superuser = User.objects.create_user( + username='staff', + password='secret', + email='staff@example.com', + is_staff=True, + ) + self.client.force_login(superuser) + known_url = reverse('admin9:admin_views_actor_changelist') + response = self.client.get(known_url[:-1]) + self.assertEqual(response.status_code, 404) + + # Same tests above with final_catch_all_view=False. + + def test_unknown_url_404_if_not_authenticated_without_final_catch_all_view(self): + unknown_url = '/test_admin/admin10/unknown/' + response = self.client.get(unknown_url) + self.assertEqual(response.status_code, 404) + + def test_unknown_url_404_if_authenticated_without_final_catch_all_view(self): + superuser = User.objects.create_superuser( + username='super', + password='secret', + email='super@example.com', + ) + self.client.force_login(superuser) + unknown_url = '/test_admin/admin10/unknown/' + response = self.client.get(unknown_url) + self.assertEqual(response.status_code, 404) + + def test_known_url_redirects_login_if_not_authenticated_without_final_catch_all_view(self): + known_url = reverse('admin10:admin_views_article_changelist') + response = self.client.get(known_url) + self.assertRedirects(response, '%s?next=%s' % (reverse('admin10:login'), known_url)) + + def test_known_url_missing_slash_redirects_with_slash_if_not_authenticated_without_final_catch_all_view(self): + known_url = reverse('admin10:admin_views_article_changelist') + response = self.client.get(known_url[:-1]) + self.assertRedirects(response, known_url, status_code=301, fetch_redirect_response=False) + + def test_non_admin_url_shares_url_prefix_without_final_catch_all_view(self): + url = reverse('non_admin10') + response = self.client.get(url[:-1]) + self.assertRedirects(response, url, status_code=301) + + def test_url_without_trailing_slash_if_not_authenticated_without_final_catch_all_view(self): + url = reverse('admin10:article_extra_json') + response = self.client.get(url) + self.assertRedirects(response, '%s?next=%s' % (reverse('admin10:login'), url)) + + def test_unkown_url_without_trailing_slash_if_not_authenticated_without_final_catch_all_view(self): + url = reverse('admin10:article_extra_json')[:-1] + response = self.client.get(url) + # Matches test_admin/admin10/admin_views/article// + self.assertRedirects(response, url + '/', status_code=301, fetch_redirect_response=False) + + @override_settings(APPEND_SLASH=True) + def test_missing_slash_append_slash_true_unknown_url_without_final_catch_all_view(self): + superuser = User.objects.create_user( + username='staff', + password='secret', + email='staff@example.com', + is_staff=True, + ) + self.client.force_login(superuser) + unknown_url = '/test_admin/admin10/unknown/' + response = self.client.get(unknown_url[:-1]) + self.assertEqual(response.status_code, 404) + + @override_settings(APPEND_SLASH=True) + def test_missing_slash_append_slash_true_without_final_catch_all_view(self): + superuser = User.objects.create_user( + username='staff', + password='secret', + email='staff@example.com', + is_staff=True, + ) + self.client.force_login(superuser) + known_url = reverse('admin10:admin_views_article_changelist') + response = self.client.get(known_url[:-1]) + self.assertRedirects(response, known_url, status_code=301, target_status_code=403) + + @override_settings(APPEND_SLASH=False) + def test_missing_slash_append_slash_false_without_final_catch_all_view(self): + superuser = User.objects.create_user( + username='staff', + password='secret', + email='staff@example.com', + is_staff=True, + ) + self.client.force_login(superuser) + known_url = reverse('admin10:admin_views_article_changelist') + response = self.client.get(known_url[:-1]) + self.assertEqual(response.status_code, 404) + + # Outside admin. + + def test_non_admin_url_404_if_not_authenticated(self): + unknown_url = '/unknown/' + response = self.client.get(unknown_url) + # Does not redirect to the admin login. + self.assertEqual(response.status_code, 404) diff --git a/tests/admin_views/urls.py b/tests/admin_views/urls.py index ca684b2f2e..355e082b68 100644 --- a/tests/admin_views/urls.py +++ b/tests/admin_views/urls.py @@ -1,8 +1,14 @@ +from django.http import HttpResponse from django.urls import include, path from . import admin, custom_has_permission_admin, customadmin, views from .test_autocomplete_view import site as autocomplete_site + +def non_admin_view(request): + return HttpResponse() + + urlpatterns = [ path('test_admin/admin/doc/', include('django.contrib.admindocs.urls')), path('test_admin/admin/secure-view/', views.secure_view, name='secure_view'), @@ -17,6 +23,10 @@ urlpatterns = [ # All admin views accept `extra_context` to allow adding it like this: path('test_admin/admin8/', (admin.site.get_urls(), 'admin', 'admin-extra-context'), {'extra_context': {}}), path('test_admin/admin9/', admin.site9.urls), + path('test_admin/admin10/', admin.site10.urls), path('test_admin/has_permission_admin/', custom_has_permission_admin.site.urls), path('test_admin/autocomplete_admin/', autocomplete_site.urls), + # Shares the admin URL prefix. + path('test_admin/admin/non_admin_view/', non_admin_view, name='non_admin'), + path('test_admin/admin10/non_admin_view/', non_admin_view, name='non_admin10'), ]