Opened 6 years ago
Closed 5 years ago
#29714 closed New feature (fixed)
Make it easier to customise ExceptionReporter output.
Reported by: | Vasili Korol | Owned by: | Nasir Hussain |
---|---|---|---|
Component: | Error reporting | Version: | dev |
Severity: | Normal | Keywords: | email, reports, cookies |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
When DEBUG=False
, and with the email settings configured, Django sends error reports to the addresses listed in the ADMINS
setting. One can filter some sensitive data out from the report, but there is no option to filter out cookies. There is no information about this in the Howto.
This may be important in the situations, when cookies scoped to the parent domain are included in the HTTP request - in this case such cookies will be dumped in the report, although they may be not related to the Django website at all.
Change History (20)
follow-up: 3 comment:1 by , 6 years ago
Type: | Uncategorized → New feature |
---|---|
Version: | 1.11 → master |
comment:2 by , 6 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
comment:3 by , 6 years ago
Replying to Carlton Gibson
Okay, thanks, this is clear. I just thought that this can be sort of a general problem for other people, too, because of the European General Data Protection Regulation (GDPR), which was introduced lately. It may make sense to update the Howto on this matter.
comment:4 by , 6 years ago
Hi Vasili. Yes, maybe. If you wanted to raise it on django-developers to draw opinions thay may be worth it. (Happy to re-open if there’s any kind of concensus on specific additons/changes we could/should make.)
follow-up: 6 comment:5 by , 6 years ago
For reference, a discussion on GDPR was already opened, but so-far has little follow-up: https://groups.google.com/d/topic/django-developers/Xhg-0JeDN50/discussion
comment:6 by , 6 years ago
I created a post there, thanks.
BTW, after looking at the source code, i can see that SafeExceptionReporterFilter
cannot provide the needed functionality. It's the ExceptionReporter
class that implements get_traceback_data
, but this class cannot be overridden...
comment:7 by , 6 years ago
ExceptionReporter
is used by django.utils.log.AdminEmailHandler
. To fully replace ExceptionReporter
,
it's necessary to replace the "mail_admins" LOGGING handler to use a different one that uses the reporter you define. The LOGGING docs explain this with more detail.
LOGGING = {..., 'handlers': { ... 'mail_admins': { 'level': 'ERROR', 'filters': ['require_debug_false'], 'class': 'myproject.log.MySafeAdminEmailHandler' } }}
The easiest way is to override AdminEmailHandler
would be to override emit()
with a copy that replaces
ExceptionReporter
. That's a lot of copy & pasted code to swap out a class used in a single line. I feel we should
make that part of the process a bit friendlier. Maybe ending up so Vasili and others could do the below:
class MySafeAdminEmailHandler(AdminEmailHandler): def __init__(self): super().__init__(exception_reporter=SafeExceptionReporter)
OR
class MySafeAdminEmailHandler(AdminEmailHandler): exception_reporter=SafeExceptionReporter
follow-up: 11 comment:8 by , 6 years ago
Resolution: | wontfix |
---|---|
Status: | closed → new |
Triage Stage: | Unreviewed → Accepted |
Yep. My mistake. The SafeExceptionReporterFilter
doesn't do what I read it to. This looks like we could/should do something here. Reopening. Thanks!
comment:9 by , 6 years ago
Summary: | Option to hide cookies in error reports → Make it easier to customise ExceptionReporter output. |
---|
comment:10 by , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:11 by , 6 years ago
Replying to Carlton Gibson:
I've made PR. Going to work on test cases.
follow-up: 15 comment:12 by , 6 years ago
PR was adjusting SafeExceptionReporterFilter
to filter cookies, rather than making ExceptionReporter
easier to customise.
As I commented on the PR I think that's the wrong approach because:
... there are a number of very similar requests to customise the error reporter output. (Not ALL of those, but several...) — If we added API to
SafeExceptionReporterFilter
for each of them, it'll become out of hand, so it's likely we need to favour a different approach. Whatever solution we take, I think we need to consider all the tickets here. (There's thoughts of deprecating `SafeExceptionReporterFilter` as the extension point entirely...)
Whether we deprecate SafeExceptionReporterFilter
or not, I think making ExceptionReporter
easier to customise would be a win (and small in footprint). From there we can look at whether we need extra API to help e.g. filtering cookies, and so on.
comment:13 by , 6 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
I'm going to deassign you Nasir (since I closed your PR). If you want to pick it up again please do. Either way, thank you for your effort!
comment:14 by , 6 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:15 by , 6 years ago
Replying to Carlton Gibson:
Hi Carlton Gibson: thanks for suggestions. I would like to create another PR.
PR was adjusting
SafeExceptionReporterFilter
to filter cookies, rather than makingExceptionReporter
easier to customise.
As I commented on the PR I think that's the wrong approach because:
... there are a number of very similar requests to customise the error reporter output. (Not ALL of those, but several...) — If we added API to
SafeExceptionReporterFilter
for each of them, it'll become out of hand, so it's likely we need to favour a different approach. Whatever solution we take, I think we need to consider all the tickets here. (There's thoughts of deprecating `SafeExceptionReporterFilter` as the extension point entirely...)
Whether we deprecate
SafeExceptionReporterFilter
or not, I think makingExceptionReporter
easier to customise would be a win (and small in footprint). From there we can look at whether we need extra API to help e.g. filtering cookies, and so on.
follow-up: 18 comment:17 by , 5 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
Patch needs improvement: | set |
comment:18 by , 5 years ago
Replying to Carlton Gibson: Hi, Is this going to be in Django 3.0 alpha; feature freeze?
comment:19 by , 5 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
PR #11021 allows using an ExceptionReporter subclass with AdminEmailHandler.
That's half the job. The other bit is making the reporter class pluggable for the 500 debug error view. I think we should address that separately. (Being DEBUG only it's not quite so pressing.)
In the How-To see the Custom Error Reports section.
Something along these lines should do you:
This seems neat enough to me.
You can always ask on django-developers but, I suspect there'd be no appetite for changing the default filtering behaviour. As such I can't really see a cleaner API being available (or worth the effort). Given that I'm going to close this as
wontfix
.Let me know if I've missed something.