[Django] #36709: Bug Report: `manage.py check` Fails to Detect `@staticmethod` Override for `is_authenticated` (auth.C010) and 'is_anonymous' (auth.C009)

[Django]

#36709: Bug Report: `manage.py check` Fails to Detect `@staticmethod` Override for
`is_authenticated` (auth.C010) and 'is_anonymous' (auth.C009)
---------------------------+----------------------------------------
Reporter: cyberstan | Type: Bug
Status: new | Component: contrib.auth
Version: 5.2 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
---------------------------+----------------------------------------
'''Summary:'''
The Django system check command (`manage.py check`) fails to identify an
issue when a custom user model overrides `is_authenticated` or
`is_anonymous` with a `@staticmethod`.

The relevant checks, `auth.C009` and `auth.C010`, appear to check only for
`MethodType`, but do not correctly identify `staticmethod` objects.

This is a flaw in the check's tooling, as this override pattern causes
`bool(User())` to incorrectly evaluate to `True`, which could lead to
issues in custom developer code that manually instantiates the user model.

=== How to Reproduce

1. '''Define a `VulnerableUser` model in `models.py`:'''
{{{#!python
from django.contrib.auth.models import AbstractUser

class VulnerableUser(AbstractUser):
"""
This model overrides is_authenticated with a staticmethod,
which the check command fails to detect.
"""

@staticmethod
def is_authenticated():
return False

@staticmethod
def is_anonymous():
return False
}}}

2. '''Point to this model in `settings.py`:'''
{{{
AUTH_USER_MODEL = 'myapp.VulnerableUser'
}}}

3. '''Run the system check:'''
{{{
python manage.py check
}}}

=== Expected Behavior

The `check` command should detect the improper override and report errors
`auth.C009` and `auth.C010`:
{{{
System check identified 2 issues:
CRITICAL: myapp.VulnerableUser.is_anonymous must be an attribute or
property... (auth.C009)
CRITICAL: myapp.VulnerableUser.is_authenticated must be an attribute or
property... (auth.C010)
}}}

=== Actual Behavior

The `check` command fails to detect the `staticmethod` and reports no
issues:
{{{
System check identified no issues (0 silenced).
}}}

=== Proposed Patch

The vulnerability in the check lies in `django/contrib/auth/checks.py`.

The current check incorrectly uses `isinstance(..., MethodType)`, which
fails to detect `staticmethod` overrides as they are resolved as type
`function`.

The check should be modified to use `callable()` instead. `AnonymousUser`
(the correct implementation) uses `@property`, which results in a `bool`
value, and `callable(False)` is `False`. The vulnerable `staticmethod`
override results in a `function` object, and `callable(<function>)` is
`True`.

'''File:''' `django/contrib/auth/checks.py`

'''Current (flawed) code:'''
{{{#!python
if isinstance(cls().is_anonymous, MethodType):
errors.append(
checks.Critical(
"%s.is_anonymous must be an attribute or property rather
than "
"a method. Ignoring this is a security issue as anonymous
"
"users will be treated as authenticated!" % cls,
obj=cls,
id="auth.C009",
)
)
if isinstance(cls().is_authenticated, MethodType):
errors.append(
checks.Critical(
"%s.is_authenticated must be an attribute or property
rather "
"than a method. Ignoring this is a security issue as
anonymous "
"users will be treated as authenticated!" % cls,
obj=cls,
id="auth.C010",
)
)
return errors
}}}

'''Suggested patch:'''
{{{#!python
if callable(cls().is_anonymous): # MODIFIED
errors.append(
checks.Critical(
"%s.is_anonymous must be an attribute or property rather
than "
"a method. Ignoring this is a security issue as anonymous
"
"users will be treated as authenticated!" % cls,
obj=cls,
id="auth.C009",
)
)
if callable(cls().is_authenticated): # MODIFIED
errors.append(
checks.Critical(
"%s.is_authenticated must be an attribute or property
rather "
"than a method. Ignoring this is a security issue as
anonymous "
"users will be treated as authenticated!" % cls,
obj=cls,
id="auth.C010",
)
)
return errors
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36709>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36709: Bug Report: `manage.py check` Fails to Detect `@staticmethod` Override for
`is_authenticated` (auth.C010) and 'is_anonymous' (auth.C009)

------------------------------+--------------------------------------
Reporter: stan shaw | Owner: Harsh Jain
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0

------------------------------+--------------------------------------
Changes (by Harsh Jain):

* has_patch: 0 => 1
* owner: (none) => Harsh Jain
* stage: Unreviewed => Accepted
* status: new => assigned

Comment:

Thanks for reporting this bug, you’re correct; that appears to be the
case. I’ve raised a pull request to fix the issue by applying the
suggested patch and adding a test to detect `@staticmethod` overrides of
`is_authenticated` and `is_anonymous`.
[https://github.com/django/django/pull/20061 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/36709#comment:1>

[Django]

#36709: Bug Report: `manage.py check` Fails to Detect `@staticmethod` Override for
`is_authenticated` (auth.C010) and 'is_anonymous' (auth.C009)

-------------------------------------+-------------------------------------

Reporter: stan shaw | Owner: Harsh
| Jain
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:

Keywords: | Triage Stage: Ready for
| checkin

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

-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* stage: Accepted => Ready for checkin

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