Dont send Request message when handle_uncaught_exception()

[Oguz Aylanc]

When 500 error happens django sends request message , then you get personel private post parameters like password, credit cards info or etc.We dont need and we must not see them if we are running public web site.

Need to set it in the settings and parameter could be "HANDLING_EXCEPTION_REQUEST_REPR" or something else.

[Russell Keith-Magee]

No, you need to run your server with DEBUG=False. When DEBUG=False, Django's debug 500 template isn't used, and the details you describe aren't exposed.

[Oguz Aylanc]

Replying to russellm:

No, you need to run your server with DEBUG=False. When DEBUG=False, Django's debug 500 template isn't used, and the details you describe aren't exposed.

I dont mean 500 template, I mean "ADMINS" error email.All admins get REQUEST and also have POST parameters.

logger.error('Internal Server Error: %s' % request.path,

exc_info=exc_info,
extra={

'status_code': 500,
'request':request

}

)

Am I wrong?

[Russell Keith-Magee]

Ok - now I understand what you're saying.

For the record, the data you're referring to isn't scrubbed out of the 500 error page either -- POST and GET are always displayed verbatim on the 500 page (both when displayed literally as the list of GET/POST data, or as part of a stack trace). On top of that, the list of protected settings on the 500 page isn't configurable.

If we're going to fix this, it should be fixed consistently, and applied to both the 500 page and 500 emails. That means a setting, a 'safe_repr' function/template filter that can be used to wrap the output of any request object, and utilities to obscure POST and GET arguments. cleanse_setting in django.views.debug is a partial guide to what is required here.

As a workaround in the meantime -- in Django trunk (which will become 1.3), you can install your own error email hander, so if you're uncomfortable with certain data going out, you can install your own error handler that puts whatever content you want into error emails.

[aakok]

I think 500 pages that rendered when debug = True must not hide any POST or GET data from user because simply the person who sees them actually the one puts them there in the first place. But it comes to emails it leads to some security issues. On the other hand those POST and GET information can be really helpful for reproducing the errors. So this option should be applicable with options per URL.

As an example following would be more explanatory:

SAFE_500_URLS = ((r'^{payment/$', {'POST' : ('credit_card', 'cvv', )}),}

(r'^{login/$', {'GET' : ('password1', 'password2', 'username')}),}

)

Which must interpreted as obscure from POST QueryDict, values 'credit_card' and 'cvv' where request.POST.get('REQUEST_URL', ) matches the r'^payment/$'.

[Julien Phalip]

I've actually gotten bitten by this recently when I embarrassingly received error emails containing a client of mine's password displayed in clear because the admin login view had encountered an unhandled exception.

I've just posted a patch with a proof of concept to obfuscate specified GET/POST parameters when unhandled exceptions are logged. It treats the issue at its core (i.e. HttpRequest) and takes the form of a view decorator in order to stay close to the action and to ease maintenance.

The nomenclature is inspired from HIDDEN_SETTINGS and get_safe_settings in the django.views.debug module. The obfuscation is only active when DEBUG is False as, like it's been pointed above, it would be useful to have access to every possible information while debugging.

So, I'd really like to get some feedback on this proof of concept, in particular:

• What do you think of the API? Should there be different or complementary ways to this decorator (e.g. for class-based views)?
• What do you think of the internals, in particular how the HttpRequest object has been modified? Can you think of anything cleaner? Should the obfuscation occur inside or outside HttpRequest?
• Should the built-in auth/admin views make use of this by default (I think they should, as done in my patch, or at least there should be an easy way of activating it for all those views).
• What do you think of the nomenclature?

Many thanks.

[Russell Keith-Magee]

Regarding API -- decorating a view seems like a natural approach to me.

However, my immediate reaction from an initial look at the patch is that it seems *way* too complicated. I'm not sure I agree that this should be attached to the request -- it's purely a debugging operation.

I agree that the login etc views should adopt this behavior.

Also - as a design consideration, this goes much deeper than GET and POST. Consider, the full stack trace contains all the variables in each stack frame, any one of which could be a password, a credit card number, or so on. There could also be passwords in a dictionary in a dictionary in a variable in the stack frame. So - you might also want to consider adding the nuclear option -- turn off *all* GET/POST/stack/META variables, as a guaranteed safety.

[Julien Phalip]

Thanks Russell for the feedback. I've submitted another patch with a different, more radical, approach. The logic is now done solely in the debugging tools. If a view and its request are marked as "sensitive", and if DEBUG is False, then the request info and the frame variables will systematically get omitted from the error reports.

It is still possible to change this logic by overriding the ExceptionReporter.display_sensitive_info() method and by providing a custom logging tool (in lieu of the default AdminEmailHandler).

