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)

comment:1 by Carlton Gibson, 6 years ago

Type: UncategorizedNew feature
Version: 1.11master

There is no information about this in the Howto.

In the How-To see the Custom Error Reports section.

If you wish to override or customize this default behavior ... you need to define your own filter class ...

Something along these lines should do you:

class CustomExceptionReporterFilter(SafeExceptionReporterFilter):
    def get_traceback_data(self):
        c = super().get_traceback_data()
        if self.is_active(self.request) and 'request_COOKIES_items' in c:
            del c['request_COOKIES_items']
        return c

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.

comment:2 by Carlton Gibson, 6 years ago

Resolution: wontfix
Status: newclosed

in reply to:  1 comment:3 by Vasili Korol, 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.

Last edited 6 years ago by Vasili Korol (previous) (diff)

comment:4 by Carlton Gibson, 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.)

comment:5 by Carlton Gibson, 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

in reply to:  5 comment:6 by Vasili Korol, 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 Michael Manfre, 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    
Last edited 6 years ago by Michael Manfre (previous) (diff)

comment:8 by Carlton Gibson, 6 years ago

Resolution: wontfix
Status: closednew
Triage Stage: UnreviewedAccepted

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 Carlton Gibson, 6 years ago

Summary: Option to hide cookies in error reportsMake it easier to customise ExceptionReporter output.

comment:10 by Nasir Hussain, 5 years ago

Owner: set to Nasir Hussain
Status: newassigned

in reply to:  8 comment:11 by Nasir Hussain, 5 years ago

Replying to Carlton Gibson:
I've made PR. Going to work on test cases and documentation.

Last edited 5 years ago by Nasir Hussain (previous) (diff)

comment:12 by Carlton Gibson, 5 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 Carlton Gibson, 5 years ago

Owner: Nasir Hussain removed
Status: assignednew

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 Nasir Hussain, 5 years ago

Owner: set to Nasir Hussain
Status: newassigned

in reply to:  12 comment:15 by Nasir Hussain, 5 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 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:16 by Nasir Hussain, 5 years ago

Replying to Carlton Gibson:
I've made PR.

comment:17 by Carlton Gibson, 5 years ago

Has patch: set
Needs documentation: set
Patch needs improvement: set

in reply to:  17 comment:18 by Nasir Hussain, 5 years ago

Replying to Carlton Gibson: Hi, Is this going to be in Django 3.0 alpha; feature freeze?

comment:19 by Carlton Gibson, 5 years ago

Triage Stage: AcceptedReady 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.)

comment:20 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 25706d72:

Fixed #29714 -- Allowed using ExceptionReporter subclass with AdminEmailHandler.

Note: See TracTickets for help on using tickets.
Back to Top