[Django] #35901: settings.DEBUG could reject non-empty string values (or in particular "off", "no", "0", "disabled", "false", "False")

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")
-------------------------------------+-------------------------------------
Reporter: Sebastian Pipping | Type:
| Uncategorized
Status: new | Component: Core
| (Other)
Version: dev | Severity: Normal
Keywords: security debug | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Hi!

I came across a setup recently where (simplified) troubling code `DEBUG =
os.environ.get("DEBUG", False)` with environment variables state `DEBUG=0`
(and later `DEBUG=False`) was activating Debug mode (while not intended to
and and unaware of) in practice because these (and all other non-empty)
strings evaluate to `True` in Python:

{{{
In [1]: bool("")
Out[1]: False

In [2]: bool("False")
Out[2]: True

In [3]: bool("0")
Out[3]: True
}}}

The related code is Open Source and my related pull request for their
project is public at
https://github.com/climateconnect/climateconnect/pull/1331 .

To cheaply protect users from accidents like these (that can easily result
in arbitrary remote code execution) in the future, Django could reject
values from `settings.DEBUG` that are (a) any string or (b) any non-empty
string or (c) in a list of known excluded words (e.g. "off", "no", "0",
"disabled", "false", "False").

What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/35901>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")

-----------------------------------+--------------------------------------
Reporter: Sebastian Pipping | Owner: (none)

Type: Uncategorized | Status: new
Component: Core (Other) | Version: dev

Severity: Normal | Resolution:

Keywords: security debug | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-----------------------------------+--------------------------------------
Comment (by Venkatesh S):

Thank you, Sebastian, for reporting this and for your detailed
explanation!

Our setup is handled this differently. However, I now see that, like many
projects, it could indeed be affected by unintended string evaluations if
values are drawn directly from .env files without parsing.

In our current Django configuration, we set DEBUG as a Boolean directly
rather than retrieving it from .env, which avoids this specific issue for
us.

Thank you!
--
Ticket URL: <https://code.djangoproject.com/ticket/35901#comment:1>

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")
-----------------------------------+--------------------------------------
Reporter: Sebastian Pipping | Owner: (none)

Type: Uncategorized | Status: closed

Component: Core (Other) | Version: dev

Severity: Normal | Resolution: invalid

Keywords: security debug | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Changes (by Venkatesh S):

* cc: Venkatesh S (added)
* resolution: => invalid
* status: new => closed

--
Ticket URL: <https://code.djangoproject.com/ticket/35901#comment:2>

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")
-----------------------------------+--------------------------------------
Reporter: Sebastian Pipping | Owner: (none)

Type: Uncategorized | Status: new

Component: Core (Other) | Version: dev
Severity: Normal | Resolution:

Keywords: security debug | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Changes (by Sebastian Pipping):

* resolution: invalid =>
* status: closed => new

Comment:

This is not about a specific setup but a pattern or I would not even
create this ticket. Many users control debug mode through an environment
variable and hopefully get this right like
https://github.com/hartwork/jawanndenn/blob/356ee23f9b1bb3fa876e405a5b9fae0c0729e0df/jawanndenn/settings.py#L31
but reality proves that users could use help with this. Please re-consider
just shutting me down. Thank you.
--
Ticket URL: <https://code.djangoproject.com/ticket/35901#comment:3>

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")
-----------------------------------+--------------------------------------
Reporter: Sebastian Pipping | Owner: (none)
Type: Uncategorized | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: security debug | Triage Stage: Unreviewed

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------
Changes (by Sebastian Pipping):

* has_patch: 0 => 1

Comment:

Pull request: https://github.com/django/django/pull/18793
--
Ticket URL: <https://code.djangoproject.com/ticket/35901#comment:4>

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")
-----------------------------------+--------------------------------------
Reporter: Sebastian Pipping | Owner: (none)
Type: Uncategorized | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: security debug | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by Tim Graham):

I'd be inclined to say wontfix unless a consensus can be reached on the
forum. See a [https://forum.djangoproject.com/t/make-the-system-check-
look-at-the-types-of-settings/27424 related thread]. on a proposal to add
a system check to verify the type of all settings. There are often
unintended side effects when we've added checks like this.

Here is [https://dev.to/codereviewdoctor/don-t-abuse-django-s-debug-
setting-1flc#feature-flag-implimentation an interesting approach] (though
perhaps not the best one) I found to help avoid a configuration mistake
with environment variables:
{{{#!python
from ast import literal_eval
from os import getenv

DEBUG = literal_eval(getenv('DEBUG', 'False'))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35901#comment:5>

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")
-----------------------------------+--------------------------------------
Reporter: Sebastian Pipping | Owner: (none)
Type: Uncategorized | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: security debug | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by Sebastian Pipping):

The code sample is a nice idea.

With regard to checking all setting types I don't see need to make the
picture that big: the type check is an implemenation detail — we could as
well just block values like `"off", "no", "0", "disabled", "false",
"False" — but then the list will never be complete. The current explicit
check also doesn't prevent any later migrations towards full blown type
checks in the future. Why not start small?

I would appreciate to consider that this change alone would have saved
unfortunate setup
https://github.com/climateconnect/climateconnect/pull/1331 from potential
remote code execution. That's the key motivator behind this suggestion: it
could have been prevented easily.
--
Ticket URL: <https://code.djangoproject.com/ticket/35901#comment:6>

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")
-----------------------------------+--------------------------------------
Reporter: Sebastian Pipping | Owner: (none)
Type: Uncategorized | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: security debug | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by Mariusz Felisiak):

Agreed with Tim. "wontfix" for me. This will definitely have side-effects
on some projects.
--
Ticket URL: <https://code.djangoproject.com/ticket/35901#comment:7>

[Django]

#35901: settings.DEBUG could reject non-empty string values (or in particular
"off", "no", "0", "disabled", "false", "False")
-----------------------------------+--------------------------------------
Reporter: Sebastian Pipping | Owner: (none)

Type: New feature | Status: closed

Component: Core (Other) | Version: dev

Severity: Normal | Resolution: wontfix
Keywords: typed settings | Triage Stage: Unreviewed

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Changes (by Natalia Bidart):

* keywords: security debug => typed settings
* resolution: => wontfix

* status: new => closed

* type: Uncategorized => New feature

Comment:

I agree with Tim and Mariusz that this proposal isn't suitable for Django
(as a core feature). Moreover, I think it qualifies as a duplicate of
#35231 (which is a ticket related to the forum link shared by Tim).

I'll close this as `wontfix` because, if we were to implement a change
that only applies to `DEBUG`, it would be incomplete and inconsistent.
Users would likely request similar handling for every boolean setting.
Even if we made the behavior consistent, I don't believe this parsing
logic belongs in the settings module. Django settings are meant to be
Python files, and the core framework doesn't include functionality for
reading from environment variables.

Another reason for closing as `wontfix` is that there are third-party
packages that handle parsing from env variables effectively. For example,
[https://django-environ.readthedocs.io/ django-environ] uses
`BOOLEAN_TRUE_STRINGS = ('true', 'on', 'ok', 'y', 'yes', '1')` for bool
logic parsing.

I think it's important to decouple the settings definition (the Python
file where a string is not a bool) from reading settings from external
sources, such as environment variables, INI config files, Puppetfiles,
etc. The former definitely belongs to Django, while the latter is better
suited for third-party packages or custom business logic.
--
Ticket URL: <https://code.djangoproject.com/ticket/35901#comment:8>