Opened 10 years ago

Closed 5 years ago

#23004 closed New feature (fixed)

Cleanse entries from request.META in debug views

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

Description (last modified by Carlton Gibson)

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 (33)

comment:1 by Daniel Hahler, 10 years ago

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 by Tim Graham, 10 years ago

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 by Daniel Hahler, 10 years ago

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 by Stephan Herzog, 10 years ago

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 by Daniel Hahler, 10 years ago

@sthzg
Your proposed changes look good to me.

comment:6 by Jack Laxson, 9 years ago

Cc: Jack Laxson added

comment:7 by Tim Graham, 9 years ago

Component: Core (Other)Error reporting

comment:8 by Tim Graham, 9 years ago

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).

comment:9 by Ryan Castner, 8 years ago

Owner: changed from nobody to Ryan Castner
Status: newassigned

comment:10 by Tim Graham, 8 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: set

PR with comments for improvement.

comment:11 by Ryan Castner, 8 years ago

Patch needs improvement: unset

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

comment:12 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:13 by Daniel Maxson, 6 years ago

Owner: changed from Ryan Castner to Daniel Maxson

comment:14 by Daniel Maxson, 6 years ago

Patch needs improvement: unset

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

Last edited 6 years ago by Daniel Maxson (previous) (diff)

comment:15 by Carlton Gibson, 6 years ago

Patch needs improvement: set

comment:16 by Carlton Gibson, 6 years ago

Patch needs improvement: unset

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

comment:17 by Oskar Haller, 6 years ago

Triage Stage: AcceptedReady for checkin

comment:18 by Markus Holtermann, 6 years ago

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

comment:19 by Markus Holtermann, 6 years ago

Triage Stage: Ready for checkinAccepted

comment:20 by Ryan Castner, 6 years ago

Description: modified (diff)

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

comment:21 by Daniel Maxson, 6 years ago

My take is that we should assume that this page will end up on production at some point. Or perhaps a developer is working on an internal Django project and thinks it's safe to keep DEBUG=TRUE there, but someone else on the inside maliciously leverages this. Or someone's reusing credentials between staging and production.

I would prefer not to hear that Django was how someone pivoted from HTTP access to a debug build to acquiring secrets. Even if some of the fault would be on bad developer practices, some of the fault would also be on Django for leaking secrets when it didn't need to (and if it does need to in specific situations, that's configurable for people who know what they're doing).

I think there's a strong security argument that we should assume this data will leak, and so should be cleansed before it does.

in reply to:  20 comment:22 by Markus Holtermann, 6 years ago

Replying to 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

Yikes, I did not see your PR. I'm sorry! It does look indeed a lot like mine. I'd be ok with re-opening your PR instead.

Replying to Daniel Maxson:

My take is that we should assume that this page will end up on production at some point. Or perhaps a developer is working on an internal Django project and thinks it's safe to keep DEBUG=TRUE there, but someone else on the inside maliciously leverages this. Or someone's reusing credentials between staging and production.

I would prefer not to hear that Django was how someone pivoted from HTTP access to a debug build to acquiring secrets. Even if some of the fault would be on bad developer practices, some of the fault would also be on Django for leaking secrets when it didn't need to (and if it does need to in specific situations, that's configurable for people who know what they're doing).

I think there's a strong security argument that we should assume this data will leak, and so should be cleansed before it does.

The difference between your and Ryan/mine PRs is that yours adds the additional functionality to specify which variables to treat as sensitive. Considering that the entire cleaning is a best-effort approach, I don't think that warrants the additional complexity. Especially as variables like HTTP_PROXY etc may still contain secrets (and I don't think we want to hide everything that contains HTTP). The only safe approach is not showing variables in the first place.

comment:23 by Daniel Maxson, 6 years ago

Thanks for the clarification. I misunderstood your previous comment. I think what you're saying gets to the point I mention in the PR here:

I added the values as optional construction arguments with the defaults coming from constants defined on the class. The class definition looks a little clunkier but my thinking was this may be more pleasant to use when actually creating subclasses than needing to update the attributes only after construction.

Still, I'm not married to this approach and can remove the arguments (and make any other changes) if the approach in the new commits doesn't sit right with people.

My PR was intended primarily as an effort to bring Ryan's PR up to date just because there hadn't been work on it in a while and I wanted to get this feature for my own use. I happened to throw in the arguments while working on that because it felt strange to me at the time not to, but having done that, I can see the case against the added complexity.

Regardless, adding the arguments is outside the scope of this ticket so in retrospect I should have limited myself only to updating Ryan's PR.

comment:24 by Oskar Haller, 6 years ago

I think Django Core Team should do a decision on how to proceed

comment:25 by Daniel Maxson, 6 years ago

@Markus Holtermann: Out of curiosity, have you seen Carlton Gibson's comment on the PR at Feb 13? Can you perhaps have that conversation and arrive at an agreement on how to proceed?

comment:26 by Oskar Haller, 5 years ago

Is there any progress on this?

comment:27 by Carlton Gibson, 5 years ago

Yes. It’s just waiting for a re-review.

comment:28 by Carlton Gibson, 5 years ago

Description: modified (diff)

comment:29 by Carlton Gibson, 5 years ago

OK... Right...

First off, thank you everybody for input on this (and related tickets).

I say "and related tickets" because this kind of thing keeps coming up. (e.g. #23951 but others) Can we filter this?, Can we filter that?, and so on.

#29714 suggests making having a custom ExceptionReporter easier. In this case you'd just override get_traceback_data() and job done. I'm hoping to get that in.

There was an idea to get rid of the exception filter as the extension point but I think that's too quick for now. As such, I'd like to go for the more ambitious of the two approaches here. I think Tim's original comment, way back when was the right one in terms of providing the right flexibility without us having to add hooks for every possible use-case.

On that basis, I'm going to close Markus' PR (Thank you again Markus) and then have one final look... I hope that makes sense.

...especially considering that this is a DEBUG=True.

It's not. ExceptionReporter is used by AdminEmailHandler at the least. It would be natural for folks to use it in other handlers too.

Last edited 5 years ago by Carlton Gibson (previous) (diff)

comment:30 by Carlton Gibson, 5 years ago

Patch needs improvement: set

PR is looking good, but needs a little work on the docs, (as per review on PR).

Daniel, I'd like to get this in: combined with #29714, it will clear up a whole group of the "Error Reporting" tickets. Please let me know if you need input or don't have capacity to finish it off.

(Thanks for your effort! And same to all!)

comment:31 by Carlton Gibson, 5 years ago

Patch needs improvement: unset

New PR adjusting for feedback https://github.com/django/django/pull/12300

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

In 581ba5a9:

Refs #23004 -- Allowed exception reporter filters to customize settings filtering.

Thanks to Tim Graham for the original implementation idea.

Co-authored-by: Daniel Maxson <dmaxson@…>

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

Resolution: fixed
Status: assignedclosed

In e2d9d66a:

Fixed #23004 -- Added request.META filtering to SafeExceptionReporterFilter.

Co-authored-by: Ryan Castner <castner.rr@…>

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