Extend sensitive post parameter filtering to be applicable to exceptions in middleware.

[Carlton Gibson]

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.

[Daniele Penazzo]

Greetings everyone,
I have the suspicion that this extension may need to be applied to the "sensitive_variables" decorator too, shall I include it in the patch I'm preparing?

Thanks in advance.

[Matthijs Kooijman]

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 simple base.html.
• Removed {% csrf_token %} from login.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 set ADMINS (otherwise no reporting is done), set ALLOWED_HOSTS (required with DEBUG=False) and then either raise the AdminLogHandler level to WARNING 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:

1. 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 changing BaseHandler, but instead modifying SafeExceptionReporterFilter to look at request.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).
2. This fix only works if the sensitive_post_parameters decorator is the outer decorator. The documentation for sensitive_variables already specify it should be the outer one, but I guess sensitive_post_parameters should then also document this (though I think sensitive_variables still must be the outer one, to prevent leaking function parameters in the wrapper generated by sensitive_post_parameters, so that means that sensitive_variables must take care to copy the sensitive_post_parameters from the callback it wraps (so you can have sensitive_post_parameters inside sensitive_variables and still have it work).
3. 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).
4. Danielle asked:

   Is it possible that this extension may need to be applied to the "sensitive_variables" decorator too?
   

   But I believe this is not necessary, because those variables will not come into existence before the view is called, so no need to hide them before that (there is one exception: If the 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.

[Matthijs Kooijman]

Project to reproduce the problem

[Daniele Penazzo]

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.

[Carlton Gibson]

Thanks Daniele, no problem. Take care.

[Mariusz Felisiak]

PR is marked as a draft.

[Simon Kohlmeyer]

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.