Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What are drawbacks of having debug toolbar in production settings ? #1435

Open
JocelynDelalande opened this issue Jan 14, 2021 · 6 comments
Open

Comments

@JocelynDelalande
Copy link

INTERNAL_IPS or DEBUG=False being not set means DDT is disabled… Or at least out of sight.

But I am wondering what are actual (performance or security) drawbacks of having DDT listed in INSTALLED_APPS and MIDDLEWARE settings when INTERNAL_IPS is not set and DEBUG=False.

In other words : How far DDT is to being a no-op when DEBUG=False and INTERNAL_IPS is not set ?

Asking because it can be handy to have same INSTALLED_APPS and MIDDLEWARE settings for dev/prod and vary only on DEBUG and INTERNAL_IPS ; it offers simpler settings management and the ability to enable it temporarily (ex: on test/pre-production server).

Thanks by advance for your input :)

Oh and thanks for that great piece of software that rocks jazzes !

@JocelynDelalande JocelynDelalande changed the title What are drawbacks of debug toolbar in production settings ? What are drawbacks of having debug toolbar in production settings ? Jan 14, 2021
@matthiask
Copy link
Member

Thank you! 💜

Here's a pull request changing the cache panel and an excerpt from the discussion: #1427

[...] I don't think it's possible to avoid that and still capture the middleware cache calls before the toolbar's middleware.

So, it may be the case that some tracking happens; performance may be degraded. There may also be security-related implications. I'm not sure. I'd like to have better dev/prod parity too, but in this case I'm a bit wary – django-debug-toolbar definitely isn't being audited properly and continuously as it should probably be for this.

@JocelynDelalande
Copy link
Author

Thanks for your input !

At the moment, I ended up with the following compromise (a bit a magic don't hurt to be confy and safe at the same time !)

settings.py

from django.core.exceptions import ImproperlyConfigured

from .base_settings import *

# Override parameters with local_settings.py
try:
    from .local_settings import *
except ImportError:
    raise ImproperlyConfigured(
        'You need to create a local_settings.py, see README.md'
    )

if DEBUG:
    # Enable debug toolbar only if DEBUG=True
    INSTALLED_APPS = INSTALLED_APPS +  ('debug_toolbar',)
    MIDDLEWARE = ['debug_toolbar.middleware.DebugToolbarMiddleware',] + MIDDLEWARE

base_settings.py

[…]
DEBUG = False

INSTALLED_APPS = (
    'django.contrib.auth',
   […]
    # enabled on DEBUG=True ; see settings.py
    # 'debug_toolbar',
)

MIDDLEWARE = [
    # enabled on DEBUG=True ; see settings.py
    # 'debug_toolbar.middleware.DebugToolbarMiddleware',
    'django.contrib.sessions.middleware.SessionMiddleware',
    […]
]

[…]

local_settings.py (not git-versioned, example for development setup)

DEBUG = True

@tim-schilling
Copy link
Member

I believe the logger also gets monkey-patched regardless if it's enabled or not.

I'm torn about wanting to have this installed and be a no-op at the production level because of my perceived prevalence of devs struggling to get the toolbar to show on a non-local, staging type machine and default to setting SHOW_TOOLBAR_CALLBACK to always return True. If we screw something up and there's a leak, the toolbar will track all requests and be at risk of exposing a lot of sensitive information. Especially if we introduce a way to persist the data to support multi-threaded servers or Channels.

An alternative is to make it easier to configure SHOW_TOOLBAR_CALLBACK. Perhaps a predefined function that returns true if request.user.is_superuser == True? We'll likely want to sanitize the locals and request data similar to what Django does with the error emails with the option to show those values.

@ryanhiebert
Copy link

Perhaps a predefined function that returns true if request.user.is_superuser == True

I think this could go a long way. Although I might suggest making a new permission if possible instead of directly using the is_superuser flag.

@tim-schilling
Copy link
Member

Although I might suggest making a new permission if possible instead of directly using the is_superuser flag.

I can see a future in which that's reasonable where the toolbar has the serializable branch done, can run in production with the metrics logged somewhere, and then accessed via the web app. However, that's a very optimistic perspective on the toolbar's feature development.

I'd be more in favor of keeping it very basic until it needs to be expanded.

@ryanhiebert
Copy link

OK, I can see that. Adding the permission would be a backward-compatible change, after all. It feels like the "right" way to do it, but this is more about making sure the production failure mode is mitigated to an acceptable degree than anything else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants