SHOW_TOOLBAR_CALLBACK no longer supports lambda #523

Closed
ross opened this Issue Jan 8, 2014 · 20 comments

Comments

Projects
None yet
9 participants

ross commented Jan 8, 2014

in my setup i always have seperate dev settings and in those settings i enable debug toolbar for all requests. i've always done that with the following config

DEBUG_TOOLBAR_CONFIG = {
    'SHOW_TOOLBAR_CALLBACK': lambda request: True,
}

after updating my virtualenv this morning i started getting errors that i've tracked down to trying to run rsplit on func_path

line 45 of debu_toolbar/middleware.py:

    mod_path, func_name = func_path.rsplit('.', 1)

it looks like the meaning of SHOW_TOOLBAR_CALLBACK may have changed to require a string and not support a callable. if that's the case i'm not sure what the solution would be. i'd rather not have to define a function somewhere and point this at it just to return true. i'd prefer if SHOW_TOOLBAR_CALLBACK supported being presented a callable.

Contributor

aaugustin commented Jan 8, 2014

This change was made on purpose to prevent people from copy-pasting the insecure example that was in the old documentation.

Is there a good reason for using this definition? If you simply copy-pasted from the docs without bothering to read the surrounding text, remove it and move on.

@aaugustin aaugustin closed this Jan 8, 2014

ross commented Jan 8, 2014

as i explained in the original description, i'm not just copy-n-pasting.

there isn't a security issue as debug_toolbar isn't even installed on the public/production boxes, much less configured this way. it's in a dev only config file/setup. it's used by developers to run with their local boxes where it's safe/desirable to allow blanket access.

Contributor

aaugustin commented Jan 8, 2014

All Django settings have primitive values, to avoid the temptation to import stuff in settings. The Debug Toolbar follows this philosophy. I'm sorry that it makes your use case marginally less practical, but I still think it's a good trade-off for the community at large.

I would suggest the following (untested) code:

DEBUG_TOOLBAR_CONFIG = {
    'SHOW_TOOLBAR_CALLBACK': "%s.true" % __name__,
}

def true(request):
    return True

ross commented Jan 8, 2014

that's effectively the work-around i've put in place and it's not that big a deal.

as a general it's a good idea to give a better error message for something that previously worked fine and you're specifically going to break. the following results in users having to open up the source and figure out what went wrong.

AttributeError: 'function' object has no attribute 'rstrip'

something like the following would be preferable.

...
if callable(func_path):
    ImproperlyConfigured('invalid SHOW_TOOLBAR_CALLBACK, a dotted path to a function is required')
...
Contributor

aaugustin commented Jan 10, 2014

Fair enough!

@aaugustin aaugustin reopened this Jan 10, 2014

ross commented Jan 10, 2014

thanks. liking the updated/cleaned up stuff

tomkins commented Jan 14, 2014

Given that the exact same thing can be done as demonstrated above, was there a need to remove the old behaviour? Just feels like it results in a slightly more ugly settings file for my local/dev instances.

My opinion for the best option would be to default to 'debug_toolbar.middleware.show_toolbar' by default as the recommended option in the documentation, with iscallable being used for those of us who actually want it to be a function/lambda.

It's a debug toolbar, I'd hope that anyone installing it would recognise the fact that it can show a lot of sensitive information and that we don't need protecting in this way.

Contributor

aaugustin commented Jan 15, 2014

You cannot make this assumption. You'd be surprised at how many people run sites in production with DEBUG = True. (More than half of them last time I had the opportunity to check a significant sample.)

:(

@aaugustin aaugustin closed this Jan 15, 2014

@aaugustin aaugustin reopened this Jan 15, 2014

Contributor

aaugustin commented Jan 26, 2014

In fact, I had already included such a warning: https://github.com/django-debug-toolbar/django-debug-toolbar/blob/master/debug_toolbar/settings.py#L66-L69. You should have seen it.

I'm hesitating a bit; I could be convinced to restore support for callables, especially if one of the other maintainers wanted it.

just 2c - the warnings don't show up on the web interface, just in the logs. While it's quick to google (http://stackoverflow.com/a/20727979), but it would be nice to have a the same error message on the UI as it is in the warnings.

foxx commented May 2, 2014

Although the __name__ snippet from @aaugustin works, this really was quite annoying and wasted quite a bit of time. That being said, I do agree that it's better to avoid putting callables into settings.

Therefore, I'd be +1 for a docs update using the __name__ snippet, but -1 on adding support for callables.

Contributor

aaugustin commented Aug 2, 2014

@tim-schilling Do you have an opinion on this?

At this point, I'm vaguely +0 on restoring support for callables, in the name of practicality.

Contributor

tim-schilling commented Aug 4, 2014

I think the toolbar should follow Django's practice of keeping the settings' values as primitives.

However, I don't think a DeprecationWarning is correct. We should raise a ImproperlyConfigured since the old values will no longer work with the new version. While this doesn't allow people to use callables, it would help them resolve the issue faster.

ross commented Sep 25, 2014

as previously mentioned if the regression is going to happen (which at this point it has and everyone has moved on) a better error message is the right path. snippet for that is above, django-debug-toolbar#523 (comment)

Contributor

dcramer commented Nov 1, 2014

+1 on restoring callables

The "Django way" is not always the right way, and this turns a simple problem into a complex problem in many situations.

@aaugustin aaugustin self-assigned this Nov 1, 2014

Contributor

aaugustin commented Nov 1, 2014

@dcramer There's also a better reason which I had forgotten about: bug reports showed that many people copy-pasted the insecure example that was in the old documentation without realizing the security consequences.

I wanted to reduce the risk of receiving one day a bug report along the lines of "I was hacked because I copy-pasted the example settings from the DjDT's docs".

Contributor

dcramer commented Nov 1, 2014

I dont think developers being ignorant is a good reason to remove simplicity :)

ross commented Nov 1, 2014

personally at this point i don't care what happens, either restore the previous behavior or put a better error message in place. either is fine, but please just do one of them and close this ticket.

@aaugustin aaugustin closed this in cb428ce Nov 1, 2014

Simanas commented Jul 2, 2015

Spend more than an hour on this until I finally got it working on my dev server. This is so frustrating! It is impossible to save idiots from making stupid mistakes. They will still find many workaround and do stupid security shit. Default security measures just makes this wonderful tool so complicated and frustrating to work with. I just want to plug and play with it. Will take over security stuff my self as I am a developer and that's my job.

All of this is completely out of the scope of The Zen of Python: https://www.python.org/dev/peps/pep-0020/

There should be no additional options at all for displaying toolbar or not. If you have it in your installed apps it's displayed if not, it's not displayed. That's it. Leave everything else for developers to decide.

Anyway, thanks for running this amazing tool! I am a huge fan of it, except for these stupid security measures.

👍 for simplicity. A dict with a string that resolves to a function that evaluates to a boolean that determines whether to expose potentially sensitive information doesn't seem to respect "economy of mechanism" as a security principle.

ryneeverett pushed a commit to ryneeverett/django-debug-toolbar that referenced this issue Oct 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment