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

Remove settings import from install instructions. #1528

Merged
merged 1 commit into from
Nov 15, 2021

Conversation

adamchainz
Copy link
Contributor

The import of settings is no longer required after #1349

The import of settings is no longer required after django-commons#1349
@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #1528 (6edb7ba) into main (d0300b1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1528   +/-   ##
=======================================
  Coverage   85.87%   85.87%           
=======================================
  Files          35       35           
  Lines        1883     1883           
  Branches      274      274           
=======================================
  Hits         1617     1617           
  Misses        187      187           
  Partials       79       79           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0300b1...6edb7ba. Read the comment docs.

Copy link
Member

@matthiask matthiask left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This makes the installation instructions more consistent.

Now that I look at the snippet again I worry a bit about potential security implications. The default SHOW_TOOLBAR_CALLBACK does make this safe but I don't know whether that's good defense in depth.

@adamchainz
Copy link
Contributor Author

I think it's sufficient protection. Asking users to put if settings.DEBUG in URLs is just “pushing the ‘if’ around”. I'm sure ~99% of users don't change SHOW_TOOLBAR_CALLBACK.

@tim-schilling
Copy link
Member

I'm sure ~99% of users don't change SHOW_TOOLBAR_CALLBACK.

Unfortunately that's not true. Dealing with docker causes folks to make changes to it. And then people who deploy to remote servers will also change it.

Though we should add a check/warning that identifies when the callback has been changed from the default and the toolbar is running with DEBUG=False. Or something that covers that case.

@matthiask matthiask merged commit 3bdd40c into django-commons:main Nov 15, 2021
@matthiask
Copy link
Member

Though we should add a check/warning that identifies when the callback has been changed from the default and the toolbar is running with DEBUG=False. Or something that covers that case.

I think we should add a big warning to the SHOW_TOOLBAR_CALLBACK docs and leave it at that. I'll submit a pull request right away. I merged this PR because the change itself isn't related to the security issue we're discussing.

Thanks @adamchainz !

@adamchainz adamchainz deleted the patch-1 branch November 15, 2021 17:34
@adamchainz
Copy link
Contributor Author

I think we should add a big warning to the SHOW_TOOLBAR_CALLBACK docs and leave it at that. I'll submit a pull request right away.

+1 to that. That would be the same kind of thing we'd do for Django core: sensible defaults, with the ability to override if required with warnings.

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

Successfully merging this pull request may close these issues.

3 participants