Cleanse entries from request.META in debug views

[Daniel Hahler]

I have noticed that the environment variables do not appear to be present when using Django's test.Client / live_server.

Shouldn't the test client's request.meta also include os.environ?

[Tim Graham]

I would do something like move the cleanse_setting() function to a method on ExceptionReporterFilter and make things like HIDDEN_SETTINGS attributes. You could then easily override them in a subclass to avoid introducing more global settings.

Regarding your comment, it's a separate issue but I don't think the test client should include os.environ. You shouldn't rely on environment variables in your views.

[Daniel Hahler]

Just answering to the separate issue from the comment about request.META - I do not have time to provide a patch for this issue myself, but thanks for accepting it and outlining how it could be done!

Regarding your comment, it's a separate issue but I don't think the test client should include os.environ. You shouldn't rely on environment variables in your views.

It's more that I want to test for e.g. assert settings.SECRET_KEY not in response.content (for a "500" page), and was surprised that request.META in the test client is different from runserver/uwsgi. I have created a new issue for it: https://code.djangoproject.com/ticket/23006

[Stephan Herzog]

I am interested in this topic and started experimenting with @timo's suggestions. I hope it is okay to put some questions here, because I am not completely sure about the scope of it and would be interested in your opinion.

---

After reading through the code, the cleanse_setting() method seems to only be relevant to parsing values from the settings. Cleansing POST for example (which like META is part of the request instance) is done as part of SafeExceptionReporterFilter. What I am experimenting with is to provide similar behavior for request.META as there already is for POST.

I implemented a get_meta_parameters() on SafeExceptionReporterFilter that cleanses all values in META that match the HIDDEN_SETTINGS (that are now an attribute of ExceptionReporterFilter).

def get_meta_parameters(self, request):
    """
    Replaces the values of META parameters that match defined patterns
    from HIDDEN_SETTINGS with stars (*********).
    """
    if request is None:
        return {}
    else:
        cleansed = request.META.copy()
        # Cleanse all values that match the regexp in HIDDEN_SETTINGS.
        for k, v in cleansed.items():
            if self.HIDDEN_SETTINGS.search(k):
                cleansed[k] = CLEANSED_SUBSTITUTE
        return cleansed

Now my idea would be to extend the Context instance in get_traceback_data() to get a filtered_META, analog to what it does to get the filtered_POST

c = {
    # ...
    'filtered_POST': self.filter.get_post_parameters(self.request),
    'filtered_META': self.filter.get_meta_parameters(self.request)
    # ...
}

Then, if filtered_META is not empty, I thought about changing the TECHNICAL_500_TEMPLATE to loop over that.

---

Before I go on I would be interested if this was still accepted in terms of behavior and scope or if a solution in that direction would be unlikely to make its way to core. If yes I would be happy trying to code it and backing it up by tests and then come back here to discuss the possible patch.

[Daniel Hahler]

@sthzg
Your proposed changes look good to me.

[Tim Graham]

​PR (currently without tests and docs and probably developed independent from the discussion in this ticket).

[Tim Graham]

​PR with comments for improvement.

[Ryan Castner]

Ok I made the updates you requested. Only thing I am unsure of is how I did the .. versionchanged 2.0 annotation.

[Daniel Maxson]

​PR updated for the current master branch and with a test for ExceptionReporterFilter.get_safe_request_meta.

[Carlton Gibson]

There have been updates since last review. Unchecking PNI to put it back in the queue.

[Markus Holtermann]

I'm not entirely sure we need that big of a change for this ticket, especially considering that this is a DEBUG=True page that should _never_ show up in production in the first place. I'd like to propose a significantly smaller PR instead: ​https://github.com/django/django/pull/11224

[Ryan Castner]

I'm not entirely sure we need that big of a change for this ticket, especially considering that this is a DEBUG=True

That is sort of frustrating because this was my original PR

​https://github.com/django/django/pull/7996/files