Opened 12 years ago

Closed 11 years ago

Last modified 11 years ago

#18923 closed Bug (fixed)

user admin sensitive_post_parameters needs method_decorator

Reported by: zbohm Owned by: nobody
Component: contrib.auth Version: dev
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)

0001-Fixed-18923-Add-decorator-sensitive_post_parameters_.patch (3.3 KB ) - added by zbohm 12 years ago.
0001-Ticket-18923-Add-test-for-ensitive_post_parameters.patch (8.1 KB ) - added by zbohm 12 years ago.
18923.diff (2.0 KB ) - added by Tim Graham 11 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 by zbohm, 12 years ago

Has patch: set

comment:2 by zbohm, 11 years ago

Resolution: invalid
Status: newclosed

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 by Collin Anderson, 11 years ago

Cc: cmawebsite@… added
Easy pickings: set
Has patch: unset
Resolution: invalid
Status: closednew
Summary: Decorator sensitive_post_parameters does not work with class methoduser admin sensitive_post_parameters needs method_decorator
Version: 1.4master

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())

by Tim Graham, 11 years ago

Attachment: 18923.diff added

comment:4 by Tim Graham, 11 years ago

Cc: timograham@… added
Has patch: set
Triage Stage: UnreviewedAccepted

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

comment:5 by Collin Anderson, 11 years ago

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

comment:6 by Tim Graham, 11 years ago

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 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 425d076d0c8cf7376a1478d118c58bcff5b1b74d:

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

Thanks Collin Anderson for the report.

comment:8 by Tim Graham <timograham@…>, 11 years ago

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 by Tim Graham <timograham@…>, 11 years ago

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 by Tim Graham <timograham@…>, 11 years ago

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