[Django] #29714: Option to hide cookies in error reports
Django
-------------------------------------+-------------------------------------
Reporter: Vasili | Owner: (none)
Korol |
Type: | Status: new
Uncategorized |
Component: Error | Version: 1.11
reporting | Keywords: email, reports,
Severity: Normal | cookies
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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
[https://docs.djangoproject.com/en/2.1/howto/error-reporting/ 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29714>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage:
cookies | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* version: 1.11 => master
* type: Uncategorized => New feature
Comment:
> There is no information about this in the
[https://docs.djangoproject.com/en/2.1/howto/error-reporting/ Howto].
In the How-To see the [https://docs.djangoproject.com/en/2.1/howto/error-
reporting/#custom-error-reports 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:1>
Django
-------------------------------------+-------------------------------------
Component: Error reporting | Version: master
Keywords: email, reports, | Triage Stage:
cookies | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:2>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Error reporting | Version: master
Severity: Normal | Resolution: wontfix
Keywords: email, reports, | Triage Stage:
cookies | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Vasili Korol):
Replying to [comment:1 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 Processing Regulation (GDPR), which was introduced lately. It may
make sense to update the Howto on this matter.
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:3>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Error reporting | Version: master
Severity: Normal | Resolution: wontfix
Keywords: email, reports, | Triage Stage:
cookies | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hi Vasili. Yes, maybe. If you wanted to raise it on
[https://groups.google.com/forum/#!forum/django-developers 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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:4>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Error reporting | Version: master
Severity: Normal | Resolution: wontfix
Keywords: email, reports, | Triage Stage:
cookies | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:5>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Error reporting | Version: master
Severity: Normal | Resolution: wontfix
Keywords: email, reports, | Triage Stage:
cookies | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Vasili Korol):
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...
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:6>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Error reporting | Version: master
Severity: Normal | Resolution: wontfix
Keywords: email, reports, | Triage Stage:
cookies | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Michael Manfre):
{{{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.
{{{
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:7>
Django
-------------------------------------+-------------------------------------
Component: Error reporting | Version: master
Severity: Normal | Resolution:
cookies |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: wontfix =>
* stage: Unreviewed => Accepted
Comment:
Yep. My mistake. The `SafeExceptionReporterFilter` doesn't do what I read
it to. This looks like we could/should do something here. Reopening.
Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: (none)
Type: New feature | Status: new
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:9>
Django
-------------------------------------+-------------------------------------
| Hussain
Type: New feature | Status: assigned
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Nasir Hussain
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: Nasir
| Hussain
Type: New feature | Status: assigned
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Nasir Hussain):
Replying to [comment:8 Carlton Gibson]:
I've made [https://github.com/django/django/pull/10886 PR]. Going to work
on test cases.
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: Nasir
| Hussain
Type: New feature | Status: assigned
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
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
[https://code.djangoproject.com/query?status=!closed&component=Error+reporting
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.
([https://code.djangoproject.com/ticket/25167#comment:1 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:12>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Nasir Hussain => (none)
* status: assigned => new
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:13>
Django
-------------------------------------+-------------------------------------
| Hussain
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: (none) => Nasir Hussain
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: Nasir
| Hussain
Type: New feature | Status: assigned
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Nasir Hussain):
Replying to [comment:12 Carlton Gibson]:
Hi [comment:12 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
[https://code.djangoproject.com/query?status=!closed&component=Error+reporting
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.
([https://code.djangoproject.com/ticket/25167#comment:1 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: Nasir
| Hussain
Type: New feature | Status: assigned
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Nasir Hussain):
Replying to [comment:8 Carlton Gibson]:
I've made [https://github.com/django/django/pull/11021 PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: Nasir
| Hussain
Type: New feature | Status: assigned
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: Nasir
| Hussain
Type: New feature | Status: assigned
Component: Error reporting | Version: master
Severity: Normal | Resolution:
Keywords: email, reports, | Triage Stage: Accepted
cookies |
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Nasir Hussain):
Replying to [comment:17 Carlton Gibson]: Hi, Is this going to be in Django
3.0 alpha; feature freeze?
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: Nasir
| Hussain
Type: New feature | Status: assigned
Component: Error reporting | Version: master
Severity: Normal | Resolution:
cookies | checkin
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/11021 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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Vasili Korol | Owner: Nasir
| Hussain
Component: Error reporting | Version: master
Keywords: email, reports, | Triage Stage: Ready for
cookies | checkin
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"25706d728536fea88e12f62856de7c4b569ae282" 25706d72]:
{{{
#!CommitTicketReference repository=""
revision="25706d728536fea88e12f62856de7c4b569ae282"
Fixed #29714 -- Allowed using ExceptionReporter subclass with
AdminEmailHandler.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29714#comment:20>