diff --git a/django/conf/urls/__init__.py b/django/conf/urls/__init__.py index 04fb1dff59..c0340c0543 100644 --- a/django/conf/urls/__init__.py +++ b/django/conf/urls/__init__.py @@ -5,8 +5,9 @@ from django.utils.importlib import import_module from django.utils import six -__all__ = ['handler403', 'handler404', 'handler500', 'include', 'patterns', 'url'] +__all__ = ['handler400', 'handler403', 'handler404', 'handler500', 'include', 'patterns', 'url'] +handler400 = 'django.views.defaults.bad_request' handler403 = 'django.views.defaults.permission_denied' handler404 = 'django.views.defaults.page_not_found' handler500 = 'django.views.defaults.server_error' diff --git a/django/contrib/admin/exceptions.py b/django/contrib/admin/exceptions.py new file mode 100644 index 0000000000..2e094c6da1 --- /dev/null +++ b/django/contrib/admin/exceptions.py @@ -0,0 +1,6 @@ +from django.core.exceptions import SuspiciousOperation + + +class DisallowedModelAdminLookup(SuspiciousOperation): + """Invalid filter was passed to admin view via URL querystring""" + pass diff --git a/django/contrib/admin/views/main.py b/django/contrib/admin/views/main.py index 21ac30b7b3..dbed21265c 100644 --- a/django/contrib/admin/views/main.py +++ b/django/contrib/admin/views/main.py @@ -14,6 +14,7 @@ from django.utils.translation import ugettext, ugettext_lazy from django.utils.http import urlencode from django.contrib.admin import FieldListFilter +from django.contrib.admin.exceptions import DisallowedModelAdminLookup from django.contrib.admin.options import IncorrectLookupParameters from django.contrib.admin.util import (quote, get_fields_from_path, lookup_needs_distinct, prepare_lookup_value) @@ -128,7 +129,7 @@ class ChangeList(six.with_metaclass(RenameChangeListMethods)): lookup_params[force_str(key)] = value if not self.model_admin.lookup_allowed(key, value): - raise SuspiciousOperation("Filtering by %s not allowed" % key) + raise DisallowedModelAdminLookup("Filtering by %s not allowed" % key) filter_specs = [] if self.list_filter: diff --git a/django/contrib/auth/tests/test_views.py b/django/contrib/auth/tests/test_views.py index fe1d7fb52f..94cad90b15 100644 --- a/django/contrib/auth/tests/test_views.py +++ b/django/contrib/auth/tests/test_views.py @@ -10,7 +10,6 @@ from django.conf import global_settings, settings from django.contrib.sites.models import Site, RequestSite from django.contrib.auth.models import User from django.core import mail -from django.core.exceptions import SuspiciousOperation from django.core.urlresolvers import reverse, NoReverseMatch from django.http import QueryDict, HttpRequest from django.utils.encoding import force_text @@ -18,7 +17,7 @@ from django.utils.html import escape from django.utils.http import urlquote from django.utils._os import upath from django.test import TestCase -from django.test.utils import override_settings +from django.test.utils import override_settings, patch_logger from django.middleware.csrf import CsrfViewMiddleware from django.contrib.sessions.middleware import SessionMiddleware @@ -155,23 +154,28 @@ class PasswordResetTest(AuthViewsTestCase): # produce a meaningful reset URL, we need to be certain that the # HTTP_HOST header isn't poisoned. This is done as a check when get_host() # is invoked, but we check here as a practical consequence. - with self.assertRaises(SuspiciousOperation): - self.client.post('/password_reset/', - {'email': 'staffmember@example.com'}, - HTTP_HOST='www.example:dr.frankenstein@evil.tld' - ) - self.assertEqual(len(mail.outbox), 0) + with patch_logger('django.security.DisallowedHost', 'error') as logger_calls: + response = self.client.post('/password_reset/', + {'email': 'staffmember@example.com'}, + HTTP_HOST='www.example:dr.frankenstein@evil.tld' + ) + self.assertEqual(response.status_code, 400) + self.assertEqual(len(mail.outbox), 0) + self.assertEqual(len(logger_calls), 1) # Skip any 500 handler action (like sending more mail...) @override_settings(DEBUG_PROPAGATE_EXCEPTIONS=True) def test_poisoned_http_host_admin_site(self): "Poisoned HTTP_HOST headers can't be used for reset emails on admin views" - with self.assertRaises(SuspiciousOperation): - self.client.post('/admin_password_reset/', - {'email': 'staffmember@example.com'}, - HTTP_HOST='www.example:dr.frankenstein@evil.tld' - ) - self.assertEqual(len(mail.outbox), 0) + with patch_logger('django.security.DisallowedHost', 'error') as logger_calls: + response = self.client.post('/admin_password_reset/', + {'email': 'staffmember@example.com'}, + HTTP_HOST='www.example:dr.frankenstein@evil.tld' + ) + self.assertEqual(response.status_code, 400) + self.assertEqual(len(mail.outbox), 0) + self.assertEqual(len(logger_calls), 1) + def _test_confirm_start(self): # Start by creating the email @@ -678,5 +682,7 @@ class ChangelistTests(AuthViewsTestCase): self.login() # A lookup that tries to filter on password isn't OK - with self.assertRaises(SuspiciousOperation): + with patch_logger('django.security.DisallowedModelAdminLookup', 'error') as logger_calls: response = self.client.get('/admin/auth/user/?password__startswith=sha1$') + self.assertEqual(response.status_code, 400) + self.assertEqual(len(logger_calls), 1) diff --git a/django/contrib/formtools/exceptions.py b/django/contrib/formtools/exceptions.py new file mode 100644 index 0000000000..f07ac9f745 --- /dev/null +++ b/django/contrib/formtools/exceptions.py @@ -0,0 +1,6 @@ +from django.core.exceptions import SuspiciousOperation + + +class WizardViewCookieModified(SuspiciousOperation): + """Signature of cookie modified""" + pass diff --git a/django/contrib/formtools/wizard/storage/cookie.py b/django/contrib/formtools/wizard/storage/cookie.py index e80361042f..9bf6503f18 100644 --- a/django/contrib/formtools/wizard/storage/cookie.py +++ b/django/contrib/formtools/wizard/storage/cookie.py @@ -1,8 +1,8 @@ import json -from django.core.exceptions import SuspiciousOperation from django.core.signing import BadSignature +from django.contrib.formtools.exceptions import WizardViewCookieModified from django.contrib.formtools.wizard import storage @@ -21,7 +21,7 @@ class CookieStorage(storage.BaseStorage): except KeyError: data = None except BadSignature: - raise SuspiciousOperation('WizardView cookie manipulated') + raise WizardViewCookieModified('WizardView cookie manipulated') if data is None: return None return json.loads(data, cls=json.JSONDecoder) diff --git a/django/contrib/sessions/backends/base.py b/django/contrib/sessions/backends/base.py index f79a264500..759d7ac7ad 100644 --- a/django/contrib/sessions/backends/base.py +++ b/django/contrib/sessions/backends/base.py @@ -2,6 +2,8 @@ from __future__ import unicode_literals import base64 from datetime import datetime, timedelta +import logging + try: from django.utils.six.moves import cPickle as pickle except ImportError: @@ -14,7 +16,9 @@ from django.utils.crypto import constant_time_compare from django.utils.crypto import get_random_string from django.utils.crypto import salted_hmac from django.utils import timezone -from django.utils.encoding import force_bytes +from django.utils.encoding import force_bytes, force_text + +from django.contrib.sessions.exceptions import SuspiciousSession # session_key should not be case sensitive because some backends can store it # on case insensitive file systems. @@ -94,12 +98,16 @@ class SessionBase(object): hash, pickled = encoded_data.split(b':', 1) expected_hash = self._hash(pickled) if not constant_time_compare(hash.decode(), expected_hash): - raise SuspiciousOperation("Session data corrupted") + raise SuspiciousSession("Session data corrupted") else: return pickle.loads(pickled) - except Exception: + except Exception as e: # ValueError, SuspiciousOperation, unpickling exceptions. If any of # these happen, just return an empty dictionary (an empty session). + if isinstance(e, SuspiciousOperation): + logger = logging.getLogger('django.security.%s' % + e.__class__.__name__) + logger.warning(force_text(e)) return {} def update(self, dict_): diff --git a/django/contrib/sessions/backends/cached_db.py b/django/contrib/sessions/backends/cached_db.py index 31c6fbfce3..be22c1f97a 100644 --- a/django/contrib/sessions/backends/cached_db.py +++ b/django/contrib/sessions/backends/cached_db.py @@ -2,10 +2,13 @@ Cached, database-backed sessions. """ +import logging + from django.contrib.sessions.backends.db import SessionStore as DBStore from django.core.cache import cache from django.core.exceptions import SuspiciousOperation from django.utils import timezone +from django.utils.encoding import force_text KEY_PREFIX = "django.contrib.sessions.cached_db" @@ -41,7 +44,11 @@ class SessionStore(DBStore): data = self.decode(s.session_data) cache.set(self.cache_key, data, self.get_expiry_age(expiry=s.expire_date)) - except (Session.DoesNotExist, SuspiciousOperation): + except (Session.DoesNotExist, SuspiciousOperation) as e: + if isinstance(e, SuspiciousOperation): + logger = logging.getLogger('django.security.%s' % + e.__class__.__name__) + logger.warning(force_text(e)) self.create() data = {} return data diff --git a/django/contrib/sessions/backends/db.py b/django/contrib/sessions/backends/db.py index 30da0b7a10..206fca2700 100644 --- a/django/contrib/sessions/backends/db.py +++ b/django/contrib/sessions/backends/db.py @@ -1,8 +1,10 @@ +import logging + from django.contrib.sessions.backends.base import SessionBase, CreateError from django.core.exceptions import SuspiciousOperation from django.db import IntegrityError, transaction, router from django.utils import timezone - +from django.utils.encoding import force_text class SessionStore(SessionBase): """ @@ -18,7 +20,11 @@ class SessionStore(SessionBase): expire_date__gt=timezone.now() ) return self.decode(s.session_data) - except (Session.DoesNotExist, SuspiciousOperation): + except (Session.DoesNotExist, SuspiciousOperation) as e: + if isinstance(e, SuspiciousOperation): + logger = logging.getLogger('django.security.%s' % + e.__class__.__name__) + logger.warning(force_text(e)) self.create() return {} diff --git a/django/contrib/sessions/backends/file.py b/django/contrib/sessions/backends/file.py index 3c3408e9a8..f47aa2d867 100644 --- a/django/contrib/sessions/backends/file.py +++ b/django/contrib/sessions/backends/file.py @@ -1,5 +1,6 @@ import datetime import errno +import logging import os import shutil import tempfile @@ -8,6 +9,9 @@ from django.conf import settings from django.contrib.sessions.backends.base import SessionBase, CreateError, VALID_KEY_CHARS from django.core.exceptions import SuspiciousOperation, ImproperlyConfigured from django.utils import timezone +from django.utils.encoding import force_text + +from django.contrib.sessions.exceptions import InvalidSessionKey class SessionStore(SessionBase): """ @@ -48,7 +52,7 @@ class SessionStore(SessionBase): # should always be md5s, so they should never contain directory # components. if not set(session_key).issubset(set(VALID_KEY_CHARS)): - raise SuspiciousOperation( + raise InvalidSessionKey( "Invalid characters in session key") return os.path.join(self.storage_path, self.file_prefix + session_key) @@ -75,7 +79,11 @@ class SessionStore(SessionBase): if file_data: try: session_data = self.decode(file_data) - except (EOFError, SuspiciousOperation): + except (EOFError, SuspiciousOperation) as e: + if isinstance(e, SuspiciousOperation): + logger = logging.getLogger('django.security.%s' % + e.__class__.__name__) + logger.warning(force_text(e)) self.create() # Remove expired sessions. diff --git a/django/contrib/sessions/exceptions.py b/django/contrib/sessions/exceptions.py new file mode 100644 index 0000000000..4f4dc6b048 --- /dev/null +++ b/django/contrib/sessions/exceptions.py @@ -0,0 +1,11 @@ +from django.core.exceptions import SuspiciousOperation + + +class InvalidSessionKey(SuspiciousOperation): + """Invalid characters in session key""" + pass + + +class SuspiciousSession(SuspiciousOperation): + """The session may be tampered with""" + pass diff --git a/django/contrib/sessions/tests.py b/django/contrib/sessions/tests.py index 1a7286e77e..cd8191a6a4 100644 --- a/django/contrib/sessions/tests.py +++ b/django/contrib/sessions/tests.py @@ -1,3 +1,4 @@ +import base64 from datetime import timedelta import os import shutil @@ -15,14 +16,16 @@ from django.contrib.sessions.models import Session from django.contrib.sessions.middleware import SessionMiddleware from django.core.cache import get_cache from django.core import management -from django.core.exceptions import ImproperlyConfigured, SuspiciousOperation +from django.core.exceptions import ImproperlyConfigured from django.http import HttpResponse from django.test import TestCase, RequestFactory -from django.test.utils import override_settings +from django.test.utils import override_settings, patch_logger from django.utils import six from django.utils import timezone from django.utils import unittest +from django.contrib.sessions.exceptions import InvalidSessionKey + class SessionTestsMixin(object): # This does not inherit from TestCase to avoid any tests being run with this @@ -272,6 +275,15 @@ class SessionTestsMixin(object): encoded = self.session.encode(data) self.assertEqual(self.session.decode(encoded), data) + def test_decode_failure_logged_to_security(self): + bad_encode = base64.b64encode(b'flaskdj:alkdjf') + with patch_logger('django.security.SuspiciousSession', 'warning') as calls: + self.assertEqual({}, self.session.decode(bad_encode)) + # check that the failed decode is logged + self.assertEqual(len(calls), 1) + self.assertTrue('corrupted' in calls[0]) + + def test_actual_expiry(self): # Regression test for #19200 old_session_key = None @@ -411,12 +423,12 @@ class FileSessionTests(SessionTestsMixin, unittest.TestCase): # This is tested directly on _key_to_file, as load() will swallow # a SuspiciousOperation in the same way as an IOError - by creating # a new session, making it unclear whether the slashes were detected. - self.assertRaises(SuspiciousOperation, + self.assertRaises(InvalidSessionKey, self.backend()._key_to_file, "a\\b\\c") def test_invalid_key_forwardslash(self): # Ensure we don't allow directory-traversal - self.assertRaises(SuspiciousOperation, + self.assertRaises(InvalidSessionKey, self.backend()._key_to_file, "a/b/c") @override_settings(SESSION_ENGINE="django.contrib.sessions.backends.file") diff --git a/django/core/exceptions.py b/django/core/exceptions.py index 233af40f88..2c79736e33 100644 --- a/django/core/exceptions.py +++ b/django/core/exceptions.py @@ -1,6 +1,7 @@ """ Global Django exception and warning classes. """ +import logging from functools import reduce @@ -9,37 +10,56 @@ class DjangoRuntimeWarning(RuntimeWarning): class ObjectDoesNotExist(Exception): - "The requested object does not exist" + """The requested object does not exist""" silent_variable_failure = True class MultipleObjectsReturned(Exception): - "The query returned multiple objects when only one was expected." + """The query returned multiple objects when only one was expected.""" pass class SuspiciousOperation(Exception): - "The user did something suspicious" + """The user did something suspicious""" + + +class SuspiciousMultipartForm(SuspiciousOperation): + """Suspect MIME request in multipart form data""" + pass + + +class SuspiciousFileOperation(SuspiciousOperation): + """A Suspicious filesystem operation was attempted""" + pass + + +class DisallowedHost(SuspiciousOperation): + """HTTP_HOST header contains invalid value""" + pass + + +class DisallowedRedirect(SuspiciousOperation): + """Redirect to scheme not in allowed list""" pass class PermissionDenied(Exception): - "The user did not have permission to do that" + """The user did not have permission to do that""" pass class ViewDoesNotExist(Exception): - "The requested view does not exist" + """The requested view does not exist""" pass class MiddlewareNotUsed(Exception): - "This middleware is not used in this server configuration" + """This middleware is not used in this server configuration""" pass class ImproperlyConfigured(Exception): - "Django is somehow improperly configured" + """Django is somehow improperly configured""" pass diff --git a/django/core/files/storage.py b/django/core/files/storage.py index 18d15e1ab6..977b6a68a8 100644 --- a/django/core/files/storage.py +++ b/django/core/files/storage.py @@ -8,7 +8,7 @@ import itertools from datetime import datetime from django.conf import settings -from django.core.exceptions import SuspiciousOperation +from django.core.exceptions import SuspiciousFileOperation from django.core.files import locks, File from django.core.files.move import file_move_safe from django.utils.encoding import force_text, filepath_to_uri @@ -260,7 +260,7 @@ class FileSystemStorage(Storage): try: path = safe_join(self.location, name) except ValueError: - raise SuspiciousOperation("Attempted access to '%s' denied." % name) + raise SuspiciousFileOperation("Attempted access to '%s' denied." % name) return os.path.normpath(path) def size(self, name): diff --git a/django/core/handlers/base.py b/django/core/handlers/base.py index 555fd98fc6..59118656c6 100644 --- a/django/core/handlers/base.py +++ b/django/core/handlers/base.py @@ -8,7 +8,7 @@ from django import http from django.conf import settings from django.core import urlresolvers from django.core import signals -from django.core.exceptions import MiddlewareNotUsed, PermissionDenied +from django.core.exceptions import MiddlewareNotUsed, PermissionDenied, SuspiciousOperation from django.db import connections, transaction from django.utils.encoding import force_text from django.utils.module_loading import import_by_path @@ -170,11 +170,27 @@ class BaseHandler(object): response = self.handle_uncaught_exception(request, resolver, sys.exc_info()) + except SuspiciousOperation as e: + # The request logger receives events for any problematic request + # The security logger receives events for all SuspiciousOperations + security_logger = logging.getLogger('django.security.%s' % + e.__class__.__name__) + security_logger.error(force_text(e)) + + try: + callback, param_dict = resolver.resolve400() + response = callback(request, **param_dict) + except: + signals.got_request_exception.send( + sender=self.__class__, request=request) + response = self.handle_uncaught_exception(request, + resolver, sys.exc_info()) + except SystemExit: # Allow sys.exit() to actually exit. See tickets #1023 and #4701 raise - except: # Handle everything else, including SuspiciousOperation, etc. + except: # Handle everything else. # Get the exception info now, in case another exception is thrown later. signals.got_request_exception.send(sender=self.__class__, request=request) response = self.handle_uncaught_exception(request, resolver, sys.exc_info()) diff --git a/django/core/urlresolvers.py b/django/core/urlresolvers.py index c3d93bb247..5f314ff490 100644 --- a/django/core/urlresolvers.py +++ b/django/core/urlresolvers.py @@ -360,6 +360,9 @@ class RegexURLResolver(LocaleRegexProvider): callback = getattr(urls, 'handler%s' % view_type) return get_callable(callback), {} + def resolve400(self): + return self._resolve_special('400') + def resolve403(self): return self._resolve_special('403') diff --git a/django/http/multipartparser.py b/django/http/multipartparser.py index 26e10da1a2..eeb435fa57 100644 --- a/django/http/multipartparser.py +++ b/django/http/multipartparser.py @@ -11,7 +11,7 @@ import cgi import sys from django.conf import settings -from django.core.exceptions import SuspiciousOperation +from django.core.exceptions import SuspiciousMultipartForm from django.utils.datastructures import MultiValueDict from django.utils.encoding import force_text from django.utils import six @@ -370,7 +370,7 @@ class LazyStream(six.Iterator): if current_number == num_bytes]) if number_equal > 40: - raise SuspiciousOperation( + raise SuspiciousMultipartForm( "The multipart parser got stuck, which shouldn't happen with" " normal uploaded files. Check for malicious upload activity;" " if there is none, report this to the Django developers." diff --git a/django/http/request.py b/django/http/request.py index 749b9f2561..e6811aa6cc 100644 --- a/django/http/request.py +++ b/django/http/request.py @@ -14,7 +14,7 @@ except ImportError: from django.conf import settings from django.core import signing -from django.core.exceptions import SuspiciousOperation, ImproperlyConfigured +from django.core.exceptions import DisallowedHost, ImproperlyConfigured from django.core.files import uploadhandler from django.http.multipartparser import MultiPartParser from django.utils import six @@ -72,7 +72,7 @@ class HttpRequest(object): msg = "Invalid HTTP_HOST header: %r." % host if domain: msg += "You may need to add %r to ALLOWED_HOSTS." % domain - raise SuspiciousOperation(msg) + raise DisallowedHost(msg) def get_full_path(self): # RFC 3986 requires query string arguments to be in the ASCII range. diff --git a/django/http/response.py b/django/http/response.py index 671fb1c573..9aa49b1d5f 100644 --- a/django/http/response.py +++ b/django/http/response.py @@ -12,7 +12,7 @@ except ImportError: from django.conf import settings from django.core import signals from django.core import signing -from django.core.exceptions import SuspiciousOperation +from django.core.exceptions import DisallowedRedirect from django.http.cookie import SimpleCookie from django.utils import six, timezone from django.utils.encoding import force_bytes, iri_to_uri @@ -452,7 +452,7 @@ class HttpResponseRedirectBase(HttpResponse): def __init__(self, redirect_to, *args, **kwargs): parsed = urlparse(redirect_to) if parsed.scheme and parsed.scheme not in self.allowed_schemes: - raise SuspiciousOperation("Unsafe redirect to URL with protocol '%s'" % parsed.scheme) + raise DisallowedRedirect("Unsafe redirect to URL with protocol '%s'" % parsed.scheme) super(HttpResponseRedirectBase, self).__init__(*args, **kwargs) self['Location'] = iri_to_uri(redirect_to) diff --git a/django/test/utils.py b/django/test/utils.py index fb9221d25c..d178c9b29c 100644 --- a/django/test/utils.py +++ b/django/test/utils.py @@ -1,3 +1,5 @@ +from contextlib import contextmanager +import logging import re import sys import warnings @@ -401,3 +403,21 @@ class IgnoreDeprecationWarningsMixin(object): class IgnorePendingDeprecationWarningsMixin(IgnoreDeprecationWarningsMixin): warning_class = PendingDeprecationWarning + + +@contextmanager +def patch_logger(logger_name, log_level): + """ + Context manager that takes a named logger and the logging level + and provides a simple mock-like list of messages received + """ + calls = [] + def replacement(msg): + calls.append(msg) + logger = logging.getLogger(logger_name) + orig = getattr(logger, log_level) + setattr(logger, log_level, replacement) + try: + yield calls + finally: + setattr(logger, log_level, orig) diff --git a/django/utils/log.py b/django/utils/log.py index a9b62caae1..2abf7701cb 100644 --- a/django/utils/log.py +++ b/django/utils/log.py @@ -63,6 +63,11 @@ DEFAULT_LOGGING = { 'level': 'ERROR', 'propagate': False, }, + 'django.security': { + 'handlers': ['mail_admins'], + 'level': 'ERROR', + 'propagate': False, + }, 'py.warnings': { 'handlers': ['console'], }, diff --git a/django/views/defaults.py b/django/views/defaults.py index 89228c50c9..c8a62fc753 100644 --- a/django/views/defaults.py +++ b/django/views/defaults.py @@ -43,6 +43,21 @@ def server_error(request, template_name='500.html'): return http.HttpResponseServerError(template.render(Context({}))) +@requires_csrf_token +def bad_request(request, template_name='400.html'): + """ + 400 error handler. + + Templates: :template:`400.html` + Context: None + """ + try: + template = loader.get_template(template_name) + except TemplateDoesNotExist: + return http.HttpResponseBadRequest('

