(urgent security problem) default _show_toolbar causes debug to leak to unprivileged IPs

[foxx]

        # if not internal ip, and not DEBUG
        if not (remote_addr in settings.INTERNAL_IPS or settings.DEBUG):
            return False

The above line of code causes debug info to be shown to non privileged IPs.

This is extremely bad and is completely against the documentation:

"The default checks are that DEBUG must be set to True and the IP of the request must be in INTERNAL_IPS. "

Please release an urgent fix for this by using:

        # if not internal ip, and not DEBUG
        if not (remote_addr in settings.INTERNAL_IPS and settings.DEBUG):
            return False

[dcramer]

I'll update the documentation, but this is going to be a wontfix.

Don't run DEBUG in production.

[foxx]

I'm not running DEBUG in production!!!!! :X I disagree that this should be a wontfix. What if you are allowing other people to test changes in dev?? I don't want them seeing the debug toolbar by default. Come on man :X

[foxx]

May I ask what the reasoning is behind marking this as wontfix? I simply don't see why you would assume users want DDT shown to all requests, just purely on debug being enabled. :S

[foxx]

Just my two cents worth but, changing documentation to get around bugs (especially when they related to security) is not very clever. DDT is a decent piece of code, well written, with a lot of time/effort gone into it. You've made a backwards incompatible change, without changing major release numbers. Look at this from a purely logical point of view, the decision of wontfix doesn't make any sense :X

[dcramer]

This isn't a bug. if says "if your ip is in the list of internal ips, or you've enabled debug, continue". It may not be completely backwards compatible, but it really shouldn't need to be as this is how production environments are intended in Django.

I wrote a lot of DDT, so you dont need to tell me that a lot of time or effort has gone into it. We haven't released any version, let alone a major version. This is a change made to master, which is the current development version.

[foxx]

So, you're saying that this particular change, will only appear in the next major release?

[foxx]

Also, you keep banging on about "production environments", but this is applicable to development environments, not production. I don't know why you keep bringing that up for. :X Also, I can't think of any instance where someone would want DDT enabled for the production environment. You even said yourself, debugging isn't for the production environment. Seriously, this change just doesn't make any sense.

[airstrike]

Totally agree with foxx on that one.

I expect DDT to only be displayed for INTERNAL_IPS and only during DEBUG mode, it shouldn't be an OR clause.

Please fix this, it's incredibly simple!

[dcramer]

The concern brought up here was that it "leaks information", which isnt true if this is development environment. Having a killswitch for DEBUG_TOOLBAR (or rather an "ENABLE IT" switch) is another story.

What kind of situation would you have DEBUG = True and not be accessing it from an "internal ip"? That's what I would define as a production environment.

[foxx]

You are suggesting that all users should be privileged to have this information, just because they have an internal IP? Sorry, but no. If our testing department has internal IPs (which they do), they would also get to see this information. On top of this, DDT adds significant page loading times when enabled, so when testers test a change in dev (and ensure it isn't taking ages to load) they can't do this unless we push this to staging/production.

(edit) I guess in smaller environments where you don't have many people testing/working on the same code, it won't make much of a difference. But for large environments like we are working on, it makes a huge difference :X

[dcramer]

@foxx that's good reasoning -- I think the kill switch is a better fit for this then

[dcramer]

Need to think about this a bit more yet. Ideally the defaults should allow easily loading the toolbar into a production environment (to put it into a temporary debug state), but we still need to restrict that to something like internal ips. I'm thinking we'll replace the and DEBUG with something like and DEBUG_TOOLBAR which will default to the value of DEBUG.

[foxx]

+1 on having DEBUG_TOOLBAR default to DEBUG, then the conditional checking for DEBUG_TOOLBAR and INTERNAL_IPS..?

[dcramer]

@foxx yep sounds like a solid solution to me (and then we dont rely on DEBUG being enabled, which can be very costly)

[jasonkeene]

I didn't see this issue before, I also agree that the prior release's behavior is preferred.

My solution if anyone wants to pull: #191

Keep up the great work folks!

[jasonkeene]

Updated pull request w/ merge to current master: #198

[jezdez]

Closing this as it seems to have been fixed a couple of months ago.

But I'd also like to take to chance to remind everyone involved, especially @foxx, to not write about "urgent security problems" in a public forum such as the issue tracker next time. Report security issues to the main developers directly, the email addresses are publicly shown on our profile pages.

[foxx]

@jezdez Good point. +1