Opened 3 years ago

Closed 2 years ago

Last modified 22 months ago

#18923 closed Bug (fixed)

user admin sensitive_post_parameters needs method_decorator

Reported by: zbohm Owned by: nobody
Component: contrib.auth Version: master
Severity: Normal Keywords:
Cc: cmawebsite@…, timograham@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Decorator sensitive_post_parameters does not work with class method and method_decorator(sensitive_post_parameters) also does not help.

contrib/auth/admin.py:

@sensitive_post_parameters()
def user_change_password(self, request, id, form_url=''):
    if not self.has_change_permission(request):
        raise PermissionDenied
    user = get_object_or_404(self.queryset(request), pk=id)
    if request.method == 'POST':
        raise Exception("Show me the debug page.")

do not hide POST parameters on the output:

Variable 	Value
password_form 	u'1'
csrfmiddlewaretoken 	u'BC3JzQcQAUlhnEGKwykvJfNfRCN28NA2'
old_password 	u'oldpassword'
new_password1 	u'newpassword'
new_password2 	u'newpassword'

Attachments (3)

Change History (13)

comment:1 Changed 3 years ago by zbohm

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by zbohm

  • Resolution set to invalid
  • Status changed from new to closed

It is possible to use method_decorator. I have used it the wrong way in the previous case.

  • tests/regressiontests/views/views.py

    diff --git a/tests/regressiontests/views/views.py b/tests/regressiontests/views/views.py
    index 17872ee..4e99d12 100644
    a b from django.views.debug import technical_500_response, SafeExceptionReporterFilt 
    1111from django.views.decorators.debug import (sensitive_post_parameters,
    1212                                           sensitive_variables)
    1313from django.utils.log import getLogger
     14from django.utils.decorators import method_decorator
    1415
    1516from . import BrokenException, except_args
    1617
    def custom_exception_reporter_filter_view(request): 
    212213
    213214class Klass(object):
    214215
    215     @sensitive_variables('sauce')
     216    @method_decorator(sensitive_variables('sauce'))
    216217    def method(self, request):
    217218        # Do not just use plain strings for the variables' values in the code
    218219        # so that the tests don't return false positives when the function's
    class Klass(object): 
    226227            send_log(request, exc_info)
    227228            return technical_500_response(request, *exc_info)
    228229
    229     @sensitive_post_parameters("password", "secret-key")
     230    @method_decorator(sensitive_post_parameters("password", "secret-key"))
    230231    def method_post(self, request):
    231232        request.method = 'POST'
    232233        request.POST._mutable = True

comment:3 Changed 2 years ago by CollinAnderson

  • Cc cmawebsite@… added
  • Easy pickings set
  • Has patch unset
  • Resolution invalid deleted
  • Status changed from closed to new
  • Summary changed from Decorator sensitive_post_parameters does not work with class method to user admin sensitive_post_parameters needs method_decorator
  • Version changed from 1.4 to master

You are correct that sensitive_post_parameters does not work with class methods and you did correctly conclude that method_decorator(sensitive_post_parameters) works correctly. But you also correctly noticed that this is not working for user admin.

https://github.com/django/django/blob/master/django/contrib/auth/admin.py.
We need to change these two cases from @sensitive_post_parameters() to @method_decorator(sensitive_post_parameters())

Changed 2 years ago by timo

comment:4 Changed 2 years ago by timo

  • Cc timograham@… added
  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

Is it ok to add an assertion to detect misuse of the decorator (see patch)?

comment:5 Changed 2 years ago by CollinAnderson

would hasattr(request, 'POST') be better than isinstance?

comment:6 Changed 2 years ago by timo

That wouldn't work -- for one thing, it would prevent GET requests. It's also less specific than the isinstance check. A view always (as far as I can think) receives an HttpRequest as the first argument. In the case where method_decorator isn't used, sensitive_post_parameters receives the class of the method that's being decorated which would cause the assertion to fail.

comment:7 Changed 2 years ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 425d076d0c8cf7376a1478d118c58bcff5b1b74d:

Fixed #18923 -- Corrected usage of sensitive_post_parameters in contrib.auth

Thanks Collin Anderson for the report.

comment:8 Changed 2 years ago by Tim Graham <timograham@…>

In 97254154ab43d7973fba09ccd7ab548866f83e03:

[1.6.x] Fixed #18923 -- Corrected usage of sensitive_post_parameters in contrib.auth

Thanks Collin Anderson for the report.

Backport of 425d076d0c from master

comment:9 Changed 22 months ago by Tim Graham <timograham@…>

In 61de57260b4b2bb54fe3c494d104b85e957c9be2:

[1.5.x] Fixed #18923 -- Corrected usage of sensitive_post_parameters in contrib.auth

Thanks Collin Anderson for the report.

Backport of 425d076d0c from master

comment:10 Changed 22 months ago by Tim Graham <timograham@…>

In 75d2bcda10f00366e6d847f2c90db3e772433e46:

Fixed #18923 -- Corrected usage of sensitive_post_parameters in contrib.auth

Thanks Collin Anderson for the report.

Backport of 425d076d0c from master

Note: See TracTickets for help on using tickets.
Back to Top