[Django] #33090: Extend sensitive post parameter filtering to be applicable to exceptions in middleware.
Django
middleware.
-------------------------------------------+------------------------
Reporter: Carlton Gibson | Owner: (none)
Type: New feature | Status: new
Component: Error reporting | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
With the current implement 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
[https://docs.djangoproject.com/en/3.2/howto/error-reporting/#filtering-
error-reports 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33090>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
middleware.
Reporter: Carlton Gibson | Owner: (none)
Type: New feature | Status: new
Component: Error reporting | Version: 3.2
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Carlton Gibson:
Old description:
New description:
With the current implementation of the `@sensitive_post_parameters`
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:1>
Django
middleware.
Reporter: Carlton Gibson | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* version: 3.2 => dev
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:2>
Django
middleware.
Reporter: Carlton Gibson | Owner: Penaz
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Penaz):
* owner: (none) => Penaz
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:3>
Django
middleware.
Reporter: Carlton Gibson | Owner: Daniele
| Penazzo
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:4>
Django
middleware.
Reporter: Carlton Gibson | Owner: Daniele
| Penazzo
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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
[https://docs.djangoproject.com/en/dev/topics/auth/default/#module-
django.contrib.auth.views 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', 'matt...@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', 'matt...@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:5>
Django
middleware.
Reporter: Carlton Gibson | Owner: Daniele
| Penazzo
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Matthijs Kooijman):
* Attachment "testproject.zip" added.
Project to reproduce the problem
Django
middleware.
Reporter: Carlton Gibson | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Daniele Penazzo):
* owner: Daniele Penazzo => (none)
* status: assigned => new
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:6>
Django
middleware.
Reporter: Carlton Gibson | Owner: (none)
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
Thanks Daniele, no problem. Take care.
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:7>
Django
middleware.
Reporter: Carlton Gibson | Owner: rajdesai24
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by rajdesai24):
* owner: (none) => rajdesai24
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:8>
Django
middleware.
Reporter: Carlton Gibson | Owner:
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sarah Boyce):
* owner: rajdesai24 => Oluwayemisi+Ismail
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:9>
Django
middleware.
Reporter: Carlton Gibson | Owner:
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sarah Boyce):
* owner: Oluwayemisi+Ismail => Oluwayemisi Ismail
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:10>
Django
middleware.
Reporter: Carlton Gibson | Owner:
| Oluwayemisi Ismail
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
Comment:
PR is marked as a draft.
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:11>
Django
middleware.
Reporter: Carlton Gibson | Owner:
| Oluwayemisi Ismail
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by voidus):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:12>
Django
middleware.
Reporter: Carlton Gibson | Owner:
| Oluwayemisi Ismail
Type: New feature | Status: assigned
Component: Error reporting | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Comment (by SnejPro):
Replying to [comment:12 Simon Kohlmeyer]:
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.
with cleartext passwords of my users.
I think this shoud be implemented per default.
I've noticed that 5.2.6 adresses a similar issue:
https://github.com/django/django/commit/037e740ec56674e69e564b97e1151950757c410d
But updating to 5.2.6 does not solve this issue.
--
Ticket URL: <https://code.djangoproject.com/ticket/33090#comment:13>