-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
Refs #31949 -- Enable @sensitive_variables to work with async functions #16668
Refs #31949 -- Enable @sensitive_variables to work with async functions #16668
Conversation
c8c999d
to
366839a
Compare
django/views/decorators/debug.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bigfootjon
I think we should be able to do this without the big if...else...
duplication by using markcoroutinefunction
:
from asgiref.sync import iscoroutinefunction, markcoroutinefunction
def decorator(func):
@wraps(func)
def sensitive_variables_wrapper(*func_args, **func_kwargs):
if variables:
sensitive_variables_wrapper.sensitive_variables = variables
else:
sensitive_variables_wrapper.sensitive_variables = "__ALL__"
return func(*func_args, **func_kwargs)
if iscoroutinefunction(func):
markcoroutinefunction(sensitive_variables_wrapper)
return sensitive_variables_wrapper
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, that doesn't appear to work. I tried it and it fails the unit test I wrote (view_tests.tests.test_debug.ExceptionReporterFilterTests.test_async_sensitive_request
, fails that test on line 1567, line 1390 is where the exception is triggered).
I think @wraps
handles async-marking for us, I reverted all our changes and inserted this print statement instead of the if iscoroutinefunction(func):
block you suggested and it prints either True True
or False False
for every test case:
print(iscoroutinefunction(func), iscoroutinefunction(sensitive_variables_wrapper))
I've tested everything I can think of, and the only thing that works is conditional definition of the wrapper as async
or not, which doesn't feel right. I think I'm missing something obvious, but after sleeping on it I can't see what it is.
If the only concern you have is code duplication: I think I can reduce duplication by pulling the if variables:
block out and putting it in a helper function, but I'm not sure whether or not that's worth it.
(for what it's worth, I really just copied this idea from there: https://github.com/django/django/blob/main/django/test/utils.py#L425-L445)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's super interesting. Using markcoroutinefunction
works fine except for the traceback inspection in get_traceback_frame_variables()
— that's black magic already. Let's leave is as it is for now 👍
I think @wraps handles async-marking for us...
It doesn't. See python/cpython#100317.
... it prints either True True or False False
Assuming my suggestion, but before applying markcoroutinefunction
I get True, False
in just the cases in question. (But short of sitting down together to look at it, it doesn't matter now. :)
If the only concern you have is code duplication: I think I can reduce duplication by pulling the if variables: block out and putting it in a helper function, but I'm not sure whether or not that's worth it.
No not worth it.
(for what it's worth, I really just copied this idea from there: https://github.com/django/django/blob/main/django/test/utils.py#L425-L445)
Sure. It's not wrong... When we can I'd like to define just the one wrapper, since if we do that consistently it'll add up over the code-base, but here, just defining two is fine.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks @bigfootjon — let's have this.
django/views/decorators/debug.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's super interesting. Using markcoroutinefunction
works fine except for the traceback inspection in get_traceback_frame_variables()
— that's black magic already. Let's leave is as it is for now 👍
I think @wraps handles async-marking for us...
It doesn't. See python/cpython#100317.
... it prints either True True or False False
Assuming my suggestion, but before applying markcoroutinefunction
I get True, False
in just the cases in question. (But short of sitting down together to look at it, it doesn't matter now. :)
If the only concern you have is code duplication: I think I can reduce duplication by pulling the if variables: block out and putting it in a helper function, but I'm not sure whether or not that's worth it.
No not worth it.
(for what it's worth, I really just copied this idea from there: https://github.com/django/django/blob/main/django/test/utils.py#L425-L445)
Sure. It's not wrong... When we can I'd like to define just the one wrapper, since if we do that consistently it'll add up over the code-base, but here, just defining two is fine.
👍
fcfee67
to
c9fca64
Compare
I'm going to revert this patch as any acceptable solution should support nested calls: diff --git a/tests/view_tests/tests/test_debug.py b/tests/view_tests/tests/test_debug.py
index d0bcc68032..36ef716bed 100644
--- a/tests/view_tests/tests/test_debug.py
+++ b/tests/view_tests/tests/test_debug.py
@@ -42,6 +42,7 @@ from django.views.decorators.debug import sensitive_post_parameters, sensitive_v
from ..views import (
async_sensitive_view,
+ async_sensitive_view_nested,
custom_exception_reporter_filter_view,
index_page,
multivalue_dict_key_error,
@@ -1567,6 +1568,15 @@ class ExceptionReporterFilterTests(
self.verify_safe_response(async_sensitive_view)
self.verify_safe_email(async_sensitive_view)
+ def test_async_sensitive_nested_request(self):
+ with self.settings(DEBUG=True):
+ self.verify_unsafe_response(async_sensitive_view_nested)
+ self.verify_unsafe_email(async_sensitive_view_nested)
+
+ with self.settings(DEBUG=False):
+ self.verify_safe_response(async_sensitive_view_nested)
+ self.verify_safe_email(async_sensitive_view_nested)
+
def test_paranoid_request(self):
"""
No POST parameters and frame variables can be seen in the
@@ -1925,6 +1935,19 @@ class NonHTMLResponseExceptionReporterFilter(
with self.settings(DEBUG=False):
self.verify_safe_response(async_sensitive_view, check_for_vars=False)
+ def test_async_sensitive_request_nested(self):
+ """
+ Sensitive POST parameters cannot be seen in the default
+ error reports for sensitive requests.
+ """
+ with self.settings(DEBUG=True):
+ self.verify_unsafe_response(
+ async_sensitive_view_nested, check_for_vars=False
+ )
+
+ with self.settings(DEBUG=False):
+ self.verify_safe_response(async_sensitive_view_nested, check_for_vars=False)
+
def test_paranoid_request(self):
"""
No POST parameters can be seen in the default error reports
diff --git a/tests/view_tests/views.py b/tests/view_tests/views.py
index 97febdaf83..21f0db79ff 100644
--- a/tests/view_tests/views.py
+++ b/tests/view_tests/views.py
@@ -196,6 +196,28 @@ async def async_sensitive_view(request):
return technical_500_response(request, *exc_info)
+@sensitive_variables("sauce")
+@sensitive_post_parameters("bacon-key", "sausage-key")
+async def async_sensitive_function(request):
+ # Do not just use plain strings for the variables' values in the code
+ # so that the tests don't return false positives when the function's source
+ # is displayed in the exception report.
+ cooked_eggs = "".join(["s", "c", "r", "a", "m", "b", "l", "e", "d"]) # NOQA
+ sauce = "".join( # NOQA
+ ["w", "o", "r", "c", "e", "s", "t", "e", "r", "s", "h", "i", "r", "e"]
+ )
+ raise Exception
+
+
+async def async_sensitive_view_nested(request):
+ try:
+ await async_sensitive_function(request)
+ except Exception:
+ exc_info = sys.exc_info()
+ send_log(request, exc_info)
+ return technical_500_response(request, *exc_info)
+
+
@sensitive_variables()
@sensitive_post_parameters()
def paranoid_view(request): Thanks @bigfootjon for the regression test. |
Ref this comment: #16636 (comment)
Basically, right now
@sensitive_variables
and@sensitive_post_parameters
don't work withasync
functions. They need to for the above PR to be accepted, and this seems like it is generally useful