Bad Request (400)

') + return http.HttpResponseBadRequest(template.render(Context({}))) + + # This can be called when CsrfViewMiddleware.process_view has not run, # therefore need @requires_csrf_token in case the template needs # {% csrf_token %}. diff --git a/docs/ref/exceptions.txt b/docs/ref/exceptions.txt index f9a1715180..6a5e6f49d5 100644 --- a/docs/ref/exceptions.txt +++ b/docs/ref/exceptions.txt @@ -44,9 +44,24 @@ SuspiciousOperation ------------------- .. exception:: SuspiciousOperation - The :exc:`SuspiciousOperation` exception is raised when a user has performed - an operation that should be considered suspicious from a security perspective, - such as tampering with a session cookie. + The :exc:`SuspiciousOperation` exception is raised when a user has + performed an operation that should be considered suspicious from a security + perspective, such as tampering with a session cookie. Subclasses of + SuspiciousOperation include: + + * DisallowedHost + * DisallowedModelAdminLookup + * DisallowedRedirect + * InvalidSessionKey + * SuspiciousFileOperation + * SuspiciousMultipartForm + * SuspiciousSession + * WizardViewCookieModified + + If a ``SuspiciousOperation`` exception reaches the WSGI handler level it is + logged at the ``Error`` level and results in + a :class:`~django.http.HttpResponseBadRequest`. See the :doc:`logging + documentation ` for more information. PermissionDenied ---------------- diff --git a/docs/releases/1.6.txt b/docs/releases/1.6.txt index 35488b4110..825c5dc28d 100644 --- a/docs/releases/1.6.txt +++ b/docs/releases/1.6.txt @@ -270,6 +270,13 @@ Minor features stores active language in session if it is not present there. This prevents loss of language settings after session flush, e.g. logout. +* :exc:`~django.core.exceptions.SuspiciousOperation` has been differentiated + into a number of subclasses, and each will log to a matching named logger + under the ``django.security`` logging hierarchy. Along with this change, + a ``handler400`` mechanism and default view are used whenever + a ``SuspiciousOperation`` reaches the WSGI handler to return an + ``HttpResponseBadRequest``. + Backwards incompatible changes in 1.6 ===================================== diff --git a/docs/topics/http/views.txt b/docs/topics/http/views.txt index 2ccedec2f7..5c27c9c958 100644 --- a/docs/topics/http/views.txt +++ b/docs/topics/http/views.txt @@ -231,3 +231,25 @@ same way you can for the 404 and 500 views by specifying a ``handler403`` in your URLconf:: handler403 = 'mysite.views.my_custom_permission_denied_view' + +.. _http_bad_request_view: + +The 400 (bad request) view +-------------------------- + +When a :exc:`~django.core.exceptions.SuspiciousOperation` is raised in Django, +the it may be handled by a component of Django (for example resetting the +session data). If not specifically handled, Django will consider the current +request a 'bad request' instead of a server error. + +The view ``django.views.defaults.bad_request``, is otherwise very similar to +the ``server_error`` view, but returns with the status code 400 indicating that +the error condition was the result of a client operation. + +Like the ``server_error`` view, the default ``bad_request`` should suffice for +99% of Web applications, but if you want to override the view, you can specify +``handler400`` in your URLconf, like so:: + + handler400 = 'mysite.views.my_custom_bad_request_view' + +``bad_request`` views are also only used when :setting:`DEBUG` is ``False``. diff --git a/docs/topics/logging.txt b/docs/topics/logging.txt index a31dc01cc5..a88201ad47 100644 --- a/docs/topics/logging.txt +++ b/docs/topics/logging.txt @@ -394,7 +394,7 @@ requirements of logging in Web server environment. Loggers ------- -Django provides three built-in loggers. +Django provides four built-in loggers. ``django`` ~~~~~~~~~~ @@ -434,6 +434,35 @@ For performance reasons, SQL logging is only enabled when ``settings.DEBUG`` is set to ``True``, regardless of the logging level or handlers that are installed. +``django.security.*`` +~~~~~~~~~~~~~~~~~~~~~~ + +The security loggers will receive messages on any occurrence of +:exc:`~django.core.exceptions.SuspiciousOperation`. There is a sub-logger for +each sub-type of SuspiciousOperation. The level of the log event depends on +where the exception is handled. Most occurrences are logged as a warning, while +any ``SuspiciousOperation`` that reaches the WSGI handler will be logged as an +error. For example, when an HTTP ``Host`` header is included in a request from +a client that does not match :setting:`ALLOWED_HOSTS`, Django will return a 400 +response, and an error message will be logged to the +``django.security.DisallowedHost`` logger. + +Only the parent ``django.security`` logger is configured by default, and all +child loggers will propagate to the parent logger. The ``django.security`` +logger is configured the same as the ``django.request`` logger, and any error +events will be mailed to admins. Requests resulting in a 400 response due to +a ``SuspiciousOperation`` will not be logged to the ``django.request`` logger, +but only to the ``django.security`` logger. + +To silence a particular type of SuspiciousOperation, you can override that +specific logger following this example:: + + 'loggers': { + 'django.security.DisallowedHost': { + 'handlers': ['null'], + 'propagate': False, + }, + Handlers -------- diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index c0f6533cb2..3485ea353b 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -11,7 +11,6 @@ except ImportError: # Python 2 from django.conf import settings, global_settings from django.core import mail -from django.core.exceptions import SuspiciousOperation from django.core.files import temp as tempfile from django.core.urlresolvers import reverse # Register auth models with the admin. @@ -30,6 +29,7 @@ from django.db import connection from django.forms.util import ErrorList from django.template.response import TemplateResponse from django.test import TestCase +from django.test.utils import patch_logger from django.utils import formats, translation, unittest from django.utils.cache import get_max_age from django.utils.encoding import iri_to_uri, force_bytes @@ -543,20 +543,21 @@ class AdminViewBasicTest(TestCase): self.assertContains(response, '%Y-%m-%d %H:%M:%S') def test_disallowed_filtering(self): - self.assertRaises(SuspiciousOperation, - self.client.get, "/test_admin/admin/admin_views/album/?owner__email__startswith=fuzzy" - ) + with patch_logger('django.security.DisallowedModelAdminLookup', 'error') as calls: + response = self.client.get("/test_admin/admin/admin_views/album/?owner__email__startswith=fuzzy") + self.assertEqual(response.status_code, 400) + self.assertEqual(len(calls), 1) - try: - self.client.get("/test_admin/admin/admin_views/thing/?color__value__startswith=red") - self.client.get("/test_admin/admin/admin_views/thing/?color__value=red") - except SuspiciousOperation: - self.fail("Filters are allowed if explicitly included in list_filter") + # Filters are allowed if explicitly included in list_filter + response = self.client.get("/test_admin/admin/admin_views/thing/?color__value__startswith=red") + self.assertEqual(response.status_code, 200) + response = self.client.get("/test_admin/admin/admin_views/thing/?color__value=red") + self.assertEqual(response.status_code, 200) - try: - self.client.get("/test_admin/admin/admin_views/person/?age__gt=30") - except SuspiciousOperation: - self.fail("Filters should be allowed if they involve a local field without the need to whitelist them in list_filter or date_hierarchy.") + # Filters should be allowed if they involve a local field without the + # need to whitelist them in list_filter or date_hierarchy. + response = self.client.get("/test_admin/admin/admin_views/person/?age__gt=30") + self.assertEqual(response.status_code, 200) e1 = Employee.objects.create(name='Anonymous', gender=1, age=22, alive=True, code='123') e2 = Employee.objects.create(name='Visitor', gender=2, age=19, alive=True, code='124') @@ -574,10 +575,9 @@ class AdminViewBasicTest(TestCase): ForeignKey 'limit_choices_to' should be allowed, otherwise raw_id_fields can break. """ - try: - self.client.get("/test_admin/admin/admin_views/inquisition/?leader__name=Palin&leader__age=27") - except SuspiciousOperation: - self.fail("Filters should be allowed if they are defined on a ForeignKey pointing to this model") + # Filters should be allowed if they are defined on a ForeignKey pointing to this model + response = self.client.get("/test_admin/admin/admin_views/inquisition/?leader__name=Palin&leader__age=27") + self.assertEqual(response.status_code, 200) def test_hide_change_password(self): """ diff --git a/tests/handlers/tests.py b/tests/handlers/tests.py index 3680eecdd2..397b63647a 100644 --- a/tests/handlers/tests.py +++ b/tests/handlers/tests.py @@ -61,6 +61,7 @@ class TransactionsPerRequestTests(TransactionTestCase): connection.settings_dict['ATOMIC_REQUESTS'] = old_atomic_requests self.assertContains(response, 'False') + class SignalsTests(TestCase): urls = 'handlers.urls' @@ -89,3 +90,11 @@ class SignalsTests(TestCase): self.assertEqual(self.signals, ['started']) self.assertEqual(b''.join(response.streaming_content), b"streaming content") self.assertEqual(self.signals, ['started', 'finished']) + + +class HandlerSuspiciousOpsTest(TestCase): + urls = 'handlers.urls' + + def test_suspiciousop_in_view_returns_400(self): + response = self.client.get('/suspicious/') + self.assertEqual(response.status_code, 400) diff --git a/tests/handlers/urls.py b/tests/handlers/urls.py index 29858055ab..eaf764b00b 100644 --- a/tests/handlers/urls.py +++ b/tests/handlers/urls.py @@ -9,4 +9,5 @@ urlpatterns = patterns('', url(r'^streaming/$', views.streaming), url(r'^in_transaction/$', views.in_transaction), url(r'^not_in_transaction/$', views.not_in_transaction), + url(r'^suspicious/$', views.suspicious), ) diff --git a/tests/handlers/views.py b/tests/handlers/views.py index 9cc86ae6f3..1b75b27043 100644 --- a/tests/handlers/views.py +++ b/tests/handlers/views.py @@ -1,5 +1,6 @@ from __future__ import unicode_literals +from django.core.exceptions import SuspiciousOperation from django.db import connection, transaction from django.http import HttpResponse, StreamingHttpResponse @@ -15,3 +16,6 @@ def in_transaction(request): @transaction.non_atomic_requests def not_in_transaction(request): return HttpResponse(str(connection.in_atomic_block)) + +def suspicious(request): + raise SuspiciousOperation('dubious') diff --git a/tests/logging_tests/tests.py b/tests/logging_tests/tests.py index 8e5445cd42..0c2d269464 100644 --- a/tests/logging_tests/tests.py +++ b/tests/logging_tests/tests.py @@ -8,9 +8,10 @@ import warnings from django.conf import LazySettings from django.core import mail from django.test import TestCase, RequestFactory -from django.test.utils import override_settings +from django.test.utils import override_settings, patch_logger from django.utils.encoding import force_text -from django.utils.log import CallbackFilter, RequireDebugFalse, RequireDebugTrue +from django.utils.log import (CallbackFilter, RequireDebugFalse, + RequireDebugTrue) from django.utils.six import StringIO from django.utils.unittest import skipUnless @@ -354,3 +355,22 @@ class SettingsConfigureLogging(TestCase): settings.configure( LOGGING_CONFIG='logging_tests.tests.dictConfig') self.assertTrue(dictConfig.called) + + +class SecurityLoggerTest(TestCase): + + urls = 'logging_tests.urls' + + def test_suspicious_operation_creates_log_message(self): + with self.settings(DEBUG=True): + with patch_logger('django.security.SuspiciousOperation', 'error') as calls: + response = self.client.get('/suspicious/') + self.assertEqual(len(calls), 1) + self.assertEqual(calls[0], 'dubious') + + def test_suspicious_operation_uses_sublogger(self): + with self.settings(DEBUG=True): + with patch_logger('django.security.DisallowedHost', 'error') as calls: + response = self.client.get('/suspicious_spec/') + self.assertEqual(len(calls), 1) + self.assertEqual(calls[0], 'dubious') diff --git a/tests/logging_tests/urls.py b/tests/logging_tests/urls.py new file mode 100644 index 0000000000..c738bd565c --- /dev/null +++ b/tests/logging_tests/urls.py @@ -0,0 +1,10 @@ +from __future__ import unicode_literals + +from django.conf.urls import patterns, url + +from . import views + +urlpatterns = patterns('', + url(r'^suspicious/$', views.suspicious), + url(r'^suspicious_spec/$', views.suspicious_spec), +) diff --git a/tests/logging_tests/views.py b/tests/logging_tests/views.py new file mode 100644 index 0000000000..c685bcc005 --- /dev/null +++ b/tests/logging_tests/views.py @@ -0,0 +1,11 @@ +from __future__ import unicode_literals + +from django.core.exceptions import SuspiciousOperation, DisallowedHost + + +def suspicious(request): + raise SuspiciousOperation('dubious') + + +def suspicious_spec(request): + raise DisallowedHost('dubious') diff --git a/tests/test_client_regress/tests.py b/tests/test_client_regress/tests.py index 2582b210c4..6a58ef6344 100644 --- a/tests/test_client_regress/tests.py +++ b/tests/test_client_regress/tests.py @@ -7,7 +7,6 @@ from __future__ import unicode_literals import os from django.conf import settings -from django.core.exceptions import SuspiciousOperation from django.core.urlresolvers import reverse from django.template import (TemplateDoesNotExist, TemplateSyntaxError, Context, Template, loader) @@ -20,6 +19,7 @@ from django.utils._os import upath from django.utils.translation import ugettext_lazy from django.http import HttpResponse +from .views import CustomTestException @override_settings( TEMPLATE_DIRS=(os.path.join(os.path.dirname(upath(__file__)), 'templates'),) @@ -619,7 +619,7 @@ class ExceptionTests(TestCase): try: response = self.client.get("/test_client_regress/staff_only/") self.fail("General users should not be able to visit this page") - except SuspiciousOperation: + except CustomTestException: pass # At this point, an exception has been raised, and should be cleared. @@ -629,7 +629,7 @@ class ExceptionTests(TestCase): self.assertTrue(login, 'Could not log in') try: self.client.get("/test_client_regress/staff_only/") - except SuspiciousOperation: + except CustomTestException: self.fail("Staff should be able to visit this page") diff --git a/tests/test_client_regress/views.py b/tests/test_client_regress/views.py index 7e86ffd8ca..71e5b526e5 100644 --- a/tests/test_client_regress/views.py +++ b/tests/test_client_regress/views.py @@ -3,12 +3,15 @@ import json from django.conf import settings from django.contrib.auth.decorators import login_required from django.http import HttpResponse, HttpResponseRedirect -from django.core.exceptions import SuspiciousOperation from django.shortcuts import render_to_response from django.core.serializers.json import DjangoJSONEncoder from django.test.client import CONTENT_TYPE_RE from django.template import RequestContext + +class CustomTestException(Exception): + pass + def no_template_view(request): "A simple view that expects a GET request, and returns a rendered template" return HttpResponse("No template used. Sample content: twice once twice. Content ends.") @@ -18,7 +21,7 @@ def staff_only_view(request): if request.user.is_staff: return HttpResponse('') else: - raise SuspiciousOperation() + raise CustomTestException() def get_view(request): "A simple login protected view" diff --git a/tests/urlpatterns_reverse/tests.py b/tests/urlpatterns_reverse/tests.py index d5d9b3a709..f54c796d30 100644 --- a/tests/urlpatterns_reverse/tests.py +++ b/tests/urlpatterns_reverse/tests.py @@ -516,7 +516,7 @@ class RequestURLconfTests(TestCase): b''.join(self.client.get('/second_test/')) class ErrorHandlerResolutionTests(TestCase): - """Tests for handler404 and handler500""" + """Tests for handler400, handler404 and handler500""" def setUp(self): from django.core.urlresolvers import RegexURLResolver @@ -528,12 +528,14 @@ class ErrorHandlerResolutionTests(TestCase): def test_named_handlers(self): from .views import empty_view handler = (empty_view, {}) + self.assertEqual(self.resolver.resolve400(), handler) self.assertEqual(self.resolver.resolve404(), handler) self.assertEqual(self.resolver.resolve500(), handler) def test_callable_handers(self): from .views import empty_view handler = (empty_view, {}) + self.assertEqual(self.callable_resolver.resolve400(), handler) self.assertEqual(self.callable_resolver.resolve404(), handler) self.assertEqual(self.callable_resolver.resolve500(), handler) diff --git a/tests/urlpatterns_reverse/urls_error_handlers.py b/tests/urlpatterns_reverse/urls_error_handlers.py index be4f42afbc..7146fdf43c 100644 --- a/tests/urlpatterns_reverse/urls_error_handlers.py +++ b/tests/urlpatterns_reverse/urls_error_handlers.py @@ -4,5 +4,6 @@ from django.conf.urls import patterns urlpatterns = patterns('') +handler400 = 'urlpatterns_reverse.views.empty_view' handler404 = 'urlpatterns_reverse.views.empty_view' handler500 = 'urlpatterns_reverse.views.empty_view' diff --git a/tests/urlpatterns_reverse/urls_error_handlers_callables.py b/tests/urlpatterns_reverse/urls_error_handlers_callables.py index fe2d3137e9..befeccaf45 100644 --- a/tests/urlpatterns_reverse/urls_error_handlers_callables.py +++ b/tests/urlpatterns_reverse/urls_error_handlers_callables.py @@ -9,5 +9,6 @@ from .views import empty_view urlpatterns = patterns('') +handler400 = empty_view handler404 = empty_view handler500 = empty_view