Opened 3 years ago
Last modified 12 months ago
#33090 assigned New feature
Extend sensitive post parameter filtering to be applicable to exceptions in middleware.
Reported by: | Carlton Gibson | Owned by: | Oluwayemisi Ismail |
---|---|---|---|
Component: | Error reporting | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
With the current implementation of the @sensitive_post_parameters
decorator, the request is not marked until the view is executed. This means that the filtering cannot be applied to reports generated by exceptions in the middleware.
Filtering is always best-effort, and all the usual caveats apply but discussion by the Django Security Team suggests that it would be feasible mark the request before processing the middleware, thus allowing the filtering in error reports even for middleware exceptions.
The first step would be to adjust sensitive_post_parameters
to mark the view callback:
diff --git a/django/views/decorators/debug.py b/django/views/decorators/debug.py index 312269baba..faa6eeb107 100644 --- a/django/views/decorators/debug.py +++ b/django/views/decorators/debug.py @@ -88,5 +88,7 @@ def sensitive_post_parameters(*parameters): else: request.sensitive_post_parameters = '__ALL__' return view(request, *args, **kwargs) + # Mark the wrapped view itself in case of middleware errors. + sensitive_post_parameters_wrapper.sensitive_post_parameters = parameters or '__ALL__' return sensitive_post_parameters_wrapper return decorator
And then have the request marked prior to processing the middleware:
diff --git a/django/core/handlers/base.py b/django/core/handlers/base.py index 728e449703..260200d5d7 100644 --- a/django/core/handlers/base.py +++ b/django/core/handlers/base.py @@ -218,6 +218,10 @@ class BaseHandler: response = None callback, callback_args, callback_kwargs = self.resolve_request(request) + # Mark the request with sensitive_post_parameters if applied. + if hasattr(callback, 'sensitive_post_parameters'): + request.sensitive_post_parameters = callback.sensitive_post_parameters + # Apply view middleware. for middleware_method in self._view_middleware: response = await middleware_method(request, callback, callback_args, callback_kwargs)
For this last, similar would be required for the async pathway.
Then it would require tests and ancillary cleanup.
Attachments (1)
Change History (13)
comment:1 by , 3 years ago
Description: | modified (diff) |
---|
comment:2 by , 3 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Version: | 3.2 → dev |
comment:3 by , 3 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:5 by , 2 years ago
I have also been running into this issue, in my case because I had AdminLogHandler configured for WARNING instead of the default ERROR, and then triggered a CSRF failure on a login form (which got e-mailed containing the password). It was only after investigating the issue in detail, that I understood the scope of it and found this issue report :-) While investigating this, I was thinking about exactly the solution proposed by Carlton, so that has my vote.
Project to reproduce
Because I already spent a bit of time making a project to reproduce this issue, I'll share that here for reference. I'll attach the full project after this comment.
To build the attached project, I made a new project and app, and then:
- Add contrib.auth views per instructions, using the provided
login.html
and a simplebase.html
. - Removed
{% csrf_token %}
fromlogin.html
to force CSRF failure - Added two flavors of failing middleware:
DEBUG = False ALLOWED_HOSTS = ['localhost'] ADMINS = [('foo', 'matthijs@stdin.nl')] EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend' if False: # This raises teh mail_admins level to WARNING. It's a hack to modify # DEFAULT_LOGGING, but this is more consise than providing a full # replacement logging config and has the same effect. from django.utils.log import DEFAULT_LOGGING DEFAULT_LOGGING['handlers']['mail_admins']['level'] = "WARNING" elif True: MIDDLEWARE.insert(0, 'testproject.middleware.FailPostViewMiddleware') else: MIDDLEWARE.insert(0, 'testproject.middleware.FailPostMiddleware')
- Added some config to disable
DEBUG
and setADMINS
(otherwise no reporting is done), setALLOWED_HOSTS
(required withDEBUG=False
) and then either raise theAdminLogHandler
level toWARNING
or enable the failing middleware:DEBUG = False ALLOWED_HOSTS = ['localhost'] ADMINS = [('foo', 'matthijs@stdin.nl')] EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend' if False: # This raises teh mail_admins level to WARNING. It's a hack to modify # DEFAULT_LOGGING, but this is more consise than providing a full # replacement logging config and has the same effect. from django.utils.log import DEFAULT_LOGGING DEFAULT_LOGGING['handlers']['mail_admins']['level'] = "WARNING" elif True: MIDDLEWARE.insert(0, 'testproject.middleware.FailPostViewMiddleware') else: MIDDLEWARE.append('testproject.middleware.FailPostMiddleware')
Note that the middleware is inserted first to prevent the CSRF error from triggering first, but without the CSRF error this would also happen with the middleware at the end.
With this, navigating to http://localhost:8000/accounts/login/
prints an error email to stdout that includes the password.
Fix
I also manually applied the fix from Carlton to my django copy and it indeed seems to work as intended.
Note that it does *not* work for my FailPostMiddleware
since that middleware runs even before the code added by Carlton, but that is really unfixable, since at that point the view to be rendered is not known yet, and the middleware might even change the view, so no way to know what to filter. Two approaches to fix this anyway that I can think of would be to 1) simple hide *all* post parameters in this case (which might hinder debugging), or 2) submit a list of sensitive parameters as part of the form (which needs cooperation of the template).
Wrt to the implementation:
- I wonder if it is clean to let BaseHandler have a special case for copying this variable (though I cannot think of a more abstracted way to get the same thing done, except for storing the entire view callback as part of the request (so the error report generator can access that directly). Actually, it seems that the callback is already stored as
request.resolver_match[0]
, so this could be implemented without changingBaseHandler
, but instead modifyingSafeExceptionReporterFilter
to look atrequest.sensitive_post_parameters
*and*request.resolver_match[0].sensitive_post_parameters
. I did not test this, since there's a couple of places that need to change and I'm out of time). - This fix only works if the
sensitive_post_parameters
decorator is the outer decorator. The documentation forsensitive_variables
already specify it should be the outer one, but I guesssensitive_post_parameters
should then also document this (though I thinksensitive_variables
still must be the outer one, to prevent leaking function parameters in the wrapper generated bysensitive_post_parameters
, so that means thatsensitive_variables
must take care to copy thesensitive_post_parameters
from the callback it wraps (so you can havesensitive_post_parameters
insidesensitive_variables
and still have it work). - The fix is applied to
BaseHandler._get_response()
, but I think it should be applied to_get_response_async()
as well (but this is resolved if my suggestion under 1. is implemented). - Danielle asked:
Is it possible that this extension may need to be applied to the "sensitive_variables" decorator too?
sensitive_variables
wrapper view itself raises an exception before it can set the request property, but that wrapper is so simple that that does not seem possible).
Related report
I think that https://code.djangoproject.com/ticket/28215 might be related, maybe even the same issue (though the issue description seems to be mostly defined in terms of a unit test, which I have not looked at closely enough to really understand it). Maybe the testcase proposed in there might be useful.
comment:6 by , 2 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I'm really sorry but I haven't been able to touch this ticket for months due to some life matters (to the point I even forgot this ticket existed). Thus I'm de-assigning this ticket and allow anyone who wants to take it.
Thank you kindly for your patience.
comment:8 by , 21 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:9 by , 17 months ago
Has patch: | set |
---|---|
Owner: | changed from | to
comment:10 by , 17 months ago
Owner: | changed from | to
---|
comment:12 by , 12 months ago
My current workaround, because I ran into this and cannot face my users without fixing it today:
in settings.py:
MIDDLEWARE = [ ..., "my.package.middleware.ensurePasswordIsntInErrorMailMiddleware", "django.middleware.csrf.CsrfViewMiddleware", ... ]
in my/package/middleware.py:
def ensurePasswordIsntInErrorMailMiddleware(get_response): def middleware(request): request.sensitive_post_parameters = ["password"] return get_response(request) return middleware
Basically setting it as a default, in case we don't get to the next point where it would be set.
The django code always runs:
request.sensitive_post_parameters = parameters
So the setting from this middleware will be overridden the next time the decorator is hit.
Greetings everyone,
Is it possible that this extension may need to be applied to the "sensitive_variables" decorator too?
Thanks in advance.