Opened 2 years ago

Last modified 9 months ago

#23004 new New feature

Cleanse entries from request.META in debug views

Reported by: Daniel Hahler Owned by: nobody
Component: Error reporting Version: master
Severity: Normal Keywords:
Cc: Jack Laxson Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the debug views settings is cleansed, which hides e.g. SECRET_KEY.

But a lot of sensible information might also be present / come from request.META, e.g. in the form of DJANGO_SECRET_KEY or DATABASE_URL.

It might be sensible to apply a filter in TECHNICAL_500_TEMPLATE (source code reference: https://github.com/django/django/blob/master/django/views/debug.py#L972-977).

I see that this can be quite specific, but I think it would be sensible to apply HIDDEN_SETTINGS to all entries starting with DJANGO_ and have a setting for additional entries, which might default to DATABASE_URL and SENTRY_DSN.

Change History (8)

comment:1 Changed 2 years ago by 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?

comment:2 Changed 2 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

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.

comment:3 Changed 2 years ago by 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

comment:4 Changed 2 years ago by 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.

comment:5 Changed 21 months ago by Daniel Hahler

@sthzg
Your proposed changes look good to me.

comment:6 Changed 18 months ago by Jack Laxson

Cc: Jack Laxson added

comment:7 Changed 17 months ago by Tim Graham

Component: Core (Other)Error reporting

comment:8 Changed 9 months ago by Tim Graham

Has patch: set
Needs documentation: set
Needs tests: set

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

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