This guarantees safety but the tradeoff is that those views are a bit harder to debug in production. Maybe there is a sweet spot that could be found somewhere between flexibility and safety.

Let me know if you've got any feedback on this other approach. I'll ponder some more and perhaps bring this to the dev-list.

[Julien Phalip]

Moved the @sensitive decorator to django.views.decorators.security and marked auth admin views as sensitive

[Julien Phalip]

Working on a new patch based on the feedback received in ​http://groups.google.com/group/django-developers/browse_frm/thread/4ccae6edc269c88a

[Julien Phalip]

This new patch introduces new ExceptionReporterFilter classes with functionality allowing for fine-grained control over how sensitive frame variables and POST parameters are handled in error reports.

A few notes:

• The auth views use the non-discriminatory style of the @sensitive_post_parameters decorator (i.e. without specified parameters) because the forms can be customized and thus we can't predict how the custom form fields will be named.
• I'm not sure in which module(s) everything should go yet (views, debug, security, logging, etc.). It would be easy to change and move things around. Let me know if you've got suggestions.
• In the tests I didn't simulate multi-level traceback frames (i.e. I put everything into a view instead of breaking it down into multiple callbacks) because of an apparent limitation of the traceback returned by sys.exc_info(). When running the tests, it only returns the frame where the exception was raised, and the previous frames are simply omitted. It works fine when running real code though, as it's processed through the logging library. Any suggestion to improve the tests here would be welcome.

[Luke Plant]

Review:

I think this is pretty good, with the following comments:

1) I think I would prefer it if ExceptionReporterFilterBase were a true strategy object that didn't store any state i.e. the request. (This would make it like PasswordResetTokenGenerator). To me, this is an improvement because:

• It's better to avoid state as a general principle, if practical.
• we can construct and re-use a single default exception reporter filter object (as with PasswordResetTokenGenerator). This removes the need to cache the constructed filter object on the request object. (get_exception_reporter_filter would still do a getattr to cater for custom filter objects, but wouldn't set the attribute).
• it avoids reference cycles that make garbage collection harder.

If there were more parameters being passed round every method, there would be more of a case for storing them on self, but with just request I think the trade-off is in favour of passing the request to each method here.

2) ExceptionReporterFilterBase.show_traceback_frame calls show_traceback_frames(), and will call it once for every frame, which seems redundant, since show_traceback_frame only gets called in the first place if show_traceback_frames() returns True.

3) There is a similar issue with get_filtered_traceback_variables (indirectly) calling show_traceback_frame, which is guaranteed to return True if get_filtered_traceback_variables is ever called. Actually, there is a bug in the way it calls it since it doesn't pass tb_frame, although the bug would never be manifested precisely because it is redundant code (unless show_traceback_variables were overridden). Redundant code is untested code! I think it would be better to remove show_traceback_variables from ExceptionReporterFilterBase altogether, or mark it semi-private, since it is not part of the externally used API, and it's not necessary.

4) The 'decorators' are either decorators or decorator factories, depending on the type of the parameters, I presume to avoid the ugliness of something like:

@sensitive_variables()
def my_function():
    pass

or

@sensitive_variables([])
def my_function():
    pass

My experience has shown that this is a bad thing to do! Look up the cache_page decorator for an example of the kind of backward compatibility nightmares you get into with that approach. It also just makes the docs and current implementation longer, even without future considerations. In fact, I don't think it really helps developers that much, because you have to remember that it has been implemented with special cases, rather than simply following the rules. "Special cases aren't special enough to break the rules."

I'd be happy with it accepting no arguments to mean 'everything is sensitive' i.e. my first example above as a shortcut for the second. But not the version where you pass in the callable directly - is should be a decorator or decorator generator, not both.

5) I'm happy for the code to live in all the modules you've put them in.

6) I haven't actually reviewed the tests in any detail, they look fine in general.

7) Obviously, it needs some docs. I wasn't expecting them before the API was firmed up, but with your latest patch I think it is now. I haven't worked out where the docs should go, feel free to add a new topic/ref section if there is nothing suitable.

[Luke Plant]

Actually, I think the decorators should probably go in django/views/decorators/debug.py rather than security.py, because we've got other decorators that are more directly about security which are *not* in that module, and the parallels with the contents of django/views/ suggests looking in views/decorators/debug.py for corresponding decorators.

[Julien Phalip]

Thanks, Luke!

I've made a few changes in the latest patch:

• Simplified the API by removing all the show_XXXX() methods (so that the filter focuses solely on filtering business) and by renaming a few other methods.
• The decorators aren't decorator factories any more.
• Added documentation.

I've got a dilemma with the get_request_repr() method. Django ships with 3 request classes (WSGIRequest, ModPythonRequest and HttpRequest) which all define their own __repr__() method. To avoid duplicating all that code I've resorted to using regular expressions to insert the filtered POST dictionary's representation. This seems a bit flaky though. Let me know if you've got ideas on how to handle this better.

