Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#34965 closed Bug (needsinfo)

@sensitive_variables for coroutine func are not recursive

Reported by: Vageeshan Mankala Owned by: vageeshan
Component: Core (Other) Version: 5.0
Severity: Normal Keywords: @sensitive_variables, @sensitive_post_parameters
Cc: Jon Janzen, Carlton Gibson Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There is a difference in functionality of how @sensitive_variables/sensitive_post_parameters work for synchronous functions and asynchronous functions.

Sync funcs. - It recursively hides the variables from all frames in the stack until new sensitive variables are defined for a frame. Example, Wrappers to nested function calls, variables are hidden.
Async funcs. - It only hides the variables in the top most frame of the stack. Example, If there is view func with sensitive variables, and it also has a decorator, it hides only in the wrapper and not in the actual view.

I would expect both to work in similar way. I am also deeply invested in the idea so I willing to contribute a PR.

Change History (12)

comment:1 by Vageeshan Mankala, 12 months ago

Owner: changed from nobody to vageeshan
Status: newassigned

comment:2 by Vageeshan Mankala, 12 months ago

Cc: Vageeshan Mankala added

comment:3 by Vageeshan Mankala, 12 months ago

Cc: Vageeshan Mankala removed
Type: UncategorizedBug

comment:4 by Vageeshan Mankala, 12 months ago

Patch needs improvement: set

comment:5 by Mariusz Felisiak, 12 months ago

Cc: Jon Janzen added
Component: UncategorizedCore (Other)
Patch needs improvement: unset
Version 0, edited 12 months ago by Mariusz Felisiak (next)

comment:6 by Mariusz Felisiak, 12 months ago

Resolution: needsinfo
Status: assignedclosed

Thanks for the report, I don't think you've explained the issue in enough detail to confirm a bug in Django. Please reopen the ticket if you can debug your issue and provide a sample project that reproduces the issue. Also, be aware that sync_to_async() and async_to_sync() are not compatible with @sensitive_variables (as documented).

comment:7 by Jon Janzen, 12 months ago

Also, be aware that sync_to_async() and async_to_sync() are not compatible with @sensitive_variables (as ​documented).

We might want to update those docs, as recent versions (>= 3.7.0) will hide variables from the internals of asgiref: https://github.com/django/asgiref/pull/383

Changelog note for asgiref: https://github.com/django/asgiref/blob/main/CHANGELOG.txt#L25

in reply to:  7 comment:8 by Mariusz Felisiak, 12 months ago

Replying to Jon Janzen:

Also, be aware that sync_to_async() and async_to_sync() are not compatible with @sensitive_variables (as ​documented).

We might want to update those docs, as recent versions (>= 3.7.0) will hide variables from the internals of asgiref: https://github.com/django/asgiref/pull/383

Changelog note for asgiref: https://github.com/django/asgiref/blob/main/CHANGELOG.txt#L25

Django 5.0+ required asgiref 3.7+. Do you think it's time to remove this warning?

comment:9 by Jon Janzen, 12 months ago

Django 5.0+ required asgiref 3.7+. Do you think it's time to remove this warning?

Yeah that's probably a good idea, I completely missed that you added this warning

comment:10 by Mariusz Felisiak, 12 months ago

Cc: Carlton Gibson added

comment:11 by Carlton Gibson, 12 months ago

Thanks for the ping. Yes, with the change to asgiref, it seems reasonable to drop the warnings. (I didn't check the internal Python frames again, but they're future related, and don't feature sensitive Django-related variables…)

in reply to:  11 comment:12 by Mariusz Felisiak, 12 months ago

Replying to Carlton Gibson:

Thanks for the ping. Yes, with the change to asgiref, it seems reasonable to drop the warnings. (I didn't check the internal Python frames again, but they're future related, and don't feature sensitive Django-related variables…)

PR

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