Also, what do you think about hiding *all* POST parameters for sensitive auth views? With this patch, all parameters are hidden (e.g. 'password', 'next', 'username' for the login view). The problem is that the forms can be customized and thus we can't predict what the names of the actual sensitive parameters (i.e. 'password' for the login view) will be. Another approach would be that if you provide a customized form then it is your responsibility to indicate what the actual sensitive parameters are, if they are different from the default ones.

[Luke Plant]

Regarding get_request_repr - I think your previous method was better, because it is more robust in the case of __repr__ on the other objects being altered, or an entirely different Request object being supplied. I wouldn't worry about the small amount of code duplication, we've got custom requirements so we are doing a custom thing.

Regarding hiding all POST parameters for login - I think that's fine, as you say we can't anticipate what parameters might be provided, or whether they'll be sensitive or not.

A few comments on the latest patch:

• I'd promote SafeExceptionReporterFilter._is_active to is_active since the docstring says it is meant to be overridden.
• The global 'default_exception_reporter_filter' should cache the instance, not the class.
• Similarly, the 'exception_reporter_filter' attribute should be assumed to be an instance, not a class, and not be called.
  • And this feature should be documented in the 'Custom error reports' section.
• In the docs you say "Your custom filter class needs to inherit from django.views.debug.ExceptionReporterFilter - perhaps you should suggest inheriting from SafeExceptionReporterFilter instead, but that it must satisfy the interface of ExceptionReporterFilter. Otherwise @sensitive_* are going to stop working if they don't reproduce the behaviour of SafeExceptionReporterFilter.

[Julien Phalip]

Thank you, Luke. It all makes sense and I'll make the amendments. Regarding get_request_repr(), the reason I've changed it is because I realised that WSGIRequest and ModPythonRequest had longer and quite different __repr__() methods than in HttpRequest.

ModPythonRequest.__repr__() shows the request path. Also both WSGIRequest.__repr__() and ModPythonRequest.__repr__() use the _post_parse_error attribute. Apparently that attribute was added in [8748] and then most of the associated logic was moved around in [14394].

While we're at it, maybe all of those methods should be normalised and consolidated into the HttpRequest class to avoid repetitions:

class HttpRequest(object):

    def __repr__(self):
        # Since this is called as part of error handling, we need to be very
        # robust against potentially malformed input.
        try:
            get = pformat(self.GET)
        except:
            get = '<could not parse>'
        if self._post_parse_error:
            post = '<could not parse>'
        else:
            try:
                post = pformat(self.POST)
            except:
                post = '<could not parse>'
        try:
            cookies = pformat(self.COOKIES)
        except:
            cookies = '<could not parse>'
        try:
            meta = pformat(self.META)
        except:
            meta = '<could not parse>'
        return smart_str(u'<%s\npath:%s,\nGET:%s,\nPOST:%s,\nCOOKIES:%s,\nMETA:%s>' %
                         (self.__class__.__name__, # So that it works with child classes too.
                          self.path,
                          unicode(get),
                          unicode(post),
                          unicode(cookies),
                          unicode(meta)))

[Luke Plant]

Hmm, good idea. Perhaps even add another layer - a build_request_repr (or something) function that explicitly takes the parameters GET, POST etc. HttpRequest.__repr__ can delegate to that, and so can the exception reporter, but it will pass in a filtered POST dictionary. That way we get control we need and avoid duplication.

[Julien Phalip]

I've made all the changes requested in comment:15 and also added a test for the HttpRequest.exception_reporter_filter feature.

A build_request_repr() method (or maybe simply build_repr() since it would belong to the HttpRequest class) sounds like a great idea to consolidate all that code. It feels like it should belong to a separate ticket, though. Otherwise reverting the commit might be awkward if an issue is found with that change later. If you agree then I'll create a new ticket with a patch once this one gets resolved.

[Luke Plant]

Yep, those changes can go in a separate ticket. Thanks!

[Luke Plant]

In [16339]:

Fixed #14614 - filtering of sensitive information in 500 error reports.

This adds a flexible mechanism for filtering what request/traceback
information is shown in 500 error emails and logs. It also applies
screening to some views known to be sensitive e.g. views that handle
passwords.

Thanks to oaylanc for the report and many thanks to Julien Phalip for the
patch and the rest of the work on this.

[Luke Plant]

BTW Julien - one note on your patch, it had a fair amount of trailing whitespace, which we are fairly picky about. You can get most VCS tools to highlight this in their diff output, or use a tool like colordiff that does it.

[Julien Phalip]

Oh yes indeed. Thanks for the tip.